Fix 58162: Mixed Content False Positive for intranet hostname certificates...

Fix 58162: Mixed Content False Positive for intranet hostname certificates

Previously, we lumped the intranet host warning in with mixed content warning. This calls it out as a separate warning.

BUG=58162
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=62178

[Finnur, 2010-10-08 13:20:53]

Adam, you probably are best equipped to review this.

Ian/Adam are probably the right candidates to come up with a blurb of text for
showing the user (generated_resources.grd).

Scott, this is more of an FYI for you. Note that there is a DCHECK I changed in
toolbar_model.cc that you added, according to the blamelist.

[abarth-chromium, 2010-10-08 16:23:20]

Hum...  This change isn't quite right, but I'm not sure the original code was
right either.  Consider the case of https://example.com/ running a script from
https://foo/.  In that case, we want to treat the script as insecure because
it's equivalent to running an HTTP script.

I think we should make this change (with real text, of course), and I'll handle
the subresource case in WebKit.

[Scott Hess - ex-Googler, 2010-10-08 16:31:26]

On 2010/10/08 13:20:53, Finnur wrote:
> Scott, this is more of an FYI for you. Note that there is a DCHECK I changed
in
> toolbar_model.cc that you added, according to the blamelist.

That change seems reasonable to me.

[Finnur, 2010-10-08 21:32:14]

Adam,

Thanks for reviewing. 

Is that an LGTM if Ian comes up with proper text for the dialog? :)

On 2010/10/08 16:23:20, abarth wrote:
> Hum...  This change isn't quite right, but I'm not sure the original code was
> right either.  Consider the case of https://example.com/ running a script from
> https://foo/.  In that case, we want to treat the script as insecure because
> it's equivalent to running an HTTP script.
> 
> I think we should make this change (with real text, of course), and I'll
handle
> the subresource case in WebKit.

[abarth-chromium, 2010-10-08 21:40:35]

Yes, it was.  Well, that plus more work for me.  :)

On Fri, Oct 8, 2010 at 2:32 PM,  <finnur@chromium.org> wrote:
> Adam,
>
> Thanks for reviewing.
>
> Is that an LGTM if Ian comes up with proper text for the dialog? :)
>
> On 2010/10/08 16:23:20, abarth wrote:
>>
>> Hum...  This change isn't quite right, but I'm not sure the original code
>> was
>> right either.  Consider the case of https://example.com/ running a script
>> from
>> https://foo/.  In that case, we want to treat the script as insecure
>> because
>> it's equivalent to running an HTTP script.
>
>> I think we should make this change (with real text, of course), and I'll
>
> handle
>>
>> the subresource case in WebKit.
>
>
>
> http://codereview.chromium.org/3536019/show
>

[wtc, 2010-10-09 00:18:56]

http://codereview.chromium.org/3536019/diff/7002/13001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3536019/diff/7002/13001#newcode5776
chrome/app/generated_resources.grd:5776: <message
name="IDS_PAGE_INFO_SECURITY_TAB_NON_UNIQUE_NAME" desc="The text of the identity
section when the host is not unique (such as with Intranet sites).">
"Intranet sites" => "Intranet host names"

http://codereview.chromium.org/3536019/diff/7002/13001#newcode5777
chrome/app/generated_resources.grd:5777: The host is not unique. Lorum ipsum
dolores shot the sheriff.
Are you serious?

http://codereview.chromium.org/3536019/diff/7002/13004
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/3536019/diff/7002/13004#newcode189
chrome/browser/ssl/ssl_policy.cc:189: if (!(entry->ssl().cert_status() &
net::CERT_STATUS_COMMON_NAME_INVALID)) {
This should probably be moved up to line 167.
It seems that we should set the CERT_STATUS_NON_UNIQUE_NAME
bit even if there are certificate errors (such as untrusted
certificate).  I need to think about this more...

http://codereview.chromium.org/3536019/diff/7002/13005
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/3536019/diff/7002/13005#newcode88
chrome/browser/toolbar_model.cc:88: net::CERT_STATUS_NON_UNIQUE_NAME));
This DCHECK is wrong, because the
CERT_STATUS_UNABLE_TO_CHECK_REVOCATION and
CERT_STATUS_NON_UNIQUE_NAME can both be set.

The correct DCHECK will have a very complicated expression,
so perhaps we should just delete this DCHECK.

http://codereview.chromium.org/3536019/diff/7002/13006
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/3536019/diff/7002/13006#newcode25
net/base/cert_status_flags.h:25: CERT_STATUS_NON_UNIQUE_NAME            = 1 <<
10,
I'm still debating whether the "non-unique host name" error
should be a certificate error.  abarth, what do you think?

A corresponding error code should be added to src/net/base/net_error_list.h:

// The server responded with a certificate that matched
// the host name, but the host name was not globally unique.
NET_ERROR(CERT_NON_UNIQUE_NAME, -210)

And then CERT_END should be updated:

NET_ERROR(CERT_END, -211)

[abarth-chromium, 2010-10-09 00:32:22]

http://codereview.chromium.org/3536019/diff/7002/13006
File net/base/cert_status_flags.h (right):

http://codereview.chromium.org/3536019/diff/7002/13006#newcode25
net/base/cert_status_flags.h:25: CERT_STATUS_NON_UNIQUE_NAME            = 1 <<
10,
On 2010/10/09 00:18:56, wtc wrote:
> I'm still debating whether the "non-unique host name" error
> should be a certificate error.  abarth, what do you think?

It's one sensible way to model the situation.  In a sense, there's something
wrong with the certificate: it doesn't mean anything.

[Finnur, 2010-10-09 14:40:57]

http://codereview.chromium.org/3536019/diff/7002/13001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3536019/diff/7002/13001#newcode5777
chrome/app/generated_resources.grd:5777: The host is not unique. Lorum ipsum
dolores shot the sheriff.
Hahaha, no, I wasn't being serious! :)

I asked in the review request to get more formal strings and reiterated with Ian
in chat later. It is in progress, as far as I know.

I'll address the rest of your comments when I have more free time...

On 2010/10/09 00:18:56, wtc wrote:
> Are you serious?

[ian fette, 2010-10-09 15:04:25]

For text, would suggest: "The identity of the server you are connected to cannot
be fully validated. You are connected to a server using a name only valid within
your network, which an external certificate authority has no way to validate. In
this case, the certificate authority is <NAME OF CA>. If this is the
administrator of your local network, then the identity of the server can be
trusted. If not, anyone can obtain such a certificate and there is no way to
validate that you are connected to the intended website and not an attacker."

Thoughts?

On 2010/10/09 14:40:57, Finnur wrote:
> http://codereview.chromium.org/3536019/diff/7002/13001
> File chrome/app/generated_resources.grd (right):
> 
> http://codereview.chromium.org/3536019/diff/7002/13001#newcode5777
> chrome/app/generated_resources.grd:5777: The host is not unique. Lorum ipsum
> dolores shot the sheriff.
> Hahaha, no, I wasn't being serious! :)
> 
> I asked in the review request to get more formal strings and reiterated with
Ian
> in chat later. It is in progress, as far as I know.
> 
> I'll address the rest of your comments when I have more free time...
> 
> On 2010/10/09 00:18:56, wtc wrote:
> > Are you serious?

[abarth-chromium, 2010-10-09 15:09:50]

It doesn't matter who the CA is.  You can't trust the connection.  The
attacker can open a hidden iframe with the CA of his choosing and
attack that tab.

Adam


On Sat, Oct 9, 2010 at 8:04 AM,  <ian@chromium.org> wrote:
> For text, would suggest: "The identity of the server you are connected to
> cannot
> be fully validated. You are connected to a server using a name only valid
> within
> your network, which an external certificate authority has no way to
> validate. In
> this case, the certificate authority is <NAME OF CA>. If this is the
> administrator of your local network, then the identity of the server can be
> trusted. If not, anyone can obtain such a certificate and there is no way to
> validate that you are connected to the intended website and not an
> attacker."
>
> Thoughts?
>
> On 2010/10/09 14:40:57, Finnur wrote:
>>
>> http://codereview.chromium.org/3536019/diff/7002/13001
>> File chrome/app/generated_resources.grd (right):
>
>> http://codereview.chromium.org/3536019/diff/7002/13001#newcode5777
>> chrome/app/generated_resources.grd:5777: The host is not unique. Lorum
>> ipsum
>> dolores shot the sheriff.
>> Hahaha, no, I wasn't being serious! :)
>
>> I asked in the review request to get more formal strings and reiterated
>> with
>
> Ian
>>
>> in chat later. It is in progress, as far as I know.
>
>> I'll address the rest of your comments when I have more free time...
>
>> On 2010/10/09 00:18:56, wtc wrote:
>> > Are you serious?
>
>
>
> http://codereview.chromium.org/3536019/show
>

