1369836 - heap-use-after-free in [@ mozilla::a11y::DocAccessible::UncacheChildrenInSubtree]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

Found while fuzzing with mozilla-central 20170602-aeb3d0ca558f on x86_64 Linux

==87334==ERROR: AddressSanitizer: heap-use-after-free on address 0x602000384250 at pc 0x7fc68b2085f6 bp 0x7ffe49a14c80 sp 0x7ffe49a14c78
READ of size 8 at 0x602000384250 thread T0
    #0 0x7fc68b2085f5 in Hdr src/obj-firefox/dist/include/nsTArray.h:525:32
    #1 0x7fc68b2085f5 in Elements src/obj-firefox/dist/include/nsTArray.h:1033
    #2 0x7fc68b2085f5 in IndexOf<mozilla::a11y::Accessible *, nsDefaultComparator<RefPtr<mozilla::a11y::Accessible>, mozilla::a11y::Accessible *> > src/obj-firefox/dist/include/nsTArray.h:1168
    #3 0x7fc68b2085f5 in RemoveElement<mozilla::a11y::Accessible *, nsDefaultComparator<RefPtr<mozilla::a11y::Accessible>, mozilla::a11y::Accessible *> > src/obj-firefox/dist/include/nsTArray.h:1751
    #4 0x7fc68b2085f5 in bool nsTArray_Impl<RefPtr<mozilla::a11y::Accessible>, nsTArrayInfallibleAllocator>::RemoveElement<mozilla::a11y::Accessible*>(mozilla::a11y::Accessible* const&) src/obj-firefox/dist/include/nsTArray.h:1765
    #5 0x7fc68b208880 in mozilla::a11y::DocAccessible::UncacheChildrenInSubtree(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:2320:14
    #6 0x7fc68b208140 in mozilla::a11y::DocAccessible::ContentRemoved(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:2009:3
    #7 0x7fc68b202755 in mozilla::a11y::DocAccessible::ContentRemoved(nsIContent*) src/accessible/generic/DocAccessible.cpp:2020:5
    #8 0x7fc68b20288a in mozilla::a11y::DocAccessible::ContentRemoved(nsIContent*) src/accessible/generic/DocAccessible.cpp:2026:5
    #9 0x7fc68b1a45cc in nsAccessibilityService::ContentRemoved(nsIPresShell*, nsIContent*) src/accessible/base/nsAccessibilityService.cpp:585:15
    #10 0x7fc688a7a25c in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags, bool*, nsIContent**) src/layout/base/nsCSSFrameConstructor.cpp:8722:19
    #11 0x7fc6889d3999 in mozilla::PresShell::ContentRemoved(nsIDocument*, nsIContent*, nsIContent*, int, nsIContent*) src/layout/base/PresShell.cpp:4482:22
    #12 0x7fc684c4e8e2 in nsNodeUtils::ContentRemoved(nsINode*, nsIContent*, int, nsIContent*) src/dom/base/nsNodeUtils.cpp:226:3
    #13 0x7fc684c4e39e in nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) src/dom/base/nsINode.cpp:1926:5
    #14 0x7fc6849abd43 in mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) src/dom/base/FragmentOrElement.cpp:1211:5
...
see log.txt

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[alexander :surkov (:asurkov)]

I was able to reproduce it on a debug build with voiceover running, fuzz opt out.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

hopefully, in bug 1359828 we will be able to simplify this logic, and make it error-proof, here's a code fix for now

Comment 4

[Eitan Isaacson [:eeejay]]

Comment on attachment 8875841 [details] [diff] [review]
patch

Looks good for a quick fix.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 8875841 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? shouldn't be too easy

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? unlikely, I think I would go with something like "ARIA owns relocated flag of accessible object may get out of sync with ARIA owns hash" as a commit message

Which older supported branches are affected by this flaw? all branches as far as I can tell: I can see same code path in esr52.

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

