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

RESOLVED FIXED in Firefox 22

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: amac, Assigned: julienw)

Tracking

unspecified
mozilla22
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

This is the Gecko counter part of Bug 836707
(Reporter)

Updated

6 years ago
Summary: [B2G RIL] Validate the numbers that are passed to nsIDOMTelephony.dial() → [B2G RIL] Validate the numbers that are passed from SMSManager.send
Assignee: nobody → vyang
SMS supports alpha-numeric addresses, see bug 788441. What kind of validation do you want actually?
(Reporter)

Comment 2

6 years ago
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.
Flags: needinfo?(brg)
Hi Antonio, 

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

Thanks, 
David
Flags: needinfo?(brg)
(Assignee)

Comment 4

6 years ago
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.
blocking-b2g: tef+ → tef?
(Reporter)

Comment 5

6 years ago
(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

5 years ago
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
No blocking, but we'll look at an uplift nomination when one is ready.
blocking-b2g: tef? → -
tracking-b2g18: --- → +
(Assignee)

Comment 8

5 years ago
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...
(Reporter)

Comment 9

5 years ago
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
No longer depends on: 836215
See Also: → bug 831683
(Reporter)

Comment 10

5 years ago
Removed the dependency by error
Depends on: 836215
Batch edit: Bugs marked status-b2g18: affected after 2/13 branching of v1.0.1 are now also status-b2g18-v1.0.1: affected
status-b2g18-v1.0.1: --- → affected
(Reporter)

Comment 12

5 years ago
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).
As Lukas said, according to current status flags, when there is a patch in master, it will be also merged en v1.0.1.
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.
blocking-b2g: - → tef?
(Assignee)

Updated

5 years ago
Assignee: vyang → felash
(Assignee)

Comment 15

5 years ago
Created attachment 722856 [details] [diff] [review]
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 ?
Attachment #722856 - Flags: review?(vyang)
Attachment #722856 - Flags: feedback?
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?
Attachment #722856 - Flags: feedback? → feedback+
(Assignee)

Comment 17

5 years ago
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 ?
(Assignee)

Updated

5 years ago
Flags: needinfo?(anygregor)
blocking-b2g: tef? → -
(Assignee)

Comment 18

5 years ago
Comment on attachment 722856 [details] [diff] [review]
patch v1

remocing the r? for now, as I'll have to upload a new patch tomorrow.
Attachment #722856 - Flags: review?(vyang)
(Assignee)

Comment 19

5 years ago
Created attachment 723416 [details] [diff] [review]
patch v2

still needs patch for Bug 846499.
Attachment #722856 - Attachment is obsolete: true
Attachment #723416 - Flags: review?(vyang)
(Assignee)

Updated

5 years ago
Depends on: 846499
(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?
Flags: needinfo?(mounir)
Comment on attachment 723416 [details] [diff] [review]
patch v2

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

Cancel review for now.
Attachment #723416 - Flags: review?(vyang)
'send-failed' is good with me.
Flags: needinfo?(mounir)
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.
Attachment #723416 - Flags: review+
(Assignee)

Comment 24

5 years ago
(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 :) ).
Flags: needinfo?(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.
Flags: needinfo?(vyang)
(Assignee)

Comment 26

5 years ago
Created attachment 724350 [details] [diff] [review]
patch v3

carrying r=vicamo
Attachment #723416 - Attachment is obsolete: true
Attachment #724350 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Comment 27

5 years ago
was tef- by :evilmachines => setting correctly flags.
status-b2g18-v1.0.0: affected → wontfix
status-b2g18-v1.0.1: affected → wontfix
(Assignee)

Updated

5 years ago
Flags: needinfo?(anygregor)
(Assignee)

Comment 29

5 years ago
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.
blocking-b2g: - → tef?
Flags: needinfo?(mvines)
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.
Flags: needinfo?(mvines)
(Reporter)

Comment 31

5 years ago
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.
(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)
blocking-b2g: tef? → -
(Assignee)

Comment 33

5 years ago
Lukas, can we land this on v1.0.1 then (cf comment 32) ?
Flags: needinfo?(lsblakk)
(Assignee)

Comment 34

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/fd51fe9f7236
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Reporter)

Comment 36

5 years ago
(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).
(Assignee)

Comment 37

5 years ago
Antonio, please file a new bug about this, this is also what David Flanagan said in Bug 846499 comment 44.
So in these cases, *don't implement the bad solution*. Find someone to do it properly instead.
(Assignee)

Comment 39

5 years ago
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.
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.
(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.
I've also filed 851201 for the corresponding telephony bug.
(Assignee)

Comment 43

5 years ago
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.
Attachment #724350 - Flags: approval-mozilla-b2g18?
(Assignee)

Comment 44

5 years ago
Created attachment 730202 [details] [diff] [review]
patch v3 for b2g18 branch

carrying r=vicamo
Attachment #730202 - Flags: review+
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
Flags: needinfo?(lsblakk)

Updated

5 years ago
Attachment #724350 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
https://hg.mozilla.org/releases/mozilla-b2g18/rev/75a9b9b712cf
status-b2g18: affected → fixed
status-firefox20: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → fixed
You need to log in before you can comment on or make changes to this bug.