[abarth-chromium, 2010-10-09 15:10:42]

Also, to be clear, this triggers a broken lock, not a full page block, right?

Adam


On Sat, Oct 9, 2010 at 8:09 AM, Adam Barth <abarth@chromium.org> wrote:
> It doesn't matter who the CA is.  You can't trust the connection.  The
> attacker can open a hidden iframe with the CA of his choosing and
> attack that tab.
>
> Adam
>
>
> On Sat, Oct 9, 2010 at 8:04 AM,  <ian@chromium.org> wrote:
>> For text, would suggest: "The identity of the server you are connected to
>> cannot
>> be fully validated. You are connected to a server using a name only valid
>> within
>> your network, which an external certificate authority has no way to
>> validate. In
>> this case, the certificate authority is <NAME OF CA>. If this is the
>> administrator of your local network, then the identity of the server can be
>> trusted. If not, anyone can obtain such a certificate and there is no way to
>> validate that you are connected to the intended website and not an
>> attacker."
>>
>> Thoughts?
>>
>> On 2010/10/09 14:40:57, Finnur wrote:
>>>
>>> http://codereview.chromium.org/3536019/diff/7002/13001
>>> File chrome/app/generated_resources.grd (right):
>>
>>> http://codereview.chromium.org/3536019/diff/7002/13001#newcode5777
>>> chrome/app/generated_resources.grd:5777: The host is not unique. Lorum
>>> ipsum
>>> dolores shot the sheriff.
>>> Hahaha, no, I wasn't being serious! :)
>>
>>> I asked in the review request to get more formal strings and reiterated
>>> with
>>
>> Ian
>>>
>>> in chat later. It is in progress, as far as I know.
>>
>>> I'll address the rest of your comments when I have more free time...
>>
>>> On 2010/10/09 00:18:56, wtc wrote:
>>> > Are you serious?
>>
>>
>>
>> http://codereview.chromium.org/3536019/show
>>
>

[ian fette, 2010-10-09 15:36:07]

Sigh. In line for bagels, will send another try in an hour.
On Oct 9, 2010 8:09 AM, "Adam Barth" <abarth@chromium.org> wrote:
> It doesn't matter who the CA is. You can't trust the connection. The
> attacker can open a hidden iframe with the CA of his choosing and
> attack that tab.
>
> Adam
>
>
> On Sat, Oct 9, 2010 at 8:04 AM, <ian@chromium.org> wrote:
>> For text, would suggest: "The identity of the server you are connected to
>> cannot
>> be fully validated. You are connected to a server using a name only valid
>> within
>> your network, which an external certificate authority has no way to
>> validate. In
>> this case, the certificate authority is <NAME OF CA>. If this is the
>> administrator of your local network, then the identity of the server can
be
>> trusted. If not, anyone can obtain such a certificate and there is no way
to
>> validate that you are connected to the intended website and not an
>> attacker."
>>
>> Thoughts?
>>
>> On 2010/10/09 14:40:57, Finnur wrote:
>>>
>>> http://codereview.chromium.org/3536019/diff/7002/13001
>>> File chrome/app/generated_resources.grd (right):
>>
>>> http://codereview.chromium.org/3536019/diff/7002/13001#newcode5777
>>> chrome/app/generated_resources.grd:5777: The host is not unique. Lorum
>>> ipsum
>>> dolores shot the sheriff.
>>> Hahaha, no, I wasn't being serious! :)
>>
>>> I asked in the review request to get more formal strings and reiterated
>>> with
>>
>> Ian
>>>
>>> in chat later. It is in progress, as far as I know.
>>
>>> I'll address the rest of your comments when I have more free time...
>>
>>> On 2010/10/09 00:18:56, wtc wrote:
>>> > Are you serious?
>>
>>
>>
>> http://codereview.chromium.org/3536019/show
>>

[Finnur, 2010-10-11 10:42:33]

Ian: Still need the revised text from you, bagel-boy. :)

Adam: Correct, this does not result in a full-page block. However, the change
you reviewed showed this as a warning (gray lock with yellow warning sign
overlaid on top) whereas this updated cl shows this as an error (gray lock with
red x overlaid on top).

