845539 - Validate numbers passed to nsIDOMTelephony.dial() is breaking bug 813679

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Anshul]

Vicamo the fix for bug 836215 is breaking the support for bug 813679 (*2 and *8 are phone numbers(in Venezuela)). When I dial *2 and *8 the IsViablePhoneNumber returns false and an error is shown on the UI.

There is another issue which is the message displayed on the dialer itself when a number fails validation. The message showed is "To make a call, the phone must be connected to a network.", which is incorrect in this case. This is because the CallError reported to content process is "BadNumberError" which translates to this message in telephony_helper.js.

Comment 1

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

(In reply to Anshul from comment #0)
> *2 and *8 are phone numbers in Venezuela.

Oops!  Any other strange but valid numbers?

> There is another issue which is the message displayed on the
> dialer itself when a number fails validation.

On dialing a bad number we throw a "BadNumberError".  I guess this is expected.  Maybe some fix in telephony_helper.js is needed.

Comment 2

[Anshul]

Vicamo, yes the telephony_helper needs to be enhanced to not show the emergencydialog body for badnumberrror if the phone is not in emergency only mode.

Should I file a separate bug for telephony_helper handling badnumber error properly and to also handle other call errors?

So would you be fixing the PhoneNumberUtils.jsm to address the *2 and *8 dialing number issue? I am not aware of any other specific numbers like these.

Comment 3

[David Flanagan [:djf]]

This is an unowned tef+ bug, so I'll take it and see whether I can make any progress.

Comment 4

[David Flanagan [:djf]]

Attachment: part 1: github PR to fix the error message for invalid numbers

This patch fixes the error dialog messages so that invalid phone numbers don't complain about the network connection or say anything about an emergency.

Germain, I'm asking for your review because you wrote the related patch for 835750.  Would you take a look at the case for DeviceNotAcceptedError?  Does the message seem right there? Is that error specific to emergency phone numbers?

Comment 5

[David Flanagan [:djf]]

The main part of this bug is that we can no longer dial the (valid in Venezuela) numbers *2 and *8 because of the recent addition of isViablePhoneNumber() to PhoneNumberUtils.jsm.

That was done in bug 836215 and is intended as a security measure to prevent injection attacks through tel:// URLs and contacts database entries.  There was no intent to save the user from airtime charges or insulate the operator's network from bogus calls to invalid numbers.

isViablePhoneNumber() is attempting to tell us whether the specified number could ever be a valid number. In general, that seems hard to do and impossible to future proof. It is pretty brittle against obscure things like *2 and *8 as we see in this bug. And, I don't see any obvious way to allow partners to customize the regular expressions used in that function.

All we really need is a function that rejects dangerous numbers that might be attacks. I suggest, therefore, that we dramatically simplify the isViable test so that it accepts any combination of -0-9*+().  It looks like the current version allows alphabetic numbers (ABC=2, DEF=3, etc). But I don't think Gaia allows those (even dialing from contacts) so maybe we can strip them from here as well.

I'm not sure I'll have time to make these changes myself, though.

Comment 6

[David Flanagan [:djf]]

Attachment: Make isViablePhoneNumber() more accepting

Vicamo,

Instead of changing isViablePhone() number completely, I just changed the first part of the regexp from [0-9]{2,} to [*#0-9]{2,} so that any string of 2 or more keys from a phone keypad are treated as valid numbers.

I'm not sure that we need the complex second part of the regexp, but I haven't modified it here.

The attached patch modified PhoneNumber.jsm directly for ease of review, but I'll also submit a corresponding PR to the appropriate upstream repo.

Vicamo: if you don't like this proposed patch, please go ahead and take the bug for yourself. I don't want to slow down the process of getting this fixed!

I turns out, by the way, that *8 is also a valid number on the AT&T network in the US.

Comment 7

[David Flanagan [:djf]]

Attachment: link to github PR to the upstream PhoneNumber.js repo

Gregor,

The patch I'm asking you to review is the same one I'm asking Vicamo to review, but your version is against the upstream PhoneNumber.js repo.  Obviously we should only land it in the repo if Vicamo agrees that it is actually the fix we need downstream.

I'm not really sure how to handle patches like this, but I hope this kind of dual review is reasonable enough.

Comment 8

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

Comment on attachment 719146 [details] [diff] [review]
Make isViablePhoneNumber() more accepting

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

::: dom/phonenumberutils/PhoneNumber.jsm
@@ +46,4 @@
>     * data. The symbol 'x' is allowed here as valid punctuation since it is often
>     * used as a placeholder for carrier codes, for example in Brazilian phone
>     * numbers. We also allow multiple '+' characters at the start.
> +   * 

trailing ws.

@@ +55,5 @@
>     * The first reg-ex is to allow short numbers (two digits long) to be parsed
> +   * if they are entered as "15" etc, but only if they include only symbols
> +   * that appear on a keypad. This also supports the Venezuelan numbers *8
> +   * and *2.
> +   * 

trailing ws.

@@ +66,4 @@
>     * Note VALID_PUNCTUATION starts with a -, so must be the first in the range.
>     */
>    const MIN_LENGTH_PHONE_NUMBER
> +    = "[*#" + VALID_DIGITS + "]{" + MIN_LENGTH_FOR_NSN + "}";

Then "***", "###", "*#*", or similar short numbers is also viable. Maybe a separate, more strict rule like:

+ const VENEZUELA_SHORT_NUMBER = "\\*[" + VALID_DIGITS + "];
  const VALID_PHONE_NUMBER_PATTERN =
    new RegExp("^" + MIN_LENGTH_PHONE_NUMBER + "$|"
+              + "^" + VENEZUELA_SHORT_NUMBER + "$|"
               + "^" + ...);

What do you think?

Comment 9

[David Flanagan [:djf]]

Vicamo,

Thanks for the quick review. I agree that it seems silly to accept *** and ### as viable numbers. I thought about requiring at least one digit, but didn't know how to write the regexp for that.

I don't think it is a good idea to make this patch specific to the Venezuela case of *2 and *8 because I'm guessing that there will be other numbers that we don't anticipate. We don't want to have to ship an OS update to enable users to dial the numbers they want to dial!

Comment 10

[David Flanagan [:djf]]

Comment on attachment 719123 [details]
part 1: github PR to fix the error message for invalid numbers

I've just updated this pull request. Germain would you take another look at it. 

And I'm also requesting a review from Anshul since his comments on github have been so helpful.

Comment 11

[David Flanagan [:djf]]

Attachment: PhoneNumber.jsm patch v2, with trailing spaces removed

I've attached a second version of the gecko patch. Its the same as the first, but removes the trailing whitespace. I had already removed that when I created the PR to the upstream repo, so I'm not updating that attachment. 

Vicamo already gave r+ to the previous version, so I'm marking this one r+ too.

Comment 12

[David Flanagan [:djf]]

Setting checkin-needed to get attachment 719365 [details] [diff] [review] landed. I don't have (don't want!) gecko commit rights, and I don't even know how to push this patch to a try server. I've updated the test associated with the patched file, so it probably should go to try first.

Comment 13

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

Reset checkin-needed because r= flag missed in commit summary. try https://tbpl.mozilla.org/?tree=Try&rev=8ab8f67ad590

Comment 14

[David Flanagan [:djf]]

Attachment: patch v3, with corrected commit summary

I've updated the patch to add r=vicamo a=tef+ to the commit summary. Sorry I forgot that! I've marked the patch r+ because of vicamo's previous review.

Comment 15

[David Flanagan [:djf]]

Okay, the gecko patch has a corrected commit summary and was green on try.

It has r+ to land in gecko but hasn't yet been accepted into the upstream github repo. Nevertheless, it is a tef+ bug, so I'm setting checkin-needed.

Setting needinfo on Gregor: maybe he can accept the github PR and check this patch in at the same time...

Comment 16

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

Attachment: patch v4

Sorry, I still want a more strict version. Disallow "**8", "#*", either.

Comment 17

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

Attachment: github PR

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/288fb7894990

Comment 19

[David Flanagan [:djf]]

Vicamo,

I'm afraid we have a fundamental disagreement over this patch.  I don't think we have any business trying to decide in gecko what a viable phone number is for an unknown carrier's network.  Adding a venezeula-specific patch will fix this particular bug, but I fear that similar bugs will still exist and will affect us.

In my opinion we should let the carrier decide if a number is valid. All we need to do in gecko is prevent malicious injection attacks and filter out obviously bogus numbers.  So I suggest that any string of digits plus * and # that the user could type on a telephone keypad ought to be accepted by gecko.

So how do we proceed from here? You've got more experience with this code than I do.  You've rejected my patch. I'm giving feedback- on your patch, but don't feel like I'm actually qualified to review it.  But I don't think that you can r+ your own patch, so we need to find another reviewer to sort this out.  Gregor?

Comment 20

[David Flanagan [:djf]]

Well, looks like my comment was too late.

Ryan, you just landed an unreviewed patch.

Comment 21

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

(In reply to David Flanagan [:djf] from comment #19)
> Vicamo,
> 
> I'm afraid we have a fundamental disagreement over this
> patch.  I don't think we have any business trying to decide in
> gecko what a viable phone number is for an unknown carrier's
> network.

This API call is for a could-be-a-phone-number check.  You'll probably never have a completed algorithm for phone number checks.  So support for some "unknown carriers" is quite impractical.

> Adding a venezeula-specific patch will fix this particular
> bug, but I fear that similar bugs will still exist and will
> affect us.

The API is for checking a validity of phone number.  Anything doesn't fall in the known scope is invalid.  Or, we just don't need this check at all.

> In my opinion we should let the carrier decide if a number is
> valid. All we need to do in gecko is prevent malicious
> injection attacks and filter out obviously bogus numbers.

They can always do.  Just fork gecko.

> So I suggest that any string of digits plus * and
> # that the user could type on a telephone keypad ought to be
> accepted by gecko.

Actually, you CANNOT type a phone number with characters provided on the dialer keypad.  So, are you suggesting we don't need such check in Gecko at all?

Comment 22

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> Actually, you CANNOT type a phone number with characters
> provided on the dialer keypad.

"Actually, you CANNOT type a phone number with characters other than those provided on the dialer keypad."

Comment 23

[David Flanagan [:djf]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #21)
> (In reply to David Flanagan [:djf] from comment #19)

> > Adding a venezeula-specific patch will fix this particular
> > bug, but I fear that similar bugs will still exist and will
> > affect us.
> 
> The API is for checking a validity of phone number.  Anything doesn't fall
> in the known scope is invalid.  Or, we just don't need this check at all.
> 

My worry is that we don't know the scope. Or that the scope may change to include numbers that we don't support.

> > In my opinion we should let the carrier decide if a number is
> > valid. All we need to do in gecko is prevent malicious
> > injection attacks and filter out obviously bogus numbers.
> 
> They can always do.  Just fork gecko.
> 

And get all customers to take over-the-air updates to allow their phone to dial the new numbers?

> > So I suggest that any string of digits plus * and
> > # that the user could type on a telephone keypad ought to be
> > accepted by gecko.
> 
> Actually, you CANNOT type a phone number with characters provided on the
> dialer keypad.  So, are you suggesting we don't need such check in Gecko at
> all?

My understanding from the original but that added this isViablePhoneNumber() check is that it was a security feature and that things like extension numbers and alphanumeric numbers were things that could come from tel:// URLs and from the contacts app. My understanding (which could be wrong) is that we need a check to prevent attacks.

But I would assume that any sequence of 0-9*# is safe from a security standpoint and it seems simplest to me to allow all sequences of that form so we don't accidentally prevent the use of carrier-specific features that we don't know about or that are added in the future.

For comparison, I've just tried my android phone, and it lets me dial numbers like 1, *** and #*#.

Anyway, your patch has landed, and it fixes the immediate tef+ bug, so let's go with that for now.  I'll do a retroactive review of it, and will open a new bug about the issue of how liberal the isViable() test should be.

Comment 24

[David Flanagan [:djf]]

Comment on attachment 719508 [details] [diff] [review]
patch v4

Cleared Vicamo's r+ so I could add my own retroactive r+.  RyanVM you can ignore my previous comment about the patch being unreviewed.

I've tested the patch and it fixes the first part of the bug.  We still need the gaia-side portion, of course.

I'll file a new bug to express my concerns that we are filtering too aggressively here.

Comment 25

[David Flanagan [:djf]]

Filed bug 846499

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/288fb7894990

Comment 27

[David Flanagan [:djf]]

Reopening and belatedly adding [leaveopen] to the whiteboard.

This bug has a gaia patch that needs to land before we can close it.  Its fine for the gecko and gaia patches to land separately, however.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/be1769152513
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/fec56cb9e94b

Leaving the status flags set to affected until the Gaia patch lands.

Comment 29

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

Reviewed and tested ;-) Works perfectly. Sorry for the delay caused by the opacity issue (https://bugzilla.mozilla.org/show_bug.cgi?id=816941) Once #8408 is merged, @davidflanagan 's update works as expected. Thanks! Definitely r+ for the patch I was reviewing ;-) Thanks!

Comment 30

[David Flanagan [:djf]]

Landed the gaia patch to master: https://github.com/mozilla-b2g/gaia/commit/50e404a41581ac2958b2a377a6e12ec5441b7580

This gaia patch needs to be uplifted and the status flags changed from affected to fixed. 

Closing.

Comment 31

[Anshul]

@David, has the change "github PR to fix the error message for invalid numbers" uplifted to v1.0.1?

Comment 32

[David Flanagan [:djf]]

I've pinged jhford on irc about this bug. Hopefully that will get it uplifted.

Comment 33

[David Flanagan [:djf]]

jhford confirms that this bug is on his list of things to be uplifted.

Comment 34

[Anshul]

David, I still don't see https://github.com/mozilla-b2g/gaia/pull/8367 on v1.0 branch. Could you please follow up?

Comment 35

[David Flanagan [:djf]]

John,

The gaia part of this patch (the first attachment) still needs to be uplifted. Can you do that, or do I need to take action?

I'm clearing a [leave open] note in the whiteboard that was there from when 
the gecko patch landed in case that was blocking the uplift.

Thanks for following up on this Anshul.

Comment 36

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Bug 832182 needs to be uplifted to v1-train after this bug is uplifted. It's using new piece of codes introduced here.

Comment 37

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Sorry, I didn't mean to cancel the needinfo. Set for comment 35.

Comment 38

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

(In reply to David Flanagan [:djf] from comment #35)
> John,
> 
> The gaia part of this patch (the first attachment) still needs to be
> uplifted. Can you do that, or do I need to take action?
> 
> I'm clearing a [leave open] note in the whiteboard that was there from when 
> the gecko patch landed in case that was blocking the uplift.
> 
> Thanks for following up on this Anshul.

Hi David,

I'm not sure why this was not showing up in the actual queries being run.  Everything is set for it to show up, but it's just not.

I've uplifted the bug to v1-train as dca7d61e3ae669ea7642a2784946a3ea2511db7f, but the v1.0.1 portion is giving me merge conflicts.  I think I know how to resolve the merge conflict, but I think it'd be better to have someone more familiar with the code do it.

The merge conflict could be resolved with:

  cd gaia
  git checkout v1.0.1
  git cherry-pick -x -m1 50e404a41581ac2958b2a377a6e12ec5441b7580
  <resolve merge conflict>
  git commit

Comment 39

[David Flanagan [:djf]]

Hmm. It doesn't apply cleanly to v1.0.1 because bug 836257 hasn't landed there.

Comment 40

[David Flanagan [:djf]]

Merged into v1.0.1: https://github.com/mozilla-b2g/gaia/commit/0b6f02d4a4ac2dfdeaed71f14256ae69df9d5f24

Comment 41

[Darren]

Cannot verify, need steps to blackbox test this issue.

Comment 42

[Jason Smith [:jsmith]]

Reporter - Can you verify this patch fixes this issue?

Comment 43

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

Hi guys! I just confirmed that it does not work for every case. I am discussing with Mari Ángeles the way to proceed ;-) I'll keep you posted.

Comment 44

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

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