[security] Race condition in udev

[security] Race condition in udev

[Florian Zumbiehl]

Hi,

reading some of the source of udev, I noticed what I would suspect to be a
race condition with security implications, namely that device nodes
are first mknod()/chmod()ed with the permission mask that they're supposed
to have at the end, but potentially at this point applying to the
wrong owner and group, before then being chown()ed to the correct
owner and group.

Now, I don't understand why this preservation-stuff (existing device nodes
don't get replaced, but instead their permissions get modified) is being
done, which is why I don't have any patch - but if you help me with
that a bit, maybe I would make up some fix ;-)

Florian

Re: [security] Race condition in udev

[Kay Sievers]

On Fri, Aug 21, 2009 at 12:24, Florian Zumbiehl<florz@florz.de> wrote:
> reading some of the source of udev, I noticed what I would suspect to be a
> race condition with security implications, namely that device nodes
> are first mknod()/chmod()ed with the permission mask that they're supposed
> to have at the end, but potentially at this point applying to the
> wrong owner and group, before then being chown()ed to the correct
> owner and group.

The device node is owned by root, what's the problem here?

Thanks,
Kay

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> On Fri, Aug 21, 2009 at 12:24, Florian Zumbiehl<florz@florz.de> wrote:
> > reading some of the source of udev, I noticed what I would suspect to be a
> > race condition with security implications, namely that device nodes
> > are first mknod()/chmod()ed with the permission mask that they're supposed
> > to have at the end, but potentially at this point applying to the
> > wrong owner and group, before then being chown()ed to the correct
> > owner and group.
> 
> The device node is owned by root, what's the problem here?

at least after the (first) chown() it potentially isn't owned by root, so
your statement in that form is false. So: Which point in the process do
you refer to (and why do you think that it being owned by root at that
point prevents permissions from potentially being more permissive than
specified in the configuration)?

Florian

Re: [security] Race condition in udev

[Kay Sievers]

On Fri, Aug 21, 2009 at 13:25, Florian Zumbiehl<florz@florz.de> wrote:
>> On Fri, Aug 21, 2009 at 12:24, Florian Zumbiehl<florz@florz.de> wrote:
>> > reading some of the source of udev, I noticed what I would suspect to be a
>> > race condition with security implications, namely that device nodes
>> > are first mknod()/chmod()ed with the permission mask that they're supposed
>> > to have at the end, but potentially at this point applying to the
>> > wrong owner and group, before then being chown()ed to the correct
>> > owner and group.
>>
>> The device node is owned by root, what's the problem here?
>
> at least after the (first) chown() it potentially isn't owned by root, so
> your statement in that form is false.

The mknod() already happens with the configured mode, so after the
chown() we already have the configured permissions/ownership set.

Kay

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> >> The device node is owned by root, what's the problem here?
> >
> > at least after the (first) chown() it potentially isn't owned by root, so
> > your statement in that form is false.
> 
> The mknod() already happens with the configured mode, so after the
> chown() we already have the configured permissions/ownership set.

well, (a) there is this does-already-exist-so-let's-preserve-it
part, in which case no mknod() does happen and (b) yeah, that was
pretty much my point: The mknod() already happens with the configured
mode(!), but AFAICS _not_ with the configured owner/group(!?). As
the config to my understanding is not supposed to say "make this
read-only to _some_ group", but rather "make this read-only to
group x", it's incorrect to apply the permissions intended for
group x to some other group, just because it happens to be a group,
too. The same applies for the chmod() part in the "preserve"-codepath.

And yeah, after the chown(), everything should be fine (given that
nobody has created a filehandle up to that point that wouldn't be
allowed anymore after that point), which is why the subject of my
mail reads "race condition" ;-)

Florian

Re: [security] Race condition in udev

[Bryan Kadzban]

[-- Attachment #1: Type: text/plain, Size: 1063 bytes --]

Florian Zumbiehl wrote:
> Hi,
> 
>>>> The device node is owned by root, what's the problem here?
>>> at least after the (first) chown() it potentially isn't owned by root, so
>>> your statement in that form is false.
>> The mknod() already happens with the configured mode, so after the
>> chown() we already have the configured permissions/ownership set.
> 
> well, (a) there is this does-already-exist-so-let's-preserve-it
> part, in which case no mknod() does happen and (b) yeah, that was
> pretty much my point: The mknod() already happens with the configured
> mode(!), but AFAICS _not_ with the configured owner/group(!?).

No, it happens with owner/group root, since that's who udev runs as
(unless I'm missing something).  Do you not trust that owner/group for
some reason?  :-)

If you still don't trust root/root, how would you suggest fixing it,
given that there is no system call to set owner, group, and mode all at
once?  If the chown() / chmod() calls are reversed, then you have the
same problem in the other direction...


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> >>>> The device node is owned by root, what's the problem here?
> >>> at least after the (first) chown() it potentially isn't owned by root, so
> >>> your statement in that form is false.
> >> The mknod() already happens with the configured mode, so after the
> >> chown() we already have the configured permissions/ownership set.
> > 
> > well, (a) there is this does-already-exist-so-let's-preserve-it
> > part, in which case no mknod() does happen and (b) yeah, that was
> > pretty much my point: The mknod() already happens with the configured
> > mode(!), but AFAICS _not_ with the configured owner/group(!?).
> 
> No, it happens with owner/group root, since that's who udev runs as

Yeah. That applies to the mknod()-case only, though.

> (unless I'm missing something).  Do you not trust that owner/group for
> some reason?  :-)

Why does it matter for this problem whether I trust root:root?

Well, OK, if I _didn't_ trust them, maybe there would be some conflicts
with the usual way linux systems are set up, but assuming that I _do_
trust them, how does trusting them help me when I tell udev to disallow
group x (where x != root) from, say, writing to some device, but udev
then goes and disallows group root from writing instead?

> If you still don't trust root/root, how would you suggest fixing it,
> given that there is no system call to set owner, group, and mode all at

Actually, there is, it's called mknod().

> once?  If the chown() / chmod() calls are reversed, then you have the
> same problem in the other direction...

Well, not really knowing the design of udev, I would guess that using
mknod() to atomically create the node in exactly the intended state
would be the cleanest way of doing it.

Another secure alternative would be to at least never give _more_
permissions to anyone than what is specified in the configuration
by taking away all the permissions from everyone around the owner/group
change. The drawback to that variant is, of course, that there is some
time window where permissions potentially are more restricted that what's
specified in the configuration.

Florian

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> reading some of the source of udev, I noticed what I would suspect to be a
[...]

could someone possibly explain to me why there is that special codepath
for cases where the device node does already exist, so I can write a
patch that's not gonna break other functionality?

Florian

Re: [security] Race condition in udev

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 735 bytes --]

