845045 - Dialer can be tricked into displaying one number but dialing another

Status: RESOLVED FIXED

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

Description

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Screen shot of the exploited dialer

The dialer does not correctly validate input to the dial activity handler. Using the following code I was able to show a false number on screen while dialing a completely different number.

From my privileged test app I executed the following code:

new MozActivity({name:'dial', data:{number:
"[phone]                                800-FREE-NUMBER"}});

Note the 30+ spaces. By changing that number you can probably perfectly center/align the fake number.

A screenshot of the result in the dialer is attached as to this bug.

:pauljt confirmed that the *first* part of the above number is actually dialed. I will try to reconfirm that on my local network here in Toronto.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

There are some hints on http://en.wikipedia.org/wiki/E.164 about the max number of digits in a phone number. Maybe we can use those rules to build a stricter number validator.

I think ideally we both validate the number very strict but also filter out whitespace and truncate it *before* we display it?

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Looking for an assignee to get an investigation started here. Marking confidential since we're 3 days from v1.0.1 CS and this might not make it in.

Comment 3

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning to Kevin for now, please re-assign if you need to.

Comment 4

[Alex Keybl [:akeybl]]

We need to be looking for the lowest risk solution here. I like Stefan's comment 1 where we just filter out spaces. An alternative (if lower risk) would be to understand why we're not triggering the ellipses at the beginning of the phone number in this case.

Comment 5

[Kevin Grandon :kgrandon]

I believe we're not triggering the ellipses due to the spaces causing the whitespace to break onto a new line. The ellipses code computes the width, which is smaller due to the line breaking, and does not add it. I'd like to solve this by continuing to allow the invocation of the activity, and simply strip the whitespace - so we trigger the ellipsis. I'm looking into this now, but I do think we can have a pretty simple fix here.

Comment 6

[Kevin Grandon :kgrandon]

Attachment: Github pull request pointer

Here is probably the simplest fix that we can implement for the time being. Ideally individual apps will have a better number parser so that they don't invoke the dialer with these malformed numbers - but I agree that we should have this sanity check inside of dialer as well.

Comment 7

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Looks good. 

Do you think we need a length check too or will the first part of the number always be shown anyway?

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Also did you test this with tabs newlines etc? I'm on mobile cant run the test right now.

Comment 9

[Kevin Grandon :kgrandon]

I'll verify with new lines - there is a test for tabs.

We can check for length if you guys want to, but this change will make bad numbers pretty apparent. (You will never assume that you are about to dial a plain 1-800 number for example.)

Comment 10

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Yeah as long as the leftmost part is still shown it is good I think

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 718113 [details]
Github pull request pointer

Hi Etienne - wondering if you can give this patch a quick review. Thanks!

Comment 12

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Screenshot of the fix

Attached is a screenshot of this exploit after applying the fix.

I think it is a huge improvement but I think it would be better if one could see the *beginning* of the number.

Are we sure we can't simply limit the number to 15 digits max so that it will completely fit without left-truncating the number?

Comment 13

[Kevin Grandon :kgrandon]

I am fairly sure that we should be able to truncate to ~20 characters. I believe Finland can have up to 20 digits in their phone numbers, see: http://en.wikipedia.org/wiki/List_of_international_call_prefixes

That said, I think we might have to unfortunately lose any supplied formatting for the dialer display. We could then re-format the number, but then we're starting to see a more complex fix than we originally wanted for this bug.

My recommendation would be to go with the fix we have for now - and we can work on additional truncation and formatting in the future.

Comment 14

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Works for me!

Comment 15

[Etienne Segonzac (:etienne)]

Comment on attachment 718113 [details]
Github pull request pointer

r=me, kudos on the simplicity of the patch and on taking the opportunity to add tests.

On that note, the new test file revealed an issue with the |utils_test.js| file, which is now failing when the whole dialer suite is launched (APP=communications/dialer make test-agent-test).

Can you add the proper
```
if (!this.SettingsListener) {
  this.SettingsListener = null;
}
```
in |utils_test.js| to ensure that the suite runs well?

Thanks !

Comment 16

[Kevin Grandon :kgrandon]

Landed in master: https://github.com/mozilla-b2g/gaia/commit/e47143b2024ea803fede35b9b1b33a77166bb483

I'm still not too familiar with the uplifting process - but let me know if I'm supposed to do that, and I can. Don't want to step on anyone's toes who's monitoring for bugs to uplift.

Comment 17

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

v1-train: 57cf61ef7d4a82904b19dc1f592d0433cb428736
v1.0.1: 9cef768a3d6ff2619f0229c5b0a1a5b976fa47c8