fix: ReDoS in cookie pair regexp

[grnd]

Limiting whitespaces to 512. While the RFC does not specify a limit on number of whitespace in key, it's unreasonable to have thousands of them.

[salesforce-cla]

Thanks for the contribution! Before we can merge this, we need @grnd to sign the Salesforce Contributor License Agreement.

[grnd]

That test was failing before the change. will look into that, and add one for the ReDoS as well

[inikulin]

@grnd test fails due to abarth/http-state#18. We just need to update it.

[inikulin]

@grnd Have you signed CLA?

[inikulin]

It would be nice to leave a comment for future generations about why we need this limitation.

[inikulin]

And we need a regression test for this.

[arlolra]

It looks like trim() is called on the captured key/val anyways, so why bother with the \s* altogether? It's already in the capture group [^=;].

[arlolra]

Also, note that LOOSE_COOKIE_PAIR seems to have the same problematic pattern.

[inikulin]

And we need a regression test for this.

[stewartjarod]

fixes #92

[stash-sfdc]

For the other ReDoS, we were able to write a test that demonstrated the bug. Would be great to see that as well here. Would be good to also do a systematic review for other ReDoS sites :(

I'll spend some time on this today. @grnd if you don't want to sign the CLA I can reject this PR and develop an alternative fix. Up to you.

[alexanderbird]

I can't sign the CLA, but here are regression tests (adapted from the example given in issue 92 to reproduce the vulnerability) for the COOKIE_PAIR and LOOSE_COOKIE_PAIR. They fail without @grnd's fix, and pass if you apply the fix to both regexes.

@grnd, you may have already written the regression tests, but in case you didn't, feel free to use this.

.addBatch({
  "ReDoS vulnerabilities": {
    topic: function() {
      function genstr(len, chr) {
        var result = "";
        for (var i=0; i<=len; i++) {
          result = result + chr;
        }
        return result;
      }

      var NS_PER_SEC = 1e9;
      var maliciousString = "x"  + genstr(50000, ' ') + "x";

      return {
        maliciousString,
        NS_PER_SEC
      };
      this.callback();
    },
    "cookie pair regex is not vulnerable to ReDoS attacks": function(t) {
      var start = process.hrtime();
      var cookie = Cookie.parse(t.maliciousString);
      var end = process.hrtime(start);

      var elapsedSeconds = end[0] + (end[1] / t.NS_PER_SEC);
      assert.ok(elapsedSeconds < 0.25, "elapsedSeconds must be less than 0.25");
    },
    "loose cookie pair regex is not vulnerable to ReDoS attacks": function(t) {
      var start = process.hrtime();
      var cookie = Cookie.parse(t.maliciousString, { loose: true });
      var end = process.hrtime(start);

      var elapsedSeconds = end[0] + (end[1] / t.NS_PER_SEC);
      assert.ok(elapsedSeconds < 0.25, "elapsedSeconds must be less than 0.25");
    }
  }
})

[westy92]

@alexanderbird Prefer chr.repeat(len) over the custom genstr(len, chr). One string allocation vs. N.

[stash-sfdc]

Going to close this in favor of #97. Thank you @alexanderbird @grnd @westy92