http://codereview.chromium.org/3536019/diff/7002/13004
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/3536019/diff/7002/13004#newcode189
chrome/browser/ssl/ssl_policy.cc:189: if (!(entry->ssl().cert_status() &
net::CERT_STATUS_COMMON_NAME_INVALID)) {
Moving it up to line 167 turns this into an error, as opposed to a warning
(error here meaning gray lock with red ex, not a full page block and not the
gray lock with the yellow warning sign on it). I read Adam's question about it
being a broken lock (and not a full page block) such that this is what is
intended.

On 2010/10/09 00:18:56, wtc wrote:
> This should probably be moved up to line 167.
> It seems that we should set the CERT_STATUS_NON_UNIQUE_NAME
> bit even if there are certificate errors (such as untrusted
> certificate).  I need to think about this more...

http://codereview.chromium.org/3536019/diff/7002/13005
File chrome/browser/toolbar_model.cc (right):

http://codereview.chromium.org/3536019/diff/7002/13005#newcode88
chrome/browser/toolbar_model.cc:88: net::CERT_STATUS_NON_UNIQUE_NAME));
The DCHECK is wrong, but there is a simple fix. However, since this is no longer
treated as a warning (due to other changes) we don't need to change this DCHECK.
See other comment for details.

On 2010/10/09 00:18:56, wtc wrote:
> This DCHECK is wrong, because the
> CERT_STATUS_UNABLE_TO_CHECK_REVOCATION and
> CERT_STATUS_NON_UNIQUE_NAME can both be set.
> 
> The correct DCHECK will have a very complicated expression,
> so perhaps we should just delete this DCHECK.

[ian fette, 2010-10-11 16:39:04]

"The identity of the server you are connected to cannot be fully validated. You
are connected to a server using a name only valid within your network, which an
external certificate authority has no way to validate ownership of. As some
certificate authorities will issue certificates for these names regardless,
there is no way to ensure you are connected to the intended website and not an
attacker."

On 2010/10/11 10:42:33, Finnur wrote:
> Ian: Still need the revised text from you, bagel-boy. :)
> 
> Adam: Correct, this does not result in a full-page block. However, the change
> you reviewed showed this as a warning (gray lock with yellow warning sign
> overlaid on top) whereas this updated cl shows this as an error (gray lock
with
> red x overlaid on top).
> 
> http://codereview.chromium.org/3536019/diff/7002/13004
> File chrome/browser/ssl/ssl_policy.cc (right):
> 
> http://codereview.chromium.org/3536019/diff/7002/13004#newcode189
> chrome/browser/ssl/ssl_policy.cc:189: if (!(entry->ssl().cert_status() &
> net::CERT_STATUS_COMMON_NAME_INVALID)) {
> Moving it up to line 167 turns this into an error, as opposed to a warning
> (error here meaning gray lock with red ex, not a full page block and not the
> gray lock with the yellow warning sign on it). I read Adam's question about it
> being a broken lock (and not a full page block) such that this is what is
> intended.
> 
> On 2010/10/09 00:18:56, wtc wrote:
> > This should probably be moved up to line 167.
> > It seems that we should set the CERT_STATUS_NON_UNIQUE_NAME
> > bit even if there are certificate errors (such as untrusted
> > certificate).  I need to think about this more...
> 
> http://codereview.chromium.org/3536019/diff/7002/13005
> File chrome/browser/toolbar_model.cc (right):
> 
> http://codereview.chromium.org/3536019/diff/7002/13005#newcode88
> chrome/browser/toolbar_model.cc:88: net::CERT_STATUS_NON_UNIQUE_NAME));
> The DCHECK is wrong, but there is a simple fix. However, since this is no
longer
> treated as a warning (due to other changes) we don't need to change this
DCHECK.
> See other comment for details.
> 
> On 2010/10/09 00:18:56, wtc wrote:
> > This DCHECK is wrong, because the
> > CERT_STATUS_UNABLE_TO_CHECK_REVOCATION and
> > CERT_STATUS_NON_UNIQUE_NAME can both be set.
> > 
> > The correct DCHECK will have a very complicated expression,
> > so perhaps we should just delete this DCHECK.

[abarth-chromium, 2010-10-11 17:32:12]

Yeah, I think we want the red x not the yellow triangle.  The attacker
can alter the behavior of the page, not just its appearance.

Adam


