Closed
Bug 967341
Opened 10 years ago
Closed 10 years ago
"ASSERTION: index exceeds allowable range" - nsStandardURL::Host
Categories
(Core :: Networking, defect)
Tracking
()
VERIFIED
FIXED
mozilla30
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)
268 bytes,
text/html
|
Details | |
13.78 KB,
text/plain
|
Details | |
8.19 KB,
patch
|
mayhemer
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr24+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
###!!! 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
Reporter | ||
Comment 1•10 years ago
|
||
Reporter | ||
Comment 2•10 years ago
|
||
Bug 967418 has another testcase involving try..catch around setting a URL's href to a string with multiple colons.
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=7ad4f0b2f629
Comment 8•10 years ago
|
||
sec-high because it looks like an array bounds issue, but maybe I'm just being paranoid.
Keywords: sec-high
Comment 9•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
status-b2g18:
--- → affected
status-firefox27:
--- → affected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox-esr24:
--- → affected
Assignee | ||
Comment 10•10 years ago
|
||
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?
Updated•10 years ago
|
status-b2g-v1.1hd:
--- → affected
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-b2g-v1.4:
--- → affected
tracking-firefox28:
--- → ?
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → +
tracking-firefox-esr24:
--- → ?
Whiteboard: [fuzzblocker] → [fuzzblocker][checkin 2/18 or later]
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
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?
Updated•10 years ago
|
Comment 14•10 years ago
|
||
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 15•10 years ago
|
||
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?
Reporter | ||
Comment 16•10 years ago
|
||
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•10 years ago
|
||
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
Assignee | ||
Comment 18•10 years ago
|
||
(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•10 years ago
|
||
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 20•10 years ago
|
||
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+
Assignee | ||
Comment 21•10 years ago
|
||
I'll do this 2/19 when I'm back in the office
Assignee | ||
Comment 22•10 years ago
|
||
(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.
Assignee | ||
Comment 23•10 years ago
|
||
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•10 years ago
|
||
landed on central as https://hg.mozilla.org/mozilla-central/rev/71f4051ae6b3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 25•10 years ago
|
||
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
Whiteboard: [fuzzblocker][checkin 2/18 or later] → [fuzzblocker]
Comment 26•10 years ago
|
||
Matt, can you please evaluate if this needs QA testing before we release Firefox 28?
Flags: needinfo?(mwobensmith)
Comment 27•10 years ago
|
||
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)
Updated•10 years ago
|
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main28+][adv-esr24.4+]
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8387757 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
comment 28 was posted into the wrong tab - not relevant. sorry for the spam.
Updated•10 years ago
|
status-b2g-v1.3T:
--- → fixed
Reporter | ||
Updated•10 years ago
|
Blocks: fuzz-urlutils
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•