Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#8802 closed bug (invalid)

createProcess implictlitly escapes and quotes command line parameters

Reported by: jstolarek Owned by:
Priority: high Milestone: 7.8.1
Component: libraries/process Version: 7.9
Keywords: Cc:
Operating System: Linux Architecture: Unknown/Multiple
Type of failure: Runtime crash Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

I've just run into severe usability issue with the process library. It turns out that it performs quoting and escaping of command line parameters. The result is that it is impossible to run many shell commands that contain "unsafe" characters:

diff <(echo $ENV_FOO) <(echo $ENV_BAR)
awk 'NR<=10' Foo.txt

because they are turned into some nonsense. The culprit is translate function in libraries/process/System/Process/Internal.hs. I'm not fixing this myself as I assume there was some motivation behind escaping and quoting. I'm assigning high priority as it makes impossible to create shell processes correctly.

Attachments (1)

T8802.hs (681 bytes) - added by jstolarek 14 months ago.
Failing call to diff

Download all attachments as: .zip

Change History (9)

comment:1 Changed 14 months ago by nomeata

I guess you talking about runCommand (which goes via the shell), and not runProcess (which executes programs directly)?

comment:2 Changed 14 months ago by jstolarek

I am thinking about createProcess. Both runCommand and runProcess are planned to be deprecated (haddock documentation already lists them in section "Old deprecated functions").

comment:3 follow-up: Changed 14 months ago by nomeata

I see. But the translate function is only used on Windows, for reasons explained in -- Escaping commands for shells in System.Process.Internals – is your bug report about Windows? Can you maybe give a concrete example of what should work and does not, so that it can be turned into a testcase?

comment:4 in reply to: ↑ 3 Changed 14 months ago by jstolarek

  • Operating System changed from Unknown/Multiple to Linux

Replying to nomeata:

But the translate function is only used on Windows

No:

translate :: String -> String
#if mingw32_HOST_OS
translate xs = '"' : snd (foldr escape (True,"\"") xs)
  where escape '"'  (_,     str) = (True,  '\\' : '"'  : str)
        escape '\\' (True,  str) = (True,  '\\' : '\\' : str)
        escape '\\' (False, str) = (False, '\\' : str)
        escape c    (_,     str) = (False, c : str)
        -- See long comment above for what this function is trying to do.
        --
        -- The Bool passed back along the string is True iff the
        -- rest of the string is a sequence of backslashes followed by
        -- a double quote.
#else
translate "" = "''"
translate str
   -- goodChar is a pessimistic predicate, such that if an argument is
   -- non-empty and only contains goodChars, then there is no need to
   -- do any quoting or escaping
 | all goodChar str = str
 | otherwise        = '\'' : foldr escape "'" str
  where escape '\'' = showString "'\\''"
        escape c    = showChar c
        goodChar c = isAlphaNum c || c `elem` "-_.,/"
#endif

is your bug report about Windows

No, it's about Linux (does Windows even have awk?)

Changed 14 months ago by jstolarek

Failing call to diff

comment:5 Changed 14 months ago by jstolarek

The example I just attached fails because parameters to diff are surrounded with ' - see comment in the file. This is exactly what translate function does.

comment:6 follow-up: Changed 14 months ago by nomeata

  • Resolution set to invalid
  • Status changed from new to closed

Well, if you check where translate is being used: It is only used on Windows (and on Linux when showing the command).

In your example you are using proc, where the main feature is precisely that it is not being passed through the shell. This is important: With proc it is safe to call proc "echo" [possibly_malicous_string]. So it is correct behaviour that you cannot used shell features with proc. Any security-aware code should only use proc or be very careful when using shell.

If you want shell features, use shell instead of proc.

comment:7 in reply to: ↑ 6 Changed 14 months ago by jstolarek

Replying to nomeata:

Well, if you check where translate is being used: It is only used on Windows

Hm... looks like you're right. In that case which part of the code quotes parameters to proc? Because this clearly is performed at some point.

you are using proc, where the main feature is precisely that it is not being passed through the shell.

Ah, now I see. This is poorly documented. Documentation for proc says:

Construct a CreateProcess record for passing to createProcess, representing a raw command with arguments. [highlight by me]

My understanding was that "raw command" is supposed to be a shell command. Now I see that proc is intended to create a raw process, not a shell command (which is implied by the function's name but certainly not by its documentation).

With proc it is safe to call proc "echo" [possibly_malicous_string]

I don't understand this. Could you give example of how possibly_malicous_string could be dangerous (assuming characters are not escaped)?

Any security-aware code should only use proc

Please explain why. If I write a Haskell program that runs external command I can do a lot of bad things even when parameters to proc are escaped.

If you want shell features, use shell instead of proc

Problem with shell is that it runs sh shell, not bash. Replacing proc with shell in my example code gives:

/bin/sh: -c: line 0: syntax error near unexpected token `('
/bin/sh: -c: line 0: `diff <(echo $FOO) <(echo $BAR)'

I don't think this ticket should be closed - this is at least a documentation bug.

comment:8 Changed 14 months ago by nomeata

Well, if you check where translate is being used: It is only used on Windows

Hm... looks like you're right. In that case which part of the code quotes parameters to proc? Because this clearly is performed at some point.

No, they are not escaped and they need not to be; they are put in separate strings and passed to execve. Escaping is only required if you use the shell to execute the program – if you don’t use the shell, no escaping is required.

With proc it is safe to call proc "echo" [possibly_malicous_string]

I don't understand this. Could you give example of how possibly_malicous_string could be dangerous (assuming characters are not escaped)?

possibly_malicous_string = "$(rm -rf /)"

If you want shell features, use shell instead of proc

Problem with shell is that it runs sh shell, not bash. Replacing proc with shell in my example code gives:

/bin/sh: -c: line 0: syntax error near unexpected token `('
/bin/sh: -c: line 0: `diff <(echo $FOO) <(echo $BAR)'

Well, if you want a different shell than your system default, I guess you need to invoke it explicitly:

proc "bash" ["-c", some_bash_script]

I don't think this ticket should be closed - this is at least a documentation bug.

I wouldn’t call it a bug; the semantics of proc vs. shell are quite standard and expected, at least with some background in Unix systems assumed.

But of course there is always room for improvement. Any suggestions? Maybe “Because the command is executed directly, and not via a shell, the arguments do not need to be escaped, but you cannot use shell features like output redirection”?

Note: See TracTickets for help on using tickets.