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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 58+ fixed
firefox57 --- wontfix
firefox58 + fixed
firefox59 + fixed

People

(Reporter: emilio, Assigned: emilio)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main58+][adv-esr52.6+][post-critsmash-triage])

Attachments

(1 file)

The UAF is legit, I've got a patch... C++ sucks.
Summary: UAF in nsCookieService with the patch for bug 1361815. → UAF in nsCookieService (uncovered by the patch for bug 1361815).
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)
[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.
OS: Unspecified → All
Hardware: Unspecified → All
Attachment #8940484 - Flags: review?(josh) → review+
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?
Gerry, what do you think about taking this for beta 16?
Flags: needinfo?(gchang)
This is sec-high. That would be good if we can include this in beta 16.
Flags: needinfo?(gchang)
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+
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 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 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.
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
https://hg.mozilla.org/mozilla-central/rev/c64f2b452add
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(abillings)
Whiteboard: [adv-main58+][adv-esr52.6+]
Flags: needinfo?(dveditz)
Flags: qe-verify-
Whiteboard: [adv-main58+][adv-esr52.6+] → [adv-main58+][adv-esr52.6+][post-critsmash-triage]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.