>Number:         30188
>Category:       bin
>Synopsis:       cp copies permissions of setuid source to existing destination without -p
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    bin-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue May 10 22:36:00 +0000 2005
>Originator:     Jed Davis
>Release:        NetBSD 2.0.2
>Organization:
Public Access Networks Corp.
>Environment:
System: NetBSD byzantium.nyc.access.net 2.0.2 NetBSD 2.0.2 (PANIX-STAFF) #0: Thu May  5 21:13:35 EDT 2005  root@juggler.panix.com:/devel/netbsd/2.0.2/src/sys/arch/i386/compile/PANIX-STAFF i386
Architecture: i386
Machine: i386
>Description:

See the how-to-repeat section for an example.  The code responsible is
in src/bin/cp/utils.c, lines 184-200 in r1.27; the purpose of that being
apparently to handle setuid bits safely.  However, it seems to me that,
if pflag==0, then there's no point to any of this and it should all
be skipped --- rather than skipping only the setfile() and proceeding
into the tests for the source's having been set[ug]id and the ensuing
fchmod().  Christos added, on tech-userlevel:

} cp without -p when executed by root is supposed to copy the
} permissions when the file is being created, but not when the file
} already exists. At least this is what solaris and linux do.


>How-To-Repeat:

  hostname# ls -l /dev/null                       
  crw-rw-rw-  1 root  wheel  2, 2 May  9 18:33 /dev/null
  hostname# cp /usr/libexec/ssh-keysign /dev/null
  hostname# ls -l /dev/null                      
  cr-sr-xr-x  1 root  wheel  2, 2 May  9 23:25 /dev/null

>Fix:

Changing line 184 to something like 

	if (pflag || (dne && my_uid == 0)) {
		if (setfile(fs, to_fd))

and then closing that on what will be line 202 (and fixing the
indentation &c) seems like it'd do what Christos speaks of, while also
preventing the setuid-/dev/null problem.