New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BF: vulnerability by command line parsing (considers valid windows escape sequences) #307
Conversation
… sequences, like backslashes followed by quote-char as well as quote triples) - SplitCommandLine works now very similar to CommandLineToArgvW (but doesn't require dependency to shell32)
…ed copying (don't gather remaining string), don't build args byte by byte
Thank you, pull request rebased and integrated. |
Thx for review and merge. |
The comments were okay, but the line lengths of them are 65 ... |
OK, understand. By the way, by fixing of that here I thought have been finding similar vulni on ReactOS and wine, just... It looks like in case of ReactOS and wine (for the function Here is small table illustrating the issue...
The way how the parsing is implemented in this PR and merged showing the right part of table (like script langs doing it). CPP/Common/CommandLineParser.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/CPP/Common/CommandLineParser.cpp b/CPP/Common/CommandLineParser.cpp
index d07f0f1..6bea75b 100644
--- a/CPP/Common/CommandLineParser.cpp
+++ b/CPP/Common/CommandLineParser.cpp
@@ -51,7 +51,7 @@ static const wchar_t * _SplitCommandLine(const wchar_t* s, UString &dest)
if (++qcount == 3)
{
dest += L'"';
- qcount = 1;
+ qcount = 0;
}
}
f = s; Just I'm still unsure which strategy is better/safer, probably must investigate in source code history of python/tcl (at least this two) or interview those community (or code originators). |
@sebres - should I release a new version with that fix? |
I'm still unsure about the further amend (with |
compatibility to CommandLineToArgvW or stdlib's main argument parsing
OK, regarding different behavior of script-langs, it looks indeed like a historic mistake. |
amend to #307, compatibility to CommandLineToArgvW or stdlib's main argument parsing
compatibility to CommandLineToArgvW or stdlib's main argument parsing Signed-off-by: Sergey G. Brester <serg.brester@sebres.de>
…of backslash and quotes); fixes mcmilkgh-312 for new command-line parser algorithm (mcmilkgh-307, mcmilkgh-310)
…of backslash and quotes); fixes mcmilkgh-312 for new command-line parser algorithm (mcmilkgh-307, mcmilkgh-310)
This PR provides a fix for certain vulnerability by command line parsing (considers valid windows escape sequences), like backslashes followed by quote-char as well as quote triples).
Windows command line processor (as well as programs and standard libraries constructing command line from arguments) use special escape and quoting sequences, so...
the proper scenario to parse command line looks like that...
"a b"
-->a b
"
\"
-->"
the remainder escaping the quote:
2n backslashes + quote --> n backslashes + quote as an argument delimiter
2n+1 backslashes + quote --> n backslashes + literal quote
a\b
-->a\b
a\\b
-->a\\b
with the remainder modulo 3 deciding whether to close the string or not.
Note that the opening quote must be counted in the consecutive quotes,
that's the (1+) below:
(1+) 3n quotes -> n quotes
(1+) 3n+1 quotes -> n quotes plus closes the quoted string
(1+) 3n+2 quotes -> n+1 quotes plus closes the quoted string
remaining consecutive quotes follow the above rule.
this causes following behavior by parsing of command line...
* missing close qoute at end
I know Windows doesn't allow quote-char in file or folder names, but command line could be built by some automation with a foreign user input and if it becomes validated using nominal condition of Windows parser, but hereafter processed in 7zip, as a consequence it could cause:
|
, 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.Alternatively one could use windows API CommandLineToArgvW, but this would imply a modifying of
SplitCommandLine
interface as well as new dependency to shell32.dll.With this fix
SplitCommandLine
works now very similar toCommandLineToArgvW
(but doesn't require dependency toshell32
library).