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

[discussion] Fix -- handling in su #256

Closed
wants to merge 1 commit into from

Conversation

hallyn
Copy link
Member

@hallyn hallyn commented May 17, 2020

Su incorrectly tries to handle some flexible usage, including

su -c command
su -c command user
su -c command - user
su -c command -- args
su -c command user -- args

etc. But it ignored the fact that if getopt_long runs into a
--, it will leave optind at the next word. So

su -c command -- arg

tried to parse 'arg' as a user. Fix that by checking for it.

Signed-off-by: Serge Hallyn shallyn@cisco.com

Su incorrectly tries to handle some flexible usage, including

su -c command
su -c command user
su -c command - user
su -c command -- args
su -c command user -- args

etc.  But it ignored the fact that if getopt_long runs into a
--, it will leave optind at the next word.  So

su -c command -- arg

tried to parse 'arg' as a user.  Fix that by checking for it.

Signed-off-by: Serge Hallyn <shallyn@cisco.com>
@vcaputo
Copy link
Contributor

vcaputo commented May 28, 2020

@hallyn I'm short on time for volunteer FOSS stuff currently...

I briefly went down this route before replacing getopt in #254. From what I recall, there was ambiguity created by getopt moving the "--" around in the argv, but maybe I had an unrelated bug misleading me.

If you don't find any problems with using getopt then great! I found it to initially work in testing --exec, but broken in more of the edge cases like missing vs. present user name.

Feel free to rebase or use whatever commits from my PR, it makes no difference to me, I'd just like the --exec/-e feature in upstream su.

@vcaputo
Copy link
Contributor

vcaputo commented May 28, 2020

Ok, I took a few minutes to poke at su with this PR applied, and reproduced the malfunction which sent me down the "there's a reason I stopped using getopt decades ago" route in #254 .

Start with a simple script at /tmp/foo for instrumenting the euid and passed-through args:

#!/bin/bash

whoami
echo args: "$@"

Now invoking #256 su with this valid commandline for running /tmp/foo as foouser, it spuriously passes the switched-to user as $1, and runs as root requiring root's password.

./su -c '/tmp/foo $0 $1 $2' - foouser -- first second third
Password: 
root
args: - foouser first

Now, repeating this exercise with #254 instead:

./su -c '/tmp/foo $0 $1 $2' - foouser -- first second third
Password: 
foouser
args: first second third

I hope that helps clear up the nature of the problem.

Edit:

Note the issue persists omitting the single '-' as well but keeping the user, it just shifts the erroneously passed-in argv user to $0, still runs unexpectedly as root:

./su -c '/tmp/foo $0 $1 $2' foouser -- first second third
Password: 
root
args: foouser first second

Using #254:

./su -c '/tmp/foo $0 $1 $2' foouser -- first second third
Password: 
foouser
args: first second third

@hallyn hallyn closed this May 30, 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.

None yet

2 participants