1487098 - AddressSanitizer: heap-buffer-overflow [@ mozEnglishWordUtils::FindNextWord] with off-by-two read

Status: VERIFIED FIXED

(Core :: Spelling checker, defect)

Description

[Christian Holler (:decoder)]

I've built a fuzzing interface target for mozEnglishWordUtils::FindNextWord because this code itself looks a little complex and it calls into the mozTXTToHTML code in necko. I immediately got an ASan crash due to an out-of-bounds read. At first I thought it was me passing in the wrong parameters, but looking at the code, the code indeed does not look right to me:

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/extensions/spellcheck/src/mozEnglishWordUtils.cpp#59

After that loop, it is possible that `p == endBuf` and `endBuf` is past the buffer we initially get. Nevertheless, we read `*p` right after that:

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/extensions/spellcheck/src/mozEnglishWordUtils.cpp#70

This is an off-by-two at most and it might be harmless, but I wonder why we didn't see this earlier. If our strings happen to be NUL-terminated then that would explain why it doesn't complain (but still the code wouldn't be right).

I've fixed this for now by inserting the following code right after the while loop:

>    if (p >= endbuf) {
>      *begin = -1;
>      *end = -1;
>      return NS_OK;
>    }

but I don't know if this is the right solution.

The following trace is on mozilla-central revision 9c13dbdf4cc9 (the line number is missing on the top frame, I don't know why, but I confirmed locally that it is crashing on the *p read):

==16643==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000047f12 at pc 0x7efc5b9c82bd bp 0x7ffcf3caa0d0 sp 0x7ffcf3caa0c8
READ of size 2 at 0x602000047f12 thread T0
    #0 0x7efc5b9c82bc in mozEnglishWordUtils::FindNextWord(char16_t const*, unsigned int, unsigned int, int*, int*) extensions/spellcheck/src/mozEnglishWordUtils.cpp
    #1 0x7efc5e48bff0 in LLVMFuzzerTestOneInput(unsigned char const*, unsigned long) extensions/spellcheck/tests/fuzztest/SpellcheckFNW_fuzzer.cpp:23:25
    #2 0x5d8a1d in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
[...]

0x602000047f12 is located 0 bytes to the right of 2-byte region [0x602000047f10,0x602000047f12)
allocated by thread T0 here:
    #0 0x4e1eb0 in malloc (objdir-ff-libfuzzer/dist/bin/firefox+0x4e1eb0)
    #1 0x52001d in moz_xmalloc memory/mozalloc/mozalloc.cpp:70:17
    #2 0x5d892b in operator new[] objdir-ff-libfuzzer/dist/include/mozilla/mozalloc.h:148:12
    #3 0x5d892b in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) tools/fuzzing/libfuzzer/FuzzerLoop.cpp:508
[...]

SUMMARY: AddressSanitizer: heap-buffer-overflow extensions/spellcheck/src/mozEnglishWordUtils.cpp in mozEnglishWordUtils::FindNextWord(char16_t const*, unsigned int, unsigned int, int*, int*)
Shadow bytes around the buggy address:
  0x0c0480000fd0: fa fa 00 00 fa fa 00 00 fa fa fd fa fa fa fd fa
=>0x0c0480000fe0: fa fa[02]fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480000ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Jonathan, is this maybe something you'd have cycles to look at?

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Christian Holler (:decoder) from comment #0)

> https://searchfox.org/mozilla-central/rev/
> 55da592d85c2baf8d8818010c41d9738c97013d2/extensions/spellcheck/src/
> mozEnglishWordUtils.cpp#59
> 
> After that loop, it is possible that `p == endBuf` and `endBuf` is past the
> buffer we initially get.

Yup, agreed.

> Nevertheless, we read `*p` right after that:
> 
> https://searchfox.org/mozilla-central/rev/
> 55da592d85c2baf8d8818010c41d9738c97013d2/extensions/spellcheck/src/
> mozEnglishWordUtils.cpp#70
> 
> This is an off-by-two at most and it might be harmless, but I wonder why we
> didn't see this earlier. If our strings happen to be NUL-terminated then
> that would explain why it doesn't complain (but still the code wouldn't be
> right).

Yes, in practice I think it has a good chance of being harmless -- but I wouldn't want to count on that! (It's possible that by auditing callers we might be able to confirm that it's not exploitable at this point, but we should just fix it anyhow.)

> I've fixed this for now by inserting the following code right after the
> while loop:
> 
> >    if (p >= endbuf) {
> >      *begin = -1;
> >      *end = -1;
> >      return NS_OK;
> >    }
> 
> but I don't know if this is the right solution.

I don't think that's right; if I am following the code here, it could lead to the spell-check ignoring a word that we actually want to handle.

However, I think there's an easy fix. The problematic line 70:

    if ( (*p == ':' || *p == '@' || *p == '.') &&  p < endbuf - 1) {

already includes a test of p that would confirm it is valid, but unfortunately it does that -after- it has already dereferenced the pointer. So if we just reorder the two sides of the && operator here, as:

    if (p < endbuf - 1 && (*p == ':' || *p == '@' || *p == '.') ) {

there's no longer a problem: if p has reached endbuf we won't even try to dereference it.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Reorder boolean expression to take advantage of short-circuiting

AFAICS this should fix the problem; Christian, could you confirm that? Thanks.

Comment 5

[Christian Holler (:decoder)]

Comment on attachment 9005668 [details] [diff] [review]
Reorder boolean expression to take advantage of short-circuiting

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

Yes, I think that should work out. Thanks!

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 9005668 [details] [diff] [review]
Reorder boolean expression to take advantage of short-circuiting

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't think this is exploitable, except as a possible DoS if someone could come up with a case where the out-of-bounds read causes a segfault. Note that when we do read a (garbage) value from *endbuf, that value will end up being ignored because the second part of the && condition must be false.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch makes it pretty obvious we're rearranging the code to test that |p| is in range before dereferencing it, but it's not clear what further implications this has.

Which older supported branches are affected by this flaw?
All

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should backport cleanly, I expect.

How likely is this patch to cause regressions; how much testing does it need?
Minimal risk, just rearranges the condition to avoid dereferencing |p| if it may be out of bounds.

Comment 7

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

Comment on attachment 9005668 [details] [diff] [review]
Reorder boolean expression to take advantage of short-circuiting

This seems safe enough to take on trunk right now. Sec-approval+ for trunk.

Comment 8

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dedd5c14209a8a120ad837784362c5c8539d8784

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/dedd5c14209a

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 9005668 [details] [diff] [review]
Reorder boolean expression to take advantage of short-circuiting

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: possible (read) access beyond array bounds, could result in crash
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): minimal, just re-orders conditions so that we check the pointer is in-bounds before dereferencing it rather than after
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 11

[Cornel Ionce [:noni] [Hubs QA]]

Confirming this fix for Fx 63.0b7 with the attached testcase.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9005668 [details] [diff] [review]
Reorder boolean expression to take advantage of short-circuiting

Fixes a sec-high, approved for ESR 60.3.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Cornel Ionce [:noni] [Hubs QA]]

Also confirming the fix using the latest build available on taskcluster: https://tools.taskcluster.net/index/gecko.v2.mozilla-esr60.latest.firefox