883616 - Bug 882607 resolution introduced a XSS vulnerability

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

In Bug 882607 we removed the latest call to Utils.Message.format, but we forgot to add the escaping back.

This bug needs to be resolved asap and needs leo+ because this is a strong security hazard (ie: a received SMS could inject HTML to our app).

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: patch v1

Carefully escape the body before injecting it into the document
---
 apps/sms/js/thread_ui.js |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Pull request is at https://github.com/mozilla-b2g/gaia/pull/10409

(no review because I haven't tried it yet)

Comment 2

[Julien Wajsberg [:julienw]]

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

tested with a message containing : <a href=plop>plop</a>

Without the patch we see the link, with the patch we see the actual message.

Comment 3

[Julien Wajsberg [:julienw]]

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

requesting from borja too, the earlier review the better.

Comment 4

[Julien Wajsberg [:julienw]]

Jon, do you think it would be possible to create a "security-thread" in sms/mms worklodas, with such crafted html in the messages ?

Comment 5

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> Jon, do you think it would be possible to create a "security-thread" in
> sms/mms worklodas, with such crafted html in the messages ?

Maybe we could also add this to unit tests ?

Comment 6

[Jason Smith [:jsmith]]

In the future - please make sure that any security-sensitive bugs are flagged as such as core-security in the group. We don't want this bug discovered openly.

I'm going with sec-critical here because injection of HTML into the SMS app would allow a malicious web developer to access certified APIs through using the SMS app to manipulate core phone functions in a malicious way.

Comment 7

[:gerard-majax]

Attachment: Tests for ensuring proper escape is done

Please find attached a set of unit tests that should help avoiding this in the future. Since this is a security issue and considering the last comment, those tests could give clue about the issue, hence I've not opened a pull request. If you think it's better to open a new bug, do not hesitate to ask for :)

Comment 8

[Rick Waldron [:rwaldron]]

LGTM (w/ inclusion of tests) (In reply to Alexandre LISSY :gerard-majax from comment #7)
> Created attachment 763281 [details] [diff] [review]
> Tests for ensuring proper escape is done
> 
> Please find attached a set of unit tests that should help avoiding this in
> the future. Since this is a security issue and considering the last comment,
> those tests could give clue about the issue, hence I've not opened a pull
> request. If you think it's better to open a new bug, do not hesitate to ask
> for :)


Both the patch and the tests LGTM

Comment 9

[Borja Salguero [:borjasalguero]]

This patch is not solving all XSS issues. I've just discovered a new vulnerability :S:S:S.

- Create a contact with name '<blink> Hola </blink>'
- Tap on the 'message icon'

EXPECTED:
You will try to send a SMS/MMS to a text <blink> Hola </blink>

CURRENTLY:
'Hola' is blinking!

Please check that every field is working as expected, because this was my first test :(.

Rick, could you take a look as well? Because you know the code behind 'multirecipients', and probably is quite easy to fix with your help. Thanks!

Comment 10

[Borja Salguero [:borjasalguero]]

Definitely this should be leo+. Dietrich could you help with this? Thanks!

Comment 11

[Julien Wajsberg [:julienw]]

Borja, could we handle this in another bug ? I wanted to fix only the regression introduced by Bug 882607 here.

I agree this is also a serious bug, but I'd prefer to keep them separated, if that's ok for you.

Comment 12

[Jon Hylands [:jhylands]]

Julienw - can you provide me with examples of what you want the message content to look like? I assume this will be MMS only? Would the html in question be in the smil, or as an attachment?

Comment 13

[Jason Smith [:jsmith]]

Btw as a FYI - When you do get a r+ patch here, make sure to ask for sec-approval before trying to land it.

Comment 14

[Julien Wajsberg [:julienw]]

> In the future - please make sure that any security-sensitive bugs are flagged as such as core-security in the group. We don't want this bug discovered openly.

I asked my self and decided to go open because it was not in any product yet.

Thanks for the sec-approval reminder.

Alexandre, your tests looks good, will have a look.

Will file another bug for Borja's findings.

Comment 15

[Julien Wajsberg [:julienw]]

Filed bug 883950 to handle XSS in the recipients editor found by borja.

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 763281 [details] [diff] [review]
Tests for ensuring proper escape is done

several problems in these tests :
* they do not pass the linter
* they do not show that we escape " and '
* essentially they're doing 3 times the same thing

I'll take them tomorrow. I'll stub the Utils.escapeHTML function using sinon because what we really want to test is that we're calling it with the payload parameter. escapeHTML already has tests to cover it so we're good :)

Thanks anyway, I'll start from your tests and add them to my patch !

