If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Dialer can be tricked into displaying one number but dialing another

RESOLVED FIXED

Status

Firefox OS
Gaia::Dialer
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: st3fan, Assigned: kgrandon)

Tracking

({sec-high})

unspecified
x86
Mac OS X
sec-high

Firefox Tracking Flags

(blocking-b2g:tef+, firefox-esr17 unaffected, b2g1820+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 718072 [details]
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:
"+11231231234                                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.
(Reporter)

Updated

5 years ago
Blocks: 754741
No longer depends on: 754741
(Reporter)

Comment 1

5 years ago
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?
Group: core-security
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.
blocking-b2g: --- → tef+
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
Assigning to Kevin for now, please re-assign if you need to.
Assignee: nobody → kgrandon

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 718113 [details]
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.
(Reporter)

Comment 7

5 years ago
Looks good. 

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

Comment 8

5 years ago
Also did you test this with tabs newlines etc? I'm on mobile cant run the test right now.
(Assignee)

Comment 9

5 years ago
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.)
(Reporter)

Comment 10

5 years ago
Yeah as long as the leftmost part is still shown it is good I think
(Assignee)

Comment 11

5 years ago
Comment on attachment 718113 [details]
Github pull request pointer

Hi Etienne - wondering if you can give this patch a quick review. Thanks!
Attachment #718113 - Flags: review?(etienne)
(Reporter)

Comment 12

5 years ago
Created attachment 718204 [details]
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?
(Assignee)

Comment 13

5 years ago
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.
(Reporter)

Comment 14

5 years ago
Works for me!
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 !
Attachment #718113 - Flags: review?(etienne) → review+
(Assignee)

Comment 16

5 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
tracking-b2g18: --- → 20+
v1-train: 57cf61ef7d4a82904b19dc1f592d0433cb428736
v1.0.1: 9cef768a3d6ff2619f0229c5b0a1a5b976fa47c8
status-b2g18: affected → fixed
status-b2g18-v1.0.1: affected → fixed
status-firefox-esr17: --- → unaffected
Group: core-security
You need to log in before you can comment on or make changes to this bug.