1839007 - (CVE-2023-4578) Access Violation on IsPatternMatching -> JS_GetProperty -> JS_ForwardGetPropertyTo

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Irvan Kurniawan [:sourc7]]

Attachment: testcase.html

After allocate memory to exhaust the process virtual memory and execute invalid JS function inside try {} catch() {} and input pattern. On Firefox 32-bit it able to crash on JS_GetProperty on address 0x000083d8, more interestingly on Firefox 64-bit it able to crash on dynamic (changing) address including 0x00050512fab2b080, 0x000514975972b080, 0x000526aa3e72b080, and more..

Currently the testcase.html is fine-tuned to make it very reliably to reproduce on Firefox 114.0.1 (32-bit) on Windows 11, I tested this very reliably crash on JS_GetProperty on both my machine Intel i9-13900k and AMD Ryzen 7 5700G, it still able to crash on Firefox Nightly 116.0a1 (2023-06-16) (32-bit), but reduced success rate and have to adjust fillMemory code to make it very reliably on Nightly. For now I decided to submit this report and adjust that later.

Tested on:

• Firefox 114.0.1 (32-bit)
• Firefox 114.0.1 (64-bit)
• Firefox Nightly 116.0a1 (2023-06-16) (32-bit)
• Firefox Nightly 116.0a1 (2023-06-16) (64-bit)

Steps to reproduce:

1. Open Firefox 114.0.1 (32-bit)
2. Visit attached testcase.html
3. Wait ~20 seconds
4. Firefox tab crashed
5. Analyze the minidump using WinDbg
6. Firefox crash on JS_GetProperty -> JS_ForwardGetPropertyTo

Comment 1

[Irvan Kurniawan [:sourc7]]

Attachment: firefox-114.0.1-32-minidump.txt

Comment 2

[Irvan Kurniawan [:sourc7]]

Attachment: firefox-114.0.1-64-minidump.txt

Comment 3

[Irvan Kurniawan [:sourc7]]

Attachment: firefox-116.0a1-20230616-64-minidump.txt

Comment 4

[Andrew McCreight [:mccr8]]

Looking at the attached stacks, nsContentUtils::IsPatternMatching() is here:
ReportPatternCompileFailure(aPattern, aDocument, &error, cx);
I'm guessing that is on this line, because that's the only call to JS_GetProperty in the file:
if (!JS_GetProperty(cx, exnObj, "message", &messageVal)) {

I'm not sure how we're getting into IsPatternMatching, though.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Because of the testcase setting inputElement.pattern, so when then later value is updated using setRangeText, that triggers HasPatternMismatch() call, I believe through https://searchfox.org/mozilla-central/rev/4e8f62a231e71dc53eb50b6d74afca21d6b254e9/dom/html/HTMLInputElement.cpp#6800,6814

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Isn't this more a JS engine issue. I don't immediately see anything unusual on the DOM side.

Comment 7

[Andrew McCreight [:mccr8]]

I have no idea.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #6)

Isn't this more a JS engine issue. I don't immediately see anything unusual on the DOM side.

If this is unrelated to the DOM, would there be a way to convert the failing test case into one which does not rely on the DOM?

Comment 9

[Irvan Kurniawan [:sourc7]]

Ok great! I got the root cause of memory allocation failure that cause the access violation, I'll post the detail soon.

Comment 10

[Irvan Kurniawan [:sourc7]]

The testcase.html is set inputElement.pattern that invokensContentUtils::IsPatternMatching to do JS::CheckRegExpSyntax -> js::FrontendContext::convertToRuntimeError -> js::ErrorToException -> js::CopyErrorReport

On CopyErrorHelper it do calloc to allocate the uint8_t* cursor using cx->pod_calloc<uint8_t>(mallocSize); then when calloc failure, the cursor is set to null then as if (!cursor) { the function will return nullptr;

When the calloc is failure at first try, it look like the js::CopyErrorReport is also call to JSRuntime::onOutOfMemory to try do GC, but it still return the nullptr for the uint8_t* cursor. Then js::ErrorToException check if (!report) { it will call return

After the fc.convertToRuntimeErrorAndClear(); function is called, then the JS::CheckRegExpSyntax will return true; cause nsContentUtils::IsPatternMatching to pass the return Nothing() so it go inside if (!error.isUndefined()) { to call ReportPatternCompileFailure(aPattern, aDocument, &error, cx);, the function call JS_GetProperty(cx, exnObj, "message", &messageVal) -> JS_ForwardGetPropertyTo -> check -> checkImpl -> check -> compartment -> shape -> headerPtr then crash at get

Comment 11

[Nicolas B. Pierron [:nbp]]

Thanks for the report!
This indeed sounds like a JS engine issue then, and now I know who to forward it to.

Bryan it sounds like JS::CheckRegExpSyntax does not behave as described in its declaration, by discarding the error and returning true.

I suspect the problem comes from either not setting or clearing the FrontendContext OutOfMemory flag ahead of returning into JS::CheckRegExpSyntax.

Comment 12

[Bryan Thrall [:bthrall]]

I'll take a look!

Comment 13

[Irvan Kurniawan [:sourc7]]

(In reply to Irvan Kurniawan (:sourc7) from comment #0)

it still able to crash on Firefox Nightly 116.0a1 (2023-06-16) (32-bit), but reduced success rate and have to adjust fillMemory code to make it very reliably on Nightly.

Ok, after testing for a while, the attached testcase.html is also very reliable on Firefox Nightly 117.0a1 (2023-07-13) (32-bit), when testing 20 times on ffpuppet launcher I can get 20 times success, 0 failure on Intel i9-13900k and AMD Ryzen 7 5700G.

When attach the process with WinDbg, I able to capture the access-violation crash, on crash stack index [0xb] the xul!ReportPatternCompileFailure code if (!JS_GetProperty(cx, exnObj, "message", &messageVal)) { the exnObj variable exnObj->ptr->header_->value_ value already set to 0x83d8, then on stack [0x0] the xul!js::gc::HeaderWord::get code uintptr_t value = value_; the this local name is set to 0x83d8 which is the crash address.

Comment 14

[Bryan Thrall [:bthrall]]

It looks like the problem is that fc.convertToRuntimeErrorAndClear(); allocates memory, so it can trigger OOM.

This can cause JS::CheckRegExpSyntax() to return true when it should return false in the following scenario:

1. JS::CheckRegExpSyntax() is called with a string that has invalid syntax
2. irregexp::CheckPatternSyntax() returns false and sets SyntaxError on the FrontendContext
3. JS::CheckRegExpSyntax() follows the failure path and calls fc.convertToRuntimeErrorAndClear()
4. convertToRuntimeErrorAndClear() triggers OOM and sets a pending OOM exception on the JSContext
5. JS::CheckRegExpSyntax() extracts the OOM exception, thinking it is the SyntaxError, and returns true

I should have a patch to correctly check for OOM in convertToRuntimeErrorAndClear() soon.

Thanks for the detailed analysis, :sourc7, it was really helpful in diagnosing the problem!

Comment 15

[Bryan Thrall [:bthrall]]

Attachment: Bug 1839007 - Return bool from ErrorToException r=arai

It is simpler to think about ErrorToException returning true when the error is
correctly converted, which is why it returns false on recursion even though you
could argue that nothing went wrong in that case.

Comment 16

[Bryan Thrall [:bthrall]]

Attachment: Bug 1839007 - Return bool from FrontendContext::convertToRuntimeErrorAndClear r=arai

Depends on D184160

Comment 17

[Bryan Thrall [:bthrall]]

Attachment: Bug 1839007 - Return false if converting to a runtime error triggers OOM r=arai

In that case, the error stored in cx is an OOM rather than the expected
SyntaxError. CheckRegExpSyntax should return false in that case.

Depends on D184161

Comment 18

[Bryan Thrall [:bthrall]]

Attachment: Bug 1839007 - Add CheckRegExpSyntax jit-test r=arai

Depends on D184162

Comment 19

[Bryan Thrall [:bthrall]]

Landing patches D184160, D184161, and D184162 without sec-approval because this is marked sec-moderate.

Patch D184254 contains tests and will be landed at a later time, according to security procedure.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/599e241178d6
Return bool from ErrorToException r=arai
https://hg.mozilla.org/integration/autoland/rev/880d3a8b52ba
Return bool from FrontendContext::convertToRuntimeErrorAndClear r=arai
https://hg.mozilla.org/integration/autoland/rev/ea812280de66
Return false if converting to a runtime error triggers OOM r=arai

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/599e241178d6
https://hg.mozilla.org/mozilla-central/rev/880d3a8b52ba
https://hg.mozilla.org/mozilla-central/rev/ea812280de66

Comment 22

[Phabricator Automation]

Comment on attachment 9345111 [details]
Bug 1839007 - Add CheckRegExpSyntax jit-test r=arai

Revision D184254 was moved to bug 1845132. Setting attachment 9345111 [details] to obsolete.

Comment 23

[Daniel Veditz [:dveditz]]

We're going to leave the severity at moderate because so far the read access violation is only for a fixed offset (slightly varying with the base allocation pointer in 64 bits, but that doesn't appear to be under your control). If there's a way to get more control over the read address we'd consider raising the severity.

Comment 24

[Bryan Thrall [:bthrall]]

Comment on attachment 9344931 [details]
Bug 1839007 - Return bool from ErrorToException r=arai

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a small change that improves content tab stability in low-memory situations and there is a low risk of applying it.
• User impact if declined: There is a chance of crashing the content tab in low-memory situations when the JavaScript executes an invalid regular expression.
• Fix Landed on Version: 117
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change only effects the code path in the case of an invalid regular expression, so only websites with broken regular expressions could potentially see any change in behavior. The change is also isolated to a small number of files and applies cleanly to the esr115 release commit.

Comment 25

[Hani Yacoub, Desktop QA]

Managed to reproduce the crash on Firefox Firefox 114.0.1 (32-bit) and on Firefox Nightly 116.0a1 (2023-06-16) (32-bit) on Windows 11 and Windows 10.
The crash is still reproducible on the latest Firefox Nightly 118.0a1 (2023-08-04) x32 on both Windows 11/10.

Bryan, could you please take a look at this to see if it's also reproducible on your side?
Thanks.

Comment 26

[Bryan Thrall [:bthrall]]

Unfortunately, the testcase for this bug eats up all the memory available to the tab, so the tab will crash even if the bug is fixed. I'm sorry for not making that clear earlier!

Does Firefox have different behavior if a tab crashes because of OOM vs. other crashes? That might be one way to verify the bug is fixed.

I'm also looking at verifying that the code behaves as I expect on current mozilla-central using a debugger.

Comment 27

[Hani Yacoub, Desktop QA]

I didn't see any different behavior between the two builds, they even crashed at approximative the same time.

Could you please mark this bug as verified after verifying that the code behaves as I expect on current mozilla-central using a debugger?
Thanks.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9344931 [details]
Bug 1839007 - Return bool from ErrorToException r=arai

Approved for 115.2esr.

Comment 29

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/e2eaa313a346
https://hg.mozilla.org/releases/mozilla-esr115/rev/f03409ee57eb
https://hg.mozilla.org/releases/mozilla-esr115/rev/301266ba9108

Comment 30

[Bryan Thrall [:bthrall]]

I couldn't get the attached testcase to reproduce the same memory failure as it originally did, but I was able to use a debugger to do so and verify that mozilla-central, including the patches for this bug, handles the OOM correctly now.

Specifically, when an OOM happens while converting the FrontendContext error to a JSContext error, JS::CheckRegExpSyntax() returns false, which is the documented behavior.

Comment 31

[Hani Yacoub, Desktop QA]

Removing qa-verify+ since we couldn't verify this issue on our end.

Comment 32

[June Wilde (she/her) [:jewilde]]

Attachment: advisory.txt

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e55f2c5a7f0b
Add CheckRegExpSyntax jit-test r=arai

Comment 34

[Ryan VanderMeulen [:RyanVM]]

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