On Tue, 2009-08-25 at 13:32 +0200, Florian Zumbiehl wrote:

> > reading some of the source of udev, I noticed what I would suspect to be a
> [...]
> 
> could someone possibly explain to me why there is that special codepath
> for cases where the device node does already exist, so I can write a
> patch that's not gonna break other functionality?
> 
For example, when using devtmpfs; in which case the device nodes already
exist.

Or when updating devices like /dev/null which are created before udevd
is started by the init script when not using devtmpfs.

Or when racing with devmapper which creates /dev/mapper/foo devices at
basically the same time as udev.

Scott
-- 
Scott James Remnant
scott@canonical.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [security] Race condition in udev

[Kay Sievers]

On Tue, Aug 25, 2009 at 13:58, Scott James Remnant<scott@canonical.com> wrote:
> On Tue, 2009-08-25 at 13:32 +0200, Florian Zumbiehl wrote:
>
>> > reading some of the source of udev, I noticed what I would suspect to be a
>> [...]
>>
>> could someone possibly explain to me why there is that special codepath
>> for cases where the device node does already exist, so I can write a
>> patch that's not gonna break other functionality?
>>
> For example, when using devtmpfs; in which case the device nodes already
> exist.
>
> Or when updating devices like /dev/null which are created before udevd
> is started by the init script when not using devtmpfs.
>
> Or when racing with devmapper which creates /dev/mapper/foo devices at
> basically the same time as udev.

Or having events not reset already applied ACLs by setting mode/perms.
Or not changing the inode number of the node, and confuse some tools
for no good reason.

Kay

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > > reading some of the source of udev, I noticed what I would suspect to be a
> > [...]
> > 
> > could someone possibly explain to me why there is that special codepath
> > for cases where the device node does already exist, so I can write a
> > patch that's not gonna break other functionality?
> > 
> For example, when using devtmpfs; in which case the device nodes already
> exist.
> 
> Or when updating devices like /dev/null which are created before udevd
> is started by the init script when not using devtmpfs.

well, in those two cases always rename()ing the new node into place would
work, too!? That would be a different strategy than what's in
place at the moment, but it wouldn't need a special case!?

> Or when racing with devmapper which creates /dev/mapper/foo devices at
> basically the same time as udev.

Seriously? How is a piece of code that does the existence check and
the subsequent action depending on the result of that check non-atomically
supposed to help avoid some race condition resulting from possible
concurrent creation of a device node?!

Florian

Re: [security] Race condition in udev

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

On Tue, 2009-08-25 at 14:21 +0200, Florian Zumbiehl wrote:

> > > > reading some of the source of udev, I noticed what I would suspect to be a
> > > [...]
> > > 
> > > could someone possibly explain to me why there is that special codepath
> > > for cases where the device node does already exist, so I can write a
> > > patch that's not gonna break other functionality?
> > > 
> > For example, when using devtmpfs; in which case the device nodes already
> > exist.
> > 
> > Or when updating devices like /dev/null which are created before udevd
> > is started by the init script when not using devtmpfs.
> 
> well, in those two cases always rename()ing the new node into place would
> work, too!? That would be a different strategy than what's in
> place at the moment, but it wouldn't need a special case!?
> 
The rename() will fail.  But you still need to apply any necessary
ownership and mode changes.

> > Or when racing with devmapper which creates /dev/mapper/foo devices at
> > basically the same time as udev.
> 
> Seriously? How is a piece of code that does the existence check and
> the subsequent action depending on the result of that check non-atomically
> supposed to help avoid some race condition resulting from possible
> concurrent creation of a device node?!
> 
Read the code and find out.  It works.

Scott
-- 
Scott James Remnant
scott@canonical.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

[...]
> Or having events not reset already applied ACLs by setting mode/perms.

I'm not all that deep into the code or even the event model of udev, and
I haven't ever used ACLs with devices/udev, but it seems to me that in
order for the system to be easy to understand, either somehow a new set
of permission ought to be applied (which would mean that existing ACLs
should disappear, and only ACLs belonging to the new set of permissions
should be installed) or the current permissions should be retained (in
which case mode and ownership should be left as-is as well, not just the
ACLs)?!

> Or not changing the inode number of the node, and confuse some tools
> for no good reason.

That's not just a hypothetical problem?

After all, that would make it impossible to update permissions on a
device (as identified by its names) atomically, and thus would probably
mean some period of reduced permissions during each permission change.

Florian

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > well, in those two cases always rename()ing the new node into place would
> > work, too!? That would be a different strategy than what's in
> > place at the moment, but it wouldn't need a special case!?
> > 
> The rename() will fail.

Because?

> But you still need to apply any necessary
> ownership and mode changes.

Erm, yeah, of course!?!

> > > Or when racing with devmapper which creates /dev/mapper/foo devices at
> > > basically the same time as udev.
> > 
> > Seriously? How is a piece of code that does the existence check and
> > the subsequent action depending on the result of that check non-atomically
> > supposed to help avoid some race condition resulting from possible
> > concurrent creation of a device node?!
> > 
> Read the code and find out.  It works.

Guess how I found out that it can not work.

Florian

Re: [security] Race condition in udev

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

On Tue, 2009-08-25 at 15:11 +0200, Florian Zumbiehl wrote:

> > > well, in those two cases always rename()ing the new node into place would
> > > work, too!? That would be a different strategy than what's in
> > > place at the moment, but it wouldn't need a special case!?
> > > 
> > The rename() will fail.
> 
> Because?
> 
POSIX.

> > > > Or when racing with devmapper which creates /dev/mapper/foo devices at
> > > > basically the same time as udev.
> > > 
> > > Seriously? How is a piece of code that does the existence check and
> > > the subsequent action depending on the result of that check non-atomically
> > > supposed to help avoid some race condition resulting from possible
> > > concurrent creation of a device node?!
> > > 
> > Read the code and find out.  It works.
> 
> Guess how I found out that it can not work.
> 
I don't know, you haven't given any detail of any problems you've
encountered.

Scott
-- 
Scott James Remnant
scott@canonical.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > > > well, in those two cases always rename()ing the new node into place would
> > > > work, too!? That would be a different strategy than what's in
> > > > place at the moment, but it wouldn't need a special case!?
> > > > 
> > > The rename() will fail.
> > 
> > Because?
> > 
> POSIX.

More specifically?

And anyhow, I thought we were talking about the Linux kernel?!

