1501152 - (CVE-2019-11752) Use after free in KeyPath::ExtractKey

Status: VERIFIED FIXED

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

Description

[zhanjiasong45]

Attachment: FireFox_IndexedDB_UAF3.html

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))) { // aKey.AppendItem can execute js getter (AppendItem => EncodeJSVal =>  EncodeJSValInternal => JS_GetElement)
      NS_ASSERTION(aKey.IsUnset(), "Encoding error should unset");
      return NS_ERROR_DOM_INDEXEDDB_DATA_ERR;
    }
  }

  aKey.FinishArray();

  return NS_OK;
}

aKey.AppendItem can execute js getter, call deleteIndex in the getter will cause UAF.

Comment 1

[zhanjiasong45]

Attachment: ASan.txt

test with https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer,65.0a1 (2018-10-22) (64-bit)

Comment 2

[:Gijs (he/him)]

Jan/Andrew, can you look at this or redirect to the right person? Thank you!

Comment 3

[Jan Varga [:janv]]

We fixed this by cloning the object before calling ExtractKey. However, there may be still some corner cases.

Comment 4

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

So, restating my understanding, the issue is now that when we are encoding a value into the key (which has a custom encoding mechanism) we have special handling for JS Arrays.  At https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/dom/indexedDB/Key.cpp#265 we manually traverse the array using JS_GetElement.  This is impacted by the prototype chain because we don't do any "own" magic and we lose.  And because we didn't make deleteIndex() throw when there's an IDB operation on the stack, we have a use-after-free.

Xray wrappers thoughts on the subject are found in the comment above the Array special-case here:
https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/js/xpconnect/wrappers/XrayWrapper.cpp#536

It looks like this is the only dangerous call for encoding given that JS_GetArrayLength has already checked the internal class of the object and so it should not reach the dangerous GetProperty call at https://searchfox.org/mozilla-central/rev/fcfb479e6ff63aea017d063faa17877ff750b4e5/js/src/builtin/Array.cpp#142 but let's involve :sfink because maybe he wants to write a bunch of cool static analyses that can help C++ code ensure that it's not triggering content-controlled JS when it doesn't want to.

:sfink, for extra context, I suggest reading the comment at https://searchfox.org/mozilla-central/source/dom/indexedDB/Key.cpp#33 which describes how IndexedDB cleverly encodes the keys defined by the spec at https://www.w3.org/TR/IndexedDB/#key-construct.   We cleverly encode the keys into a binary representation so SQLite can just do its binary comparison thing without us needing to provide a custom collation function.  (Structured Clone can't do that for us... yet... ;)

Comment 5

[Steve Fink [:sfink] [:s:]]

Oops, I didn't mean to un-needinfo myself.

Comment 6

[Steve Fink [:sfink] [:s:]]

This really ought to be caught by the static rooting hazard analysis. It currently is not.

aValue is kept live across GC calls: it's live upon entry, live across both GC calls GetJSValFromKeyPathString() and AppendItem() during the first iteration of the loop, and then consumed at the top of the second iteration. It's not reported by the analysis because the analysis sees const Value& as Value*, and that's a pointer to a pointer encoded in a Value. Pointers to pointers don't get invalidated by GC. It *should* be reported as a warning in the caller, but it won't because of bug 1497593 (not that we're good about warnings.)

(In reply to Jan Varga [:janv] from comment #3)
> We fixed this by cloning the object before calling ExtractKey. However,
> there may be still some corner cases.

That would fix the problem where the getter removes the value or some property of it, but cloning by itself wouldn't solve the rooting problem. But I see that it's done with ValueWrapper, which contains mValue in a Rooted, so that fixes the rooting hazard.

This bug makes me think that perhaps bug 1497593 is not enough; that would only produce warnings in the caller that we would almost certainly ignore. (We periodically sweep through those looking for real problems, but rarely.) Conceptually, this passing of a const Value& ought to be analyzed as if we were passing a plain Value. We're only passing by reference because (1) alignment constraints make passing by value problematic, and (2) it is more efficient on 32-bit (though I'm not wholly convinced that's really true, but it makes life easier on the JIT so I won't argue.)

