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

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Henri Sivonen (:hsivonen)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Andrew McCreight [:mccr8]]

This exploit is very public already, so hiding it isn't very useful.

Comment 5

[Alex Keybl [:akeybl]]

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.

Comment 6

[John Schoenick [:johns]]

Looking into fixes for this

Comment 7

[John Schoenick [:johns]]

Attachment: Don't act on tel: uris with * or # present

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)

Comment 8

[Boris Zbarsky [: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?

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

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 (#*#).

Comment 10

[John Schoenick [:johns]]

(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

Comment 11

[Henri Sivonen (:hsivonen)]

(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.

Comment 12

[Philipp von Weitershausen [:philikon]]

(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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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....

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 15

[Alex Keybl [:akeybl]]

(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.

Comment 16

[Alex Keybl [:akeybl]]

Or perhaps, any phone number with more than one of either * or # and only numbers should be ignored.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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">

Comment 18

[Daniel Veditz [:dveditz]]

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

Comment 19

[Daniel Veditz [:dveditz]]

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.

Comment 20

[John Schoenick [:johns]]

(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.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 22

[Johnny Stenback (:jst)]

(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.

Comment 23

[Alex Keybl [:akeybl]]

(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.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 25

[John Schoenick [:johns]]

(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 26

[John Schoenick [:johns]]

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.

Comment 27

[John Schoenick [:johns]]

Comment on attachment 665660 [details] [diff] [review]
Don't act on tel: uris with * or # present

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae30378c203

Comment 29

[Alex Keybl [:akeybl]]

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!

Comment 30

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-beta/rev/23a26ecfa0ca
https://hg.mozilla.org/releases/mozilla-aurora/rev/6c540806f986

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ae30378c203

Should this have a test?

Comment 32

[John Schoenick [:johns]]

I opened bug 797034 to look into improving this for 17+

Comment 33

[Aaron Train [:aaronmt]]

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).

Comment 34

[Mounir Lamouri (:mounir)]

(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?