HTML injection on System app via Bluetooth device name

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
Gaia::System
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: Muneaki Nishimura, Assigned: gandalf)

Tracking

({sec-high, wsec-xss})

unspecified
FxOS-S4 (07Aug)
ARM
Gonk (Firefox OS)
sec-high, wsec-xss
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-v2.2r fixed, b2g-master fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
Created attachment 8642128 [details]
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.
Flags: sec-bounty?
Fred can you investigate this please assuming you are correct person?
Flags: needinfo?(gasolin)
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.
Keywords: sec-high

Comment 3

3 years ago
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
Flags: needinfo?(gasolin)
(Assignee)

Comment 4

3 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(stas)
(Assignee)

Comment 6

3 years ago
Created attachment 8642552 [details] [review]
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.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Attachment #8642552 - Flags: review?(stas)
(Assignee)

Comment 7

3 years ago
Created attachment 8642577 [details]
screenshot with a fix

Screenshot with the change. It properly resets directionality without <bdi> or innerHTML.
Attachment #8642552 - Flags: review?(stas) → review+
(Assignee)

Comment 8

3 years ago
Filed bug 1190539 for escaping l10n vars (not a security bug anymore, but still a potentially unwanted behavior).
See Also: → bug 1190539
(Assignee)

Comment 9

3 years ago
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
Attachment #8642552 - Flags: approval-gaia-v2.2?
(Assignee)

Comment 11

3 years ago
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?
Flags: needinfo?(ptheriault)
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.
Flags: needinfo?(ptheriault) → needinfo?(ryanvm)
Keywords: wsec-xss
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
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → fixed
status-b2g-v2.2r: --- → affected
status-b2g-master: --- → fixed
Flags: needinfo?(ryanvm)
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S4 (07Aug)
(Assignee)

Updated

3 years ago
Attachment #8642552 - Flags: approval-gaia-v2.2?
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2r: affected → fixed
(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?
Flags: needinfo?(gandalf)
Flags: sec-bounty? → sec-bounty+
(Assignee)

Comment 15

3 years ago
(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.
Flags: needinfo?(gandalf)

Updated

3 years ago
Group: core-security → core-security-release
(Reporter)

Comment 16

3 years ago
Still I can reproduce this bug on my Flame with the latest 2.5 Nightly. Has the fix been landed?
Flags: needinfo?(gandalf)
(Assignee)

Comment 17

3 years ago
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! :)
Flags: needinfo?(gandalf)
(Assignee)

Comment 18

3 years ago
I backported the patch to l10n.js on gaia master (bug 1190539). It should be fixed starting from today.
Gandalf, is it needed to backport this to eg v2.2 or even previous branches as well?
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.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.