Then again, if I *did* make this change to the analysis, it would turn this case into a false positive. But it really does smell like a problem if you only look at it locally. What would be better, and would survive the intended analysis change, would be if ExtractKey took a Handle<JS::Value> instead of a const Value&. I think that should just work, since ValueWrapper hands out its Value as a Rooted<Value>&. Which, by the way, is kind of weird -- I would prefer that to be returning a Handle<Value> instead.

Comment 7

[Steve Fink [:sfink] [:s:]]

Filed bug 1504075 for treating const Value& as a GC pointer.

Comment 8

[Jan Varga [:janv]]

It seems, we can just replace:

if (!JS_GetElement(aCx, obj, index, &val)) {
  IDB_REPORT_INTERNAL_ERR();
  return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
}

with:

JS::RootedId id(aCx);
if (!JS_IndexToId(aCx, index, &id)) {
  IDB_REPORT_INTERNAL_ERR();
  return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
}

JS::Rooted<JS::PropertyDescriptor> desc(aCx);
if (!JS_GetOwnPropertyDescriptorById(aCx, obj, id, &desc)) {
  IDB_REPORT_INTERNAL_ERR();
  return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
}

if (!desc.object()) {
  return NS_ERROR_DOM_INDEXEDDB_DATA_ERR;
}

I'll submit a patch once I finish testing.

Comment 9

[Jan Varga [:janv]]

I have a patch and all tests pass except key-conversion-exceptions.htm, trying to fix that.

Comment 10

[Jan Varga [:janv]]

testing on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c29d5448a8a63b464e04680b0be7894cf59bc753

Comment 11

[Jan Varga [:janv]]

Attachment: patch for spidermonkey

Maybe there's a better way (more efficient), but this works good enough.

Comment 12

[Jan Varga [:janv]]

Attachment: patch for indexeddb

Andrew, I checked if JS_GetArrayLength() and I believe it's safe. This JS code:
arr1.__proto__.__defineGetter__("length", function() {
  objectStore.deleteIndex("index_name");
  return "ID_002";
});

doesn't work, because the "length" property is not configurable.

However, the patch is a bit more complicated because of testing/web-platform/tests/IndexedDB/key-conversion-exceptions.htm
I had to add the aCallGetters argument all over to make this test pass.

Comment 13

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

Comment on attachment 9023991 [details] [diff] [review]
patch for indexeddb

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

