Skip to content
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

Fix incorrect escaping of quotes for CMD when quoting #998

Merged
merged 5 commits into from Jun 27, 2023

Conversation

ericcornelissen
Copy link
Owner

@ericcornelissen ericcornelissen commented Jun 26, 2023

Relates to #986

Summary

Update escaping for quoting for CMD following a fuzz crash found during nightly fuzzing. The crash detected incorrect results for certain uses of double quotes. This has lead to a significant change to the implementation of escaping for quoting for CMD. In particular, double quotes will now be escaped as \\" (correspondingly, backslashes will be escaped as \\\\) based on the rules laid out in (of all sources) this gawk documentation. As a consequence, escaping of special characters is now only done when there's an even number of quotes preceding that character (don't ask me why...).

Notes

  • The bug label isn't applied because this doesn't fix a released problem.
  • The fact that this eliminates the argument preparation for CMD when quoting for fuzzing suggests all other argument preparation can be overcome by better implementations as well. Doing this is considered out of scope of this ticket, but anyone looking at this is welcome to contribute such a change.

@github-actions github-actions bot added the test Relates to testing label Jun 26, 2023
Update the `escapeArgForQuoted` function for CMD, preserving how quoting
is done. The majority of the old escape logic is gone (except for
control characters) and is replaced with new logic based first on (of
all sources) [1] to escape double quotes (which have to be escaped
somehow because they're also used to do the quoting). In short, the
rules for escaping quotes are as follows (adapted from [1]):
1. "For each double quote in the original string, let N be the number
   of backslash(es) before it, N might be zero. Replace these N
   backslash(es) by 2*N+1 backslash(es)."
2. "Let N be the number of backslash(es) [before a space or tab in] the
   original string, N might be zero. Replace these N backslash(es)
   by 2*N backslash(es)."

Second, based on [2] and experimentation, special characters are escaped
if and only if they're preceded by an even number (and/including 0) of
quotes. I can't really tell why this is correct, but from testing it
appears to be necessary as otherwise the escape character ("^") would
appear in the argument received by the target program when there's an
odd number of quotes preceding it.

--
1. https://www.gnu.org/software/gawk/manual/html_node/DOS-Quoting.html
2. https://ss64.com/nt/
@ericcornelissen ericcornelissen marked this pull request as ready for review June 27, 2023 20:53
@ericcornelissen ericcornelissen changed the title Fuzz crash for cmd.exe Fix incorrect escaping of quotes for CMD when quoting Jun 27, 2023
@ericcornelissen ericcornelissen merged commit 3364979 into main Jun 27, 2023
57 of 62 checks passed
@ericcornelissen ericcornelissen deleted the cmd-fuzz-crash branch June 27, 2023 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Relates to testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant