Bug 12625 (CVE-2011-1089)

Summary: mntent operations provide no indication of failure due to RLIMIT_FSIZE (CVE-2011-1089)
Product: glibc Reporter: Dan Rosenberg <dan.j.rosenberg>
Component: libcAssignee: Ulrich Drepper <drepper.fsp>
Status: RESOLVED FIXED    
Severity: normal CC: fweimer, ludwig.nussel, pasky, thoger
Priority: P2 Flags: fweimer: security+
Version: unspecified   
Target Milestone: ---   
Host: Target:
Build: Last reconfirmed:
Attachments: Simple test case

Description Dan Rosenberg 2011-03-30 19:19:10 UTC 
See thread on oss-security mailing list [1] for additional reference.  This issue has been assigned CVE-2011-1089.

Essentially every setuid mount helper, including util-linux mount, fails to handle a low RLIMIT_FSIZE.  Since addmntent() uses fprintf() without a corresponding fflush(), it will return success even in the case when the actual write will fail due to the resource limit.  And then endmntent() always returns 1, so that's no help.  As a result, these helpers can be used to write corrupted entries into /etc/mtab.

Rather than forcing every setuid mount helper to explicitly alter resource limits prior to calling addmntent(), etc., it would be better if addmntent() attempted to flush and return failure based on the success of the fflush(), as suggested by Tomas Hoger:

 if (fprintf (stream, "%s %s %s %s %d %d\n", ...) < 0)
   return 1;

 return (fflush(stream) == 0 ? 0 : 1);


[1] http://www.openwall.com/lists/oss-security/2011/03/04/9
[2] http://www.openwall.com/lists/oss-security/2011/03/07/9
Comment 1 Tomas Hoger 2011-03-31 09:31:29 UTC 
Created attachment 5346 [details]
Simple test case

gmane thread view may offer a better overview over the discussion Dan pointed out:
http://thread.gmane.org/gmane.comp.security.oss.general/4374
Comment 2 Ulrich Drepper 2011-04-18 01:05:13 UTC 
(In reply to comment #0)
> Rather than forcing every setuid mount helper to explicitly alter resource
> limits prior to calling addmntent(), etc., it would be better if addmntent()
> attempted to flush and return failure based on the success of the fflush(), as
> suggested by Tomas Hoger:

That's completely bogus.  Applications which cannot handle problems introduced like this also won't check the return value of addmntent.  Just fix the damned programs which doesn't use the existing interface correctly.
Comment 3 Tomas Hoger 2011-04-18 13:44:08 UTC 
(In reply to comment #2)
> (In reply to comment #0)
> > Rather than forcing every setuid mount helper to explicitly alter resource
> > limits prior to calling addmntent(), etc., it would be better if addmntent()
> > attempted to flush and return failure based on the success of the fflush(), as
> > suggested by Tomas Hoger:
> 
> That's completely bogus.  Applications which cannot handle problems introduced
> like this also won't check the return value of addmntent.

This bug is not about fixing apps that don't check addmntent return value via some glibc-side magic, it is about fixing addmntent to not return success when mtab update failed (or is going to fail when the buffer is flushed in endmntent / fclose), so the applications that do check return value can detect the error.

> Just fix the damned programs which doesn't use the existing interface correctly.

Can you clarify what exactly you refer to as incorrect use of existing interface?  Even if you check addmntent return value, you can't rely on it.  endmntent always returns the same value, hence no check is possible there.
Comment 4 Petr Baudis 2011-04-19 13:19:14 UTC 
Reopening per previous comment; even applications that properly use the existing interface (I assume you mean that they check addmntent() return value) wouldn't detect the mtab corruption.
Comment 5 Ulrich Drepper 2011-05-12 03:39:48 UTC 
The only reason I made a change is because some braindead idiot put in the man page that endmntent never returns anything but 1.  This is not what the libc implementation says and it should have been the correct way to report this type of problem.
Comment 6 Tomas Hoger 2011-05-12 10:11:19 UTC 
Commit link noted for posterity:
http://sourceware.org/git/?p=glibc.git;a=commitdiff;h=e1fb097f44

(In reply to comment #5)
> The only reason I made a change is because some braindead idiot put in the man
> page that endmntent never returns anything but 1.  This is not what the libc
> implementation says and it should have been the correct way to report this type
> of problem.

Yes, it would be cleaner to have endmntent return error when fclose fails, but given the documentation, it's not easy to change that way.  The man page seems to correctly document the implementation:

http://sourceware.org/git/?p=glibc.git;a=blob;f=misc/mntent_r.c;h=6959f0e2#l58
Comment 7 Ulrich Drepper 2011-05-12 20:10:08 UTC 
(In reply to comment #6)
> The man page seems
> to correctly document the implementation:

That's no excuse.  Implementations change.  The glibc documentation always documented it correctly.  The person writing that text should be held accountable for the crap s/he did.
Comment 8 jsm-csl@polyomino.org.uk 2014-06-13 11:23:51 UTC 
On Fri, 13 Jun 2014, fweimer at redhat dot com wrote:

> Florian Weimer <fweimer at redhat dot com> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>               Flags|                            |security-

Are you sure?  This has a CVE....
Comment 9 Florian Weimer 2014-06-13 11:39:31 UTC 
Fixed in this commit: http://sourceware.org/git/?p=glibc.git;a=commitdiff;h=e1fb097f447a89