Medium firefox Logic Error 🔧 Commit mapped

Overview

Medium
Severity
CVSS
No
Exploited ITW
Fixed
Fix Status
Impactmoderate
DescriptionDue to insufficient escaping of the newline character in the “Copy as cURL” feature, an attacker could trick a user into using this command, potentially leading to local code execution on the user's system.
ComponentCore
Bug ClassLogic Error
Tracker1950001
Fix commit494bb420bcad (firefox) +42/-53
CISA KEVNot listed
CreditedAmeen Basha M K
Disclosed2025-05-27

Changed Functions

FunctionChangeNotes
generateCommand
devtools/client/shared/curl.js
modified
if
devtools/client/shared/curl.js
modified

Files Changed

  • devtools/client/netmonitor/test/browser_net_curl-utils.js
  • devtools/client/shared/curl.js
  • devtools/client/shared/test/xpcshell/test_curl.js
diff --git a/devtools/client/netmonitor/test/browser_net_curl-utils.js b/devtools/client/netmonitor/test/browser_net_curl-utils.js
index 17452405eda..da44ebfaf47 100644
--- a/devtools/client/netmonitor/test/browser_net_curl-utils.js
+++ b/devtools/client/netmonitor/test/browser_net_curl-utils.js
@@ -153,7 +153,7 @@ function testDataArgumentOnGeneratedCommand(data) {
 }
 
 function testDataEscapeOnGeneratedCommand(data) {
-  const paramsWin = `--data-raw "{""param1"":""value1"",""param2"":""value2""}"`;
+  const paramsWin = `--data-raw ^"{\\"param1\\":\\"value1\\",\\"param2\\":\\"value2\\"}^"`;
   const paramsPosix = `--data-raw '{"param1":"value1","param2":"value2"}'`;
 
   let curlCommand = Curl.generateCommand(data, "WINNT");
diff --git a/devtools/client/shared/curl.js b/devtools/client/shared/curl.js
index 0af5a608cb2..35f40544045 100644
--- a/devtools/client/shared/curl.js
+++ b/devtools/client/shared/curl.js
@@ -58,17 +58,11 @@ const Curl = {
   generateCommand(data, platform) {
     const utils = CurlUtils;
 
-    let command = ["curl"];
+    let commandParts = [];
 
     // Make sure to use the following helpers to sanitize arguments before execution.
-    const addParam = value => {
-      const safe = /^[a-zA-Z-]+$/.test(value) ? value : escapeString(value);
-      command.push(safe);
-    };
-
-    const addPostData = value => {
-      const safe = /^[a-zA-Z-]+$/.test(value) ? value : escapeString(value);
-      postData.push(safe);
+    const escapeStringifNeeded = value => {
+      return /^[a-zA-Z-]+$/.test(value) ? value : escapeString(value);
     };
 
     const ignoredHeaders = new Set();
@@ -77,17 +71,17 @@ const Curl = {
     // The cURL command is expected to run on the same platform that Firefox runs
     // (it may be different from the inspected page platform).
     const escapeString =
-      currentPlatform == "WINNT"
+      currentPlatform === "WINNT"
         ? utils.escapeStringWin
         : utils.escapeStringPosix;
 
     // Add URL.
-    addParam(data.url);
+    commandParts.push(escapeString(data.url));
 
     // Disable globbing if the URL contains brackets.
     // cURL also globs braces but they are already percent-encoded.
     if (data.url.includes("[") || data.url.includes("]")) {
-      addParam("--globoff");
+      commandParts.push("--globoff");
     }
 
     let postDataText = null;
@@ -104,13 +98,13 @@ const Curl = {
       // which composed using \n only, not \r\n, may be not parsable for
       // peers which split parts of multipart payload using \r\n.
       postDataText = data.postDataText;
-      addPostData("--data-binary");
+      postData.push("--data-binary");
       const boundary = utils.getMultipartBoundary(data);
       const text = utils.removeBinaryDataFromMultipartText(
         postDataText,
         boundary
       );
-      addPostData(text);
+      postData.push(escapeStringifNeeded(text));
       ignoredHeaders.add("content-length");
     } else if (
       data.postDataText &&
@@ -119,8 +113,10 @@ const Curl = {
     ) {
       // When no postData exists, --data-raw should not be set
       postDataText = data.postDataText;
-      addPostData("--data-raw");
-      addPostData(utils.writePostDataTextParams(postDataText));
+      postData.push(
+        "--data-raw " +
+          escapeStringifNeeded(`${utils.writePostDataTextParams(postDataText)}`)
+      );
       ignoredHeaders.add("content-length");
     }
     // curl generates the host header itself based on the given URL
@@ -128,20 +124,19 @@ const Curl = {
 
     // Add --compressed if the response is compressed
     if (utils.isContentEncodedResponse(data)) {
-      addParam("--compressed");
+      commandParts.push("--compressed");
     }
 
     // Add -I (HEAD)
     // For servers that supports HEAD.
     // This will fetch the header of a document only.
     if (data.method === "HEAD") {
-      addParam("-I");
+      commandParts.push("-I");
     } else if (data.method !== "GET") {
       // Add method.
       // For HEAD and GET requests this is not necessary. GET is the
       // default, -I implies HEAD.
-      addParam("-X");
-      addParam(data.method);
+      commandParts.push("-X " + escapeStringifNeeded(`${data.method}`));
     }
 
     // Add request headers.
@@ -155,14 +150,26 @@ const Curl = {
       if (ignoredHeaders.has(header.name.toLowerCase())) {
         continue;
       }
-      addParam("-H");
-      addParam(header.name + ": " + header.value);
+      commandParts.push(
+        "-H " + escapeStringifNeeded(`${header.name}: ${header.value}`)
+      );
     }
 
     // Add post data.
-    command = command.concat(postData);
-
-    return command.join(" ");
+    commandParts = commandParts.concat(postData);
+
+    // Format with line breaks if the command has more than 2 parts
+    // e.g
+    // Command with 2 parts  - curl https://foo.com
+    // Commands with more than 2 parts -
+    // curl https://foo.com
+    // -X POST
+    // -H "Accept : */*"
+    // -H "accept-language: en-US"
+    const joinStr = currentPlatform === "WINNT" ? " ^\n  " : " \\\n  ";
+    return (
+      "curl " + commandParts.join(commandParts.length >= 3 ? joinStr : " ")
+    );
   },
 };
 
@@ -444,18 +451,16 @@ const CurlUtils = {
       same escape characters, they can interact with each other in
       horrible ways, the order of operations is critical.
     */
-    const encapsChars = '"';
+    const encapsChars = '^"';
     return (
       encapsChars +
       str
-
         //  Replace \ with \\ first because it is an escape character for certain
         // conditions in both parsers.
         .replace(/\\/g, "\\\\")
 
-        // Replace double quote chars with two double quotes (not by escaping with \") because it is
-        // recognized by both cmd.exe and MS Crt arguments parser.
-        .replace(/"/g, '""')
+        // Escape double quotes with double slashes.
+        .replace(/"/g, '\\"')
 
         // Escape ` and $ so commands do not get executed e.g $(calc.exe) or `\$(calc.exe)
         .replace(/[`$]/g, "\\$&")
@@ -473,15 +478,10 @@ const CurlUtils = {
         // by the previous replace.
         .replace(/%(?=[a-zA-Z0-9_])/g, "%^")
 
-        // We replace \r and \r\n with \n, this allows to consistently escape all new
-        // lines in the next replace
-        .replace(/\r\n?/g, "\n")
-
         // Lastly we replace new lines with ^ and TWO new lines because the first
         // new line is there to enact the escape command the second is the character
         // to escape (in this case new line).
-        // The extra " enables escaping new lines with ^ within quotes in cmd.exe.
-        .replace(/\n/g, '"^\r\n\r\n"') +
+        .replace(/\r?\n/g, "^\n\n") +
       encapsChars
     );
   },
diff --git a/devtools/client/shared/test/xpcshell/test_curl.js b/devtools/client/shared/test/xpcshell/test_curl.js
index a2a6c3412ee..a04d0462fc9 100644
--- a/devtools/client/shared/test/xpcshell/test_curl.js
+++ b/devtools/client/shared/test/xpcshell/test_curl.js
@@ -230,18 +230,15 @@ add_task(async function () {
   );
 
   // Check binary data
-  const dataBinaryPos = cmd.indexOf("--data-binary");
-  const dataBinaryParam = `--data-binary ${isWin() ? "" : "$"}${escapeNewline(
-    quote(request.postDataText)
-  )}`;
+  const dataBinaryParam = `--data-binary \\\n  $'------------14808\\r\\n`;
   Assert.notStrictEqual(
-    dataBinaryPos,
+    cmd.indexOf("--data-binary"),
     -1,
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/devtools/client/netmonitor/test/browser_net_curl-utils.js b/devtools/client/netmonitor/test/browser_net_curl-utils.js
index 17452405eda..da44ebfaf47 100644
--- a/devtools/client/netmonitor/test/browser_net_curl-utils.js
+++ b/devtools/client/netmonitor/test/browser_net_curl-utils.js
@@ -153,7 +153,7 @@ function testDataArgumentOnGeneratedCommand(data) {
 }
 
 function testDataEscapeOnGeneratedCommand(data) {
-  const paramsWin = `--data-raw "{""param1"":""value1"",""param2"":""value2""}"`;
+  const paramsWin = `--data-raw ^"{\\"param1\\":\\"value1\\",\\"param2\\":\\"value2\\"}^"`;
   const paramsPosix = `--data-raw '{"param1":"value1","param2":"value2"}'`;
 
   let curlCommand = Curl.generateCommand(data, "WINNT");
diff --git a/devtools/client/shared/test/xpcshell/test_curl.js b/devtools/client/shared/test/xpcshell/test_curl.js
index a2a6c3412ee..a04d0462fc9 100644
--- a/devtools/client/shared/test/xpcshell/test_curl.js
+++ b/devtools/client/shared/test/xpcshell/test_curl.js
@@ -230,18 +230,15 @@ add_task(async function () {
   );
 
   // Check binary data
-  const dataBinaryPos = cmd.indexOf("--data-binary");
-  const dataBinaryParam = `--data-binary ${isWin() ? "" : "$"}${escapeNewline(
-    quote(request.postDataText)
-  )}`;
+  const dataBinaryParam = `--data-binary \\\n  $'------------14808\\r\\n`;
   Assert.notStrictEqual(
-    dataBinaryPos,
+    cmd.indexOf("--data-binary"),
     -1,
     "--data-binary param present in curl output"
   );
-  equal(
-    cmd.substr(dataBinaryPos, dataBinaryParam.length),
-    dataBinaryParam,
+
+  Assert.ok(
+    cmd.includes(dataBinaryParam),
     "proper multipart data present in curl output"
   );
 });
@@ -353,14 +350,6 @@ function quote(str) {
   return QUOTE + escaped + QUOTE;
 }
 
-function escapeNewline(txt) {
-  if (isWin()) {
-    // Add `"` to close quote, then escape newline outside of quote, then start new quote
-    return txt.replace(/[\r\n]{1,2}/g, '"^$&$&"');
-  }
-  return txt.replace(/\r/g, "\\r").replace(/\n/g, "\\n");
-}
-
 // Header param is formatted as -H "Header: value" or -H 'Header: value'
 function headerParam(h) {
   return "-H " + quote(h);
Loading diff…