845383 - Dialer accepts super long phone number which breaks the phone until reboot

Status: RESOLVED FIXED

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

Description

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

This is very much related to bug 845361 and will probably be fixed when that bug has been resolved. I'm still filing a separate bug anyway because I think this should not just be checked from the lower level code that sends the dial request to the radio.

Steps to reproduce:

1) Execute the following code:

var reallyLongString = "";
for (var i = 0; i < 12345; i++) {
    reallyLongString += "cheese";
}
new MozActivity({name:'dial',data:{number:reallyLongString}})

2) Dial the number

Expected:

The number is rejected. Erasing the number from the dialer and dialing a legal number works.

Actual:

The phone seems to send the number to the RIL to dial it. After that things are broken and it is not possible to make any call with the phone again until I reboot the device.


Again, I know this will probably be caught when the dial activity has better input validation. But, the fact that I am totally unable to make any calls again until I reboot my phone also shows that the number is going way further down into the lower level bits of the dialing machinery it should be.

I think this bug should be about also doing better input validation in the code around the bits that touch the RIL directly.

For example in gecko/dom/system/gonk/ril_worker.js the dial() function blindly accepts an incoming phone number and puts it into a parcel that is sent to the RIL. Same in RILContentHelper.js.

I think we need more defensive coding there and not trust incoming data.

Comment 1

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

... because I think this should ALSO be checked ...

Grrr.

Comment 2

[Kevin Grandon :kgrandon]

Seems bad - guess we should've gone with your truncation as suggested in 845045. As I just implemented the validation function, I can take this one as well.

Comment 3

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

Yes, but it should also be fixed in the lower levels of the call code. For example i discovered that a bluetooth device can call a number directly, bypassing the keypad or activity checks.

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

This should have been fixed on 836215 and 835750, those bugs were supposed to add sanity checking to the dialed numbers that get passed to the RIL. And those were landed already. Dunno if there was a regression or the tests weren't enough.

Comment 5

[Michael Vines [:m1] [:evilmachines]]

(clearing tef? for now. doesn't sound like a security issue or critical failure for v1.0.1)

Comment 6

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Michael Vines [:m1] [:evilmachines] (Vacation 3/2-3/9) from comment #5)
> (clearing tef? for now. doesn't sound like a security issue or critical
> failure for v1.0.1)
If it is failing as described on a recent build this *is* a security and a serious issue. If something is breaking there's a good chance we're causing a buffer overflow down the line. And I'd that's what's happening this is a serious issue. As I said, this should not happen because it should have been fixed already on bug 836215. But if it's happening still it has to be fixed. 

Renoming because it has to be confirmed and fixed or being declared a non-issue. It cannot just be ignored.

Comment 7

[Michael Vines [:m1] [:evilmachines]]

There are a lot of If's in comment 6, and that's why this not tef+ right now.   We need to understand the issue fully before tef+ can be considered, and even better provide a r+ patch at the same time as the tef? request. 

Of course I am not at all proposing that this be ignored, I am instead requesting that the issue at least be well understood first. 

tef+ is not a prerequisite for performing this analysis.  tef+ just means "land this patch on v1.0.1", and at this point we don't even know what that patch would look like, or if one is even necessary.

Comment 8

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

The super quick fix for this issue can be an improvement to the patch for bug 845045. Simply truncate long numbers.

That won't solve the core issue of course and like Michael I still would like to understand where it actually blows up.

In the longer term we must do proper and strict input validation on phone numbers coming in through WebActivity and BlueTooth APIs. (Maybe there are even more APIs that talk to mozTelephony)

Comment 9

[Marcelino Veiga Tuimil [:sonmarce]]

We can postpone the decision of being a blocker for v1.0.1 or not, but meanwhile we should work on it to get more information for the final decision, so nominating as tracking.

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Which Gecko and Gaia versions were used to make this tests? I've compiled with B2G18 changeset:   118641:c11d91e1b575 and Gaia v1_train 26f9f0a and I can't reproduce it. I'm getting a different error where the MozActivity doesn't play the transition and instead just shows a white screen but that's a different thing I believe. When I get the white screen I can press home-> go to the dialer, it shows a lot of "cheese"s on the dialer 'screen' and when I hit dial... nothing happens.

If I then delete the cheeses and dial a valid number, the number is dialed correctly.

In fact I don't know how it can fail on a post-bug 835750 version. The dialer sanitizes the number before passing it down, and if the sanitized number is longer than 50 chars of length it gives an error and doesn't even try to place the call. See [1]. 

So, for me at least, this is a works-for-me.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/telephony_helper.js#L5

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Asking for QA to see if this issue can be reproduced (to the level of needing to reboot the device) if not, there's no need to track.

Comment 12

[Tony Chung [:tchung]]

Not really clear what this bug is asking for from QA.  if its just pocket dialing a really long number, i just tried entering a number with over 30 -digits and hitting send, and it just clears the number; no crash or reboot.

If someone can give concrete User-initiated steps that cause concern, please add and renom qawanted.   Otherwise, this seems like an edge case that i can't reproduce as a user.

Tested on:
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/aec2f0738fab
Gaia   601f50f4b498dd952008f60dca0be1db4e2ba2ed
BuildID 20130310070201
Version 18.0

Comment 13

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

(In reply to Tony Chung [:tchung] from comment #12)
> Not really clear what this bug is asking for from QA.  if its just pocket
> dialing a really long number, i just tried entering a number with over 30
> -digits and hitting send, and it just clears the number; no crash or reboot.

The number I dialed was 75000 digits long.

> 
> If someone can give concrete User-initiated steps that cause concern, please
> add and renom qawanted.   Otherwise, this seems like an edge case that i
> can't reproduce as a user.

This bug needs to be triggered through a MozActivity() programatically.

But see comment 10, this might already be fixed with the change Antonio talks about.

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Not tracking this, if there was more work done in this area that was low risk enough to be uplifted then it should be nominated.  Also an automated test for a generic 'pocket dialing' scenario might also be useful but it looks like the RIL validation is covering the average use case right now.

Comment 15

[gbennett]

I am unable to repro this issue typing the max allowed characters and making a call. Verified on the latest 1.3 and 1.4.

Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140103004001
Gaia: ae7d05689b6b9ac4ec6182217dfdef06be28e886
Gecko: d9226a660d52
Version: 28.0a2
Firmware Version: v1.2_20131115

Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140103040201
Gaia: 83cc63f728489a24256731adf558354bb2012a59
Gecko: 49d2fce9a86c
Version: 29.0a1
Firmware Version: v1.2_20131115

Comment 16

[Gabriele Svelto [:gsvelto]]

This was fixed as noted by comment 15 though we still have some limitations regarding the maximum number of digits we can input.