Closed
Bug 1428589
Opened 6 years ago
Closed 6 years ago
UAF in nsCookieService (uncovered by the patch for bug 1361815).
Categories
(Core :: Networking: Cookies, defect)
Core
Networking: Cookies
Tracking
()
RESOLVED
FIXED
mozilla59
People
(Reporter: emilio, Assigned: emilio)
References
Details
(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main58+][adv-esr52.6+][post-critsmash-triage])
Attachments
(1 file)
3.14 KB,
patch
|
jdm
:
review+
gchang
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr52+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
The UAF is legit, I've got a patch... C++ sucks.
Assignee | ||
Updated•6 years ago
|
Summary: UAF in nsCookieService with the patch for bug 1361815. → UAF in nsCookieService (uncovered by the patch for bug 1361815).
Assignee | ||
Comment 1•6 years ago
|
||
Consider the following situation, which is what causes the failure: * `cookies` is an array of length 1 (either from the beginning, or because other cookies have been expired while in this loop). * the only cookie that remains is expired. We append the cookie to removedList, and then we call gCookieService->RemoveCookieFromList, which has the following code: if (aIter.entry->GetCookies().Length() == 1) { mDBState->hostTable.RawRemoveEntry(aIter.entry); } else { aIter.entry->GetCookies().RemoveElementAt(aIter.index); } If we enter the first branch, as it's the case, that will destroy the array. We're effectively removing stuff from the array while mutating it, which is scary. It's fine if we don't delete the array, since we iterate through it using indices, but still it's dangerous as heck. If we're the last element in the array though, we're doomed, because `cookies` is now destroyed. We not only try to access the array length again, but also we try to index on it the next time because we never stopped the loop (`i` is still zero, and the length may very well be garbage). Fix it by keeping the length in sync from the stack and breaking out from the loop if appropriately.
Attachment #8940484 -
Flags: review?(josh)
Comment 2•6 years ago
|
||
[Tracking Requested - why for this release]: legit UAFs are no fun. ESR at least has the same code, so assuming that it is affected. Thanks for investigating this, Emilio.
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox59:
--- → affected
status-firefox-esr52:
--- → affected
tracking-firefox59:
--- → ?
Keywords: csectype-uaf,
sec-high
OS: Unspecified → All
Hardware: Unspecified → All
Updated•6 years ago
|
Attachment #8940484 -
Flags: review?(josh) → review+
Assignee | ||
Comment 3•6 years ago
|
||
Comment on attachment 8940484 [details] [diff] [review] Don't read a destroyed cookie list if the last cookie in the entry expired. [Security approval request comment] How easily could an exploit be constructed based on the patch? Probably not too hard. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes. I could remove the commit message, which is comment 1, but even with that the patch is kind of obvious. Which older supported branches are affected by this flaw? All, it seems. 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? Given the code is the same in all branches, and the patch is simple, it won't be hard. It probably just applies cleanly. How likely is this patch to cause regressions; how much testing does it need? Not very likely, it just avoids the problem in the most trivial way possible.
Attachment #8940484 -
Flags: sec-approval?
Comment 4•6 years ago
|
||
Gerry, what do you think about taking this for beta 16?
tracking-firefox58:
--- → +
Flags: needinfo?(gchang)
Comment 5•6 years ago
|
||
This is sec-high. That would be good if we can include this in beta 16.
Flags: needinfo?(gchang)
Comment 6•6 years ago
|
||
Comment on attachment 8940484 [details] [diff] [review] Don't read a destroyed cookie list if the last cookie in the entry expired. sec-approval+. Can we get a beta nomination on this patch or one that applies? I'll give it beta+ now that Gerry has approved.
Attachment #8940484 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 7•6 years ago
|
||
Comment on attachment 8940484 [details] [diff] [review] Don't read a destroyed cookie list if the last cookie in the entry expired. Approval Request Comment [Feature/Bug causing the regression]: n/a [User impact if declined]: sec bug [Is this code covered by automated tests?]: only with the patch from bug 1361815 [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: no [Is the change risky?]: not risky [Why is the change risky/not risky?]: maintains the array length on the stack to avoid using a potentially destroyed array. [String changes made/needed]: none
Attachment #8940484 -
Flags: approval-mozilla-beta?
Comment 8•6 years ago
|
||
Comment on attachment 8940484 [details] [diff] [review] Don't read a destroyed cookie list if the last cookie in the entry expired. Fix a sec-high. Beta58+.
Attachment #8940484 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 9•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/000398f8c373407ba621d56cbd3b370cf43469a8
Updated•6 years ago
|
Comment 10•6 years ago
|
||
Comment on attachment 8940484 [details] [diff] [review] Don't read a destroyed cookie list if the last cookie in the entry expired. This grafts cleanly to ESR52. Let's get it approved for there too :)
Attachment #8940484 -
Flags: approval-mozilla-esr52?
Comment on attachment 8940484 [details] [diff] [review] Don't read a destroyed cookie list if the last cookie in the entry expired. Sec-high, taking it. Al, Dan, FYI
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Attachment #8940484 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
This never landed on 59. RyanVM is on it.
Comment 13•6 years ago
|
||
uplift |
It's on inbound, just missed this morning's merges. Should be in the next ones. https://hg.mozilla.org/releases/mozilla-esr52/rev/f7865afb1fe6
tracking-firefox-esr52:
--- → ?
Comment 14•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c64f2b452add
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•6 years ago
|
Flags: needinfo?(abillings)
Updated•6 years ago
|
Whiteboard: [adv-main58+][adv-esr52.6+]
Updated•6 years ago
|
Flags: needinfo?(dveditz)
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main58+][adv-esr52.6+] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security → core-security-release
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•