824437 - XSS in SMS app via Activity Handler

Status: RESOLVED FIXED

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

Description

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

The |number| parameter is not sanitized when handling web activities. Any web content can send a malicious message which results in arbitrary content injection in the SMS app. CSP prevents easy exploitation of this issue however. So far I tested a few CSP bypass mechanisms (stuff like http://lcamtuf.coredump.cx/postxss/) but haven't been able to exfiltrate data or anything like that yet. I could spend more time for a POC, but really we should just fix this.

The following activity triggers the issue:

new MozActivity( {"name":"new","data":{"type":"websms/sms","number":"Image<img src='http://test.web/img'>"}});

This request results in a request to http://test.web/img.

Comment 1

[Andreas Gal :gal]

Why was this b+-ed without an assignee? Also no movement since 5 days. Faramarz, can you please triage and find out why this isn't moving?

Comment 2

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

I thought I set this to bb?, not bb+ (so that it would be nominated for basecamp). Not sure how it became bb+ or if I somehow set that (bb+ doesn't usually appear as an option for me).

In any case, CC'ing Borja, since I think you are working on the SMS app.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Needinfo'ing Faramarz re comment 1 as a reminder.

Comment 4

[Francisco Jordano [:arcturus] [:francisco]]

Hi folks,

we can fix this quickly in the gaia side, but I guess this will happen in more places, not just the sms app.

Is basically something related to activities, and passing information from one app to another. This mean, we can repeat this XSS in the dialer app:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/dialer.js#L28

Seems to me that we could filter in gecko using the mime type, in this way we could have a robust solution, not just per app.

As I said before, perhaps now is better just to do a quick fix in the apps, but maybe for v2 we could add this check by mime type in gecko.

Comment 5

[Francisco Jordano [:arcturus] [:francisco]]

After talking with :vingtetun we will fix this using the regular expression filtering in the manifest.

Still think that a long term fix could be the filtering in gecko ... but ... let's do this in gaia.

Also will check with Alberto if this is happening in the dialer

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Francisco Jordano [:arcturus] from comment #5)
> After talking with :vingtetun we will fix this using the regular expression
> filtering in the manifest.
> 
> Still think that a long term fix could be the filtering in gecko ... but ...
> let's do this in gaia.
> 
> Also will check with Alberto if this is happening in the dialer

Blake wants to have some fun with it. Assigning to him.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

We can't seems to reproduce the request to the web server. Paul are you sure it works?

Renominating until it has been confirmed.

Comment 8

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

I'll test again now, but it definitely did work previously.

Comment 9

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

So testing this now, _number_ seems to need to be a number otherwise the SMS app doesnt handle the input. I guess somebody fixed the gaia sms app? 

I think it might have been this patch which fixed it: https://github.com/mozilla-b2g/gaia/commit/fdc0b4fdea7b56b27b2918d4eed9acf8ed83cb96

It added a call to getNormalizedInternationalNumber which is defined here: http://mxr.mozilla.org/gaia/source/apps/sms/js/phoneNumberUtils.js#74

So the input is parsed prior to being rendered which prevents injection.

So this can probably be closed, but I am leaving it open since comment 4 suggests that this is a problem in the dialer too, so leaving this open until that is confirmed closed too.

Comment 10

[David Scravaglieri [:scravag]]

Please renominate if needed.

Comment 11

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

Marking this as closed since as far as I can tell, the dialer isnt vulnerable. The activity handler sends the unsanitized number to  KeypadManager.updatePhoneNumber but this just calls function which sets .textContent for the field. So here we are doing the right thing (and not using innerHTML, which was the previous issue).