Re: [security] Race condition in udev

[Greg KH <greg@kroah.com>]

On Tue, Aug 25, 2009 at 08:11:37PM +0200, Florian Zumbiehl wrote:
> Hi,
> 
> > On Tue, Aug 25, 2009 at 07:31:30PM +0200, Florian Zumbiehl wrote:
> > > Assumption:
> > > 
> > >  /dev/foo is configured to be owned by user root, group users, mode 0646.
> > >  The attacker tries to open /dev/foo for writing as a user that's not
> > >  root, not a member of the group root, but a member of the group users.
> > > 
> > > The Trace:
> > > 
> > >   action                     | owner | group | mode    | open(O_WRONLY)?
> > >  ----------------------------+-------+-------+---------+-----------------
> > >   mknod(/dev/foo)            | root  | root  | 0644(?) | no
> > >   chmod(/dev/foo,0646)       | root  | root  | 0646    | yes
> > >   chown(/dev/foo,root,users) | root  | users | 0646    | no
> > 
> > Are there any current device nodes that get set to this kind of "odd"
> > permissions with the current udev ruleset?
> 
> None that I am aware of - I haven't really looked, though :-)
> 
> But I have encountered such "odd" permissions in the wild for other
> purposes - so, it's probably not a thing that has to be fixed in all
> distributions by yesterday morning, but as a matter of reliability,
> I think it still ought to be fixed.

And again, specifically how do you think this should be fixed?

And see Scott's responses as to why this type of configuration is
invalid.

> > > Could we now take care of the bug?
> > 
> > Do you have a proposed patch?
> 
> Nope, not yet - it was part of my intention behind this thread to gain
> some understanding of the code so as to possibly be able to provide a
> patch that doesn't break the code in some other way. In particular, as
> mentioned, the purpose of the special case codepath for a pre-existing
> device node isn't all that clear to me.
> 
> I guess the general ideas I have in mind are roughly:
> 
> a) mknod() it under some temporary name with appropriate euid/egid and
>    umask, then rename it into place.
> 
>    This one seems to have some issues that I can't really judge yet
>    (changing inode number, ACLs being dropped).

That will not work for the case of an existing device node, correct?

> b) (optionally mknod() with mode&0600), chmod() to mode&0600,
>    chown() to configured owner/group, chmod() to configured mode.
> 
>    This one potentially temporarily reduces permissions to a proper
>    subset of both the permissions before and after the change -
>    I guess that that's not desirable?

See Scott's response as to why this isn't ok.

thanks,

greg k-h