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

RESOLVED FIXED in Firefox 22

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: anshulj, Assigned: djf)

Tracking

({verifyme})

unspecified
B2G C4 (2jan on)
ARM
All
verifyme
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -

Firefox Tracking Flags

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

Details

(Whiteboard: QARegressExclude)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(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.
blocking-b2g: tef? → tef+
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → affected
(Reporter)

Comment 2

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

Comment 3

6 years ago
This is an unowned tef+ bug, so I'll take it and see whether I can make any progress.
Assignee: nobody → dflanagan
(Assignee)

Comment 4

6 years ago
Created attachment 719123 [details]
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?
Attachment #719123 - Flags: review?(gtorodelvalle)
(Assignee)

Comment 5

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

Comment 6

6 years ago
Created attachment 719146 [details] [diff] [review]
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.
Attachment #719146 - Flags: review?(vyang)
(Assignee)

Comment 7

6 years ago
Created attachment 719148 [details]
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.
Attachment #719148 - Flags: review?(anygregor)
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?
Attachment #719146 - Flags: review?(vyang) → review+
(Assignee)

Comment 9

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

Comment 10

6 years ago
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.
Attachment #719123 - Flags: review?(anshulj)
(Assignee)

Comment 11

6 years ago
Created attachment 719365 [details] [diff] [review]
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.
Attachment #719146 - Attachment is obsolete: true
Attachment #719365 - Flags: review+
(Assignee)

Comment 12

6 years ago
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.
Keywords: checkin-needed
Reset checkin-needed because r= flag missed in commit summary. try https://tbpl.mozilla.org/?tree=Try&rev=8ab8f67ad590
Keywords: checkin-needed
(Assignee)

Comment 14

6 years ago
Created attachment 719488 [details] [diff] [review]
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.
Attachment #719365 - Attachment is obsolete: true
Attachment #719488 - Flags: review+
(Assignee)

Comment 15

6 years ago
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...
Flags: needinfo?(anygregor)
Keywords: checkin-needed
Created attachment 719508 [details] [diff] [review]
patch v4

Sorry, I still want a more strict version. Disallow "**8", "#*", either.
Attachment #719488 - Attachment is obsolete: true
Attachment #719508 - Flags: review+
Created attachment 719520 [details]
github PR
Attachment #719148 - Attachment is obsolete: true
Attachment #719148 - Flags: review?(anygregor)
Attachment #719520 - Flags: review?(anygregor)
(Assignee)

Comment 19

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

Comment 20

6 years ago
Well, looks like my comment was too late.

Ryan, you just landed an unreviewed patch.
(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?
(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."
(Assignee)

Comment 23

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

Updated

6 years ago
Attachment #719508 - Flags: review+ → feedback+
(Assignee)

Comment 24

6 years ago
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.
Attachment #719508 - Flags: review+
(Assignee)

Comment 25

6 years ago
Filed bug 846499
Attachment #719520 - Flags: review?(anygregor) → review+
https://hg.mozilla.org/mozilla-central/rev/288fb7894990
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

6 years ago
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.
Status: RESOLVED → REOPENED
Flags: needinfo?(anygregor)
Resolution: FIXED → ---
Whiteboard: [leaveopen]
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.
status-firefox20: --- → wontfix
status-firefox21: --- → wontfix
status-firefox22: --- → fixed
Whiteboard: [leaveopen] → [leave open]
Target Milestone: --- → B2G C4 (2jan on)
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!
Attachment #719123 - Flags: review?(gtorodelvalle) → review+
(Assignee)

Comment 30

6 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Attachment #719123 - Flags: review?(anshulj)
(Reporter)

Comment 31

6 years ago
@David, has the change "github PR to fix the error message for invalid numbers" uplifted to v1.0.1?
(Assignee)

Comment 32

6 years ago
I've pinged jhford on irc about this bug. Hopefully that will get it uplifted.
(Assignee)

Comment 33

6 years ago
jhford confirms that this bug is on his list of things to be uplifted.
Depends on: 836215
(Reporter)

Comment 34

6 years ago
David, I still don't see https://github.com/mozilla-b2g/gaia/pull/8367 on v1.0 branch. Could you please follow up?
Flags: needinfo?(dflanagan)
(Assignee)

Comment 35

6 years ago
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.
Flags: needinfo?(dflanagan) → needinfo?(jhford)
Whiteboard: [leave open]
Bug 832182 needs to be uplifted to v1-train after this bug is uplifted. It's using new piece of codes introduced here.
Flags: needinfo?(jhford)
Sorry, I didn't mean to cancel the needinfo. Set for comment 35.
Flags: needinfo?(jhford)
(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
status-b2g18: affected → fixed
Flags: needinfo?(jhford)
(Assignee)

Comment 39

6 years ago
Hmm. It doesn't apply cleanly to v1.0.1 because bug 836257 hasn't landed there.
(Assignee)

Comment 40

6 years ago
Merged into v1.0.1: https://github.com/mozilla-b2g/gaia/commit/0b6f02d4a4ac2dfdeaed71f14256ae69df9d5f24
status-b2g18-v1.0.1: affected → fixed

Updated

6 years ago
Whiteboard: QARegressExclude

Updated

6 years ago
Flags: in-moztrap-

Comment 41

6 years ago
Cannot verify, need steps to blackbox test this issue.
Reporter - Can you verify this patch fixes this issue?
Flags: needinfo?(anshulj)
Keywords: verifyme
(Reporter)

Updated

6 years ago
Flags: needinfo?(anshulj)
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.
Please, refer to bug 859719 regarding the testing/validation of this bug. Thanks!
You need to log in before you can comment on or make changes to this bug.