935499 - accessing short IDN causes Firefox crash

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Makoto Kato [:m_kato]]

Repro Step
==========
1 Acess http://xn--?-jju/ (or browse firefox this bug entry)

Crash ID
========
bp-dbcbfad2-12ba-4596-b27e-c99fc2131106

Comment 1

[Makoto Kato [:m_kato]]

Attachment: IsACE should return false if too short

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 828539 [details] [diff] [review]
IsACE should return false if too short

># HG changeset patch
># Parent f48aa69ea925895e66dd84178e01987f7d054cc8
>Bug 935499. try: -b do -p all -u all -t none
>
>diff --git a/netwerk/dns/nsIDNService.cpp b/netwerk/dns/nsIDNService.cpp
>--- a/netwerk/dns/nsIDNService.cpp
>+++ b/netwerk/dns/nsIDNService.cpp
>@@ -265,16 +265,22 @@ nsresult nsIDNService::ACEtoUTF8(const n
>   }
> 
>   return NS_OK;
> }
> 
> /* boolean isACE(in ACString input); */
> NS_IMETHODIMP nsIDNService::IsACE(const nsACString & input, bool *_retval)
> {
>+  if (input.Length() <= 4) {
>+    // input string is too short for ACE/RACE
>+    *_retval = false;
>+    return NS_OK;
>+  }
>+
>   nsACString::const_iterator begin;
>   input.BeginReading(begin);
> 
>   const char *data = begin.get();
>   uint32_t dataLen = begin.size_forward();
> 
>   // look for the ACE prefix in the input string.  it may occur
>   // at the beginning of any segment in the domain name.  for
>diff --git a/netwerk/test/unit/test_bug935499.js b/netwerk/test/unit/test_bug935499.js
>new file mode 100644
>--- /dev/null
>+++ b/netwerk/test/unit/test_bug935499.js
>@@ -0,0 +1,8 @@
>+function run_test() {
>+  var idnService = Cc["@mozilla.org/network/idn-service;1"]
>+                   .getService(Ci.nsIIDNService);
>+
>+  // don't crash xn-- IDN
>+  var isASCII = {};
>+  do_check_eq(idnService.convertToDisplayIDN("xn--", isASCII), "xn--");
>+}
>diff --git a/netwerk/test/unit/xpcshell.ini b/netwerk/test/unit/xpcshell.ini
>--- a/netwerk/test/unit/xpcshell.ini
>+++ b/netwerk/test/unit/xpcshell.ini
>@@ -159,16 +159,17 @@ skip-if = os == "android"
> [test_bug669001.js]
> # Bug 675039: intermittent fail on Android-armv6 
> skip-if = os == "android"
> [test_bug712914_secinfo_validation.js]
> [test_bug770243.js]
> [test_bug894586.js]
> # Allocating 4GB might actually succeed on 64 bit machines
> skip-if = bits != 32
>+[test_bug935499.js]
> [test_doomentry.js]
> [test_cacheflags.js]
> # Bug 675039: intermittent fail on Android-armv6 
> skip-if = os == "android"
> [test_cache_jar.js]
> [test_channel_close.js]
> [test_compareURIs.js]
> [test_compressappend.js]

Comment 3

[Makoto Kato [:m_kato]]

Comment on attachment 828539 [details] [diff] [review]
IsACE should return false if too short

Current code passes length = 0 to punycode_decode, so Gecko will crash.

Comment 4

[Honza Bambas (:mayhemer)]

Definitely the problem had been introduced with this changeset:

http://hg.mozilla.org/mozilla-central/rev/515d75612e53

It actually contains a classic security bug...

You apparently cannot do |j = input_length - 1| and then operate on |input[j]| w/o verifying that input_length is >= 1.

The fix has to be there IMO, however Makoto's patch is probably correct too.

Rather hiding this bug as a precaution.  At least this is a bug allowing application crash on bad memory access at any time just by referring e.g. an image in bad address.  No idea what happens when the *(input-1) memory is valid and we continue executing the code after the scan loop with a random value of |b|.

Comment 5

[Patrick McManus [:mcmanus]]

dup of 935499?

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> dup of 935499?

It's this bug ;)

Comment 7

[pchang9]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Definitely the problem had been introduced with this changeset:
> 
> http://hg.mozilla.org/mozilla-central/rev/515d75612e53
> 
> It actually contains a classic security bug...
> 
> You apparently cannot do |j = input_length - 1| and then operate on
> |input[j]| w/o verifying that input_length is >= 1.

"j" should be a signed value, not unsigned "punycode_uint".
Once "input_length" is 0, "j" becomes -1, or largest value "punycode_uint" can hold.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 828539 [details] [diff] [review]
IsACE should return false if too short

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

Sorry, not the correct place to fix this bug IMO.  See my earlier comments on a proposed fix.  If there is a reason to add this one patch, then please add more explanatory comments (at least to the bug) why we should be taking it in.

We should move forward with this bug, it's kinda critical...

r-

Comment 9

[Patrick McManus [:mcmanus]]

introduced in ff25

Comment 10

[Makoto Kato [:m_kato]]

Attachment: v2

Even if not landing bug 898221, new test case (test_bug935499.js) for this bug will hit the assertion by "ASCII label in IDN checking" in nsIDNService::isLabelSafe().  So I think punycode_decode() should returns error if input_length == 0.

Comment 11

[Daniel Veditz [:dveditz]]

Why is this crashing some people, but not me on a nightly?

A crashing bug comment is not a good approach if you actually want that bug to be fixed, please put crashing things in attachment testcases in the future.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 831427 [details] [diff] [review]
v2

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

r=honzab

thanks.  this according mxr examination has the same affect as the first patch, but is on the right place.

Comment 13

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd82c64f4b7

Comment 14

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/mozilla-central/rev/5dd82c64f4b7

Comment 15

[Makoto Kato [:m_kato]]

Attachment: for Aurora/Beta

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 898221
User impact if declined: If html has link of http:// xn-- / or access to it, Firefox will crash
Testing completed (on m-c, etc.): landed in m-c.  Also added test case
Risk to taking this patch (and alternatives if risky): low. just check length of IDN.
String or IDL/UUID changes made by this patch: no

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Please follow the procedure at https://wiki.mozilla.org/Security/Bug_Approval_Process. AFAICT, this patch should not have landed without sec-approval+.

Comment 17

[Makoto Kato [:m_kato]]

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #16)
> Please follow the procedure at
> https://wiki.mozilla.org/Security/Bug_Approval_Process. AFAICT, this patch
> should not have landed without sec-approval+.

I'm sorry.  I don't know this page, so I will follow it next.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8333613 [details] [diff] [review]
for Aurora/Beta

Well since it did already land to central we'd better take it on branches.  And for what it's worth, my Firefox is currently unusable for bug triage because I keep crashing on this very bug.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 831427 [details] [diff] [review]
v2

I'm editing this bug in Chrome. I hate this bug.

Since we just potentially pwned ourselves by checking it in, I'll go ahead and give retroactive approval. Let's try not to do this. :-)

