Replace bad regexp with trim

[wesleytodd]

The trim regexp this uses is dangerous. For info check here:

http://stackstatus.net/post/147710624694/outage-postmortem-july-20-2016

This does drop support for IE <9, so would be a major version bump, but the potential for exploit here is enough to make it worth it IMO.

[voltace]

Good find. The regular expression is indeed used to support IE<9.

If I understand the article correctly the problem was an outage due to high server side CPU usage, however browser-cookies runs client side. Also, setting a malformed cookie would require javascript or server access in which case a malicious user could do much worse damage.

So I'm wondering is there a scenario left in which the regular expression could be exploited?

[wesleytodd]

Yeah the ramifications are different on the client for sure. But, a mistake by any third party script, be it ad network code, or google tracking, or a browser plugin could cause the tab to crash. Making it look like the site was broken. Either way, using a bad regex should be avoided when possible, right?

[voltace]

I'll do some more research. First of all I'm wondering whether browser trim the white space of cookie names before storing the name:value pairs in document.cookie. Else it may also be worth benchmarking the additional regex processing time (versus using trim) to quantify the impact.

[voltace]

Created a quick test to check whether browsers store additional white space when saving cookie names: https://jsfiddle.net/bhfo5uja/1/

I've ran this jsfiddle on a recent version of Chrome, Firefox, Internet Explorer and Edge. Fortunately all of these browser trim the whitespace of cookie names so it seems (at least these) browsers already take care of excessive white space in the cookie name.

Are you OK with closing this issue?

[wesleytodd]

Based on your test, we should just remove the trim and regex right?

Also, I think you might be miss-understanding the attack. The issue is where there is a string with large whitespace then a trailing non-whitespace char. So the browser trimming wouldn't solve this.

[voltace]

I've read the article you linked more carefully and I had indeed misunderstood the browser-attack. I've now created the following jsfiddle which shows your suggested solution solves the performance exploit: https://jsfiddle.net/0wu9oyry/

Your remark to remove the trim and regex may also be interesting. document.cookie is a cookie-string which is defined as follows in https://tools.ietf.org/html/rfc6265:

   cookie-string = cookie-pair *( ";" SP cookie-pair )
   cookie-pair   = cookie-name "=" cookie-value

SP means a single space, so strictly speaking only a single leading space needs to be removed. Modifying the regex to only remove the leading space would resolve the potential for an exploit. Would this be an acceptable solution to you?

[wesleytodd]

Yeah! This is a great solution, and good sleuthing. Want me to update this? Or have you already taken care of it?

[voltace]

I haven't updated the code, so if you're willing to do so yes that would be great!

ps: Learned a new word today, hadn't heard of sleuthing before ;). This thread does indeed show the effectiveness of deliberating on these kind of issues together.

[wesleytodd]

So I added the leading whitespace to your benchmark and it is still slower than trim. So I actually think we should keep this PR as is.

https://jsfiddle.net/0wu9oyry/1/

[voltace]

The newly added function wasn't called in the jsfiddle:

  .add('regex', test_regex)
  .add('regex_leading', test_regex)

I've fixed the jsfiddle to call the new function and the performance of your new regex seems to be good enough: https://jsfiddle.net/0wu9oyry/2/

[wesleytodd]

Haha, oh yeah, copy paste mistake. Good catch. Still looks like trim is faster though, tested in Chrome and Firefox.

[voltace]

Trim is indeed faster, though I'm still getting 1 million operations a second on my machine so both options are quite fast. The regex is (unfortunately) required to support IE<9.

[wesleytodd]

Hm, so you are not comfortable dropping support for IE9? As of a year ago even MS doesn't support it. Global usage is really low and the only thing a user who needs it would have to do is pin to the 1.0 branch. And in my tests this gets about a 3X speed improvement in your benchmark.

EDIT: https://www.microsoft.com/en-us/WindowsForBusiness/End-of-IE-support

[voltace]

I guess IE8 is mainly used by the pool souls still stuck on Windows XP ;) Though I can imagine a lot of websites are OK with dropping support I'm sure there are also those not willing or allowed to drop support for older IE versions, even if it's only 1% of their user base.

The performance difference may seem large in the jsfiddle because it's a micro benchmark, but in absolute terms both operations are quite cheap. I don't think it's worth dropping support for IE<9 (or maintaining a separate 2.x.x branch) for a few millionth of a second performance gain.

[wesleytodd]

Updates made!