107743 – Add ASSERT_WITH_SECURITY_IMPLICATION to detect bad casts in rendering

Bug 107743 - Add ASSERT_WITH_SECURITY_IMPLICATION to detect bad casts in rendering

Summary: Add ASSERT_WITH_SECURITY_IMPLICATION to detect bad casts in rendering

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Layout and Rendering (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:

Depends on:
Blocks:

Reported:	2013-01-23 14:56 PST by Abhishek Arya
Modified:	2013-01-24 12:26 PST (History)
CC List:	18 users (show)

See Also:

Attachments
Patch (48.55 KB, patch) 2013-01-23 15:02 PST, Abhishek Arya	no flags	Details \| Formatted Diff \| Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Abhishek Arya 2013-01-23 14:56:30 PST

Add ASSERT_WITH_SECURITY_IMPLICATION to detect bad casts in rendering

Comment 1 Abhishek Arya 2013-01-23 15:02:18 PST

Created attachment 184322 [details]
Patch

Comment 2 Eric Seidel (no email) 2013-01-23 15:03:29 PST

Comment on attachment 184322 [details]
Patch

LGTM.

Comment 3 Oliver Hunt 2013-01-23 15:30:07 PST

We have RELEASE_ASSERT now, why do we need another?

Comment 4 Abhishek Arya 2013-01-23 15:32:46 PST

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

Comment 5 Oliver Hunt 2013-01-23 15:35:50 PST

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

Comment 6 Abhishek Arya 2013-01-23 15:38:42 PST

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

Comment 7 WebKit Review Bot 2013-01-23 20:15:55 PST

Comment on attachment 184322 [details]
Patch

Clearing flags on attachment: 184322

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

Comment 8 WebKit Review Bot 2013-01-23 20:15:59 PST

All reviewed patches have been landed.  Closing bug.

Comment 9 Darin Adler 2013-01-24 12:13:28 PST

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?

Comment 10 Abhishek Arya 2013-01-24 12:26:43 PST

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