Bug 106608 – [v8] Security feature: JavaScript Bindings hardening

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Thomas Sepez]

This is a tracking bug for some of the Bindings hardening work we are contemplating for the V8 bindings used by Chromium.

Recent full Chromium exploits at some point relied upon aliasing two JavaScript wrapper objects to the same underlying C++ object, and invoking methods from the unrelated class on that object. Run-time type checking would prevent this from happening and would stop the exploits. However, run-time type checks are too expensive in terms of cycles to perform everywhere throughout the code, but the JS Bindings code provides a clean API between layers at where these checks could be applied at reduced cost.

[Kentaro Hara]

I would recommend you discuss with the V8 team first. They were talking about a couple of concerns about the design.

[Thomas Sepez]

Created attachment 182407 [details]
Patch for discussion only.

This is the minimal patch in some sense in that it doesn't disturb webkit classes (to get better coverage) or V8 APIs (to get better performance).
I'm seeing about a 4% hit on Dromaeo/dom-traverse.html.  It would be good to get other data points from folks used to benchmarking these.
We can drop this to about 2% hit if the two calls to the V8 API in the toNativeChecked() method are combined through a new API returning two items at once.

[Adam Barth]

Comment on attachment 182407 [details]
Patch for discussion only.

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:58
> +my %vtableExceptions = (

Can we make this an IDL attribute rather than hardcoding this list in the code generator?

[Thomas Sepez]

> Can we make this an IDL attribute rather than hardcoding this list in the code generator?

I'd expect that we'd do it that way long-term.  But that requires modifying files in webkit, and the associated fist-fights.

[Thomas Sepez]

(In reply to comment #4)
> > Can we make this an IDL attribute rather than hardcoding this list in the code generator?
> 
> I'd expect that we'd do it that way long-term.  But that requires modifying files in webkit, and the associated fist-fights.

Also note that the plan would be to get rid of everything in %vtableExceptions by giving them virtual destructors on the platforms that implement this. The %polymorphicExceptions is a little harder.

[Thomas Sepez]

> Also note that the plan would be to get rid of everything in %vtableExceptions by giving them virtual destructors on the platforms that implement this.

Or, to be more precise, nearly everything in that list except for the small hot items.

[Adam Barth]

I should say that 2-4% on dom-traverse.html sounds like a reasonable price to pay for this security hardening.  Obviously, I'd prefer 2% rather than 4% (and the patch needs to be cleaned up a bit).

[Thomas Sepez]

(In reply to comment #7)
> and the patch needs to be cleaned up a bit.
Sure.  got any suggestions?

[Adam Barth]

Comment on attachment 182407 [details]
Patch for discussion only.

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:470
> +    # DO NOT CI this next line.  The symbol would come from the build environment.
> +    # We kludge this here to make building the prototype easier.

This stands out.  :)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:533
> +#ifdef V8_BINDING_TYPE_CHECKS_ENABLED

V8_BINDING_TYPE_CHECKS_ENABLED -> ENABLE(RUNTIME_TYPE_CHECKS_IN_BINDINGS) ?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:538
> +        void* lowPtr = (void*)((void**)classInfo->instanceValidationPointer + 2);
> +        void* highPtr = (void*)((void**)classInfo->instanceValidationPointer + 3);
> +        void* vtablePtr = *(void**)objectPtr;

These should use C++ style casts (e.g., reinterpret_cast)

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:973
> +

This change seems spurious

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2846
> +    push(@implContentDecls, "#ifdef V8_BINDING_TYPE_CHECKS_ENABLED\nextern \"C\" { extern void* ${vtableName}[]; }\n#endif\n\n") if $vtableName;

Can we use the << style instead of using \n ?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3732
> +sub GetVtableNameForType

Everything below this line is just magic to me.  :)

> Source/WebCore/bindings/v8/ScriptWrappable.h:42
> +    virtual ~ScriptWrappable() {}

I'm not sure about this change.  Won't this cause some objects to have multiple vtables?

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:69
> -        
> -        
> +
> +

Lots of suprious whitespace changes

[Thomas Sepez]

Created attachment 182616 [details]
Full perf test results

Full test results: some of these are off more than the Dom-traverse.

[Adam Barth]

It's hard to tell whether those results are due to noise.  When you run that many tests, you need to worry about multiple hypothesis testing.  http://en.wikipedia.org/wiki/Multiple_comparisons

[Thomas Sepez]

Comment on attachment 182407 [details]
Patch for discussion only.

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:470
>> +    # We kludge this here to make building the prototype easier.
> 
> This stands out.  :)

Clearly. :)

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:533
>> +#ifdef V8_BINDING_TYPE_CHECKS_ENABLED
> 
> V8_BINDING_TYPE_CHECKS_ENABLED -> ENABLE(RUNTIME_TYPE_CHECKS_IN_BINDINGS) ?

changed to ENABLE(BINDING_INTEGRITY)

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:538
>> +        void* vtablePtr = *(void**)objectPtr;
> 
> These should use C++ style casts (e.g., reinterpret_cast)

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:973
>> +
> 
> This change seems spurious

