Replace RELEASE_ASSERT_WITH_SECURITY_IMPLICATION with SECURITY_CHECK.

Replace RELEASE_ASSERT_WITH_SECURITY_IMPLICATION with SECURITY_CHECK.

SECURITY_CHECK is based on base/logging.h.  SECURITY_CHECK is
 - equivalent to SECURITY_DCHECK if ADDRESS_SANITIZER is defined.
 - equivalent to CHECK otherwise.

It's similar to RELEASE_ASSERT_WITH_SECURITY_IMPLICATION.

BUG=596760
Committed: https://crrev.com/be2579d48fbb2b4c98f693ab1c62cf1a9d60a06d
Cr-Commit-Position: refs/heads/master@{#384205}

[tkent, 2016-03-31 00:16:07]

Patchset #1 (id:1) has been deleted

[tkent, 2016-03-31 00:26:17]

tkent@chromium.org changed reviewers:
+ inferno@chromium.org

[tkent, 2016-03-31 00:26:18]

inferno@, would you review this please?

I don't try to remove/improve RELEASE_ASSERT_WITH_SECURITY_IMPLICATION in this
CL.

[haraken, 2016-03-31 00:54:58]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2016-03-31 00:54:59]

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Assertions.h:220: // with SECURITY_DCHECK.

SECURITY_DCHECK => SECURITY_CHECK ?

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Assertions.h:250: // A SECURITY_CHECK failure is
actually not vulnerable.

Maybe failures that are not vulnerable should use CHECK?

BTW, I don't think Blink has distinguished
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT very well. Is it
worth having the difference?

[dcheng, 2016-03-31 01:09:39]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2016-03-31 01:09:40]

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Assertions.h:220: // with SECURITY_DCHECK.
On 2016/03/31 at 00:54:58, haraken wrote:
> SECURITY_DCHECK => SECURITY_CHECK ?

I'm not sure how expensive this will be, all the toDerivedClass macros use
ASSERT_WITH_SECURITY_IMPLICATIONS right now.

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Assertions.h:250: // A SECURITY_CHECK failure is
actually not vulnerable.
On 2016/03/31 at 00:54:58, haraken wrote:
> Maybe failures that are not vulnerable should use CHECK?
> 
> BTW, I don't think Blink has distinguished
RELEASE_ASSERT_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT very well. Is it
worth having the difference?

(I think) the security team uses this as a signal of how critical a reported
issue might be.

[inferno, 2016-03-31 04:04:18]

inferno@chromium.org changed reviewers:
+ mbarbella@chromium.org, ochang@chromium.org

[inferno, 2016-03-31 04:04:18]

lgtm

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Assertions.h:250: // A SECURITY_CHECK failure is
actually not vulnerable.
On 2016/03/31 01:09:40, dcheng wrote:
> On 2016/03/31 at 00:54:58, haraken wrote:
> > Maybe failures that are not vulnerable should use CHECK?
> > 
> > BTW, I don't think Blink has distinguished
> RELEASE_ASSERT_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT very well. Is it
> worth having the difference?
> 
> (I think) the security team uses this as a signal of how critical a reported
> issue might be.

Yes correct, otherwise we won't be able to distinguish these. and hard to
prioritize stuff everything shows as plain checks.

[haraken, 2016-03-31 04:07:53]

On 2016/03/31 04:04:18, inferno wrote:
> lgtm
> 
>
https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/Assertions.h (right):
> 
>
https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/Assertions.h:250: // A SECURITY_CHECK failure is
> actually not vulnerable.
> On 2016/03/31 01:09:40, dcheng wrote:
> > On 2016/03/31 at 00:54:58, haraken wrote:
> > > Maybe failures that are not vulnerable should use CHECK?
> > > 
> > > BTW, I don't think Blink has distinguished
> > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT very well. Is it
> > worth having the difference?
> > 
> > (I think) the security team uses this as a signal of how critical a reported
> > issue might be.
> 
> Yes correct, otherwise we won't be able to distinguish these. and hard to
> prioritize stuff everything shows as plain checks.

Makes sense. LGTM.

[tkent, 2016-03-31 05:26:12]

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Assertions.h:220: // with SECURITY_DCHECK.
> SECURITY_DCHECK => SECURITY_CHECK ?

SECURITY_DCHECK is correct here.

ASSERT -> DCHECK
RELEASE_ASSERT -> CHECK
ASSERT_WITH_S_I -> SECURITY_DCHECK
RELEASE_ASSERT_WITH_S_SI -> SECURITY_CHECK

