836708 - [B2G RIL] Validate the numbers that are passed from SMSManager.send

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Antonio Manuel Amaya Calvo (:amac)]

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

This is the Gecko counter part of Bug 836707

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

SMS supports alpha-numeric addresses, see bug 788441. What kind of validation do you want actually?

Comment 2

[Antonio Manuel Amaya Calvo (:amac)]

I believe that while you can get an SMS that has an alphanumeric string as origin you cannot send an SMS to an alphanumeric string, not from user equipment anyway. AFAIK SMS destination must be MSISDNs. Copying Beatriz in case she has more information on this.

Comment 3

[David Palomino [:dpalomino]]

Hi Antonio, 

That is correct, SMS's sent from devices have to be sent to MSISDN recipients only... 

Thanks, 
David

Comment 4

[Julien Wajsberg [:julienw]]

As I explained in Bug 836707 comment 2, the user _can_ send SMS alphanumeric string as recipient. It is transformed according certain rules inside Gecko.

Maybe we can check just before sending it to RIL if it is a number.

reseting the tef flag that was sent because of the clone.

Comment 5

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Julien Wajsberg [:julienw] from comment #4)
> As I explained in Bug 836707 comment 2, the user _can_ send SMS alphanumeric
> string as recipient. It is transformed according certain rules inside Gecko.

I had found the transforming call (I assumed you're referring to the PhoneNumberUtils.parse call in [1]), but since when it fails it just passes the input string as is I had dismissed it completely.

> 
> Maybe we can check just before sending it to RIL if it is a number.

Yes, I think that adding a verification that what we have in receiver looks like a valid MSISDN just before before [2] should suffice. The verification that's been added in bug 836215 can be reused here in fact.

> 
> reseting the tef flag that was sent because of the clone.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/sms/src/ril/SmsDatabaseService.js#782
[2] https://mxr.mozilla.org/mozilla-central/source/dom/sms/src/ril/SmsDatabaseService.js#796

Comment 6

[Alex Keybl [:akeybl]]

Not clear whether or not this would block v1.0.0 - asked a question in https://bugzilla.mozilla.org/show_bug.cgi?id=836707#c6

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

No blocking, but we'll look at an uplift nomination when one is ready.

Comment 8

[Julien Wajsberg [:julienw]]

Antonio, could you prepare something here ? I don't feel I know this enough to do a security-sensitive check here, and Taipei is in holiday...

Comment 9

[Antonio Manuel Amaya Calvo (:amac)]

Sorry, somehow this managed to fall between my mails. I could, but the problem is that the easiest (and more correct) way to do this would be just to reuse the number checking of Bug 836215 just before [1]... And I've just noticed that the SmsDatabaseService.js file doesn't exist anymore on Mozilla Central. It's on dom/mobilemessage/src/ril/MobileMessageDatabaseService.js instead now. That change was on Bug 831683.

In any case, once Bug 836215 lands this one should be quite easy to fix (leaving aside having to do the fix twice one for b2g18 and another for m-c :) ), but fixing it before that is doing the work twice.



[1] https://mxr.mozilla.org/mozilla-b2g18/source/dom/sms/src/ril/SmsDatabaseService.js#795

Comment 10

[Antonio Manuel Amaya Calvo (:amac)]

Removed the dependency by error

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected

Comment 12

[Antonio Manuel Amaya Calvo (:amac)]

What's the status of this? There's no protection on Gaia side against invalid numbers for SMS (there is for dialed numbers now) and no protection on Gecko side either. As I said on C9, it should be pretty easy to fix now that Bug 836215 has landed (maybe adding an extra check for maximum length to the phone number).

Comment 13

[Marcelino Veiga Tuimil [:sonmarce]]

As Lukas said, according to current status flags, when there is a patch in master, it will be also merged en v1.0.1.

Comment 14

[David Flanagan [:djf]]

Re-nominating this bug as tef+ because:

1) it was filed by one of our partners at TEF

2) It is a gaping hole in our SMS API.

This is a security issue and it must be fixed.

Comment 15

[Julien Wajsberg [:julienw]]

Attachment: patch v1

* Normalize the number before storing it in database
* if it's not sendable, fail immediately

This applies on top of the patch for Bug 846499 obviously.

