1410134 - use-after-destruction in nsCookieService::RemoveCookiesWithOriginAttributes

Status: RESOLVED FIXED

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

Description

[Nathan Froyd [:froydnj]]

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.

Comment 1

[Nathan Froyd [:froydnj]]

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.

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Tanvi Vyas[:tanvi]]

Ethan, will someone from your team be taking this?

Comment 5

[Ethan Tseng [:ethan]]

(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.

Comment 6

[Tim Huang[:timhuang]]

Attachment: Fixing the remove-while-iterating for RemoveCookiesWithOriginAttributes.

Comment 7

[Josh Matthews [:jdm]]

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")

Comment 8

[Tim Huang[:timhuang]]

Attachment: Fixing the remove-while-iterating for RemoveCookiesWithOriginAttributes.

Comment 9

[Tim Huang[:timhuang]]

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=329fc83410a54b33285bd4def3d6ef9b6d55ace0

Comment 10

[Josh Matthews [:jdm]]

This patch needs the sec-approval flag added to it, since this is a secure bug.

Comment 11

[Ethan Tseng [:ethan]]

(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?

Comment 12

[Daniel Veditz [: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.

Comment 13

[Tim Huang[:timhuang]]

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

Comment 14

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 15

[Tim Huang[:timhuang]]

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

Comment 16

[Tim Huang[:timhuang]]

Attachment: Patch for esr52.

Comment 17

[Tim Huang[:timhuang]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffab26e34d92c1fc2e5103d2bad3625e180963bb

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ffab26e34d92

Comment 20

[Gerry Chang [:gchang]]

Comment on attachment 8926256 [details] [diff] [review]
Fixing the remove-while-iterating for RemoveCookiesWithOriginAttributes.

Fix a sec-high. Beta58+ & ESR52+.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b6d37f26c25b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5e7b16213198