SECURITY_ASSERT will replace some asserts that are known to cause security vulnerabilities and will be enabled in our fuzzing builds (e.g. http://trac.webkit.org/changeset/97009).
Some candidates are all the bad cast catchers [long-list, will be another bug] e.g. inline RenderSVGShape* toRenderSVGShape(RenderObject* object) { - ASSERT(!object || object->isSVGShape()); + SECURITY_ASSERT(!object || object->isSVGShape()); return static_cast<RenderSVGShape*>(object); } and some of Sergey's recent dom tree corruption bug due to document confusion void ContainerNode::parserAppendChild(PassRefPtr<Node> newChild) { ....... ASSERT(document() == newChild->document());
Created attachment 184251 [details] Patch
Created attachment 184252 [details] Patch
Attachment 184252 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Assertions.h', u'Source/WTF/wtf/Vector.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/ContainerNode.cpp']" exit_code: 1 Source/WTF/wtf/Assertions.h:276: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #4) > Attachment 184252 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Assertions.h', u'Source/WTF/wtf/Vector.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/ContainerNode.cpp']" exit_code: 1 > Source/WTF/wtf/Assertions.h:276: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] > Total errors found: 1 in 5 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. false positive indeed, see the code in ASSERT().
Crazy. Seems reasonable that you might want some, but not all ASSERTS on for specific builds.
At some level though, aren't all ASSERTS security asserts? :) I guess it's too slow to turn them all on?
(In reply to comment #7) > At some level though, aren't all ASSERTS security asserts? :) I guess it's too slow to turn them all on? I think only 5-10% asserts cause security bugs. We can't turn them all on. For a fuzzed testcase, some of the ones will always hit like unsupported css property value, etc.
(In reply to comment #8) > (In reply to comment #7) > > At some level though, aren't all ASSERTS security asserts? :) I guess it's too slow to turn them all on? > > I think only 5-10% asserts cause security bugs. We can't turn them all on. For a fuzzed testcase, some of the ones will always hit like unsupported css property value, etc. And yes, it makes build slow to turn everything on. And we are making security_asserts for only those asserts that we know 100% sure to cause security vulns, like all the bad cast ones.
I like the idea but: -I would add a descriptive comment in Assertions.h explaining its purpose. Contrary to regular assertions, this concept is not common and would benefit from an explanation. -I think the name is not great. SECURITY_ASSERT could be the name of an Assertion that would always cause a crash because hitting it means a security issue. We have various ASSERT_FOOBAR already. What about ASSERT_WITH_SECURITY_RISK || ASSERT_WITH_SECURITY_IMPLICATION || ASSERT_WITH_SECURITY_HINT or something like that? -In addition to Vector.h, HashTable has many very good assertion that have helped catching use after free in the past.
(In reply to comment #10) > I like the idea but: > -I would add a descriptive comment in Assertions.h explaining its purpose. Contrary to regular assertions, this concept is not common and would benefit from an explanation. > Fixed. > -I think the name is not great. SECURITY_ASSERT could be the name of an Assertion that would always cause a crash because hitting it means a security issue. We have various ASSERT_FOOBAR already. What about ASSERT_WITH_SECURITY_RISK || ASSERT_WITH_SECURITY_IMPLICATION || ASSERT_WITH_SECURITY_HINT or something like that? Fixed. > > -In addition to Vector.h, HashTable has many very good assertion that have helped catching use after free in the past. This is just the beginning, we need to change many of these asserts from now on. As i was looking at HashTable, it has far too many interesting ones (also hidden behind CHECK_HASHTABLE_ITERATORS), i will add it in follow up patches, just like the numerous others I have in plans already. ASSERT(!object || object->isSVGShape());. Phase 1 for this will be only changing those we are sure for 100% security vulnerability indicator.
Created attachment 184315 [details] Patch
Comment on attachment 184315 [details] Patch Seems reasonable. I'm concerned that this list will grow out of control, and I'm not sure who's going to audit it. :)
Attachment 184315 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Assertions.h', u'Source/WTF/wtf/Vector.h', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/ContainerNode.cpp']" exit_code: 1 Source/WTF/wtf/Assertions.h:282: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 184315 [details] Patch I like this a lot. I don't cq+ to give a chance for other to raise concerns. Give it a little time before landing. Good luck finding new security issues.
(In reply to comment #16) > (From update of attachment 184315 [details]) > I like this a lot. I don't cq+ to give a chance for other to raise concerns. Give it a little time before landing. > Sure. Sounds good. > Good luck finding new security issues. I gave it a shot with very few asserts locally, and already found some bad casts. The key was to filter these interesting asserts out and i am thankful to you and Eric for the support of this idea.
Comment on attachment 184315 [details] Patch Clearing flags on attachment: 184315 Committed r140633: <http://trac.webkit.org/changeset/140633>
All reviewed patches have been landed. Closing bug.
On Wed, Jan 23, 2013 at 10:59 PM, Simon Fraser <simon.fraser@apple.com> wrote: It seems weird to add this new kind of assertion under the cover of a security bug, for two reasons. First, how are WebKit developers supposed to know when to use it? Second, it's just adding release-build assertion that (I'm guessing) you enable for fuzzing. That doesn't seem to be exposing new vulnerabilities. My mistake for filing it as a security bug, i just made it public. However, I did leave out a comment in the code explaining the purpose of this ASSERT. We cannot enable all asserts on our fuzzing cluster since only few asserts are known to cause security vulnerabilities. These asserts added in the two patches are known to cause bad casts [static_cast problems], out of bounds reads[vector.h ones], containernode document confusion[use after frees]. The places we are adding them are sure-shot security bugs, like bad cast due to wrong static_cast usage, out of bounds access in vector and list will grow with time. In normal builds, it will work as regular asserts. This assert will also help a dev hitting it to know that they should file a security bug when they encounter such issues (also in the comment) [seeing the assert name and read the description in assertions.h]. Regarding how webkit developers should add it is still hard and not encouraged in the first phase. We want security group members who know about these security problems should add it for now.
To give you examples of bugs we found using this in a trial run. https://bugs.webkit.org/show_bug.cgi?id=107748 https://bugs.webkit.org/show_bug.cgi?id=107237