On Mon, Oct 11, 2010 at 9:39 AM,  <ian@chromium.org> wrote:
> "The identity of the server you are connected to cannot be fully validated.
> You
> are connected to a server using a name only valid within your network, which
> an
> external certificate authority has no way to validate ownership of. As some
> certificate authorities will issue certificates for these names regardless,
> there is no way to ensure you are connected to the intended website and not
> an
> attacker."
>
> On 2010/10/11 10:42:33, Finnur wrote:
>>
>> Ian: Still need the revised text from you, bagel-boy. :)
>
>> Adam: Correct, this does not result in a full-page block. However, the
>> change
>> you reviewed showed this as a warning (gray lock with yellow warning sign
>> overlaid on top) whereas this updated cl shows this as an error (gray lock
>
> with
>>
>> red x overlaid on top).
>
>> http://codereview.chromium.org/3536019/diff/7002/13004
>> File chrome/browser/ssl/ssl_policy.cc (right):
>
>> http://codereview.chromium.org/3536019/diff/7002/13004#newcode189
>> chrome/browser/ssl/ssl_policy.cc:189: if (!(entry->ssl().cert_status() &
>> net::CERT_STATUS_COMMON_NAME_INVALID)) {
>> Moving it up to line 167 turns this into an error, as opposed to a warning
>> (error here meaning gray lock with red ex, not a full page block and not
>> the
>> gray lock with the yellow warning sign on it). I read Adam's question
>> about it
>> being a broken lock (and not a full page block) such that this is what is
>> intended.
>
>> On 2010/10/09 00:18:56, wtc wrote:
>> > This should probably be moved up to line 167.
>> > It seems that we should set the CERT_STATUS_NON_UNIQUE_NAME
>> > bit even if there are certificate errors (such as untrusted
>> > certificate).  I need to think about this more...
>
>> http://codereview.chromium.org/3536019/diff/7002/13005
>> File chrome/browser/toolbar_model.cc (right):
>
>> http://codereview.chromium.org/3536019/diff/7002/13005#newcode88
>> chrome/browser/toolbar_model.cc:88: net::CERT_STATUS_NON_UNIQUE_NAME));
>> The DCHECK is wrong, but there is a simple fix. However, since this is no
>
> longer
>>
>> treated as a warning (due to other changes) we don't need to change this
>
> DCHECK.
>>
>> See other comment for details.
>
>> On 2010/10/09 00:18:56, wtc wrote:
>> > This DCHECK is wrong, because the
>> > CERT_STATUS_UNABLE_TO_CHECK_REVOCATION and
>> > CERT_STATUS_NON_UNIQUE_NAME can both be set.
>> >
>> > The correct DCHECK will have a very complicated expression,
>> > so perhaps we should just delete this DCHECK.
>
>
>
> http://codereview.chromium.org/3536019/show
>

[ian fette, 2010-10-11 17:50:43]

Adam, did the latest text work for you?

On Mon, Oct 11, 2010 at 10:31 AM, Adam Barth <abarth@chromium.org> wrote:

