Closed Bug 1082734 Opened 5 years ago Closed 5 years ago
.location .search Params can steal search params from future cross-origin sites
614 bytes, text/html
71.78 KB, patch
|Details | Diff | Splinter Review|
7.24 KB, patch
|Details | Diff | Splinter Review|
7.28 KB, patch
|Details | Diff | Splinter Review|
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.
[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.
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?
Sorry, in the previous comment I meant: ifr.src = "http://something.else"
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.
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.
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.
Wouldn't it make more sense to make URLSearchParams just do the same hardcoded C++ security checks that Location does?
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.
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).
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.
Isn't this just a convenience API? I say drop it, let libraries offer it instead.
Should we disable the API for now, and do that rather soon since this is in Beta.
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.
This patch removes 'searchParams' from URLUtils.
Attachment #8508629 - Flags: review?(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
Attachment #8508629 - Flags: review?(bzbarsky) → review+
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.
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.
Attachment #8508629 - Attachment is obsolete: true
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.)
(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.
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?
Attachment #8509395 - Attachment is obsolete: true
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.
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.
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
sorry had to back this out for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3242159&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/87433694df01 webplatform-tests fixed.
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
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. ;)
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.
Yeah, for beta that seems best and minimally invasive...
Also, you don't have to ask the gaia people, or rather you can ask in the form of a pull request. ;)
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.
Attachment #8510414 - Flags: review?(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.
Attachment #8510414 - Flags: review?(bzbarsky) → review-
I don't know if I understood your comment properly. But maybe this is good enough.
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.
Attachment #8510480 - Flags: review?(bzbarsky) → review+
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=75b492dd54fc Asking for beta approval if fully green on try.
Attachment #8510480 - Attachment is obsolete: true
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.
Attachment #8510872 - Flags: approval-mozilla-beta?
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?
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?
Attachment #8513492 - Flags: review?(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
Attachment #8513492 - Flags: review?(bzbarsky) → review+
I'm unclear which patch you want to uplift at this point. Can you please clarify?
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.
Attachment #8513492 - Flags: approval-mozilla-aurora?
Attachment #8515437 - Attachment is obsolete: true
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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8513492 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8513492 - Flags: sec-approval?
Comment on attachment 8510872 [details] [diff] [review] beta Beta+
Attachment #8510872 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8513492 - Flags: sec-approval? → sec-approval+
Confirmed bug in Fx34, 2014-10-06. Verified fixed in Fx34, Fx35 and Fx36, builds from 2014-11-19.
You need to log in before you can comment on or make changes to this bug.