[PATCH xserver] check for elevated privileges not uid=0 (V2)

[Tormod Volden]

On Mon, Oct 10, 2011 at 6:43 PM, Antoine Martin wrote:
>> I have been using this patch for days without any visible side effects.
>> Can I please get some reviewed-by / acks?
>>

Any functional issues aside, your patch will need some style polishing
before it can get any rubber stamps:

> Subject: [PATCH xserver] check for elevated privileges not uid=0 (V3)

First, the V3 in the subject should be inside the []'s to not be
included in the commit message. Check other commits to see if there is
a common practice of capitalization or module or section prefix in
this line if you really want to please.

+static Bool privsTested = FALSE;
+static Bool privsElevated = FALSE;
+Bool xf86PrivsElevated(void)

You must have messed up from V2 to V3 because the variables ended
outside the function again. Note that in V2 there was missing a blank
line after the variable declarations.

+      uid_t ruid, euid, suid;
+      gid_t rgid, egid, sgid;
+      if ((getresuid(&ruid, &euid, &suid) == 0) &&

Missing blank line again.

+       */
+      unsigned int oldeuid;
+      oldeuid = geteuid();

And again.

+          (getresgid(&rgid, &egid, &sgid) == 0))
+        privsElevated = (euid != suid) || (egid != sgid);
+#else
+      /*
+       * If there are saved ID's the process might still be priviledged

privileged

+       * even though the above test succeeded.  If issetugid() and

I also consider double space after period a typographical mistake, but
I am not gonna run a crusade here :)

+        if (seteuid(0) != 0) {
+          privsElevated = FALSE;

You use != 0 to check for failure here...

+        } else {
+          if (seteuid(oldeuid) == -1) {

... but == -1 here. Is there any reason for not being consistent?

+            /* XXX ouch, coudn't get back to original uid
+               what can we do ??? */

If we get to this point, the code in the patch has failed. It should
apologize deeply and offer your phone number for technical support :)
Seriously, it should print an "internal error" message of some kind,
maybe including __function__. Just quitting silently and leave a
cryptic 127 that might or not propagate to the user does not cut it.

+            _exit(127);

Why do you use _exit() and not exit() here? This is not e.g. a forked
child that should escape normal clean-up?

Best regards,
Tormod