Comment 17

[:gerard-majax]

(In reply to Julien Wajsberg [:julienw] from comment #16)
> Comment on attachment 763281 [details] [diff] [review]
> Tests for ensuring proper escape is done
> 
> several problems in these tests :
> * they do not pass the linter
> * they do not show that we escape " and '
> * essentially they're doing 3 times the same thing
> 
> I'll take them tomorrow. I'll stub the Utils.escapeHTML function using sinon
> because what we really want to test is that we're calling it with the
> payload parameter. escapeHTML already has tests to cover it so we're good :)
> 
> Thanks anyway, I'll start from your tests and add them to my patch !

No problem, it was a first shot dedicated to get some feedback and setup things :)

Comment 18

[Julien Wajsberg [:julienw]]

Attachment: patch v2

Diff to the previous patch:

* added tests for both SMS and MMS cases (thanks Alex for the reminder !)
* removed a strange condition where the body would not be escaped for other types than SMS in threadlist. IMHO MMS don't have bodies right now anyway, but if they ever do (eg: we would display the first text) I want to escape it.

PR is still at https://github.com/mozilla-b2g/gaia/pull/10409

Comment 19

[Julien Wajsberg [:julienw]]

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

Borja, feel free to steal the review too if you have time.

Comment 20

[Corey Frang [:gnarf]]

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

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

Just some small comments that I won't block on.  r=me

