836707 - Validate the numbers that are passed to SMSManager

Status: RESOLVED WONTFIX

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

Description

[Antonio Manuel Amaya Calvo (:amac)]

+++ This bug was initially created as a clone of Bug #835750 +++

Currently neither Gaia nor Gecko make any kind of control on what they pass to the RIL. The SMS app allows entering text on the address (to search for a contact) but when there's no contact it passes whatever has been written to SMSManager. Another way to get a random string passed is by using the contact API to enter an invalid number. At some point that  string will then be passed as is to the underlying RIL library send sms function.

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 Gaia should make a sanity check of the number it's going to pass to SMSManager before passing it and throw early if it's not a correct number.

Comment 1

[Steve Chung [:steveck]]

Since RIL will handle the number validation in 836708, is this necessary to duplicate the task in both side? It would be nice if we have UX responding for invalid number case, but having the checking logic in gaia seems not so urgent. Thanks.

Comment 2

[Julien Wajsberg [:julienw]]

Actually, the "pass a string to SMSManager" is a feature. The PhoneNumber library converts this to a number using, for each character, the number button that used to have this letter on it on non-smartphones.

If we must do a check, it must be either in PhoneNumber or in SMSManager after the PhoneNumber call. Anyway it must _not_ be in Gaia, but in Gecko. The Sms App is dumb regarding the numbers, and that's expected.

Removing the tef+ flag that was set because of the clone.

Comment 3

[Julien Wajsberg [:julienw]]

So I'm tempted to close this bug invalid, and only keep Bug 836708 that might do something in Gecko (and I'm not even sure we should do that).

Comment 4

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Julien Wajsberg [:julienw] from comment #3)
> So I'm tempted to close this bug invalid, and only keep Bug 836708 that
> might do something in Gecko (and I'm not even sure we should do that).

Answered in bug 836708 about the Gecko part. About this one, I'm a firm believer that following the robustness principle when possible [1]. And that means that if Gecko is going to admit strings with [0-9a-zA-Z+#*]{1;MAX_MSISDN_LENGTH] then Gaia should try to send it only that and not to send it trash, on the reasoning that the higher up something explodes,the less probable is that it'll break something valuable :)

In any case, if this is fixed in Gecko and you don't want to tackle it on Gaia right now, that's acceptable. Not fixing it on any place is not, it would leave a nice way to send random strings of any length to the RIL. 

[1] http://tools.ietf.org/html/rfc761#section-2.10

Comment 5

[Alex Keybl [:akeybl]]

Not a blocker if this functionality is being implemented elsewhere, and would thus be a matter of correctness.

Comment 6

[Alex Keybl [:akeybl]]

mvines - can you confirm whether or not this will be fixed in you RIL, and re-nominate if not?

Comment 7

[Jason Duell]

If bug 836708 fixes this in a RIL-independent way (sounds like it), then we don't need it fixed in the RIL, just in gecko (i.e. we'll block on and fix in bug 836708).

Comment 8

[Borja Salguero [:borjasalguero]]

Attachment: PR

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky):

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

Looks good to me. If anything, I'm not sure if * and # should also be valid characters for SMS numbers. Dunno if the RIL will do anything with it or not.

Comment 10

[Borja Salguero [:borjasalguero]]

[:amac] : Do have I to accept as well '(' ,')', '#', '-' & '*' ???? Because from Contacts one phone number could be created with these characters as well...

Comment 11

[Julien Wajsberg [:julienw]]

Hey, thanks Borja !

However as I said in comment 3, I'm not sure at all we should fix this here. Rather this should be fixed in Gecko and that's all.

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 710117 [details]
PR

also, about the patch, I think we should not silently discard these characters but rather have an error.

Comment 13

[Alhad Purnapatre]

(In reply to Alex Keybl [:akeybl] from comment #6)
> mvines - can you confirm whether or not this will be fixed in you RIL, and
> re-nominate if not?

With the fix suggested in bug 836708, there will be no change required in the RIL layer. I have asked Michael to make the call on nominating bug 836708.

Comment 14

[Julien Wajsberg [:julienw]]

While this specific bug should be closed wontfix, I'd fix both bug 836708 and bug 835750.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.