Closed Bug 1410134 Opened 2 years ago Closed 2 years ago

use-after-destruction in nsCookieService::RemoveCookiesWithOriginAttributes

Categories

(Core :: Networking: Cookies, defect, P2)

defect

Tracking

()

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

People

(Reporter: froydnj, Assigned: timhuang)

References

Details

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

Attachments

(2 files, 1 obsolete file)

Noticed this while testing a contributor's patches in bug 1361815.  RemoveCookiesWithOriginAttributes does:

for (auto iter = mDBState->hostTable.Iter(); ...) {
  ...
  const nsCookieEntry::ArrayType& cookies = entry.GetCookies();

  while (!cookies.IsEmpty()) {
    ...
    Remove(<stuff based on host/name/path of cookie>)
  }
}

The above Remove() call then locates the cookie based on the host/name/path (why we do this is unclear to me, since we already have the cookie to remove from the above loop) and calls RemoveCookieFromList.  RemoveCookieFromList then does, after some modifications to the cookie database:

  if (aIter.entry->GetCookies().Length() == 1) {
    // we're removing the last element in the array - so just remove the entry
    // from the hash. note that the entryclass' dtor will take care of
    // releasing this last element for us!
    mDBState->hostTable.RawRemoveEntry(aIter.entry);

  } else {
    // just remove the element from the list
    aIter.entry->GetCookies().RemoveElementAt(aIter.index);
  }

and if we're in the first arm of the `if`, then we delete the entry from the hashtable.  But RemoveCookiesWithOriginAttributes is still holding a reference to the array contained in the entry we just removed (and destroyed).  And we're going to use that reference to check whether the (now-destroyed) array is empty to terminate the loop.  (Deleting entries out of the hashtable during iteration without using the Remove() function provided by the iterator being used to iterate is also sketchy, but happens to work in this case.)

We don't run into issues now because ~nsTArray() winds up setting itself to an empty array and so reusing the destroyed nsTArray works, because we can still test the destroyed-but-empty array for emptiness in the loop.  We shouldn't be doing this, though: we're relying on properties of nsTArray that are not guaranteed to hold.  If we could fix the hashtable removal during iteration issue as well, that would be excellent.