It would be nice if someone actually answered the sec-approval template questions since they help with writing advisories and such.

Comment 20

[Patrick McManus [:mcmanus]]

(In reply to Al Billings [:abillings] from comment #19)
> Comment on attachment 831427 [details] [diff] [review]
> v2
> 
> I'm editing this bug in Chrome. I hate this bug.

setting about:config network.IDN_show_punycode to true seemed to let me see this bug without crashing. fwiw.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/de6d1ca54ab2
https://hg.mozilla.org/releases/mozilla-beta/rev/a14c36a25b92

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Al Billings [:abillings] from comment #19)
> It would be nice if someone actually answered the sec-approval template
> questions since they help with writing advisories and such.

Kato-san, please set the sec-approval flag on the aurora/beta patch to ?. This should cause a template of questions to come up. Please answer those questions as best as you can. Thanks!

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> https://hg.mozilla.org/releases/mozilla-beta/rev/a14c36a25b92

Looks like this broke an xpcshell test on beta: https://tbpl.mozilla.org/php/getParsedLog.php?id=30727813&tree=Mozilla-Beta

I don't have a clone of beta handy to back this out, though.

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

rnewman backed it out in https://hg.mozilla.org/releases/mozilla-beta/rev/266f8698dbb8

Comment 25

[Makoto Kato [:m_kato]]

Comment on attachment 8333613 [details] [diff] [review]
for Aurora/Beta

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
This causes crash by HTML.  Firefox for OSX/Linux x86-64 will crash by opening this bug entry.

When using http:// xn-- /  (remove space), punycode_decode reads out of range memory.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
user can make crash.

Which older supported branches are affected by this flaw?
Release, Beta and Aurora.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes.  But I need update unit test for beta.

How likely is this patch to cause regressions; how much testing does it need?
No. since this affects xn-- punycode only, I added unit test

Comment 26

[Makoto Kato [:m_kato]]

ah(In reply to Wes Kocher (:KWierso) from comment #23)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> > https://hg.mozilla.org/releases/mozilla-beta/rev/a14c36a25b92
> 
> Looks like this broke an xpcshell test on beta:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=30727813&tree=Mozilla-Beta
> 
> I don't have a clone of beta handy to back this out, though.

I need update unit test for Cc is undefined...

Comment 27

[Makoto Kato [:m_kato]]

Attachment: for beta

Comment 28

[Makoto Kato [:m_kato]]

Comment on attachment 8334413 [details] [diff] [review]
for beta

unit test for Beta needs Cc/Ci define.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8334413 [details] [diff] [review]
for beta

Given that I would have pushed this change as a trivial test-only bustage fix had I been around to do so, I don't think this needs re-review.

https://hg.mozilla.org/releases/mozilla-beta/rev/5ebf49f88efe

Comment 30

[Ioana (away)]

I couldn't reproduce this issue on buggy Firefox 25 and 26 builds on Windows 7 64bit. It did reproduce on Ubuntu 12.10 64bit though, so I also verified it on this OS:

I can't reproduce the crash anymore on the 11/19 Aurora and 11/18 Nightly.
The same crash still happens on Firefox 26.0b6: https://crash-stats.mozilla.com/report/index/4fcdd848-8ee6-4d44-97de-e1dc22131119

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/5ebf49f88efe

Comment 33

[David Chan [:dchan]]

Per comment 7 / 9, b2g18 isn't affected. The bug was introduced in end of July so b2g1.1 may be affected, though this was a won't fix for Fx25

Comment 34

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on FF26, 27 and 28, 2013-11-13.
Verified fixed on FF26, 27 and 28, 2013-11-21.