> > > > > Or when racing with devmapper which creates /dev/mapper/foo devices at
> > > > > basically the same time as udev.
> > > > 
> > > > Seriously? How is a piece of code that does the existence check and
> > > > the subsequent action depending on the result of that check non-atomically
> > > > supposed to help avoid some race condition resulting from possible
> > > > concurrent creation of a device node?!
> > > > 
> > > Read the code and find out.  It works.
> > 
> > Guess how I found out that it can not work.
> > 
> I don't know, you haven't given any detail of any problems you've
> encountered.

I haven't "encountered any problems", nor have I claimed to have
"encountered any problems".

You stated that the codepath in udev-node.c for the case when a
device node does already exist was somehow there for the case
when udev races with devmapper. I noted that that codepath is not
of any use in such a case, and that your argument thus is invalid.

Florian

Re: [security] Race condition in udev

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]

On Tue, 2009-08-25 at 16:22 +0200, Florian Zumbiehl wrote:

> > > > > well, in those two cases always rename()ing the new node into place would
> > > > > work, too!? That would be a different strategy than what's in
> > > > > place at the moment, but it wouldn't need a special case!?
> > > > > 
> > > > The rename() will fail.
> > > 
> > > Because?
> > > 
> > POSIX.
> 
> More specifically?
> 
> And anyhow, I thought we were talking about the Linux kernel?!
> 
If you don't know why rename() might fail, you really shouldn't be
mucking around with this kind of code.

> > > > > > Or when racing with devmapper which creates /dev/mapper/foo devices at
> > > > > > basically the same time as udev.
> > > > > 
> > > > > Seriously? How is a piece of code that does the existence check and
> > > > > the subsequent action depending on the result of that check non-atomically
> > > > > supposed to help avoid some race condition resulting from possible
> > > > > concurrent creation of a device node?!
> > > > > 
> > > > Read the code and find out.  It works.
> > > 
> > > Guess how I found out that it can not work.
> > > 
> > I don't know, you haven't given any detail of any problems you've
> > encountered.
> 
> I haven't "encountered any problems", nor have I claimed to have
> "encountered any problems".
> 
> You stated that the codepath in udev-node.c for the case when a
> device node does already exist was somehow there for the case
> when udev races with devmapper. I noted that that codepath is not
> of any use in such a case, and that your argument thus is invalid.
> 
Since I wrote this code, and the code in devmapper, and have not only
strenuously tested it; but have at least 18 million for whom it works
every day, I'd argue that my argument is quite valid ;-)

Scott
-- 
Scott James Remnant
scott@canonical.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > > > > The rename() will fail.
> > > > 
> > > > Because?
> > > > 
> > > POSIX.
> > 
> > More specifically?
> > 
> > And anyhow, I thought we were talking about the Linux kernel?!
> > 
> If you don't know why rename() might fail, you really shouldn't be
> mucking around with this kind of code.

see, that's why I am trying to consult with you before trying to
"muck around with this kind of code". However, you may have noticed
that this message of yours was rather much void of any information
that could help me in doing so. Also, you are welcome to fix the
bug I was reporting using your understanding of the code, so I
don't have to "muck around with this kind of code" - I really am
not all that keen on doing so.

> > > > > > > Or when racing with devmapper which creates /dev/mapper/foo devices at
> > > > > > > basically the same time as udev.
> > > > > > 
> > > > > > Seriously? How is a piece of code that does the existence check and
> > > > > > the subsequent action depending on the result of that check non-atomically
> > > > > > supposed to help avoid some race condition resulting from possible
> > > > > > concurrent creation of a device node?!
> > > > > > 
> > > > > Read the code and find out.  It works.
> > > > 
> > > > Guess how I found out that it can not work.
> > > > 
> > > I don't know, you haven't given any detail of any problems you've
> > > encountered.
> > 
> > I haven't "encountered any problems", nor have I claimed to have
> > "encountered any problems".
> > 
> > You stated that the codepath in udev-node.c for the case when a
> > device node does already exist was somehow there for the case
> > when udev races with devmapper. I noted that that codepath is not
> > of any use in such a case, and that your argument thus is invalid.
> > 
> Since I wrote this code, and the code in devmapper, and have not only
> strenuously tested it; but have at least 18 million for whom it works
> every day, I'd argue that my argument is quite valid ;-)

No, arguments are not valid because something unrelated is true.

Florian

Re: [security] Race condition in udev

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 931 bytes --]

On Tue, 2009-08-25 at 18:27 +0200, Florian Zumbiehl wrote:

> see, that's why I am trying to consult with you before trying to
> "muck around with this kind of code". However, you may have noticed
> that this message of yours was rather much void of any information
> that could help me in doing so. Also, you are welcome to fix the
> bug I was reporting using your understanding of the code, so I
> don't have to "muck around with this kind of code" - I really am
> not all that keen on doing so.
> 
But you haven't reported a bug.

You say that setting the mode of a device node before setting the
ownership is a security issue, *but* you have not demonstrated how this
might be exploited.

Since device nodes are created with root ownership, setting the mode
before the ownership is *not a concern* because it can only have less
access than afterwards.

Scott
-- 
Scott James Remnant
scott@canonical.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > see, that's why I am trying to consult with you before trying to
> > "muck around with this kind of code". However, you may have noticed
> > that this message of yours was rather much void of any information
> > that could help me in doing so. Also, you are welcome to fix the
> > bug I was reporting using your understanding of the code, so I
> > don't have to "muck around with this kind of code" - I really am
> > not all that keen on doing so.
> > 
> But you haven't reported a bug.
> 
> You say that setting the mode of a device node before setting the
> ownership is a security issue, *but* you have not demonstrated how this
> might be exploited.
> 
> Since device nodes are created with root ownership, setting the mode
> before the ownership is *not a concern* because it can only have less
> access than afterwards.

actually, I even explained _how_ this could lead to temporarily wider
permissions than configured, but let's trace through an example for
Mr. POSIX:

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

Could we now take care of the bug?

Oh, and would you mind explaining why the rename() could fail?

Florian

Re: [security] Race condition in udev

[Greg KH]

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?

> Could we now take care of the bug?

Do you have a proposed patch?

thanks,

greg k-h

Re: [security] Race condition in udev

[Robby Workman]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]

On Tue, 25 Aug 2009 10:42:49 -0700
Greg KH <greg@kroah.com> wrote:

> 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?


Even if there are, I still don't see how it's a bug in udev.

If you have a rule that configures a device with world-writable
permissions, why does it matter which group owns the device?

-RW

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [security] Race condition in udev

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 3037 bytes --]

On Tue, 2009-08-25 at 19:31 +0200, Florian Zumbiehl wrote:

> actually, I even explained _how_ this could lead to temporarily wider
> permissions than configured, but let's trace through an example for
> Mr. POSIX:
> 
> 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
> 
> Could we now take care of the bug?
> 
Sure, let's invert the chmod() and chown()...

BUT WAIT!

Assumption:

 /dev/foo is configured to be owned by user mrposix, mode 0466.  The
 attacker (mrposix) tries to open /dev/foo for writing.

The Trace:

  action                        | owner   | mode    | open(O_WRONLY)?
  ------------------------------+---------+---------+-----------------
  mknod(/dev/foo)               | root    | 0644    | no
  chown(/dev/foo,mrposix,users) | mrposix | 0644    | yes
  chmod(/dev/foo,0466)          | mrposix | 0466    | no


In other words, exactly the same race.

Since we don't have a chownmod syscall, we have to do this
non-atomically, which means that either there's a race for denying
access for particular users or a race for denying access for particular
groups.


Now, let's put aside for a moment the fact that either of these modes is
insane.  Which of the two attempts to deny access _should_ work?

The surprising answer is that the attempt to deny access to the *user*
is the one that should work (ie. don't change the code), not the attempt
to deny access to a group.


There's a simple reason for this.

A user's uid is fixed, and is a key fact of that user's privilege.  The
only way for "mrposix" to change users is if he was really root in the
first place, or has access to a setuid program.

A user's gid *is not* fixed, in fact, a user may be a member of any one
or more groups at their own choice.  An entry in /etc/passwd
or /etc/groups does not *force* the user into those groups, it simply
grants the user *access* to be a member of those groups.

There are tried, tested and (most importantly) *permitted* methods for a
user to shed group membership.  Since a user can shed their group
membership in this way, they can work around your attempt to restrict
access, and thus grant themselves write access after all.


(This is, in fact, why it's "insane" to deny access using modes; a user
 can always find a way to become *less* privileged, and thus if you're
 trying to deny them access to things, actually gain privilege)

Scott
-- 
Scott James Remnant
scott@canonical.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [security] Race condition in udev

[Florian Zumbiehl]

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.

> > 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).

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?

Florian

Re: [security] Race condition in udev

[Kay Sievers]

On Tue, Aug 25, 2009 at 20:11, Florian Zumbiehl<florz@florz.de> wrote:
>> 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.
>
>> > 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).
>
> 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?

I suggest you just do something useful instead of wasting our time.

Thanks,
Kay

Re: [security] Race condition in udev

[Greg KH]

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

Re: [security] Race condition in udev

[Greg KH]

On Tue, Aug 25, 2009 at 01:04:17PM -0500, Robby Workman wrote:
> On Tue, 25 Aug 2009 10:42:49 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> > 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?
> 
> 
> Even if there are, I still don't see how it's a bug in udev.
> 
> If you have a rule that configures a device with world-writable
> permissions, why does it matter which group owns the device?

Exactly :)

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> Sure, let's invert the chmod() and chown()...
> 
> BUT WAIT!
> 
> Assumption:
> 
>  /dev/foo is configured to be owned by user mrposix, mode 0466.  The
>  attacker (mrposix) tries to open /dev/foo for writing.
> 
> The Trace:
> 
>   action                        | owner   | mode    | open(O_WRONLY)?
>   ------------------------------+---------+---------+-----------------
>   mknod(/dev/foo)               | root    | 0644    | no
>   chown(/dev/foo,mrposix,users) | mrposix | 0644    | yes
>   chmod(/dev/foo,0466)          | mrposix | 0466    | no
> 
> 
> In other words, exactly the same race.
> 
> Since we don't have a chownmod syscall, we have to do this
> non-atomically, which means that either there's a race for denying
> access for particular users or a race for denying access for particular
> groups.
> 
> 
> Now, let's put aside for a moment the fact that either of these modes is
> insane.  Which of the two attempts to deny access _should_ work?
> 
> The surprising answer is that the attempt to deny access to the *user*
> is the one that should work (ie. don't change the code), not the attempt
> to deny access to a group.

In the further text, you assume that the user can execute arbitrary
code - in that case, though, the owner can change the permissions
of his own files, so you can't really deny much.

But yeah, just reordering the calls obviously doesn't avoid
the problem.

> There's a simple reason for this.
> 
> A user's uid is fixed, and is a key fact of that user's privilege.  The
> only way for "mrposix" to change users is if he was really root in the
> first place, or has access to a setuid program.
> 
> A user's gid *is not* fixed, in fact, a user may be a member of any one
> or more groups at their own choice.  An entry in /etc/passwd
> or /etc/groups does not *force* the user into those groups, it simply
> grants the user *access* to be a member of those groups.
> 
> There are tried, tested and (most importantly) *permitted* methods for a
> user to shed group membership.  Since a user can shed their group
> membership in this way, they can work around your attempt to restrict
> access, and thus grant themselves write access after all.

There are? In the kernel?

> (This is, in fact, why it's "insane" to deny access using modes; a user
>  can always find a way to become *less* privileged, and thus if you're
>  trying to deny them access to things, actually gain privilege)

As kindof noted above, I think that cases of users capable of doing read-
and write-accesses but not of executing arbitrary code should be
considered.

Or in more general terms: Well, yeah, there probably are many userspace
configurations where such permissions would not be a wise thing to use.
But still, there probably are just as many cases that are perfectly
safe - and I think that udev should not decide for the user which kinds
of "odd" permissions with otherwise well-defined effects they are
supposed to use, in particular so if it's not documented (which I
suppose it's not?)

Florian

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > > > 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?

Scott claims so, but so far hasn't explained why. After all, the current
code does that rename()ing in case there is some device node with
different device numbers under the name to be created - but maybe there
is some reason for why this works even though it may not work in other
cases?

> > 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.

I can't find anything as to why this wouldn't be ok in any of his emails.

Florian

Re: [security] Race condition in udev

[Greg KH]

On Tue, Aug 25, 2009 at 08:53:18PM +0200, Florian Zumbiehl wrote:
> > > 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.
> 
> I can't find anything as to why this wouldn't be ok in any of his emails.

Because, again, you aren't really protecting anything here.

Especially as you point out that there are no existing device node rules
that have problems in them.  So I fail to see the issue.

thanks,

greg k-h

Re: [security] Race condition in udev

[Mr POSIX]

On Tue, 2009-08-25 at 20:38 +0200, Florian Zumbiehl wrote:

> Or in more general terms: Well, yeah, there probably are many userspace
> configurations where such permissions would not be a wise thing to use.
> But still, there probably are just as many cases that are perfectly
> safe
> 
No, there really isn't.

Let's go back to basics of the UNIX security. model, and most
importantly, how this is *interpreted* by applications.


The model is one of "grant".  That is to say, that to be able to perform
any privileged action, you must be granted that privilege.

Even your uid is a "grant" of privilege, it enables you to communicate
and change other processes running under that same uid.

Likewise a gid is a "grant" of privilege.


Therefore there is an assumption that a newly created user, with a
unique uid and gid not used anywhere, has effectively no privilege.
This assumption is used in many places, but most notably when daemons
and services run as a user of their own - or even the "nobody" user.

Your example breaks this assertion.  By giving a user or group *less*
privilege than other users, you have effectively granted a privilege to
"nobody" and secure users that genuine users *do not have*.

Put simply, a mask should decrease in value when read from left to right
- 755 is valid, 577 isn't.


Giving a user or group less privilege than "anybody else" is easy to
circumvent, because the basic assumption is that by changing user or
adding a group you are *gaining* privilege. not dropping it - and thus
by switching to a "nobody" user you are *dropping* privilege not gaining
it.

Scott
-- 
Scott James Remnant
scott@canonical.com

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi Mr. POSIX, ;-)

