[Toybox] [landley/toybox] install -d -o USER -g GROUP DEST doesn't set uid/gid (#105)

[Rob Landley]

"install -d" doesn't honor custom uid/gid.
install_main returns early if (flags & FLAG_d), doing mkpathat without fchown.

Hmmm... I just pushed a patch to that, but it's not quiet right.

When calling lchown() it does xgetuid() and xgetgid() for the default values
you're not overriding (when you have -o or -g but not both). Technically that
should be the fsuid, but although there's a setfsuid() I can't find a _getfsuid()_?

Man 7 capabilities is uninformative. And I still dunno what the point of suid
is. (We added real and effective, but programs know to look for both of those
know and freak if they're different! Let's add /opt and start sticking files in
there, then we'll need /opt/usr for when people start to expect it!)

Sorry, that's another rant:

http://lists.busybox.net/pipermail/busybox/2010-December/074114.html

(Which I got slightly wrong: the / disk was half a megabyte, the rk05 disk packs
were 2.5 megabytes each. Still adds up to 3 megabytes though. Primary sources
are https://www.bell-labs.com/usr/dmr/www/notes.html and
https://www.bell-labs.com/usr/dmr/www/hist.html)

Anyway, install -d does what you expect now, UNLESS you run it in a context
where you've changed fsuid (which was implemented for samba, so the server could
read/write files as different users without having to fork and run entirely _as_
those users). That would be ignored on the last path component, but not the ones
before it.

Let me know if anybody comes up with a proper fix. :)

Rob

[scsijon]

Message: 2
Date: Sat, 17 Nov 2018 12:28:32 -0600
Subject: Re: [Toybox] [landley/toybox] install -d -o USER -g GROUP
DEST doesn't set uid/gid (#105)
Content-Type: text/plain; charset=utf-8

"install -d" doesn't honor custom uid/gid.
install_main returns early if (flags & FLAG_d), doing mkpathat without fchown.

Hmmm... I just pushed a patch to that, but it's not quiet right.
When calling lchown() it does xgetuid() and xgetgid() for the default values
you're not overriding (when you have -o or -g but not both). Technically that
should be the fsuid, but although there's a setfsuid() I can't find a _getfsuid()_?
Man 7 capabilities is uninformative. And I still dunno what the point of suid
is. (We added real and effective, but programs know to look for both of those
know and freak if they're different! Let's add /opt and start sticking files in
there, then we'll need /opt/usr for when people start to expect it!)
http://lists.busybox.net/pipermail/busybox/2010-December/074114.html
(Which I got slightly wrong: the / disk was half a megabyte, the rk05 disk packs
were 2.5 megabytes each. Still adds up to 3 megabytes though. Primary sources
are https://www.bell-labs.com/usr/dmr/www/notes.html and
https://www.bell-labs.com/usr/dmr/www/hist.html)
Anyway, install -d does what you expect now, UNLESS you run it in a context
where you've changed fsuid (which was implemented for samba, so the server could
read/write files as different users without having to fork and run entirely _as_
those users). That would be ignored on the last path component, but not the ones
before it.
Let me know if anybody comes up with a proper fix. :)
Rob

Arn't they part of stat?

I believe openbsd have a getfsuid(), though not sure how good it is.

regards
scsijon

[Rob Landley]

Post by scsijon
Arn't they part of stat?
I believe openbsd have a getfsuid(), though not sure how good it is.

Yeah, I could open(O_NOFOLLOW) the file, confirm it's a directory, fstat(),
fchown() with the same filehandle, and close(). It's just 3 times longer than
what I'm doing and still _feels_ racy. :(

The problem is mkdir/mknod don't return a filehandle to the newly created inode,
so there's a race condition between creating the entry and then doing something
else to it later. (That's why I'm using lchown(), so if somebody drops a symlink
the chown() doesn't follow it.)

The nice thing about checking attributes on the _process_ is I don't have to
worry about things changing in the world-visible filesystem between two
non-atomic operations in a way I didn't anticipate. (I can't immediately think
how this is exploitable, but my instincts are to not allow gaps where things can
be fiddled with where somebody cleverer than me thinks of something I didn't.
Read/gap-where-it-can-change/write is a race condition. Sometimes unavoidable,
but never something to be happy about in a general system tool running from
scripts.)

Rob