[Toybox] oh, the irony...

[enh]

$ ./toybox su --help
toybox: Not root (see "toybox --help")

not sure what the fix is there though.

[Rob Landley]

Post by enh
$ ./toybox su --help
toybox: Not root (see "toybox --help")
not sure what the fix is there though.

Hmmm, I think it's that TOYFLAG_NEEDROOT should be checking geteuid() not
getuid(), but that's a security thing and I want to go over all the users
thoroughly before making the change.

The todo list for this release is now:

Finish/promote watch
fix crunch_str(): utf8 combining chars are trailing, not leading
promote gzip (decompression side, using lib/deflate.c)
Finish 79c403179b603a9f 482c422f8e8ec51 009b55edc4bad5b4 8993496e496cdbc
getconf: -a, build on musl (missing symbol?)
find -empty
NDK dynamic build (__android_log_write redef)
su - root says "not root"
TOYFLAG_NEEDROOT should check geteuid() not getuid()? Audit everybody.

Rob

[enh]

Post by Rob Landley

Post by enh
$ ./toybox su --help
toybox: Not root (see "toybox --help")
not sure what the fix is there though.

Hmmm, I think it's that TOYFLAG_NEEDROOT should be checking geteuid() not
getuid(), but that's a security thing and I want to go over all the users
thoroughly before making the change.

Isn't the problem that we should handle --help before checking whether the
caller is root?

Post by Rob Landley
Finish/promote watch
fix crunch_str(): utf8 combining chars are trailing, not leading
promote gzip (decompression side, using lib/deflate.c)
Finish 79c403179b603a9f 482c422f8e8ec51 009b55edc4bad5b4 8993496e496cdbc
getconf: -a, build on musl (missing symbol?)
find -empty
NDK dynamic build (__android_log_write redef)
su - root says "not root"
TOYFLAG_NEEDROOT should check geteuid() not getuid()? Audit everybody.
Rob

[Rob Landley]

On 10/18/2018 12:59 PM, enh wrote:> On Thu, Oct 18, 2018, 10:51 Rob Landley

Post by Rob Landley

Post by enh
$ ./toybox su --help
toybox: Not root (see "toybox --help")
not sure what the fix is there though.

Hmmm, I think it's that TOYFLAG_NEEDROOT should be checking geteuid() not
getuid(), but that's a security thing and I want to go over all the users
thoroughly before making the change.
Isn't the problem that we should handle --help before checking whether the
caller is root?

Hmmm, "yes but".

It drops privileges literally as early as possible. Minimizing the amount of
common code run as root when you have the suid bit set on the thing. Which means
it's before it's checked for --help.

So I see your point, but... hmmm.

Looks like I need to split it into two functions. I can do the test and drop
privs, record the results, and then have the error_exit() with the messages
happen later after it's parsed --help.

Throw it on the todo heap. (Sorry, worked late yesterday and today's busy too.
Trying to ship a thing.)

Rob

[Rob Landley]

Post by enh
$ ./toybox su --help
toybox: Not root (see "toybox --help")
not sure what the fix is there though.

Ok, dug into it some more.

If you "chown root:root toybox" and "chmod +s toybox", the su command then works
as advertised. Including su --help. What's it's _complaining_ about is this
command needs suid root to function, and toybox is not installed with that.

The fact --help doesn't work in that case is still a problem, but it sounds like
the "Not installed suid root" error message a couple lines earlier is what it
should be printing in this case. (It's guarded by TOYBOX_DEBUG because it's
"your system is built wrong", not runtime user error, but you have that on so
should have seen it.)

Except that's not printing for the busybox multiplexer itself (which !=
toy_list; the standalone case shouldn't have CFG_TOYBOX_SUID set because then
the individual command binary either has the suid bit set or it doesn't and
we're not _dropping_ it), because the _multiplexer_ doesn't know (at this stage)
what command we're going to be running.

I suspect the "not installed right" error message should be printed here
instead, but need to redo the logic. Right now it's right from a security
perspective, but not from a usability perspective, and I kinda privilege the
former over the latter... :P

Queued up for next release...

Rob

P.S. I've meant for a while to have "make config-allsuid" and "config-nosuid"
targets so people can have two binaries and only install the suid bit on the one
containing the commands that needs them. I just haven't gotten around to it
because $DAYJOB. (And really it's a bit like the "make single" stuff because the
binaries would have to be named differently to be insalled next to each other...
toybox-suid and toybox-nosuid, so really the targets should be "make
toybox-allsuid" and "toybox-nosuid"... which is why it's on my todo list after
the kconfig rewrite.)

Rob

[enh]

Post by Rob Landley

Post by enh
$ ./toybox su --help
toybox: Not root (see "toybox --help")
not sure what the fix is there though.

Ok, dug into it some more.
If you "chown root:root toybox" and "chmod +s toybox", the su command then works
as advertised. Including su --help. What's it's _complaining_ about is this
command needs suid root to function, and toybox is not installed with that.
The fact --help doesn't work in that case is still a problem, but it sounds like
the "Not installed suid root" error message a couple lines earlier is what it
should be printing in this case. (It's guarded by TOYBOX_DEBUG because it's
"your system is built wrong", not runtime user error, but you have that on so
should have seen it.)

Android doesn't actually build any of the TOYFLAG_ROOTONLY stuff... i
saw this on the host when i was trying to check all the --help output
for obvious mistakes the other day.

speaking of which, http://landley.net/toybox/help.html should probably
include the `toybox --version` output so it's clear what version of
toybox that page refers to...

Post by Rob Landley
Except that's not printing for the busybox multiplexer itself (which !=
toy_list; the standalone case shouldn't have CFG_TOYBOX_SUID set because then
the individual command binary either has the suid bit set or it doesn't and
we're not _dropping_ it), because the _multiplexer_ doesn't know (at this stage)
what command we're going to be running.
I suspect the "not installed right" error message should be printed here
instead, but need to redo the logic. Right now it's right from a security
perspective, but not from a usability perspective, and I kinda privilege the
former over the latter... :P
Queued up for next release...
Rob
P.S. I've meant for a while to have "make config-allsuid" and "config-nosuid"
targets so people can have two binaries and only install the suid bit on the one
containing the commands that needs them. I just haven't gotten around to it
because $DAYJOB. (And really it's a bit like the "make single" stuff because the
binaries would have to be named differently to be insalled next to each other...
toybox-suid and toybox-nosuid, so really the targets should be "make
toybox-allsuid" and "toybox-nosuid"... which is why it's on my todo list after
the kconfig rewrite.)
Rob

[Rob Landley]

Post by enh

Post by Rob Landley

Post by enh
$ ./toybox su --help
toybox: Not root (see "toybox --help")
not sure what the fix is there though.

Ok, dug into it some more.
If you "chown root:root toybox" and "chmod +s toybox", the su command then works
as advertised. Including su --help. What's it's _complaining_ about is this
command needs suid root to function, and toybox is not installed with that.
The fact --help doesn't work in that case is still a problem, but it sounds like
the "Not installed suid root" error message a couple lines earlier is what it
should be printing in this case. (It's guarded by TOYBOX_DEBUG because it's
"your system is built wrong", not runtime user error, but you have that on so
should have seen it.)

Android doesn't actually build any of the TOYFLAG_ROOTONLY stuff... i
saw this on the host when i was trying to check all the --help output
for obvious mistakes the other day.

Ah. That explains it. (I'm getting the message here when I enable CONFIG_DEBUG.)

Hmmm... on the one hand, I don't want "this should never happen" code in the
defconfig build. On the other hand, I want to make this easy to build systems
with (hence mkroot). Hmmm...

Post by enh
speaking of which, http://landley.net/toybox/help.html should probably
include the `toybox --version` output so it's clear what version of
toybox that page refers to...

You're right.

Really I should move the version info to version.h at the top level and have it
#included from toys.h. I didn't because clutter, but its _job_ is to change
every release, and that would make it available to help -a... Although a
standalone build of help -a isn't going to print anything useful anyway...

(It's always the _little_ decisions that are hard to make. Small differences are
harder to see. :P )

Rob