247562 2022-11-07 01:13:58 -0800 Intl.Locale.prototype.hourCycles leaks empty JSValue to script 2022-11-08 19:55:48 -0800 1 1 1 Unclassified WebKit JavaScriptCore WebKit Local Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 saelo d_degazio bfulgham darin d_degazio mark.lam webkit-bug-importer ysuzuki oldest_to_newest 1910717 0 saelo 2022-11-07 01:13:58 -0800 The following program crashes JSC, both local builds from HEAD and in Safari 16.1 function main() { const v24 = new Intl.Locale("trimEnd", { 'numberingSystem': "foobar" }); let empty = v24.hourCycles; print(empty); // use console.log in Safari } main(); The issue seems to be that the intlLocalePrototypeGetterHourCycles function [1] doesn't check that the return value of `locale->hourCycles(globalObject))` isn't nullptr and so may end up leaking the empty JSValue to script. I'm not sure if this can be abused to cause memory corruption. The empty JSValue is used for a few internal things, for example to indicate holes in Arrays (so if the leaked empty value is assigned to an array element, that will now appear as a hole) or to signal failure cases in C++ code [2]. As such, it may be possible to confuse some other code with this empty value, but I couldn't find an obvious case where this would then cause memory corruption. [1] https://github.com/WebKit/WebKit/blob/443de8e8e9716b164f0a25da35e9235efd72d4dd/Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp#L265 [2] https://github.com/WebKit/WebKit/blob/443de8e8e9716b164f0a25da35e9235efd72d4dd/Source/JavaScriptCore/runtime/JSCJSValue.h#L449 1910718 1 webkit-bug-importer 2022-11-07 01:14:12 -0800 <rdar://problem/102031379> 1910785 2 darin 2022-11-07 08:48:57 -0800 Looking at IntlLocale::hourCycles and other functions in the file I see that not only do they return nullptr, but they do so without using the RELEASE_AND_RETURN macro. I think that is also incorrect. There are three options here for behavior for these cases returning nullptr: 1) Raise a JavaScript exception. In that case the return value doesn't matter and an empty value would be OK. 2) Return a JavaScript undefined. 3) Return a JavaScript null. Not sure whether we should fix this in the bindings or in the IntlLocale class itself. 1910786 3 darin 2022-11-07 08:50:38 -0800 OK, I studied this more closely. The other places that return nullptr are doing fine because they all raise exceptions. It’s only this one place that has it wrong, when udatpg_open fails. I believe the correct fix is to add this line of code before the "return nullptr": throwTypeError(globalObject, scope, "invalid locale"_s); 1910787 4 darin 2022-11-07 08:53:50 -0800 This code with the mistake was introduced in bug 227830. 1911111 5 d_degazio 2022-11-08 14:48:59 -0800 Pull request: https://github.com/WebKit/WebKit/pull/6266 1911167 6 ews-feeder 2022-11-08 19:55:46 -0800 Committed 256473@main (86fbeb6fcd63): <https://commits.webkit.org/256473@main> Reviewed commits have been landed. Closing PR #6266 and removing active labels.