Closed Bug 1634738 (CVE-2020-12425) Opened 5 months ago Closed 4 months ago

1 byte OOB read in Date.parse

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox-esr68 --- wontfix
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- fixed

People

(Reporter: brunokeith2, Assigned: Waldo)

Details

(Keywords: csectype-bounds, sec-low, Whiteboard: [post-critsmash-triage][adv-main78+])

Attachments

(18 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
209 bytes, text/plain
Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.122 Safari/537.36

Steps to reproduce:

I found a 1-byte OOB bug in the implementation of Date.parse in Spidermonkey.

When calling Date.parse in JavaScript, Spidermonkey will convert the input object to a JSLinearString where the string is represented inside a contiguous buffer and the core implementation will iterate over the string to parse it. This is implemented in ParseDate<unsigned char> in gecko-dev/js/src/jsdate.cpp:1093.

The implementation consists of a big while loop guarded by a i < length check where i is the current index inside the string buffer. However the implementation is buggy and can lead to a 1-byte OOB read under specific circumstances

The core of the loop looks like the following:

	   while (i < length) {
	     int c = s[i];
	     i++; [[ 1 ]]
	     if (c <= ' ' || c == ',' || c == '-') {
	       if (c == '-' && '0' <= s[i] && s[i] <= '9') { [[ 2 ]]
	         prevc = c;
	       }

At [[ 1 ]], the index is incremented and based on what the value of the currently read character is, the code will try to look-up the next character to determine what it is parsing. However it does not check in the particular code-path where c is '-' that i has not become greater than length therefore [[ 2 ]] will read 1-byte OOB of the string buffer.

The simplest PoC that highlights the issue is the following code:

s = "-".repeat(0x100000);
Date.parse(s);

Running this inside the shell should instantly crash the process with a SIGSEGV.

As far as security impact, I think this should be classified as low impact as at best it could potentially maybe be used as an OOB 1-byte oracle.

(While getting a bit ahead, if this is considered for a potential bounty, I want to point out that I have previously received bounty payments from Mozilla but that information is now outdated and not valid anymore as I operate under my own company now)

Actual results:

The process crashed by reading OOB at an unmapped memory address

Expected results:

Nothing should happen, the code for Date.parse should handle that case gracefully

Group: core-security → javascript-core-security
Keywords: csectype-bounds

Ugh, we should at least use RangedPtr<> in this code...

I failed to reproduce the issue with the simple PoC. However, I did not try with an ASan build nor valgrind.
In the mean time setting P1 & S2.

Severity: normal → S2
Flags: needinfo?(jdemooij)
Priority: -- → P1

The minimal PoC segfaults immediately for me on Mac, even without valgrind.

So I assumed it would just work in its simplest form, no questions asked because that what it does for me (for reference I am using a Linux build of spidermonkey).

However it is in theory possible that the read happens silently OOB in another mapped page, maybe calling the trigger repeatedly in a loop would allow you to witness the crash?

This is a very old bug which comes back from at least from 1998 !
No "regressed-by" flag then.

I wonder, this bug might come from the time we moved away from having a zero-ending string.
Jan, would you remember when this was?

(In reply to Nicolas B. Pierron [:nbp] from comment #6)

I wonder, this bug might come from the time we moved away from having a zero-ending string.
Jan, would you remember when this was?

That was bug 1330776 but it's unrelated: the ensureLinear call in date_parse has been there for at least 10 years AFAICT (before bug 1330776, external strings didn't require null-termination either).

Waldo, maybe this is something you could look at?

Flags: needinfo?(jdemooij) → needinfo?(jwalden)
Keywords: sec-low

External strings may not have had assertions to require null-termination of their external contents, but stuff like e.g. memory accounting seems to have included a terminator. Of course, the API never documents any requirement of null-termination.

It may well be possible that some caller of JS_NewExternalString used to exist that would pass in non-null-terminated contents -- and with the character past the limit being outside the allocation containing the passed-in memory. External strings are not presently used in our tree other than for tests. They also were similarly unused in ESR 68. Similarly ESR 60. ESR 45 appears to be the most recent readily-accessed/searchable source that has a JS_NewExternalString use. And there, only for one case in XPC conversion for literal strings, which appear to be exactly C string literals if I look at all instances and make certain assumptions about the callers based on function name.

I think this probably is an issue that became observable only once bug 1330776 was fixed. Looking at how to patch now...

Sigh, apparently JS_NewMaybeExternalString is the function everyone actually uses to create external strings. Ignore most of the previous comment.

Flags: needinfo?(jwalden)

(In reply to Jeff Walden [:Waldo] from comment #8)

I think this probably is an issue that became observable only once bug 1330776 was fixed. Looking at how to patch now...

External strings didn't require null-termination for some time before bug 1330776 was fixed (there was an ensureFlat method on them to create a null-terminated string - removing that complexity was one of the motivations for fixing bug 1330776). That said, given how DOM strings are implemented, it's possible this could result in correctness bugs but not crashes because these external strings are always part of a bigger string...

If they wouldn't be crash bugs, they wouldn't be correctness bugs -- it's a read one past the end, but that read is immediately followed by exiting the loop and never using the value that was read. If the read is from actually allocated memory, you're going to get something and you will not be invoking undefined behavior, but then you're never going to use it so it doesn't even matter. If the read is not from allocated memory, that's the only way things go awry. (Granted I am assuming the compiler isn't going to escalate a read-past-the-end into nasal demons rather than a page fault or just getting a character from some unrelated memory allocation that is promptly ignored. So it goes, too often, with UB.)

I have started to tease apart the ParseDate function locally with an eye to making the i++ not happen until the end of each iteration of the loop. But first, the Augean stables need the start of some paths dug through them. And even with the paths, the final patch to make the increment not happen is going to be...tricky, given all the places that depend on the semantically-precise pointing i does right now. Then end of day all the patches will probably have to be folded together and stripped of all comments, for obfuscatory purposes and paranoia... :-|

(The point of moving the i++ to the end of loop being to ensure that the loop's i < length is always checked before that increment; other increments in the loop will be written so similar checks are obvious. Better abstractions could be used for all of this, to be sure, but none of it seems worth the trouble/time to me.)

https://dxr.mozilla.org/classic/source/js/src/jsdate.c#558 for anyone else who wants to see this code as it very similarly existed in 1998. I cannot comprehend how this code was human-authored; it resembles no grammar, no intelligently designed semantics I could ever imagine. Maybe Netscape hired infinite monkeys as contractors?

Assignee: nobody → jwalden
Status: NEW → ASSIGNED

Depends on D74783

Incidentally, in the course of working on this, I have discovered some fascinating behaviors:

js> new Date("May 11, 2020 12:00 AM").toString()
"Mon May 11 2020 00:00:00 GMT-0700 (Pacific Daylight Time)"
js> new Date("May 11, 2020 12:00 AM PM").toString()
"Mon May 11 2020 12:00:00 GMT-0700 (Pacific Daylight Time)"
js> new Date("May 11, 2020 12:00 AM PM AM").toString()
"Mon May 11 2020 00:00:00 GMT-0700 (Pacific Daylight Time)"
js> new Date("May 11, 2020 12:00 AM PM AM PM").toString()
"Mon May 11 2020 12:00:00 GMT-0700 (Pacific Daylight Time)"

(This ^ is actually compatible with v8!)

js> new Date("Ma 11, 2020").toString()
"Mon May 11 2020 00:00:00 GMT-0700 (Pacific Daylight Time)"

(This is not -- v8 requires at least the first three characters be present.)

js> new Date("Mayz 11, 2020").toString()
"Invalid Date"

(On the other hand, v8 literally only cares about the first three characters -- the above is "Mon May 11 2020 00:00:00 GMT-0700 (Pacific Daylight Time)" in v8.)

And then there's a bunch of other bizarro math I haven't been able to quickly observe.

I would like to return to this all at some point to maybe prune some of the crazier edge cases, actually enforce range limits on unbounded-value fields like years, eliminate prevc and fully handle each field on the basis of its leading character, etc. with an eye toward writing out an algorithm that is actually understandable and sensible and even standardizable (or in such shape that adding other engines' extensions to it would produce something we could standardize). Not quite today, tho.

Attached file Bug 1634738. r=jandem!

Queued up for landing.

I didn't actually end up removing all the comments and whitespace from the final landed patch, because when I looked at the full diff, I concluded removing all that stuff actually made more clear the things that had changed, and therefore made it likelier someone reading the patch would quickly figure out what was the important part of it. In principle the only way to avoid that would have been with a cover bug so no one would even think to look...but for a one-character overread that's then never used, it seemed overkill. So, I landed everything collapsed without further changes (except for the nitpicks mentioned in the separate reviews).

Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

The patch landed in nightly and beta is affected.
:Waldo, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(jwalden)

I'm inclined to say no. It's a one-character (i.e. one or two bytes) read past the end of a string (with a resulting value that is never used), which may be into allocated or unallocated memory. If allocated, there is no problem. If not allocated, well, in theory the compiler can escalate that to full UB and do whatever, but in practice it's hard to imagine that optimized code would go further awry than accessing an unallocated page and faulting. It was minor enough to just land without approval, it's minor enough to ride trains.

Flags: needinfo?(jwalden)
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main78+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.