memmove negative-size-param in [@mozilla::a11y::HyperTextAccessible::InsertChildAt]

RESOLVED FIXED in Firefox 56

Status

()

Core
Disability Access APIs
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: tsmith, Assigned: eeejay)

Tracking

(Blocks: 1 bug, {crash, sec-high, testcase})

Trunk
mozilla57
crash, sec-high, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56+ fixed, firefox57 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

7 months ago
Created attachment 8891444 [details]
test_case.html

Changeset: 16ffc1d05422a81099ce8b9b59de66dde4c8b2f0
Build ID: 20170728132457

==4272==ERROR: AddressSanitizer: negative-size-param: (size=-17179869184)
==4272==AddressSanitizer: while reporting a bug found another one. Ignoring.
==4272==AddressSanitizer: while reporting a bug found another one. Ignoring.
    #0 0x4a4afb in __asan_memmove /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:445:3
    #1 0x7fb1835638b2 in MoveOverlappingRegion src/obj-firefox/dist/include/nsTArray.h:621:5
    #2 0x7fb1835638b2 in ShiftData<nsTArrayInfallibleAllocator> src/obj-firefox/dist/include/nsTArray-inl.h:272
    #3 0x7fb1835638b2 in RemoveElementsAt src/obj-firefox/dist/include/nsTArray.h:2061
    #4 0x7fb1835638b2 in mozilla::a11y::HyperTextAccessible::InsertChildAt(unsigned int, mozilla::a11y::Accessible*) src/accessible/generic/HyperTextAccessible.cpp:1914
    #5 0x7fb1835520e4 in mozilla::a11y::DocAccessible::MoveChild(mozilla::a11y::Accessible*, mozilla::a11y::Accessible*, int) src/accessible/generic/DocAccessible.cpp:2302:15
    #6 0x7fb183551477 in mozilla::a11y::DocAccessible::DoARIAOwnsRelocation(mozilla::a11y::Accessible*) src/accessible/generic/DocAccessible.cpp:2157:9
    #7 0x7fb1834bd6ac in mozilla::a11y::NotificationController::WillRefresh(mozilla::TimeStamp) src/accessible/base/NotificationController.cpp:811:18
    #8 0x7fb1802e8057 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1854:12
    #9 0x7fb1802f77d5 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:298:7
    #10 0x7fb1802f7492 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:319:5
    #11 0x7fb1802f9b2b in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:761:5
    #12 0x7fb1802f9b2b in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:674
    #13 0x7fb1802f4e97 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:520:20
    #14 0x7fb1795d8a1e in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1446:14
    #15 0x7fb1795dec98 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:480:10
    #16 0x7fb17a3f0951 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #17 0x7fb17a34de1b in RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
    #18 0x7fb17a34de1b in RunHandler src/ipc/chromium/src/base/message_loop.cc:319
    #19 0x7fb17a34de1b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
    #20 0x7fb17fc04e4f in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:156:27
    #21 0x7fb183ce45f1 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:287:30
    #22 0x7fb183ec02d4 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4596:22
    #23 0x7fb183ec1ed8 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4760:8
    #24 0x7fb183ec330b in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4855:21
    #25 0x4eb643 in do_main src/browser/app/nsBrowserApp.cpp:236:22
    #26 0x4eb643 in main src/browser/app/nsBrowserApp.cpp:309
    #27 0x7fb196b9a82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
    #28 0x41d198 in _start (m-c-1501248297-asan-opt/firefox+0x41d198)
Flags: in-testsuite?
(Assignee)

Comment 1

7 months ago
Created attachment 8891529 [details] [diff] [review]
Allow same id in aria-owns. r?surkov
Attachment #8891529 - Flags: review?(surkov.alexander)
(Assignee)

Comment 2

7 months ago
I think this will also fix bug 1385404.
Assignee: nobody → eitan
(Assignee)

Comment 3

7 months ago
This is a regression from bug 1378257.
Blocks: 1378257
status-firefox54: --- → unaffected
status-firefox55: --- → unaffected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox56: --- → ?

Comment 4

