-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
fix: sanitize source url to avoid attack #128
Conversation
lib/utils.js
Outdated
@@ -54,8 +54,23 @@ function stripHttp1ConnectionHeaders (headers) { | |||
return dest | |||
} | |||
|
|||
// Remove host portion (http://, abc://, //foo.com) | |||
const regex = /^((\w+)?:?\/{2,})/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow the issue, but it seems to me that sticking a complicated regular expression in this path is asking for a redos vulnerability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, would you rather replace strings manually? like str.replace('http://', '').replace('https://', '').replace('//', '/')?
I'm open to any other suggestion.
The intent here is to prevent changing base host if set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear how the base would get changed:
> new URL('https://example.com/https://foo.com')
URL {
href: 'https://example.com/https://foo.com',
origin: 'https://example.com',
protocol: 'https:',
username: '',
password: '',
host: 'example.com',
hostname: 'example.com',
port: '',
pathname: '/https://foo.com',
search: '',
searchParams: URLSearchParams {},
hash: ''
}
But, yeah, I think a dest.startsWith(/(http|\/\/)/)
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, yeah, I think a dest.startsWith(/(http|//)/) would be better.
It's not just urls that start with http. See the screenshot, urls starting with // also cause trouble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed there's a regex in startsWith
. It does not support regex.
The reason my original regex may look complicated is because I was trying to
- Remove any protocol string from the beginning (including arbitrary one's like foo://)
- Remove any number of forward slashes in the beginning. If I just remove 2, and if the string is
////foo.com
then it'll still fall prey to the attack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, looking at the docs for the URL
function, I see what the issue is:
The code in this module is:
- If there is not a pre-defined "base" URL, accept
source
as-is - If there is a pre-defined "base" URL, we will always append the given
source
to thebase
It is case 2 that exhibits the problem wherein an absolute source
is ignoring the pre-defined base
. I propose the following:
const destURL
if (base) {
if (/^\/\w/.test(source) === false) {
throw Error('source must be a relative path string')
}
destURL = new URL(input, base)
} else {
destURL = new URL(source)
}
It is my opinion that it is a mistake to provide any non-relative source when a base
is defined. We should just not even be attempting to sanitize it ourselves. The user of the module is doing something incorrectly. Either they are forgetting they defined a base
or they are passing along user input untested. Regardless of which, or whatever else, it is on them to fix the issue.
Also consider:
> new URL('urn:foo:bar', 'https://example.com')
URL {
href: 'urn:foo:bar',
origin: 'null',
protocol: 'urn:',
username: '',
password: '',
host: '',
hostname: '',
port: '',
pathname: 'foo:bar',
search: '',
searchParams: URLSearchParams {},
hash: ''
}
The following is correct and totally valid:
> new URL('/urn:foo:bar', 'https://example.com')
URL {
href: 'https://example.com/urn:foo:bar',
origin: 'https://example.com',
protocol: 'https:',
username: '',
password: '',
host: 'example.com',
hostname: 'example.com',
port: '',
pathname: '/urn:foo:bar',
search: '',
searchParams: URLSearchParams {},
hash: ''
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I had considered throw
ing but thought it'd be breaking change so wasn't sure. But guess it makes sense to throw
when base is defined and url still contains a host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is likely a breaking change. But probably a necessary one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye pushed an update with different approach and no regex checks at all 🎉
I'll update docs once the approach is finalized Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
As reported in fastify/fast-proxy#42, this module too has similar vulnerability.
This PR fixes that.
Checklist
npm run test
andnpm run benchmark
and the Code of conduct