Regex injection in enable(namespaces)

[adamcohenrose]

Coverity static analysis is complaining that enable(namespaces) uses an unescaped user input as the basis for a regular expression.

It follows the path from the user-defined window.localStorage.debug value through the load() function in browser.js into the enable(namespaces) function in common.js.

I understand that this debug input is used to control what is logged or not -- but it leaves the library (and any dependent ones) open to receiving crafted input that could cause a denial of service attack on the user's browser (ReDoS attack). I don't believe this is an issue for a server-side DoS attack -- as the input on the server comes from an environment variable rather than the less-protected browser context.

One solution might be to look at something like https:/davisjam/safe-regex to defend against some types of problematic regexes -- there are other suggestions in that repo's readme as well.

[Qix-]

Which version, and can you please show the report?

[adamcohenrose]

The coverity report was for version 2.6.9 but I believe I can trace the same path in the current master branch.

I'm not sure how to include the report here -- it marks up the code with hyperlinks and comments...

Extracts from debug.js v.2.6.9 (but similar path in latest master browser.js and common.js):

browser.js#L13 picks up the browser local storage:

exports.storage = 'undefined' != typeof chrome
               && 'undefined' != typeof chrome.storage
                  ? chrome.storage.local
                  : localstorage();

browser.js#L150 returns the local storage from the load() function:

function load() {
  var r;
  try {
    r = exports.storage.debug;
  } catch(e) {}
  ...
  return r;
}

browser.js#L168 sends the local storage to enable():

exports.enable(load());

debug.js#L151 and L153 create a regex using the user defined input:

function enable(namespaces) {
  ...
  var split = (typeof namespaces === 'string' ? namespaces : '').split(/[\s,]+/);
  var len = split.length;

  for (var i = 0; i < len; i++) {
    if (!split[i]) continue; // ignore empty strings
    namespaces = split[i].replace(/\*/g, '.*?');
    if (namespaces[0] === '-') {
      exports.skips.push(new RegExp('^' + namespaces.substr(1) + '$'));    // <-- HERE
    } else {
      exports.names.push(new RegExp('^' + namespaces + '$'));    // <-- AND HERE
    }
  }
}

[Qix-]

Ah yeah okay. I've always felt awkward about how this was handled. This should be factored into the .enable() overhaul. I'll add it to the 5.x list :)

Thanks for the details!

[malcomio]

This is still being flagged by scanners such as Checkmarx when using debug 4.3.6 - see https://devhub.checkmarx.com/cve-details/Cx8bc4df28-fcf5/

[Qix-]

Yep, aware. It's not a high risk. If it's flagged as such, it's yet another example of the broken CVE system. It's not a high risk.

It's not been fixed, and likely won't be very soon. If you're actually affected by this in the wild then you probably have some XSS vector yourself or you're allowing users direct access to environment variables or something else equally as dangerous, which even with a fix won't protect you from malicious behavior.

If you're controlling the namespaces yourself (the case 99.9999% of the time, I would imagine) then this won't affect you whatsoever. Sorry the CVE registries can't do basic due-diligence.

I want to reiterate the point that this was never raised to me privately or publicly before the "vulnerability" here was registered, even just to ask what I thought the severity might be. For me, it calls into question the legitimacy of services like Checkmarx (which is a for profit entity).

Locking this for now, not because of your responses or followups but because of the long history of auto-pingbacks Github adds which you seemingly cannot hide.