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

Status: RESOLVED FIXED

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

Description

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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.

Comment 1

[Doug Turner (:dougt)]

hey mounir, do you think you can take a look?

Comment 2

[Bobby Holley (:bholley)]

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?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yup, we should require that for activity handler hrefs too.

Comment 4

[Bobby Holley (:bholley)]

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

[Mounir Lamouri (:mounir)]

(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.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Validate the href field of ActivityHandlerDescription during manifest parsing. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 8

[Mounir Lamouri (: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 { }

Comment 9

[Mounir Lamouri (:mounir)]

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".

Comment 10

[Bobby Holley (:bholley)]

(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.

Comment 11

[Bobby Holley (:bholley)]

(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.

Comment 12

[Mounir Lamouri (: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" },
  ]

Comment 13

[Bobby Holley (:bholley)]

Attachment: Validate Activity and Message hrefs during manifest parsing. v2

Comment 14

[Bobby Holley (:bholley)]

Attachment: 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.

Comment 15

[Mounir Lamouri (:mounir)]

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"

Comment 16

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=0047143a382d

Comment 17

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f58e59682bef
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/393ab74d5a63

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f58e59682bef
https://hg.mozilla.org/mozilla-central/rev/393ab74d5a63

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/a8d1adfeefd4
https://hg.mozilla.org/releases/mozilla-b2g18/rev/fae4a62da6cf

Comment 20

[Gene Lian (I already quit Mozilla)]

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.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.