zhanjiasong45 Reporter
	Description • 6 years ago

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.

	Andrew McCreight [:mccr8]
	Comment 3 • 6 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 6 years ago

This is a sec-critical, needs an owner.

And does KeyPath::ExtractKeyAsJSVal have similar issue?

	Jan Varga [:janv] Assignee
	Comment 5 • 6 years ago

Ok, I'll take a look.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 6 years ago

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

	Daniel Veditz [:dveditz]
	Comment 7 • 6 years ago

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.

	Jan Varga [:janv] Assignee
	Comment 8 • 6 years ago

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

	Marion Daly [:mdaly]
	Comment 9 • 6 years ago

Need an update.

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 6 years ago

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

	Marion Daly [:mdaly]
	Comment 11 • 6 years ago

Jan please give an update.

	Jan Varga [:janv] Assignee
	Comment 12 • 6 years ago

Will do on Monday.

	Jan Varga [:janv] Assignee
	Comment 15 • 6 years ago

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

	Jan Varga [:janv] Assignee
	Comment 16 • 6 years ago

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

	Jan Varga [:janv] Assignee
	Comment 17 • 6 years ago

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

	Jan Varga [:janv] Assignee
	Comment 18 • 6 years ago

A submitted a patch for this in bug 1404274.

	Andrew Sutherland [:asuth] (he/him)
	Comment 19 • 6 years ago

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!

	Jan Varga [:janv] Assignee
	Comment 20 • 6 years ago

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

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 21 • 6 years ago

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

	Andrew Sutherland [:asuth] (he/him)
	Comment 22 • 6 years ago

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

	Andrew Sutherland [:asuth] (he/him)
	Comment 23 • 6 years ago

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.

	Jan Varga [:janv] Assignee
	Comment 24 • 6 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 25 • 6 years ago

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.

	Jan Varga [:janv] Assignee
	Comment 26 • 6 years ago

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

	Catalin Sasca, Desktop QA [:csasca]
	Comment 27 • 6 years ago

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