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.
I would recommend you discuss with the V8 team first. They were talking about a couple of concerns about the design.
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.
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?
> 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.
(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.
> 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.
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).
(In reply to comment #7) > and the patch needs to be cleaned up a bit. Sure. got any suggestions?
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
Created attachment 182616 [details] Full perf test results Full test results: some of these are off more than the Dom-traverse.
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
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.
Created attachment 182626 [details] Patch.
Created attachment 182630 [details] Patch, fix custom .cpp file.
Q. How does one get a --[no-]binding-integrity in build-webkit scripts? Is there bribery involved :) ?
> 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.
Created attachment 182637 [details] Patch, add to features.gypi (initially disabled).
Created attachment 182650 [details] Patch, fix casting.
Created attachment 182836 [details] Patch, windows support attempt.
Created attachment 184603 [details] Patch, avoid merge conflicts, initial setting is "disable"
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.
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).
(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.
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)
(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.
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.
(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.
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.
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.
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.
Don't be afraid of editing 100s of files. That's better than hard-coding interface names in the code generator.
Created attachment 185118 [details] Patch, specify exceptions in IDL files.
(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.)
(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].
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.
> > 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.
(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!
Comment on attachment 185118 [details] Patch, specify exceptions in IDL files. Clearing flags on attachment: 185118 Committed r141034: <http://trac.webkit.org/changeset/141034>
All reviewed patches have been landed. Closing bug.
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>
(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?
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.
> 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.)
BTW, how are [JSNoStaticTables] and [ImplementationLacksVTable] different? Maybe can we merge the two IDL attributes?
Nostatictables is about whether the generated code is thread safe.
(In reply to comment #45) > Nostatictables is about whether the generated code is thread safe. Ah, makes sense. Thanks.