1459383 - (CVE-2018-12378) Use After Free in indexedDB

Status: VERIFIED FIXED

(Core :: Storage: IndexedDB, defect, P1)

Description

[zhanjiasong45]

Attachment: FireFox_IndexDB_UAF.html

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/65.0.3325.181 Safari/537.36

Steps to reproduce:

run firefox with attached html


Actual results:

https://searchfox.org/mozilla-central/source/dom/indexedDB/KeyPath.cpp#323

nsresult
KeyPath::ExtractKey(JSContext* aCx, const JS::Value& aValue, Key& aKey) const
{
  uint32_t len = mStrings.Length();
  JS::Rooted<JS::Value> value(aCx);

  aKey.Unset();

  for (uint32_t i = 0; i < len; ++i) {
    nsresult rv = GetJSValFromKeyPathString(aCx, aValue, mStrings[i],
                                            value.address(),
                                            DoNotCreateProperties, nullptr,
                                            nullptr);
    if (NS_FAILED(rv)) {
      return rv;
    }

    if (NS_FAILED(aKey.AppendItem(aCx, IsArray() && i == 0, value))) {
      NS_ASSERTION(aKey.IsUnset(), "Encoding error should unset");
      return NS_ERROR_DOM_INDEXEDDB_DATA_ERR;
    }
  }

  aKey.FinishArray();

  return NS_OK;
}

GetJSValFromKeyPathString can execute js callback by setting a getter. In the callback, one can call deleteIndex free the KeyPath now using, which make mStrings invalid.

Comment 1

[zhanjiasong45]

Attachment: ASAN ouput.txt

Comment 2

[zhanjiasong45]

Attachment: debug output.txt

Comment 3

[Andrew McCreight [:mccr8]]

Jan, could you take a look at this, please? Thanks.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

This is a sec-critical, needs an owner.

And does KeyPath::ExtractKeyAsJSVal have similar issue?

Comment 5

[Jan Varga [:janv]]

Ok, I'll take a look.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Thanks, assigning to you then, so that this doesn't show up as unassigned bug in the bugzilla queries.

Comment 7

[Daniel Veditz [:dveditz]]

Testcase crashes on opt builds too, though less obviously a UAF (null crash). bp-3b759c24-5385-4600-a7e7-237970180521

Jan: how are we coming on this one? Since we apparently need to uplift to ESR-52 the sooner the better for test/verification.

Comment 8

[Jan Varga [:janv]]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> Testcase crashes on opt builds too, though less obviously a UAF (null
> crash). bp-3b759c24-5385-4600-a7e7-237970180521
> 
> Jan: how are we coming on this one? Since we apparently need to uplift to
> ESR-52 the sooner the better for test/verification.

Working on it.

Comment 9

[Marion Daly [:mdaly]]

Need an update.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Per Jan, this isn't going to be fixed in time for 61.

Comment 11

[Marion Daly [:mdaly]]

Jan please give an update.

Comment 12

[Jan Varga [:janv]]

Will do on Monday.

Comment 13

[Marion Daly [:mdaly]]

Update please.

Comment 14

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

