Closed Bug 845045 Opened 11 years ago Closed 11 years ago

Dialer can be tricked into displaying one number but dialing another

Categories

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

x86
macOS
defect
Not set
normal

Tracking

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

RESOLVED FIXED
blocking-b2g tef+
Tracking Status
firefox-esr17 --- unaffected
b2g18 20+ fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: st3fan, Assigned: kgrandon)

References

Details

(Keywords: sec-high)

Attachments

(3 files)

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.
Blocks: 754741
No longer depends on: 754741
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+
Assigning to Kevin for now, please re-assign if you need to.
Assignee: nobody → kgrandon
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.
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.
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.
Looks good. 

Do you think we need a length check too or will the first part of the number always be shown anyway?
Also did you test this with tabs newlines etc? I'm on mobile cant run the test right now.
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.)
Yeah as long as the leftmost part is still shown it is good I think
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)
Attached image 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?
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.
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+
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
Closed: 11 years ago
Resolution: --- → FIXED
v1-train: 57cf61ef7d4a82904b19dc1f592d0433cb428736
v1.0.1: 9cef768a3d6ff2619f0229c5b0a1a5b976fa47c8
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: