[Web Activities] Activity Handler 'href' parameter must be sanitized to prevent abuse

RESOLVED FIXED in Firefox 21

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: pauljt, Assigned: Bobby Holley (On Leave Until June 11th))

Tracking

Trunk
B2G C4 (2jan on)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
blocking-basecamp: ? → +

Comment 1

6 years ago
hey mounir, do you think you can take a look?
Assignee: nobody → mounir
Assignee: mounir → bobbyholley+bmo
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)
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.
(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.
Created attachment 697163 [details] [diff] [review]
Validate the href field of ActivityHandlerDescription during manifest parsing. v1
Attachment #697163 - Flags: review?(mounir)
Created attachment 697164 [details] [diff] [review]
Tests. v1
Attachment #697164 - Flags: review?(mounir)
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 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

6 years ago
Target Milestone: --- → B2G C4 (2jan on)
(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.
(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.
Flags: needinfo?(mounir)
(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)
Created attachment 698772 [details] [diff] [review]
Validate Activity and Message hrefs during manifest parsing. v2
Attachment #698772 - Flags: review?(mounir)
Created attachment 698774 [details] [diff] [review]
Tests. v3 r=mounir

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+
Attachment #697163 - Attachment is obsolete: true
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+
https://hg.mozilla.org/mozilla-central/rev/f58e59682bef
https://hg.mozilla.org/mozilla-central/rev/393ab74d5a63
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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 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.
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.
You need to log in before you can comment on or make changes to this bug.