Bug 107743 – Add ASSERT_WITH_SECURITY_IMPLICATION to detect bad casts in rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Abhishek Arya]

Add ASSERT_WITH_SECURITY_IMPLICATION to detect bad casts in rendering

[Abhishek Arya]

Created attachment 184322 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 184322 [details]
Patch

LGTM.

[Oliver Hunt]

We have RELEASE_ASSERT now, why do we need another?

[Abhishek Arya]

(In reply to comment #3)
> We have RELEASE_ASSERT now, why do we need another?

cced you on https://bugs.webkit.org/show_bug.cgi?id=107699 for more context. We cannot enable these for release build by default because of performance implications. these need to enabled for fuzzing builds and also help people to identify and file these security vulnerabilities properly.

[Oliver Hunt]

See I would say any assertion failure should imply security issues, maybe that's the problem...

[Abhishek Arya]

(In reply to comment #5)
> See I would say any assertion failure should imply security issues, maybe that's the problem...

Any assertion failure does not indicate a security issue. We have ton of asserts for functionality checks, null checks, etc, those are just crashers, wrong renderings, etc. The places where we are adding these ASSERT_WITH_SECURITY_IMPLICATION are 100% sure shot security bugs.

[WebKit Review Bot]

Comment on attachment 184322 [details]
Patch

Clearing flags on attachment: 184322

Committed r140640: <http://trac.webkit.org/changeset/140640>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

If we expect future programmers to use this macro correctly, we need a guideline to explain what this is for and how to use it. Where is that going to go?

[Abhishek Arya]

(In reply to comment #9)
> If we expect future programmers to use this macro correctly, we need a guideline to explain what this is for and how to use it. Where is that going to go?

I think we can explain in more detail inside Assertions.h [http://trac.webkit.org/changeset/140633/trunk/Source/WTF/wtf/Assertions.h]. Or we can update webkit wiki page here - http://www.webkit.org/coding/assertion-guidelines.html. Frankly, right now, we don't want non-security members to use this assert since they need to understand the vulnerabilities involved before using them. Bad cast ones are the easiest and can be explained easily. The other ones will be based on experience of previous discovered security vulnerabilities.