Commas in process output are used as separators in brace expansion

[mqudsi]

I originally thought that this bug affected only fish 3.0 with the changes to brace expansion that imbued it with significantly more functionality and flexibility, but it turns out that even fish 2.7.1 is affected by this bug.

~> set -l tokens "foo,bar"
~> printf "'%s' " {$tokens}
'foo,bar'
~> printf "'%s' " {(echo $tokens)}
'foo' 'bar'

The output of a process should never be evaluated in this fashion, as this poses a major security risk.

[faho]

as this poses a major security risk.

I would actually dispute that characterization - the output is not "evaluated" as code. It's "parsed" as a certain form of expression.

You can't use this to rm anything or cat anything else. There's really no way for anyone malicious to abuse this.

So I'd merely call it a bug.

[mqudsi]

It's not arbitrary code execution, but it it is a vulnerability. Imagine privileged code that executes some command based off of usernames, perhaps something like a script that changes a user's password. It's pretty much along the lines of classical SQL injection vulnerabilities due to lack of argument escaping.

Let's say there's are sensitive resources located on a server that can only be accessed by the user and shared resources that can be accessed by anyone. Charlie wants to gain access to Alice's file and finds somewhere where the sysadmin unwittingly wrote the following code which runs in an elevated context, assuming it to be safe:

for file in {(get-username), shared}.txt
    echo $file:
    cat $file
end

By creating an account with the name bob,alice in the system, Charlie gets access to alice.txt because {(get-username), shared}.txt now evaluates to [bob.txt, alice.txt, shared.txt] instead of [bob,alice.txt shared.txt].

Security. Fun stuff.

[faho]

By creating an account with the name bob,alice in the system

If Charlie can create accounts, then why not create alice directly?

[mqudsi]

If Charlie can create accounts, then why not create alice directly?

Because alice is already taken by Alice. (Imagine these files are created for each user when their account is created in the system or something.)

Of course this was just an arbitrary example.

[anordal]

If this was nothing more than yet another way to split a string into an array, then it wouldn't be a security issue.

The crux of the point is that you don't expect it to happen. Or do you? That's one question.

printf '%s\n' rm -rf /home/{(echo alice,bob),charlie}

Expected:

rm
-rf
/home/alice,bob
/home/charlie

Actual:

rm
-rf
/home/alice
/home/bob
/home/charlie

Another premise for this to be a security issue is whether it would be forgivable to use this funky {()} syntax when it is important to not split the command output. I think not, because if the splitting of alice,bob is an issue, then the splitting of alice\nbob would also be, but that's the usual behavior of command substitutions.

printf '%s\n' rm -rf /home/{(printf alice\nbob),charlie}

Expected:

Nobody expects anything meaningful out of this (therefore, you wouldn't write it).

Actual:

rm
-rf
/home/alice
/home/charlie
/home/bob
/home/charlie

[ridiculousfish]

This is a surprising behavior but it's not a security issue. Someone might hypothetically expect different behavior and introduce a vulnerability, but that's true for any surprising behavior.

We should fix this, but it doesn't have to go into fish 3.0. Moving the milestone and retitling ("unsafe evaluation" makes it sound like shellshock or something)