[PATCH 0/1] MR2472: CommandLineToArgvW: fixed vulnerability by incorrect parsing
### Merge request This fixes a vulnerability by incorrect parsing of command line arguments. The way how a triple quote chars (or double quote chars inside a quoted string) will be currently parsed in `CommandLineToArgvW` of wine is wrong, therefore opens an attack vector (using artificially created command line validated with different rules matching default parsing mechanism of Windows), so could be even considered as a vulnerability. ## Proposed changes `CommandLineToArgvW` will parse command line arguments correct and more similar to Windows. Here is the diff illustrating wrong behavior: ```diff ## # test is an alias for ## # python.exe -c "from sys import argv; del argv[0]; print(str(len(argv)) + ' | ' + ' | '.join(argv))" - # Wine + # Windows (and fixed variant) ## double in quoted string: > test "abc""def" ghi" xxx - 2 | abc"def ghi | xxx + 2 | abc"def | ghi xxx > test "abc"""def" ghi" xxx - 3 | abc"def | ghi" xxx + 2 | abc"def ghi | xxx ## triple in unquoted string: > test abc"""def" ghi" xxx - 2 | abc"def ghi | xxx + 2 | abc"def | ghi xxx > test abc""""def" ghi" xxx - 2 | abc"def | ghi xxx + 2 | abc"def ghi | xxx ``` As a consequence: - the arguments can be parsed incorrectly (compared to default behavior of Windows parser) in the way that parts of quoted arguments may become unquoted and vice versa, as well as swim between different args; - an attacker may create artificial command line that pass validation rules of nominal condition of Windows, but vulnerable here (inject, data steal, etc), because the arguments would deviate between validation and execution phase - special tokens like pipe `|`, ampersand `&` or redirecting tokens like `>` that normally included in quoted string (and validated as a string) could abrupt get different meaning and used for piping, redirecting etc, that beside the injection possibility opens still worse attacking vector that can even cause RCE or used to create persistent exploits. <details><summary>Here is the nominal condition how arguments will be parsed in Windows...</summary> cmd line|arg1|arg2 ---|---|--- abc" "def|abc def| abc\\" \\"def|abc"|"def "abc\\" \\"def"|abc" "def| "abc"" ""def"|abc" "def| abc"" ""def|abc|def abc""" """def|abc" "def| abc\\""" \\"""def|abc"|"def abc\\\\""" \\\\"""def|abc\\" \\"def "abc"""def" ghi"</sup>|abc"def ghi| "abc"""def" ghi<br><sup>* missing close qoute at end</sup>|abc"def ghi| "abc""def" ghi|abc"def|ghi "abc\\"""def" ghi|abc""def|ghi "abc\\\\"""def" ghi"|abc\\"def ghi| "abc\\\\\\"""def" ghi|abc\\""def|ghi "abc\\\\\\\\"""def" ghi"|abc\\\\"def ghi| </details> For PoC one could use any lang like tcl, python, etc, or even a self-written executable getting the argc/argv from main.<br/> The above diff and nominal condition was generated using this python script on Windows box: ```python from sys import argv; del argv[0]; print(str(len(argv)) + ' | ' + ' | '.join(argv)) ``` ## References [Related PR](https://github.com/reactos/reactos/pull/5186) with fix for ReactOS. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2472
From: "Sergey G. Brester" <gitlab(a)sebres.de> --- dlls/shcore/main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/shcore/main.c b/dlls/shcore/main.c index 278eeb82fd5..24d6436de25 100644 --- a/dlls/shcore/main.c +++ b/dlls/shcore/main.c @@ -383,10 +383,10 @@ WCHAR** WINAPI CommandLineToArgvW(const WCHAR *cmdline, int *numargs) /* consecutive quotes, see comment in copying code below */ while (*s == '"') { - qcount++; + if (++qcount==3) + qcount=1; s++; } - qcount = qcount % 3; if (qcount == 2) qcount = 0; } @@ -498,7 +498,7 @@ WCHAR** WINAPI CommandLineToArgvW(const WCHAR *cmdline, int *numargs) if (++qcount == 3) { *d++ = '"'; - qcount = 0; + qcount = 1; } s++; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2472
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=130933 Your paranoid android. === debian11 (32 bit report) === shell32: shlexec.c:1362: Test failed: exe three"""quotes next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe three"""quotes next: arg[1] expected L"three\"quotes" but got L"three\"quotes next" shlexec.c:1362: Test failed: exe four"""" quotes" next 4%3=1: expected 4 arguments, but got 3 shlexec.c:1370: Test failed: exe four"""" quotes" next 4%3=1: arg[1] expected L"four\" quotes" but got L"four\"" shlexec.c:1370: Test failed: exe four"""" quotes" next 4%3=1: arg[2] expected L"next" but got L"quotes next 4%3=1" shlexec.c:1362: Test failed: exe five"""""quotes next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe five"""""quotes next: arg[1] expected L"five\"quotes" but got L"five\"\"quotes next" shlexec.c:1370: Test failed: exe seven""""""" quotes" next 7%3=1: arg[1] expected L"seven\"\" quotes" but got L"seven\"\"\" quotes" shlexec.c:1370: Test failed: exe twelve""""""""""""quotes next: arg[1] expected L"twelve\"\"\"\"quotes" but got L"twelve\"\"\"\"\"quotes" shlexec.c:1370: Test failed: exe thirteen""""""""""""" quotes" next 13%3=1: arg[1] expected L"thirteen\"\"\"\" quotes" but got L"thirteen\"\"\"\"\"\" quotes" shlexec.c:1362: Test failed: exe "two""quotes next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe "two""quotes next: arg[1] expected L"two\"quotes" but got L"two\"quotes next" shlexec.c:1362: Test failed: exe "two"" next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe "two"" next: arg[1] expected L"two\"" but got L"two\" next" shlexec.c:1362: Test failed: exe "three""" quotes" next 4%3=1: expected 4 arguments, but got 3 shlexec.c:1370: Test failed: exe "three""" quotes" next 4%3=1: arg[1] expected L"three\" quotes" but got L"three\"" shlexec.c:1370: Test failed: exe "three""" quotes" next 4%3=1: arg[2] expected L"next" but got L"quotes next 4%3=1" shlexec.c:1362: Test failed: exe "four""""quotes next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe "four""""quotes next: arg[1] expected L"four\"quotes" but got L"four\"\"quotes next" shlexec.c:1370: Test failed: exe "six"""""" quotes" next 7%3=1: arg[1] expected L"six\"\" quotes" but got L"six\"\"\" quotes" shlexec.c:1370: Test failed: exe "eleven"""""""""""quotes next: arg[1] expected L"eleven\"\"\"\"quotes" but got L"eleven\"\"\"\"\"quotes" shlexec.c:1370: Test failed: exe "twelve"""""""""""" quotes" next 13%3=1: arg[1] expected L"twelve\"\"\"\" quotes" but got L"twelve\"\"\"\"\"\" quotes" shlexec.c:1362: Test failed: exe "the crazy \\"""\\" quotes: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe "the crazy \\"""\\" quotes: arg[1] expected L"the crazy \\\"\\" but got L"the crazy \\\"\\ quotes"
On Wed Mar 22 19:20:39 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=130933 Your paranoid android. === debian11 (32 bit report) === shell32: shlexec.c:1362: Test failed: exe three"""quotes next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe three"""quotes next: arg[1] expected L"three\"quotes" but got L"three\"quotes next" shlexec.c:1362: Test failed: exe four"""" quotes" next 4%3=1: expected 4 arguments, but got 3 shlexec.c:1370: Test failed: exe four"""" quotes" next 4%3=1: arg[1] expected L"four\" quotes" but got L"four\"" shlexec.c:1370: Test failed: exe four"""" quotes" next 4%3=1: arg[2] expected L"next" but got L"quotes next 4%3=1" shlexec.c:1362: Test failed: exe five"""""quotes next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe five"""""quotes next: arg[1] expected L"five\"quotes" but got L"five\"\"quotes next" shlexec.c:1370: Test failed: exe seven""""""" quotes" next 7%3=1: arg[1] expected L"seven\"\" quotes" but got L"seven\"\"\" quotes" shlexec.c:1370: Test failed: exe twelve""""""""""""quotes next: arg[1] expected L"twelve\"\"\"\"quotes" but got L"twelve\"\"\"\"\"quotes" shlexec.c:1370: Test failed: exe thirteen""""""""""""" quotes" next 13%3=1: arg[1] expected L"thirteen\"\"\"\" quotes" but got L"thirteen\"\"\"\"\"\" quotes" shlexec.c:1362: Test failed: exe "two""quotes next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe "two""quotes next: arg[1] expected L"two\"quotes" but got L"two\"quotes next" shlexec.c:1362: Test failed: exe "two"" next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe "two"" next: arg[1] expected L"two\"" but got L"two\" next" shlexec.c:1362: Test failed: exe "three""" quotes" next 4%3=1: expected 4 arguments, but got 3 shlexec.c:1370: Test failed: exe "three""" quotes" next 4%3=1: arg[1] expected L"three\" quotes" but got L"three\"" shlexec.c:1370: Test failed: exe "three""" quotes" next 4%3=1: arg[2] expected L"next" but got L"quotes next 4%3=1" shlexec.c:1362: Test failed: exe "four""""quotes next: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe "four""""quotes next: arg[1] expected L"four\"quotes" but got L"four\"\"quotes next" shlexec.c:1370: Test failed: exe "six"""""" quotes" next 7%3=1: arg[1] expected L"six\"\" quotes" but got L"six\"\"\" quotes" shlexec.c:1370: Test failed: exe "eleven"""""""""""quotes next: arg[1] expected L"eleven\"\"\"\"quotes" but got L"eleven\"\"\"\"\"quotes" shlexec.c:1370: Test failed: exe "twelve"""""""""""" quotes" next 13%3=1: arg[1] expected L"twelve\"\"\"\" quotes" but got L"twelve\"\"\"\"\"\" quotes" shlexec.c:1362: Test failed: exe "the crazy \\"""\\" quotes: expected 3 arguments, but got 2 shlexec.c:1370: Test failed: exe "the crazy \\"""\\" quotes: arg[1] expected L"the crazy \\\"\\" but got L"the crazy \\\"\\ quotes" ``` So CI is ready and as expected several tests failed. At first glance they look artificial, for instance testing the first on Windows show:
> python three"""quotes next
python: can't open file 'three"quotes next': [Errno 22] Invalid argument
> python -c "from sys import argv; del argv[0]; print(str(len(argv)) + ' | ' + ' | '.join(argv))" three"""quotes next
1 | three"quotes next
whereas the test assumes 2 parameters: `three"quotes` and `next` instead of `three"quotes next` (+ 1 for exe-name). I'll check all failing tests against default windows notation and adjust the tests accordingly. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2472#note_27584
On Wed Mar 22 20:28:02 2023 +0000, Sergey G. Brester wrote:
So CI is ready and as expected several tests failed. At first glance they look artificial, for instance testing the first on Windows show: ```
python three"""quotes next python: can't open file 'three"quotes next': [Errno 22] Invalid argument python -c "from sys import argv; del argv[0]; print(str(len(argv)) + ' | ' + ' | '.join(argv))" three"""quotes next 1 | three"quotes next
whereas the test assumes 2 parameters: `three"quotes` and `next` instead of `three"quotes next` (+ 1 for exe-name). I'll check all failing tests against default windows notation and adjust the tests accordingly. OK, the tests look good if one try it with a small exe using `CommandLineToArgvW` on windows. It only deviates from whatever script lang I tried to use.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2472#note_27651
This merge request was closed by Sergey G. Brester. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2472
Weird, but everyone seems really to use own parser for splitting of command line parameters. The tests are good (and this PR is then wrong), if one tries it with a small [test-cltaw.exe](https://github.com/sebres/PoC/raw/master/SB-0D-001-win-exec/test-cltaw.exe) (from [test-cltaw.c](https://github.com/sebres/PoC/blob/master/SB-0D-001-win-exec/test-cltaw.c)) using `CommandLineToArgvW` on windows. Exactly in the same way it does with [test-dump.exe](https://github.com/sebres/PoC/raw/master/SB-0D-001-win-exec/test-dump.exe) using `args/argv` pair from `main`, but... But it completely deviates from `python`, `tcl`, whatever script lang, that seem to use own command line parser for arguments if built for windows (and then the PR would be correct). <details><summary>Here is small table illustrating the issue...</summary> <table> <tr><th rowspan="2">command line</th><th colspan="4">CommandLineToArgvW / main</th><th colspan="4">python, tcl, etc</th></tr> <tr><th>C</th><th>A1</th><th>A2</th><th>A3</th><th>C</th><th>A1</th><th>A2</th><th>A3</th></tr> <tr><td>three"""quotes next</td><td>2</td><td>three"quotes</td><td>next</td><td></td> <td>1</td><td>three"quotes next</td><td></td><td></tr> <tr><td>four"""" quotes" next 4%3=1</td><td>3</td><td>four" quotes</td><td>next</td><td>4%3=1</td> <td>2</td><td>four"</td><td>quotes next 4%3=1</td><td></tr> <tr><td>five"""""quotes next</td><td>2</td><td>five"quotes</td><td>next</td><td></td> <td>1</td><td>five""quotes next</td><td></td><td></tr> <tr><td>seven""""""" quotes" next 7%3=1</td><td>3</td><td>seven"" quotes</td><td>next</td><td>7%3=1</td> <td>3</td><td>seven""" quotes</td><td>next</td><td>7%3=1</tr> <tr><td>twelve""""""""""""quotes next</td><td>2</td><td>twelve""""quotes</td><td>next</td><td></td> <td>2</td><td>twelve"""""quotes</td><td>next</td><td></tr> <tr><td>thirteen""""""""""""" quotes" next 13%3=1</td><td>3</td><td>thirteen"""" quotes</td><td>next</td><td>13%3=1</td> <td>3</td><td>thirteen"""""" quotes</td><td>next</td><td>13%3=1</tr> <tr><td>"two""quotes next</td><td>2</td><td>two"quotes</td><td>next</td><td></td> <td>1</td><td>two"quotes next</td><td></td><td></tr> <tr><td>"two"" next</td><td>2</td><td>two"</td><td>next</td><td></td> <td>1</td><td>two" next</td><td></td><td></tr> <tr><td>"three""" quotes" next 4%3=1</td><td>3</td><td>three" quotes</td><td>next</td><td>4%3=1</td> <td>2</td><td>three"</td><td>quotes next 4%3=1</td><td></tr> <tr><td>"four""""quotes next</td><td>2</td><td>four"quotes</td><td>next</td><td></td> <td>1</td><td>four""quotes next</td><td></td><td></tr> <tr><td>"six"""""" quotes" next 7%3=1</td><td>3</td><td>six"" quotes</td><td>next</td><td>7%3=1</td> <td>3</td><td>six""" quotes</td><td>next</td><td>7%3=1</tr> <tr><td>"eleven"""""""""""quotes next</td><td>2</td><td>eleven""""quotes</td><td>next</td><td></td> <td>2</td><td>eleven"""""quotes</td><td>next |</tr> <tr><td>"twelve"""""""""""" quotes" next 13%3=1</td><td>3</td><td>twelve"""" quotes</td><td>next</td><td>13%3=1</td> <td>3</td><td>twelve"""""" quotes</td><td>next</td><td>13%3=1</tr> <tr><td>"the crazy \\"""\\" quotes</td><td>2</td><td>the crazy \"\</td><td>quotes</td><td></td> <td>1</td><td>the crazy \"\ quotes</td><td></td><td></tr> </table> </details> Just since it is supposed to be a fix for `CommandLineToArgvW` (to behave more similar on windows), I'll close it now (and gonna open few PRs for the mentioned langs for windows). Thus closed, sorry for noise. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2472#note_27656
participants (3)
-
Marvin -
Sergey G. Brester -
Sergey G. Brester (＠sebres)