> On Tue, 2009-08-25 at 20:38 +0200, Florian Zumbiehl wrote:
> 
> > Or in more general terms: Well, yeah, there probably are many userspace
> > configurations where such permissions would not be a wise thing to use.
> > But still, there probably are just as many cases that are perfectly
> > safe
> > 
> No, there really isn't.
> 
> Let's go back to basics of the UNIX security. model, and most
> importantly, how this is *interpreted* by applications.
> 
> 
> The model is one of "grant".  That is to say, that to be able to perform
> any privileged action, you must be granted that privilege.
> 
> Even your uid is a "grant" of privilege, it enables you to communicate
> and change other processes running under that same uid.
> 
> Likewise a gid is a "grant" of privilege.

isn't that a bit at odds with the fact that the kernel does _not_ check
against the accumulation of all of owner, group and others permissions
that would apply to the process in question? Wouldn't really be all that
difficult to implement, after all.

> Therefore there is an assumption that a newly created user, with a
> unique uid and gid not used anywhere, has effectively no privilege.
> This assumption is used in many places, but most notably when daemons
> and services run as a user of their own - or even the "nobody" user.
> 
> Your example breaks this assertion.  By giving a user or group *less*
> privilege than other users, you have effectively granted a privilege to
> "nobody" and secure users that genuine users *do not have*.

Well, IMO you are mixing up what the userspace conventions of most
desktop/server installations look like, and what the security model
of the kernel is.

Given that udev is nearly a component of the kernel, IMO it should
follow the security model of the kernel, and not force userspace to
follow any additional conventions. Think an embedded system that uses
its own userspace implementation, but uses udev for managing its /dev.
Or a chrooted session of some application-specific protocol that for
some reason bind-mounts /dev. Not so much a user logging into a
regular ssh remote shell.

> Put simply, a mask should decrease in value when read from left to right
> - 755 is valid, 577 isn't.

Says who?

> Giving a user or group less privilege than "anybody else" is easy to
> circumvent, because the basic assumption is that by changing user or
> adding a group you are *gaining* privilege. not dropping it - and thus
> by switching to a "nobody" user you are *dropping* privilege not gaining
> it.

How is that a "basic assumption", given that the kernel implements the
exact opposite, and that this exact opposite is documented behaviour?

You didn't really answer a question the answer to which probably would be
rather important in this context: Is there any way for a non-privileged
process to drop a group membership without exec()ing?

Also, I really would like to understand why the rename() in that scenario
could fail, independent of whether we'll use that for anything.

To point out another place where the kernel's model deviates from the
"usual way things work in userspace": Capabilities. Yes, a lot of
userspace programs do assume that uid 0 equals "is allowed to do anything".
Still, the kernel does allow for uid 0 to be completely unprivileged.
Should we now drop that from the kernel, given that it contradicts common
assumptions about how userspace works, or is there maybe a point to
supporting different userspace security models?

Florian

Re: [security] Race condition in udev

[Scott James Remnant]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

On Tue, 2009-08-25 at 23:55 +0200, Florian Zumbiehl wrote:

> isn't that a bit at odds with the fact that the kernel does _not_ check
> against the accumulation of all of owner, group and others permissions
> that would apply to the process in question? Wouldn't really be all that
> difficult to implement, after all.
> 
The kernel doesn't check that the netmask of a network route is of the
form <1>s<0>s and not something random like 10101010... yet if you try
and use that kind of network route, you'll discover that it just won't
work out.

> Well, IMO you are mixing up what the userspace conventions of most
> desktop/server installations look like, and what the security model
> of the kernel is.
> 
> Given that udev is nearly a component of the kernel, IMO it should
> follow the security model of the kernel, and not force userspace to
> follow any additional conventions.
> 
udev doesn't enforce any permission or mode restriction; you can put
whatever you like in there.

Of course, it probably won't work out.


More to the point, you haven't explained how to work around the fact
that simply inverting the chmod/chown (or any variation of that) doesn't
remove the race condition - just moves it between the user or group.

> You didn't really answer a question the answer to which probably would be
> rather important in this context: Is there any way for a non-privileged
> process to drop a group membership without exec()ing?
> 
> Also, I really would like to understand why the rename() in that scenario
> could fail, independent of whether we'll use that for anything.
> 
Apparently I'm "Mr GOOGLE" as well as "Mr POSIX"

Scott
-- 
Scott James Remnant
scott@canonical.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> On Tue, 2009-08-25 at 23:55 +0200, Florian Zumbiehl wrote:
> 
> > isn't that a bit at odds with the fact that the kernel does _not_ check
> > against the accumulation of all of owner, group and others permissions
> > that would apply to the process in question? Wouldn't really be all that
> > difficult to implement, after all.
> > 
> The kernel doesn't check that the netmask of a network route is of the
> form <1>s<0>s and not something random like 10101010... yet if you try
> and use that kind of network route, you'll discover that it just won't
> work out.

now, how is that "analogy" supposed to support your statement that
the traditional interpretation of permissions on unix systems was the
opposite of what the documented behaviour is?

> > Well, IMO you are mixing up what the userspace conventions of most
> > desktop/server installations look like, and what the security model
> > of the kernel is.
> > 
> > Given that udev is nearly a component of the kernel, IMO it should
> > follow the security model of the kernel, and not force userspace to
> > follow any additional conventions.
> > 
> udev doesn't enforce any permission or mode restriction; you can put
> whatever you like in there.
> 
> Of course, it probably won't work out.

