bug#21702: shell-quote-argument semantics and safety

[Taylan Ulrich Bayırlı/Kammer]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: taylanbayirli@gmail.com (Taylan Ulrich
>>      Bayırlı/Kammer)
>> Date: Sun, 18 Oct 2015 14:36:03 +0200
>> 
>> The documentation of shell-quote-argument only says
>> 
>>     Quote ARGUMENT for passing as argument to an inferior shell.
>> 
>> It's unclear for which shells this is supposed to work.
>
> I fixed the doc string to clarify that this function works correctly
> with the system's standard shell.
>
>> In a recent thread in emacs-devel, it has been demonstrated that if
>> the result is passed to csh, it can allow an attacker to execute an
>> arbitrary shell command
>
> As I understand it, csh is not the standard shell on Posix systems, so
> the fixed doc string now says not to expect it to work with csh.
>
>> The function should clearly document
>> 
>>     1) for which shells will the quoting work absolutely, i.e. lead to
>>     the given string to appear *verbatim* in an element of the ARGV of
>>     the called command,
>> 
>>     2) optionally, for which shells will the quoting at least prevent
>>     code injection,
>> 
>>     3) optionally, for which shells and character sets for ARGUMENT will
>>     the quoting work absolutely,
>> 
>>     4) optionally, for which shells and character sets for ARGUMENT will
>>     the quoting at least prevent code injection,
>> 
>>     5) optionally, for which shells will the quoting work at all even if
>>     it provides no clear semantics, such that one can at least use it
>>     with data coming from trusted sources (e.g. other parts of Emacs's
>>     source code, or the user sitting in front of Emacs), where it's the
>>     user's/programmer's responsibility to stick to values for ARGUMENT
>>     that are intuitively known to be unproblematic even if the character
>>     set isn't well-defined.
>> 
>> Currently #5 seems to be implied for all shells, for lack of further
>> documentation.  Possibly, the function was never meant to be used with
>> untrusted data, but there's no warning against doing so either.
>
> I thin 1) is now covered, and the rest are optional.  In particular,
> our way to provide better safety is not by documenting APIs that could
> be maliciously abused, but by marking the related variables as unsafe
> unless they have special predefined values.  So I don't think we
> should extend this particular doc string with safety information.

Hm, there seems to be a fundamental difference in mindset here in how
one might use Elisp.

I'd like to point out that (in the most extreme cases) people have
actually been writing web servers and other such programs in Elisp for
which one would normally use a general-purpose language.

That is, "APIs that could be maliciously abused" is not the right way to
look at it.  It's not about the Elisp programmer abusing the API, it's
about a malicious data source exploiting a (potential) flaw in an Elisp
function, which Elisp programmers have relied on and thus made their
programs vulnerable to code injection.


That's why I was being so careful with regard to the safety guarantees
of the "shell-quasiquote" package I contributed.  I would like people to
be able to use that as part of a general-purpose Elisp language, and so
being safe against code injection is an absolute must.  They might after
all use it as part of a network-facing service.


Actually that might also apply when using e.g. TRAMP, which also
communicates with remote hosts and is a normal part of Emacs.  I've been
told it receives file names from remote hosts and passes them through
shell-quote-argument before giving them to a shell.  So maybe my
concerns apply there as well.


Given that, "I think 1) is now covered" is not very relieving to hear.
It amounts to "I think this is safe against code injection" which is
rather alarming to hear.  Either it's very confidently known to be safe
and so one may use it for network-facing code, or it's not confidently
known to be safe and so one shouldn't use it for network-facing code.
This should be documented clearly especially so that users who aren't
very aware of injection attacks won't nonchalantly use the function for
their network-facing code (when the function isn't known to be safe for
this), but also so that users who are aware of such issues know they can
use the function and don't instead invent their own thing (when it is
known to be safe).

Does that make sense?

Taylan