Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Constrain spaces before = to 256 #97

Merged
merged 2 commits into from Sep 21, 2017
Merged

Constrain spaces before = to 256 #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f1ed420
Constrain spaces before = to 256
stash-sfdc Sep 21, 2017
4e2fb0b
Document the 256 spaces limit
stash-sfdc Sep 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Expand Up @@ -134,6 +134,9 @@ else
cookies = [Cookie.parse(res.headers['set-cookie'])];
```

_Potentially non-standard behavior:_ currently, tough-cookie will limit the number of spaces before the `=` to 256 characters.
See [Issue 92](https://github.com/salesforce/tough-cookie/issues/92)

### Properties

Cookie object properties:
Expand Down
8 changes: 6 additions & 2 deletions lib/cookie.js
View file
Open in desktop
Expand Up @@ -53,16 +53,20 @@ var COOKIE_OCTETS = new RegExp('^'+COOKIE_OCTET.source+'+$');

var CONTROL_CHARS = /[\x00-\x1F]/;

// For COOKIE_PAIR and LOOSE_COOKIE_PAIR below, the number of spaces has been
// restricted to 256 to side-step a ReDoS issue reported here:
// https://github.com/salesforce/tough-cookie/issues/92

// Double quotes are part of the value (see: S4.1.1).
// '\r', '\n' and '\0' should be treated as a terminator in the "relaxed" mode
// (see: https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/parsed_cookie.cc#L60)
// '=' and ';' are attribute/values separators
// (see: https://github.com/ChromiumWebApps/chromium/blob/b3d3b4da8bb94c1b2e061600df106d590fda3620/net/cookies/parsed_cookie.cc#L64)
var COOKIE_PAIR = /^(([^=;]+))\s*=\s*([^\n\r\0]*)/;
var COOKIE_PAIR = /^(([^=;]+))\s{0,256}=\s*([^\n\r\0]*)/;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a comment mentioning why we have this new restriction?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, will do


// Used to parse non-RFC-compliant cookies like '=abc' when given the `loose`
// option in Cookie.parse:
var LOOSE_COOKIE_PAIR = /^((?:=)?([^=;]*)\s*=\s*)?([^\n\r\0]*)/;
var LOOSE_COOKIE_PAIR = /^((?:=)?([^=;]*)\s{0,256}=\s*)?([^\n\r\0]*)/;

// RFC6265 S4.1.1 defines path value as 'any CHAR except CTLs or ";"'
// Note ';' is \x3B
Expand Down
113 changes: 112 additions & 1 deletion test/parsing_test.js
View file
Open in desktop
Expand Up @@ -36,6 +36,9 @@ var assert = require('assert');
var tough = require('../lib/cookie');
var Cookie = tough.Cookie;

var LOTS_OF_SEMICOLONS = ';'.repeat(65535);
var LOTS_OF_SPACES = ' '.repeat(65535);

vows
.describe('Parsing')
.addBatch({
Expand Down Expand Up @@ -327,7 +330,7 @@ vows
"way too many semicolons followed by non-semicolon": {
topic: function() {
// takes abnormally long due to semi-catastrophic regexp backtracking
var str = 'foo=bar' + (';'.repeat(65535)) + ' domain=example.com';
var str = 'foo=bar' + LOTS_OF_SEMICOLONS + ' domain=example.com';
return Cookie.parse(str) || null;
},
"parsed": function(c) { assert.ok(c) },
Expand All @@ -336,6 +339,114 @@ vows
"no path": function(c) { assert.equal(c.path, null) },
"no domain": function(c) { assert.equal(c.domain, 'example.com') },
"no extensions": function(c) { assert.ok(!c.extensions) }
},
"way too many spaces": {
topic: function() {
// takes abnormally long due to semi-catastrophic regexp backtracking
var str1 = "x" + LOTS_OF_SPACES + "x";
var str2 = "x x";
var t0 = Date.now();
var cookie1 = Cookie.parse(str1) || null;
var t1 = Date.now();
var cookie2 = Cookie.parse(str2) || null;
var t2 = Date.now();
return { cookie1: cookie1, cookie2: cookie2, dt1: t1-t0, dt2: t2-t1 };
},
"large one doesn't parse": function(c) { assert.equal(c.cookie1, null) },
"small one doesn't parse": function(c) { assert.equal(c.cookie2, null) },
"takes about the same time for each": function(c) {
var long1 = c.dt1 + 1; // avoid 0ms
var short2 = c.dt2 + 1; // avoid 0ms
var ratio = Math.abs(long1 / short2);
assert.lesser(ratio, 250); // if broken, goes 2000-4000x
}
},
"way too many spaces with value": {
topic: function() {
// takes abnormally long due to semi-catastrophic regexp backtracking
var str1 = "x" + LOTS_OF_SPACES + "=x";
var str2 = "x =x";
var t0 = Date.now();
var cookie1 = Cookie.parse(str1) || null;
var t1 = Date.now();
var cookie2 = Cookie.parse(str2) || null;
var t2 = Date.now();
return { cookie1: cookie1, cookie2: cookie2, dt1: t1-t0, dt2: t2-t1 };
},
"large one parses": function(c) {
assert.ok(c.cookie1);
assert.equal(c.cookie1.key, "x");
assert.equal(c.cookie1.value, "x");
},
"small one parses": function(c) {
assert.ok(c.cookie2)
assert.equal(c.cookie2.key, "x");
assert.equal(c.cookie2.value, "x");
},
"takes about the same time for each": function(c) {
var long1 = c.dt1 + 1; // avoid 0ms
var short2 = c.dt2 + 1; // avoid 0ms
var ratio = Math.abs(long1 / short2);
assert.lesser(ratio, 250); // if broken, goes 2000-4000x
}
},
"way too many spaces in loose mode": {
topic: function() {
// takes abnormally long due to semi-catastrophic regexp backtracking
var str1 = "x" + LOTS_OF_SPACES + "x";
var str2 = "x x";
var t0 = Date.now();
var cookie1 = Cookie.parse(str1, {loose:true}) || null;
var t1 = Date.now();
var cookie2 = Cookie.parse(str2, {loose:true}) || null;
var t2 = Date.now();
return { cookie1: cookie1, cookie2: cookie2, dt1: t1-t0, dt2: t2-t1 };
},
"large one parses": function(c) {
assert.ok(c.cookie1);
assert.equal(c.cookie1.key, "");
assert.equal(c.cookie1.value, "x" + LOTS_OF_SPACES + "x");
},
"small one parses": function(c) {
assert.ok(c.cookie2)
assert.equal(c.cookie2.key, "");
assert.equal(c.cookie2.value, "x x");
},
"takes about the same time for each": function(c) {
var long1 = c.dt1 + 1; // avoid 0ms
var short2 = c.dt2 + 1; // avoid 0ms
var ratio = Math.abs(long1 / short2);
assert.lesser(ratio, 250); // if broken, goes 2000-4000x
}
},
"way too many spaces with value in loose mode": {
topic: function() {
// takes abnormally long due to semi-catastrophic regexp backtracking
var str1 = "x" + LOTS_OF_SPACES + "=x";
var str2 = "x =x";
var t0 = Date.now();
var cookie1 = Cookie.parse(str1, {loose:true}) || null;
var t1 = Date.now();
var cookie2 = Cookie.parse(str2, {loose:true}) || null;
var t2 = Date.now();
return { cookie1: cookie1, cookie2: cookie2, dt1: t1-t0, dt2: t2-t1 };
},
"large one parses": function(c) {
assert.ok(c.cookie1);
assert.equal(c.cookie1.key, "x");
assert.equal(c.cookie1.value, "x");
},
"small one parses": function(c) {
assert.ok(c.cookie2)
assert.equal(c.cookie2.key, "x");
assert.equal(c.cookie2.value, "x");
},
"takes about the same time for each": function(c) {
var long1 = c.dt1 + 1; // avoid 0ms
var short2 = c.dt2 + 1; // avoid 0ms
var ratio = Math.abs(long1 / short2);
assert.lesser(ratio, 250); // if broken, goes 2000-4000x
}
}
})
.export(module);