(In reply to Marion Daly [:mdaly] from comment #13)
> Update please.

Any updates will be in this bug.

Comment 15

[Jan Varga [:janv]]

(In reply to Marion Daly [:mdaly] from comment #11)
> Jan please give an update.

This is my top priority now. I'll send more details soon.

Comment 16

[Jan Varga [:janv]]

I think the best solution would be to do a clone before extracting keys, see bug 1404274.
I'm going to investigated this more ...

Comment 17

[Jan Varga [:janv]]

Yes, cloning fixes this bug, I have a patch, but I need to test it a bit more.

Comment 18

[Jan Varga [:janv]]

A submitted a patch for this in bug 1404274.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Doing the restating for the patch on bug 1404274 here.  The fix there looks great from a security perspective.  I'll also comment about a nit or two on that bug.

Severity-wise, I'm not sure this bug is actually exploitable in a sec-critical sense.  The KeyPath is used in a read-only fashion, specifically its mType and its nsTArray<nsString> mStrings.  If the memory gets poisoned, we expect a crash dereferencing the array's mHdr.  If the memory gets zeroed, we expect a null-crash on mHdr.  If the memory got reused, we expect either nsTArray to crash because of an illegal index, or a potentially wild nsString read which potentially will cause a crash.  If the wild string read could be controlled and the object being put() is actually a Proxy, the payload of the string (which is used as a key), could then I think be exposed to content.  So in that case this would be sec-high.  This would take some doing, however.

## The security problem
The getter deletes an index inside a version-change transaction.  Because we don't perform a structured clone to create an inert object to process, all of [any objectstore key path traversal, the index path traversals, and the value structured clone itself] each provide an opportunity for content code to trigger unexpected re-entrancy through the use of getters/similar.

Aside: The logic in IDBObjectStore::GetAddInfo obtains a `const nsTArray<IndexMetadata>&` reference to `mSpec->indexes()`.  mSpec is an IPDL ObjectStoreSpec instance.  The logic also saves off indexes.Length() into idxCount for loop iteration, which is sketchy, but actually is somewhat helpful as nsTArray::ElementAt will force a crash if we subscript off the end of the array.  However, this only happens if the index that's deleted is not already the last index, which it is in this case (and any intentional attack that wants to trigger the problem).

AppendIndexUpdateInfo takes a number of references to the current IndexMetadata's sub-fields at the time of entry.  Of particular note is the KeyPath, which is a non-IPDL class nested inside the IPDL IndexMetadata.  If mSpec->indexes()'s array is reallocated or freed, the KeyPath reference is now to freed memory.  In this example case, we go from an array length of 1 to an array length of 0, so nsTArray ShrinkCapacity() will definitely be invoked and definitely free the memory out from under the KeyPath.

The actual freeing will occur in the helper function GetJSValFromKeyPathString.  Its aKeyPathString will potentially be invalid after the JS engine call, however a use-after-free dereference will only occur there if we have a period-delimited string that was being tokenized.  (The tokenizer's hasMoreTokens() won't dereference the string; it just compares pointers.)  So we return to KeyPath::ExtractKey.  The ASAN build then explodes inside the call to (this->)IsArray because `this` is now referencing freed memory.  The non-ASAN build faults on the next turn of the loop on mStrings[i] because the array, which is also living in freed memory, has a poisoned mHdr value.

## The security fix
### Overview
By structured cloning the user-provided payload values into a cloned representation that we perform all subsequent traversals on, we can be confident that we won't be calling into user code that could cause re-entrancy that might violate invariants.  This was already needed for correctness anyways, which is nice.

There is a performance consideration here, which is that there is a cost to structured clones, so if we're not going to perform any key-path traversals, then it's preferable that we execute only a single structured clone for the value itself.  However, for security purposes, it's critical that there be no accumulated state on the stack that could be rendered invalid or a dangling pointer/reference from that structured clone itself.

### Entry-point auditing of potentially traversed values
Going from WebIDL here.

Stored values that can involve indices:
- IDBObjectStore.put/add:
  - GetAddInfo now takes a ValueWrapper and that's propagated up to AddOrPut.  GetAddInfo invokes the clones-at-most-once Clone() method if there's a primary key-path and if there are any indices.
- IDBCursor.update:
  - We clone if there's a key-path.  This is necessary to do rather than just letting GetAddInfo provide the coverage like in the above cases because we invoke KeyPath::ExtractKey on the provided value prior to calling into AddOrPut.  That said, this logic seems redundant since the AddorPut call case should perform the same equivalent call and return the same error code via GetAddInfo().  We might want a follow-up to eliminate the redundant path.

Other "any" instances that aren't keys: None, hooray!

Comment 20

[Jan Varga [:janv]]

A patch to fix this issue landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85380831fe4

Comment 21

[Pascal Chevrel:pascalc]

Andrew, do you intend to uplift your patch in bug 1404274 to beta?

Comment 22

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Pascal Chevrel:pascalc from comment #21)
> Andrew, do you intend to uplift your patch in bug 1404274 to beta?

:janv authored the fixes, I just reviewed them, but I think the answer is: yes, we should uplift it.  However, there was an oversight in the patch, thankfully caught by the ASAN build.  That is tracked and was fixed on bug 1478573 and we want to uplift the combination of those two patches.  :janv is out on PTO until Monday, so I'll put up the combination of the two on bug 1404274 for uplift approval momentarily.

Comment 23

[Andrew Sutherland [:asuth] (he/him)]

Okay, the uplift request has been made there.  I was a bit nebulous and handwave-y, so if you can see that it gets approved for uplift despite me not sufficiently justifying it in the bug, that would be great.  We definitely want the fix uplifted for security reasons.

Comment 24

[Jan Varga [:janv]]

Yes, we need to uplift it.
Andrew, thanks for driving the uplift while I was on PTO.

Comment 25

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

Marking fixed for 63 and 62 since bug 1404274 is fixed there. Are we going to fix this on ESR60 as well? As a sec-high, we ideally should.

Comment 26

[Jan Varga [:janv]]

(In reply to Al Billings [:abillings] from comment #25)
> Marking fixed for 63 and 62 since bug 1404274 is fixed there. Are we going
> to fix this on ESR60 as well? As a sec-high, we ideally should.

Yes, I requested an approval for that in bug 1404274.

Comment 27

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I successfully reproduced the issue on Firefox Nightly 61.0a1 (2018-05-04) under Ubuntu 16.04 (x64) using STR from Comment 0.

The issue is no longer reproducible on latest Firefox 63.0a1 (2018-08-09), Firefox Beta 62.0b15 and on Firefox ESR 60.1.1, downloaded from Threeherder (revision e31b504b403e). The tests were performed under Windows 10 (x64), macOS 10.12 and Ubuntu 16.04 (x64).