Closed
Bug 824670
Opened 12 years ago
Closed 12 years ago
[Web Activities] Activity Handler 'href' parameter must be sanitized to prevent abuse
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: pauljt, Assigned: bholley)
References
Details
Attachments
(2 files, 2 obsolete files)
4.35 KB,
patch
|
mounir
:
review+
|
Details | Diff | Splinter Review |
6.49 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
When registering an Activity Handler in a manifest, an app can choose to load a specific page to be loaded if it is chosen. This |href| parameter is not validated at all which may to issues. From some brief testing: - any URL appears to be able to be loaded: - supply a javascript: would be XSS in the system app if not for CSP! (or thats what it looks like from the error message) - supplying a data: uri is allowed, would this be HTML injection into the system app content (CSP prevented me from figuring out where I am injecting, but from the CSP errors it says app://system.gaiamobile.org/index.html) I guess we should be validating the href parameter in the same way that the launch_path manifest parameter is validate? I think this should be blocking given the potential for abuse here.
Updated•12 years ago
|
blocking-basecamp: ? → +
Assignee | ||
Updated•12 years ago
|
Assignee: mounir → bobbyholley+bmo
Assignee | ||
Comment 2•12 years ago
|
||
It looks like we currently solve this for launch_path and entry_points by requiring relative URIs when validating the manifest: http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/AppsUtils.jsm#164 Does this requirement exist / make sense for ActivityHandlerDescription.href? The alternative is to just check if the URI is same-origin with the App. It seems like either would get rid of javascript: URIs, data: URIs, and other nastiness. sicking - thoughts?
Flags: needinfo?(jonas)
Yup, we should require that for activity handler hrefs too.
Flags: needinfo?(jonas)
Assignee | ||
Comment 4•12 years ago
|
||
I'm looking into where to put this check. Am I correct that we don't currently implement nsIDOMNavigatorActivities on the navigator object, relying instead on declarative activities in the manifest? I can't find any implementation of mozRegisterActivityHandler. If so, the best place to put this logic would be in the manifest checker, but I'm concerned that this check will get lost if/when we do implement mozRegisterActivityHandler.
Comment 5•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #4) > Am I correct that we don't currently implement nsIDOMNavigatorActivities on > the navigator object, relying instead on declarative activities in the > manifest? I can't find any implementation of mozRegisterActivityHandler. This is correct. > If so, the best place to put this logic would be in the manifest checker, > but I'm concerned that this check will get lost if/when we do implement > mozRegisterActivityHandler. I guess if you put the check in _createActivitiesToRegister() or in _registerActivitiesForApps(), it might be low enough that mozRegisterActivityHandler would use that code. Also, you can add a comment in the bug regarding mozRegisterActivityHandler implementation or file a follow-up bug blocking mozRegisterActivityHandler's bug.
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #697163 -
Flags: review?(mounir)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #697164 -
Flags: review?(mounir)
Comment 8•12 years ago
|
||
Comment on attachment 697163 [details] [diff] [review] Validate the href field of ActivityHandlerDescription during manifest parsing. v1 Review of attachment 697163 [details] [diff] [review]: ----------------------------------------------------------------- Shouldn't you make sure the following code doesn't pass an absolute URI: https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#444 Same question for: https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#692 Otherwise, maybe instead of throwing, you could call Cu.reportError() and simply do an early return? r=me either way. ::: dom/apps/src/AppsUtils.jsm @@ +25,5 @@ > //dump("-*- AppsUtils.jsm: " + s + "\n"); > } > > +function isAbsoluteURI(aURI) { > + // See bug 810551 Even if you only copied this, could you remove the bug reference? We don't do that unless it is a TODO. @@ +450,5 @@ > > resolveFromOrigin: function(aURI) { > + // This should be enforced higher up, but check it here just in case. > + if (!isAbsoluteURI(aURI)) > + throw new Error("Webapps.jsm: non-relative URI passed to resolveFromOrigin"); nit: add { }
Attachment #697163 -
Flags: review?(mounir) → review+
Comment 9•12 years ago
|
||
Comment on attachment 697164 [details] [diff] [review] Tests. v1 Review of attachment 697164 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure why you are setting |returnValue| to true in all those manifest. Unless you have a reason for that, could you remove it? r=me with that and the two minor comments below fixed. ::: dom/tests/mochitest/webapps/apps/invalid_activity_href.webapp @@ +6,5 @@ > + "returnValue": true > + }, > + "absolute_href": { > + "returnvalue": true, > + "href": "http://www.mozilla.org" Could you put this at the end and name the activity "invalid_href"? ::: dom/tests/mochitest/webapps/apps/invalid_activity_href2.webapp @@ +10,5 @@ > + "href": "/transfer.html" > + }, > + "data_href": { > + "returnvalue": true, > + "href": "data:text/html;base64,PGh0bWw+DQo8aGVhZD4NCjwvaGVhZD4NCjxib2R5Pg0KPGI+SGFoITwvYj4NCjwvYm9keT4NCjwvaHRtbD4=" I think it would be clearer to name this activity "invalid_href".
Attachment #697164 -
Flags: review?(mounir) → review+
Updated•12 years ago
|
Target Milestone: --- → B2G C4 (2jan on)
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #9) > I'm not sure why you are setting |returnValue| to true in all those > manifest. Unless you have a reason for that, could you remove it? I just didn't want without_href to be totally empty. If it's ok for it to be empty then I'm fine removing them. > Could you put this at the end Given the iteration that goes on under the hood, I think there's minor value in positioning the invalid entry differently in invalid_activity_href and invalid_activity_href2. > and name the activity "invalid_href"? Sure. > I think it would be clearer to name this activity "invalid_href". Done.
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #8) > Shouldn't you make sure the following code doesn't pass an absolute URI: > https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#444 > > Same question for: > https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell. > js#692 Are these "messages" from the System Message API? I can't find any docs on it. Where do they come from? If they come from content, then it seems like we should validate that early-on, the same way we check for absolute URIs while parsing the manifest. The check in resolveFromOrigin is more or less supposed to be an assertion. > Otherwise, maybe instead of throwing, you could call Cu.reportError() and > simply do an early return? That seems unideal to me. We'd end up returning a null or garbage URI to consumers that aren't expecting it. Rather than getting consumers to handle it, we should just make sure that absolute URIs never make it that far. But I'm certainly no expert on the conventions of JS-implemented DOM APIs.
Assignee | ||
Updated•12 years ago
|
Flags: needinfo?(mounir)
Comment 12•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #11) > (In reply to Mounir Lamouri (:mounir) from comment #8) > > Shouldn't you make sure the following code doesn't pass an absolute URI: > > https://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm#444 > > > > Same question for: > > https://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell. > > js#692 > > Are these "messages" from the System Message API? I can't find any docs on > it. Where do they come from? If they come from content, then it seems like > we should validate that early-on, the same way we check for absolute URIs > while parsing the manifest. The check in resolveFromOrigin is more or less > supposed to be an assertion. They come from the manifest, like the activities. It is using that format: "messages": [ { "message-name-1": "/relative-url.html" }, { "message-name-2": "/other-relative-url.html" }, ]
Flags: needinfo?(mounir)
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #698772 -
Flags: review?(mounir)
Assignee | ||
Comment 14•12 years ago
|
||
I added some analagous tests for the system message stuff. I don't think it needs more review. Carrying over review from v1.
Attachment #697164 -
Attachment is obsolete: true
Attachment #698774 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Attachment #697163 -
Attachment is obsolete: true
Comment 15•12 years ago
|
||
Comment on attachment 698772 [details] [diff] [review] Validate Activity and Message hrefs during manifest parsing. v2 Review of attachment 698772 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/AppsUtils.jsm @@ +198,5 @@ > + } > + } > + } > + > + // |messages| is an array of items, where each item is eithe a string or nit: "ether"
Attachment #698772 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 16•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0047143a382d
Assignee | ||
Comment 17•12 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f58e59682bef remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/393ab74d5a63
Comment 18•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f58e59682bef https://hg.mozilla.org/mozilla-central/rev/393ab74d5a63
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 19•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/a8d1adfeefd4 https://hg.mozilla.org/releases/mozilla-b2g18/rev/fae4a62da6cf
status-b2g18:
--- → fixed
status-firefox19:
--- → wontfix
status-firefox20:
--- → wontfix
status-firefox21:
--- → fixed
Comment 20•12 years ago
|
||
Comment on attachment 698772 [details] [diff] [review] Validate Activity and Message hrefs during manifest parsing. v2 Review of attachment 698772 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/apps/src/AppsUtils.jsm @@ +470,5 @@ > resolveFromOrigin: function(aURI) { > + // This should be enforced higher up, but check it here just in case. > + if (!isAbsoluteURI(aURI)) { > + throw new Error("Webapps.jsm: non-relative URI passed to resolveFromOrigin"); > + } Why !isAbsoluteURI(aURI)? The aURI should be a relative one and that's why we need to resolve it from origin. This wrong sanity check would destroy our B2G start-up while reading the messages property. Fire bug 828161. Please correct me if I'm wrong.
Updated•12 years ago
|
So philor and I are getting pretty close to backing this out based on the fact that *most* of the marionnette tests since it landed have been red: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=b2g_ics_armv7a_gecko_emulator%20mozilla-inbound%20opt%20test%20marionette-webapi https://tbpl.mozilla.org/?jobname=b2g_ics_armv7a_gecko_emulator%20mozilla-central%20opt%20test%20marionette-webapi https://tbpl.mozilla.org/?tree=Mozilla-B2g18&jobname=b2g_ics_armv7a_gecko_emulator%20mozilla-b2g18%20opt%20test%20marionette-webapi though there are a lot of gaps there, some filling in as we speak due to retriggers.
I landed bug 828161 on mozilla-central (it was already on mozilla-inbound and mozilla-b2g18) instead of backing this out, since it *sounds* like that should fix the red. If not, please back out. And please watch the tree to see if it does (using the URLs above, even); I'm going to sleep.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•