> Yeah, I think we want the red x not the yellow triangle.  The attacker
> can alter the behavior of the page, not just its appearance.
>
> Adam
>
>
> On Mon, Oct 11, 2010 at 9:39 AM,  <ian@chromium.org> wrote:
> > "The identity of the server you are connected to cannot be fully
> validated.
> > You
> > are connected to a server using a name only valid within your network,
> which
> > an
> > external certificate authority has no way to validate ownership of. As
> some
> > certificate authorities will issue certificates for these names
> regardless,
> > there is no way to ensure you are connected to the intended website and
> not
> > an
> > attacker."
> >
> > On 2010/10/11 10:42:33, Finnur wrote:
> >>
> >> Ian: Still need the revised text from you, bagel-boy. :)
> >
> >> Adam: Correct, this does not result in a full-page block. However, the
> >> change
> >> you reviewed showed this as a warning (gray lock with yellow warning
> sign
> >> overlaid on top) whereas this updated cl shows this as an error (gray
> lock
> >
> > with
> >>
> >> red x overlaid on top).
> >
> >> http://codereview.chromium.org/3536019/diff/7002/13004
> >> File chrome/browser/ssl/ssl_policy.cc (right):
> >
> >> http://codereview.chromium.org/3536019/diff/7002/13004#newcode189
> >> chrome/browser/ssl/ssl_policy.cc:189: if (!(entry->ssl().cert_status() &
> >> net::CERT_STATUS_COMMON_NAME_INVALID)) {
> >> Moving it up to line 167 turns this into an error, as opposed to a
> warning
> >> (error here meaning gray lock with red ex, not a full page block and not
> >> the
> >> gray lock with the yellow warning sign on it). I read Adam's question
> >> about it
> >> being a broken lock (and not a full page block) such that this is what
> is
> >> intended.
> >
> >> On 2010/10/09 00:18:56, wtc wrote:
> >> > This should probably be moved up to line 167.
> >> > It seems that we should set the CERT_STATUS_NON_UNIQUE_NAME
> >> > bit even if there are certificate errors (such as untrusted
> >> > certificate).  I need to think about this more...
> >
> >> http://codereview.chromium.org/3536019/diff/7002/13005
> >> File chrome/browser/toolbar_model.cc (right):
> >
> >> http://codereview.chromium.org/3536019/diff/7002/13005#newcode88
> >> chrome/browser/toolbar_model.cc:88: net::CERT_STATUS_NON_UNIQUE_NAME));
> >> The DCHECK is wrong, but there is a simple fix. However, since this is
> no
> >
> > longer
> >>
> >> treated as a warning (due to other changes) we don't need to change this
> >
> > DCHECK.
> >>
> >> See other comment for details.
> >
> >> On 2010/10/09 00:18:56, wtc wrote:
> >> > This DCHECK is wrong, because the
> >> > CERT_STATUS_UNABLE_TO_CHECK_REVOCATION and
> >> > CERT_STATUS_NON_UNIQUE_NAME can both be set.
> >> >
> >> > The correct DCHECK will have a very complicated expression,
> >> > so perhaps we should just delete this DCHECK.
> >
> >
> >
> > http://codereview.chromium.org/3536019/show
> >
>

[abarth-chromium, 2010-10-11 17:55:13]

Yeah, looks great.

On Mon, Oct 11, 2010 at 10:49 AM, Ian Fette <ian@chromium.org> wrote:
> Adam, did the latest text work for you?
>
> On Mon, Oct 11, 2010 at 10:31 AM, Adam Barth <abarth@chromium.org> wrote:
>>
>> Yeah, I think we want the red x not the yellow triangle.  The attacker
>> can alter the behavior of the page, not just its appearance.
>>
>> Adam
>>
>>
>> On Mon, Oct 11, 2010 at 9:39 AM,  <ian@chromium.org> wrote:
>> > "The identity of the server you are connected to cannot be fully
>> > validated.
>> > You
>> > are connected to a server using a name only valid within your network,
>> > which
>> > an
>> > external certificate authority has no way to validate ownership of. As
>> > some
>> > certificate authorities will issue certificates for these names
>> > regardless,
>> > there is no way to ensure you are connected to the intended website and
>> > not
>> > an
>> > attacker."
>> >
>> > On 2010/10/11 10:42:33, Finnur wrote:
>> >>
>> >> Ian: Still need the revised text from you, bagel-boy. :)
>> >
>> >> Adam: Correct, this does not result in a full-page block. However, the
>> >> change
>> >> you reviewed showed this as a warning (gray lock with yellow warning
>> >> sign
>> >> overlaid on top) whereas this updated cl shows this as an error (gray
>> >> lock
>> >
>> > with
>> >>
>> >> red x overlaid on top).
>> >
>> >> http://codereview.chromium.org/3536019/diff/7002/13004
>> >> File chrome/browser/ssl/ssl_policy.cc (right):
>> >
>> >> http://codereview.chromium.org/3536019/diff/7002/13004#newcode189
>> >> chrome/browser/ssl/ssl_policy.cc:189: if (!(entry->ssl().cert_status()
>> >> &
>> >> net::CERT_STATUS_COMMON_NAME_INVALID)) {
>> >> Moving it up to line 167 turns this into an error, as opposed to a
>> >> warning
>> >> (error here meaning gray lock with red ex, not a full page block and
>> >> not
>> >> the
>> >> gray lock with the yellow warning sign on it). I read Adam's question
>> >> about it
>> >> being a broken lock (and not a full page block) such that this is what
>> >> is
>> >> intended.
>> >
>> >> On 2010/10/09 00:18:56, wtc wrote:
>> >> > This should probably be moved up to line 167.
>> >> > It seems that we should set the CERT_STATUS_NON_UNIQUE_NAME
>> >> > bit even if there are certificate errors (such as untrusted
>> >> > certificate).  I need to think about this more...
>> >
>> >> http://codereview.chromium.org/3536019/diff/7002/13005
>> >> File chrome/browser/toolbar_model.cc (right):
>> >
>> >> http://codereview.chromium.org/3536019/diff/7002/13005#newcode88
>> >> chrome/browser/toolbar_model.cc:88: net::CERT_STATUS_NON_UNIQUE_NAME));
>> >> The DCHECK is wrong, but there is a simple fix. However, since this is
>> >> no
>> >
>> > longer
>> >>
>> >> treated as a warning (due to other changes) we don't need to change
>> >> this
>> >
>> > DCHECK.
>> >>
>> >> See other comment for details.
>> >
>> >> On 2010/10/09 00:18:56, wtc wrote:
>> >> > This DCHECK is wrong, because the
>> >> > CERT_STATUS_UNABLE_TO_CHECK_REVOCATION and
>> >> > CERT_STATUS_NON_UNIQUE_NAME can both be set.
>> >> >
>> >> > The correct DCHECK will have a very complicated expression,
>> >> > so perhaps we should just delete this DCHECK.
>> >
>> >
>> >
>> > http://codereview.chromium.org/3536019/show
>> >
>
>