So, you are saying "your way won't work, but I don't force you to use
my way", right?

Anyhow, the current code does potentially allow more access than one
would expect when interpreting udev's configuration using the
well-known semantics of unix permissions, which is kindof worse
than "just not working".

> More to the point, you haven't explained how to work around the fact
> that simply inverting the chmod/chown (or any variation of that) doesn't
> remove the race condition - just moves it between the user or group.

If I may quote from one of my mails:

| 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?

Possibly skipping it all when no changes are necessary would even make
it free from any possible regressions from the current state?

> > You didn't really answer a question the answer to which probably would be
> > rather important in this context: Is there any way for a non-privileged
> > process to drop a group membership without exec()ing?
> > 
> > Also, I really would like to understand why the rename() in that scenario
> > could fail, independent of whether we'll use that for anything.
> > 
> Apparently I'm "Mr GOOGLE" as well as "Mr POSIX"

"GOOGLE"? What's that?

Well, I certainly would appreciate anything I could feed into this
google thing in order to get to an understanding of the issues.
But a reference to a specific paragraph of text that you think
applies in this case really would be appreciated even more.

Florian

Re: [security] Race condition in udev

[Greg KH]

On Wed, Aug 26, 2009 at 07:41:34PM +0200, Florian Zumbiehl wrote:
> Anyhow, the current code does potentially allow more access than one
> would expect when interpreting udev's configuration using the
> well-known semantics of unix permissions, which is kindof worse
> than "just not working".

Again, you have failed to show how this would happen, given udev's
existing rules that all distros ship.

Until you do that, this thread is going nowhere.

thanks,

greg k-h

Re: [security] Race condition in udev

[Matthias Schwarzott]

On Mittwoch, 26. August 2009, Florian Zumbiehl wrote:
>
> Anyhow, the current code does potentially allow more access than one
> would expect when interpreting udev's configuration using the
> well-known semantics of unix permissions, which is kindof worse
> than "just not working".

The only case I can imagine where the race you try to describe gives more 
permission than meant by the rule writer is this scenario:

1. ruleset contains:
KERNEL="mydev", MODE="640", GROUP="readers"

2. after boot:
# ls -l /dev/mydev
brw-r----- 1 root writers ?, ? 25. Aug 16:09 /dev/mydev

3. Change the ruleset to contain (udev will reload it notified by inotify):
KERNEL="mydev", MODE="660", GROUP="writers"

4. run udevadm trigger

5. udev will process the new rule and

6. first chmod /dev/mydev
# ls -l /dev/mydev
brw-rw---- 1 root readers ?, ? 25. Aug 16:09 /dev/mydev

7. and then chown /dev/mydev so it gets its final permissions
# ls -l /dev/mydev
brw-rw---- 1 root writers ?, ? 25. Aug 16:09 /dev/mydev

So there is this small time window between 6 and 7 where group readers has 
more permissions it should. BUT: This can only happen if admin changes the 
rules, or does manually adjust permissions of some devices and then triggers 
udev.

If you show that this can really happen one could do something like this:

1. first chmod to safe umask (either logical and of old and new umask, or 
0000)
2. then chown
3. then chmod to new umask

Regards
Matthias

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> More to the point, you haven't explained how to work around the fact
> that simply inverting the chmod/chown (or any variation of that) doesn't
> remove the race condition - just moves it between the user or group.

below you find a patch that should fix the specific issue - I just am
not sure that it interacts nicely with the rest of udev. Also, I haven't
tried it, not even compiled it.

Florian

diff --git a/udev/udev-node.c b/udev/udev-node.c
index 03ab0ea..d73bc6c 100644
--- a/udev/udev-node.c
+++ b/udev/udev-node.c
@@ -88,11 +88,11 @@ int udev_node_mknod(struct udev_device *dev, const char *file, dev_t devnum, mod
 			util_strscpyl(file_tmp, sizeof(file_tmp), file, TMP_FILE_EXT, NULL);
 			unlink(file_tmp);
 			udev_selinux_setfscreatecon(udev, file_tmp, mode);
-			err = mknod(file_tmp, mode, devnum);
+			err = mknod(file_tmp, 0, devnum);
 			udev_selinux_resetfscreatecon(udev);
 			if (err != 0) {
 				err(udev, "mknod(%s, %#o, %u, %u) failed: %m\n",
-				    file_tmp, mode, major(devnum), minor(devnum));
+				    file_tmp, 0, major(devnum), minor(devnum));
 				goto exit;
 			}
 			err = rename(file_tmp, file);
@@ -102,17 +102,33 @@ int udev_node_mknod(struct udev_device *dev, const char *file, dev_t devnum, mod
 			}
 		}
 	} else {
-		info(udev, "mknod(%s, %#o, (%u,%u))\n", file, mode, major(devnum), minor(devnum));
+		info(udev, "mknod(%s, %#o, (%u,%u))\n", file, 0, major(devnum), minor(devnum));
 		udev_selinux_setfscreatecon(udev, file, mode);
-		err = mknod(file, mode, devnum);
+		err = mknod(file, 0, devnum);
 		udev_selinux_resetfscreatecon(udev);
 		if (err != 0) {
-			err(udev, "mknod(%s, %#o, (%u,%u) failed: %m\n", file, mode, major(devnum), minor(devnum));
+			err(udev, "mknod(%s, %#o, (%u,%u)) failed: %m\n", file, 0, major(devnum), minor(devnum));
 			goto exit;
 		}
 	}
 