Vicamo, are you the good person to review this ? Or is :philikon a better match ?

Comment 16

[David Flanagan [:djf]]

Comment on attachment 722856 [details] [diff] [review]
patch v1

Review of attachment 722856 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2549,5 @@
> +      if (PhoneNumberUtils.isSendablePhoneNumber(options.number)) {
> +        this.worker.postMessage(options);
> +      } else {
> +        debug('Number ' + options.number + ' is not sendable.');
> +        this.handleSmsSendFailed(options);

I'm surprised to see the call it isSendable() so far away from the call to normalize(). If the number is invalid, do we really want to save the message to the database? Is this the only way to send a reasonable error message, after all the DB maintenance has been done?

I suppose this is probably the right way to do it, but it surprised me.  Maybe add a comment explaining the situation?

I wish we could reject invalid numbers synchronously from the SMSManager.send() call.  Maybe we should file a bug to do that and reference it here?

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 722856 [details] [diff] [review]
patch v1

Review of attachment 722856 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2549,5 @@
> +      if (PhoneNumberUtils.isSendablePhoneNumber(options.number)) {
> +        this.worker.postMessage(options);
> +      } else {
> +        debug('Number ' + options.number + ' is not sendable.');
> +        this.handleSmsSendFailed(options);

In the case of SMS we really need to store it, otherwise the user won't have a mean to retrieve his message later.

The call to normalize is far from here because, indeed, the options object should really be made inside the callback, we don't need it before. But I wanted this patch to be as surgical as possible because it needs to land on v1.0.1 too, that's why I didn't change this.

Do you think it is ok for you ?

I'm not sure this is good to file a bug now for the synchronous check, we should really wait for the standardization process... Gregor, Vicamo, what's your thought on that ?

Comment 18

[Julien Wajsberg [:julienw]]

Comment on attachment 722856 [details] [diff] [review]
patch v1

remocing the r? for now, as I'll have to upload a new patch tomorrow.

Comment 19

[Julien Wajsberg [:julienw]]

Attachment: patch v2

still needs patch for Bug 846499.

Comment 20

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Julien Wajsberg [:julienw] from comment #17)
> The call to normalize is far from here because, indeed, the options object
> should really be made inside the callback, we don't need it before. But I
> wanted this patch to be as surgical as possible because it needs to land on
> v1.0.1 too, that's why I didn't change this.

According to current blocking-b2g flag, it's not.

> Do you think it is ok for you ?

I suggest we throw an exception for unacceptable inputs instead.  However, we can't have that with current SMS IPC, so sending a sendfailed event back becomes the only option for us for now.  I need feedbacks from DOM peers.

Mounir, could you give some opinions?

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 723416 [details] [diff] [review]
patch v2

Review of attachment 723416 [details] [diff] [review]:
-----------------------------------------------------------------

Cancel review for now.

Comment 22

[Mounir Lamouri (:mounir)]

'send-failed' is good with me.

Comment 23

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 723416 [details] [diff] [review]
patch v2

Review of attachment 723416 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with |options.number| fixed :)

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2672,3 @@
>      let options = this._fragmentText(message, null, strict7BitEncoding);
>      options.rilMessageType = "sendSMS";
>      options.number = number;

Use |options.number = PhoneNumberUtils.normalize(number);|.  Both message body and receiver address stored in database are exactly identical to those passed in by content.

Comment 24

[Julien Wajsberg [:julienw]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #23)
> Comment on attachment 723416 [details] [diff] [review]
> patch v2
> 
> Review of attachment 723416 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with |options.number| fixed :)
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +2672,3 @@
> >      let options = this._fragmentText(message, null, strict7BitEncoding);
> >      options.rilMessageType = "sendSMS";
> >      options.number = number;
> 
> Use |options.number = PhoneNumberUtils.normalize(number);|.  Both message
> body and receiver address stored in database are exactly identical to those
> passed in by content.

Actually, I thought it was nice to show the user the actual number that was sent. Do you think it's better to keep the content he gave ?

(if you answer yes to this, I won't argue more and just carry on your suggestion, but I'd like to be sure before :) ).

Comment 25

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Julien Wajsberg [:julienw] from comment #24)
> Actually, I thought it was nice to show the user the actual number that was
> sent. Do you think it's better to keep the content he gave ?

