Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Add --exec / -e to su for shell-bypass #254

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vcaputo
Copy link
Contributor

@vcaputo vcaputo commented May 10, 2020

This adds the ability to run commands via su without an interpretive shell, enabling a thinner user-switching veneer over execve() mode of operation.

In preparation for supporting --exec I was testing the robustness
of "--" handling and it became apparent that things are currently
a bit broken in `su`.

Since "--" is currently of limited utility, as the subsequent
words are simply passed to the shell after "-c","command_string",
it seems to have gone unnoticed for ages.

However, with --exec, it's expected that "--" would be an almost
required separator with every such usage, considering the
following flags must be passed verbatim to execve() and will
likely begin with hyphens looking indistinguishable from any
other flags in lieu of shell interpolation to worry about.

For some practical context of the existing situation, this
invocation doesn't work today:
```
  $ su --command ls -- flags for shell
  No passwd entry for user 'flags'
  $
```

This should just run ls as root with "flags","for","shell"
forwarded to the shell after "-c","ls".

The "--" should block "flags" from being treated as the user.
That particular issue isn't a getopt one per-se, it's arguably
just a bug in su.c's implementation.

It *seemed* like an easy fix for this would be to add a check if
argv[optind-1] were "--" before treating argv[optind] as USER.

But testing that fix revealed getopt was rearranging things when
encountering "--", the "--" would always separate the handled
opts from the unhandled ones.  USER would become shifted to
*after* "--" even when it occurred before it!

If we change the command to specify the user, it works as-is:
```
  $ su --command ls root -- flags for shell
  Password:
  testfile
  $

```

But what's rather surprising is how that works; the argv winds up:

"su","--command","ls","--","root","flags","for","shell"

with optind pointing at "root".

That arrangement of argv is indistinguishable from omitting the
user and having "root","flags","for","shell" as the stuff after
"--".

This makes it non-trivial to fix the bug of omitting user
treating the first word after "--" as the user, which one could
argue is a potentially serious security bug if you omit the user,
expect the command to run as root, and the first word after "--"
is a valid user, and what follows that something valid and
potentially destructive not only running in unintended form but
as whatever user happened to be the first word after "--".

So, it seems like something important to fix, and getopt seems to
be getting in the way of fixing it properly without being more
trouble than replacing getopt.

In disbelief of what I was seeing getopt doing with argv here, I
took a glance at the getopt source and found the following:

```
      /* The special ARGV-element '--' means premature end of options.
	 Skip it like a null option,
	 then exchange with previous non-options as if it were an option,
	 then skip everything else like a non-option.  */

      if (d->optind != argc && !strcmp (argv[d->optind], "--"))
```

I basically never use getopt personally because ages ago it
annoyed me with its terrible API for what little it brought to
the table, and this brings it to a whole new level of awful.
Mechanical rename distinguishing this variable from intended changes
supporting executing commands without using an interpretive shell
(i.e. no '/bin/sh -c').
It's now possible to run commands as other users without shell
interpolation by using "--exec":

Read /etc/shadow as root without specifying user:
```
su --exec /bin/cat -- /etc/shadow
```

Or specify user:
```
su --exec /bin/cat root -- /etc/shadow
```
@vcaputo vcaputo changed the title [RFC] Add --exec-command / -e to su for shell-bypass [RFC] Add --exec / -e to su for shell-bypass May 12, 2020
@hallyn
Copy link
Member

hallyn commented May 16, 2020

Thanks - this looks good to me.

I'm a bit skiddish about the possibility that, however correct it is, it might break someone's usage in some script somewhere from way back when. So I'm going to take another look with fresh eyes soon, before I merge.

@hallyn
Copy link
Member

hallyn commented May 16, 2020

Actually, you marked this as 'draft' - are you intending to make more changes?

@vcaputo
Copy link
Contributor Author

vcaputo commented May 16, 2020

@hallyn Well, I didn't update the docs in this PR, which I presumed would be necessary before merging. So I figured leave it as an RFC draft in case we need to bikeshed a bit on the flag naming or anything else before updating the docs/man page.

But no, I didn't have any planned future modifications to the code.

My only apprehension with this PR is I fairly quickly did the getopt replacement in frustration and didn't make an exhaustive effort to verify I replicated exactly all of the error messages getopt might produce with bad flags. I made some effort, but I didn't go crazy with it, like I didn't scrutinize the getopt code to run down all the error paths and ensure I replicated all of them. I'm not sure how important that aspect is.

@hallyn
Copy link
Member

hallyn commented May 17, 2020

Hi,

in PR #256 I proposed an alternate fix for the 'user' handling in su.

While doing this I noticed that the argument handling for -c is wrong anyway - it appears if you do

su -c /bin/cat -- /somefile

It will do

$shell -c /bin/cat /somefile

but it would need to do

$shell -c "/bin/cat /somefile"

for the arguments to get sent to the command. It seems an easy enough fix, but I didn't do it just now.

