`fexpand` filename `%`/`#` expansion is shell-unsafe → command injection via `!cmd %`

[mat-mo]

The ! shell-escape command in less (command.c:322 A_SHELL branch) processes the user-typed string with fexpand(), then hands the result to lsystem():

case A_SHELL: {
    ...
    if (*cbuf != '!')
    {
        if (shellcmd != NULL)
            free(shellcmd);
        shellcmd = fexpand(cbuf);   /* (1) expands % and # */
    }
    ...
    lsystem(shellcmd, done_msg);
    break;
}

fexpand() substitutes % with the current filename and # with the previous filename, using xcpy_filename():

static void xcpy_filename(xcpy *xp, constant char *str)
{
    /* If filename contains spaces, quote it
     * to prevent edit_list from splitting it. */
    lbool quote = (strchr(str, ' ') != NULL);
    if (quote)
        xcpy_char(xp, openquote);
    for (;  *str != '\0';  str++)
        xcpy_char(xp, *str);
    if (quote)
        xcpy_char(xp, closequote);
}

The function quotes only when the filename contains a space. Filenames containing other shell metacharacters (;, |, &, $, <, >, backticks, (, ), etc.) but no space pass through verbatim. When lsystem() then hands the assembled command to $SHELL -c, the inner shell parses these as metacharacters.

lsystem()'s call to shell_quote(cmd) protects only the outer-shell level — the inner shell sees the original cmd and parses its metachars normally.

Reproduction

$ mkdir -p /tmp/metafile_test
$ echo 'innocent' > '/tmp/metafile_test/foo;rm'
$ less '/tmp/metafile_test/foo;rm'
:!cat %

cat foo;rm is what reaches the inner shell — the ; is unquoted because the filename contains no space.

In a more realistic attack model the user opens a directory of files (e.g. a cloned repo) containing one with shell metacharacters in its name, then uses !cmd % against it. Filenames with metacharacters but no spaces are uncommon but achievable in attacker-controlled directories.

I think using shell_quote() rather than the space-only check is safer, similar to what is done with the %g prompt protochar in prompt.c:342 which already uses shell_quote() correctly for the v (visual) editor path; aligning xcpy_filename with that approach removes the asymmetry.

Of course, if the user uses single quotes around the CMD command the problem is avoided.

[avih]

Of course, if the user uses single quotes around the CMD command the problem is avoided.

If I understand the assertion correctly, then I don't think it's avoided. For instance if the file name is foo';echo PAWNED;'. Single pair of prefix/suffix single quotes don't safely-quote arbitrary values. They only safely-quote values without single quotes.

[mat-mo]

Of course, if the user uses single quotes around the CMD command the problem is avoided.

If I understand the assertion correctly, then I don't think it's avoided. For instance if the file name is foo';echo PAWNED;'. Single pair of prefix/suffix single quotes don't safely-quote arbitrary values. They only safely-quote values without single quotes.

Fair point, and I agree with this expansion. I would admit I didn't test this specific case.

[avih]

@gwsw by the way, correct me if I'm wrong, but I don't think there's a different version of shell_quote for windows?

If that's correct, then the current shell_quote likely completely doesn't work for windows, which has very different (and very strange) quoting rules.

Here's a version of windows shell-quote which I wrote for the tiny-c compiler https://repo.or.cz/tinycc.git/commitdiff/1cf33feb0f4494a53d7730a164fa8bbe51f28f1a and includes a link to the Microsoft quoting docs.

Feel free to take it as is (except replacing tcc_malloc with malloc or something else, and ensure the allocation succeeded).

This code is battle-proven, and a variant of it (also based on my code) is the main windows-quoting routine in busybox-w32 as well: https://github.com/rmyorston/busybox-w32/blob/f9e24f4766afdf2c0a1011097bd2fd6e01fd39e0/win32/process.c#L115-L150

[avih]

I would admit I didn't test this specific case.

That makes both of us! ;)

[avih]

I don't think there's a different version of shell_quote for windows?

Hmm.. I think there isn't, but it's not used from shellcmd or lsystem (I think nothing is quoted?). However, it is used in other places on windows, like edit_ifile, open_altfile, and maybe more. Not sure what it would do there. The whole shell-quoting-on-windows subject in "less" is likely worth some review.

[gwsw]

It's unclear to me what the desired behavior is for expanding the % character in a ! command. Quoting it would indeed be safer, but in some cases might not be what the user wants. Admittedly it's hard to think of such cases. Things are more under the user's control in this situation than in some other attacks, since the user has to deliberately type in the command. If the filename contains weird characters, the user can get a shell-quoted name by using the # command rather than the ! command, and use %g in the shell string.

I'll note that vi, vim and nvim all have the same issue when they expand % in a :! command; all of them expand % as the unquoted filename, and all of them will execute back-ticked commands in a filename if you do :!echo %.

The whole shell-quoting-on-windows subject in "less" is likely worth some review.

This is undoubtedly true. As a first step I'll take a look at your Windows code.

[avih]

This is undoubtedly true. As a first step I'll take a look at your Windows code.

Right. Both links are the same code, with only trivial coding style differences. The tcc code has a comment explaining the quoting behavior/requirements, which the busybox-w32 code lacks.

This function takes an arbitrary string and returns a newly-allocated windows-shell-quoted version of that string.

[gwsw]

@avih I have ported your Windows quoting function in ba58a22 in the post700 branch. The main change, other than minor stylistic tweaks, was to search for spaces/tabs in a separate pass.

One thing I noticed when testing is if I open a filename containing double quotes, each double-quote char is somehow converted to a U+F022 char. I think this happens in _findfirst/_findnext in lglob. Nevertheless, using this converted filename correctly opens the file. Do you know what's happening here?

[avih]

One thing I noticed when testing is if I open a filename containing double quotes, each double-quote char is somehow converted to a U+F022 char. I think this happens in _findfirst/_findnext in lglob. Nevertheless, using this converted filename correctly opens the file. Do you know what's happening here?

No idea, and I didn't look at _findfirst/_findnext, but that function doesn't modify any char. The most it does is add backslashes, and maybe one pair of surrounding double quotes.

It does sound very weird though, and U+F022 is private use area. Is your mingw UCRT? (and hence with Unicode file names support in "less") or msvcrt? (without unicode file names)

[avih]

Also, we should probably move the windows quoting discussion elsewhere. I happened to look at it because shell_quote was mentioned, but it's really unrelated to this issue with !cmd %.

[gwsw]

in some cases might not be what the user wants.

On further thought, I think that it would be better to use the shell-quoted filename as the replacement. In fact, I think it is a serious enough vulnerability that it should go into the next release. I have implemented this in 9f2e50f.