Yes. Please just keep it. Don't want to have any normalization on stored numbers any more. Or user will just see an empty string when dialing a number consisted of all illegal characters.

Comment 26

[Julien Wajsberg [:julienw]]

Attachment: patch v3

carrying r=vicamo

Comment 27

[Julien Wajsberg [:julienw]]

was tef- by :evilmachines => setting correctly flags.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd51fe9f7236

Comment 29

[Julien Wajsberg [:julienw]]

Michael, I'd like to explain your rationale for not blocking here. We believe this is a security issue, without this any content could be sent as a number to the modem.

Comment 30

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

Sure, sorry for not providing that information initially.   The patch in question affects the reference RIL and the commercial RIL used in the v1.0.1 products does not have this issue so there's no need to block on this bug.

Comment 31

[Antonio Manuel Amaya Calvo (:amac)]

Sorry for piping in so late, but why is the normalizing/checking done so deep? The check should have been done *before* calling the RIL, not inside the RIL. Specially if the RIL is replaceable because that leaves the check outside of B2G control for all practical senses. 

If the check were done at a higher level (SMS DOM comes to mind) then both the commercial and reference RIL would be protected. As would any other implementation of the RIL, we would assure we were not passing trash to it.

Comment 32

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

(settings this back to tef- as per comment 30 it's not blocking, mainly just to get this bug off the triage list.  However given that this bug is technically NPOTB I don't have a problem with this landing in v1.0.1)

Comment 33

[Julien Wajsberg [:julienw]]

Lukas, can we land this on v1.0.1 then (cf comment 32) ?

Comment 34

[Julien Wajsberg [:julienw]]

Antonio > I generally agree, that's what I said in comment 4. Problem is that this part is done in C++ and nobody with these skills stepped in to do that.

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fd51fe9f7236

Comment 36

[Antonio Manuel Amaya Calvo (:amac)]

(In reply to Julien Wajsberg [:julienw] from comment #34)
> Antonio > I generally agree, that's what I said in comment 4. Problem is
> that this part is done in C++ and nobody with these skills stepped in to do
> that.

My bad then... I could have done that. Still can if this is reopened at some later point (which I think it should).

Comment 37

[Julien Wajsberg [:julienw]]

Antonio, please file a new bug about this, this is also what David Flanagan said in Bug 846499 comment 44.

Comment 38

[[:fabrice] Fabrice Desré]

So in these cases, *don't implement the bad solution*. Find someone to do it properly instead.

Comment 39

[Julien Wajsberg [:julienw]]

I didn't think this was a _bad_ solution, I thought it wasn't the _best_ solution but that it was good enough for v1.

Comment 40

[David Flanagan [:djf]]

In this bug and in bug 846499, we were following Vicamo's lead in bug 836215, where he added this validation step in the RIL. The intent was to fix the broken validation.

Comment 30 makes me want to hit my head against a wall, however. We really need to fix this at the WebAPI level, I guess.

Comment 41

[David Flanagan [:djf]]

(In reply to Julien Wajsberg [:julienw] from comment #37)
> Antonio, please file a new bug about this, this is also what David Flanagan
> said in Bug 846499 comment 44.

I've filed bug 851194 to fix this correctly at the API level, and I've nominated it as a tef blocker.

Comment 42

[David Flanagan [:djf]]

I've also filed 851201 for the corresponding telephony bug.

Comment 43

[Julien Wajsberg [:julienw]]

Comment on attachment 724350 [details] [diff] [review]
patch v3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: it's possible to send unsafe numbers to SMS backend. And we can't use "numbers with letters".
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low, the patch is very short
String or UUID changes made by this patch: none

Seeing it was not uplifted during the uplift period, I ask this approval now.

This only affects Mozilla RIL. It may not be the best way to fix this, but it's there. We have follow-up bugs to make it in a better way, but I think we need this in the b2g18 branch now.

Comment 44

[Julien Wajsberg [:julienw]]

Attachment: patch v3 for b2g18 branch

carrying r=vicamo

Comment 45

[Alex Keybl [:akeybl]]

Missed the tracking cutoff by a couple of days, poses almost no risk to partners (Moz RIL), and has had bake time on m-c. Approving for v1.1

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/75a9b9b712cf