yep.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2846
>> +    push(@implContentDecls, "#ifdef V8_BINDING_TYPE_CHECKS_ENABLED\nextern \"C\" { extern void* ${vtableName}[]; }\n#endif\n\n") if $vtableName;
> 
> Can we use the << style instead of using \n ?

Done.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3732
>> +sub GetVtableNameForType
> 
> Everything below this line is just magic to me.  :)

Added link to ABI spec.

>> Source/WebCore/bindings/v8/ScriptWrappable.h:42
>> +    virtual ~ScriptWrappable() {}
> 
> I'm not sure about this change.  Won't this cause some objects to have multiple vtables?

Now surrounded with ENABLE(BINDING_INTEGRITY).  This class typically comes first, so its effect should be to grow an initial vtable.  There may be multiple ones.

>> Source/WebCore/bindings/v8/WrapperTypeInfo.h:69
>> +
> 
> Lots of suprious whitespace changes

Replaced lines with 8 spaces with truly blank lines cause it irked me.

[Thomas Sepez]

Created attachment 182626 [details]
Patch.

[Thomas Sepez]

Created attachment 182630 [details]
Patch, fix custom .cpp file.

[Thomas Sepez]

Q. How does one get a --[no-]binding-integrity in build-webkit scripts?  Is there bribery involved :) ?

[Adam Barth]

> Q. How does one get a --[no-]binding-integrity in build-webkit scripts?  Is there bribery involved :) ?

Those flags don't do anything in Chromium.  For Chromium, you want to add something to features.gypi.

[Thomas Sepez]

Created attachment 182637 [details]
Patch, add to features.gypi (initially disabled).

[Thomas Sepez]

Created attachment 182650 [details]
Patch, fix casting.

[Thomas Sepez]

Created attachment 182836 [details]
Patch, windows support attempt.

[Thomas Sepez]

Created attachment 184603 [details]
Patch, avoid merge conflicts, initial setting is "disable"

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/v8/ScriptWrappable.h', u'Source/WebCore/bindings/v8/WrapperTypeInfo.h', u'Source/WebCore/bindings/v8/custom/V8HTMLImageElementConstructor.cpp', u'Source/WebCore/bindings/v8/custom/V8PerformanceEntryCustom.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/features.gypi']" exit_code: 1
Source/WebKit/chromium/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/bindings/v8/ScriptWrappable.h:43:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 8 files


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

[Adam Barth]

Can you explain the scenario in which this patch helps security?  I'm not sure I quite understand.  If there's a JS wrapper pointing to a native object, the JS wrapper should be retaining the object, preventing it from being freed (and therefore used-after-freed).

[Thomas Sepez]