Looks great!  (And I'm glad you put the helper method in JS space so it can be used by all!)

Restating:
- The semantics for IDB and getters is that we want to evaluate a getter at most once when there's a getter involved *per argument*.  The spec is aware of the impact of this and so has specified that the structured clone algorithm be invoked in the cases where multiple traversals that could trigger.  For efficiency reasons, we somewhat complicate this so that add()/put()/update() do not need to perform a wasteful clone when out-of-line keys are used.
- Accordingly, the basic invariants we are trying to maintain are:
  - We never call getters when we're building the key from an already-evaluated value.  We require that the arrays are raw, actual arrays/whatever, not crazy getter things.
  - We do not call getters on a cloned value, AKA: (aCallGetters == !valueWrapper.mCloned)
  - We make at most one of the following invocations per value from content (which may mean multiples calls for multiple values in the case of IDBKeyRange, and both the value and the out-of-line key when out-of-line keys are in use): blah(...aCallGetters==true), valueWrapper.Clone().
- GetAddInfo is the interesting case because the state of ValueWrapper.mCloned can vary, and in the case where mCloned is not expected to be true at entry because we currently allow createIndex/deleteIndex to be invoked by re-entrant code and the evaluation of the out-of-line key and the value BOTH can trigger getters, whether mCloned will be invoked can change!

::: dom/indexedDB/IDBCursor.cpp
@@ +727,5 @@
>  
> +    aRv = keyPath.ExtractKey(aCx,
> +                             valueWrapper.Value(),
> +                             key,
> +                             /* aCallGetters */ false);

(This is right because this is the payload value and we cloned it above so we don't call getters because they've already been called.)

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1554,5 @@
>    nsresult rv;
>  
>    if (!HasValidKeyPath()) {
>      // Out-of-line keys must be passed in.
> +    rv = aKey.SetFromJSVal(aCx, aKeyVal, /* aCallGetters */ true);

(This is right because the out-of-line key should be evaluated with getters invoked because the out-of-line key is logically distinct from the payload value.)

@@ +1566,5 @@
>  
> +    rv = GetKeyPath().ExtractKey(aCx,
> +                                 aValueWrapper.Value(),
> +                                 aKey,
> +                                 /* aCallGetters */ false);

(This is right because this is the payload value and we just ensured we cloned it.)

@@ +1622,5 @@
>                                           aValueWrapper.Value(),
>                                           aKey,
>                                           &GetAddInfoCallback,
> +                                         &data,
> +                                         /* aCallGetters */ false);

(This is right because this is the payload value and we just ensured we cloned it.)

Comment 14

[Jan Varga [:janv]]

Waldo, can you take a look at the spidermonkey patch ?

Comment 15

[Jan Varga [:janv]]

Comment on attachment 9023990 [details] [diff] [review]
patch for spidermonkey

I was told Waldo is super busy.

Comment 16

[Jason Orendorff [:jorendorff]]

Comment on attachment 9023990 [details] [diff] [review]
patch for spidermonkey

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

This patch looks fine, as far as the code goes. But (being unfamiliar with IndexedDB) the API looks a bit like a wrong thing to want.

I'm out of the office Wednesday through Friday, so I'm forwarding this review to tcampbell; or maybe sfink will chime in.

::: js/src/jsapi.cpp
@@ +2107,5 @@
> +        return false;
> +    }
> +
> +    if (desc.object()) {
> +        vp.set(desc.value());

Style nit: Please write

    if (desc.object() && desc.isDataDescriptor()) {

instead of 

    if (desc.object()) {

Comment 17

[Ted Campbell [:tcampbell]]

Comment on attachment 9023990 [details] [diff] [review]
patch for spidermonkey

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

Unfortunately |GetOwnElement| is not a general purpose API. Depending on context one should prefer JS_GetElement/JS_GetProperty family or the GetOwnPropertyDescriptor family of APIs and avoid any sort of hybrid. I think for your case here you should continue to directly call GetOwnPropertyDescriptor and be explicit. Note that GetOwnPropertyDescriptor may still invoke arbitrary code if the object is a property.

If you explicitly check JS_IsArrayObject before calling GetOwnPropertyDescriptor* then you should be in safe territory. We can arrange a chat is at all-hands with some JS people to give some guidance on how IndexedDB code should in general interact with SpiderMonkey.

Comment 18

[Ted Campbell [:tcampbell]]

Comment on attachment 9023991 [details] [diff] [review]
patch for indexeddb

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1075,5 @@
>      }
>  
>      for (uint32_t arrayIndex = 0; arrayIndex < arrayLength; arrayIndex++) {
>        JS::Rooted<JS::Value> arrayItem(aCx);
> +      if (NS_WARN_IF(!JS_GetOwnElement(aCx, array, arrayIndex, &arrayItem))) {

Instead, you probably want a helper function that asserts array is JS_IsArrayObject, then use JS_GetOwnPropertyDescriptorById. The helper should probably have a comment explaining why undefined is a the appropriate result to use for a non-data PropertyDescriptor.

::: dom/indexedDB/Key.cpp
@@ +265,5 @@
>  
>        for (uint32_t index = 0; index < length; index++) {
>          JS::Rooted<JS::Value> val(aCx);
> +        bool ok = aCallGetters ? JS_GetElement(aCx, obj, index, &val)
> +                               : JS_GetOwnElement(aCx, obj, index, &val);

I wonder if |aCanCallGetters| is a better description of what this represents.

Comment 19

[Ted Campbell [:tcampbell]]

One part that isn't clear to me when I look at this code versus the spec is..

https://www.w3.org/TR/IndexedDB/#convert-a-value-to-a-key -> IsArray -> Step 5.1

The property must be checked by HasOwnProperty or return invalid so the prototype chain should never be involved.

Comment 20

[Jan Varga [:janv]]

(In reply to Ted Campbell [:tcampbell] from comment #19)
> One part that isn't clear to me when I look at this code versus the spec is..
> 
> https://www.w3.org/TR/IndexedDB/#convert-a-value-to-a-key -> IsArray -> Step
> 5.1
> 
> The property must be checked by HasOwnProperty or return invalid so the
> prototype chain should never be involved.

You mean that there shouldn't be aCanCallGetters and just always avoid searching the prototype chain ?
Well, there is a wpt test - testing/web-platform/tests/IndexedDB/key-conversion-exceptions.htm and that proofs that sometimes we need to search the prototype chain.
You may want to look at comment 12 and comment 13.
I don't know maybe the spec should be updated or the test fixed, but the patch should be safe because we don't allow calling getters for values logically distinct from the payload value.

Comment 21

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

Thanks for the eagle eyes on the spec, Ted!  I absolutely missed that, as was probably clear from my explicitly stating in vidyo that I thought we needed to have the same side-effect semantics as structured clone for key evaluation of out-of-line keys.

(In reply to Jan Varga [:janv] from comment #20)
> Well, there is a wpt test -
> testing/web-platform/tests/IndexedDB/key-conversion-exceptions.htm and that
> proofs that sometimes we need to search the prototype chain.

I think the test is wrong.  "Convert a value to a key" is very explicit.  That getter should never be invoked.  DataCloneError should be thrown.  I suggest we change the WPT test to be correct, citing the spec.

Comment 22

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

(In reply to Andrew Sutherland [:asuth] from comment #21)
> I think the test is wrong.  "Convert a value to a key" is very explicit. 
> That getter should never be invoked.  DataCloneError should be thrown.  I
> suggest we change the WPT test to be correct, citing the spec.

Note, I was wrong here.  After talking with Ted it turns out I missed that the test is defining a property on the test object, not the Object prototype.  So HasOwnProperty will pass, and the getter will be invoked and explode with its custom exception.  So the test is legit.  And then there are some other nuances that Ted is writing up for our benefit that we'll receive via email soon!  I just wanted to update the bug before this falls out of my brain, or in case other people reading the bug get misled.

Comment 23

[Ted Campbell [:tcampbell]]

Jan, I'm organizing a few notes to try and motivate some of the subtlety here. For this bug, I think having leaving jsapi.h alone is best. We should use JS_GetOwnPropertyDescriptorById in the code for the |aCanCallGetters| case, but please include |MOZ_ASSERT(JS_IsArrayObject(...))| check for documentation.

The JS teams experience with algorithms like https://www.w3.org/TR/IndexedDB/#convert-a-value-to-a-key is to go step by step tracking the spec as comments, and if there are optimizations to justify why they are valid. See https://searchfox.org/mozilla-central/rev/55895c49f55073d82d977cb74ec1d3a71ae4b25f/js/src/builtin/Stream.cpp#753 for an example of the detailed steps.

For this bug, I think we can focus on the sec issue by preventing the getter from being called in the specific case. I can help review a helper method (put in indexeddb) that uses JS_GetOwnPropertyDescriptorById to avoid calling getters and combines the HasOwnProperty and Get steps from the spec.

Comment 24

[Ted Campbell [:tcampbell]]

Something vaguely like so we can be explicit about not handling getters and about why HasOwnProperty itself isn't triggering code.

> MOZ_ASSERT(JS_IsArrayObject(input));
> 
> // 1. Let hop = ? HasOwnProperty(input, index)
> // NOTE: Array uses OrdinaryGetOwnProperty so we can get the descriptor
> // directly.
> Rooted<PropertyDescriptor> desc(cx);
> Rooted<jsid> id(cx, INT_TO_JSID(index));
> JS_GetPropertyDescriptorById(cx, input, id, &desc);
> 
> // 2. If hop is false, return invalid.
> if (!desc.object()) {
>   return invalid;
> }
> 
> // 3. Let entry be ? Get(input, index)
> if (!desc.isDataDescriptor()) {
>     // Has a getter
>     ???
>     // Note, if you wanted to evaluate the getter, you'd call
>     // JS_GetElement. This won't have any issues with the
>     // JS_GetPropertyDescriptById call above.
> }
> 
> // The array element
> Rooted<Value> elem(cx, desc.value());

Comment 25

[Andrew McCreight [:mccr8]]

Any change you can look at this again, Jan? Thanks.

Comment 26

[zhanjiasong45]

Is this bug fixed? I wonder when can I disclose it.

Comment 27

[Frederik Braun [:freddy]]

We'd appreciate if you hold off disclosure until the bug is fixed, as I just learned that a fix for this is imminent.
Thank you for reporting the bug and for your patience so far.

Comment 28

[Yaron Tausky [:ytausky]]

I'll take this over.

First thing, I'll try to refactor the code to be more similar to the spec's control flow and add corresponding excerpts (probably under a different bug number, since it shouldn't look too suspicious). Then I'll fix the wrong JS object accesses here, and finally, see if there's a way to avoid creating a cloned JS object while still maintaining the correctness of the whole thing. I suspect that by weaving key path traversal into the serialization code this would be possible, but let's only do that once correctness and safety are established.

Comment 29

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

FWIW, I did propose weaving key path traversal into the serialization, but it added a lot of complexity and potentially required performance-hindering alterations to the JS engine's structured clone logic.

(Specifically, if there was a mechanism for something to listen to the the serialization process, say using the visitor idiom, it could maintain an understanding of the current "path" and capture values at the right path points. Where capturing values could entail listening to further nested traversal. However, I believe I was erroneously assuming that the existing structured clone ops already provided the visibility we needed. I believe reality is that the hooks are not called during normal plain object serialization, and so would need to be made more generic. This was deemed undesirable for complexity reasons and because structured cloning is generally performance-sensitive.)

Comment 30

[Yaron Tausky [:ytausky]]

Yeah, let's fix it first and then check the performance implications.

Comment 31

[Daniel Veditz [:dveditz]]

This security vulnerability affects ESR-60 as well.

Comment 32

[Yaron Tausky [:ytausky]]

Small update: I'm still working on this; making control flow as close as possible to the spec without touching the lower-level implementation details is a little tricky here.

We would probably need to consider more hooks into the structured clone algorithm, though. It seems to be used as a basic building block for optimization-ripe spec operations. There are ways to mitigate potential performance problems for non-specialized use cases (e.g. have versions with and without hooks instantiated from a template), so it would probably boil down to the trade-off between implementation complexity and some DOM components' performance.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

This is going to miss the 67/60.7.0esr releases. If you think we may come up with a fix and it would drive a dot release, we can keep tracking for 67. Otherwise we should wontfix it and move the flags to 68.

Comment 34

[Yaron Tausky [:ytausky]]

I'm following tcampbell's recommendations concerning this bug, and they necessitate a bigger-ish refactoring of the surrounding code, including iteration on the design of several classes. I'm not sure it would make sense as a point release.

Comment 35

[Liz Henry (:lizzard) (relman/hg->git project)]

That makes sense. Thanks!

Comment 36

[Julien Cristau [:jcristau]]

Too late now for 68.

Comment 37

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Tom is helping here while Yaron is on leave. Thanks, Tom!

Comment 38

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1501152 - Have a function to get elements from JS array;

Comment 39

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

To clarify, I was under the impression that Yaron's work on bug 1544750 was basically ready to land, modulo the one line: "Blockers: The missing SupressExceptions and calling aRv.Throw without checking it isn't already having an Exception." in https://phabricator.services.mozilla.com/D28976#1020675 with the rest able to be follow-ups. Is there a reason not to land that?

(I realize there may have been some confusion since we didn't explicitly establish a "see also" relationship between bug 1544750 and this one because it would make it clear that bug 1544750 is a security fix pretending to be a cleanup.)

Comment 40

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #39)

To clarify, I was under the impression that Yaron's work on bug 1544750 was basically ready to land, modulo the one line: "Blockers: The missing SupressExceptions and calling aRv.Throw without checking it isn't already having an Exception." in https://phabricator.services.mozilla.com/D28976#1020675 with the rest able to be follow-ups. Is there a reason not to land that?

(I realize there may have been some confusion since we didn't explicitly establish a "see also" relationship between bug 1544750 and this one because it would make it clear that bug 1544750 is a security fix pretending to be a cleanup.)

Ah, I somehow couldn't connect these two tickets with each other. I'll rebase and try to address the patch in bug 1544750.

Comment 41

[Tom Tung [:tt, :ttung]]

Hi Daniel,

The fix for this issue has been landed by pretending itself to be a cleanup in bug 1544750. Since this is not the general procedure for security issue (at least I couldn't find a document for this), I have a few questions for the status of this bug. Would you mind giving me some feedback? Thanks!

My questions here are:

• Can we close this bug now? And would it be safe if we set any dependency between this and bug 1544750 now? (e.g. duplicate or see also)
• Should we uplift the patches in bug 1544750 to Beta since this is a sec-high issue from your perspective? (I wouldn't recommend doing that since the patches also refactor surrounding code)

Comment 42

[Daniel Veditz [:dveditz]]

One problem with with hiding security fixes in unrelated bugs is that it completely bypasses the sec-approval process which would have answered some of those questions, as well as explored potential solutions to problems such as "if we can't uplift this to Beta, how the heck are we going to secure ESR-68 for the next year it's supported?" This process is linked to in the warning text "sec-approval required on patches before landing" in the attachments area of this bug.

Can we close this bug now? And would it be safe if we set any dependency between this and bug 1544750 now? (e.g. duplicate or see also)

We need to mark this bug FIXED so it triggers the proper advisory and bug bounty treatment. Linking them now is OK.

So now what? If we wait until Firefox 70 ships that's late October which is pretty long for a sec-high fix to sit in the tree. We've done that before but this patch looks fairly straightforward, if large, so maybe it's safe to backport? We definitely need to backport to ESR-68 branch anyway.

We're also still supporting ESR-60. How complex would it be to backport to that? I can't imagine IndexedDB has changed much.

Comment 43

[Ted Campbell [:tcampbell]]

Tom, are we sure that Bug 1544750 covered all the cases for this bug? For example, [1] still can run arbitrary JS which is what the spec allows. This fixes the prototype stuff (and we now better follow spec), but is the re-entrancy issue addressed in other ways?

[1] https://hg.mozilla.org/integration/autoland/rev/b14b8efda186#l8.169

Comment 44

[Tom Tung [:tt, :ttung]]

(In reply to Daniel Veditz [:dveditz] from comment #42)

One problem with with hiding security fixes in unrelated bugs is that it completely bypasses the sec-approval process which would have answered some of those questions, as well as explored potential solutions to problems such as "if we can't uplift this to Beta, how the heck are we going to secure ESR-68 for the next year it's supported?" This process is linked to in the warning text "sec-approval required on patches before landing" in the attachments area of this bug.

I see. Thanks for the explanation! I will try to ask the sec-approval in the security bug for the patches in the unrelated bugs if I have similar condition next time. So that, we can also cover this problem.

So now what? If we wait until Firefox 70 ships that's late October which is pretty long for a sec-high fix to sit in the tree. We've done that before but this patch looks fairly straightforward, if large, so maybe it's safe to backport? We definitely need to backport to ESR-68 branch anyway.

It makes sense to backport it to ESR-68, but it's better to observe the status for the patch on the Nightly for a while (a week). If the patch is stable (there is no new issues related to that for a week), then I will check and try to uplift the patch to ESR-68.

We're also still supporting ESR-60. How complex would it be to backport to that? I can't imagine IndexedDB has changed much.

I rebased both part1 and part2 on Bug 1544750. There are some conflicts on dom/indexedDB/ActorsParent.cpp and dom/indexedDB/Key.cpp. I'll take a closer look (checking the changes on IDBKey after ESR60 til now since patches on Bug 1544750 refactor the code there) next week when I rebase the patches to ESR68.
Also, we still need to check the performance impact after applying the patches on Bug 1544750.

Keep the ni flag to remind me of checking the complexity of backporting to 60 next week.

Comment 45

[Tom Tung [:tt, :ttung]]

(In reply to Ted Campbell [:tcampbell] from comment #43)

Tom, are we sure that Bug 1544750 covered all the cases for this bug? For example, [1] still can run arbitrary JS which is what the spec allows. This fixes the prototype stuff (and we now better follow spec), but is the re-entrancy issue addressed in other ways?

[1] https://hg.mozilla.org/integration/autoland/rev/b14b8efda186#l8.169

Ah, sorry for the confusion. I was thinking this bug is mainly for the prototype stuff and the rest of the works should be taken care of in other bugs. And I was wrong. I'll ensure that we have taken all the cases before closing this security bug. Thanks for the notice!

Comment 46

[Yaron Tausky [:ytausky]]

In the strictest sense, the re-entrancy issue is already addressed by cloning the value, which strips getters. By avoiding the prototype chain, I believe the security hole is plugged. The PoC posted on this issue doesn't produce an ASan message for me any more.

Comment 47

[Jan Varga [:janv]]

I think there are still some theoretical edge cases in spidermonkey, I don't remember exactly, maybe when some wrappers or proxies are involved, so long term it would be safer to fix bug 1570585.

Comment 48

[Tom Tung [:tt, :ttung]]

Quick update: the plan for uplift is that if I/Yaron can implement a quick & simple fix for bug 1570585 (I have a naive plan for fixing that in mind, but it need to be verified), then maybe it's better to uplift the patch there to Beta or ESR since I expect the patch there would be smaller and should also fix the issue here. If we cannot make it, I would suggest just uplifting patches here.

Comment 49

[Yaron Tausky [:ytausky]]

Jan (and Ted, privately) are correct, I forgot about the possibility of using proxies. So this is not fixed yet.

Comment 50

[Yaron Tausky [:ytausky]]

Back-flipping yet again, step 23 of StructuredSerializeInternal makes clones of objects with proxies impossible, so I once again think this is actually fixed in m-c. I think this highlights the need for a systematic approach to dealing with bug 1570585.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

We have one beta left before 69 goes to RC next week. If we plan to do something for Fx69/68.1esr, that needs to be sorted out ASAP.

Comment 52

[Yaron Tausky [:ytausky]]

I'm compiling the patches from bug 1544750 for beta, when that's done I'll do it for the ESR as well.

Comment 53

[Yaron Tausky [:ytausky]]

Attachment: Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell

This commit adds the text of the spec as inline comments and refactors
the code such that it directly corresponds to the spec's steps. This
makes it easier to understand how the spec's algorithm is implemented.

Comment 54

[Yaron Tausky [:ytausky]]

Attachment: Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell

This commit adds the text of the spec as inline comments and refactors
the code such that it directly corresponds to the spec's steps. This
makes it easier to understand how the spec's algorithm is implemented.

Comment 55

[Yaron Tausky [:ytausky]]

Comment on attachment 9087003 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell

Beta/Release Uplift Approval Request

• User impact if declined: Potentially exploitable use-after-free
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's been on nightly and we haven't seen regressions.
• String changes made/needed: None

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Easy to trigger UAF, not sure how easy it is 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?: esr68
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: It's possible that it introduces regressions since it's a new implementation of existing functionality, but it's been on nightly and we haven't seen any regressions.

Comment 56

[Yaron Tausky [:ytausky]]

Comment on attachment 9087298 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Potentially exploitable UAF
• Fix Landed on Version: 70
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's been tested on nightly and we haven't seen regressions.
• String or UUID changes made by this patch: None

Comment 57

[Ryan VanderMeulen [:RyanVM]]

Can you clarify the status for esr60? The sec-approval says this only goes back as far as esr68, but comment 31 says esr60 is also affected. Also, updating the flags to reflect that this is fixed in 70 already.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9087003 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell

This has baked on Nightly for awhile now, let's take it for 69.0b16 also.

Comment 59

[Ryan VanderMeulen [:RyanVM]]

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

Comment 60

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

Comment on attachment 9087003 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell

Giving sec-approval+ since it is already in all the things. :-)

Comment 61

[Yaron Tausky [:ytausky]]

I forgot about esr60, I'll backport it now.
Update: it does not apply cleanly, so this may require more work and testing.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9087003 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell

This isn't obsolete just because it was uplifted already.

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9087298 [details]
Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell

Approved for 68.1esr.

Comment 64

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

https://hg.mozilla.org/releases/mozilla-esr68/rev/ebb385dc5c27434ca6e1b951fa57492bdc1e1d62

Comment 65

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

Backed out for from ESR68 bustage in IDBResult.h:

https://hg.mozilla.org/releases/mozilla-esr68/rev/9fd0d4493c77bfed937299c2b7fd3843f8543a3a

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-esr68&revision=091473bf2727d37a4107cfca1185ef99c3f2eeff
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=263243122&repo=mozilla-esr68

[task 2019-08-23T20:05:13.369Z] 20:05:13 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/IDBCursor.h:14:
[task 2019-08-23T20:05:13.369Z] 20:05:13 INFO - In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/indexedDB/Key.h:10:
[task 2019-08-23T20:05:13.369Z] 20:05:13 ERROR - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/indexedDB/IDBResult.h:164:35: error: bad implicit conversion constructor for 'IDBResult'
[task 2019-08-23T20:05:13.369Z] 20:05:13 INFO - using IDBResult::IDBResultBase::IDBResultBase;
[task 2019-08-23T20:05:13.369Z] 20:05:13 INFO - ^
[task 2019-08-23T20:05:13.370Z] 20:05:13 INFO - /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/indexedDB/IDBResult.h:164:35: note: consider adding the explicit keyword to the constructor
[task 2019-08-23T20:05:13.370Z] 20:05:13 INFO - using IDBResult::IDBResultBase::IDBResultBase;
[task 2019-08-23T20:05:13.370Z] 20:05:13 INFO - ^
[task 2019-08-23T20:05:13.370Z] 20:05:13 INFO - explicit

Comment 66

[Yaron Tausky [:ytausky]]

Right, that patch triggered a bug in the static analysis, which was fixed in bug 1554989. I'll have to uplift that one as well.

Comment 67

[Yaron Tausky [:ytausky]]

Attachment: Bug 1554989 - Fix implicit checker on inheriting constructors r=andi

Inheriting constructors are implicitly introduced via a using-declaration.
Since the C++ grammar doesn't allow attributes on using-declarations, it
is currently impossible to add MOZ_IMPLICIT to implicit inheriting
constructors.

This commit changes the AST matcher such that it ignores inheriting
constructors altogether. If they are inheriting from an implicit inherited
constructor, that constructor's check should be enough to ensure that no
constructors are unintentionally implicit.

Bug 1544750 - Part 2: Refactor Key::EncodeJSValInternal to show direct correspondence to spec r=asuth,tcampbell a=RyanVM

This commit adds the text of the spec as inline comments and refactors
the code such that it directly corresponds to the spec's steps. This
makes it easier to understand how the spec's algorithm is implemented.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/80175938fbd8

Comment 69

[Ryan VanderMeulen [:RyanVM]]

Hi Yaron, any updates on the ESR60 patch?

Comment 70

[Yaron Tausky [:ytausky]]

I'm slowed down with it because my local build of esr60 fails and there are some merge conflicts that need to be solved and tested.

Comment 71

[Cristian Baica [:cbaica], Release Desktop QA]

I've used Fx65.0a1 (2018-10-22) asan build and tried loading the test-case to reproduce the issue, but the tabs keep crashing so I cannot run the test-case.
Is there anything that can be done by QA to verify this?

Comment 72

[Yaron Tausky [:ytausky]]

You could verify that there are no crashes with the newest builds for ESR68 and Beta; all other versions are known to be affected (I'm still working on ESR60).

Comment 73

[Cristian Baica [:cbaica], Release Desktop QA]

Verified with the fixed Fx70.0a1, Fx69.0b16 and ESR68 builds. The tabs no longer crash when loading the test-case. Waiting for the fix on ESR60.

Comment 74

[Julien Cristau [:jcristau]]

Attachment: patch for esr60

Andrew, can you sanity-check this?

There were some conflicts in Key.cpp around bug 1499854, and a dependency on bug 1429613 for IDBResult (the latter fixed by janv with https://paste.mozilla.org/1CY8myH6). This applies on top of https://hg.mozilla.org/mozilla-central/rev/e8c59393c809 (which grafts cleanly to esr60).

Comment 75

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

Comment on attachment 9089466 [details] [diff] [review]
patch for esr60

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

So, it turns out I also can't actually build esr60 on my machine, but I can certify this as sane and resembling the code I previously reviewed.  (It seems like rustc 1.36.0 is probably too recent, but I'm very wary of messing with any default build toolchains right now because it might invalidate some of my other-secbug rr traces.)

Comment 76

[Jan Varga [:janv]]

Attachment: Matcher fix

Sorry, the pastebin lasted just one hour.

Comment 77

[Jan Varga [:janv]]

I have rustc 1.36.0 too and I can build it. However, one need to do this before configure/build:
export RUSTFLAGS="--cap-lints warn"
to workaround bug 1519629, this was all discovered by Julien (thanks for the investigation).

Comment 78

[Julien Cristau [:jcristau]]

Comment on attachment 9089466 [details] [diff] [review]
patch for esr60

taking this for 60.9.

Comment 79

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/2ce17180fd4dd8828caa964ba55205a51c313355
https://hg.mozilla.org/releases/mozilla-esr60/rev/d8dd10ac54302e4bd9cf6c537f790933b149099d

Comment 80

[Cristian Baica [:cbaica], Release Desktop QA]

Confirming the tabs no longer crash on Fx 60.9.0ESR buildID: 20190901094603. Marking the entire ticket as verified fixed.