[ian fette, 2010-10-11 17:55:41]

Adam, did the latest text work for you?

On Mon, Oct 11, 2010 at 10:31 AM, Adam Barth <abarth@chromium.org> wrote:

> Yeah, I think we want the red x not the yellow triangle.  The attacker
> can alter the behavior of the page, not just its appearance.
>
> Adam
>
>
> On Mon, Oct 11, 2010 at 9:39 AM,  <ian@chromium.org> wrote:
> > "The identity of the server you are connected to cannot be fully
> validated.
> > You
> > are connected to a server using a name only valid within your network,
> which
> > an
> > external certificate authority has no way to validate ownership of. As
> some
> > certificate authorities will issue certificates for these names
> regardless,
> > there is no way to ensure you are connected to the intended website and
> not
> > an
> > attacker."
> >
> > On 2010/10/11 10:42:33, Finnur wrote:
> >>
> >> Ian: Still need the revised text from you, bagel-boy. :)
> >
> >> Adam: Correct, this does not result in a full-page block. However, the
> >> change
> >> you reviewed showed this as a warning (gray lock with yellow warning
> sign
> >> overlaid on top) whereas this updated cl shows this as an error (gray
> lock
> >
> > with
> >>
> >> red x overlaid on top).
> >
> >> http://codereview.chromium.org/3536019/diff/7002/13004
> >> File chrome/browser/ssl/ssl_policy.cc (right):
> >
> >> http://codereview.chromium.org/3536019/diff/7002/13004#newcode189
> >> chrome/browser/ssl/ssl_policy.cc:189: if (!(entry->ssl().cert_status() &
> >> net::CERT_STATUS_COMMON_NAME_INVALID)) {
> >> Moving it up to line 167 turns this into an error, as opposed to a
> warning
> >> (error here meaning gray lock with red ex, not a full page block and not
> >> the
> >> gray lock with the yellow warning sign on it). I read Adam's question
> >> about it
> >> being a broken lock (and not a full page block) such that this is what
> is
> >> intended.
> >
> >> On 2010/10/09 00:18:56, wtc wrote:
> >> > This should probably be moved up to line 167.
> >> > It seems that we should set the CERT_STATUS_NON_UNIQUE_NAME
> >> > bit even if there are certificate errors (such as untrusted
> >> > certificate).  I need to think about this more...
> >
> >> http://codereview.chromium.org/3536019/diff/7002/13005
> >> File chrome/browser/toolbar_model.cc (right):
> >
> >> http://codereview.chromium.org/3536019/diff/7002/13005#newcode88
> >> chrome/browser/toolbar_model.cc:88: net::CERT_STATUS_NON_UNIQUE_NAME));
> >> The DCHECK is wrong, but there is a simple fix. However, since this is
> no
> >
> > longer
> >>
> >> treated as a warning (due to other changes) we don't need to change this
> >
> > DCHECK.
> >>
> >> See other comment for details.
> >
> >> On 2010/10/09 00:18:56, wtc wrote:
> >> > This DCHECK is wrong, because the
> >> > CERT_STATUS_UNABLE_TO_CHECK_REVOCATION and
> >> > CERT_STATUS_NON_UNIQUE_NAME can both be set.
> >> >
> >> > The correct DCHECK will have a very complicated expression,
> >> > so perhaps we should just delete this DCHECK.
> >
> >
> >
> > http://codereview.chromium.org/3536019/show
> >
>