7 months ago
Comment on attachment 8891529 [details] [diff] [review]
Allow same id in aria-owns. r?surkov

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

::: accessible/generic/DocAccessible.cpp
@@ +2109,5 @@
>  #endif
>  
> +    if (owned->IndexOf(child) < idx) {
> +      continue; // ignore second entry of same ID
> +    }

doesn't it fail now for aria-owns regroup case like
<div aria-owns="c1 c2">.setAttribute('aria-owns', 'c2 c1');
(Assignee)

Comment 5

7 months ago
It shouldn't fail.

In the second relocation, c2 is in index 1 in the 'owned' array, and does not appear before the current idx (0).
(Assignee)

Comment 6

7 months ago
Created attachment 8892222 [details] [diff] [review]
Allow same id in aria-owns. r?surkov

Pulled out test to standalone file. Added the case Alex mentioned above to the test.
Attachment #8892222 - Flags: review?(surkov.alexander)
(Assignee)

Updated

7 months ago
Attachment #8891529 - Attachment is obsolete: true
Attachment #8891529 - Flags: review?(surkov.alexander)

Comment 7

7 months ago
this one? :)

<div aria-owns="a b c">.setAttribute('aria-owns', 'c a b')
(Assignee)

Updated

7 months ago
Blocks: 1386024
(Assignee)

Comment 8

7 months ago
Yeah. That works too.

The loop that iterates over the ids inserts into the owned at the same index as the id position in the attribute. The check that I moved up only makes sure that the same element does not appear in prior indexes.

So given aria-owns="c a b", we have the following owned array from the previous attribute value: ["a", "b", "c"].

The loop does this:
childEl="c",idx=0 -> ["c", "b", "c"]
childEl="a",idx=1 -> ["c", "a", "c"]
childEl="b",idx=2 -> ["c", "a", "b"]
No longer blocks: 1386024

Comment 9

7 months ago
Track 56+ as regression.
tracking-firefox56: ? → +
(Assignee)

Updated

7 months ago
Blocks: 1386024

Comment 10

7 months ago
Comment on attachment 8892222 [details] [diff] [review]
Allow same id in aria-owns. r?surkov

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

::: accessible/generic/DocAccessible.cpp
@@ +2109,5 @@
>  #endif
>  
> +    if (owned->IndexOf(child) < idx) {
> +      continue; // ignore second entry of same ID
> +    }

could you please add an extended comment to explain how this line works, something lile:

// Ignore second entry of same ID. All processed ARIA owned children are inserted into the |owned| array at |idx| index. If a child is found in the array at preceding index, then it means the child has a re-entry, for example, aria-owns='c c'.

::: accessible/tests/browser/tree/browser_bug_1385372.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

You decided to go with new-test-file-per-a-bug approach? It may be harder to find out what has a test coverage and what doesn't. At least it makes sense to put the file into aria-owns subfolder or prefix the test like browser_ariaowns_bugnumber.js. I think I would stick with more generic file name convension, something like browser_ariaowns_1.js if for some reason you don't want to put all these tests into one file.

also please note, that the test probably should be separated from the main patch because this is security issue.