How likely is this patch to cause regressions; how much testing does it need? it'd be good to run for a while before throughout backports

Comment 6

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

Sec-approval+ for checkin on trunk on June 26, two weeks into the new cycle.
We'll want Beta and ESR52 branches made and nominated to go in after it lands on trunk.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Al Billings [:abillings] from comment #6)
> Sec-approval+ for checkin on trunk on June 26, two weeks into the new cycle.
> We'll want Beta and ESR52 branches made and nominated to go in after it
> lands on trunk.

I feel confused, is the patch not allowed for landing before June 26, what will keep Firefox vulnerable for two weeks more? Are there some changes in landing policies and nightly is no longer open for all commits?

Comment 8

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

(In reply to alexander :surkov from comment #7)

> I feel confused, is the patch not allowed for landing before June 26, what
> will keep Firefox vulnerable for two weeks more? Are there some changes in
> landing policies and nightly is no longer open for all commits?


I suggest you read https://wiki.mozilla.org/Security/Bug_Approval_Process, which has been the policy for years.

Sec-high or sec-critical bugs that affect more than trunk do not land without sec-approval+. I and Dan Veditz make a decision about when the bugs land during the (normally six week to eight week) development cycle based on risk.

Our Firefox users are not, as a rule, running nightly. If you land on nightly, the fix is public at that point, putting all of the OTHER release users (and Beta normally) at risk until the fix ships on those releases, hence my note about making sure you nominate branch patches as well. We won't protect users on our release branch until Firefox 55 ships, which is on August 8.

So, folks are not vulnerable for "two weeks more." They are vulnerable until August 8. The sooner you check in this fix, the longer they are vulnerable if anyone is watching for us to check in security bugs (and people do watch our check ins).

Comment 9

[alexander :surkov (:asurkov)]

I see the point of keeping the patch off eyes, however I'm not clear what time is given for the patch to run it on nightly for regressions finding. If it is expected that the patch will be landed on beta and releases channels soon after nightly landing, then we may be at greater risk than we were since untested stuff goes that far quickly.

Delays in landing is also bad for development process: merging, testing and all stuff around are harder, which I guess may be sacrificed for security reasons, but I'm curious if certain steps can be taken to hide or mask sec-matter of the patch, so delays can be avoided.

Comment 10

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

Then delay the beta landing a few days after it lands on trunk. No one is making you land it on Beta and ESR52 the same or next day. I expect devs to do the right things with their patches and watch results in their areas after they land code. This process is merely a gating function on initial (public) checkins of sec-high and sec-critical bugs.

> I'm curious if certain steps can be taken to hide or mask sec-matter of the patch, so delays can be avoided.

You can check in under a cover bug if the patch can be subsumed into a large check in that is going on in the same area. This has been done somewhat often. Sometimes there is no way to hide how obvious the security issue or bug is on check in (hence the template questions for sec-approval around this). I suggest talking with others in your engineering org, or people like Boris, about best practices here. 

If we want to have further discussions, I suggest that we take it to email. Sec-approval has been a largely static and accepted process since 2012. I doubt a debate in a bug is going to resolve any concerns.

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to alexander :surkov from comment #9)
> I'm not clear what time is given for the patch to run it on nightly for
> regressions finding. If it is expected that the patch will be landed on
> beta and releases channels soon after nightly landing, then we may be at
> greater risk than we were since untested stuff goes that far quickly.

Absolutely, it's all a big juggling act. We're at risk of reverse engineering the vulnerability from the moment the patch appears in public (because it's trivial to tell it's a "security fix"), and we're at risk of uncaught regressions if we don't allow enough testing time. Few of our regressions are caught in nightly (not many users!) unless they're caught by our tests, but a try build can do that. So land on nightly and beta at the same time and we get maximum testing for the longest period of time that we think we can stand the exposure. If you're not ready to land on beta when you land on nightly (needs a different patch?) then we want to know that _before_ we approve it to land on nightly because that adds risk.

In this case Al has given 6 weeks which frankly scares me as being too long. We could shorten it, but we're unlikely to lengthen it. If we had perfect confidence in all the patches we would wait and not land them at all until the night before the final Release Candidate build. No one could get any advance clues about the security patch then! Obviously that's far too dangerous given our regression rate.

> Delays in landing is also bad for development process: merging, testing and
> all stuff around are harder, which I guess may be sacrificed for security
> reasons, but I'm curious if certain steps can be taken to hide or mask
> sec-matter of the patch, so delays can be avoided.

Security bugs are bad for everything. Each one is a pain in the ass in every possible way and the only way to stop the pain is to write more secure code. Until then we try to spread the pain around so it doesn't build up too much in any one place.

Comment 12

[alexander :surkov (:asurkov)]

This was the first time when my patch was given two weeks before landing, it was used to be always sec-approval+, then land it. So this caught me by surprise, and that's why I was asking the questions.

I definitely see your points, I'm not clear though why this specific patch receives this specific timeframe. You probably shouldn't teach me your tricks, but this is definitely useful knowledge.

The one thing you should be aware of, I will be on pto around 26 and after, and won't be able to help if something goes wrong. I can test and upload patch for the beta just in case, but that's probably it.

Comment 13

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

(In reply to alexander :surkov from comment #12)
> This was the first time when my patch was given two weeks before landing, 

I almost never approve any security bugs to land in the first two weeks of the cycle so this was just a random matter of timing (for you).

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch2

this is a lighter version and more correct, since even if an accessible was moved inside its parent, then ariaowns array should be updated as well.

Comment 15

[alexander :surkov (:asurkov)]

Attachment: patch3

polishing

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 8878225 [details] [diff] [review]
patch3

[Security approval request comment]
How easily could an exploit be constructed based on the patch? shouldn't be too easy

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? unlikely, I think I would go with something like "ARIA owns relocated flag of accessible object may get out of sync with ARIA owns hash" as a commit message

Which older supported branches are affected by this flaw? all branches as far as I can tell: I can see same code path in esr52.

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

How likely is this patch to cause regressions; how much testing does it need? it'd be good to run for a while before throughout backports

Comment 17

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

Comment on attachment 8878225 [details] [diff] [review]
patch3

sec-approval+ with the same instructions as previous sec-approval. :-)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80be04d5ab1a67f463a0e6060c37c896806af96b

This grafts cleanly to Beta but will need rebasing for ESR52. Please provide a rebased patch and request branch approvals when you get a chance.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80be04d5ab1a

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 80be04d5ab1a67f463a0e6060c37c896806af96b
> 
> This grafts cleanly to Beta but will need rebasing for ESR52. Please provide
> a rebased patch and request branch approvals when you get a chance.

I was mistaken when I said about the crash path is going up to 52. This crash path was introduced by bug 1362063. So technically we don't have to backport it, however the fix makes sense for 52 and may fix other issues. It might be a bad justification for backporting though.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please at least request Beta approval one way or another. I'll defer to your judgement as to whether it's worth rebasing for ESR52 and requesting approval.

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 8878225 [details] [diff] [review]
patch3

Approval Request Comment
[Feature/Bug causing the regression]: bug 1362063
[User impact if declined]:crashes
[Is this code covered by automated tests?]:somewhat
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: run a test case from this bug, see comment #2
[List of other uplifts needed for the feature/fix]:no
[Is the change risky?]:fair risk
[Why is the change risky/not risky?]:small changes in complicated code
[String changes made/needed]:no

Comment 23

[Julien Cristau [:jcristau]]

Comment on attachment 8878225 [details] [diff] [review]
patch3

sec-crit fix for beta55

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

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

Are we getting an ESR52 patch for this?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Al Billings [:abillings] from comment #25)
> Are we getting an ESR52 patch for this?

I'd say no since there's no known sec issue there, see comment #20