Closed Bug 1459383 (CVE-2018-12378) Opened 2 years ago Closed 2 years ago

Use After Free in indexedDB

Categories

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

59 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 - wontfix
firefox-esr60 62+ verified
firefox60 --- wontfix
firefox61 - wontfix
firefox62 + verified
firefox63 + verified

People

(Reporter: zhanjiasong45, Assigned: janv)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [fixed on trunk in bug 1404274][post-critsmash-triage][adv-main62+][adv-esr60.2+])

Attachments

(3 files)

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.
Attached file ASAN ouput.txt
Attached file debug output.txt
Flags: sec-bounty?
Jan, could you take a look at this, please? Thanks.
Group: core-security → dom-core-security
Flags: needinfo?(jvarga)
Priority: -- → P1
Flags: needinfo?(jvarga)
This is a sec-critical, needs an owner.

And does KeyPath::ExtractKeyAsJSVal have similar issue?
Flags: needinfo?(jvarga)
Flags: needinfo?(bugmail)
Ok, I'll take a look.
Flags: needinfo?(jvarga)
Thanks, assigning to you then, so that this doesn't show up as unassigned bug in the bugzilla queries.
Assignee: nobody → jvarga
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(jvarga)
(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.
Need an update.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Per Jan, this isn't going to be fixed in time for 61.
Jan please give an update.
Will do on Monday.
(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.
Flags: needinfo?(jvarga)
I think the best solution would be to do a clone before extracting keys, see bug 1404274.
I'm going to investigated this more ...
Yes, cloning fixes this bug, I have a patch, but I need to test it a bit more.
A submitted a patch for this in bug 1404274.
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!
A patch to fix this issue landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85380831fe4
Keywords: sec-criticalsec-high
Whiteboard: [fixed on trunk in bug 1404274]
Andrew, do you intend to uplift your patch in bug 1404274 to beta?
Flags: needinfo?(bugmail)
(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.
Flags: needinfo?(bugmail)
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.
Flags: needinfo?(pascalc)
Yes, we need to uplift it.
Andrew, thanks for driving the uplift while I was on PTO.
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.
Flags: needinfo?(pascalc)
(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.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Group: dom-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [fixed on trunk in bug 1404274] → [fixed on trunk in bug 1404274][post-critsmash-triage]
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [fixed on trunk in bug 1404274][post-critsmash-triage] → [fixed on trunk in bug 1404274][post-critsmash-triage][adv-main62+][adv-esr60.2+]
Depends on: 1478573
Flags: sec-bounty? → sec-bounty+
Alias: CVE-2018-12378
See Also: → 1489020
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.