967341 - "ASSERTION: index exceeds allowable range" - nsStandardURL::Host

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: index exceeds allowable range: 'i <= mLength', file nsTString.h

>    frame #0: 0x0000000101622ddb XUL`nsCString::CharAt(this=0x000000012505b540, i=4294967295) const + 91 at nsTString.h:105
>    frame #1: 0x00000001017d2efb XUL`nsStandardURL::Host(this=0x000000012505b500) + 91 at nsStandardURL.h:352
>    frame #2: 0x00000001017c0048 XUL`nsStandardURL::GetHost(this=0x000000012505b500, result=0x00007fff5fbf6c68) + 40 at nsStandardURL.cpp:1016
>    frame #3: 0x000000010332d17e XUL`mozilla::dom::URL::GetHostname(this=0x00000001262ffc70, aHostname=0x00007fff5fbf6d18) const + 126 at URL.cpp:334
>    frame #4: 0x0000000102d7ffae XUL`mozilla::dom::URLBinding::get_hostname(cx=0x000000011f8c3e70, obj=Handle<JSObject *> at 0x00007fff5fbf6de0, self=0x00000001262ffc70, args=JSJitGetterCallArgs at 0x00007fff5fbf6dd0) + 94 at URLBinding.cpp:622

Comment 1

[Jesse Ruderman]

Attachment: stack+

Comment 2

[Jesse Ruderman]

Bug 967418 has another testcase involving try..catch around setting a URL's href to a string with multiple colons.

Comment 3

[Patrick McManus [:mcmanus]]

awesome test case.

I diagnosed this as a failed SetSpec() that didn't clear() its internal state after partially parsing the url and before returning the failure code. A subsequent access to the url deref'd ptr + 0xffffffff

That doesn't seem easily exploitable to me, but I'm better at writing fixes than exploits.

this code has been there since before the hg migration.

Comment 5

[Patrick McManus [:mcmanus]]

due to the url being referenced after the try/catch there are also many tripped NS_ERRORs of the form

METHODIMPL foo()
{
...
  if (!precond) {
     NS_ERROR("whoa!");
     return NS_ERROR_UNEXPECTED;
   }
 ...
 return NS_OK;
}

I've changed those to NS_WARNINGs as the code is doing the right thing, and its obviously possible to invoke the method from arbitrary JS (as the testcase shows)

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: [PATCH 1/1] wip

From 744634ee1e53a0ad1202615d10cfc25d1398869b Mon Sep 17 00:00:00 2001
---
 netwerk/base/src/nsStandardURL.cpp           | 36 +++++++++++++----------
 netwerk/test/mochitests/mochitest.ini        |  1 +
 netwerk/test/mochitests/test_uri_scheme.html | 43 ++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 15 deletions(-)
 create mode 100644 netwerk/test/mochitests/test_uri_scheme.html

Comment 7

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=7ad4f0b2f629

Comment 8

[Daniel Veditz [:dveditz]]

sec-high because it looks like an array bounds issue, but maybe I'm just being paranoid.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

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

The test only produces assertion failure when the c-patch is not applied.  I don't know if this makes the run orange...  Probably does.

r=honzab

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

its unclear how serious the exploit is - its a readonly array index problem always offset 0xffffffff

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

easily

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

yes

Which older supported branches are affected by this flaw?

all of them

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

they should be almost identical. this code is stable.

How likely is this patch to cause regressions; how much testing does it need?

it involves cleanup in an error path, so it is unlikely to cause regressions.

Comment 11

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

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

sec-approval+ for trunk but not before 2/18/2014. I would like us to be a few weeks into the cycle before we check in something that you say can be easily reverse engineered from the checkin.

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip


[Approval Request Comment]
Bug caused by (feature/regressing bug #): original sin
User impact if declined: easy crash bug, potential data leakage
Testing completed: mochitest included
Risk to taking this patch (and alternatives if risky): fix is in an error path, so believed to be low risk. alternative would be to simply wait - this has existed for a long time.
String or UUID changes made by this patch: none

I'm not up to date on what the appropriate b2g backport targets are right now - I only flagged one - but if the set should be different please adjust the flags.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

Is that normal that the title of the commit is WIP?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

beta28 is merging to b2g28 during this cycle, so additional approval isn't needed.

https://wiki.mozilla.org/Release_Management/B2G_Landing#Landing_Procedure_4

Comment 16

[Jesse Ruderman]

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

Can you make correctness tests? e.g. after the patch, the "o.href = ...;" line should not have any effect on the values returned by various getters? I think that would be more solid than importing the testcase as a crashtest, and also less helpful to attackers trying to construct an exploit.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Note that netwerk/tests/mochitests was created in bug 497003, which landed on Fx25. So for esr24 and b2g18, you'll need to do some extra work for these tests. Not sure what all of what landed in the below push is needed for the test you're adding here:
https://hg.mozilla.org/mozilla-central/rev/57ffaa460a6b

Comment 18

[Patrick McManus [:mcmanus]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #17)
> Note that netwerk/tests/mochitests was created in bug 497003, which landed
> on Fx25. So for esr24 and b2g18, you'll need to do some extra work for these
> tests. Not sure what all of what landed in the below push is needed for the
> test you're adding here:
> https://hg.mozilla.org/mozilla-central/rev/57ffaa460a6b

thanks ryan.. the lower risk approach might actually just be to skip the test for <25. This isn't at high risk of regression.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

Actually, we shouldn't approve this for uplift yet until after 2/18 (see whiteboard) so changing the flags back until that time has come.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

The date has come! :)

Comment 21

[Patrick McManus [:mcmanus]]

I'll do this 2/19 when I'm back in the office

Comment 22

[Patrick McManus [:mcmanus]]

(In reply to Jesse Ruderman from comment #16)
> Comment on attachment 8370296 [details] [diff] [review]
> 
> Can you make correctness tests? e.g. after the patch, the "o.href = ...;"
> line should not have any effect on the values returned by various getters?

can't quite do that because the bad assign does have an effect actually - the whole thing is partially processed. The bug fix here is to clean it up and not leave it in an inconsistent internal state that will crash if used again.

I did change the test to assign a valid href to the URI after the access that previously triggered the bug and updated the test to assert the attributes of the new href.

Comment 23

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71f4051ae6b3
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/98352d9e9067
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/5428e2888c94
remote:   https://hg.mozilla.org/releases/mozilla-esr24/rev/7aa4288ec774

Comment 24

[Carsten Book [:Tomcat]]

landed on central as https://hg.mozilla.org/mozilla-central/rev/71f4051ae6b3

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/5428e2888c94
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7560863a3742
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1421a6b7fc51
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/1421a6b7fc51

Comment 26

[u279076]

Matt, can you please evaluate if this needs QA testing before we release Firefox 28?

Comment 27

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

Confirmed crash on FF 27, release non-ASan build, as well as ASan FF28 build pre-patch.
Verified fixed on ASan builds of FF28, 29 and 30, 2014-03-03.
Verified fixed on non-ASan build of ESR24, 2014-03-01.

Comment 28

[Patrick McManus [:mcmanus]]

Attachment: spdy/http-2 server push fails on image revalidation

Comment 29

[Patrick McManus [:mcmanus]]

comment 28 was posted into the wrong tab - not relevant. sorry for the spam.