exec: Add credential change information into imgp for process_exec hook.
ClosedPublic
Actions

Authored by bdrewery on May 24 2016, 9:03 PM.

Details

Reviewers

kib
mjg

Commits

rS300792: exec: Add credential change information into imgp for process_exec hook.

Summary

This allows an EVENTHANDLER(process_exec) hook to see if the new image
will cause credentials to change whether due to setgid/setuid or because
of POSIX saved-id semantics.

This adds 3 new fields into image_params:

struct ucred *newcred		Non-null if the credentials will change.
bool credential_setid		True if the new image is setuid or setgid.
int credential_changing	1 if the credentials are changing.

This will pre-determine the new credentials before invoking the image
activators, where the process_exec hook is called. The new credentials
will be installed into the process in the same place as before, after
image activators are done handling the image.

MFC after: 2 weeks
Sponsored by: EMC / Isilon Storage Division

Test Plan

Tested a boot with running setuid binaries.

# su - bryan
You can make a log of your terminal session with script(1).

## setuid script but not interp
$ cat test.sh
#! /bin/sh
id
$ ls -al test.sh
-rwsr-xr-x  1 root  bryan  22 May 26 10:03 test.sh*
$ ./test.sh
uid=1001(bryan) gid=1001(bryan) groups=1001(bryan)


## setuid interp
$ cat test2.sh
#! /home/bryan/sh
id
$ ls -al test2.sh
-rwxr-xr-x  1 root  bryan  22 May 26 17:04 test2.sh
$ ./test2.sh
uid=1001(bryan) gid=1001(bryan) euid=0(root) groups=1001(bryan)

## non-setuid binary
$ /bin/sh -c id
uid=1001(bryan) gid=1001(bryan) groups=1001(bryan)

## setuid binary
$ ls -al sh
-r-sr-xr-x  1 root  bryan  151400 May 26 17:02 sh
$ ./sh -c id
uid=1001(bryan) gid=1001(bryan) euid=0(root) groups=1001(bryan)

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bdrewery updated this revision to Diff 16818.May 24 2016, 9:03 PM

bdrewery retitled this revision from to exec: Add credential change information into imgp for process_exec hook..

bdrewery updated this object.

bdrewery edited the test plan for this revision. (Show Details)

bdrewery added reviewers: kib, mjg.

Herald added a subscriber: imp. · View Herald TranscriptMay 24 2016, 9:03 PM

bdrewery edited the test plan for this revision. (Show Details)May 24 2016, 9:05 PM

bdrewery added inline comments.

sys/kern/kern_exec.c
942 ↗	(On Diff #16818)	Technically the 'oldcred != NULL' check here is not needed since nothing will goto this area before it is set, however I feel it is safer in case of later refactoring.
sys/sys/imgact.h
89 ↗	(On Diff #16818)	credential_changing is redundant with newcred since newcred is only set if it is changing. I moved it here per request but feel it is not needed. Plus I left it as an int (it's really only 0 or 1) instead of bool because of bit \|= operations that I wasn't sure about style of implicit casts on. I'm also unsure on proper sorting here.

bdrewery added a child revision: D6545: filemon exec: Cease tracing if credentials will change with the new image..May 24 2016, 9:09 PM

bdrewery added inline comments.May 24 2016, 9:14 PM

sys/kern/kern_exec.c
528–534 ↗	(On Diff #16818)	The unlock/lock window, even with VV_TEXT, does allow chown/chmod. However, the current version already has this problem since the image activators drop and require the exclusive lock in quite a few places as well. We execute/change cred (if setuid) to the permissions/ownership it was at the exec_check_permissions() lines above (which calls VOP_GETATTR). If someone does chmod/chown the binary after that initial check it will not impact what user is setuid to since we're using the one we already looked up. So I think there is no problem with the windows open here.

bdrewery added a child revision: D6546: exec: Provide execpath in imgp for the process_exec hook..May 24 2016, 9:32 PM

kib added inline comments.May 26 2016, 5:52 AM

sys/kern/kern_exec.c
803 ↗	(On Diff #16818)	I think that these two lines can be moved out of the 'else' branch. Then, proc_set_cred() can be removed from the 'then' branch.
942 ↗	(On Diff #16818)	If crfree() is called on NULL, nice trap is reported and the issue is semi-evident. I do not see a reason to check for NULL, in fact, since oldcred is never assigned NULL. This check is confusing.

It seems that the imgp->interpreted case is handled incorrectly. If the file supplied to execve(2) is shebang, then goto interpet would not clear newcred and credential_setid in the imgp. In fact, this results in setuid _working_ on shebang scripts, and ignoring setuid on the real binary. Both are wrong, if my reading of code is correct.

bdrewery added inline comments.May 26 2016, 3:55 PM

sys/kern/kern_exec.c
803 ↗	(On Diff #16818)	Ah, agreed!
942 ↗	(On Diff #16818)	Ah you're right. I swore it was assigned NULL in the declaration but I see now it is not.

bdrewery added inline comments.May 26 2016, 4:38 PM

sys/kern/kern_exec.c
789–792 ↗	(On Diff #16818)	In the current code, this transition is not in the non-setuid but saved-id changed cred else branch. Is that a bug? The will_transition variable is assigned from mac_vnode_execve_will_transition() in all cases. I don't really understand the saved-id stuff and how it is used when not setuid.

In D6544#138829, @kib wrote:

It seems that the imgp->interpreted case is handled incorrectly. If the file supplied to execve(2) is shebang, then goto interpet would not clear newcred and credential_setid in the imgp. In fact, this results in setuid _working_ on shebang scripts, and ignoring setuid on the real binary. Both are wrong, if my reading of code is correct.

I was always setting credential_set to false and credential_changing to 0, so the only missing part was newcred being cleaned. Because imgp is bzero'd I'll just move the false/0 to the interpret cleanup area too.

Remove bad oldcred != NULL check and unneeded oldcred != imgp->newcred
Use just 1 proc_set_cred call
Fix cleanup of imgp->newcred before goto interpret
Move zeroing of imgp->credential vars to the interpret cleanup since it was already bzero'd

bdrewery edited the test plan for this revision. (Show Details)May 26 2016, 5:16 PM

Why did you left credential_changing in the *imgp ?

I mentioned it in a comment on the struct. I would prefer to not have it since it is redundant but thought you wanted it. Do you agree with removing it?

Regards,
Bryan Drewery

In D6544#139142, @bdrewery wrote:

I mentioned it in a comment on the struct. I would prefer to not have it since it is redundant but thought you wanted it. Do you agree with removing it?

I need credential_setid, or even newcred != NULL. Is the later statement correct, and credential_setid is redundand ?

credential_changing is redundant with newcred != NULL. I will just remove credential_changing.

credential_setid is different since it only happens with setuid/setgid, the credential can also change in the non-setid case using the saved ids.

Regards,
Bryan Drewery

Actually credential_changing should be named more like credential_want_setid. It is not set in the saved-id non-setid case but it is if the image is setid but other restrictions could prevent actually using setid.

It's still useless so I'll remove.

Regards,
Bryan Drewery

bdrewery added inline comments.May 26 2016, 8:07 PM

sys/kern/kern_exec.c
853 ↗	(On Diff #16923)	I believe there is an existing bug here. This really should be newcred != NULL with the new code. As noted in another comment, credential_changing is 1 if the image is setuid/setgid but it doesn't mean the credentials are changing because capability mode, or mount nosuid, or traced process, all will prevent setid from being used. Hwpmc will incorrectly think the credentials are changing though.