[Finnur, 2010-10-11 19:26:19]

Uploaded new cl (just changed it to use the text Ian sent).

I believe Adam has given the go-ahead. Changes that Scott reviewed have been
removed. Wan-Teh, do you have further comments about this cl?



On 2010/10/11 17:55:41, ian fette wrote:
> Adam, did the latest text work for you?
> 
> On Mon, Oct 11, 2010 at 10:31 AM, Adam Barth <mailto:abarth@chromium.org>
wrote:
> 
> > Yeah, I think we want the red x not the yellow triangle.  The attacker
> > can alter the behavior of the page, not just its appearance.
> >
> > Adam
> >
> >
> > On Mon, Oct 11, 2010 at 9:39 AM,  <mailto:ian@chromium.org> wrote:
> > > "The identity of the server you are connected to cannot be fully
> > validated.
> > > You
> > > are connected to a server using a name only valid within your network,
> > which
> > > an
> > > external certificate authority has no way to validate ownership of. As
> > some
> > > certificate authorities will issue certificates for these names
> > regardless,
> > > there is no way to ensure you are connected to the intended website and
> > not
> > > an
> > > attacker."
> > >
> > > On 2010/10/11 10:42:33, Finnur wrote:
> > >>
> > >> Ian: Still need the revised text from you, bagel-boy. :)
> > >
> > >> Adam: Correct, this does not result in a full-page block. However, the
> > >> change
> > >> you reviewed showed this as a warning (gray lock with yellow warning
> > sign
> > >> overlaid on top) whereas this updated cl shows this as an error (gray
> > lock
> > >
> > > with
> > >>
> > >> red x overlaid on top).
> > >
> > >> http://codereview.chromium.org/3536019/diff/7002/13004
> > >> File chrome/browser/ssl/ssl_policy.cc (right):
> > >
> > >> http://codereview.chromium.org/3536019/diff/7002/13004#newcode189
> > >> chrome/browser/ssl/ssl_policy.cc:189: if (!(entry->ssl().cert_status() &
> > >> net::CERT_STATUS_COMMON_NAME_INVALID)) {
> > >> Moving it up to line 167 turns this into an error, as opposed to a
> > warning
> > >> (error here meaning gray lock with red ex, not a full page block and not
> > >> the
> > >> gray lock with the yellow warning sign on it). I read Adam's question
> > >> about it
> > >> being a broken lock (and not a full page block) such that this is what
> > is
> > >> intended.
> > >
> > >> On 2010/10/09 00:18:56, wtc wrote:
> > >> > This should probably be moved up to line 167.
> > >> > It seems that we should set the CERT_STATUS_NON_UNIQUE_NAME
> > >> > bit even if there are certificate errors (such as untrusted
> > >> > certificate).  I need to think about this more...
> > >
> > >> http://codereview.chromium.org/3536019/diff/7002/13005
> > >> File chrome/browser/toolbar_model.cc (right):
> > >
> > >> http://codereview.chromium.org/3536019/diff/7002/13005#newcode88
> > >> chrome/browser/toolbar_model.cc:88: net::CERT_STATUS_NON_UNIQUE_NAME));
> > >> The DCHECK is wrong, but there is a simple fix. However, since this is
> > no
> > >
> > > longer
> > >>
> > >> treated as a warning (due to other changes) we don't need to change this
> > >
> > > DCHECK.
> > >>
> > >> See other comment for details.
> > >
> > >> On 2010/10/09 00:18:56, wtc wrote:
> > >> > This DCHECK is wrong, because the
> > >> > CERT_STATUS_UNABLE_TO_CHECK_REVOCATION and
> > >> > CERT_STATUS_NON_UNIQUE_NAME can both be set.
> > >> >
> > >> > The correct DCHECK will have a very complicated expression,
> > >> > so perhaps we should just delete this DCHECK.
> > >
> > >
> > >
> > > http://codereview.chromium.org/3536019/show
> > >
> >
>

[wtc, 2010-10-11 19:56:00]

LGTM.  Thanks for fixing the typos in net_error_list.

http://codereview.chromium.org/3536019/diff/44001/45006
File net/base/net_error_list.h (right):

http://codereview.chromium.org/3536019/diff/44001/45006#newcode281
net/base/net_error_list.h:281: // The host specified in the certificate is not
unique.
Nit: host => host name

Or just "name".