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://github.com/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!