> I'm not sure how expensive this will be, all the toDerivedClass macros use
ASSERT_WITH_SECURITY_IMPLICATIONS right now.

We'll replace all ASSERT_WITH_S_Is with SECURITY_DCHECKs.

[tkent, 2016-03-31 05:26:28]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2016-03-31 05:26:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841363003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841363003/20001

[commit-bot: I haz the power, 2016-03-31 06:12:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-31 06:12:24]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tkent, 2016-03-31 06:13:24]

The CQ bit was checked by tkent@chromium.org

[Oliver Chang, 2016-03-31 06:13:39]

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/Assertions.h (right):

https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/Assertions.h:250: // A SECURITY_CHECK failure is
actually not vulnerable.
On 2016/03/31 04:04:18, inferno wrote:
> On 2016/03/31 01:09:40, dcheng wrote:
> > On 2016/03/31 at 00:54:58, haraken wrote:
> > > Maybe failures that are not vulnerable should use CHECK?
> > > 
> > > BTW, I don't think Blink has distinguished
> > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT very well. Is it
> > worth having the difference?
> > 
> > (I think) the security team uses this as a signal of how critical a reported
> > issue might be.
> 
> Yes correct, otherwise we won't be able to distinguish these. and hard to
> prioritize stuff everything shows as plain checks.

Why do we need to distinguish between CHECK and SECURITY_CHECK here? As tkent
mentioned, SECURITY_CHECK failures don't indicate any exploitable vulnerability,
so there's no reason to prioritise SECURITY_CHECK over CHECK or make this
distinction? The one advantage I see is that it could deter someone from
unknowingly removing a security sensitive CHECK (or turning it into a DCHECK).

Either way, as this is right now it can actually cause confusion for us during
fuzzing since we just see the "Security check failed:" message for both
SECURITY_CHECK and SECURITY_DCHECK, but only the latter represents a real
vulnerability.

