1082734 - Saving window.location.searchParams can steal search params from future cross-origin sites

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Benjamin Smedberg]

Attachment: searchParams-testcase.html

Saving the result of window.location.searchParams allows one site to read the search params of future cross-origin visits in the same window.

Testcase attached. This occurs with both iframes and window.open location references.

I believe this is a regression from bug 1037715.

Comment 1

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]: Security bug: cross-site data leak

Yeah, this is bad.  The URLSearchParams in question should have the sorts of security checks Location does or something.

Comment 2

[Andrea Marchesini [:baku]]

annevk, can you update the spec and cover these issues:

1. security exceptions when location.searchParam is used from a different origin.

2. what about this code:

var usp = ifr.contentWindow.location.searchParams;
var u = new URL();
u.searchParams = usp;
usp.src = "http://something.else";

what should 'u.searchParam' be? what should 'u' be?

Comment 3

[Andrea Marchesini [:baku]]

Sorry, in the previous comment I meant: ifr.src = "http://something.else"

Comment 4

[Anne (:annevk)]

Wow, I had not thought of this :-(

The simplest solution seems to be to replace the URLSearchParams object if origin boundaries are crossed. That way the old object is not updated and the new object will not be accessible.

We give the "set the input" algorithm in the specification a "replace flag" argument of sorts. This does mean that the "update steps" of Location objects need to parse the new URL again to be able to make the comparison, but I don't really see a good way around that now.

Comment 5

[Anne (:annevk)]

Ian, I introduced a security bug when URLSearchParams got added to Location. See comment 4 for a potential solution from the perspective of the specification. If you have a different idea let me know.

Comment 6

[Anne (:annevk)]

Andrea, as for your question. new URL() throws. Assuming you passed valid arguments however and something.else is cross-origin, u would still be a URL object associated with usp. ifr.contentWindow.location.searchParams would however be a fresh object.

Now I could imagine that my per-origin URLSearchParams object is not what we want. But that'd we rather change it if the Window/Document changes. That seems fine too, but I'm not sure how to define that specification-wise. Ian would need to weigh in.

Comment 7

[Bobby Holley (:bholley)]

Wouldn't it make more sense to make URLSearchParams just do the same hardcoded C++ security checks that Location does?

Comment 8

[Anne (:annevk)]

It seems safer to have less objects that require special security checks and instead mandate that it gets replaced either as the Document dies or when an origin boundary is crossed.

Otherwise URLSearchParams when associated with a Location object becomes a very special beast.

Comment 9

[Bobby Holley (:bholley)]

Could we also just say that URLSearchParams describes its associated document, and not the document's browsing context's active document à la Location? The difference is only observable in edge cases, and we only do it that way for legacy compat (which URLSearchParams should be free of).

Comment 10

[Anne (:annevk)]

It might be a bit tricky to define that way given that URLUtils is shared across Location, HTMLAnchorElement, HTMLAreaElement, and URL. But I'll let Ian speak to that. If that is better we should probably just do that.

Comment 11

[Hixie (not reading bugmail)]

Isn't this just a convenience API? I say drop it, let libraries offer it instead.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Should we disable the API for now, and do that rather soon since this is in Beta.

Comment 13

[Anne (:annevk)]

We could remove `URLUtils.prototype.searchParams`. That way developers can still use `new URLSearchParams(url.search)` to deal with a URL's query.

That makes URLSearchParams just a simple way to parse and serialize application/x-www-form-urlencoded and leaves all the complexity we have now out.

Comment 14

[Andrea Marchesini [:baku]]

Attachment: searchParams.patch

This patch removes 'searchParams' from URLUtils.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8508629 [details] [diff] [review]
searchParams.patch

Sorry you had to go through all this back-and-forth.  :(


We're going to need a slightly different patch for beta (and maybe Aurora) because we don't want to change the docshell IID there.

Most simply, just leave the IDL be but make nsIDocShell.getURLSearchParams always return null.

>+++ b/dom/base/URL.cpp

Does this still need to be CCed?  Followup bug to make it not be if it's not needed.

It might not need to be nsISupports either, right?  Again, followup.

>+++ b/dom/base/URLSearchParams.cpp

Can we make this one wrappercache: False (and then not wrapper cached and not cced)?  Also might not need to be nsISupports.  Again, followups.

>+++ b/dom/workers/URL.cpp

Again, may not need to be cced, right?

> URL::SetSearch(const nsAString& aSearch, ErrorResult& aRv)

The behavior here is changing in an odd way.  Can we both reach the JS_ReportPendingException and have an error on aRv?  Seems like if both can happen at once that will lead to some weirdness...  Couldn't happen before, since we ignored throws on "rv".  Which behavior is the right one?

Don't you also need to update dom/base/test/test_urlSearchParams.html and dom/base/test/test_location_searchParams.html?

r=me with the nits picked

Comment 16

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5954e9319649

There is a lot of gecko code using URLSearchParams actually. Let's see how many things this patch breaks.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: searchParams.patch

This patch fixes all the tests (actually I'm still waiting for a full green result on try).

But I see 2 major issues with URLSearchParams disjointed from URL:

1.  These 2 are not the same:
var a = new URL("http://foo.bar?a?");
a.searchParams.append('b', 'c');
a.toString() // http://foo.bar/?b=c&a%3F=

var a = new URL("http://foo.bar?a?");
var b =  new URLSearchParams(a.search);
b.append('b', 'c');
a.search = b; a.toString()
a.toString() //http://foo.bar/?%3Fa%3F=&b=c

So the correct way is to do: var b = new URLSearchParams(a.search.substr(1)).

2. URL.search, without URLSearchParams is completely unusable.
The parsing of the string is relatively complex. And also with URLSearchParams you have to do 2 additional steps (the creation of the URLSearchParams object, and then URL.search = that object).

I think we should find a better solution. What about if we block URLSearchParams just for location?
Another temporary solution could be to revert the patch for bug 1037715.

Comment 18

[Anne (:annevk)]

I'm supportive of not supporting this on Location for the time being. I'm also supportive of not supporting searchParams at all for the time being.

(I still have a hard time understanding this problem given that the specification describes Document and Location as having a 1:1 relationship, which would mean that this situation could not occur.)

Comment 19

[Bobby Holley (:bholley)]

(In reply to Anne (:annevk) from comment #18)
> (I still have a hard time understanding this problem given that the
> specification describes Document and Location as having a 1:1 relationship,
> which would mean that this situation could not occur.)

The problem is that the Location object is specced to describe the active document in the browser context, rather than its own document.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: searchParams.patch

ok, so the patch is ready and green on try.
Do we want to proceed in this way or do we want to revert URLSearchParams in location? Both approaches fix the problem.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c39f35d8034dause

How do we want to proceed?

Comment 21

[Anne (:annevk)]

I'm having a hard time deciding (who is this even up to?) between ditching searchParams and requiring developers to use

  new URLSearchParams(someURL.search.substr(1))

and keeping searchParams around for objects other than Location (and requiring the above for Location).

(Comment 9 seems too magical a solution.)

If we ditch it entirely we could make

  new URLSearchParams(someURL)

work at some point in the future. It would automatically grab someURL's query (without putting a "?" in front). It would still be somewhat more awkward API, but given that there's then no links required between the URL object and the URLSearchParams object, it would also be quite a bit simpler.

I'm sort of inclined to opt for ditching searchParams entirely just so we can get feedback from a larger group of people on this before reinstating it in some form.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I would personally be OK with just getting rid of URLSearchParams on Location.

But the |new URLSeachParams((URL or Location or ...))| approach is not terrible.

I agree that it might be a good idea to disable it altogether while we decide which approach to take.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: searchParams.patch

Approval Request Comment
[Feature/regressing bug #]: 1037715
[User impact if declined]: searchParam object can show cross-origin values without security check.
[Describe test coverage new/current, TBPL]: tbpl
[Risks and why]: URL API is a new API so it should not be used so much, but we removed 1 attribute and this could cause problem. Is URL API already in official ff builds?
[String/UUID change made/needed]: none.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fd8079af209a

https://hg.mozilla.org/integration/mozilla-inbound/rev/2340c1d21e94

Comment 24

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3242159&repo=mozilla-inbound

Comment 25

[Andrea Marchesini [:baku]]

Attachment: searchParams.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/87433694df01

webplatform-tests fixed.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for Gaia unit test orange.
https://hg.mozilla.org/integration/mozilla-inbound/rev/874815b0d42b

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3248134&repo=mozilla-inbound

Comment 27

[Boris Zbarsky [:bzbarsky]]

There's actually Gaia code using searchParams: http://mxr.mozilla.org/gaia/source/apps/system/js/net_error.js#109

Looks like that's what the test is testing, in fact, which is nice: actual bustage, not test mock bustage.  ;)

Comment 28

[Andrea Marchesini [:baku]]

it turned out that gaia is using URLSearchParams... so I cannot remove this feature with a single patch but I have to ask people from gaia team to remove the use of url.searchParams from their repo.

I propose to disable location.searchParam instead removing it from everywhere. At least for beta.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Yeah, for beta that seems best and minimally invasive...

Comment 30

[Boris Zbarsky [:bzbarsky]]

Also, you don't have to ask the gaia people, or rather you can ask in the form of a pull request.  ;)

Comment 31

[Andrea Marchesini [:baku]]

Attachment: beta

This patch for beta is just the revert of searchParams for location but without touching nsIDocShell.idl.

I'll propose the same patch for m-i. Then we can take a decision if we want to remove everything or not. It seems that this API is used in gecko and gaia, so probably would be nice to find a different solution instead removing it completely.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8510414 [details] [diff] [review]
beta

The right patch for beta at this point is to simply change the IDL so there is no .searchParams on Location without changing any C++ code.  Imo.

Comment 33

[Andrea Marchesini [:baku]]

Attachment: beta

I don't know if I understood your comment properly. But maybe this is good enough.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8510480 [details] [diff] [review]
beta

Yeah, this seems fine.

Cleaner would be to move .searchParams off URLUtils to another interface and have Location implement URLUtils and all the others implement both URLUtils and the other interface.

Comment 35

[Andrea Marchesini [:baku]]

Attachment: beta

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=75b492dd54fc

Asking for beta approval if fully green on try.

Comment 36

[Andrea Marchesini [:baku]]

Comment on attachment 8510872 [details] [diff] [review]
beta

Approval Request Comment
[Feature/regressing bug #]: 1037715
[User impact if declined]: searchParam object can show cross-origin values without security check.
[Describe test coverage new/current, TBPL]: tbpl
[Risks and why]: This removes location.searchParams without touching the C++ code, but just changing the webild files.
[String/UUID change made/needed]: none.

Comment 37

[Lawrence Mandel [:lmandel] (use needinfo)]

Given that this bug impacts 34-36 and is rated sec-high, I think we're going to want to land this concurrently on all 3 branches. Does searchParams.patch apply cleanly to Aurora and, if so, can you please request Aurora approval?

Comment 38

[Andrea Marchesini [:baku]]

Attachment: second approach

This patch works for m-i/m-c. Bz, can we apply this instead the previous one?
And try to fix the issue at a spec level?

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8513492 [details] [diff] [review]
second approach

I have no idea why I never got bugmail for this review request...

r=me

Comment 40

[Andrea Marchesini [:baku]]

Attachment: patch2.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=818a03936c9a

Comment 41

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm unclear which patch you want to uplift at this point. Can you please clarify?

Comment 42

[Andrea Marchesini [:baku]]

Comment on attachment 8513492 [details] [diff] [review]
second approach

https://hg.mozilla.org/integration/mozilla-inbound/rev/4545e522f4d6

Approval Request Comment
[Feature/regressing bug #]: 1037715
[User impact if declined]: searchParam object can show cross-origin values without security check.
[Describe test coverage new/current, TBPL]: tbpl
[Risks and why]: This removes location.searchParams without touching the C++ code, but just changing the webild files.
[String/UUID change made/needed]: none.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4545e522f4d6

This should have had sec-approval before landing, no?
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 44

[Andrea Marchesini [:baku]]

Comment on attachment 8513492 [details] [diff] [review]
second approach

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

This issue is about having params of another window.location violating the cross-origin policy. It's easy to reproduce.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes.

Which older supported branches are affected by this flaw?

aurora and beta.

If not all supported branches, which bug introduced the flaw?

yes.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

easy.

How likely is this patch to cause regressions; how much testing does it need?

No C++ code is touched. I don't see reasons for regressions.

Comment 45

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8510872 [details] [diff] [review]
beta

Beta+

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/84f5013c9e13
https://hg.mozilla.org/releases/mozilla-beta/rev/d8080081d33a

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/d8080081d33a

Comment 48

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed bug in Fx34, 2014-10-06.
Verified fixed in Fx34, Fx35 and Fx36, builds from 2014-11-19.