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.

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

Vulnerable Regular Expression #92

Closed
cristianstaicu opened this issue Sep 5, 2017 · 22 comments
Closed

Vulnerable Regular Expression #92

cristianstaicu opened this issue Sep 5, 2017 · 22 comments

Comments

@cristianstaicu
Copy link

cristianstaicu commented Sep 5, 2017

The following regular expression used for parsing the cookie is vulnerable to ReDoS:

/^(([^=;]+))\s*=\s*([^\n\r\0]*)/

The slowdown is moderately low: for 50.000 characters around 2.5 seconds matching time. However, I would still suggest one of the following:

  • remove the regex,
  • limit the number of characters that can be matched by the repetition,
  • limit the input size.
    I noticed there is another bug report regarding the correctness of this regular expression.

If needed, I can provide an actual example showing the slowdown.

@adamwdennis
Copy link

Nice find. An actual example will be helpful in verification of a fix.

@cristianstaicu
Copy link
Author

Sure, here it is:

function genstr(len, chr) {
    var result = "";
    for (i=0; i<=len; i++) {
        result = result + chr;
    }
    return result;
}

var start = process.hrtime();
var tough = require('tough-cookie');
var str =  "x" + genstr(50000, ' ') + "x"; 
var Cookie = tough.Cookie;
var cookie = Cookie.parse(str);
var end = process.hrtime(start);

console.info("Execution time (hr): %ds %dms", end[0], end[1] / 1000000);

@evilpacket
Copy link

@cristianstaicu remember to start your timer right before your parse operation (i.e. after your require and string generation) for accurate time measurements. Given that node limits header size to 80kb the dos is limited to about 7.3 seconds. At Node Security we consider anything over 1 second to be a valid issue.

@eddieajau
Copy link

Just fyi, nsp has this on their radar so anyone with nsp in their CI build pipeline will now be experiencing failing builds.

@gemal
Copy link

gemal commented Sep 21, 2017

https://nodesecurity.io/advisories/525

@tkalfigo
Copy link

Indeed. Got the nsp error from our pre-commit hook. But is there a way to overcome it temporarily until a fix for tough-cookie is in place? I'd like to avoid of course disabling entirely the nsp check.

@Hellenic
Copy link

You could add an exception to your .npmrc for that

@subesokun
Copy link

subesokun commented Sep 21, 2017

@adamwdennis Please fix as our builds are failing and I don't want to whitelist the advisory

@gemal
Copy link

gemal commented Sep 21, 2017

add the following to a .nsprc file:

{
  "exceptions": ["https://nodesecurity.io/advisories/525"]
}

gregory-latinier pushed a commit to steemit/faucet that referenced this issue Sep 21, 2017
Please remove this once this Issue have been fixed and used in the different packages
salesforce/tough-cookie#92
@henrysachs
Copy link

Hey Guys,
do you think it will take long for an update of the package. Because i want to use this in a project of mine and would like this to count on a fix in a decent amount of time. If that sounds kinda harsh it shouldn't. :D

@grnd
Copy link

grnd commented Sep 21, 2017

While we haven't heard from the maintainers, suggesting limiting the number of whitespaces in the key.
#94

@tizmagik
Copy link

Snyk is picking this up now too, https://snyk.io/vuln/npm:tough-cookie:20170905

@stewartjarod
Copy link

Known vulnerability for 16+ days... time to patch guys.

@inikulin
Copy link
Contributor

Once we have #94 issues resolved can someone from @salesforce/tough-cookie-contributors with npm permissions publish an update?

@stash-sfdc
Copy link
Contributor

I apologize for the late pickup of this. I'm closely monitoring this issue and PR now.

@inikulin yes I can push an update as soon as we get a fix.

stash-sfdc added a commit that referenced this issue Sep 21, 2017
Side-steps ReDoS in Issue #92
@bfla
Copy link

bfla commented Sep 21, 2017

My team's CI security checks started failing today due to this issue and I'm happy to see that you are on top of it. Thanks for your work on this! It is much appreciated! 👍

@stash-sfdc
Copy link
Contributor

stash-sfdc commented Sep 21, 2017

Published fix as 2.3.3 - will leave this ticket open until I've resolved it with nsp/snyk

@mshibl
Copy link

mshibl commented Sep 21, 2017

do you know how long it takes nsp to update on their side?

@stash-sfdc
Copy link
Contributor

@mshibl unsure, but I've emailed both nodesecurity and snyk

@guypod
Copy link

guypod commented Sep 21, 2017

We updated Snyk's DB, results should factor in the 2.3.3 fix now.

@stash-sfdc
Copy link
Contributor

Snyk and nodesecurity are both updated. Closing issue.

One more apology (can't help it, 🇨🇦 ): sorry for the delay in fixing this folks. I've fixed my notification settings and email filters so this won't happen again.

I've been working on a change that removes the problematic regex parsing entirely. Hopefully more on this soon, but a preview is on the no-re-parser branch. As a side-effect, it appears to run the entire tough-cookie test suite about 1.5% faster for me!

Thank you to @cristianstaicu @grnd @inikulin and everyone else (especially for your patience)

domenic pushed a commit to jsdom/jsdom that referenced this issue Sep 25, 2017
See: https://www.versioneye.com/Node.JS/tough-cookie/2.3.2; salesforce/tough-cookie#97; salesforce/tough-cookie#92. (A large cookie could cause a slowdown.)

This is not really necessary since fresh installs or installs using package-lock.json already get the 2.3.3 version, but we might as well.
dancrumb added a commit to dancrumb/node-auth0 that referenced this issue Sep 28, 2017
`request` pulls in `tough-cookie` which recently address a ReDoS vulnerability: salesforce/tough-cookie#92
dancrumb added a commit to dancrumb/node-pre-gyp that referenced this issue Sep 28, 2017
Latest version of `request` pulls in `tough-cookie` which recently address a ReDoS vulnerability: salesforce/tough-cookie#92
johnfrench3 pushed a commit to johnfrench3/node-auth-circleci that referenced this issue Aug 11, 2023
`request` pulls in `tough-cookie` which recently address a ReDoS vulnerability: salesforce/tough-cookie#92
wjhsf pushed a commit that referenced this issue Feb 8, 2024
* Winter '20 (API 47.0) prep

* 0.6.0

* use node LTS in CI

* Prerelease 0.6.1 (#98)

* 0.6.1-alpha1

* wip: synthetic shadow dep

* wip: add testEnvironment to preset

* update yargs for security alert (#90)

* update yargs for security alert

* fix test

* Bump eslint-utils from 1.3.1 to 1.4.2 (#89)

Bumps [eslint-utils](https://github.com/mysticatea/eslint-utils) from 1.3.1 to 1.4.2.
- [Release notes](https://github.com/mysticatea/eslint-utils/releases)
- [Commits](mysticatea/eslint-utils@v1.3.1...v1.4.2)

Signed-off-by: dependabot[bot] <[email protected]>

* Update input stub to support "autocomplete" property (#92)

* Update to include date-style and time-style attributes (#94)

* 0.6.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests