Closed Bug 1538042 Opened 1 year ago Closed 1 year ago

nsFind::Find uses array index -1 access when searching for the null character

Categories

(Core :: Find Backend, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox-esr60 67+ fixed
firefox66 --- wontfix
firefox67 + fixed
firefox68 + fixed

People

(Reporter: jkratzer, Assigned: bradwerth)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(5 keywords, Whiteboard: [adv-main67+][adv-esr60.7+])

Attachments

(4 files, 3 obsolete files)

Found while fuzzing mozilla-central rev feda786b35cb. I'm currently reducing the testcase and will attach it once it's done.

==29764==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7f3ffa2a489e at pc 0x7f3ff72c6abd bp 0x7ffebc1adb10 sp 0x7ffebc1adb08
READ of size 2 at 0x7f3ffa2a489e thread T0 (file:// Content)
#0 0x7f3ff72c6abc in nsFind::Find(char16_t const*, nsRange*, nsRange*, nsRange*, nsRange**) /builds/worker/workspace/build/src/toolkit/components/find/nsFind.cpp:628:12
#1 0x7f3ff72c9b67 in nsWebBrowserFind::SearchInFrame(nsPIDOMWindowOuter*, bool, bool*) /builds/worker/workspace/build/src/toolkit/components/find/nsWebBrowserFind.cpp:676:14
#2 0x7f3ff72c7a87 in nsWebBrowserFind::FindNext(bool*) /builds/worker/workspace/build/src/toolkit/components/find/nsWebBrowserFind.cpp:109:8
#3 0x7f3fecec0f75 in nsGlobalWindowOuter::FindOuter(nsTSubstring<char16_t> const&, bool, bool, bool, bool, bool, bool, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp:6589:20
#4 0x7f3fef5462f4 in mozilla::dom::Window_Binding::find(JSContext*, JS::Handle<JSObject*>, nsGlobalWindowInner*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:6594:21
#5 0x7f3ff04d12f8 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3144:13
#6 0x7f3ff7c3b4a7 in CallJSNative /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:442:13
#7 0x7f3ff7c3b4a7 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:534
#8 0x7f3ff8ddefd2 in js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:3818:10
#9 0x345896d69887 (<unknown module>)

0x7f3ffa2a489e is located 2 bytes to the left of global variable 'gNullChar' defined in '/builds/worker/workspace/build/src/xpcom/string/nsSubstring.cpp:56:23' (0x7f3ffa2a48a0) of size 2
0x7f3ffa2a489e is located 46 bytes to the right of global variable '<string literal>' defined in '/builds/worker/workspace/build/src/xpcom/string/nsTString.h:454:5' (0x7f3ffa2a47c0) of size 176
'<string literal>' is ascii string 'MOZ_RELEASE_ASSERT(this->mData[substring_type::mLength] == 0) (nsTDependentString must wrap only null-terminated strings. You are probably looking for nsTDependentSubstring.)'
SUMMARY: AddressSanitizer: global-buffer-overflow /builds/worker/workspace/build/src/toolkit/components/find/nsFind.cpp:628:12 in nsFind::Find(char16_t const*, nsRange*, nsRange*, nsRange*, nsRange**)
Shadow bytes around the buggy address:
0x0fe87f44c8c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fe87f44c8d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fe87f44c8e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fe87f44c8f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x0fe87f44c900: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f9 f9
=>0x0fe87f44c910: f9 f9 f9[f9]02 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
0x0fe87f44c920: 00 00 00 03 f9 f9 f9 f9 00 00 00 00 00 00 00 00
0x0fe87f44c930: 00 07 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 07 f9
0x0fe87f44c940: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 03 f9 f9
0x0fe87f44c950: f9 f9 f9 f9 06 f9 f9 f9 f9 f9 f9 f9 00 06 f9 f9
0x0fe87f44c960: f9 f9 f9 f9 02 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
Shadow gap: cc
==29764==ABORTING

Attached file testcase.html
Flags: in-testsuite?
Group: core-security → dom-core-security
Keywords: csectype-uaf

Whoops, I just assumed it was a UAF.

Keywords: csectype-uaf

nsFind::Find() has a bunch of C-style arrays, and the test case involves an embedded null character.

Keywords: csectype-bounds
Component: DOM: Core & HTML → Find Backend

The conversion from nsString -> const char16_t* -> nsAutoString is losing track of the fact that the '\0' string has length 1. By the time it's picked up in https://searchfox.org/mozilla-central/rev/a7315d78417179b151fef6108f2bce14786ba64d/toolkit/components/find/nsFind.cpp#476, the length of the auto string is considered to be 0, which calculates a -1 array index used later.

We'll need some more sensitive handling of strings in nsFind::Find. Possibly use a different string type, or pass in the length along with the raw characters.

Ugh, since nsIFind::Find is defined in an idl, the change in method signature will have to propagate up to idl.

I'll mark this sec-high, though I'm not sure how exploitable accessing index -1 of an array is in practice.

Keywords: sec-high
Assignee: nobody → bwerth
Attachment #9053799 - Attachment description: Bug 1538042 Part 1: Change nsIFind::Find to use an inout search string parameter, so we also get length. → Bug 1538042 Part 1: Change nsIFind::Find to also require text length.
Summary: AddressSanitizer: global-buffer-overflow /builds/worker/workspace/build/src/toolkit/components/find/nsFind.cpp:628:12 in nsFind::Find(char16_t const*, nsRange*, nsRange*, nsRange*, nsRange**) → nsFind::Find uses array index -1 access when searching for the null character
Attachment #9054056 - Attachment is obsolete: true
Attachment #9053800 - Attachment is obsolete: true
Attachment #9053799 - Attachment description: Bug 1538042 Part 1: Change nsIFind::Find to also require text length. → Bug 1538042 Part 1: Change nsIFind::Find to have access to text length
Attachment #9053799 - Attachment description: Bug 1538042 Part 1: Change nsIFind::Find to have access to text length → Bug 1538042 Part 1: Change nsIFind::Find to have access to text length.
Priority: -- → P1
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

As a sec-high or higher bug that affects more than Nightly, this should have gotten sec-approval before landing. You should nominate it for uplift to all other branches.

Flags: needinfo?(bwerth)

(In reply to Andrew McCreight [:mccr8] from comment #17)

You should nominate it for uplift to all other branches.

By "all other branches", I mean beta and ESR.

Comment on attachment 9053799 [details]
Bug 1538042 Part 1: Change nsIFind::Find to have access to text length.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Unknown abuse potential by triggering find-in-page.
  • Fix Landed on Version: 68
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Changes string type in a IDL parameter listing, but makes no other changes to behavior.
  • String or UUID changes made by this patch:
  • Do you want to request approval of these patches as well?: on
Flags: needinfo?(bwerth)
Attachment #9053799 - Flags: approval-mozilla-esr60?

Comment on attachment 9053799 [details]
Bug 1538042 Part 1: Change nsIFind::Find to have access to text length.

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: None
  • User impact if declined: Unknown potential for abuse by supplying null characters to find-in-page.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • 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): Changes IDL parameter string type, but makes no other changes to behavior.
  • String changes made/needed:
  • Do you want to request approval of these patches as well?: on
Attachment #9053799 - Flags: approval-mozilla-beta?

(In reply to Andrew McCreight [:mccr8] from comment #17)

As a sec-high or higher bug that affects more than Nightly, this should have gotten sec-approval before landing.

I'm sorry I didn't follow the procedure properly. I'll read up on it more thoroughly to make sure I get it right next time.

(In reply to Brad Werth [:bradwerth] from comment #21)

I'm sorry I didn't follow the procedure properly. I'll read up on it more thoroughly to make sure I get it right next time.

It happens. I should have mentioned it when I did your review.

FYI, ESR60 will need a rebased patch.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)

FYI, ESR60 will need a rebased patch.

Yes, of course. I'll generate one and upload it soon.

Attached patch Bug1538042Part1-esr60.patch (obsolete) — Splinter Review
Flags: needinfo?(bwerth)

Comment on attachment 9055006 [details] [diff] [review]
Bug1538042Part1-esr60.patch

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Find-in-page searches starting with the NULL character will trigger out of bounds memory access.
  • Fix Landed on Version: 68
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Changes type of a string parameter, and not much else.
  • String or UUID changes made by this patch:
Attachment #9055006 - Flags: approval-mozilla-esr60?
Attachment #9053799 - Flags: approval-mozilla-esr60?

(In reply to Brad Werth [:bradwerth] from comment #21)

(In reply to Andrew McCreight [:mccr8] from comment #17)

As a sec-high or higher bug that affects more than Nightly, this should have gotten sec-approval before landing.

I'm sorry I didn't follow the procedure properly. I'll read up on it more thoroughly to make sure I get it right next time.

Could you fill the sec-approval form anyway so as that the bug gets on our sec team radar before I take the uplift to beta please? Thanks!

Flags: needinfo?(bwerth)

Comment on attachment 9053799 [details]
Bug 1538042 Part 1: Change nsIFind::Find to have access to text length.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Difficult to use. The attacker would trigger a read to a two-byte value immediate preceding a parameter string. Within the algorithm, this value is used in a comparison but not returned. Hard to imagine how it could be a practical 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?: all
  • 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?: Very unlikely to cause a regression.
Flags: needinfo?(bwerth)
Attachment #9053799 - Flags: sec-approval?
Attachment #9055006 - Flags: sec-approval?

Comment on attachment 9053799 [details]
Bug 1538042 Part 1: Change nsIFind::Find to have access to text length.

sec-approval+ for mozilla-central. We should get a ESR60 patch nominated as well.

The test that I see in one of the patches should not be checked in until after we release. We don't check in tests at the same time as high or critical rated security bugs normally.

Attachment #9053799 - Flags: sec-approval?
Attachment #9053799 - Flags: sec-approval+
Attachment #9053799 - Flags: approval-mozilla-beta?
Attachment #9053799 - Flags: approval-mozilla-beta+
Comment on attachment 9055006 [details] [diff] [review]
Bug1538042Part1-esr60.patch

Nevermind, I found the ESR60 patch. :-)
Attachment #9055006 - Flags: sec-approval?
Attachment #9055006 - Flags: sec-approval+
Attachment #9055006 - Flags: approval-mozilla-esr60?
Attachment #9055006 - Flags: approval-mozilla-esr60+
Attachment #9055006 - Attachment is obsolete: true
Flags: needinfo?(bwerth)

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Find-in-page searches starting with the NULL character will trigger out of bounds memory access.
  • Fix Landed on Version: 68
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Changes type of a string parameter, and not much else.
  • String or UUID changes made by this patch:

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Difficult to use. The attacker would trigger a read to a two-byte value immediate preceding a parameter string. Within the algorithm, this value is used in a comparison but not returned. Hard to imagine how it could be a practical 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?: all
  • 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?: Very unlikely to cause a regression.
Attachment #9060273 - Flags: sec-approval?
Attachment #9060273 - Flags: approval-mozilla-esr60?
Comment on attachment 9055006 [details] [diff] [review]
Bug1538042Part1-esr60.patch

You don't need to re-request approval when rebasing a patch. Thanks, though :)
Attachment #9055006 - Flags: sec-approval+
Attachment #9055006 - Flags: approval-mozilla-esr60+
Attachment #9060273 - Flags: sec-approval?
Attachment #9060273 - Flags: sec-approval+
Attachment #9060273 - Flags: approval-mozilla-esr60?
Attachment #9060273 - Flags: approval-mozilla-esr60+
Whiteboard: [adv-main67+][adv-esr60.7+]

Is it now appropriate to land the tests?

Flags: needinfo?(continuation)

The release date isn't actually until tomorrow, and then it will take a while to roll out to everybody. Next Monday should be reasonable. Thanks for following up here! It is easy to forget about landing the tests later.

Flags: needinfo?(continuation)

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Comment on attachment 9054318 [details]
Bug 1538042 Part 2: Add tests of null character searches.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: This remaining piece is just a unit test and a mochitest. The behavior-changing code patch landed some months ago and was backported to ESR.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Backports of the tests are not needed.
  • How likely is this patch to cause regressions; how much testing does it need?: A change to the tests won't cause regressions.
Flags: needinfo?(bwerth)
Attachment #9054318 - Flags: sec-approval?
Regressions: 1578996

Landed:
Part 2: Add tests of null character searches. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/4035fee4dd33e16b02bee9bf7b5b14088fe80104

Backed out for frequent mochitest failures in toolkit/components/windowcreator/test/test_nsFind.html on Linux Quantumrender debug:

https://hg.mozilla.org/integration/autoland/rev/6c74ea8db80ca56f0553730043c6e07df3eb0415

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=265103744&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cquantumrender%2Cm%2816%29&revision=4035fee4dd33e16b02bee9bf7b5b14088fe80104
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=265105361&repo=autoland

[task 2019-09-05T09:18:52.490Z] 09:18:52 INFO - TEST-PASS | toolkit/components/windowcreator/test/test_nsFind.html | "native null" should be found
[task 2019-09-05T09:18:52.491Z] 09:18:52 INFO - Buffered messages finished
[task 2019-09-05T09:18:52.492Z] 09:18:52 INFO - TEST-UNEXPECTED-FAIL | toolkit/components/windowcreator/test/test_nsFind.html | "injected null

Flags: needinfo?(bwerth)

Emilio, the intermittent test failure of https://phabricator.services.mozilla.com/D25293 appears to happen because the test logic is running before the appended text node receives a frame. What event should the test wait on to ensure that the frame has been created?

Flags: needinfo?(bwerth) → needinfo?(emilio)

I'm assuming it's test_nsFind, not test_find what's failing, right?

Probably nsFind should flush layout if it depends on frames, but given it doesn't seem to, just calling getBoundingClientRect() on the container or something of that sort should do.

Flags: needinfo?(emilio)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #50)

I'm assuming it's test_nsFind, not test_find what's failing, right?

Probably nsFind should flush layout if it depends on frames, but given it doesn't seem to, just calling getBoundingClientRect() on the container or something of that sort should do.

Now that our find code checks for frame visibility (since Bug 1302470) it IS dependent on frame construction. I'll add a getBoundingClientRect() call and see if that fixes the intermittent.

(In reply to Brad Werth [:bradwerth] from comment #51)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #50)

I'm assuming it's test_nsFind, not test_find what's failing, right?

Probably nsFind should flush layout if it depends on frames, but given it doesn't seem to, just calling getBoundingClientRect() on the container or something of that sort should do.

Now that our find code checks for frame visibility (since Bug 1302470) it IS dependent on frame construction. I'll add a getBoundingClientRect() call and see if that fixes the intermittent.

Well but it was before too, right? I think this is just a matter of nsIFind not flushing, but its consumers (nsTypeAheadFind and co.) doing it.

This is now ready to land again. The intermittent appears to be resolved. I'll await the handling of the sec-approval? flag and not attempt to land it myself.

Comment on attachment 9054318 [details]
Bug 1538042 Part 2: Add tests of null character searches.

sec-approval+ for mozilla-central.

Attachment #9054318 - Flags: sec-approval? → sec-approval+
Regressions: 1544147
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.