Closed Bug 912885 Opened 7 years ago Closed 6 years ago

Possible HTML injection in SMS app

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:leo+, firefox-esr24 unaffected, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

RESOLVED FIXED
blocking-b2g leo+
Tracking Status
firefox-esr24 --- unaffected
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: pauljt, Assigned: julienw)

Details

(Keywords: sec-moderate)

Attachments

(2 files, 1 obsolete file)

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.
We should urlencode when using a data url, right ?

Otherwise we could try to use container.contentDocument.innerHTML instead ?
Yes I think so. The latter seems preferable to me, but only because I don't exactly know what magic happens with data URIs.
blocking-b2g: --- → koi?
Assignee: nobody → felash
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.
Paul, I think this is in v1.1 too, should we make this bug leo+ ?
Joe, needinfo you because otherwise I think you don't see this bug.
Flags: needinfo?(jcheng)
Attached patch patch v1 (obsolete) — Splinter Review
---
 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
Attachment #802587 - Flags: review?
Attachment #802587 - Flags: review? → review?(kaze)
leo? to triage this on v1.1
blocking-b2g: koi? → leo?
Flags: needinfo?(jcheng)
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-.
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 on attachment 802587 [details] [diff] [review]
patch v1

r=me, please add unit tests in a follow-up
Attachment #802587 - Flags: review?(kaze) → review+
Attached patch patch v2Splinter Review
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
Attachment #802587 - Attachment is obsolete: true
Attachment #803627 - Flags: review+
Attached patch tests patch v1Splinter Review
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.
Attachment #803628 - Flags: review?(kaze)
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.
Flags: needinfo?(ptheriault)
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.
Attachment #803627 - Flags: sec-approval+
Flags: needinfo?(ptheriault)
Comment on attachment 803628 [details] [diff] [review]
tests patch v1

Looks grüt to me.
Attachment #803628 - Flags: review?(kaze) → review+
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.
Attachment #803628 - Flags: sec-approval?
1st patch master: 9659e823b9461c4ea6baf1477501ac9f202a5ad5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 803628 [details] [diff] [review]
tests patch v1

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

sec-moderate - fine to land tests.
Attachment #803628 - Flags: sec-approval? → sec-approval+
2nd patch master: d4c347b4a4093c86b3b84df2f0af47e4cb19736c
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.
blocking-b2g: leo? → leo+
Was this fixed in 1.1?
(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.
Oops, that's bad.

Will uplift tuesday if John does not do it before this (or if there is a conflict).
Flags: needinfo?(jhford)
Flags: needinfo?(felash)
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
Flags: needinfo?(felash)
v1-train: 9659e823b9461c4ea6baf1477501ac9f202a5ad5 cherry picked as 0148c21f7186d85b56b8ceff1f17e0835028056b
v1-train: d4c347b4a4093c86b3b84df2f0af47e4cb19736c cherry picked as cdcb99873ad8133a6500884feee5a3e0e957d302
Flags: needinfo?(jhford)
Flags: needinfo?(felash)
Tim, are you still doing the merges to v1.1.0hd ?
Flags: needinfo?(timdream)
v1.1.0hd: cdcb99873ad8133a6500884feee5a3e0e957d302
v1.1.0hd: 0148c21f7186d85b56b8ceff1f17e0835028056b
Flags: needinfo?(timdream)
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.
Flags: needinfo?(praghunath)
Thanks will take a look.
Flags: needinfo?(praghunath)
Group: b2g-core-security → core-security
Group: core-security
You need to log in before you can comment on or make changes to this bug.