Bug 765

Summary:	$var expanded too early when using sudo -i.
Product:	Sudo	Reporter:	Roger Peppe <rogpeppe>
Component:	Sudo	Assignee:	Todd C. Miller <Todd.Miller>
Status:	ASSIGNED ---
Severity:	normal	CC:	lukeshu
Priority:	low
Version:	1.8.19
Hardware:	PC
OS:	Linux

Description Roger Peppe 2017-01-04 02:19:22 MST

Bug 413 has mostly been fixed, but the code in parse_args.c ("quote potential meta characters") specifically excludes $.

This means that $var references in the command can get
expanded too early.

I would expect the following two commands to have the same output
("foo is xxx") but that is not the case:

    % sudo -i bash -c 'FOO=xxx; echo foo is $FOO'
    foo is
    % sudo bash -c 'FOO=xxx; echo foo is $FOO'
    foo is xxx

Changing:

    /* quote potential meta characters */
    if (!isalnum((unsigned char)*src) && *src != '_' && *src != '-' && *src != '$')

to:
    /* quote potential meta characters */
    if (!isalnum((unsigned char)*src) && *src != '_' && *src != '-')

seems to fix the issue.

Comment 1 Todd C. Miller 2017-01-17 10:30:28 MST

Escaping of '$' was excluded as a fix for Bug 564.  Unfortuately, there is disagreement on what should be expanded when running commands via "sudo -i".

Comment 2 Luke Shumaker 2017-05-04 18:38:01 MDT

I believe that Bug 564 should have been marked "invalid".

However, if we do accept that it was a real bug, even then, the fix was a buggy half-measure.The fix to Bug 564 was a half-measure:

    # using C-style strings and arrays; the output of strace

    # works in the trivial case
     ["sudo", "-i", "/bin/echo", "$USER"] -> ["-bash", "--login", "-c", "\\/bin\\/echo $USER"]
    # but confusingly fails if we try deviating from that
    ["sudo", "-i", "/bin/echo", "${USER}"] -> ["-bash", "--login", "-c", "\\/bin\\/echo $\\{USER\\}"]
    ["sudo", "-i", "/bin/echo", "$@"] -> ["-bash", "--login", "-c", "\\/bin\\/echo $\\@"]

Ok, so what went wrong, and why I believe that Bug 564 was invalid:

There are two possible ways that the command could be passed around:

1. A list of strings to use as arguments; the same kind of thing that you would pass to execve(2).

2. A single string to be evaluated by the shell; the same kind of thing that you would pass to system(3).

Now, internally, sudo needs #1 normally, but needs #2 if given `-s` or `-i`.

So, the "rewrite argv" routine in parse_args.c was written to transform #1 in #2 when int needs #2 internally.  This way, it could expect to always consume #1 from the outside world; it would never consume #2.

Then, Bug 654 was opened by Bert, who expecting what sudo consumes to have attributes of #2.  They seemed to have a little confused thinking.  It wasn't totally unreasonable of them; the man page ambiguously suggests that with `-s` or `-i` that it might take #2.  However, it seems odd to me that they would expect `sudo -i echo '$USER'` to expand $USER, when `sudo echo '$USER'` (without the `-i` flag) it doesn't.  It really sounds to me like Bert just had code that happened to rely on Bug #413, and then filed a bug that his code broke when #413 was fixed.

Anyway, the "fix" to Bug 654 switched `-i` and `-s` from consuming #1 to consuming some weird mashup of #1 and #2 that doesn't really make sense to anybody, but satisfied Bert's testcase.

Now, the downside is: There's a bug here; and there's no way to fix it without breaking someone's code, as someone out there (and, err, (in my opinion) Bert in particular) is relying on the buggy behavior.