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

Status: RESOLVED FIXED

(Core :: Find Backend, defect, P1)

Description

[Jason Kratzer [:jkratzer]]

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

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Comment 2

[Andrew McCreight [:mccr8]]

Whoops, I just assumed it was a UAF.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Brad Werth [:bradwerth]]

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.

Comment 5

[Brad Werth [:bradwerth]]

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

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1538042 Part 1: Change nsIFind::Find to have access to text length.

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1538042 Part 2: Make nsFind::Find handle strings containing null characters.

Depends on D25005

Comment 8

[Andrew McCreight [:mccr8]]

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

Comment 9

[Brad Werth [:bradwerth]]

Attachment: Bug 1538042 Part 3: Update test usage of nsIFind::Find, and search for null characters.

Depends on D25006

Comment 10

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=946ade13124d532556a68f0463fa75ba08d7e1ea

Comment 11

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ab29c27ea6118460c9b82e75c0588ec5d050bcd

Comment 12

[Brad Werth [:bradwerth]]

Attachment: Bug 1538042 Part 2: Add tests of null character searches.

Depends on D25005

Comment 13

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3596c5ec4429d9f6688f62851b0c685f286bc6f1

Comment 14

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f75d3b5dc83b634357a5fd387629a5c956f6cc9

Comment 15

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=731d94e730078028e2f90cc876556eb2b3739975

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/9414b7e93b3570b3e4209992a3adf3c94e388dc1
https://hg.mozilla.org/mozilla-central/rev/9414b7e93b35

Comment 17

[Andrew McCreight [:mccr8]]

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.

Comment 18

[Andrew McCreight [:mccr8]]

(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 19

[Brad Werth [:bradwerth]]

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

Comment 20

[Brad Werth [:bradwerth]]

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

Comment 21

[Brad Werth [:bradwerth]]

(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.

Comment 22

[Andrew McCreight [:mccr8]]

(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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

FYI, ESR60 will need a rebased patch.

Comment 24

[Brad Werth [:bradwerth]]

(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.

Comment 25

[Brad Werth [:bradwerth]]

Attachment: Bug1538042Part1-esr60.patch

Comment 26

[Brad Werth [:bradwerth]]

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:

Comment 27

[Pascal Chevrel:pascalc]

(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!

Comment 28

[Brad Werth [:bradwerth]]

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.

Comment 29

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

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.

Comment 30

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

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

Nevermind, I found the ESR60 patch. :-)

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/828f8931afe0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/7405c40534e0

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241254735&repo=mozilla-esr60&lineNumber=24156

https://hg.mozilla.org/releases/mozilla-esr60/rev/a238b04b4addc802ab3d68573bebbea57594fe3a

Comment 34

[Brad Werth [:bradwerth]]

Attachment: 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:

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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

You don't need to re-request approval when rebasing a patch. Thanks, though :)

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/4774268f333c

Comment 37

[Brad Werth [:bradwerth]]

Is it now appropriate to land the tests?

Comment 38

[Andrew McCreight [:mccr8]]

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.

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Backed out for causing perma failures on test_nsFind.html:
https://hg.mozilla.org/integration/autoland/rev/a5ef15be200da67ba537f9072b2b76dca37fb6d2

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=248815064&repo=autoland

Comment 40

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4375c5d4832e55b7b24e36fff223d404cbf3f7c3

Comment 41

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd92de24bd4e716ab483bf9aafd7f4f552d22309

Comment 42

[BugBot [:suhaib / :marco/ :calixte]]

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

Comment 43

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd5671ef10a59645b8c080e70053c0ca5506638b

Comment 44

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0ee2cb5c9affe31aa3e175ebb9d8c65fa96b3c5

Comment 45

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57d8aebe2c1f9351f7f1791b8d237f35203f71c2

Comment 46

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=902c1eab38c8524a907663d8ef89a1a08daf95ba

Comment 47

[Brad Werth [:bradwerth]]

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.

Comment 48

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 49

[Brad Werth [:bradwerth]]

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?

Comment 50

[Emilio Cobos Álvarez (: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.

Comment 51

[Brad Werth [:bradwerth]]

(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.

Comment 52

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 53

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0010b4ae1f38f9d4043c31a64679d59839c50659

Comment 54

[Brad Werth [:bradwerth]]

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 55

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

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

sec-approval+ for mozilla-central.

Comment 56

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Part 2: Add tests of null character searches. r=mccr8
https://hg.mozilla.org/integration/autoland/rev/e81772a3f304128b06ddc20c3fb0539904e2984f
https://hg.mozilla.org/mozilla-central/rev/e81772a3f304