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

XSS with location.href behavior of some browsers #4787

Closed
masatokinugawa opened this Issue Aug 2, 2012 · 18 comments

Comments

Projects
None yet
4 participants
@masatokinugawa

masatokinugawa commented Aug 2, 2012

This bug differs from Issue #1990. I tested on Safari 5.1.7 for Windows, Safari Mobile(iOS 5.1.1).
The vector is:

http://l0.cm%2F@jquerymobile.com/demos/1.2.0-alpha.1/#//l0.cm/jqm

These browsers percent-decode "user:password@" part of location.href. I think XSS comes from this behavior.
FYI, this behavior is fixed as CVE-2012-3695 in Safari 6. See: http://support.apple.com/kb/HT5400

@mala

This comment has been minimized.

Show comment
Hide comment
@mala

mala Aug 2, 2012

I tested with android emulator, it works on Android 4.0.3 and not works on Android 4.1.
So, it has influence on over 90% of Android users. http://developer.android.com/about/dashboards/index.html

It's a webkit's bug, but I think that it's also jQuery Mobile's bug.
Same-origin checking should be use location.protocol + "//" + location.host

http://trac.webkit.org/changeset/96779
https://bugs.webkit.org/show_bug.cgi?id=30225

Simple workaround code is here

- documentUrl = path.parseUrl( location.href ),
+ documentUrl = path.parseUrl( location.protocol + "//" + location.host + location.pathname + location.hash ),

mala commented Aug 2, 2012

I tested with android emulator, it works on Android 4.0.3 and not works on Android 4.1.
So, it has influence on over 90% of Android users. http://developer.android.com/about/dashboards/index.html

It's a webkit's bug, but I think that it's also jQuery Mobile's bug.
Same-origin checking should be use location.protocol + "//" + location.host

http://trac.webkit.org/changeset/96779
https://bugs.webkit.org/show_bug.cgi?id=30225

Simple workaround code is here

- documentUrl = path.parseUrl( location.href ),
+ documentUrl = path.parseUrl( location.protocol + "//" + location.host + location.pathname + location.hash ),

@ghost ghost assigned johnbender Aug 6, 2012

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 6, 2012

Contributor

@masatokinugawa

Thanks for the heads up. I'll be addressing this today.

@mala

Unfortunately firefox has the exact same issue but with the location.hash and not the location.href, so we'll have to do some twiddling there. Otherwise I'm going to centralize the handling so there's less chance of someone using location.href in the future to avoid this.

Contributor

johnbender commented Aug 6, 2012

@masatokinugawa

Thanks for the heads up. I'll be addressing this today.

@mala

Unfortunately firefox has the exact same issue but with the location.hash and not the location.href, so we'll have to do some twiddling there. Otherwise I'm going to centralize the handling so there's less chance of someone using location.href in the future to avoid this.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 6, 2012

Contributor

@mala

I'm curious if you can reproduce with window.location.toString(). I'd imagine that the underlying implementation is the same but it would make the fix more simple.

Contributor

johnbender commented Aug 6, 2012

@mala

I'm curious if you can reproduce with window.location.toString(). I'd imagine that the underlying implementation is the same but it would make the fix more simple.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 6, 2012

Contributor

@masatokinugawa @mala

We're now in a position to fix this but I'd like to use window.location.toString() if possible. I'm working on reproducing the issue so I can test it.

Contributor

johnbender commented Aug 6, 2012

@masatokinugawa @mala

We're now in a position to fix this but I'd like to use window.location.toString() if possible. I'm working on reproducing the issue so I can test it.

@mala

This comment has been minimized.

Show comment
Hide comment
@mala

mala Aug 7, 2012

@johnbender

In my knowledge, location.toString cannot be overwritten, even if it can do, it will not necessarily work in the future.

I just forgot location.search, location.href without user:pass is
location.protocol + "//" + location.host + location.pathname + location.search + location.hash
it also works on file:// url.

It's not related but important, "isSameDomain" can load http page from https page.
I think isSameOrigin is required for more strict security.

mala commented Aug 7, 2012

@johnbender

In my knowledge, location.toString cannot be overwritten, even if it can do, it will not necessarily work in the future.

I just forgot location.search, location.href without user:pass is
location.protocol + "//" + location.host + location.pathname + location.search + location.hash
it also works on file:// url.

It's not related but important, "isSameDomain" can load http page from https page.
I think isSameOrigin is required for more strict security.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 7, 2012

Contributor

@mala

In my knowledge, location.toString cannot be overwritten

I'm not sure what you mean. I was curious if window.location.toString() still decoded the user:pass segment. I'm going to try and reproduce tomorrow and if that doesn't work I'll use the string concat.

Thanks for all your help.

Contributor

johnbender commented Aug 7, 2012

@mala

In my knowledge, location.toString cannot be overwritten

I'm not sure what you mean. I was curious if window.location.toString() still decoded the user:pass segment. I'm going to try and reproduce tomorrow and if that doesn't work I'll use the string concat.

Thanks for all your help.

@mala

This comment has been minimized.

Show comment
Hide comment
@mala

mala Aug 7, 2012

@johnbender
window.location.toString() contains decoded username and
window.location.toString = myToStringFunction not work in almost all cases.
getLocation: function() { return … }, is OK.

mala commented Aug 7, 2012

@johnbender
window.location.toString() contains decoded username and
window.location.toString = myToStringFunction not work in almost all cases.
getLocation: function() { return … }, is OK.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 7, 2012

Contributor

@mala

Yah I would never overwrite a built in method like the toString on location even if it were possible.

Contributor

johnbender commented Aug 7, 2012

@mala

Yah I would never overwrite a built in method like the toString on location even if it were possible.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 7, 2012

Contributor

@mala

Unfortunately the string concat doesn't appear to work on Android 2.3 which means we really have no recourse in addressing this issue for that platform. You said this works for Android 4.0.3 though?

Contributor

johnbender commented Aug 7, 2012

@mala

Unfortunately the string concat doesn't appear to work on Android 2.3 which means we really have no recourse in addressing this issue for that platform. You said this works for Android 4.0.3 though?

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 7, 2012

Contributor

@mala

I, of course, am a moron. I'll play around with some regex twiddling to see if I can fix it.

Contributor

johnbender commented Aug 7, 2012

@mala

I, of course, am a moron. I'll play around with some regex twiddling to see if I can fix it.

@johnbender johnbender closed this in 75ba273 Aug 7, 2012

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 7, 2012

Contributor

@mala @masatokinugawa

I would appreciate your eyes on this fix to make sure I've fully addressed the issue.

75ba273
4348eac

Contributor

johnbender commented Aug 7, 2012

@mala @masatokinugawa

I would appreciate your eyes on this fix to make sure I've fully addressed the issue.

75ba273
4348eac

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 7, 2012

Contributor

All,

Examining the possibility that our url parser might also be gamed by a well crafted authority in this case. More later.

Contributor

johnbender commented Aug 7, 2012

All,

Examining the possibility that our url parser might also be gamed by a well crafted authority in this case. More later.

@mala

This comment has been minimized.

Show comment
Hide comment
@mala

mala Aug 9, 2012

@johnbender

Hi,

user:pass in location.href is dependent on browser's implementation.
Non-webkit browsers remove user:pass in location.href by default.

see http://jsrun.it/miya2000/2Lyx

It is not necessary to consider percent encoding.
I think that just remove user:pass from location.href is no problem at all.

mala commented Aug 9, 2012

@johnbender

Hi,

user:pass in location.href is dependent on browser's implementation.
Non-webkit browsers remove user:pass in location.href by default.

see http://jsrun.it/miya2000/2Lyx

It is not necessary to consider percent encoding.
I think that just remove user:pass from location.href is no problem at all.

johnbender added a commit that referenced this issue Aug 9, 2012

strip authority to avoid exploits in parse regex
As explained by @mala in Issue #4787, most browsers simply strip the
authority from `location.href` anyway. We can simply mimick this more
secure behavior for the browsers that don't thereby avoiding the
decoding xss.

johnbender added a commit that referenced this issue Aug 9, 2012

strip authority to avoid exploits in parse regex
As explained by @mala in Issue #4787, most browsers simply strip the
authority from `location.href` anyway. We can simply mimick this more
secure behavior for the browsers that don't thereby avoiding the
decoding xss.
@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Aug 9, 2012

