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

[taylanbayirli@gmail.com (Taylan Ulrich Bayırlı/Kammer)]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: taylanbayirli@gmail.com (Taylan Ulrich Bayırlı/Kammer)
>> Cc: 21702@debbugs.gnu.org
>> Date: Sun, 18 Oct 2015 21:12:34 +0200
>> 
>> 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.
>
> Item 1 was this:
>
>> >> 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,
>
> There's nothing about safety here, only about correctness.  That is
> the aspect that I think is now covered, as the doc string now says for
> which shells one can have correct results.

Usually it's indeed correctness that protects against injection attacks.
A quoting mechanism that's correct is automatically safe.  Another way
to make it safe would be to error when the given string contains
characters outside of a limited character set.

Either way, the safeness should be documented clearly, either implicitly
through a clear documentation of the correctness, or explicitly.

In your patch, correctness is implied, but the complexity of the problem
domain (and thus the function itself) and the importance of possible
repercussions of an incorrect implementation leave clearer documentation
to be desired.  While any function is really implied to be correct by
its existence, any function is also implied to very possibly contain
bugs, as is natural for software.  In many cases these bugs are
unimportant.  In this case not.

I would propose something along the lines of:

    It is guaranteed that ARGUMENT will be parsed as a single token by
    shells X, Y, and Z, as long as it is separated from other text via a
    delimiter in the syntax of the respective shell.

(That's even better than the patch I proposed, which didn't mention the
problem of delimiting.)

I think it's also important to provide some explicit enumeration of
shells for which the function is safe, because the systems Emacs
supports may change over time, and there is no guarantee that a change
to this function will not entail bugs.  If we add wording like the
above, then any programmer who sits down to expand the function's
semantics to another shell will be forced to think very hard about what
they're doing; otherwise they might try to do a "good enough" job but
not make sure that all edge-cases are handled.  "Designed to work with"
is after all not an absolute claim of correctness and absence of bugs.

>> 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?
>
> Maybe it does, but only if we start documenting these aspects
> project-wide.  It makes little sense to me to do that for a single
> API, and not an important one at that.  But that's me.

This is an API which if its implementation is imperfect will result in
programs prone to code injection attacks when these programs face
untrusted input sources.  Why do you say it's not an important one?

Taylan