Bug 16636

Summary: Incorrect escaping in shQuote on Windows (security issue)
Product: R Reporter: Otto Seiskari <otto.seiskari>
Component: LanguageAssignee: R-core <R-core>
Status: CLOSED FIXED    
Severity: major CC: murdoch
Priority: P5    
Version: R 3.2.2   
Hardware: x86_64/x64/amd64 (64-bit)   
OS: Windows 64-bit   

Description Otto Seiskari 2015-12-18 10:45:57 UTC
With argument type="cmd", the shQuote function uses backslash as an escape character for double quotes instead of the correct escaping method of doubling these characters. Backslash is not an escape character in cmd.exe. As a result, the escaped strings are not safe to use in cmd.exe or the arguments of system2 on Windows.

For example

    > shQuote('"', type="cmd")
    [1] "\"\\\"\""

while the correct quotation is '""' = "\"\"". This opens a command injection vulnerability.

Example: invoking TYPE to print file contents

    winCat <- function(filename) {
      shell(paste0("TYPE ", shQuote(filename)))
    }

    # Prints the contents of somefile.txt
    winCat("somefile.txt")

    # Arbitrary command execution
    winCat('" & nslookup google.com & REM "')

    # Correctly escaped version (error)
    shell('TYPE """ & nslookup google.com & REM """')

For some reason, apparently due to the way system and system2 invoke (or do not invoke) the cmd.exe shell, changing 'shell' to 'system' breaks this. But here's another example showing how the bug also affects the system and system2 commands.

    winFind <- function(string, filename) {
      system2("FIND", c(
        shQuote(string, type="cmd"),
        shQuote(filename, type="cmd")))
    }

    # correctly searches somefile.txt for "some string"
    winFind("some string", "somefile.txt")

    # fails with an error
    winFind('"', "somefile.txt")

    # Prints all \-containing lines in WindowsUpdate.log
    winFind('" C:\\Windows\\WindowsUpdate.log "', "somefile.txt")

In addition, command injection also works for some command using 'system', for example, if Azure CLI is installed, then the following injects a command

    argument <- '" & nslookup google.com & REM "'
    system2("azure", c("help", shQuote(argument, type="cmd")))
Comment 1 Duncan Murdoch 2015-12-18 17:40:34 UTC
I've been unsuccessful at finding any authoritative documentation from Microsoft about the proper way to escape special chars, but this page

http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/everyone-quotes-arguments-the-wrong-way.aspx

says that repeating quotes is not the proper way.  The cmd interpreter uses ^ as the escape character, and the next level of processing by CommandLineToArgvW uses a backslash as an escape.  What reference makes you think that a backslash is not correct?
Comment 2 Otto Seiskari 2015-12-21 09:43:13 UTC
The problem is that, on Windows, it depends on the invoked command how the arguments should be escaped and there seems to be no consistent method that would be safe for all commands.

In R documentation for v. 3.2.2 (for some reason, nothing is mentioned for v. 3.3.0 here https://stat.ethz.ch/R-manual/R-devel/library/base/html/shQuote.html) it is stated that

    "... this makes shQuote safe for use with file paths in system, and
    with shell if the default shell is used."

and

    "It will usually be safe to use shQuote to quote arguments of a command,
    but because system does not use a shell, interpretation of quoted
    arguments is done by the run-time code of the executable"
    
I would replace 'usually safe' by 'not safe' since the chosen quotation method leaves many (if not most) cases wide open for command injection, as demonstrated in my earlier message, and the problem is not only about using or not using a shell (cmd.exe).
    
To underline my point, here are some more examples demonstrating the complicated and unintuitive escaping rules in cmd.exe. Sometimes one can use backslash to esacpe quotes, sometimes not. The same goes for escaping & as ^&.
    
Here are six valid ways of passing the string 'double "quotes" & dir' to different programs

    echo double "quotes" ^& dir
    find "double ""quotes"" & dir" somefile.txt
    perl -e "print \"double \\\"quotes\\\" ^& dir\""
    azure help "double \"quotes\" & dir"
    azure help "double ""quotes"" & dir"
    azure help "double """quotes""" & dir"
    
Here are some examples that do not work as supposed to

    :: prints backslashes and extra quotes
    echo "double \"quotes\" ^& dir"
    
    :: escaping & is not allowed here
    find "double ""quotes"" ^& dir" somefile.txt
    
    :: side effect: attempts to read extra files, e.g., 'double \quotes\'
    find "double \"quotes\" & dir" somefile.txt
    find "double """quotes""" & dir" somefile.txt
    find "double ^"quotes^" ^& dir" somefile.txt

and more attempts of trying to pass '" & dir "' to the Azure help command. Only the first one works, all the others result to a command injection.

    azure help """ & dir """
    azure help "\" & dir \""
    azure help "\" ^& dir \""
    azure help """" & dir """"
    azure help """" ^& dir """"
Comment 3 Duncan Murdoch 2015-12-22 17:17:08 UTC
You didn't address my question.  You claimed that doubling quotes is the right way to escape them in cmd.  I don't think that's true.

I'm not sure we're doing it right, but I'm not going to make random changes just to get a few examples to work.
Comment 4 Otto Seiskari 2015-12-23 10:31:53 UTC
You are right, I was too hasty to conclude that doubling quotes would be the correct solution in general. In the (special?) case of the FIND command, it is confirmed as the correct solution here

	https://technet.microsoft.com/en-us/library/2ca66b22-3b7c-4166-8503-eb75fc53ab46
	
I have not been able to find any official documentation about quotes in cmd.exe (I don't think it exist), or any complete documentation for that matter. The most accurate resources have been Stack Overflow posts guessing how the interpreter works by experimentation, e.g.,

	http://stackoverflow.com/a/15262019/1426569    http://stackoverflow.com/questions/4094699/how-does-the-windows-command-interpreter-cmd-exe-parse-scripts/4095133#4095133

What you should anyway conclude is that, unlike its apparently bulletproof UNIX counterparts, such as shQuote(x, type="sh"), the current Windows version is NOT, in general, a safe way of "Quoting Strings for Use in OS Shells".

I do not know what would be the best way of fixing this but at the very least, the user should be clearly warned about the shortcomings of the implementation.
Comment 5 Duncan Murdoch 2015-12-23 20:20:13 UTC
system() and system2() don't invoke cmd.exe unless you include it in the "command" arg.  

shell() does by default, though as ?shell says, there are ways to use different shells.

As far as I can see, the *only* correct way to quote things on the cmd.exe command line is to precede them with "^".  I will add this as an option.  So for "internal" commands like TYPE, that's the quoting that's needed.  External commands like FIND do their own command line handling, and potentially they all do their own thing.

For back compatibility I won't change shQuote(type = "cmd") quoting.
Comment 6 Duncan Murdoch 2015-12-24 21:49:42 UTC
Fixed in R-devel; soon in R-patched.  The fix adds a new value for the "type" argument:  "cmd2" does escaping suitable for cmd.exe (but probably not for the command it runs).