Bugzilla

Updated

•

6 years ago

Group: core-security → dom-core-security

Keywords: csectype-uaf, sec-high, testcase

David Bolter [:davidb] (NeedInfo me for attention)

Comment 2

•

6 years ago

David, can you help us assign this?

Flags: needinfo?(dbolter)

Comment 3

•

6 years ago

Sorry I missed this bug somehow. Thanks for fuzzing!

Alex can you take a look?

Flags: needinfo?(dbolter) → needinfo?(surkov.alexander)

Comment 4

•

6 years ago

it seems funfuzz installation process have changed over last weeks/months, and I cannot install a funfuzz.xpi addon I have on Nightly 55 anymore. Funfuzz repo [1] doesn't seem containing the extension any longer. Could you please update me on steps how to install the extension on my machine?

Flags: needinfo?(surkov.alexander) → needinfo?(nils)

Nils

Reporter

Comment 5

•

6 years ago

This could be due to the disabling of legacy addons. try with:

user_pref("extensions.allow-non-mpc-extensions", true);

Flags: needinfo?(nils)

Comment 6

•

6 years ago

(In reply to Nils from comment #5)
> This could be due to the disabling of legacy addons. try with:
> 
> user_pref("extensions.allow-non-mpc-extensions", true);

this did a trick. cannot reproduce, might be correlating with bug 1369836.

Comment 7

•

6 years ago

note, there's one more ARIA related fix in bug 1372985, which may correlate here. Note, I cannot reproduce on bare trunk btw.

Comment 8

•

6 years ago

(In reply to alexander :surkov from comment #6)
> (In reply to Nils from comment #5)
> > This could be due to the disabling of legacy addons. try with:
> > 
> > user_pref("extensions.allow-non-mpc-extensions", true);
> 
> this did a trick. cannot reproduce, might be correlating with bug 1369836.

Nils, can you retest
It could be that your bug was also found by one of our own fuzzers and we fixed this in another bug.

Flags: needinfo?(nils)

Updated

•

6 years ago

Flags: sec-bounty?

Nils

Reporter

Comment 9

•

6 years ago

Frederik, this still reproduces on a very recent build (BuildID=20170711160010)

Flags: needinfo?(nils) → needinfo?(fbraun)

Comment 10

•

6 years ago

I can confirm this still reproduces.
Alexander, here some STR:

1) allow unsigned add-ons, i.e., set xpinstall.signatures.required to false
2) install https://www.squarefree.com/extensions/domFuzzLite3.xpi 
3) visit the test HTML file

This crash here has proper debug symbols and good links to the relevant source lines,
<https://crash-stats.mozilla.com/report/index/ef1ba3ab-ee7d-4857-8db8-1351f0170712> (just like the ASAN output in attachment 8866330 [details]).

Back to Alex..

Flags: needinfo?(fbraun) → needinfo?(surkov.alexander)

Tyson Smith [:tsmith]

Comment 11

•

6 years ago

On Linux the add-on is not required. Launching with GNOME_ACCESSIBILITY=1 will also do what you need.

Comment 12

•

6 years ago

It doesn't crash with me on nightly debug build on os x with voice over running and commented out fuzzPriv.enableAccessibility(); line.

Debug build not ASAN one for sure, but if the problem here is in bad indices, then InsertElementAt should catch that by [1].

Btw, line 2 from comment #10 says me that the plugin is not compatible with Nightly 56a01. Any missed step there?

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsTArray.h?q=nsTArray&redirect_type=direct#2118

Flags: needinfo?(surkov.alexander)

Comment 13

•

6 years ago

Eitan, are you able to reproduce it? Is any correlation possible between this stack and your work in bug 1378257?

Assignee

Comment 14

•

6 years ago

I'm in this code right now, so I will look at it next.

Flags: needinfo?(eitan)

Comment 15

•

6 years ago

Cannot install the fuzz extension:
"DOM Fuzz Helper could not be installed because it is incompatible with Nightly 56.0a1"
Frederik, do you have an update on it?

Flags: needinfo?(fbraun)

Comment 16

•

6 years ago

I really hope this back-and-forth is coming to an end, this is getting ridiculous.
This reproduced very predictably 7 days ago. Please follow the steps from comment 10.
If that does not work, use a Nightly from 7 days ago.