-	if (!preserve || stats.st_mode != mode) {
+	if (!preserve || stats.st_mode != mode || stats.st_uid != uid || stats.st_gid != gid) {
+		if (!preserve || stats.st_uid != uid || stats.st_gid != gid) {
+			info(udev, "chmod(%s, %#o)\n", file, 0);
+			err = chmod(file, 0);
+			if (err != 0) {
+				err(udev, "chmod(%s, %#o) failed: %m\n", file, 0);
+				goto exit;
+			}
+
+			info(udev, "chown(%s, %u, %u)\n", file, uid, gid);
+			err = chown(file, uid, gid);
+			if (err != 0) {
+				err(udev, "chown(%s, %u, %u) failed: %m\n", file, uid, gid);
+				goto exit;
+			}
+		}
+
 		info(udev, "chmod(%s, %#o)\n", file, mode);
 		err = chmod(file, mode);
 		if (err != 0) {
@@ -120,15 +136,6 @@ int udev_node_mknod(struct udev_device *dev, const char *file, dev_t devnum, mod
 			goto exit;
 		}
 	}
-
-	if (!preserve || stats.st_uid != uid || stats.st_gid != gid) {
-		info(udev, "chown(%s, %u, %u)\n", file, uid, gid);
-		err = chown(file, uid, gid);
-		if (err != 0) {
-			err(udev, "chown(%s, %u, %u) failed: %m\n", file, uid, gid);
-			goto exit;
-		}
-	}
 exit:
 	return err;
 }

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> On Wed, Aug 26, 2009 at 07:41:34PM +0200, Florian Zumbiehl wrote:
> > Anyhow, the current code does potentially allow more access than one
> > would expect when interpreting udev's configuration using the
> > well-known semantics of unix permissions, which is kindof worse
> > than "just not working".
> 
> Again, you have failed to show how this would happen, given udev's
> existing rules that all distros ship.
> 
> Until you do that, this thread is going nowhere.

so, you think that udev's rules are not configuration that is to be
touched by an admin, but rather part of the code?

Florian

Re: [security] Race condition in udev

[Greg KH]

On Thu, Aug 27, 2009 at 05:13:30PM +0200, Florian Zumbiehl wrote:
> Hi,
> 
> > On Wed, Aug 26, 2009 at 07:41:34PM +0200, Florian Zumbiehl wrote:
> > > Anyhow, the current code does potentially allow more access than one
> > > would expect when interpreting udev's configuration using the
> > > well-known semantics of unix permissions, which is kindof worse
> > > than "just not working".
> > 
> > Again, you have failed to show how this would happen, given udev's
> > existing rules that all distros ship.
> > 
> > Until you do that, this thread is going nowhere.
> 
> so, you think that udev's rules are not configuration that is to be
> touched by an admin, but rather part of the code?

For the most part, none of the default udev rules should need to be
touched by an admin, otherwise the device naming scheme that is
consistant across all distros would be messed up.

Sure they can add their own rules if they want to, and lots do, but
that's not the issue here, right?

thanks,

greg k-h

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> So there is this small time window between 6 and 7 where group readers has 
> more permissions it should. BUT: This can only happen if admin changes the 
> rules, or does manually adjust permissions of some devices and then triggers 
> udev.

so, you think that having, say, a sudo-callable script that modifies
permissions in udev rules is an invalid configuration under any and
all circumstances?

And BTW, why does the chmod() have any error handling? If the time
between 6 and 7 is guaranteed to be small, it's impossible for an error
to ever happen there, anyhow. Well, OK, except for ENOENT, kindof,
maybe ;-)

Or in other words: (a) Race conditions stay race conditions even if
they seem difficult to provoke, (b) what seems difficult to provoke
under certain circumstances isn't necessarily all that difficult to
provoke under different circumstances, and (c) it's usually a bad idea
to base the security of a large and flexible system on a complicated
and undocumented set of preconditions unless you are sure that those
will be met, pretty much no matter what crazy ideas the user has.

But thanks for pointing out another scenario that could happen
in "more usual setups" where this bug would potentially allow
more access than one would expect from the configuration.

Florian

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > so, you think that udev's rules are not configuration that is to be
> > touched by an admin, but rather part of the code?
> 
> For the most part, none of the default udev rules should need to be
> touched by an admin, otherwise the device naming scheme that is
> consistant across all distros would be messed up.
> 
> Sure they can add their own rules if they want to, and lots do, but
> that's not the issue here, right?

it's not? I would think that that's kindof the primary issue!?
After all, I would think that one typical reason for adding rules
is for overriding permissions for a specific device. Of course, the
same applies if you change the default rules ...

Florian

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> below you find a patch that should fix the specific issue - I just am
> not sure that it interacts nicely with the rest of udev. Also, I haven't
> tried it, not even compiled it.

here is another patch for basically the same problem in
util_unlink_secure(), same disclaimer applies.

Florian

diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
index 3641b36..28008c5 100644
--- a/libudev/libudev-util-private.c
+++ b/libudev/libudev-util-private.c
@@ -102,6 +102,10 @@ int util_unlink_secure(struct udev *udev, const char *filename)
 {
 	int retval;
 
+	retval = chmod(filename, 0000);
+	if (retval)
+		err(udev, "chmod(%s, 0000) failed: %m\n", filename);
+
 	retval = chown(filename, 0, 0);
 	if (retval)
 		err(udev, "chown(%s, 0, 0) failed: %m\n", filename);

Re: [security] Race condition in udev

[Kay Sievers]

On Fri, Aug 28, 2009 at 19:34, Florian Zumbiehl<florz@florz.de> wrote:
>> below you find a patch that should fix the specific issue - I just am
>> not sure that it interacts nicely with the rest of udev. Also, I haven't
>> tried it, not even compiled it.
>
> here is another patch for basically the same problem in
> util_unlink_secure(), same disclaimer applies.
>
> Florian
>
> diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
> index 3641b36..28008c5 100644
> --- a/libudev/libudev-util-private.c
> +++ b/libudev/libudev-util-private.c
> @@ -102,6 +102,10 @@ int util_unlink_secure(struct udev *udev, const char *filename)
>  {
>        int retval;
>
> +       retval = chmod(filename, 0000);
> +       if (retval)
> +               err(udev, "chmod(%s, 0000) failed: %m\n", filename);
> +
>        retval = chown(filename, 0, 0);
>        if (retval)
>                err(udev, "chown(%s, 0, 0) failed: %m\n", filename);

We need only one chmod() here. I changed the order.

Thanks,
Kay

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
> > index 3641b36..28008c5 100644
> > --- a/libudev/libudev-util-private.c
> > +++ b/libudev/libudev-util-private.c
> > @@ -102,6 +102,10 @@ int util_unlink_secure(struct udev *udev, const char *filename)
> >  {
> >        int retval;
> >
> > +       retval = chmod(filename, 0000);
> > +       if (retval)
> > +               err(udev, "chmod(%s, 0000) failed: %m\n", filename);
> > +
> >        retval = chown(filename, 0, 0);
> >        if (retval)
> >                err(udev, "chown(%s, 0, 0) failed: %m\n", filename);
> 
> We need only one chmod() here. I changed the order.

no, you need both. In the case that the device belonged to non-root before,
the owner could do a chmod() in between the chmod() and chown() and thus
retain privileges on the device node.

Florian

Re: [security] Race condition in udev

[Kay Sievers]

On Sat, Aug 29, 2009 at 16:20, Florian Zumbiehl<florz@florz.de> wrote:
>> > diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
>> > index 3641b36..28008c5 100644
>> > --- a/libudev/libudev-util-private.c
>> > +++ b/libudev/libudev-util-private.c
>> > @@ -102,6 +102,10 @@ int util_unlink_secure(struct udev *udev, const char *filename)
>> >  {
>> >        int retval;
>> >
>> > +       retval = chmod(filename, 0000);
>> > +       if (retval)
>> > +               err(udev, "chmod(%s, 0000) failed: %m\n", filename);
>> > +
>> >        retval = chown(filename, 0, 0);
>> >        if (retval)
>> >                err(udev, "chown(%s, 0, 0) failed: %m\n", filename);
>>
>> We need only one chmod() here. I changed the order.
>
> no, you need both. In the case that the device belonged to non-root before,
> the owner could do a chmod() in between the chmod() and chown() and thus
> retain privileges on the device node.

What about fchmod(), how's that handled in such case?

Kay

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> On Sat, Aug 29, 2009 at 16:20, Florian Zumbiehl<florz@florz.de> wrote:
> >> > diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
> >> > index 3641b36..28008c5 100644
> >> > --- a/libudev/libudev-util-private.c
> >> > +++ b/libudev/libudev-util-private.c
> >> > @@ -102,6 +102,10 @@ int util_unlink_secure(struct udev *udev, const char *filename)
> >> >  {
> >> >        int retval;
> >> >
> >> > +       retval = chmod(filename, 0000);
> >> > +       if (retval)
> >> > +               err(udev, "chmod(%s, 0000) failed: %m\n", filename);
> >> > +
> >> >        retval = chown(filename, 0, 0);
> >> >        if (retval)
> >> >                err(udev, "chown(%s, 0, 0) failed: %m\n", filename);
> >>
> >> We need only one chmod() here. I changed the order.
> >
> > no, you need both. In the case that the device belonged to non-root before,
> > the owner could do a chmod() in between the chmod() and chown() and thus
> > retain privileges on the device node.
> 
> What about fchmod(), how's that handled in such case?

I assumed that it's guaranteed for there to not be any open fds on the
device anymore at unlink() time!?

Florian

Re: [security] Race condition in udev

[Kay Sievers]

On Sat, Aug 29, 2009 at 16:41, Florian Zumbiehl<florz@florz.de> wrote:
>> On Sat, Aug 29, 2009 at 16:20, Florian Zumbiehl<florz@florz.de> wrote:
>> >> > diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
>> >> > index 3641b36..28008c5 100644
>> >> > --- a/libudev/libudev-util-private.c
>> >> > +++ b/libudev/libudev-util-private.c
>> >> > @@ -102,6 +102,10 @@ int util_unlink_secure(struct udev *udev, const char *filename)
>> >> >  {
>> >> >        int retval;
>> >> >
>> >> > +       retval = chmod(filename, 0000);
>> >> > +       if (retval)
>> >> > +               err(udev, "chmod(%s, 0000) failed: %m\n", filename);
>> >> > +
>> >> >        retval = chown(filename, 0, 0);
>> >> >        if (retval)
>> >> >                err(udev, "chown(%s, 0, 0) failed: %m\n", filename);
>> >>
>> >> We need only one chmod() here. I changed the order.
>> >
>> > no, you need both. In the case that the device belonged to non-root before,
>> > the owner could do a chmod() in between the chmod() and chown() and thus
>> > retain privileges on the device node.
>>
>> What about fchmod(), how's that handled in such case?
>
> I assumed that it's guaranteed for there to not be any open fds on the
> device anymore at unlink() time!?

The entire hardlink thing is purely theoretical and a pretty useless
exercise because we are on our own filesystem and can not hardlink
anywhere else. But I guess, there can not be any assumptions about no
fds open.

Kay

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> On Sat, Aug 29, 2009 at 16:41, Florian Zumbiehl<florz@florz.de> wrote:
> >> On Sat, Aug 29, 2009 at 16:20, Florian Zumbiehl<florz@florz.de> wrote:
> >> >> > diff --git a/libudev/libudev-util-private.c b/libudev/libudev-util-private.c
> >> >> > index 3641b36..28008c5 100644
> >> >> > --- a/libudev/libudev-util-private.c
> >> >> > +++ b/libudev/libudev-util-private.c
> >> >> > @@ -102,6 +102,10 @@ int util_unlink_secure(struct udev *udev, const char *filename)
> >> >> >  {
> >> >> >        int retval;
> >> >> >
> >> >> > +       retval = chmod(filename, 0000);
> >> >> > +       if (retval)
> >> >> > +               err(udev, "chmod(%s, 0000) failed: %m\n", filename);
> >> >> > +
> >> >> >        retval = chown(filename, 0, 0);
> >> >> >        if (retval)
> >> >> >                err(udev, "chown(%s, 0, 0) failed: %m\n", filename);
> >> >>
> >> >> We need only one chmod() here. I changed the order.
> >> >
> >> > no, you need both. In the case that the device belonged to non-root before,
> >> > the owner could do a chmod() in between the chmod() and chown() and thus
> >> > retain privileges on the device node.
> >>
> >> What about fchmod(), how's that handled in such case?
> >
> > I assumed that it's guaranteed for there to not be any open fds on the
> > device anymore at unlink() time!?
> 
> The entire hardlink thing is purely theoretical and a pretty useless
> exercise because we are on our own filesystem and can not hardlink

That's not just the "usual configuration"?

> anywhere else. But I guess, there can not be any assumptions about no
> fds open.

It just occured to me that it doesn't matter for this anyhow - fchmod()
applies the same checks as chmod(), so anyone (non-privileged and non-root)
who has an open fd can't fchmod() after the chown() to root anymore.

Florian

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > > I assumed that it's guaranteed for there to not be any open fds on the
> > > device anymore at unlink() time!?
> > 
> > The entire hardlink thing is purely theoretical and a pretty useless
> > exercise because we are on our own filesystem and can not hardlink
> 
> That's not just the "usual configuration"?
> 
> > anywhere else. But I guess, there can not be any assumptions about no
> > fds open.
> 
> It just occured to me that it doesn't matter for this anyhow - fchmod()
> applies the same checks as chmod(), so anyone (non-privileged and non-root)
> who has an open fd can't fchmod() after the chown() to root anymore.

any reason for not re-adding the second chmod() yet?

Florian

Re: [security] Race condition in udev

[Florian Zumbiehl]

Hi,

> > More to the point, you haven't explained how to work around the fact
> > that simply inverting the chmod/chown (or any variation of that) doesn't
> > remove the race condition - just moves it between the user or group.
> 
> below you find a patch that should fix the specific issue - I just am
> not sure that it interacts nicely with the rest of udev. Also, I haven't
> tried it, not even compiled it.
[...]

ping?!

Any hints how we could move forward with this?

Florian