(In reply to comment #22)
> Can you explain the scenario in which this patch helps security?  I'm not sure I quite understand.  If there's a JS wrapper pointing to a native object, the JS wrapper should be retaining the object, preventing it from being freed (and therefore used-after-freed).

That would imply that the object is already free at the time it is first wrapped.

[Kentaro Hara]

A few tips for stable performance testing:

(1) You can use ./run-perf-tests instead of chrome.

(2) You can specify CPU affinity by a taskset command; e.g. 'taskset -c 12 ./Tools/Scripts/run-perf-tests Dromaeo/dom-traverse'. It's important to specify a CPU ID that is not likely to be used by other processes (i.e. you want to avoid 0)

[Thomas Sepez]

(In reply to comment #23)
> (In reply to comment #22)
> > Can you explain the scenario in which this patch helps security?  I'm not sure I quite understand.  If there's a JS wrapper pointing to a native object, the JS wrapper should be retaining the object, preventing it from being freed (and therefore used-after-freed).
> 
> That would imply that the object is already free at the time it is first wrapped.

which begs the question "why not just check there"?  We had convinced ourselves that an object could be free and still have an intact vtable pointer.  But that was before the performance headaches, so maybe we could get away with this on platforms that trash the first word with something like the heap freelist pointer upon free.

[Thomas Sepez]

Thanks for the tips.  Responses inline:

> (1) You can use ./run-perf-tests instead of chrome.
Yep.  That's what I'm using.

> 
> (2) You can specify CPU affinity by a taskset command; e.g. 'taskset -c 12 ./Tools/Scripts/run-perf-tests Dromaeo/dom-traverse'. It's important to specify a CPU ID that is not likely to be used by other processes (i.e. you want to avoid 0
Thanks.  That's a good idea.  I've been closing all GUI apps on the mac, but there's still a ton of stuff that's always running.

[Thomas Sepez]

(I
> > (2) You can specify CPU affinity by a taskset command; e.g. 'taskset -c 12 ./Tools/Scripts/run-perf-tests Dromaeo/dom-traverse'. It's important to specify a CPU ID that is not likely to be used by other processes (i.e. you want to avoid 0
> Thanks.  That's a good idea.  I've been closing all GUI apps on the mac, but there's still a ton of stuff that's always running.

Except that I don't see an equivalent for OSX.  A quick search turns up a couple of pages claiming that no such feature exists for mac.

[Thomas Sepez]

Created attachment 185033 [details]
Patch, check at wrapper creation time.

I did not notice any performance hit when I ran dom-traverse against this patch.

[Adam Barth]

Comment on attachment 185033 [details]
Patch, check at wrapper creation time.

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3739
> +        || $typename eq "AudioBuffer"
> +        || $typename eq "AudioListener"
> +        || $typename eq "AudioSourceNode"
> +        || $typename eq "AudioSourceNode"
> +        || $typename eq "CSSPrimitiveValue"
> +        || $typename eq "CSSValue"

We don't want to the these big lists of interface names in the code generator.  Instead, we should use IDL attributes to control the code generator.

[Thomas Sepez]

Yep, and there's a comment about doing this long-term in V8CodeGenerator.pm.  The alternative is to patch 100s of files now, which we can do incrementally otherwise.

[Adam Barth]

Don't be afraid of editing 100s of files.  That's better than hard-coding interface names in the code generator.

[Thomas Sepez]

Created attachment 185118 [details]
Patch, specify exceptions in IDL files.

[Kentaro Hara]

(In reply to comment #28)
> Created an attachment (id=185033) [details]
> Patch, check at wrapper creation time.
> 
> I did not notice any performance hit when I ran dom-traverse against this patch.

Would you also check the perf impact on Bindings/first-child.html? It will tell you the worst case number. (I'm not intending to block your work by the number, I just want to know the number.)

[Thomas Sepez]

(In reply to comment #33)
> (In reply to comment #28)
> > Created an attachment (id=185033) [details] [details]
> > Patch, check at wrapper creation time.
> > 
> > I did not notice any performance hit when I ran dom-traverse against this patch.
> 
> Would you also check the perf impact on Bindings/first-child.html? It will tell you the worst case number. (I'm not intending to block your work by the number, I just want to know the number.)

No Problem.  before = [ 458, 462 ], after = [ 459, 455].

[Adam Barth]

Comment on attachment 185118 [details]
Patch, specify exceptions in IDL files.

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

This looks much better.  Thank you for pushing this data out into IDL attributes.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3603
> +sub GetGnuVTableOffsetForType

I probably would have added a FIXME or a comment explaining this function.

> Source/WebCore/css/WebKitCSSFilterValue.idl:30
>          Conditional=CSS_FILTERS,
>          IndexedGetter,
> -        DoNotCheckConstants
> +        DoNotCheckConstants,
> +    ImplementationLacksVTable

Looks like there are some indent issues.  It would be nice to clean up the bad indents to match the four-space indent, but we can do that in a followup patch.

> Source/WebKit/chromium/features.gypi:37
> +      'ENABLE_BINDING_INTEGRITY=0',

Did you mean to land the patch with this disabled?  I guess we can flip the switch separately from the big code edit.

[Adam Barth]

> > Would you also check the perf impact on Bindings/first-child.html? It will tell you the worst case number. (I'm not intending to block your work by the number, I just want to know the number.)
> 
> No Problem.  before = [ 458, 462 ], after = [ 459, 455].

@haraken: He's moved the check from toNative to createWrapper, which means this version is much better for performance.

[Kentaro Hara]

(In reply to comment #36)
> > No Problem.  before = [ 458, 462 ], after = [ 459, 455].
> 
> @haraken: He's moved the check from toNative to createWrapper, which means this version is much better for performance.

Great!

[Adam Barth]

Comment on attachment 185118 [details]
Patch, specify exceptions in IDL files.

Clearing flags on attachment: 185118

Committed r141034: <http://trac.webkit.org/changeset/141034>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.

[Zoltan Arvai]

After the patch landed some binging tests are failing. 
Can you check it, please?

(In reply to comment #38)
> (From update of attachment 185118 [details])
> Clearing flags on attachment: 185118
> 
> Committed r141034: <http://trac.webkit.org/changeset/141034>

[Florin Malita]

(In reply to comment #40)
> After the patch landed some binging tests are failing. 
> Can you check it, please?

Looks like the results just need to be updated: http://trac.webkit.org/changeset/141105

Adam,Thomas: can you please double-check that the new results are correct?

[Thomas Sepez]

The changes look OK, but having said that, it seems a little strange that we have source-controlled files that must match generated ones -- maybe someone with some background on run-bindings-tests can let me in on the thinking?

I missed this because I only ran run-webkit-tests --- not the run-bindings-tests.  Sorry.

[Adam Barth]

> The changes look OK, but having said that, it seems a little strange that we have source-controlled files that must match generated ones -- maybe someone with some background on run-bindings-tests can let me in on the thinking?

It helps us test the code generator and makes it easier to review changes to the generated code.  We should run them automatically in the EWS to remind folks to update the results in their patch.  (It's just test data---it's not compiled into the product.)

[Kentaro Hara]

BTW, how are [JSNoStaticTables] and [ImplementationLacksVTable] different? Maybe can we merge the two IDL attributes?

[Adam Barth]

Nostatictables is about whether the generated code is thread safe.

[Kentaro Hara]

(In reply to comment #45)
> Nostatictables is about whether the generated code is thread safe.

Ah, makes sense. Thanks.