[beta] Backport fix of CVE-2024-24576 #123682
Conversation
rustbot
added
O-windows
|
@bors r+ p=500 rollup=never |
bors
added
S-waiting-on-bors
… r=pietroalbini [beta] Backport fix of CVE-2024-24576 r? `@ghost` See https://blog.rust-lang.org/2024/04/09/cve-2024-24576.html
rustbot
added
A-testsuite
|
Backported a fix for the tidy issue we saw in #123681 (comment). @bors retry r+ p=500 rollup=never |
This comment has been minimized.
This comment has been minimized.
… r=pietroalbini [beta] Backport fix of CVE-2024-24576 r? `@ghost` See https://blog.rust-lang.org/2024/04/09/cve-2024-24576.html
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
1 similar comment
|
☀️ Test successful - checks-actions |
| // Appending an additional double-quote acts as an escape. | ||
| cmd.push(b'"' as u16) |
There was a problem hiding this comment.
This doesn't sound correct to me. A double-quote does leave the state of the quote-flag inside cmd unchanged, but it also adds an extra quote character to the command line being executed, which changes the meaning of the that argument. For example, cmd /c echo " " " should print a single ", as it would do if written ". But if the call was something like echo "contains evil" and the evil would be contained inside those quotes. But after this new transform, the argument looks like it becomes echo """contains evil""" and that evil may be no longer contained inside those quotes. In particular, if "evil" contained an escaped quote such as "foo \" bar", this step appears to now inject an unescaped quote here: """foo \\\"" bar""".
Additionally, \ is not a special character in cmd / batch, so it is suspect to me that would appear in the list of special characters to handle. I think the PR has gotten mixed up on what processing is done by the processing of the arguments to batch, the call within batch, and the program executed by batch. Those each require separate handling (because batch/cmd/windows parsing is a crazy world).
Source: I am the author of the Julia algorithm for handling this (shell_escape_wincmd) https://github.com/JuliaLang/julia/blob/e9a24d4cee4153023a7841a75fc4285229f26720/base/shell.jl#L351-L356, and I once tried to write a blog post about how every algorithm I could find for this online seemed to have a least one significant bug, but I couldn't quite manage to finish listing and validating all of the exception cases that I found: JuliaLang/www.julialang.org@main...vtjnash:www.julialang.org:jn/str-escape#diff-8a99ded732ef1d9e4a4810df7c302c887c5616aa829fa0b52efe57cf7f8e17c8R458-R491
There was a problem hiding this comment.
Thanks for taking a closer look at this
Cc @ChrisDenton
There was a problem hiding this comment.
echo "contains evil" will become "echo ""contains evil""", which isn't great but is not a shell escape. Similarly "foo \" bar" becomes """foo \\"" bar""".
It's a bit confusing because the code is indeed doing two things. The main goal of course is to prevent shell injection. A secondary goal was to be as close to the old behaviour as possible (so as to not break existing code that worked). The old behaviour didn't really consider cmd.exe at all (though a few escapes were tacked on later) and instead prepared arguments assuming they'd be passed through to an exe. So when there was a decision between accurately preserving them in batch files or preserving them when passed through to an exe, the latter won out.
There was a problem hiding this comment.
I see what you mean there, but I still don't think you are using the right algorithm to accomplish that correctly. First of all because there should be double+1 \ here before the " to correctly preserve the " through the Window's command line parser. And secondly because the behavior of doubling the " is undocumented and inconsistent between compilers, so while it usually does preserve a single " it usually also terminates the argument there also:
JuliaLang/www.julialang.org@main...vtjnash:www.julialang.org:jn/str-escape#diff-8a99ded732ef1d9e4a4810df7c302c887c5616aa829fa0b52efe57cf7f8e17c8R409-R411 such that our "contains evil" example results in 2 arguments getting received by C: echo "contains and evil"
There was a problem hiding this comment.
We do document that we specifically follow MSVC parsing rules. These rules may not be publicly documented but they have been consistent and well known for well over a decade at this point (e.g. see top voted stackoverflow answers).
Admittedly this is a trade-off. For our normal (not-bat) argument passing we don't make use of "". Maybe we can indeed make better choices here but that's something that can be worked on publicly.
There was a problem hiding this comment.
We do document that we specifically follow MSVC parsing rules
I guess that is fair, it seems that this should follow along with those rules. I suppose I should know that, but I forgot that I was the one to write that documentation in ages past: MicrosoftDocs/cpp-docs@e5bdbb7
There was a problem hiding this comment.
they have been consistent and well known for well over a decade at this point
They actually changed about 3 years ago (removing was_in_quote). And as the PR noted that the common Windows rules have change several times (although the latest change was probably slightly more than a decade ago):
e26dda5#diff-96f5abd15201d718909ac0227541d568d9985ac9b075a34ae7eb9f5f66a562f9
I have found from experience that most internet sites (particularly stack overflow) have incorrect information regarding Windows and cmd argument parsing (and have attempted to correct some of it), so I put little value in what the top answer said.
There was a problem hiding this comment.
Ok, so in testing it was found that ^" didn't always work to properly escape the quote character in cmd (the double quote was escaped but it could still affect parsing in some situations). The first version of the patch disallowed " for this reason. However, it was found that some users were using double quotes. So our options were:
- break user's code (which we're very reluctant to do in a point release unless absolutely necessary)
- have some algorithm for deciding if we can allow quotes (i.e if the quotes are balanced and there's nothing that would be affected by quoting/unquoting)
- do the
""thing (which not everything supports and will look weird in batch files)
We wanted to avoid the first and the second adds a fair bit complexity (and may be fragile). So it was thought the third was the least worst option.
There was a problem hiding this comment.
I don't think ^" ever escapes a quote character: the ^ is just ignored if the subsequent character cannot be quoted, which includes ". The algorithm for 2 is very simple: you just quote everything that might be special except when inside quotes, when you quote nothing. The cmd parser doesn't have a notion of "balanced" quotes, as it simply toggles the escape state at each " scanning left to right, and does not care whether the end state is on or off. This algorithm has the advantage that it doesn't change the actual values of the arguments that the user provided.
There was a problem hiding this comment.
The cmd parser doesn't have a notion of "balanced" quotes, as it simply toggles the escape state at each " scanning left to right, and does not care whether the end state is on or off.
That's what I mean by "balanced", sorry for the imprecision. At the end of a single argument (e.g. %1, etc in bash), we do need to be careful of the escape state,
The algorithm for 2 is very simple: you just quote everything that might be special except when inside quotes, when you quote nothing
That is mostly what we do. But, as far as I'm aware, you can't quote a quote.
r? @ghost
See https://blog.rust-lang.org/2024/04/09/cve-2024-24576.html