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

[BUG] potential security issue: sending cookies to wrong origin over redirect #70

Closed
glasser opened this issue Jan 18, 2022 · 4 comments · Fixed by #71
Closed

[BUG] potential security issue: sending cookies to wrong origin over redirect #70

glasser opened this issue Jan 18, 2022 · 4 comments · Fixed by #71
Assignees

Comments

@glasser
Copy link
Contributor

glasser commented Jan 18, 2022

(I could not find a more specific place to file security issues; I'm sorry if this was the wrong place.)

What / Why

make-fetch-happen contains code to strip authorization headers upon redirect to hosts other than the originally requested host.

A recently-reported CVE in node-fetch (a different package that make-fetch-happen does not depend on) fixes that package to contain similar logic. However, the node-fetch logic also strips the cookie and cookie2 headers (plus the www-authenticate header, which seems odd because that is a server->client header).

This means that any cookies sent with the request in make-fetch-happen will be sent to the redirect target site, even if it has a different request/origin. Certainly this is not a great match for what happens in the browser, as browser cookies are origin-specific. So it seems like this may be a bug/security issue in make-fetch-happen and that it should strip cookie (and I guess cookie2) on cross-domain redirects.

When

Run this code:

import fetch from "make-fetch-happen";
import { createServer } from "http";

const redirectTargetServer = createServer((req, res) => {
  console.log("Redirect request Authorization", req.headers.authorization);
  console.log("Redirect request Cookie", req.headers.cookie);
  res.writeHead(200);
  res.end();
});

redirectTargetServer.listen(0, () => {
  const redirectTargetPort = redirectTargetServer.address().port;

  const redirectSourceServer = createServer((req, res) => {
    console.log("Original request Authorization", req.headers.authorization);
    console.log("Original request Cookie", req.headers.cookie);
    // We redirect to 127.0.0.1 but make the original request to localhost, so that
    // the domain names differ.
    res.writeHead(302, { Location: `http://127.0.0.1:${redirectTargetPort}` });
    res.end();
  });

  redirectSourceServer.listen(0, async () => {
    console.log("Sending request");
    await fetch(`http://localhost:${redirectSourceServer.address().port}`, {
      headers: { Authorization: "auth", Cookie: "cook" },
    });
    process.exit(0);
  });
});

It prints:

Sending request
Original request Authorization auth
Original request Cookie cook
Redirect request Authorization undefined
Redirect request Cookie cook

You can see that Authorization is stripped on redirect and Cookie is not. (Note that the reproduction deliberately uses localhost for the initial request and 127.0.0.1 for the redirect target; if your system doesn't support connecting to those two names then it may not work.)

Where

All versions of make-fetch-happen

How

Current Behavior

authorization but not cookie is stripped for cross-domain redirects

Steps to Reproduce

See above

Expected Behavior

cookie should probably be stripped too

Who

Not sure what this means

References

https://nvd.nist.gov/vuln/detail/CVE-2022-0235
node-fetch/node-fetch#1449

@glasser
Copy link
Contributor Author

glasser commented Jan 19, 2022

Thanks!

@glasser
Copy link
Contributor Author

glasser commented Jan 19, 2022

I don't suppose there's any chance of an 8.x backport of this? We still use the CacheManager API.

@glasser
Copy link
Contributor Author

glasser commented Jan 25, 2022

(Well, not just an 8.x backport — I don't think you've released the fix at all.)

@wraithgar
Copy link
Member

No, I was waiting on #72 to land, and now #76. Once those two land I will publish, likely today.

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.

2 participants