Closed Bug 794034 Opened 11 years ago Closed 11 years ago

Make sure tel: URLs that contain * or # are not dereferenced

Categories

(Core :: DOM: Navigation, defect)

All
Android
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla18
Tracking Status
firefox16 + verified
firefox17 + verified
firefox18 --- verified

People

(Reporter: hsivonen, Assigned: johns)

References

(Blocks 1 open bug, )

Details

(Keywords: sec-high, sec-vector, verifyme)

Attachments

(1 file)

https://twitter.com/pof/status/250540790491787264 claims Galaxy S3 to be vulnerable to drive-by factory reset with <frame src="tel:*2767*3855%23" />.

We should make sure that on all platforms
 * tel: URLs are never referenced as addressed of embedded content (<img src>, <embed src>, etc.)
 * tel: URLs that contain * or # in links are never passed to a helper app that could be vulnerable to autodialing a pseudo-phonenumber that is a control code for the phone, for the SIM or for the network.

Filing as UNCONFIRMED, because I have not confirmed a vulnerability in Firefox.
The non-iframe cases should already be covered, I think: we don't allow protocols that don't return data in those cases.

For iframe we need to do something.  Something stronger than the suggestions in bug 566893 because we want to disallow explicit navigations too, right?
This exploit is very public already, so hiding it isn't very useful.
Group: core-security
Keywords: sec-high, sec-vector
Status: UNCONFIRMED → NEW
Ever confirmed: true
Tracking, but we may decide to wontfix for FF16 if we don't find a fix we're comfortable with in time for Monday's beta build.
Looking into fixes for this
Assignee: nobody → jschoenick
Status: NEW → ASSIGNED
See Also: → 793310
Try:
https://tbpl.mozilla.org/?tree=Try&rev=be3166b79df4

This is a minimal workaround to not act on tel: uris that contain # or *.
Philipp's comment here: https://bugzilla.mozilla.org/show_bug.cgi?id=793310#c15
Indicates that this is the most precise way to do it.

This should be fairly safe for branches. FWIW, it looks like
Jellybean/CyanogenMod10 is already immune to this (the dialer wont act on such
codes unless they are manually entered)
Attachment #665660 - Flags: review?(bzbarsky)
Comment on attachment 665660 [details] [diff] [review]
Don't act on tel: uris with * or # present

This looks ok to me, but it'd be good to al get review from someone who really knows this code.  In particular:

1)  Are we guaranteed that "tel" URIs will go through this branch, and not one of the two branches above?

2)  Do we need to do this for "sms" URIs too?
Attachment #665660 - Flags: review?(bzbarsky) → review+
Attachment #665660 - Flags: review?(blassey.bugs)
Comment on attachment 665660 [details] [diff] [review]
Don't act on tel: uris with * or # present

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

I'd really rather this be implemented in the uriloader code. Also, I'd prefer to warn the user when a number is potentially dangerous rather than out right refuse to load the uri.

