Bug 107699 – Add SECURITY_ASSERT

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Abhishek Arya]

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).

[Abhishek Arya]

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());

[Abhishek Arya]

Created attachment 184251 [details]
Patch

[Abhishek Arya]

Created attachment 184252 [details]
Patch

[WebKit Review Bot]

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.

[Abhishek Arya]

(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().

[Eric Seidel (no email)]

Crazy.  Seems reasonable that you might want some, but not all ASSERTS on for specific builds.

[Eric Seidel (no email)]

At some level though, aren't all ASSERTS security asserts? :)  I guess it's too slow to turn them all on?

[Abhishek Arya]

(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.

[Abhishek Arya]

(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.

[Benjamin Poulain]

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.

[Abhishek Arya]

(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.

[Abhishek Arya]

Created attachment 184315 [details]
Patch

[Eric Seidel (no email)]

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. :)

[WebKit Review Bot]

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.

[Benjamin Poulain]

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.

[Benjamin Poulain]

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.

[Abhishek Arya]

(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.

[Abhishek Arya]

Comment on attachment 184315 [details]
Patch

Clearing flags on attachment: 184315

Committed r140633: <http://trac.webkit.org/changeset/140633>

[Abhishek Arya]

All reviewed patches have been landed.  Closing bug.

[Abhishek Arya]

On Wed, Jan 23, 2013 at 10:59 PM, Simon Fraser <[e-mail]> 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.

[Abhishek Arya]

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