835750 - Validate the numbers that are passed to the RIL to dial

Status: VERIFIED FIXED

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

Description

[Antonio Manuel Amaya Calvo (:amac)]

Currently neither Gaia nor Gecko make any kind of control on what they pass to the RIL. The Dialer doesn't allow to input other things that numbers directly (although it can receive it by WebActivities) and anyone that uses the contact API can store any string they choose on a phone number. That string will then be passed as is to the underlying RIL library.

While it's true that the RIL library should protect itself too, it's bad form to trust completely on that and can facilitate the remote exploit of any kind of shellcode exploits that appear for the RIL libraries. 

I think Gecko should control that what it has received as a number to dial is indeed a number, and throw early otherwise.

Comment 1

[Antonio Manuel Amaya Calvo (:amac)]

Marking as blocking since this is a potential huge security hole, with a very easy fix.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking status-b2g18 and status-b2g18-v1.0.0 as affected, please update the status to fixed once this is verified landed on v1-train/mozilla-b2g18 and v1.0.0/mozilla-b2g18_v_1_0_0

Comment 3

[Germán Toro del Valle (:gtorodelvalle)]

Hi Victoria, we need to show a new informational message to the user when for some reason the device tries to establish a call to a not correct phone number and I was wondering if you could provide me with the text and options (buttons) you would like to show to the user. Thanks! ;-)

Comment 4

[Germán Toro del Valle (:gtorodelvalle)]

Mari Ángeles tells me that this is more an Ayman's type of thing so I include you in the discussion ;-) Thanks!

Comment 5

[Germán Toro del Valle (:gtorodelvalle)]

Attachment: Associated PR.

NOTE: If blocking-basecamp+ is set, just land it for now.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Security code analysis.
User impact if declined: Malicious codes could be dialed.
Testing completed: Tested in Unagui forcing to show the informative message in case the number to be called was not valid. Otherwise, the call is made.
Risk to taking this patch (and alternatives if risky): No risk. We have just included a control to check if the number to dial is a valid one before going on to check the rest of controls (SIM availability, mobile connection availability, etc.).

Comment 6

[Germán Toro del Valle (:gtorodelvalle)]

Hi :stas, just to inform you that we had to include a new informative message to the user in case she tries to call an invalid phone number ;-)

Comment 7

[Francisco Jordano [:arcturus] [:francisco]]

No pretty clear to me why we have 2 gecko bugs related to this, and we are fixing here the gaia part.

I would suggest to keep this bug as a meta bug, and add an extra bug for the gaia validation.

Comment 8

[Germán Toro del Valle (:gtorodelvalle)]

Do you refer to the bugs Antonio mentions this bug depends on? If yes, both are for the SMS app / case so I do not know it is very appropriate those "depends on" flags :-(

Comment 9

[Germán Toro del Valle (:gtorodelvalle)]

Deleting the "need-info" flags from Victoria and Ayman since we discussed it offline ;-)
The message's title should be: "Invalid phone number"
The message's body should be: "The phone number you are calling is not valid."
Thanks guys!

Comment 10

[Francisco Jordano [:arcturus] [:francisco]]

Hi German,

I left you a comment on Github, just adding them here, could you resolve the problems with the unit tests?

I'm starting to be an asshole asking for passing unit tests for those apps that already have.

Thanks!

Comment 11

[Axel Hecht [:Pike]]

Please request info if you need info, but otherwise don't.

Comment 12

[Germán Toro del Valle (:gtorodelvalle)]

Thanks Axel! I was not aware of that late-l10n keyword ;-) Thanks!

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Hi German,

how are those unit test going?

Thanks

Comment 14

[Germán Toro del Valle (:gtorodelvalle)]

Being solved by Fernando Jiménez (https://bugzilla.mozilla.org/show_bug.cgi?id=838622) ;-) I included a comment in the pull request (https://github.com/mozilla-b2g/gaia/pull/7985) asking about the convenience to wait for them to be solved before merging this bug :-) What do you think?

Comment 15

[Francisco Jordano [:arcturus] [:francisco]]

I think that if we opened a bug that is blocking this we should use bugzilla to mark that.

Cheers!

Comment 16

[Germán Toro del Valle (:gtorodelvalle)]

Well, that is what we are discussing... If it is blocking it or not :-p In my opinion, it is not. It is about an absolutely distinct issue :-) Anyhow, since I guess you are suggesting to mark this bug as blocked by the one Fernando is solving, I'll do it :-) Thanks!

Comment 17

[Alex Keybl [:akeybl]]

Please only land once bug 838622 (now tef+) is resolved, to prevent a unit test regression.

Is bug 836708 also a blocker for landing?

Comment 18

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Alex Keybl [:akeybl] from comment #17)
> Is bug 836708 also a blocker for landing?

For landing this one it should not be.

Comment 19

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