@hallyn
Copy link
Member

hallyn commented May 26, 2020

@vcaputo how would you feel about rebasing this patch set on top of PR#256 instead of patch 1/3 ?

@hallyn
Copy link
Member

hallyn commented May 28, 2020

ping? I may go ahead and rebase it myself this weekend if you don't have time but don't object.

@vcaputo
Copy link
Contributor Author

vcaputo commented May 28, 2020

See comments in #256

@vcaputo
Copy link
Contributor Author

vcaputo commented May 30, 2020

BTW as demonstrated in #256, the way the POSIX shell handles subsequent arguments following -c foo makes the existing su -c foo -- args to foo very awkward and unintuitive to anyone actually attempting to make use of --, even when there aren't su bugs interfering.

I don't know anyone who would naturally think to add explicit parameters suffixing the -c foo argument, but that's what's needed to actually plumb the arguments through the intermediate shell. i.e. su -c 'foo $0 $1' -- arg arg etc... At least not without some reading of the shell man page:

       -c        If the -c option is present, then commands are read from  the
                 first non-option argument command_string.  If there are argu‐
                 ments  after  the  command_string,  the  first  argument   is
                 assigned  to  $0  and any remaining arguments are assigned to
                 the positional parameters.  The assignment  to  $0  sets  the
                 name  of  the  shell, which is used in warning and error mes‐
                 sages.

predicated on the realization that there's another shell gating things in the middle there, so go look at its man page about its -c handling.

So, if some form of --exec lands, I'd like to see mentioning something about this in the man page - perhaps even promoting --exec use over --command or demoting --command in the manual as something you should only use if shell features are required. The way argument passing behaves with --exec is much more sane IMHO.

@HarmtH
Copy link
Contributor

HarmtH commented May 31, 2020

I don't think this PR is needed, as sh -s already does this?

@vcaputo
Copy link
Contributor Author

vcaputo commented May 31, 2020

@HarmtH I presume you meant su -s.

It's not equivalent because that's su executing an arbitrary program as an interactive shell. When su --command is used, the controlling tty is closed. When su --shell without a command is used, the controlling tty remains.

And if you attempt to somehow contort this into working by mixing --command and --shell, you'll find there's always a -c separating what you passed to --shell and --command. Because the argument of --shell is expected to be a POSIX-compliant shell accepting -c command.

su simply doesn't support executing programs without a shell.

@HarmtH
Copy link
Contributor

HarmtH commented May 31, 2020

Yes, I meant su -s indeed 😃. Can you give me an example of the controlling tty being closed? I seem to be able to ^C programs with ether su -s or su -c.

@vcaputo
Copy link
Contributor Author

vcaputo commented May 31, 2020

Here's the relevant code in su https://github.com/shadow-maint/shadow/blob/master/src/su.c#L1110

@hallyn
Copy link
Member

hallyn commented Jun 1, 2020

BTW as demonstrated in #256, the way the POSIX shell handles subsequent arguments following -c foo makes the existing su -c foo -- args to foo very awkward and unintuitive to anyone actually attempting to make use of --, even when there aren't su bugs interfering.

Yes, I thought I'd mentioned that in my pr but I guess I edited that out in a rebase.

I'm going to think just a bit more about this and then probably merge it and update the manpage.

@vcaputo
Copy link
Contributor Author

vcaputo commented Jun 1, 2020

I'm in no hurry - it'll be forever before this percolates into my molasses-like distro anyways. What started out as just wanting to add a shell bypass mode to su turned into finding a dusty skeleton in this old closet.

Honestly, I didn't exhaustively test this PR, because it was apparent that there'd appropriately be some friction, and likely discussion and iteration necessary if it would land at all. I just made things presentable enough to try move things forward, get the ball rolling on upstream understanding what had been uncovered.

So definitely take care in sanity checking this stuff before merging. I'll see if I can make some time to do more testing of this PR myself as well.

Let's not replace an existing argument handling bug with new ones out of haste, especially with a suid binary.

@hallyn
Copy link
Member

hallyn commented Jun 1, 2020

Yeah - I was planning on adding some tests based on the ones you showed above.

Thanks for this.

@HarmtH
Copy link
Contributor

HarmtH commented Jun 1, 2020

Here's the relevant code in su https://github.com/shadow-maint/shadow/blob/master/src/su.c#L1110

I meant an example which showes the necessity / use case of the controlling terminal being closed 😅 Maybe unnecessary to say, but having a controlling terminal and being an interactive shell are two entirely different concepts. You can have a non-interactive shell with a controlling terminal, and you can have an interactive shell without a controlling terminal (although you won't have job control then).

Btw, as soon as you give additional arguments to a (non-)shell passed to -e, the controlling terminal also already gets closed, see https://github.com/shadow-maint/shadow/blob/master/src/su.c#L832

brauner pushed a commit that referenced this pull request Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants