632121 - Using pushState with a URL in about: or chrome: documents fails due to URL checks

Status: NEW

(Core :: DOM: Navigation, defect, P5)

Description

[Dave Townsend [:mossop]]

Because chrome: and about: don't support usernames the same origin URL checks fail if you try to push a state with a new URL.

Comment 1

[Boris Zbarsky [:bzbarsky]]

pushState is really not designed to work for non-authority schemes....

Comment 2

[Dave Townsend [:mossop]]

So there are three things going on here that cause problems:

The URL check takes two paths depending on whether it is a local file URL or not. Currently chrome and about go down the non-local path, if they went down the local path then it works.

In the non-local path it attempts to get the username and password from the old and new URLs. chrome and about return a failure there which causes the change to fail. Seems to me if both fail to return one then it might make sense to continue.

After that in the non-local path we end up in NS_SecurityCompareURIs. The only problem in here is the checks added by bug 602780.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> The URL check takes two paths depending on whether it is a local file URL

Hmm.  This is the URIIsLocalFile call in nsDocShell::AddState, which checks for the URI_IS_LOCAL_FILE bit?  It does seem like that should be checking the URI_IS_LOCAL_RESOURCE bit instead to me.  Why is it doing what it's doing right now?  I guess it's also not doing a real same-origin check; _that_ difference should probably be restricted to file://....

> Seems to me if both fail to return one then it might make sense to continue.

The problem is that you can't tell whether they failed because they're not nsStandardURL or because they just failed to allocate the string or whatever.

The right way to handle this would be to QI to nsIURL and only worry about userpass if both are nsIURL, I think.

Comment 4

[Dave Townsend [:mossop]]

I guess I'm not sure of the best way to proceed here. chrome: protocol has URI_IS_LOCAL_RESOURCE so that change would help there but about: does not. However if we push about: down that path some other way then basically we're allowing only chrome privileged about: pages to change their url. That is certainly safe and good enough for my purposes.

Maybe rather than messing about with either of the paths there should just be a new test that checks if the calling JS has chrome privs and if so just allows the new url regardless?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> but about: does not.

I wonder why... I guess nothing prevents about: from redirecting to the network.

I don't see why the "calling JS" (which is not a well-defined concept, by the way; we want to use it as little as possible) should matter here.

Comment 6

[Dave Townsend [:mossop]]

Well I guess I mean checking if the principal of the document is the system principal.

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #5)
> > but about: does not.
> 
> I wonder why... I guess nothing prevents about: from redirecting to the
> network.

Indeed, take about:credits f.e.

Comment 8

[Dave Townsend [:mossop]]

Attachment: Use principal.checkMayLoad for local and about: uris

So I guess this is my first pass at something that seems right with a testcase showing the matrix of what works.

Basically when attempting to change the URI if the new URI is local or in the about protocol then we call CheckMayLoad for the current document principal. For other cases we do the same origin check.

The result is that chrome: and privileged about: gets to change its url to anything except http:. Unprivileged about: can't change its url to anything.

The http: and file: cases remain unchanged.

This seems reasonably safe and sane to me, what do you think?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Why is about: special here?  I _really_ dislike scheme checks of various sorts.  This is what we have protocol flags for.

So what are we really trying to check for here?

Comment 10

[Dave Townsend [:mossop]]

So in my opinion chrome: and privileged about: pages should just be allowed to change their URL pretty much as they want. This was the easiest change I could think of to do that. I would also be happy with just allowing them to amend the fragment on the end of the url but that is probably going to work out more complicated to test.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> So in my opinion chrome: and privileged about: pages should just be allowed to
> change their URL pretty much as they want

So perhaps we should be doing a Subsumes() check here, not a same-origin check?

And then fix the codepath that looks for userpass to deal correctly with URIs that don't have it?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 518575 [details] [diff] [review]
Use principal.checkMayLoad for local and about: uris

I don't think this is the right approach.

Comment 13

[Dave Townsend [:mossop]]

I don't have the time for this anymore

Comment 14

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 16

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.