[commit-bot: I haz the power, 2016-03-31 06:14:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841363003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841363003/20001

[haraken, 2016-03-31 06:21:46]

On 2016/03/31 06:13:39, Oliver Chang wrote:
>
https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/Assertions.h (right):
> 
>
https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/Assertions.h:250: // A SECURITY_CHECK failure is
> actually not vulnerable.
> On 2016/03/31 04:04:18, inferno wrote:
> > On 2016/03/31 01:09:40, dcheng wrote:
> > > On 2016/03/31 at 00:54:58, haraken wrote:
> > > > Maybe failures that are not vulnerable should use CHECK?
> > > > 
> > > > BTW, I don't think Blink has distinguished
> > > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT very well. Is
it
> > > worth having the difference?
> > > 
> > > (I think) the security team uses this as a signal of how critical a
reported
> > > issue might be.
> > 
> > Yes correct, otherwise we won't be able to distinguish these. and hard to
> > prioritize stuff everything shows as plain checks.
> 
> Why do we need to distinguish between CHECK and SECURITY_CHECK here? As tkent
> mentioned, SECURITY_CHECK failures don't indicate any exploitable
vulnerability,
> so there's no reason to prioritise SECURITY_CHECK over CHECK or make this
> distinction? The one advantage I see is that it could deter someone from
> unknowingly removing a security sensitive CHECK (or turning it into a DCHECK).
> 
> Either way, as this is right now it can actually cause confusion for us during
> fuzzing since we just see the "Security check failed:" message for both
> SECURITY_CHECK and SECURITY_DCHECK, but only the latter represents a real
> vulnerability.

I don't fully understand why we need SECURITY_DCHECK. If the check matters for
security, shouldn't it be SECURITY_CHECK?

[Oliver Chang, 2016-03-31 06:30:26]

On 2016/03/31 06:21:46, haraken wrote:
> On 2016/03/31 06:13:39, Oliver Chang wrote:
> >
>
https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/wtf/Assertions.h (right):
> > 
> >
>
https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/wtf/Assertions.h:250: // A SECURITY_CHECK failure
is
> > actually not vulnerable.
> > On 2016/03/31 04:04:18, inferno wrote:
> > > On 2016/03/31 01:09:40, dcheng wrote:
> > > > On 2016/03/31 at 00:54:58, haraken wrote:
> > > > > Maybe failures that are not vulnerable should use CHECK?
> > > > > 
> > > > > BTW, I don't think Blink has distinguished
> > > > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT very well.
Is
> it
> > > > worth having the difference?
> > > > 
> > > > (I think) the security team uses this as a signal of how critical a
> reported
> > > > issue might be.
> > > 
> > > Yes correct, otherwise we won't be able to distinguish these. and hard to
> > > prioritize stuff everything shows as plain checks.
> > 
> > Why do we need to distinguish between CHECK and SECURITY_CHECK here? As
tkent
> > mentioned, SECURITY_CHECK failures don't indicate any exploitable
> vulnerability,
> > so there's no reason to prioritise SECURITY_CHECK over CHECK or make this
> > distinction? The one advantage I see is that it could deter someone from
> > unknowingly removing a security sensitive CHECK (or turning it into a
DCHECK).
> > 
> > Either way, as this is right now it can actually cause confusion for us
during
> > fuzzing since we just see the "Security check failed:" message for both
> > SECURITY_CHECK and SECURITY_DCHECK, but only the latter represents a real
> > vulnerability.
> 
> I don't fully understand why we need SECURITY_DCHECK. If the check matters for
> security, shouldn't it be SECURITY_CHECK?

My understanding is that it's too expensive performance wise to have every one
of these be a release CHECK. We need SECURITY_DCHECK to be able to prioritise
these failures as security bugs over the non security DCHECK failures. Also, we
turn SECURITY_DCHECKs on for ASan release builds during fuzzing on ClusterFuzz
(as fuzzing with all DCHECKS is not feasible).

[commit-bot: I haz the power, 2016-03-31 06:53:53]

Committed patchset #1 (id:20001)

[commit-bot: I haz the power, 2016-03-31 06:55:07]

Description was changed from

==========
Replace RELEASE_ASSERT_WITH_SECURITY_IMPLICATION with SECURITY_CHECK.

SECURITY_CHECK is based on base/logging.h.  SECURITY_CHECK is
 - equivalent to SECURITY_DCHECK if ADDRESS_SANITIZER is defined.
 - equivalent to CHECK otherwise.

It's similar to RELEASE_ASSERT_WITH_SECURITY_IMPLICATION.

BUG=596760
==========

to

==========
Replace RELEASE_ASSERT_WITH_SECURITY_IMPLICATION with SECURITY_CHECK.

SECURITY_CHECK is based on base/logging.h.  SECURITY_CHECK is
 - equivalent to SECURITY_DCHECK if ADDRESS_SANITIZER is defined.
 - equivalent to CHECK otherwise.

It's similar to RELEASE_ASSERT_WITH_SECURITY_IMPLICATION.

BUG=596760

Committed: https://crrev.com/be2579d48fbb2b4c98f693ab1c62cf1a9d60a06d
Cr-Commit-Position: refs/heads/master@{#384205}
==========

[commit-bot: I haz the power, 2016-03-31 06:55:08]

Patchset 1 (id:??) landed as
https://crrev.com/be2579d48fbb2b4c98f693ab1c62cf1a9d60a06d
Cr-Commit-Position: refs/heads/master@{#384205}

[aarya, 2016-03-31 14:58:57]

On 2016/03/31 06:13:39, Oliver Chang wrote:
>
https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/wtf/Assertions.h (right):
> 
>
https://codereview.chromium.org/1841363003/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/wtf/Assertions.h:250: // A SECURITY_CHECK failure is
> actually not vulnerable.
> On 2016/03/31 04:04:18, inferno wrote:
> > On 2016/03/31 01:09:40, dcheng wrote:
> > > On 2016/03/31 at 00:54:58, haraken wrote:
> > > > Maybe failures that are not vulnerable should use CHECK?
> > > > 
> > > > BTW, I don't think Blink has distinguished
> > > RELEASE_ASSERT_WITH_SECURITY_IMPLICATION and RELEASE_ASSERT very well. Is
it
> > > worth having the difference?
> > > 
> > > (I think) the security team uses this as a signal of how critical a
reported
> > > issue might be.
> > 
> > Yes correct, otherwise we won't be able to distinguish these. and hard to
> > prioritize stuff everything shows as plain checks.
> 
> Why do we need to distinguish between CHECK and SECURITY_CHECK here? As tkent
> mentioned, SECURITY_CHECK failures don't indicate any exploitable
vulnerability,
> so there's no reason to prioritise SECURITY_CHECK over CHECK or make this
> distinction? The one advantage I see is that it could deter someone from
> unknowingly removing a security sensitive CHECK (or turning it into a DCHECK).
> 
> Either way, as this is right now it can actually cause confusion for us during
> fuzzing since we just see the "Security check failed:" message for both
> SECURITY_CHECK and SECURITY_DCHECK, but only the latter represents a real
> vulnerability.

SECURITY_CHECK are put in some few places (so yes it is discouraged and people
should put CHECK) where we actually want to get reproducible repro and
prioritize it for fixing the underlying cause. Otherwise, it just gets lost.