Contributor

@mala

Ah! Sorry to take such a roundabout path to your original solution. I was laboring under the false impression that we should preserve the authority, but if most strip it anyway (which they do in my testing) we can't support that.

Thanks again for all your help.

Contributor

johnbender commented Aug 9, 2012

@mala

Ah! Sorry to take such a roundabout path to your original solution. I was laboring under the false impression that we should preserve the authority, but if most strip it anyway (which they do in my testing) we can't support that.

Thanks again for all your help.

@johnbender johnbender closed this Aug 9, 2012

@ojid49883

This comment has been minimized.

Show comment
Hide comment
@ojid49883

ojid49883 Sep 24, 2012

Please teach it.

Is 1.2.0 RC1/RC2 a modified mistake?

http://www.yahoo.com%2F@jquerymobile.com/demos/1.2.0-beta.1/
getLocation
uri.host = jquerymobile.com
uri.hostname = jquerymobile.com

http://www.yahoo.com%2F@jquerymobile.com/demos/1.2.0-rc.1/
getLocation
uri.host = www.yahoo.com
uri.hostname = www.yahoo.com

http://www.yahoo.com%2F@jquerymobile.com/demos/1.2.0-rc.2/
getLocation
uri.host = www.yahoo.com
uri.hostname = www.yahoo.com

ojid49883 commented Sep 24, 2012

Please teach it.

Is 1.2.0 RC1/RC2 a modified mistake?

http://www.yahoo.com%2F@jquerymobile.com/demos/1.2.0-beta.1/
getLocation
uri.host = jquerymobile.com
uri.hostname = jquerymobile.com

http://www.yahoo.com%2F@jquerymobile.com/demos/1.2.0-rc.1/
getLocation
uri.host = www.yahoo.com
uri.hostname = www.yahoo.com

http://www.yahoo.com%2F@jquerymobile.com/demos/1.2.0-rc.2/
getLocation
uri.host = www.yahoo.com
uri.hostname = www.yahoo.com

@mala

This comment has been minimized.

Show comment
Hide comment
@mala

mala Sep 24, 2012

@johnbender @ojid49883

ah, enbugged by this commit 51d82b0
location.href returns wrong value, you shouldn't use that.

mala commented Sep 24, 2012

@johnbender @ojid49883

ah, enbugged by this commit 51d82b0
location.href returns wrong value, you shouldn't use that.

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Sep 25, 2012

Contributor

Argh.

Sorry two problems confused there. The hash is also autodecoded by firefox, I'll have to get the hash from a parsed form of the url and everything else from the location object directly.

Thanks for keeping an eye on this :(

Contributor

johnbender commented Sep 25, 2012

Argh.

Sorry two problems confused there. The hash is also autodecoded by firefox, I'll have to get the hash from a parsed form of the url and everything else from the location object directly.

Thanks for keeping an eye on this :(

@johnbender johnbender reopened this Sep 25, 2012

@johnbender

This comment has been minimized.

Show comment
Hide comment
@johnbender

johnbender Sep 25, 2012

Contributor

https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.navigation.js#L55

@mala @ojid49883

If you guys can take a quick look that should address the issue. You'll note that it handles both cases.

By default it uses the location objects values and parses the hash out of location.href to avoid FF's decoding of location.hash.

Thanks

Contributor

johnbender commented Sep 25, 2012

https://github.com/jquery/jquery-mobile/blob/master/js/jquery.mobile.navigation.js#L55

@mala @ojid49883

If you guys can take a quick look that should address the issue. You'll note that it handles both cases.

By default it uses the location objects values and parses the hash out of location.href to avoid FF's decoding of location.hash.

Thanks

@johnbender johnbender closed this Sep 25, 2012

arschmitz added a commit to arschmitz/jquery-mobile that referenced this issue Oct 16, 2012

arschmitz added a commit to arschmitz/jquery-mobile that referenced this issue Oct 16, 2012

strip authority to avoid exploits in parse regex
As explained by @mala in Issue #4787, most browsers simply strip the
authority from `location.href` anyway. We can simply mimick this more
secure behavior for the browsers that don't thereby avoiding the
decoding xss.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment