Closed Bug 1385372 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 + fixed
firefox57 --- fixed

People

(Reporter: tsmith, Assigned: eeejay)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-high, testcase)

Attachments

(3 files, 3 obsolete files)

Attached file 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?
Attachment #8891529 - Flags: review?(surkov.alexander)
I think this will also fix bug 1385404.
Assignee: nobody → eitan
This is a regression from bug 1378257.
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');
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).
Pulled out test to standalone file. Added the case Alex mentioned above to the test.
Attachment #8892222 - Flags: review?(surkov.alexander)
Attachment #8891529 - Attachment is obsolete: true
Attachment #8891529 - Flags: review?(surkov.alexander)
this one? :)

<div aria-owns="a b c">.setAttribute('aria-owns', 'c a b')
Blocks: 1386024
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
Track 56+ as regression.
Blocks: 1386024
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
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
(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
Attached patch Allow same id in aria-owns. (obsolete) — Splinter Review
Attachment #8892222 - Attachment is obsolete: true
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+
(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+
Keywords: checkin-needed
Needs a rebased patch.
Flags: needinfo?(eitan)
Keywords: checkin-needed
Attachment #8893498 - Attachment is obsolete: true
Rebased and removed the test as we talked about above.
Flags: needinfo?(eitan)
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Since this is fixed, no need to track it for 57. NI Eitan, could you please nominate a patch for Beta uplift?
Flags: needinfo?(eitan)
Attached patch Beta patchSplinter Review
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.