912885 - Possible HTML injection in SMS app

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

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

This line looks dangerous to me in the SMS app:
https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/attachment.js#L185

  // Attach click listeners and fire the callback when rendering is
  // complete: we can't bind `readyCallback' to the `load' event
  // listener because it would break our unit tests.
  container.addEventListener('load', iframeLoad);
  container.src = 'data:text/html,' + tmplSrc;

Since our escaping is done using Template.escape, I am worried that any URL encoded strings will pass unmodified through our santization, and then be unencoded when parsed as a data URI.

If I am tracing correctly, the appears to be in the part of the code where you are drafting a new MMS. So I think the most like exploitation vector here is a malformed web activity. But I am still trying to test this. (having trouble pushing to my leo device to test this at the moment, and sms app no longer works in the simulator for me).

There is a minor mitigation here that the MMS iframe has the sandbox set on it, but same-origin is set, so this doesn't add much protection.

Comment 1

[Julien Wajsberg [:julienw]]

We should urlencode when using a data url, right ?

Otherwise we could try to use container.contentDocument.innerHTML instead ?

Comment 2

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

Yes I think so. The latter seems preferable to me, but only because I don't exactly know what magic happens with data URIs.

Comment 3

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

I have set sec-moderate here based on HTML injection into an app with sensitive permissions.

I don't think script execution is possible due to iframe sandbox. (note however that I think CSP will be disabled on the iframe due to bug 886164). So currently the worst impact I can think of is being able to load arbitrary remote content into the SMS app.

Comment 4

[Julien Wajsberg [:julienw]]

Paul, I think this is in v1.1 too, should we make this bug leo+ ?

Comment 5

[Julien Wajsberg [:julienw]]

Joe, needinfo you because otherwise I think you don't see this bug.

Comment 6

[Julien Wajsberg [:julienw]]

Attachment: patch v1

---
 apps/sms/js/attachment.js             |   17 ++++++++++++-----
 apps/sms/test/unit/attachment_test.js |   25 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 5 deletions(-)

Asking a review from :kaze here, because he did most of the original code.

I need to use innerHTML inside a load handler, because we don't have a contentDocument until after the iframe is appended to the DOM.

There is a github pull request at https://github.com/mozilla-b2g/gaia/pull/12093

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

leo? to triage this on v1.1

Comment 8

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

Re: leo? On it's own, I think this bug allows an attacker to send an MMS to the phone which renders arbitrary HTML content inside a sandboxed iframe. So script is blocked, but my main concern was an attacker could in HTML which navigates the SMS app using <base target='_parent'> or similar. But iframe sandbox seems to block that, even with allow-same-origin set.

So really the main risk here is that an attacker can load remote content into the attachment frame. This could be used to track when the user saw the MMS, or to trigger a separate vulnerability in Gecko. 

A such, I am still leaning to sec-moderate for this. Would we chemspill for this? Not on it's own. So we are only taking critical security bugs at this point, I think it is probably leo-.

Comment 9

[Fabien Cazenave [:kaze]]

Julien, could you add some test that allows us to check that injected code will be escaped? E.g. by setting an image attachment with a data-URL in the unit tests?

Comment 10

[Fabien Cazenave [:kaze]]

Comment on attachment 802587 [details] [diff] [review]
patch v1

r=me, please add unit tests in a follow-up

Comment 11

[Julien Wajsberg [:julienw]]

Attachment: patch v2

rewrote a little the unit tests

carrying r=kaze for this one (but kaze you can have a look if you want)

pushed to github PR and will see if this is still green before merging

Comment 12

[Julien Wajsberg [:julienw]]

Attachment: tests patch v1

separate patch for the malicious unit tests.

I think we're supposed to land those only after the release is out. I've not made a PR out of them.

Comment 13

[Julien Wajsberg [:julienw]]

Paul,

I am supposed to ask sec-approval on the patches, right ?

Also, I'd like to have this leo+ so that I'll be able to land the malicious unit tests earlier. If I wait for january I'll probably forget about them, and I think that's bad.

Comment 14

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

Comment on attachment 803627 [details] [diff] [review]
patch v2

The earlier the better for me, and yes it would be better to land it all together if leo+ is possible.

Comment 15

[Fabien Cazenave [:kaze]]

Comment on attachment 803628 [details] [diff] [review]
tests patch v1

Looks grüt to me.

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 803628 [details] [diff] [review]
tests patch v1

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

Quite easily, need to craft a MMS with a malicious file name in it. Should probably be possible using a normal phone.


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

These are tests especially to test the security problem.


Which older supported branches are affected by this flaw?

1.1


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

I don't remember the actual bug, there were separate bugs for the MMS support.


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

Unrisky, that part is quite the same between gaia master and v1-train. Probably applies without a conflict  (haven't tried it though).


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

Tested the image use case, albeit not other use cases yet, because my music files are too big and my video app is crashing... But should be quite the same, we can ask for test from qa before uplifting.

Comment 17

[Julien Wajsberg [:julienw]]

1st patch master: 9659e823b9461c4ea6baf1477501ac9f202a5ad5

Comment 18

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

Comment on attachment 803628 [details] [diff] [review]
tests patch v1

Review of attachment 803628 [details] [diff] [review]:
-----------------------------------------------------------------

sec-moderate - fine to land tests.

Comment 19

[Julien Wajsberg [:julienw]]

2nd patch master: d4c347b4a4093c86b3b84df2f0af47e4cb19736c

Comment 20

[Jason Smith [:jsmith]]

I'm going to block on this mainly for the zero-day risk - if we land a security bug on master that's a bug in a major release going out the door, then we risk the zero-day attack.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

Was this fixed in 1.1?

Comment 22

[Jason Smith [:jsmith]]

(In reply to Al Billings [:abillings] from comment #21)
> Was this fixed in 1.1?

Doesn't look like it. It didn't get uplifted.

Comment 23

[Julien Wajsberg [:julienw]]

Oops, that's bad.

Will uplift tuesday if John does not do it before this (or if there is a conflict).

Comment 24

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 9659e823b9461c4ea6baf1477501ac9f202a5ad5
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1-train
  git cherry-pick -x  d4c347b4a4093c86b3b84df2f0af47e4cb19736c
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 25

[Julien Wajsberg [:julienw]]

v1-train: 9659e823b9461c4ea6baf1477501ac9f202a5ad5 cherry picked as 0148c21f7186d85b56b8ceff1f17e0835028056b
v1-train: d4c347b4a4093c86b3b84df2f0af47e4cb19736c cherry picked as cdcb99873ad8133a6500884feee5a3e0e957d302

Comment 26

[Julien Wajsberg [:julienw]]

Tim, are you still doing the merges to v1.1.0hd ?

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: cdcb99873ad8133a6500884feee5a3e0e957d302
v1.1.0hd: 0148c21f7186d85b56b8ceff1f17e0835028056b

Comment 28

[Julien Wajsberg [:julienw]]

Hey Preeti,

Flagging you here just to indicate that this landed on 1.1 only last tuesday because it was somehow forgotten. Since it's security-sensitive (although not a lot, see comment 3), I don't know if you intend to include it in a next possible hotfix release.

Comment 29

[Preeti Raghunath(:Preeti)]

Thanks will take a look.