(In reply to Alex Keybl [:akeybl] from comment #17)
> Please only land once bug 838622 (now tef+) is resolved, to prevent a unit
> test regression. 

Alex, I don't see tef+ on that bug, or any branch flags for that matter.  Please advise.

> Is bug 836708 also a blocker for landing?

Comment 20

[Alex Keybl [:akeybl]]

(In reply to John Ford [:jhford] from comment #19)
> (In reply to Alex Keybl [:akeybl] from comment #17)
> > Please only land once bug 838622 (now tef+) is resolved, to prevent a unit
> > test regression. 
> 
> Alex, I don't see tef+ on that bug, or any branch flags for that matter. 
> Please advise.

Fixed, it's now tef+

Comment 21

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

This commit does not apply cleanly to v1-train, v1.0.1.  If this patch depends on another bug, please comment here and I will retry when that bug is approved to land on all branches that this bug needs to land on.  If the merge conflict needs to be resolved by hand, the following commands could be a useful starting point:

cd gaia
git checkout v1-train
git cherry-pick -x -m1 0511964bbbd3fbec9810bf8477f50607cf3776a7
<resolve merge conflict>
git checkout v1.0.1
git cherry-pick -x $(git log --pretty=%H -n1 v1-train)


#	both modified:      apps/communications/dialer/js/telephony_helper.js
#	both modified:      apps/communications/dialer/test/unit/mock_utils.js

Comment 22

[Germán Toro del Valle (:gtorodelvalle)]

Hi John, this bug depends on https://bugzilla.mozilla.org/show_bug.cgi?id=838622 as stated in the "depends" on field ;-) I have just evolved that bug to RESOLVED-FIXED since its pull request already landed. Thanks!

(In reply to John Ford [:jhford] from comment #21)
> This commit does not apply cleanly to v1-train, v1.0.1.  If this patch
> depends on another bug, please comment here and I will retry when that bug
> is approved to land on all branches that this bug needs to land on.  If the
> merge conflict needs to be resolved by hand, the following commands could be
> a useful starting point:
> 
> cd gaia
> git checkout v1-train
> git cherry-pick -x -m1 0511964bbbd3fbec9810bf8477f50607cf3776a7
> <resolve merge conflict>
> git checkout v1.0.1
> git cherry-pick -x $(git log --pretty=%H -n1 v1-train)
> 
> 
> #	both modified:      apps/communications/dialer/js/telephony_helper.js
> #	both modified:      apps/communications/dialer/test/unit/mock_utils.js

Comment 23

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

(In reply to gtorodelvalle from comment #22)
> Hi John, this bug depends on
> https://bugzilla.mozilla.org/show_bug.cgi?id=838622 as stated in the
> "depends" on field ;-) I have just evolved that bug to RESOLVED-FIXED since
> its pull request already landed. Thanks!

Hey, I uplifted 838622 and I'm still seeing the merge conflict in apps/communications/dialer/js/telephony_helper.js

Comment 24

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to John Ford [:jhford] from comment #23)
> (In reply to gtorodelvalle from comment #22)
> > Hi John, this bug depends on
> > https://bugzilla.mozilla.org/show_bug.cgi?id=838622 as stated in the
> > "depends" on field ;-) I have just evolved that bug to RESOLVED-FIXED since
> > its pull request already landed. Thanks!
> 
> Hey, I uplifted 838622 and I'm still seeing the merge conflict in
> apps/communications/dialer/js/telephony_helper.js

Hi John, you are completely right ;-) After having a look at it, it seems that the update which is missing is the one provided by Etienne to solve this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=840538 This is the PR: https://github.com/mozilla-b2g/gaia/pull/8063 The code regarding "postponing" the RIL instantiation (var telephony = navigator.mozTelephony;)

Comment 25

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

v1-train: 7d739c1
v1.0.1: caede03

Comment 26

[amiller]

Test Case is not needed in Moztrap for this issue.

Comment 27

[mdaniloff]

Can you please provide steps to verify this fix as we will blackbox test from the UI?

Comment 28

[cbarker]

Antonio if you can Verify this bug out, it would be much appreciated.

Comment 29

[Germán Toro del Valle (:gtorodelvalle)]

As mentioned in bug 845539 and shared yesterday with Antonio via Skype, this is not working. I am discussing with Mari Ángeles the way to proceed ;-) I'll keep you posted.

Comment 30

[Germán Toro del Valle (:gtorodelvalle)]

Please, refer to bug 859719 regarding the testing/validation of this bug. Thanks!

Comment 31

[amiller]

The issue appears to be fixed on v1.1 and v1.0.1 builds. 

Environmental  Variables:
Unagi Build ID: 20130410070209
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/56c922308fd1
Gaia: 0a9f78bffafda93a159c1f502e8b110c2f49a500

Invalid phone number (The phone number you are calling is not valid) Displays as expected.

Comment 32

[Antonio Manuel Amaya Calvo (:amac)]

Clearing the need info here as this was answered by German.

Comment 33

[Johan Lorenzo [:jlorenzo]]

Shally, can you paste the revisions against which you verified?

Comment 34

[Shally Li（leave from Mozilla）]

Per comment 22,I clear "Verifyme".

Comment 35

[Shally Li（leave from Mozilla）]

Sorry,please ingnore Comment 34.
Per comment 31,I clear "Verifyme".

Comment 36

[Shally Li（leave from Mozilla）]

If someone feels convenient, please verify on "b2g18".

Comment 37

[Johan Lorenzo [:jlorenzo]]

No need to verify on this version anymore. We don't have any flame build for that and the code base has probably changed for the past 2 years.

Comment 38

[Shally Li（leave from Mozilla）]

(In reply to Johan Lorenzo [:jlorenzo] (QA) from comment #37)
> No need to verify on this version anymore. We don't have any flame build for
> that and the code base has probably changed for the past 2 years.

OK,thanks