(In reply to Frederik Braun [:freddyb] from comment #10)
> I can confirm this still reproduces.
> Alexander, here some STR:
> 
> 1) allow unsigned add-ons, i.e., set xpinstall.signatures.required to false
> 2) install https://www.squarefree.com/extensions/domFuzzLite3.xpi 
> 3) visit the test HTML file
> 
> This crash here has proper debug symbols and good links to the relevant
> source lines,
> <https://crash-stats.mozilla.com/report/index/ef1ba3ab-ee7d-4857-8db8-
> 1351f0170712> (just like the ASAN output in attachment 8866330 [details]).
> 
> Back to Alex..

Flags: needinfo?(fbraun)

Comment 17

•

6 years ago

For the record, this is on linux. That seems pretty important here.

Assignee

Updated

•

6 years ago

Assignee: nobody → eitan

Flags: needinfo?(eitan)

Assignee

Comment 18

•

6 years ago

What is happening is that DoARIAOwnsRelocation gets an array for the container from mARIAOwnsHash[1] then calls MoveChild[2] that will delete the previous's container array if it is empty[3]. After MoveChild returns, DoARIAOwnsRelocation inserts an element into the new container's array[4].

If the old container, and the new container are identical, the array in DoARIAOwnsRelocation is deleted, and it tries to insert into a deleted array.

1. https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/accessible/generic/DocAccessible.cpp#2073
2. https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/accessible/generic/DocAccessible.cpp#2135
3. https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/accessible/generic/DocAccessible.cpp#2224
4. https://dxr.mozilla.org/mozilla-central/rev/7d2e89fb92331d7e4296391213c1e63db628e046/accessible/generic/DocAccessible.cpp#2137

Assignee

Comment 19

•

6 years ago

The reason this takes a while to crash, after an indeterminate amount of scripted reloads, is because there is a race between two containers doing DocAccessible::DoARIAOwnsRelocation, an SVG filter and an iframe.

as for the SVG filter, DoARIAOwnsRelocation is scheduled four times:
1. When the container is bound to the document with aria-owns="id2 id4"
2. When div#id2 node is walked (the accessible is not created because it is referenced in aria-owns in another node, instead creation is scheduled via DoARIAOwnsRelocation).
3. When feDisplacementMap#id4 is walked (same case as (2)).
4. When input#id2 is walked (same case as (2)).

(Usually the relocation would only be scheduled 3 times in such a case, but because there is an erroneous use of the same id in the document, it is scheduled 4 times. This is not a problem, but it increases the odds of this race)

If the iframe's DoARIAOwnsRelocation is called between any of SVG filter's relocations, we get into a bad state. In part, this is because an iframe (OuterDocAccessible) does not accept relocated children.

There are three things I think need to be fixed (any one of these will avoid the crash, but we should do all 3).

1. A container should not be allowed to steal a child via aria-owns if it is already aria-owned by another container.
2. MoveChild should fail in the case of the new parent not accepting the child before it resets the relocated state of the child.
3. We should be checking that we are not using a dead array from mARIAOwnsHash after MoveChild.

Also, a side effect of bug 1378257 will be that this UAF will go away, I think. But I don't think that is the appropriate fix.

Assignee

Comment 20

•

6 years ago

Attached file Simplified testcase — Details

Here is a simplified case that will crash your browser much more efficiently.

Assignee

Comment 21

•

6 years ago

Attached patch Prevent aria-owned nodes from getting into bad state. r?surkov (obsolete) — Details — Splinter Review

Attachment #8889642 - Flags: review?(surkov.alexander)

Updated

•

6 years ago

Priority: -- → P1

Comment 22

•

6 years ago

Comment on attachment 8889642 [details] [diff] [review]
Prevent aria-owned nodes from getting into bad state. r?surkov

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

