RFR: 8282008: Incorrect handling of quoted arguments in ProcessBuilder [v3]

[Olga Mikhaltsova]

On Fri, 18 Feb 2022 17:21:48 GMT, Olga Mikhaltsova <omikhaltcova at openjdk.org> wrote:

>> This fix made equal processing of strings such as ""C:\\Program Files\\Git\\"" before and after JDK-8250568.
>> 
>> For example, it's needed to execute the following command on Windows:
>> `C:\Windows\SysWOW64\WScript.exe "MyVB.vbs" "C:\Program Files\Git" "Test"`
>> it's equal to:
>> `new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", ""C:\\Program Files\\Git\\"", "Test").start();`
>> 
>> While processing, the 3rd argument ""C:\\Program Files\\Git\\"" treated as unquoted due to the condition added in JDK-8250568.
>> 
>>     private static String unQuote(String str) {
>>     .. 
>>        if (str.endsWith("\\"")) {
>>             return str;    // not properly quoted, treat as unquoted
>>         }
>>     ..
>>     }
>> 
>> 
>> that leads to the additional surrounding by quotes in ProcessImpl::createCommandLine(..) because needsEscaping(..) returns true due to the space inside the string argument.
>> As a result the native function CreateProcessW (src/java.base/windows/native/libjava/ProcessImpl_md.c) gets the incorrectly quoted argument: 
>> 
>> pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = true)
>> pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git\\"" Test
>> (jdk.lang.Process.allowAmbiguousCommands = false)
>> 
>> 
>> Obviously, a string ending with `"\\""` must not be started with `"""` to treat as unquoted overwise it’s should be treated as properly quoted.
>
> Olga Mikhaltsova has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add a new line to the end of test file for JDK-8282008

Roger, writing a test via echo was not a good idea obviously for this particular case because of the fact well shown in the doc "4. Everyone Parses Differently", https://daviddeley.com/autohotkey/parameters/parameters.htm#WINCRULESMSEX. The task is more complicated than it seems at the first glance. The same command line correctly parsed in an app written in C/C++ might be incorrectly parsed in a VBS app etc. 

The suggestion not to use the path-argument surroundings with `'"'` doesn't fix the issue in case of VBS. It leads to a resulting path-argument ending with the doubled backslash in a VBS app according to the rules "10.1 The WSH Command Line Parameter Parsing Rules", https://daviddeley.com/autohotkey/parameters/parameters.htm#WSH.

Below there are some experiments with an app attached to JDK-8282008: 

NO FIX

1. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", ""C:\\Program Files\\Git\\"", "Test").start();

1.1 allowAmbiguousCommands = false
   arg[0] = \C:\Program 
   arg[1] = Files\Git1\\\  
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs ""C:\Program Files\Git1\\"" Test   

1.2 allowAmbiguousCommands = true
   arg[0] = C:\Program 
   arg[1] = Files\Git1\  
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs ""C:\Program Files\Git1"" Test   

2. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", "C:\\Program Files\\Git\", "Test").start();

2.1 allowAmbiguousCommands = false
   arg[0] = C:\Program Files\Git1\\ 
   arg[1] = Test  
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs "C:\Program Files\Git1\" Test   

2.2 allowAmbiguousCommands = true
   arg[0] = C:\Program Files\Git1\\ 
   arg[1] = Test 
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs "C:\Program Files\Git1\" Test   


FIXED (as in pr)

1. new ProcessBuilder("C:\\Windows\\SysWOW64\\WScript.exe", "MyVB.vbs", ""C:\\Program Files\\Git\\"", "Test").start();

1.1 allowAmbiguousCommands = false
   arg[0] = C:\Program Files\Git1\ 
   arg[1] = Test 
CreateProcessW: pcmd = "C:\Windows\SysWOW64\WScript.exe" MyVB.vbs "C:\Program Files\Git1" Test   

1.2 allowAmbiguousCommands = true
 arg[0] = C:\Program Files\Git1\ 
 arg[1] = Test 
CreateProcessW: pcmd = C:\Windows\SysWOW64\WScript.exe MyVB.vbs "C:\Program Files\Git1" Test  
``` 

Reading the code of unQuote() in terms of logic: "no beginning or ending quote, or too short => not quoted", - and it seems that if all these conditions are just opposite (starting and ending quotes and long enough) then a string should be treated as quoted but it's not. One exception was added and it's strange that it's applied even in case of paired quotes. Is it truly necessary for the security fix to skip (=to treat as unquoted) a string starting and ending with `'"'` in case of a `"\\""` tail?

This proposed fix returns back possibility (as was previously) to use surrounding with `'"'` as an argument mark-up that allows to pass correctly a path with a space inside in case of VBS. Roger, would you be so kind to take a look at this small fix once again, please, and to pay attention to VBS parsing arguments problem?!

-------------

PR: https://git.openjdk.java.net/jdk/pull/7504