Closed Bug 967341 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Networking, defect)

x86_64
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox27 --- wontfix
firefox28 + verified
firefox29 + verified
firefox30 + verified
firefox-esr24 28+ verified
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: jruderman, Assigned: mcmanus)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [fuzzblocker][adv-main28+][adv-esr24.4+])

Attachments

(3 files, 1 obsolete file)

Attached file 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
Attached file stack+
Bug 967418 has another testcase involving try..catch around setting a URL's href to a string with multiple colons.
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.
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)
Attached patch [PATCH 1/1] wipSplinter Review
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
Attachment #8370296 - Flags: review?(honzab.moz)
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
sec-high because it looks like an array bounds issue, but maybe I'm just being paranoid.
Keywords: sec-high
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
Attachment #8370296 - Flags: review?(honzab.moz) → review+
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.
Attachment #8370296 - Flags: sec-approval?
Whiteboard: [fuzzblocker] → [fuzzblocker][checkin 2/18 or later]
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.
Attachment #8370296 - Flags: sec-approval? → sec-approval+
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.
Attachment #8370296 - Flags: approval-mozilla-esr24?
Attachment #8370296 - Flags: approval-mozilla-beta?
Attachment #8370296 - Flags: approval-mozilla-b2g28?
Attachment #8370296 - Flags: approval-mozilla-aurora?
Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

Is that normal that the title of the commit is WIP?
Attachment #8370296 - Flags: approval-mozilla-esr24?
Attachment #8370296 - Flags: approval-mozilla-esr24+
Attachment #8370296 - Flags: approval-mozilla-beta?
Attachment #8370296 - Flags: approval-mozilla-beta+
Attachment #8370296 - Flags: approval-mozilla-aurora?
Attachment #8370296 - Flags: approval-mozilla-aurora+
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
Attachment #8370296 - Flags: approval-mozilla-b2g28?
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.
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
(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 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.
Attachment #8370296 - Flags: approval-mozilla-esr24?
Attachment #8370296 - Flags: approval-mozilla-esr24+
Attachment #8370296 - Flags: approval-mozilla-beta?
Attachment #8370296 - Flags: approval-mozilla-beta+
Attachment #8370296 - Flags: approval-mozilla-aurora?
Attachment #8370296 - Flags: approval-mozilla-aurora+
Comment on attachment 8370296 [details] [diff] [review]
[PATCH 1/1] wip

The date has come! :)
Attachment #8370296 - Flags: approval-mozilla-esr24?
Attachment #8370296 - Flags: approval-mozilla-esr24+
Attachment #8370296 - Flags: approval-mozilla-beta?
Attachment #8370296 - Flags: approval-mozilla-beta+
Attachment #8370296 - Flags: approval-mozilla-aurora?
Attachment #8370296 - Flags: approval-mozilla-aurora+
I'll do this 2/19 when I'm back in the office
(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.
landed on central as https://hg.mozilla.org/mozilla-central/rev/71f4051ae6b3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Matt, can you please evaluate if this needs QA testing before we release Firefox 28?
Flags: needinfo?(mwobensmith)
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.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mwobensmith)
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main28+][adv-esr24.4+]
Attachment #8387757 - Attachment is obsolete: true
comment 28 was posted into the wrong tab - not relevant. sorry for the spam.
Group: core-security
You need to log in before you can comment on or make changes to this bug.