::: apps/sms/js/thread_list_ui.js
@@ +368,5 @@
>      if (thread.unreadCount > 0) {
>        li.classList.add('unread');
>      }
>  
> +    if (body) {

I kind of like the idea of explicitly showing HTML in var names.

Perhaps we should just:

var bodyHTML = Utils.escapeHTML(thread.body || '');

And also rename it in the template

::: apps/sms/js/thread_ui.js
@@ +997,5 @@
>      }
>  
>      if (message.type && message.type === 'sms') {
> +      var escapedBody = Utils.escapeHTML(message.body);
> +      bodyHTML = LinkHelper.searchAndLinkClickableData(escapedBody);

Perhaps we can reuse the above var:

var bodyHTML = Utils.escapeHTML(message.body || '');
// .....
bodyHTML = LinkHelper.searchAndLinkClickableData(bodyHTML);

Comment 21

[Jason Smith [:jsmith]]

Julien - Pull requests used on a public repository cannot use any wording to paint a bulls eye at a security hole. Otherwise, malicious users who study our repositories will be able to identify weak points in our codebase and study it carefully to find other exploits we haven't found yet.

Comment 22

[Julien Wajsberg [:julienw]]

Jason: yep, I think I let it out before this bug was closed to external eyes. I guess it's too late ?

Comment 23

[Jason Smith [:jsmith]]

(In reply to Julien Wajsberg [:julienw] from comment #22)
> Jason: yep, I think I let it out before this bug was closed to external
> eyes. I guess it's too late ?

Can you change the title of the pull request to something that doesn't paint the bulls eye?

Comment 24

[Julien Wajsberg [:julienw]]

(In reply to Corey Frang [:gnarf] [:gnarf37] from comment #20)
> Comment on attachment 764672 [details] [diff] [review]
> patch v2
> 
> Review of attachment 764672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just some small comments that I won't block on.  r=me
> 
> ::: apps/sms/js/thread_list_ui.js
> @@ +368,5 @@
> >      if (thread.unreadCount > 0) {
> >        li.classList.add('unread');
> >      }
> >  
> > +    if (body) {
> 
> I kind of like the idea of explicitly showing HTML in var names.
> 
> Perhaps we should just:
> 
> var bodyHTML = Utils.escapeHTML(thread.body || '');
> 
> And also rename it in the template

done.

> 
> ::: apps/sms/js/thread_ui.js
> @@ +997,5 @@
> >      }
> >  
> >      if (message.type && message.type === 'sms') {
> > +      var escapedBody = Utils.escapeHTML(message.body);
> > +      bodyHTML = LinkHelper.searchAndLinkClickableData(escapedBody);
> 
> Perhaps we can reuse the above var:
> 
> var bodyHTML = Utils.escapeHTML(message.body || '');
> // .....
> bodyHTML = LinkHelper.searchAndLinkClickableData(bodyHTML);

I'm not fond of this; variables are cheap (this is not a new instance) so I don't like using bodyHTML if we don't put HTML in it. So I'll keep it like this here.

Jason I renamed the PR and removed the description. Should I also pay attention to the commit log ?

Comment 25

[Julien Wajsberg [:julienw]]

Attachment: patch v3

addressed review comments and carrying r=gnarf 
requesting sec-approval

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
-> quite easily, this is a standard XSS injection.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
-> the patch includes tests, the check-in comment could be changed if necessary


Which older supported branches are affected by this flaw?
-> none, it was a very new regression

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

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

How likely is this patch to cause regressions; how much testing does it need?
-> no regression, this patch is quite small. Many unit tests, real testing happened on the device as well.

Comment 26

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

Comment on attachment 764778 [details] [diff] [review]
patch v3

Thanks for checking but a trunk only security issue doesn't need sec approval. Only if it has gone to branches. Check it in!

Comment 27

[Jason Smith [:jsmith]]

(In reply to Al Billings [:abillings] from comment #26)
> Comment on attachment 764778 [details] [diff] [review]
> patch v3
> 
> Thanks for checking but a trunk only security issue doesn't need sec
> approval. Only if it has gone to branches. Check it in!

That isn't trunk only. This affects two branches - master & v1-train (equivalent of b2g18).

Comment 28

[Corey Frang [:gnarf]]

Comment on attachment 764778 [details] [diff] [review]
patch v3

See above comment from Julien - This is on branches

Comment 29

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

Comment on attachment 764778 [details] [diff] [review]
patch v3

Normally the requirement then is that tests be put into a separate patch that isn't checked in until six weeks or more following the release of the version of Firefox where this issue is fixed. Otherwise, your tests on a security bug create an instant 0 Day by showing people what we changed in detail and what we're wanting to check against.

Comment 30

[Julien Wajsberg [:julienw]]

> Thanks for checking but a trunk only security issue doesn't need sec
> approval. Only if it has gone to branches. Check it in!

Yep, sorry, this is on branch v1-train, I mixed up my sec-approval request, sorry :/


(In reply to Al Billings [:abillings] from comment #29)
> Normally the requirement then is that tests be put into a separate patch
> that isn't checked in until six weeks or more following the release of the
> version of Firefox where this issue is fixed. Otherwise, your tests on a
> security bug create an instant 0 Day by showing people what we changed in
> detail and what we're wanting to check against.

But the truth is that the security bug is in no release of Firefox OS yet. It was a bad check-in that happened last Friday, and was uplifted to the branch the same day.

It's maybe in a beta OTA somewhere, therefore we can maybe wait for the next beta until we checkin the test ? I'd rather have the tests right now to prevent a future stupid mistake like this one...

Comment 31

[Jason Smith [:jsmith]]

(In reply to Al Billings [:abillings] from comment #29)
> Comment on attachment 764778 [details] [diff] [review]
> patch v3
> 
> Normally the requirement then is that tests be put into a separate patch
> that isn't checked in until six weeks or more following the release of the
> version of Firefox where this issue is fixed. Otherwise, your tests on a
> security bug create an instant 0 Day by showing people what we changed in
> detail and what we're wanting to check against.

Note - B2G's releases work far differently, so I don't think argument works here. B2G's 1.01 and 1.1 act more like long running waterfalls in terms of schedule that go for a long time until we figure out when we need to ship.

In terms of this bug, I don't understand the 0 day can happen. This isn't a exploit already possible to show up in one release, wait 6 weeks, then be fixed in the followup. The issue that introduced this bug was during development of 1.1 and pushed to branches that have code not shipped yet.

I think the 0 zero day case is more relevant in such a case as:

We find a sec-critical exploit in 1.01 after it ships. We have a patch to fix it and can push the patch via a FOTA. However, we can't push the tests immediately, as that would expose the exploit to users who have failed to update their phone. That would be a case to wait 6 weeks to land the tests.

Comment 32

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

If we can guarantee that we never ship this issue, then you can check in tests.

Comment 33

[Julien Wajsberg [:julienw]]

Let's do that then. I guess I need the sec-approval+ before doing this though ?

Comment 34

[Julien Wajsberg [:julienw]]

master: 079b57f45f2e272ceb4e0f3f067cc3d829b3f2b4
v1-train: 11b472e74a2032ab56a984f07314654e21bb06f2

Comment 35

[Julien Wajsberg [:julienw]]

John, I don't know if I should merge or cherry-pick in v1.1.0hd ?

Comment 36

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

(In reply to Julien Wajsberg [:julienw] from comment #35)
> John, I don't know if I should merge or cherry-pick in v1.1.0hd ?

This will be merged in to v1.1.0hd today, we're using a merge strategy for commits that should be common to v1.1.0hd and v1-train.

Comment 37

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

1.1hd 11b472e74a2032ab56a984f07314654e21bb06f2