::: accessible/generic/DocAccessible.cpp
@@ -2132,5 @@
>        }
>      }
>  
>      if (MoveChild(child, aOwner, insertIdx)) {
> -      MOZ_ASSERT(mARIAOwnsHash.Get(aOwner) == owned, "We still have our array");

is this against trunk?

Assignee

Comment 23

•

6 years ago

Attached patch Prevent aria-owned nodes from getting into bad state. r?surkov (obsolete) — Details — Splinter Review

Oops. Yeah, that wasn't against trunk.

Attachment #8889923 - Flags: review?(surkov.alexander)

Assignee

Updated

•

6 years ago

Attachment #8889642 - Attachment is obsolete: true

Attachment #8889642 - Flags: review?(surkov.alexander)

Comment 24

•

6 years ago

Comment on attachment 8889923 [details] [diff] [review]
Prevent aria-owned nodes from getting into bad state. r?surkov

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

::: accessible/generic/DocAccessible.cpp
@@ +2124,5 @@
>      if (child->Parent() != aOwner) {
> +      // Child is aria-owned by another container, skip.
> +      if (child->IsRelocated()) {
> +        continue;
> +      }

if you moved this condition few lines above, then we could get rid of owned->IndexOf(child) < idx check

@@ +2138,5 @@
>      }
>  
>      if (MoveChild(child, aOwner, insertIdx)) {
> +      nsTArray<RefPtr<Accessible> >* relocated = mARIAOwnsHash.LookupOrAdd(aOwner);
> +      MOZ_ASSERT(relocated == owned);

my impression was that this indeed may happen, if so, then why do you assert?

@@ +2204,3 @@
>    }
>  
>    aChildren->RemoveElementsAt(aStartIdx, aChildren->Length() - aStartIdx);

aChildren may be dead after unsuccessful MoveChild, correct?
also how do we want to proceed with rejected children, shutdown them?

@@ +2224,5 @@
>  #endif
>  
> +  if (!aNewParent->IsAcceptableChild(aChild->GetContent())) {
> +    return false;
> +  }

it makes sense to move it before TreeInfo logging stuff

Assignee

Comment 25

•

6 years ago

(In reply to alexander :surkov from comment #24)
> Comment on attachment 8889923 [details] [diff] [review]
> Prevent aria-owned nodes from getting into bad state. r?surkov
> 
> Review of attachment 8889923 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +2124,5 @@
> >      if (child->Parent() != aOwner) {
> > +      // Child is aria-owned by another container, skip.
> > +      if (child->IsRelocated()) {
> > +        continue;
> > +      }
> 
> if you moved this condition few lines above, then we could get rid of
> owned->IndexOf(child) < idx check


I don't see how that would work. We still accept relocated children if they are moved within this container.


> 
> @@ +2138,5 @@
> >      }
> >  
> >      if (MoveChild(child, aOwner, insertIdx)) {
> > +      nsTArray<RefPtr<Accessible> >* relocated = mARIAOwnsHash.LookupOrAdd(aOwner);
> > +      MOZ_ASSERT(relocated == owned);
> 
> my impression was that this indeed may happen, if so, then why do you assert?

I can't think of any legitimate case where a single relocated child should be removed and added again to the array. Instead of UAFing, we should have an assertion failure.

> 
> @@ +2204,3 @@
> >    }
> >  
> >    aChildren->RemoveElementsAt(aStartIdx, aChildren->Length() - aStartIdx);
> 
> aChildren may be dead after unsuccessful MoveChild, correct?
> also how do we want to proceed with rejected children, shutdown them?

I don't think that is possible. MoveChild is being called on children that are explicitly set to not-relocated, so they are not removed from the mARIAOwnsHash.

> 
> @@ +2224,5 @@
> >  #endif
> >  
> > +  if (!aNewParent->IsAcceptableChild(aChild->GetContent())) {
> > +    return false;
> > +  }
> 
> it makes sense to move it before TreeInfo logging stuff

Done.

Comment 26

•

6 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #25)

> > if you moved this condition few lines above, then we could get rid of
> > owned->IndexOf(child) < idx check
> 
> 
> I don't see how that would work. We still accept relocated children if they
> are moved within this container.

you're right indeed, we don't clear the flag if the children are rearranged by aria-owns change.

> > >      if (MoveChild(child, aOwner, insertIdx)) {
> > > +      nsTArray<RefPtr<Accessible> >* relocated = mARIAOwnsHash.LookupOrAdd(aOwner);
> > > +      MOZ_ASSERT(relocated == owned);
> > 
> > my impression was that this indeed may happen, if so, then why do you assert?
> 
> I can't think of any legitimate case where a single relocated child should
> be removed and added again to the array. Instead of UAFing, we should have
> an assertion failure.

> > >    aChildren->RemoveElementsAt(aStartIdx, aChildren->Length() - aStartIdx);
> > 
> > aChildren may be dead after unsuccessful MoveChild, correct?
> > also how do we want to proceed with rejected children, shutdown them?
> 
> I don't think that is possible. MoveChild is being called on children that
> are explicitly set to not-relocated, so they are not removed from the
> mARIAOwnsHash.

fair enough, let's go with assertions

Comment 27

•

6 years ago

Comment on attachment 8889923 [details] [diff] [review]
Prevent aria-owned nodes from getting into bad state. r?surkov

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

::: accessible/generic/DocAccessible.cpp
@@ +2197,5 @@
>          }
>        }
>      }
> +
> +    if (!MoveChild(child, origContainer, idxInParent)) {

do DebugOnly<bool> please

Attachment #8889923 - Flags: review?(surkov.alexander) → review+

Assignee

Comment 28

•

6 years ago

Attached patch Prevent aria-owned nodes from getting into bad state. (obsolete) — Details — Splinter Review

Assignee

Updated

•

6 years ago

Attachment #8889923 - Attachment is obsolete: true

Comment 29

•

6 years ago

Comment on attachment 8890563 [details] [diff] [review]
Prevent aria-owned nodes from getting into bad state.

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

::: accessible/generic/DocAccessible.cpp
@@ +2163,2 @@
>        child->SetRelocated(true);
> +      relocated->InsertElementAt(idx, child);

btw, if we are confident than 'relocated' always equals 'owned' then we don't need 'relocated' variable, and then
MOZ_ASSERT(owned == mARIAOwnsHash.Get(aOwner), "MoveChild killed ARIA owned array")

Assignee

Comment 30

•

6 years ago

(In reply to alexander :surkov from comment #29)
> Comment on attachment 8890563 [details] [diff] [review]
> Prevent aria-owned nodes from getting into bad state.
> 
> Review of attachment 8890563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +2163,2 @@
> >        child->SetRelocated(true);
> > +      relocated->InsertElementAt(idx, child);
> 
> btw, if we are confident than 'relocated' always equals 'owned' then we
> don't need 'relocated' variable, and then
> MOZ_ASSERT(owned == mARIAOwnsHash.Get(aOwner), "MoveChild killed ARIA owned
> array")

Oh crap, yeah thats not a good change then.

The conditional in the previous patch made sure we wouldn't ever get to a UAF. Should I change it back?

Comment 31

•

6 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #30)

> > btw, if we are confident than 'relocated' always equals 'owned' then we
> > don't need 'relocated' variable, and then
> > MOZ_ASSERT(owned == mARIAOwnsHash.Get(aOwner), "MoveChild killed ARIA owned
> > array")
> 
> Oh crap, yeah thats not a good change then.
> 
> The conditional in the previous patch made sure we wouldn't ever get to a
> UAF. Should I change it back?

yeah (I'm just not sure what UAF is :) ) I'd love to have assertions though, in case if MoveChild changes ARIA owned array. It makes me really worry.

Assignee

Comment 32

•

6 years ago

use after free. its my new security lingo i adapted in the last 2 weeks :)

Assignee

Comment 33

•

6 years ago

(In reply to alexander :surkov from comment #29)
> Comment on attachment 8890563 [details] [diff] [review]
> Prevent aria-owned nodes from getting into bad state.
> 
> Review of attachment 8890563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +2163,2 @@
> >        child->SetRelocated(true);
> > +      relocated->InsertElementAt(idx, child);
> 
> btw, if we are confident than 'relocated' always equals 'owned' then we
> don't need 'relocated' variable, and then
> MOZ_ASSERT(owned == mARIAOwnsHash.Get(aOwner), "MoveChild killed ARIA owned
> array")

I take it back, again. So double back.

I didn't change that block. We still get another reference of the array and compare to it.

Assignee

Comment 34

•

6 years ago

Attached patch Prevent aria-owned nodes from getting into bad state. (obsolete) — Details — Splinter Review

Sorry to come back with another review request. I needed to change a test, and you should probably take a look.

Attachment #8890897 - Flags: review?(surkov.alexander)

Assignee

Updated

•

6 years ago

Attachment #8890563 - Attachment is obsolete: true

Comment 35

•

6 years ago

Comment on attachment 8890897 [details] [diff] [review]
Prevent aria-owned nodes from getting into bad state.

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

::: accessible/tests/mochitest/treeupdate/test_ariaowns.html
@@ +343,5 @@
>  
>      /**
> +     * Attempt to steal an element from other ARIA owns element. This should
> +     * not be possible. The only child that will get owned into this
> +     * container is a previously not aria-owned one.

maybe:

Unsuccessful attempt to steal an element from other ARIA owns element. ARIA owns reference to an element is ignored, if the element is still referenced by other ARIA owns element.

@@ -352,5 @@
>        ];
>  
>        this.invoke = function stealFromOtherARIAOwns_invoke()
>        {
> -        getNode("t3_container2").setAttribute("aria-owns", "t3_child");

why do you have to change this? You could have same test case but having unexpectedInvokerChecker, no?

Updated

•

6 years ago

Flags: needinfo?(eitan)

Assignee

Comment 36

•

6 years ago

(In reply to alexander :surkov from comment #35)
> @@ -352,5 @@
> >        ];
> >  
> >        this.invoke = function stealFromOtherARIAOwns_invoke()
> >        {
> > -        getNode("t3_container2").setAttribute("aria-owns", "t3_child");
> 
> why do you have to change this? You could have same test case but having
> unexpectedInvokerChecker, no?

Mostly out of confusion and uncertainty. How can you be sure an event did *not* happen? What if the first invoker in a queue expects an event to not happen, but then the second invoker expects it to happen? How do you distinguish them correctly? It seems like a recipe for intermittent failures.

Instead I chose to pair the denied aria-owns with one that would succeed, so that we will know that after we receive a REORDER that no other reorders are scheduled.

Flags: needinfo?(eitan)

Comment 37

•

6 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #36)

> > why do you have to change this? You could have same test case but having
> > unexpectedInvokerChecker, no?
> 
> Mostly out of confusion and uncertainty. How can you be sure an event did
> *not* happen? What if the first invoker in a queue expects an event to not
> happen, but then the second invoker expects it to happen? How do you
> distinguish them correctly? It seems like a recipe for intermittent failures.

we used to wait for a timeout listening events, if the event was happening then we reported about error. I don't recall really any intermittent issues caused by timeout, so I have impression of it as a reliable enough approach.

I'm not sure I get your question right about expected/unepxted event ordering problem. Could you rephrase it please? In general we wait for all expected events and then we wait for a certain time for unexpected events.

> Instead I chose to pair the denied aria-owns with one that would succeed, so
> that we will know that after we receive a REORDER that no other reorders are
> scheduled.

this definitely make sense, but as a matter of fact it is a different test case, and not necessary equivalent one taking account how complicated aria-owns implementation may be. For a sake of test coverage I wish we had both of them.

Comment 38

•

6 years ago

Eitan, are you ok to get back the original test to keep it together with your new test or rather not? While it's not huge deal I believe it is something nice to have.

Flags: needinfo?(eitan)

Assignee

Comment 39

•

6 years ago

Attached patch Prevent aria-owned nodes from getting into bad state. — Details — Splinter Review

Attachment #8893568 - Flags: review?(surkov.alexander)

Assignee

Updated

•

6 years ago

Attachment #8890897 - Attachment is obsolete: true

Attachment #8890897 - Flags: review?(surkov.alexander)

Assignee

Updated

•

6 years ago

Flags: needinfo?(eitan)

Comment 40

•

6 years ago

Comment on attachment 8893568 [details] [diff] [review]
Prevent aria-owned nodes from getting into bad state.

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

r=me with comment #9 addressed

::: accessible/generic/DocAccessible.cpp
@@ +2165,2 @@
>        child->SetRelocated(true);
> +      relocated->InsertElementAt(idx, child);

please don't forget to address comment #9 before landing

::: accessible/tests/browser/tree/browser_test_aria_owns.js
@@ +81,5 @@
> +
> +  await onReorders;
> +
> +  testChildrenIds(four, ["b", "e"]);
> +  testChildrenIds(two, ["d", "c"]);

I'm good that the test was moved into a separate test entity (was separated from "aa" test), but not sure I understand why you switched from "three" to "four" testing. The test itself looks correct.

::: accessible/tests/mochitest/treeupdate/test_ariaowns.html
@@ +353,5 @@
>        ];
>  
>        this.invoke = function stealFromOtherARIAOwns_invoke()
>        {
> +        getNode("t3_container3").setAttribute("aria-owns", "t3_child t3_child2");

the test looks fine, however not sure why you didn't come with unexpectedInvokerChecker, which is simpler.

@@ +380,5 @@
>        }
>  
>        this.getID = function stealFromOtherARIAOwns_getID()
>        {
> +        return "Steal! an element from other ARIA owns element";

the string looks funny, Steal! -> Attempt to steal?

Attachment #8893568 - Flags: review?(surkov.alexander) → review+

Assignee

Comment 41

•

6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7faa02d1a6e235009e284fd3e5a7411305689a8a

Assignee

Comment 42

•

6 years ago

(In reply to alexander :surkov from comment #40)
> @@ +380,5 @@
> >        }
> >  
> >        this.getID = function stealFromOtherARIAOwns_getID()
> >        {
> > +        return "Steal! an element from other ARIA owns element";
> 
> the string looks funny, Steal! -> Attempt to steal?

Oops! Fixed that.

Also, I'll have better tests for this soon after a test refactor.

Assignee

Comment 43

•

6 years ago

Crap, I totally forgot to ask for sec approval here.

Comment 44

•

6 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #41)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 7faa02d1a6e235009e284fd3e5a7411305689a8a

#9, #9, #9, comment #9 doesn't seem addressed :)

Assignee

Comment 45

•

6 years ago

Comment on attachment 8893568 [details] [diff] [review]
Prevent aria-owned nodes from getting into bad state.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Medium to hard? I don't think it is a blueprint of how to write an exploit in JS, but the UAF protection is apparent in the patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No, I don't think so. We change the behavior now where one container can't "steal" a aria-owned child from another, but I don't think it is apparent how that can be exploited.

Which older supported branches are affected by this flaw?

This was introduced in 48:
https://bugzilla.mozilla.org/show_bug.cgi?id=1256461

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I would split this patch, and only backport the part of this patch that avoids a UAF. That patch should be easy to rebase.

How likely is this patch to cause regressions; how much testing does it need?

That is why I would suggest the above partial fix. The patch as-is has some real behavioral changes that should not be backported, and will only increase the risk of regressions on stable branches.

Attachment #8893568 - Flags: sec-approval?

Assignee

Comment 46

•

6 years ago

Attached patch beta patch (obsolete) — Details — Splinter Review

Alex, can you verify how I split the patch here. Since this is a release channel, just want to make sure.

Attachment #8894002 - Flags: review?(surkov.alexander)

Assignee

Comment 47

•

6 years ago

Attached patch release patch (obsolete) — Details — Splinter Review

same.

Attachment #8894003 - Flags: review?(surkov.alexander)

Comment 48

•

6 years ago

We'll need an ESR52 patch as well. I hope it'll be reasonably similar to the release patch.

Blocks: 1256461

status-firefox55: --- → wontfix

status-firefox56: --- → affected

status-firefox57: --- → affected

status-firefox-esr52: --- → affected

tracking-firefox56: --- → +

tracking-firefox57: --- → +

tracking-firefox-esr52: --- → 56+

Keywords: regression

Comment 49

•

6 years ago

Comment on attachment 8894002 [details] [diff] [review]
beta patch

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

I don't see "// Child is aria-owned by another container, skip." part from the nightly part, missed? Otherwise looks good, r=me with comments addressed

::: accessible/generic/DocAccessible.cpp
@@ +2160,2 @@
>        child->SetRelocated(true);
> +      relocated->InsertElementAt(idx, child);

it doesn't have to go to a backport, right?

@@ +2224,5 @@
>      //    after load: $("list").setAttribute("aria-owns", "a b");
>      //    later:      $("list").setAttribute("aria-owns", "");
>      if (origContainer != owner || child->IndexInParent() != idxInParent) {
> +      DebugOnly<bool> moved = MoveChild(child, origContainer, idxInParent);
> +      MOZ_ASSERT(moved, "Failed to put child back.");

same here, just debug stuff, up to you

@@ +2249,5 @@
>    Accessible* curParent = aChild->Parent();
>  
> +  if (!aNewParent->IsAcceptableChild(aChild->GetContent())) {
> +    return false;
> +  }

I missed that in the nightly patch, but this condition should be moved one like up, it doesn't make sense to define 'curParent' variable if the child is not acceptable by a parent

Attachment #8894002 - Flags: review?(surkov.alexander) → review+

Comment 50

•

6 years ago

Comment on attachment 8894003 [details] [diff] [review]
release patch

r=me with same comments addressed as for the beta patch

Attachment #8894003 - Flags: review?(surkov.alexander) → review+

Comment 51

•

6 years ago

https://hg.mozilla.org/mozilla-central/rev/7faa02d1a6e2
https://hg.mozilla.org/mozilla-central/rev/a5d58cfbab42

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox57: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla57

Updated

•

6 years ago

Group: dom-core-security → core-security-release

Assignee

Comment 52

•

6 years ago

(In reply to alexander :surkov from comment #49)
> Comment on attachment 8894002 [details] [diff] [review]
> beta patch
> 
> Review of attachment 8894002 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't see "// Child is aria-owned by another container, skip." part from
> the nightly part, missed? Otherwise looks good, r=me with comments addressed
> 
> ::: accessible/generic/DocAccessible.cpp
> @@ +2160,2 @@
> >        child->SetRelocated(true);
> > +      relocated->InsertElementAt(idx, child);
> 
> it doesn't have to go to a backport, right?

Using the newly retrieved array? Yes it does. This is the part that remedies the UAF.

> 
> @@ +2224,5 @@
> >      //    after load: $("list").setAttribute("aria-owns", "a b");
> >      //    later:      $("list").setAttribute("aria-owns", "");
> >      if (origContainer != owner || child->IndexInParent() != idxInParent) {
> > +      DebugOnly<bool> moved = MoveChild(child, origContainer, idxInParent);
> > +      MOZ_ASSERT(moved, "Failed to put child back.");
> 
> same here, just debug stuff, up to you

Pretty benign, I'll leave it.

> 
> @@ +2249,5 @@
> >    Accessible* curParent = aChild->Parent();
> >  
> > +  if (!aNewParent->IsAcceptableChild(aChild->GetContent())) {
> > +    return false;
> > +  }
> 
> I missed that in the nightly patch, but this condition should be moved one
> like up, it doesn't make sense to define 'curParent' variable if the child
> is not acceptable by a parent

Good call.

Assignee

Comment 53

•

6 years ago

Attached patch release patch — Details — Splinter Review

Assignee

Updated

•

6 years ago

Attachment #8894003 - Attachment is obsolete: true

Assignee

Comment 54

•

6 years ago

Attached patch beta patch — Details — Splinter Review

Assignee

Updated

•

6 years ago

Attachment #8894002 - Attachment is obsolete: true

Al Billings [:abillings - ex-MoCo]

Updated

•

6 years ago

Flags: sec-bounty? → sec-bounty+

Comment 55

•

6 years ago

Comment on attachment 8893568 [details] [diff] [review]
Prevent aria-owned nodes from getting into bad state.

sec-approval+ at this point.

What is the beta patch waiting for here to get nominated?

Attachment #8893568 - Flags: sec-approval? → sec-approval+

Assignee

Updated

•

6 years ago

Comment 56

•

6 years ago

Eitan or Alexander, can you request uplift to beta? Thanks.

Flags: needinfo?(surkov.alexander)

Flags: needinfo?(eitan)

Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)

Assignee

Comment 57

•

6 years ago

The patch in bug 1387918 solves this issue more holistically with minimal changes. Can we wait for August 28 to land the patches there instead of these? Bill said to have those patches nominated and ready on the 28th. Does that mean requesting uplift before they land on m-c?

Flags: needinfo?(surkov.alexander)

Flags: needinfo?(eitan)

Flags: needinfo?(abillings)

Comment 58

•

6 years ago

Sure, that makes sense, and we can wait till August 28. Sorry, I missed your link to bug 1387918.  Thanks Eitan.

Flags: needinfo?(abillings)

Comment 59

•

6 years ago

(In reply to Eitan Isaacson [:eeejay] from comment #57)
> The patch in bug 1387918 solves this issue more holistically with minimal
> changes.

The patches from this bug have some good changes which are not part of the minimal changes of bug 1387918, and may be important for stability. I'm worried that betas won't receive them. Eitan, are you sure we don't want these backported?

Tyson Smith [:tsmith]

Updated

•

6 years ago

Flags: needinfo?(eitan)

Assignee

Comment 60

•

6 years ago

(In reply to alexander :surkov from comment #59)
> (In reply to Eitan Isaacson [:eeejay] from comment #57)
> > The patch in bug 1387918 solves this issue more holistically with minimal
> > changes.
> 
> The patches from this bug have some good changes which are not part of the
> minimal changes of bug 1387918, and may be important for stability. I'm
> worried that betas won't receive them. Eitan, are you sure we don't want
> these backported?

OK. I'll nominate this patch, and rebase the one in bug 1387918.

Flags: needinfo?(eitan)

Assignee

Comment 61

•

6 years ago

Comment on attachment 8894534 [details] [diff] [review]
beta patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1256461
[User impact if declined]: A possible UAF
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is pretty minimal and it has been on Nightly for a while now.
[String changes made/needed]:

Attachment #8894534 - Flags: approval-mozilla-beta?

Gerry Chang [:gchang]

Comment 62

•

6 years ago

Comment on attachment 8894534 [details] [diff] [review]
beta patch

Fix a sec-high. Beta56+.

Attachment #8894534 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 63

•

6 years ago

uplift

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

status-firefox56: affected → fixed

Comment 64

•

6 years ago

Please nominate this for ESR52 approval when you get a chance (and patches in other bugs it depends on if necessary).

Flags: needinfo?(eitan)

Assignee

Comment 65

•

6 years ago

Comment on attachment 8894534 [details] [diff] [review]
beta patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Potential use after free
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): This patch has been introduced in beta and nightly for a while with no regressions.
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Flags: needinfo?(eitan)

Attachment #8894534 - Flags: approval-mozilla-esr52?

Assignee

Updated

•

6 years ago

Attachment #8894534 - Flags: approval-mozilla-esr52?

Assignee

Comment 66

•

6 years ago

Come to think of it. I think it would be safer to just take the patch nominated in bug 1387918 that fixes the same UAF.

The patch here fixes other functional issues, but I don't think it appropriate for ESR.

Ryan, would that be OK?

Flags: needinfo?(ryanvm)

Comment 67

•

6 years ago

Fine by me! You're the expert here :)

Flags: needinfo?(ryanvm)

Al Billings [:abillings - ex-MoCo]

Updated

•

6 years ago

status-firefox-esr52: affected → fixed

Updated

•

6 years ago

Alias: CVE-2017-7818

Whiteboard: [adv-main56+][adv-esr52.4+]

Matt Wobensmith [:mwobensmith][:matt:]

Updated

•

6 years ago

Flags: qe-verify+

Whiteboard: [adv-main56+][adv-esr52.4+] → [adv-main56+][adv-esr52.4+][post-critsmash-triage]

Ciprian Georgiu, Desktop QA

Comment 68

•

6 years ago

Reproduced this crash using the test case from comment 20, on an affected ASAN Nightly build (2017-05-10).

Verified fixed on latest ASAN builds 56.0 (20170920224824) and esr 52.4.0 (20170920172358) under Ubuntu 16.04 x64.

It seems that on the latest ASAN Nightly 57.0a1 build the fuzPriv extensions can't be installed even if "extensions.allow-non-mpc-extensions" is set to true and "xpinstall.signatures.required" is set to false. I've also tried to install it from about:debugging, but it did not work that way either. Since this crash requires fuzPriv installed I cannot verify this in 57.0a1 Nightly.

I think it would be useful for QE to have some info on how to install this extension in Firefox 57 or make it compatibile with this version. Any thoughts about this?

Status: RESOLVED → VERIFIED

status-firefox56: fixed → verified

status-firefox-esr52: fixed → verified

Flags: qe-verify+

Updated

•

6 years ago

Depends on: 1388987