1895055 - (CVE-2024-5694) Assertion failure: (current[-1] == 'l', ...) , at vm/JSONParser.cpp:244

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[lukas.bernhard]

Steps to reproduce:

On git commit 38377227b8f96fda8f418db614e6a8aa67d01c31 the attached sample asserts in the js-shell when invoked as obj-x86_64-pc-linux-gnu/dist/bin/js --fast-warmup --fuzzing-safe --gc-zeal=14 crash.js
Bisecting points to commit 45832c17d5e63d06a6dee45033851041aff1ddea related to bug 1879918.

for (let v0 = 0; v0 < 100; v0++) {
    class C1 { }
    new C1();
    eval();
    const v17 = ensureLinearString(("{ \"phbbbbbbbbbbbbbbttt!!!!??\": [1] }").substr(0, ("{ \"phbbbbbbbbbbbbbbttt!!!!??\": [1] }").length / 2) + ("{ \"phbbbbbbbbbbbbbbttt!!!!??\": [1] }").substr(("{ \"phbbbbbbbbbbbbbbttt!!!!??\": [1] }").length / 2));
    ensureLinearString(newRope(newRope(v17, "\n"), "toLowerCase"));
    JSON.parse(v17);
}

#0  AssertPastValue<unsigned char> (current=...) at js/src/vm/JSONParser.cpp:237
#1  0x000055555762cb14 in js::JSONTokenizer<unsigned char, js::JSONPerHandlerParser<unsigned char, js::JSONFullParseHandler<unsigned char> >, js::JSONFullParseHandler<unsigned char>::StringBuilder>::advanceAfterProperty (this=this@entry=0x7fffffffcb58) at js/src/vm/JSONParser.cpp:250
#2  0x00005555575db517 in js::JSONPerHandlerParser<unsigned char, js::JSONFullParseHandler<unsigned char> >::parseImpl<JS::Rooted<JS::Value>, js::JSONParser<unsigned char>::parse(JS::MutableHandle<JS::Value>)::{lambda(JS::Handle<JS::Value>)#1}>(JS::Rooted<JS::Value>&, js::JSONParser<unsigned char>::parse(JS::MutableHandle<JS::Value>)::{lambda(JS::Handle<JS::Value>)#1}) (
    this=this@entry=0x7fffffffca78, value=..., setResult=setResult@entry=...) at js/src/vm/JSONParser.cpp:874
#3  0x00005555575db32b in js::JSONParser<unsigned char>::parse (this=0x7fffffffca78, vp=...) at js/src/vm/JSONParser.cpp:1070
#4  0x000055555734b895 in js::MutableWrappedPtrOperations<js::JSONParser<unsigned char>, JS::Rooted<js::JSONParser<unsigned char> > >::parse (this=0x7fffffffca60, vp=...)
    at js/src/vm/JSONParser.h:653
#5  ParseJSON<unsigned char> (cx=cx@entry=0x7ffff7439100, chars=..., vp=vp@entry=...) at js/src/builtin/JSON.cpp:1956
#6  0x000055555734b267 in js::ParseJSONWithReviver<unsigned char> (cx=cx@entry=0x7ffff7439100, chars=..., reviver=reviver@entry=..., vp=vp@entry=...) at js/src/builtin/JSON.cpp:1973
#7  0x00005555573ae972 in json_parse (cx=0x7ffff7439100, argc=<optimised out>, vp=<optimised out>) at js/src/builtin/JSON.cpp:2024
#8  0x0000082f78341be3 in ?? ()
#9  0x000000000000015f in ?? ()
#10 0x00007fffffffd3e8 in ?? ()
#11 0x00007ffff542f4e4 in ?? ()
#12 0x0000000000000000 in ?? ()

Comment 1

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

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

:sfink, since you are the author of the regressor, bug 1879918, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Steve Fink [:sfink] [:s:]]

This is caused by bug 1879918 but distinct from bug 1890909. I haven't fully it tracked it down yet, but this is an excellent fuzz bug to receive. It shows that there is yet another problem with the string bit handling.

Comment 3

[Steve Fink [:sfink] [:s:]]

The problem is that AutoStableStringChars of a dependent string does not fully prevent the string's chars from being changed (and the old pointer freed). It prevents the owning string from having its chars moved, but consider:

    AutoStableStringChars assc(cx);
    assc.init(dep);
    char* chars = assc.chars();
    minorgc();
    use(chars);

The chars pointer here will remain valid as long as the owning string doesn't get collected. ASSC contains a Rooted<JSString*> containing dep to prevent collection. However, during the minor GC dep is deduplicated to a different dependent string, and so the Rooted inside of assc now keeps a different owner alive — in particular, an owner that does not own the cached chars. So dep.base can be collected if nothing else keeps it alive, and thus the allocation containing chars can be freed.

There are two ways to fix this. The most straightforward is for ASSC to keep the owning string alive. It actually doesn't need to keep dep alive at all; all ASSC needs is for its cached chars pointer to continue to be valid, and that's all about the owning string. The original dependent string from which we pulled the pointer is neither necessary nor sufficient for maintaining the pointer's validity. The other fix would be to prevent dep from being deduplicated, since then its chars will stay consistent.

I'm choosing the first. Everything about AutoStableStringChars should be about the owning string. In fact, the first thing JSString::hasMovableChars() already does is walk up to the owning string, which should have given me a hint!

Comment 4

[Steve Fink [:sfink] [:s:]]

Regressed in bug 1879918 because I weakened MarkStringAndBasesNonDeduplicatable to PreventRootBaseDeduplication there. As in, it previously marked dep from the example in the previous comment as non-deduplicatable, but stopped doing that when I realized that it didn't strictly matter for other purposes. I missed that it was still necessary here, partly because it's kind of the wrong way to ensure the pointer's stability. (It's the 2nd approach described above.)

Comment 5

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1895055 - Hold onto owning string in AutoStableStringChars

Comment 6

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

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

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1895055 - test

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9400818 [details]
Bug 1895055 - Hold onto owning string in AutoStableStringChars

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It's a UAF, but I've only seen read accesses and the address isn't controllable and will always be in the string heap partition. So it isn't exploitable unless I missed something.
• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: the automation is right
• If not all supported branches, which bug introduced the flaw?: Bug 1879918
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: There are a bunch of changes in this area, so it might collide with something, but conceptually at least it's nicely separate. It won't be hard to backport.
• How likely is this patch to cause regressions; how much testing does it need?: I didn't notice the length problem until my reviewer pointed it out, but really there's not much happening here. It's a small change that doesn't matter unless the relatively rare triggering condition is happening, and then makes a very local change.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 9400818 [details]
Bug 1895055 - Hold onto owning string in AutoStableStringChars

approved to land and uplift

Comment 10

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1895055 - Hold onto owning string in AutoStableStringChars

Original Revision: https://phabricator.services.mozilla.com/D209877

Comment 11

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: Potential security issues (UAF)
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: Test is in a separate unlanded patch.
• Risk associated with taking this patch: fairly low
• Explanation of risk level: it closes a known hole that was recently introduced
• String changes made/needed: none
• Is Android affected?: yes

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6ef5f7f7ff9
Hold onto owning string in AutoStableStringChars r=jandem

Comment 13

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

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

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/646fe555b999

Comment 15

[Tom Ritter [:tjr]]

Based on the limits of the primitive, downgrading this to moderate.

Comment 16

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 17

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

2 months ago, tjr placed a reminder on the bug using the whiteboard tag [reminder-test 2024-07-23] .

sfink, please refer to the original comment to better understand the reason for the reminder.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2dc302b2f2f8
test

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/2dc302b2f2f8