1190139 - HTML injection on System app via Bluetooth device name

Status: RESOLVED FIXED

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

Description

[Muneaki Nishimura]

Attachment: reproduced.png

1. Prepare a device that Firefox OS 2.5.0.0-prelease is running.
   I used my Flame with a firmware id 201507311030207.
2. Prepare a Bluetooth device (e.g., Android phone) that device name is "<s>pwn<iframe src=/></iframe>".
3. On the Firefox OS device, launch Bluetooth menu on Setting app and enable "Visible to all" option.
4. On the Bluetooth device prepared at step 2, transfer a image file to the Firefox OS device.
5. If the bug reproduced an iframe is shown on an "Accept Bluetooth file transfer?" dialog on System app (see reproduced.png).

Note that this attack doesn't require paring the device beforehand and the injected code runs with System app privilege.

Comment 1

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

Fred can you investigate this please assuming you are correct person?

Comment 2

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

Ok I think i found it:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/locales/system.en-US.properties#L203

This is crazy convoluted. We need a better system here since I don't trust a single instance of innerHTML in that file.

In this the flow is:

The deviceName is used to create a title for an event here:

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/bluetooth_transfer.js#L213

    this.getDeviceName(address).then(function(deviceName) {
      Service.request('showCustomDialog', 'acceptFileTransfer',
        {
          id: 'wantToReceiveFile',
          args: {
            deviceName: deviceName,
            fileName: fileName,
            fileSize: fileSize
          }
        },
        cancel,
        confirm
      );
    });

This leads to custom_dialog_service.js and then to custom_dialog.js, which loads a acceptFileTransfer pattern from the localization file above.

Comment 3

[[e-mail]]

Ouch, this issue will effect all dialogs using shared/js/custom_dialog.js

The hole is converged in Bug 1043556 so in 2.2 above version we might only need turn arguments to plain text then format the mozL10n string here

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/custom_dialog.js#L100

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

I think it's the same bug as bug 1190038.

So, it seems that the problem is here:

-- HTML:

mozL10n.setAttributes(element, 'foo', {
  name: "<s>pwn<iframe src=/></iframe>",
});

-- Prop:

foo = {{name}} is my name

---------------

Now, the question is, should we at all allow for HTML in attributes from a developer? Fairly often they will come from the user, and by default I think that they rarely *should* have HTML at all.

While patch from bug 1190038 should fix this bug, I'm still wondering if a bluetooth device name "<strong>foo</strong>" should trigger DOM Overlay.

So maybe we should escape HTML in l10n-args?

Comment 5

[Staś Małolepszy :stas]

We could, yes, but it's not high-sec anymore.  Let's file a new bug for this.

Also note that at least some of the strings in this bug actually use .innerHTML and so they skip the whole overlay sanitization logic and directly assign innerHTML.  We should get some traction on bug 1027117.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: pull request

Ok, this is actually caused by bug 1152232.

We don't need this for bdi, because l10n.js is smart enough to insert directionality marks on its own.

Comment 7

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: screenshot with a fix

Screenshot with the change. It properly resets directionality without <bdi> or innerHTML.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Filed bug 1190539 for escaping l10n vars (not a security bug anymore, but still a potentially unwanted behavior).

Comment 9

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8642552 [details] [review]
pull request

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 1152232
[User impact] if declined: An attacker may use crafted device name to compromise Gaia's system security
[Testing completed]: device
[Risk to taking this patch] (and alternatives if risky): none
[String changes made]: wantToReceiveFile

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/cfc42655ad5ffb9b07454bc12c9164922c427a79

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Paul, what should happen with this bug wrt. 2.2? Are we still getting 2.2 approvals? How should I proceed to fix it in 2.2?

Comment 12

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

There are no devices on 2.2 yet so we are ok just to land ths.  Sec-high bugs have automatic approval to land I believe, according to https://wiki.mozilla.org/Release_Management/B2G_Landing
Ryan, can you help here with the uplift here? (or tell me who to can help?) Thanks.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Setting the appropriate status flags are all that's needed to get it on the uplift radar. More importantly, it makes the bug visible to anybody looking at those bug queries instead of one specific person when ni? is requested.

v2.2: https://github.com/mozilla-b2g/gaia/commit/46676928e1d95d1bcfe4f2425782cba15402dd78

Comment 14

[Daniel Veditz [:dveditz]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> [User impact] if declined: An attacker may use crafted device name to
> compromise Gaia's system security

I thought in comment 4 you said bug 1190038 fixed the security bug?

Comment 15

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Daniel Veditz [:dveditz] from comment #14)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #9)
> > [User impact] if declined: An attacker may use crafted device name to
> > compromise Gaia's system security
> 
> I thought in comment 4 you said bug 1190038 fixed the security bug?

It did. I wasn't sure if l10n.js covers this scenario so I provided a patch that ensures we don't use .innerHTML in this localization, but in fact l10n.js does cover that with https://github.com/mozilla-b2g/gaia/blob/master/shared/js/l10n.js#L1946

So without this patch but with patch from bug 1190038 this bug is not a vector of attack.

Comment 16

[Muneaki Nishimura]

Still I can reproduce this bug on my Flame with the latest 2.5 Nightly. Has the fix been landed?

Comment 17

[Zibi Braniecki [:zbraniecki][:gandalf]]

It landed in bug 1190539, but it seems that we didn't backport it to l10n.js which is used by many apps (and will be in 2.5).

I noticed it in bug 1203905. We'll get the backport on Monday.

Thanks for testing it again and pointing out! :)

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

I backported the patch to l10n.js on gaia master (bug 1190539). It should be fixed starting from today.

Comment 19

[Julien Wajsberg [:julienw]]

Gandalf, is it needed to backport this to eg v2.2 or even previous branches as well?

Comment 20

[Staś Małolepszy :stas]

Yes, we should backport bug 1190539 to v2.2 and v2.1 to safeguard against the foo.innerHTML = {{ dangerousInput }} attack vector.  Let's move the discussion there.