Skip to content

Commit

Permalink
[Backport] CVE-2024-9965: Insufficient data validation in DevTools
Browse files Browse the repository at this point in the history
Manual backport of patch originally reviewed on
https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5850610:
Fix curl escaping for cmd when ampersand follows a newline.

Currently, generated command allows arbitrary code execution, due to incomplete escaping: an ampersand in multi-line strings is not escaped.

This is fixed by always escaping outer quotes, thus disabling cmd double quote escaping, and escaping ampersand with a caret.

Double quote escaping is very convenient and easy to read, but it doesn't support multi-line strings. Thus we need to be able to escape individual characters with caret, which was added in https://codereview.chromium.org/2182213006/.

That CL only escaped outer double quotes around multi-line strings for better readability. However this lead to carets being interpreted as a literal character in single-line strings. This caused crrev.com/2514441 which disabled caret escaping of ampersand even for multi-line strings with escaped quotes and crrev.com/c/5126051 which enabled escaping quotes for the single-line strings with special characters (including ampersand, which was still exempt from caret escaping:).

As this is all rather complicated and hard to follow, this CL sacrifices the generated command readability in favor of correctness and simplicity (relatively speaking) of the generator code, leaving only one mode of escaping.

Bug: 352651673
Change-Id: I4c25b165c6ba7b3eae3891179c8e371fc16c91f2
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/5850610
Commit-Queue: Danil Somsikov <[email protected]>
Reviewed-by: Benedikt Meurer <[email protected]>
Reviewed-on: https://codereview.qt-project.org/c/qt/qtwebengine-chromium/+/597949
Reviewed-by: Michal Klocek <[email protected]>
  • Loading branch information
danilsomsikov authored and mibrunin committed Oct 21, 2024
1 parent 41183a7 commit 5ba02ae
Showing 1 changed file with 3 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2184,8 +2184,7 @@ export class NetworkLogView extends Common.ObjectWrapper.eventMixin<EventTypes,
const ignoredHeaders = new Set<string>(['accept-encoding', 'host', 'method', 'path', 'scheme', 'version']);

function escapeStringWin(str: string): string {
/* If there are no new line characters do not escape the " characters
since it only uglifies the command.
/* Always escape the " characters so that we can use caret escaping.
Because cmd.exe parser and MS Crt arguments parsers use some of the
same escape characters, they can interact with each other in
Expand All @@ -2211,11 +2210,11 @@ export class NetworkLogView extends Common.ObjectWrapper.eventMixin<EventTypes,
new line is there to enact the escape command the second is the character
to escape (in this case new line).
*/
const encapsChars = /[\r\n]/.test(str) ? '^"' : '"';
const encapsChars = '^"';
return encapsChars +
str.replace(/\\/g, '\\\\')
.replace(/"/g, '\\"')
.replace(/[^a-zA-Z0-9\s_\-:=+~'\/.',?;()*`&]/g, '^$&')
.replace(/[^a-zA-Z0-9\s_\-:=+~'\/.',?;()*`]/g, '^$&')
.replace(/%(?=[a-zA-Z0-9_])/g, '%^')
.replace(/\r?\n/g, '^\n\n') +
encapsChars;
Expand Down

0 comments on commit 5ba02ae

Please sign in to comment.