Question about a recent change to uidswap.c in the portability snapshot

[Darren Tucker]

David Holmes wrote:
> A change was recently introduced into uidswap.c to cover the case where
> the user is root.  The change is "&& pw->pw_uid != 0 &&".
>  
>         /* Try restoration of GID if changed (test clearing of saved
> gid) */
>         if (old_gid != pw->pw_gid && pw->pw_uid != 0 &&
>             (setgid(old_gid) != -1 || setegid(old_gid) != -1))
>                 fatal("%s: was able to restore old [e]gid", __func__);
> 
> My question is, should this change also be included in the setuid() call
> a few lines later?
>  
>         /* Try restoration of UID if changed (test clearing of saved
> uid) */
>         if (old_uid != pw->pw_uid &&   [should change be here also?]
>             (setuid(old_uid) != -1 || seteuid(old_uid) != -1))
>                 fatal("%s: was able to restore old [e]uid", __func__);

I don't think it's necessary.

The gid test was put in to handle the case where the root user is 
running as their non-default group (ie they've run "newgrp").  In that 
case, because root can set its uid to whatever it wants, the group 
restore test will fail.  For the second test, old_uid and pw_uid will be 
the same so the second test won't fail either.

-- 
Darren Tucker (dtucker at zip.com.au)
GPG key 8FF4FA69 / D9A3 86E9 7EEE AF4B B2D4  37C9 C982 80C7 8FF4 FA69
     Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.