1634738 - (CVE-2020-12425) 1 byte OOB read in Date.parse

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Bruno Keith]

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

Comment 1

[Jan de Mooij [:jandem]]

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

Comment 2

[Nicolas B. Pierron [:nbp]]

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.

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Bruno Keith]

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?

Comment 5

[Nicolas B. Pierron [:nbp]]

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

Comment 6

[Nicolas B. Pierron [:nbp]]

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?

Comment 7

[Jan de Mooij [:jandem]]

(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?

Comment 8

[Jeff Walden [:Waldo]]

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...

Comment 9

[Jeff Walden [:Waldo]]

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

Comment 10

[Jan de Mooij [:jandem]]

(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...

Comment 11

[Jeff Walden [:Waldo]]

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... :-|

Comment 12

[Jeff Walden [:Waldo]]

(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.)

Comment 13

[Jeff Walden [:Waldo]]

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?

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Add blank space and early-continues to ParseDate's main parsing loop. r=jandem!

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Add comments by most of the various sub-parsing algorithms in date parsing. r=jandem!

Depends on D74772

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Parse delimiter characters in clearer fashion. r=jandem!

Depends on D74773

Comment 17

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Use IsAsciiDigit rather than manual ASCII comparisons in date parsing. r=jandem!

Depends on D74774

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Rename the i variable in date parsing to index, for better readability/searchability. r=jandem!

Depends on D74775

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Rename the st variable in date parsing to start, for better readability/searchability. r=jandem!

Depends on D74776

Comment 20

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Move a couple static const arrays used only in |ParseDate|, immediately next to it. (|ParseDate| is a template instantiated multiple times, so they can't be moved inside it.) r=jandem!

Depends on D74777

Comment 21

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Combine the two static arrays in |ParseDate| into a single array. r=jandem!

Depends on D74778

Comment 22

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Continue early if the field doesn't match the current keyword. r=jandem!

Depends on D74779

Comment 23

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Handle days of the week by early exit. r=jandem!

Depends on D74780

Comment 24

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Handle AM/PM with an early break if encountered as field. r=jandem!

Depends on D74781

Comment 25

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Make month action values be the 1-based number of the month. r=jandem!

Depends on D74782

Comment 26

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Record time zone offsets. r=jandem!

Depends on D74783

Comment 27

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Rename |RegionMatches| to |IsPrefixOfKeyword|, and simplify it based on the characteristics of its input. r=jandem!

Depends on D74784

Comment 28

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Move |PrefixIsKeyword| into |ParseDate| directly. r=jandem!

Depends on D74785

Comment 29

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738 - Don't read one past the end of the string when parsing a date string that ends with '-'. r=jandem!

Depends on D74786

Comment 30

[Jeff Walden [:Waldo]]

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.

Comment 31

[Jeff Walden [:Waldo]]

Attachment: Bug 1634738. r=jandem!

Comment 32

[Jeff Walden [:Waldo]]

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).

Comment 33

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

https://hg.mozilla.org/integration/autoland/rev/d8ebfbf2c0fc852a21be6390455cf16d53705d7b
https://hg.mozilla.org/mozilla-central/rev/d8ebfbf2c0fc

Comment 34

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

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.

Comment 35

[Jeff Walden [:Waldo]]

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.

Comment 36

[Tom Ritter [:tjr]]

Attachment: advisory.txt