Filing this as a security bug just in case there's some other dodginess going on here.  I don't *think* the above could be exploited currently, but I'd also like somebody to verify my opinion.
njn may be interested in this, since the hash table checking bits weren't able to catch this case.  This is a known problem (see the comment in PLDHashTable::RawRemove), but it'd be nice if we could strengthen the checks.
Nope, this is totally a UAF, because the remove can reallocate the underlying table, and then the `cookies` variable is pointing to freed memory.  Calling this a sec-high just because of UAF, but still don't know how exploitable this is.
Probably really goes back before bug 1214071, but that's the one with the blame on these particular lines, even if the remove-while-iterating bug was present before that.
Group: core-security → network-core-security
Ethan, will someone from your team be taking this?
Flags: needinfo?(ettseng)
Priority: -- → P2
Whiteboard: [OA]
(In reply to Tanvi Vyas[:tanvi] from comment #4)
> Ethan, will someone from your team be taking this?

Let me coordinate this.  Leave the NI open as a reminder for myself.
Assignee: nobody → ettseng
Flags: needinfo?(ettseng)
Assignee: ettseng → tihuang
Comment on attachment 8924552 [details] [diff] [review]
Fixing the remove-while-iterating for RemoveCookiesWithOriginAttributes.

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

::: netwerk/cookie/nsCookieService.cpp
@@ +5047,5 @@
>  
>      // Pattern matches. Delete all cookies within this nsCookieEntry.
>      const nsCookieEntry::ArrayType& cookies = entry->GetCookies();
> +    nsCookieEntry::ArrayType cookiesToBeRemoved(cookies.Length());
> +    for (nsCookieEntry::IndexType i = 0 ; i < cookies.Length(); ++i) {

I think we can use `nsCookieEntry::ArrayType cookiesToBeRemoved(cookies);`. If not, we should be able to use `cookiesToBeRemoved.AppendElements(cookies)` instead.

@@ +5057,3 @@
>  
>        nsAutoCString host;
>        cookie->GetHost(host);

Let's do this:
* use RefPtr<nsCookie> instead of nsCookie*
* call RemoveCookieFromList directly instead of Remove
* call NotifyChanged(cookie, u"deleted")
Attachment #8924552 - Flags: review?(josh) → review-
Attachment #8924552 - Attachment is obsolete: true
Attachment #8926256 - Flags: review?(josh) → review+
This patch needs the sec-approval flag added to it, since this is a secure bug.
Keywords: checkin-needed
(In reply to Josh Matthews [:jdm] from comment #10)
> This patch needs the sec-approval flag added to it, since this is a secure
> bug.
Dan, can you set sec-approval?
Flags: needinfo?(dveditz)
(In reply to Ethan Tseng [:ethan] - 57 Regression Engineering Support from comment #11)
> Dan, can you set sec-approval?

No, Tim needs to do it (or whoever is working on the patch). Setting the flag pre-populates a series of questions that need to be answered so that we can determine whether it's safe to grant sec-approval.
Flags: needinfo?(dveditz) → needinfo?(tihuang)
Comment on attachment 8926256 [details] [diff] [review]
Fixing the remove-while-iterating for RemoveCookiesWithOriginAttributes.

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

According to comment 0 and comment 2, this is hard to exploit.

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

No

[Which older supported branches are affected by this flaw?]

All of them

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

I believe this patch can be applied to all branches except esr52. For esr52, it is easy to create one.

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

The risk is low since it is a small patch and there is a test [1] to make sure RemoveCookiesWithOriginAttributes() works as expected.

[1] https://searchfox.org/mozilla-central/source/netwerk/cookie/test/browser/browser_originattributes.js#64
Flags: needinfo?(tihuang)
Attachment #8926256 - Flags: sec-approval?
Sec-approval+ for checkin on November 28, two weeks into the new cycle. Please do not check in before that date.

We'll want this on other branches so ESR52 should be made and nominated at that time as well as beta.
Whiteboard: [OA] → [OA][checkin on 11/28]
Attachment #8926256 - Flags: sec-approval? → sec-approval+
Whiteboard: [OA][checkin on 11/28] → [OA][checkin on 11/28][necko-triaged]
Comment on attachment 8926256 [details] [diff] [review]
Fixing the remove-while-iterating for RemoveCookiesWithOriginAttributes.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1214071
[User impact if declined]: use-after-destruction
[Is this code covered by automated tests?]: yes
[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?]: low-risk
[Why is the change risky/not risky?]: It just changes the way how to remove cookies. Its behavior does change at all. 
[String changes made/needed]: no
Attachment #8926256 - Flags: approval-mozilla-beta?
Attached patch Patch for esr52.Splinter Review
Attachment #8932345 - Flags: review+
Comment on attachment 8932345 [details] [diff] [review]
Patch for esr52.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: use-after-destruction
Fix Landed on Version: 59
Risk to taking this patch (and alternatives if risky): low-risk
String or UUID changes made by this patch: nope

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8932345 - Flags: approval-mozilla-esr52?
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffab26e34d92c1fc2e5103d2bad3625e180963bb
Whiteboard: [OA][checkin on 11/28][necko-triaged] → [OA][necko-triaged]
https://hg.mozilla.org/mozilla-central/rev/ffab26e34d92
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Group: network-core-security → core-security-release
Comment on attachment 8926256 [details] [diff] [review]
Fixing the remove-while-iterating for RemoveCookiesWithOriginAttributes.

Fix a sec-high. Beta58+ & ESR52+.
Attachment #8926256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8932345 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [OA][necko-triaged] → [OA][necko-triaged][adv-main58+][adv-esr52.6+]
Flags: qe-verify-
Whiteboard: [OA][necko-triaged][adv-main58+][adv-esr52.6+] → [OA][necko-triaged][adv-main58+][adv-esr52.6+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.