Re: [PATCH] Declare set*id with warn_unused_result

[Paul Eggert <eggert at cs dot ucla dot edu>]

On 07/24/2012 06:42 AM, Florian Weimer wrote:

> It could (and probably should) call abort because something has gone
> horribly wrong.

Library functions that call abort() can cause further problems.
The application may be in the middle of a delicate operation, which
presumably is security-relevant, and aborting may itself cause further
security issues.

> I don't see how encouraging the programmer to add checks could make
> things any worse.

It could make things worse by diverting scarce developer resources
into risky and unnecessary work.

> even current kernels code can return ENOMEM, it is just harder to hit.

?!?  Worse and worse.  Where is this documented?
It sounds like another kernel bug....


>> If this proposed change were installed, it's
>> plausible that an application developer wouldn't notice this
>> sort of issue, and would introduce further security bugs
>> in an attempt to pacify GCC.
>
> Which issue?

The issue that a program often cannot do anything reasonable if the
system call fails.  Here's an example abstracted from Gnulib: it's a
library function that does something like this:

   int
   dosomething (void)
   {
     uid_t euid = geteuid ();
     int result;
     seteuid (getuid ());
     result = something ();
     seteuid (euid);
     return result;
   }

Suppose the proposed patch is used.  To pacify GCC, the developer is
likely to change the code to something like this:

   int
   dosomething (void)
   {
     uid_t euid = geteuid ();
     int result;
     if (seteuid (getuid ()) != 0)
       return -1;
     result = something ();
     if (seteuid (euid) != 0)
       return -1;
     return result;
   }

This is plausibly correct, but is wrong: even though GCC has been
pacified, the bug is still there (just in a different way).
Worse, it's plausible the developer will change it to something
like this:

   int
   dosomething (void)
   {
     uid_t euid = geteuid ();
     int result = seteuid (getuid ());
     if (result == 0)
       {
	 result = something ();
	 if (result == 0)
	   result = seteuid (euid);
       }
     return result;
   }

This introduces security holes even atop a fixed kernel, all
in the cause of pacifying GCC.

In this particular case, can the unmodified Gnulib code fail
due to seteuid returning -1 even in fixed kernels?  If so, surely
more kernel fixes are needed.  If not, the unmodified Gnulib code
is an example of useful code for which the proposed patch would
generate false alarms.