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

Handle backslashes like Node.js and Chrome #233

Merged
merged 1 commit into from Jul 24, 2015

Conversation

@kara-ryli
Copy link
Contributor

@kara-ryli kara-ryli commented Jul 24, 2015

RFC 2396 section 2.4.3 puts backslashes (\) in the "unwise" list
of characters that aren't allowed in URIs. However, IE, Opera and Chrome
normalize backslashes to slashes (/), as noted in Chromium.

Since URI.js doesn't do this, it creates possible vulnerabilities. For
example:

var page = URI(window.location.href);
var redirect = URI(page.search(true).redirect_uri);
if (page.domain() === redirect.domain()) {
  window.location = redirect.href();
}

This logic will work fine, except when redirect has backslashes in the
host, e.g.

http://i.xss.com\www.example.org/foo

In this case, you'll get:

URI("http://www.example.org").domain();
// example.org
URI("http://i.xss.com\\www.example.org/foo").domain();
// example.org

...yet the browsers will redirect you to

http://i.xss.com/www.example.org/foo

which could be a phishing site.

The supplied change simply replaces all backslashes before the query/hash with slashes. This workaround is also in Node.

[RFC 2396][] section 2.4.3 puts backslashes (`\`) in the "unwise" list
of characters that aren't allowed in URIs. However, IE, Opera and Chrome
normalize backslashes to slashes (`/`), as noted in [Chromium][].

Since URI.js doesn't do this, it creates possible vulnerabilities. For
example:

```js
var page = URI(window.location.href);
var redirect = URI(page.search(true).redirect_uri);
if (page.domain() === redirect.domain()) {
  window.location = redirect.href();
}
```

This logic will work fine, except when `redirect` has backslashes in the
host, e.g.

```
http://i.xss.com\www.example.org/foo
```

In this case, you'll get:

```js
URI("http://www.example.org").domain();
// example.org
URI("http://i.xss.com\\www.example.org/foo").domain();
// example.org
```

...yet the browsers will redirect you to

```
http://i.xss.com/www.example.org/foo
```

which could be a phishing site.

The supplied change simply replaces all backslashes before the query/hash with slashes. This workaround is also in [Node][Node].

[RFC 2396]: https://www.ietf.org/rfc/rfc2396.txt
[Chromium]: https://code.google.com/p/chromium/issues/detail?id=25916
[Node]: https://github.com/joyent/node/blob/386fd24f49b0e9d1a8a076592a404168faeecc34/lib/url.js#L115-L124
rodneyrehm added a commit that referenced this issue Jul 24, 2015
security(parse-url) Handle backslashes like Node.js and Chrome
@rodneyrehm rodneyrehm merged commit 5271f4e into medialize:master Jul 24, 2015
@rodneyrehm
Copy link
Member

@rodneyrehm rodneyrehm commented Jul 24, 2015

I wonder if escaping \ to %5C might be the better choice. I'm going with your suggestion for now, but might later switch to escape instead of replace.

@rodneyrehm
Copy link
Member

@rodneyrehm rodneyrehm commented Jul 24, 2015

I've made your fix also apply to the mutator functions and released v1.16.0

@kara-ryli kara-ryli deleted the backslashes branch Jul 24, 2015
@kara-ryli
Copy link
Contributor Author

@kara-ryli kara-ryli commented Jul 24, 2015

Regarding \ -> %5C the WhatWG spec recommends throwing an error if you encounter invalid characters, but I prefer the above method since it's what the browsers do and doesn't require me to wrap my URL parsing code in a try/catch with untrusted input.

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

Successfully merging this pull request may close these issues.

None yet

2 participants