@@ +41,5 @@
> +
> +  await ContentTask.spawn(browser, null, async function() {
> +    // Swap the order of relocated children, makes sure
> +    // we don't drop any of them by mistake.
> +    document.getElementById("two").setAttribute("aria-owns", "b a");

Not something I'm asking you to do, but it'd be nice to have a aria-owns test for permutations of N children. At least I wouldn't come questioning you whether 'b a' or 'c a b' work :)
Attachment #8892222 - Flags: review?(surkov.alexander) → review+
Keywords: sec-high
(Assignee)

Comment 11

7 months ago
I asked Yura about bug_xxx test files, and he was fine with it. I wasn't sure either initially, but its hard to be imaginative with test names. I also think the current aria-owns browser test is growing too big. I want very simple and quickly understandable tests. I'll remove the test from this patch, and will followup with a redone generalized aria-owns test setup.
Keywords: sec-high

Comment 12

7 months ago
(In reply to Eitan Isaacson [:eeejay] from comment #11)
> I asked Yura about bug_xxx test files, and he was fine with it. I wasn't
> sure either initially, but its hard to be imaginative with test names. I
> also think the current aria-owns browser test is growing too big.

yeah, and then we could with go with tree/browser_ariaowns.js :)

btw, not sure if it's a good idea to have a test case mentioning a security bug in its name :)
Keywords: sec-high
(Assignee)

Comment 13

7 months ago
Created attachment 8893498 [details] [diff] [review]
Allow same id in aria-owns.
(Assignee)

Updated

7 months ago
Attachment #8892222 - Attachment is obsolete: true
(Assignee)

Comment 14

7 months ago
Comment on attachment 8893498 [details] [diff] [review]
Allow same id in aria-owns.

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

The patch doesn't directly show how a signed index is possible. But as the test case shows, it is easily reproducable.

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?

56.

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

Bug 1378257.


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

It should apply cleanly to 56.

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

I have tests that I can land in a followup.
Attachment #8893498 - Flags: sec-approval?
Attachment #8893498 - Flags: review+
(Assignee)

Comment 15

7 months ago
(In reply to alexander :surkov from comment #12)
> (In reply to Eitan Isaacson [:eeejay] from comment #11)
> > I asked Yura about bug_xxx test files, and he was fine with it. I wasn't
> > sure either initially, but its hard to be imaginative with test names. I
> > also think the current aria-owns browser test is growing too big.
> 
> yeah, and then we could with go with tree/browser_ariaowns.js :)
> 
> btw, not sure if it's a good idea to have a test case mentioning a security
> bug in its name :)

Right, I will have this just be a new case we test for, no bug numbers.
Comment on attachment 8893498 [details] [diff] [review]
Allow same id in aria-owns.

sec-approval+. We'll want this on 56 if it gets checked into 57.
Attachment #8893498 - Flags: sec-approval? → sec-approval+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed
Needs a rebased patch.
status-firefox57: --- → affected
tracking-firefox57: --- → ?
Flags: needinfo?(eitan)
Keywords: checkin-needed
(Assignee)

Comment 18

7 months ago
Created attachment 8895427 [details] [diff] [review]
Allow same id in aria-owns.
(Assignee)

Updated

7 months ago
Attachment #8893498 - Attachment is obsolete: true
(Assignee)

Comment 19

7 months ago
Rebased and removed the test as we talked about above.
Flags: needinfo?(eitan)
(Assignee)

Updated

7 months ago
Blocks: 1385404
https://hg.mozilla.org/mozilla-central/rev/f2f297d544ff

Please request Beta approval on this when you get a chance. It grafts cleanly.
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57

Comment 22

7 months ago
Since this is fixed, no need to track it for 57. NI Eitan, could you please nominate a patch for Beta uplift?
tracking-firefox57: ? → ---
Flags: needinfo?(eitan)
(Assignee)

Comment 23

6 months ago
Created attachment 8895888 [details] [diff] [review]
Beta patch
(Assignee)

Comment 24

6 months ago
Comment on attachment 8895888 [details] [diff] [review]
Beta patch

Approval Request Comment
[Feature/Bug causing the regression]: bug bug 1378257
[User impact if declined]: Potential UAF
[Is this code covered by automated tests?]: It will be covered in 57 in a followup bug 1388062
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not riskier than the UAF it fixes.
[Why is the change risky/not risky?]:
[String changes made/needed]: None.
Flags: needinfo?(eitan)
Attachment #8895888 - Flags: review+
Attachment #8895888 - Flags: approval-mozilla-beta?
Comment on attachment 8895888 [details] [diff] [review]
Beta patch

Fix a sec-high. Beta56+. Should be in 56.0b3.
Attachment #8895888 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: dom-core-security → core-security-release
(In reply to Eitan Isaacson [:eeejay] from comment #24)
> [Is this code covered by automated tests?]: It will be covered in 57 in a
> followup bug 1388062
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Eitan's assessment on manual testing needs.
Flags: qe-verify-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.