::: mobile/android/base/GeckoAppShell.java
@@ +1193,5 @@
> +                // Bug 794034 - We don't want to pass MWI or USSD codes to the
> +                // dialer, and ensure the Uri class doesn't parse a tel: URI as
> +                // containing a fragment ('#')
> +                final String number = uri.getSchemeSpecificPart();
> +                if (number.contains("#") || number.contains("*") || uri.getFragment() != null) {

it is perfectly legit to have *'s and #'s in phone numbers. I'd prefer to restrict this to the specific patterns we seen be problematic (#*#).
Attachment #665660 - Flags: review?(blassey.bugs) → review-
(In reply to Brad Lassey [:blassey] from comment #9)
> I'd really rather this be implemented in the uriloader code.

The idea here is a simple fix that we could take on the branches, with a more involved fix following

> it is perfectly legit to have *'s and #'s in phone numbers. I'd prefer to
> restrict this to the specific patterns we seen be problematic (#*#).

In bug 793310 comment 15 Philipp suggests otherwise:
> Nope. We will HAVE TO ignore any tel: uris with any * or # as they could be MWI / USSD codes. #111# is a valid MWI code AFAIK.

For instance, *2767*3855# is a factory-reset code on at least some android phones.

> Also, I'd
> prefer to warn the user when a number is potentially dangerous rather than
> out right refuse to load the uri.

Seeing as this is a dialer issue that affects other browsers, treating this more as a workaround for an upstream bug seems appropriate. On my CM10 device, the dialer no longer acts on MWI codes unless they are explicitly typed.

I'll have to look at this more tomorrow to see if there's a minimal way to do this in uri loader code
(In reply to Brad Lassey [:blassey] from comment #9)
> I'd really rather this be implemented in the uriloader code.

FWIW, I filed this under Core in order to suggest the fix reside in Core, so that B2G and desktops with dialer apps (VoIP or GSM dongle) get covered.
(In reply to Brad Lassey [:blassey] from comment #9)
> it is perfectly legit to have *'s and #'s in phone numbers.

Which phone numbers are those?
b2g may have a sufficiently different implementation strategy here that there's no way to cover both it and non-b2g cases with a single check.

If we want to cover the rest, we should do it in the external protocol handler, I guess.  As long as no extensions install a tel: protocol handler....
(In reply to Philipp von Weitershausen [:philikon] from comment #12)
> (In reply to Brad Lassey [:blassey] from comment #9)
> > it is perfectly legit to have *'s and #'s in phone numbers.
> 
> Which phone numbers are those?

For example, the number for a local radio station in Boston is #969 on Verizon Wireless
(In reply to Brad Lassey [:blassey] from comment #14)
> (In reply to Philipp von Weitershausen [:philikon] from comment #12)
> > (In reply to Brad Lassey [:blassey] from comment #9)
> > > it is perfectly legit to have *'s and #'s in phone numbers.
> > 
> > Which phone numbers are those?
> 
> For example, the number for a local radio station in Boston is #969 on
> Verizon Wireless

I've looked at https://www.rfc-editor.org/rfc/rfc2806.txt - between phone numbers that start with a * and people dialing conference rooms (think Vidyo), we'll definitely break some real user scenario (however rare) by taking a fix that just doesn't act upon numbers with those characters.

Perhaps we should look for phone numbers with multiple */#, but no pauses? That would mean that it's not a typical phone number, and not a conference number of some sort.
Or perhaps, any phone number with more than one of either * or # and only numbers should be ignored.
The real question is: How many pages out there won't work if we disable tel: with numbers containing * or #.

No one is suggesting that we block dailing numbers with * and #. You can still dial those in the dialer like normal, this bug only affects pages with markup like <a href="tel:#1234">
For reference, this affects a broad array of phones, not just Samsung (Samsung is the only one with a published reset code, but other codes exist for other phones):

http://www.guardian.co.uk/technology/2012/sep/27/samsung-htc-phones-remote-wipe
http://www.isk.kth.se/~rbbo/ussdvul.html

Some phones have gotten an updated dialer app and are no longer vulnerable. Many older phones have not.

It's unfortunate that USSD/MMI codes can all be expressed as syntactically valid tel: URIs, but a traditional dialer only has 12 buttons so what else could they do?

I'm not perticularly angsty if we break things like a tel: link of a radio stations #969 number -- the reason they buy special codes like that is because they're short and memorable so it's not terribly inconvenient if users have to manually type it. On the other hand we probably don't have to break it, most of those cases I've seen are simple *XXXX or #xxxx which won't be confused with USSD/MMI codes.

The codes are hardware and/or GSM specific, but every spec or list I could find had them ALWAYS ending with a '#' (not counting the commands that end in a phone number to be dialed, which should be safe enough). The pattern we want to block is

  ^[*#].*#$   // starts with '*' or '#' and ends with '#'

http://forum.xda-developers.com/showthread.php?t=1894102
http://rishirajput.blogspot.com/2011/02/mobile-operator-codes-ussd-codes-for.html
I don't think anyone's going to cry about overblocking in Fx 16 with a simpler patch and then test the regexp approach in Fx 17. For one thing I've got a "pre-ponderance of the evidence" sort of case, but haven't found an actual spec that requires that format (should be somewhere on http://www.3gpp.org/ but I didn't have time) -- and of course we don't know if some OEMs didn't follow the spec for their own internal codes.
(In reply to Daniel Veditz [:dveditz] from comment #18)
>   ^[*#].*#$   // starts with '*' or '#' and ends with '#'

*#*#7780#*#* is a factory reset on some android devices

More at:
http://www.askvg.com/google-android-hidden-secret-codes/

This is clearly a dialer bug, and as mentioned my dialer already wont act on these codes when fed in externally. The carrier can really use whatever codes they feel like, so trying to regexp this very much runs the risk of leaving some phone/carrier/code combination vulnerable.

Seeing as this is a bug in android's dialer, and b2g's dialer is already being addressed in bug 793310 in non-shared code, I think taking a patch similar to mine in fennec's URI passing is the simplest way forward, with follow up bugs if there are better ideas for refactoring this.

One change we could make is to ignore digits after pauses, which are not sent as part of the number, but rather as touch tones once the call is connected. This would mean |8005555555,#4000| would still work.
I think the safest fix for 16 is to flip the  network.protocol-handler.warn-external.tel pref back to its default value of true.
(In reply to Brad Lassey [:blassey] from comment #21)
> I think the safest fix for 16 is to flip the 
> network.protocol-handler.warn-external.tel pref back to its default value of
> true.

My worry there is that people won't look at the number being dialed (assuming it's even shown) and just hit accept/continue like people do in most dialogues.

I'd personally much rather land John's patch for 16, try a regexp for 17 and so on. Breaking some misc sites here is IMO *much* better than letting even some users get exploited by this bug.
(In reply to Brad Lassey [:blassey] from comment #21)
> I think the safest fix for 16 is to flip the 
> network.protocol-handler.warn-external.tel pref back to its default value of
> true.

What's the UX when we flip this pref? I'm OK with this option as long as it presents the phone #, considering phones that let you still hit "send" after seeing the USSD code are considered unaffected.

Otherwise, let's go with the current patch.
The current patch seems like a pretty confusing user experience, they'll click on a link and it will say that we can't open it.

I believe the current warning if we flip the pref is "This link needs to be opened with an application." Which admittedly isn't much more useful.

https://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/handling/handling.properties#6
(In reply to Brad Lassey [:blassey] from comment #24)
> The current patch seems like a pretty confusing user experience, they'll
> click on a link and it will say that we can't open it.
> 
> I believe the current warning if we flip the pref is "This link needs to be
> opened with an application." Which admittedly isn't much more useful.
> 
> https://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/
> mozapps/handling/handling.properties#6

We should definitely look into a followup for providing better UX on trunk, but this still seems like the sanest way to get a fix into FF16 - especially if providing a dialog of some sort would require new l10n strings.
Comment on attachment 665660 [details] [diff] [review]
Don't act on tel: uris with * or # present

brad okay'd this on the assumption we'll look into improving it post-16, nominating for branches.
Attachment #665660 - Flags: review-
Attachment #665660 - Flags: approval-mozilla-beta?
Attachment #665660 - Flags: approval-mozilla-aurora?
Comment on attachment 665660 [details] [diff] [review]
Don't act on tel: uris with * or # present

[Triage Comment]
Please land on branches asap. Thanks for the quick fix!
Attachment #665660 - Flags: approval-mozilla-beta?
Attachment #665660 - Flags: approval-mozilla-beta+
Attachment #665660 - Flags: approval-mozilla-aurora?
Attachment #665660 - Flags: approval-mozilla-aurora+
Keywords: verifyme
https://hg.mozilla.org/mozilla-central/rev/4ae30378c203

Should this have a test?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Blocks: 797034
I opened bug 797034 to look into improving this for 17+
Verified FIXED

Used http://dylanreeve.com/phone.php :: tel:*%2306%23 :: 

Samsung Galaxy Nexus (Contacts, 4.1.1-398337, stock dialler). No dialler on load, where we used to get a dialler with the address *#06# prior).
Status: RESOLVED → VERIFIED
(In reply to Ryan VanderMeulen from comment #31)
> https://hg.mozilla.org/mozilla-central/rev/4ae30378c203
> 
> Should this have a test?

Could a mochitest check the low level code?
See Also: → CVE-2022-22758
You need to log in before you can comment on or make changes to this bug.