1938615 - Assertion failure: this->is<T>(), at js/src/vm/JSObject.h:498

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Nils Bars]

Steps to reproduce:

Version: 43ce3de32b3a946bceac74c3badf442af9f245c0
Args: js --fuzzing-safe <test-case>
Test case:

function a() {
    b = this
    b[0] = b
}
Object.prototype.__defineSetter__(1, a)
c = JSON.stringify([
    ,
    2.2250738585072014e308
])
JSON.parse(c, BigInt64Array)

Actual results:

Assertion failure: this->is<T>(), at js/src/vm/JSObject.h:498

#0 0x5642c0586154 in js::ParseRecordObject& JSObject::as<js::ParseRecordObject>() js/src/vm/JSObject.h:498:5
#1 0x5642c058455f in InternalizeJSONProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, JS::Handle<js::ParseRecordObject*>, JS::MutableHandle<JS::Value>) js/src/builtin/JSON.cpp:1844:47
#2 0x5642c0552549 in Revive(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::ParseRecordObject*>, JS::MutableHandle<JS::Value>) js/src/builtin/JSON.cpp:1951:10
#3 0x5642c0551cfd in bool js::ParseJSONWithReviver<unsigned char>(JSContext*, mozilla::Range<unsigned char const>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) js/src/builtin/JSON.cpp:1981:12
#4 0x5642c05886e0 in json_parse(JSContext*, unsigned int, JS::Value*) js/src/builtin/JSON.cpp:2026:16
#5 0x5642c046af4e in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) js/src/vm/Interpreter.cpp:532:13
#6 0x5642c046a21c in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) js/src/vm/Interpreter.cpp:628:12
#7 0x5642c0483c04 in js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason) js/src/vm/Interpreter.cpp:700:10
#8 0x5642c0483c04 in js::Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:3338:16
#9 0x5642c04691f1 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:502:13
#10 0x5642c046e431 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, js::AbstractFramePtr, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:893:13
#11 0x5642c046ec3c in js::Execute(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSObject*>, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:926:10
#12 0x5642c06667d9 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::MutableHandle<JS::Value>) js/src/vm/CompilationAndEvaluation.cpp:601:10
#13 0x5642c0666a57 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) js/src/vm/CompilationAndEvaluation.cpp:625:10
#14 0x5642c03c6618 in RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool, bool) js/src/shell/js.cpp:1325:10
#15 0x5642c03c5700 in Process(JSContext*, char const*, bool, FileKind) js/src/shell/js.cpp
#16 0x5642c03821f1 in ProcessArgs(JSContext*, js::cli::OptionParser*) js/src/shell/js.cpp:11821:10
#17 0x5642c03821f1 in Shell(JSContext*, js::cli::OptionParser*) js/src/shell/js.cpp:12089:12
#18 0x5642c0378bbd in main js/src/shell/js.cpp:12515:12
#19 0x7f05b5c233b7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#20 0x7f05b5c2347a in __libc_start_main csu/../csu/libc-start.c:360:3
#21 0x5642c033fb78 in _start (fuzzbuild/dist/bin/js+0x1c4eb78) (BuildId: a084412fdeb1bfc37ecd0b649c348fdf)

Comment 1

[Jan de Mooij [:jandem]]

I see ParseRecordObject in the stack trace so over to Bryan :)

Comment 2

[Bryan Thrall [:bthrall]]

Attachment: Bug 1938615 - Use null prototype for ParseRecordEntry r=mgaudet!

The prototype chain for ParseRecordEntry allows scripts to interfere with how
JSONReviveHandler tracks object and array parse information.

Comment 3

[Bryan Thrall [:bthrall]]

Attachment: Bug 1938615 - Store entries in slot r=mgaudet!

We already have to allocate the EntryMap object, so we aren't saving anything
by storing the entries directly on the ParseRecordObject and might even be
slowing things down by copying over the properties.

Comment 4

[Bryan Thrall [:bthrall]]

Attachment: Bug 1938615 - Remove ParseRecordEntry typedef r=mgaudet!

It was an extra layer of naming that I think is making it harder to understand
what type is being used.

Comment 5

[Phabricator Automation]

Comment on attachment 9446167 [details]
Bug 1938615 - Store entries in slot r=mgaudet!

Revision D233442 was moved to bug 1940551. Setting attachment 9446167 [details] to obsolete.

Comment 6

[Phabricator Automation]

Comment on attachment 9446168 [details]
Bug 1938615 - Remove ParseRecordEntry typedef r=mgaudet!

Revision D233443 was moved to bug 1940551. Setting attachment 9446168 [details] to obsolete.

Comment 7

[Bryan Thrall [:bthrall]]

See bug 1940560 for a simplified test case.

Comment 8

[Bryan Thrall [:bthrall]]

Comment on attachment 9446166 [details]
Bug 1938615 - Use null prototype for ParseRecordEntry r=mgaudet!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is pretty simple to cause type confusion in the JSON.parse code (see bug 1940560's test case), where a JSObject (which should be completely hidden from user scripts) can be confused for a NativeObject (ParseRecordObject). I'm not sure how significant this problem is.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: beta
• If not all supported branches, which bug introduced the flaw?: Bug 1925128
• 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?: It needs minimal testing, and is very unlikely to cause regressions because it affects a feature (JSON.parse with source) that was implemented under a disabled flag until Firefox 135.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Unknown

Comment 9

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1925128

Comment 10

[BugBot [:suhaib / :marco]]

The severity field for this bug is set to S3. However, the bug is flagged with the sec-high keyword.
:bthrall, could you consider increasing the severity of this security bug?

For more information, please visit BugBot documentation.

Comment 11

[BugBot [:suhaib / :marco]]

The bug is marked as tracked for firefox135 (beta) and tracked for firefox136 (nightly). However, the bug still has low priority and has low severity.

:sdetar, could you please increase the priority and increase the severity for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9446166 [details]
Bug 1938615 - Use null prototype for ParseRecordEntry r=mgaudet!

approved to land and uplift

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/731ad65d0088
Use null prototype for ParseRecordEntry r=mgaudet

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/731ad65d0088

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta uplift when you get a chance.

Comment 16

[Bryan Thrall [:bthrall]]

Comment on attachment 9446166 [details]
Bug 1938615 - Use null prototype for ParseRecordEntry r=mgaudet!

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: The "JSON.parse with source" feature will be enabled by default with the 135 release, and this uplift is needed to avoid enabling the feature with a security issue.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): This only affects users who have already manually enabled the "JSON.parse with source" feature (by protecting them from the security issue; other behavior does not change) and does not change behavior for other users.
• String changes made/needed:
• Is Android affected?: Yes

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9446166 [details]
Bug 1938615 - Use null prototype for ParseRecordEntry r=mgaudet!

Approved for 135.0b5.

Comment 18

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/514947dd16c1