Bug 196631 – [JSC] makeBoundFunction should not assume incoming "length" value is Int32 because it performs some calculation in bytecode

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 366780 [details]
Patch

[Yusuke Suzuki]

Double prediction injection found this issue.

[Yusuke Suzuki]

Created attachment 366783 [details]
Patch

[EWS Watchlist]

Attachment 366783 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:11:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Created attachment 366784 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 366784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366784&action=review

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:257
> +    RELEASE_AND_RETURN(scope, JSValue::encode(JSBoundFunction::create(vm, exec, globalObject, target, boundThis, boundArgs.isCell() ? jsCast<JSArray*>(boundArgs) : nullptr, length, name)));

I'll change `name` part to `WTFMove(name)`

[EWS Watchlist]

Attachment 366784 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:15:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 366784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366784&action=review

> Source/JavaScriptCore/runtime/RandomizingFuzzerAgent.cpp:47
> +    if (Options::injectDoublePredictionsForNumbers()) {
> +        if (original && mergeSpeculations(original, SpecBytecodeNumber) == SpecBytecodeNumber)
> +            return SpecBytecodeDouble;
> +        return original;
> +    }

Can we make this a different fuzzer agent? It's no longer random.

[Saam Barati]

Comment on attachment 366784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366784&action=review

> Source/JavaScriptCore/runtime/Options.h:439
> +    v(bool, injectDoublePredictionsForNumbers, false, Normal, nullptr) \

Then this can be called "useDoublePredictionFuzzerAgent" or something similar.

[Yusuke Suzuki]

Comment on attachment 366784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366784&action=review

>> Source/JavaScriptCore/runtime/RandomizingFuzzerAgent.cpp:47
>> +    }
> 
> Can we make this a different fuzzer agent? It's no longer random.

Yeah, I'll change to do that.

[Yusuke Suzuki]

Created attachment 366788 [details]
Patch

[EWS Watchlist]

Attachment 366788 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:15:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer, fuzzer, fuzzer, fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 366788 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366788&action=review

r=me

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:251
> +    int32_t length = lengthValue.toInt32(exec);

Should assert it's a number. Maybe even isAnyInt and that it's within 32-bits?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:252
> +    RETURN_IF_EXCEPTION(scope, { });

Should be an assert.

> JSTests/stress/make-bound-function-should-not-assume-int32-length.js:1
> +//@ runDefault("--useDoublePredictionFuzzerAgent=1")

maybe also concurrentJIT=0 to ensure we JIT?

[Yusuke Suzuki]

Comment on attachment 366788 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366788&action=review

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:251
>> +    int32_t length = lengthValue.toInt32(exec);
> 
> Should assert it's a number. Maybe even isAnyInt and that it's within 32-bits?

Changed.

>> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:252
>> +    RETURN_IF_EXCEPTION(scope, { });
> 
> Should be an assert.

Fixed.

>> JSTests/stress/make-bound-function-should-not-assume-int32-length.js:1
>> +//@ runDefault("--useDoublePredictionFuzzerAgent=1")
> 
> maybe also concurrentJIT=0 to ensure we JIT?

Sounds fine. Fixed.

[Yusuke Suzuki]

Committed r243925: <https://trac.webkit.org/changeset/243925>

[Radar WebKit Bug Importer]

<rdar://problem/49634426>