Structurally fixing command injection bugs

Structurally fixing command injection bugs

[Vasilij Schneidermann]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

I've come across a few recent bugfixes arising from the same underlying problem
recently:

- Command injection in etags via system(3): CVE-2022-45939
- Command injection in htmlfontify.el via `shell-command-to-string`
- Command injection in ruby-mode.el via `shell-command-to-string`

The issue is well-known: Passing user input containing shell control
characters to system(3) is dangerous. Quoting the argument strings is a
band-aid solution. The text-book solution is to avoid using the shell in
the first place whenever possible. Emacs even provides a convenient
function for this, `process-lines`. It does not use the shell, accepts
several argument strings, raises errors (rather than failing silently)
and returns its output as a list of lines, thereby removing the need for
removing the trailing newline.

I see several options for moving forward:

- Keep using `shell-command-to-string` and `shell-quote-argument`
- Migrate existing use of `shell-command-to-string` to `process-lines` 
- Come up with a different replacement working much like
  `process-lines`, but returning a string instead (I have no idea what
  an appropriate name would be, maybe `command-to-string`?)

PS: Where should I report analogous misuse of `shell-command-to-string`?
I cannot submit patches currently because I've changed employers and
need to renew copyright assignment, again (that would be the third time
already).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Structurally fixing command injection bugs

[lux]

On Wed, 2023-02-22 at 11:08 +0100, Vasilij Schneidermann wrote:
> I've come across a few recent bugfixes arising from the same
> underlying problem
> recently:
> 
> - Command injection in etags via system(3): CVE-2022-45939
> - Command injection in htmlfontify.el via `shell-command-to-string`
> - Command injection in ruby-mode.el via `shell-command-to-string`
> 
> The issue is well-known: Passing user input containing shell control
> characters to system(3) is dangerous. Quoting the argument strings is
> a
> band-aid solution. The text-book solution is to avoid using the shell
> in
> the first place whenever possible. Emacs even provides a convenient
> function for this, `process-lines`. It does not use the shell,
> accepts
> several argument strings, raises errors (rather than failing
> silently)
> and returns its output as a list of lines, thereby removing the need
> for
> removing the trailing newline.
> 
> I see several options for moving forward:
> 
> - Keep using `shell-command-to-string` and `shell-quote-argument`
> - Migrate existing use of `shell-command-to-string` to `process-
> lines` 
> - Come up with a different replacement working much like
>   `process-lines`, but returning a string instead (I have no idea
> what
>   an appropriate name would be, maybe `command-to-string`?)
> 
> PS: Where should I report analogous misuse of `shell-command-to-
> string`?
> I cannot submit patches currently because I've changed employers and
> need to renew copyright assignment, again (that would be the third
> time
> already).

You can send to bug-gnu-emacs@gnu.org

Re: Structurally fixing command injection bugs

[Vasilij Schneidermann]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On 02/22/23 at 06:20pm, lux wrote:
> > PS: Where should I report analogous misuse of `shell-command-to-
> > string`?  I cannot submit patches currently because I've changed
> > employers and need to renew copyright assignment, again (that would
> > be the third time already).
> 
> You can send to bug-gnu-emacs@gnu.org

Yes, usually I'd just use M-x report-emacs-bug, but in this case it's
different because I plan to develop proof of concept code (PoC) and
submit it to the responsible maintainer for verifying the vulnerability
and the fix. Publicly disclosing PoC code is usually frowned upon, no
matter how trivial/exploitable the issue is.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Structurally fixing command injection bugs

[lux]

On Wed, 2023-02-22 at 11:08 +0100, Vasilij Schneidermann wrote:
> I've come across a few recent bugfixes arising from the same
> underlying problem
> recently:
> 
> - Command injection in etags via system(3): CVE-2022-45939
> - Command injection in htmlfontify.el via `shell-command-to-string`
> - Command injection in ruby-mode.el via `shell-command-to-string`
> 
> The issue is well-known: Passing user input containing shell control
> characters to system(3) is dangerous. Quoting the argument strings is
> a
> band-aid solution. The text-book solution is to avoid using the shell
> in
> the first place whenever possible. Emacs even provides a convenient
> function for this, `process-lines`. It does not use the shell,
> accepts
> several argument strings, raises errors (rather than failing
> silently)
> and returns its output as a list of lines, thereby removing the need
> for
> removing the trailing newline.
> 
> I see several options for moving forward:
> 
> - Keep using `shell-command-to-string` and `shell-quote-argument`
> - Migrate existing use of `shell-command-to-string` to `process-
> lines` 
> - Come up with a different replacement working much like
>   `process-lines`, but returning a string instead (I have no idea
> what
>   an appropriate name would be, maybe `command-to-string`?)

I've been working on these things lately. You are welcome to submit
patches to make Emacs more secure.

Re: Structurally fixing command injection bugs

[lux]

On Wed, 2023-02-22 at 11:34 +0100, Vasilij Schneidermann wrote:
> On 02/22/23 at 06:20pm, lux wrote:
> > > PS: Where should I report analogous misuse of `shell-command-to-
> > > string`?  I cannot submit patches currently because I've changed
> > > employers and need to renew copyright assignment, again (that
> > > would
> > > be the third time already).
> > 
> > You can send to bug-gnu-emacs@gnu.org
> 
> Yes, usually I'd just use M-x report-emacs-bug, but in this case it's
> different because I plan to develop proof of concept code (PoC) and
> submit it to the responsible maintainer for verifying the
> vulnerability
> and the fix. Publicly disclosing PoC code is usually frowned upon, no
> matter how trivial/exploitable the issue is.

At present, there is no better channel.I feel open the PoCs good for
developers to understand, fix vulnerability and improving the code
security. Make the problem public instead of hiding it.

I also want to hear the thoughts of others.

Re: Structurally fixing command injection bugs

[Gregory Heytings]

>
> I plan to develop proof of concept code (PoC) and submit it to the 
> responsible maintainer for verifying the vulnerability and the fix. 
> Publicly disclosing PoC code is usually frowned upon, no matter how 
> trivial/exploitable the issue is.
>

If you're unsure what to do, you can always send a private mail to the two 
head maintainers: Eli Zaretskii <eliz@gnu.org> and Lars Ingebrigtsen 
<larsi@gnus.org>.

Re: Structurally fixing command injection bugs

[Jim Porter]

On 2/22/2023 2:08 AM, Vasilij Schneidermann wrote:
> I see several options for moving forward:
[snip]
> - Come up with a different replacement working much like
>    `process-lines`, but returning a string instead (I have no idea what
>    an appropriate name would be, maybe `command-to-string`?)

Where possible, I think this is probably best, but there are likely 
times where you really want the benefits of a shell. For example, what 
if the command you want to run involves a pipeline? One option for this 
would be to enhance 'shell-command-to-string' so that you can pass it 
arguments that will be correctly escaped when substituted into the final 
command string. For example:

   (shell-command-to-string "cat %s | rev" "file with a $weird name.txt")
   ;; Runs "cat 'file with a $weird name.txt' | rev"

(This is similar to what you might do when parameterizing an SQL query 
or something.)