Eight days ago I wrote that npm audit had never seen the worst bug in my own package. I closed that post on the question I thought would have caught it: given that nobody has audited this code, what would I find if I read the places where it hands user input to something that acts on it. I asked it, I found an open redirect on the login route, and 0.3.0 went out with the fix.
0.3.0 shipped with a second open redirect in the same function.
const CASAuthentication = require('cas-authentication-user'); // 0.3.0
const cas = new CASAuthentication({
cas_url: 'https://cas.example.edu/cas',
service_url: 'https://app.example.edu'
});
const req = {
originalUrl: '/\\bad.example.com',
query: {}, // no returnTo, so the new guard has nothing to check
session: { cas_user: 'victim' }
};
cas.bounce_redirect(req, { redirect: (to) => console.log('Location:', to) }, () => {});
Enter fullscreen mode Exit fullscreen mode
Location: //bad.example.com
Enter fullscreen mode Exit fullscreen mode
A Location beginning with two slashes is protocol-relative, so the browser reads what follows as a hostname rather than as a path, and the victim lands on bad.example.com. The request that produces it needs no returnTo parameter, no service ticket and no cooperation from the CAS server; an authenticated client is redirected before any of that happens. That makes it cheaper to reach than the one I had just fixed.
The guard rejects the exact string it then emits
The 0.3.0 fix added isSafeReturnTo, and it is not a weak check. It requires a leading slash, and it refuses a second slash or a backslash in position two, which is to say it was written knowing that //host is protocol-relative and that /\host is the variant browsers normalise into one.
isSafeReturnTo("//bad.example.com") false
isSafeReturnTo("/\\bad.example.com") false
isSafeReturnTo("/safe") true
Enter fullscreen mode Exit fullscreen mode
Now set returnTo to something off-site so the guard fires, and keep the same request path:
const req = {
originalUrl: '/\\bad.example.com',
query: { returnTo: 'https://bad.example.com/phish' }, // rejected by isSafeReturnTo
session: { cas_user: 'victim' }
};
Enter fullscreen mode Exit fullscreen mode
Location: //bad.example.com
Enter fullscreen mode Exit fullscreen mode
The guard did its job. It looked at the attacker’s returnTo, refused it, and fell back to what the code treated as the safe default: the path the request had arrived on. That path was /\bad.example.com, which Node’s url.parse reports with a pathname of //bad.example.com. So the fallback for rejecting a protocol-relative URL was a line that manufactures one. The function rejects the string and then hands the browser the same string, assembled from a different source.
One sink, two inputs, and I audited an input
This is the modelling error, and it is a small one with a large blast radius. I went looking for user input and found returnTo, because returnTo is a query parameter, and a query parameter is what user input looks like. What I should have found is the sink. In 0.3.0 the sink is two lines, at 318 and 708 of index.js:
res.redirect(req.session.cas_return_to);
res.redirect(req.session.cas_return_to || requestPath(req));
Enter fullscreen mode Exit fullscreen mode
Two assignments fill that session value, at lines 315 and 453, and I validated the value that arrives at both of them from the query string. The other value that arrives at both of them is the request path. The second sink falls back to that path on its own as well, and I never classified the request path as input at all. It reads as a fact about the request rather than as something a stranger chooses, though a stranger chooses it completely: it is whatever they put after the hostname in the link they send.
Auditing the input tells you that one road into the variable is guarded; auditing the sink tells you how many roads there are. Those produce the same answer only when the count is one, and I never checked the count.
Both of them were there from the first commit
I assumed, while writing the fix, that the request-path redirect was mine. It was a reasonable guess: 0.3.0 had rewritten the URL handling to support mounted routers, and adding a fresh hole while patching an old one is an ordinary way for that to go. It would also have made a neater story.
It is not what happened. The fork’s first commit, 2019-07-30, assigns url.parse(req.url).path to cas_return_to and redirects to it after validating the ticket, which reproduces on that commit as cas_return_to = "//bad.example.com". Both open redirects are the same age; they were eight days apart in being fixed, not seven years apart in existing. I only know that because the guess was cheap to check, and I checked it before writing it down.
The inherited line is also not a lapse by kayleecodes1, whose library this forked. url.parse was the URL API in Node when that code was written. The behaviour that makes it dangerous here, reporting /\host as a pathname of //host, is a documented quirk of a parser that predates the WHATWG standard. Node now deprecates it as DEP0169 and says plainly that “CVEs are not issued for url.parse() vulnerabilities”. That is a strong sentence to find in a deprecation notice, and a fair description of where this bug lived for seven years.
What 0.4.0 does instead
0.4.0 parses request URLs with the WHATWG URL API against a base that cannot exist, then reads the origin back off the result and replaces anything that has moved with /. That is the part that matters: the WHATWG parser resolves /\bad.example.com into a URL whose host is bad.example.com, so the escape shows up as a changed origin rather than as a longer path. The check is on the shape of the result rather than on a list of prefixes, which is what makes it cover the forms I did not think to enumerate.
The same run reports zero for both versions:
$ npm i [email protected] && npm audit
found 0 vulnerabilities
Enter fullscreen mode Exit fullscreen mode
That is the same clean bill of health the previous post quoted, on the same package, on the version whose entire selling point was that it had fixed an open redirect. Nobody has filed an advisory against this package’s own code, so there is nothing to match, and there will be nothing to match after 0.4.0 either.
What I could not settle
I do not know whether either redirect was ever exploited, and I have no way to find out; the fork has no telemetry, and a redirect leaves its evidence in someone else’s access logs. I also cannot claim the 0.4.0 check is complete, only that it is a different kind of check. The 0.3.0 guard enumerated bad prefixes, and I am fairly confident that the enumerating is what failed. Even so, “resolve it and compare origins” is an argument about the parser’s behaviour rather than a proof about mine.
The part that generalises
The previous post’s lesson was that found 0 vulnerabilities describes the advisory database rather than the code, and I still think that is right. What I got wrong was the next step. I treated “read the places where it hands user input to something that acts on it” as an instruction to go and find the user input, and user input is the half of that sentence with no fixed cardinality. Two lines in this library redirect to that one session value, two assignments fill it, and one of the sinks falls back to the request path without consulting either.
So the version I would give myself eight days ago is to start at the dangerous call and enumerate backwards. Every assignment to the variable it reads, not the first one that looks like it came from a stranger. And where a guard rejects a value, ask what happens next: a validator’s fallback runs precisely when someone is attacking you, and it is the one path nobody writes a test for. Mine rejected //bad.example.com and then went and rebuilt it.