SSLPolicy fix: Step 9.

SSLPolicy fix: Step 9 of 9 (hopefully!).

Change our algorithm for computing the state of our SSL security indicators.  Previously, we were computing this state for a single navigation entry.  Although this matches other browsers, it fails to take the same-origin policy into account.  For example, if one tab is contaminated with insecure content, that insecure content can spread to all the tabs in the same security origin.

R=jcampan,wtc
BUG=8706
TEST=SSLUITest.TestMixedContentsRandomizeHash,SSLUITest.TestMixedContentsTwoTabs


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

[abarth-chromium, 2009-03-18 23:03:59]

Hopefully this will be the last step in fixing this bug.  :)

[jcampan, 2009-03-19 17:18:45]

http://codereview.chromium.org/42314/diff/39/44
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/42314/diff/39/44#newcode279
Line 279: entry->ssl().set_security_style(SECURITY_STYLE_AUTHENTICATION_BROKEN);
Since mixed-content is now reported as AUTHENTICATION_BROKEN, does that mean we
get a broken keypad in that case?
I thought we wanted to show unauthenticated in that case.

http://codereview.chromium.org/42314/diff/39/51
File chrome/browser/ssl/ssl_uitest.cc (right):

http://codereview.chromium.org/42314/diff/39/51#newcode258
Line 258: // Load a page with mixed-content, the default behavior is to show the
mixed
Comment isn't accurate. We are filtering here.

http://codereview.chromium.org/42314/diff/39/51#newcode426
Line 426: // Visits a page that opens a new tab with
Comment is incomplete.

[abarth-chromium, 2009-03-19 19:27:49]

http://codereview.chromium.org/42314/diff/39/44
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/42314/diff/39/44#newcode279
Line 279: entry->ssl().set_security_style(SECURITY_STYLE_AUTHENTICATION_BROKEN);
On 2009/03/19 17:18:45, jcampan wrote:
> Since mixed-content is now reported as AUTHENTICATION_BROKEN, does that mean
we
> get a broken keypad in that case?

The UI looks the same as our current UI to me.  My original intention for
AUTHENTICATION_BROKEN was "this was supposed to be authenticated, but something
went wrong and we can't promise that its authenticated any more," which matches
the mixed content scenario here.

We can probably delete this line of code without really changing anything. 
Should we try that?

http://codereview.chromium.org/42314/diff/39/51
File chrome/browser/ssl/ssl_uitest.cc (right):

http://codereview.chromium.org/42314/diff/39/51#newcode258
Line 258: // Load a page with mixed-content, the default behavior is to show the
mixed
Fixed.

http://codereview.chromium.org/42314/diff/39/51#newcode426
Line 426: // Visits a page that opens a new tab with
Fixed.

[wtc, 2009-03-19 21:12:03]

LGTM.

I reviewed Patch Set 5, not the latest Patch Set 6.
In most places I did a superficial review because
I am not familiar with the mixed content code.  I'm
counting on jcampan to do a thorough review of this
important piece of code.

http://codereview.chromium.org/42314/diff/39/40
File chrome/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/42314/diff/39/40#newcode641
Line 641: UpdateEntry(entry);
Question: do we need to call UpdateEntry(entry) even if
details->is_main_frame is false?

http://codereview.chromium.org/42314/diff/39/44
File chrome/browser/ssl/ssl_policy.cc (left):

http://codereview.chromium.org/42314/diff/39/44#oldcode319
Line 319: // Default behavior for rejecting a certificate.
Nit: I think it's fine to keep this comment, but if you
delete this, also delete the "Default behavior for accepting
a certificate" comment below for consistency.

http://codereview.chromium.org/42314/diff/39/44
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/42314/diff/39/44#newcode41
Line 41: static void MarkOriginAsBroken(SSLManager* manager, const std::string&
origin) {
Nit: no need to declare these four functions as file-scope
static because they're already in an anonymous namespace.
But I see that the existing functions below are already
declared as file-scope static.  Oh well.

http://codereview.chromium.org/42314/diff/39/44#newcode58
Line 58: static void UpdateStateForMixedContent(SSLManager::RequestInfo* info) {
This function is the meat of this CL, isn't it?  It seems
to be the new definition of mixed content.

http://codereview.chromium.org/42314/diff/39/44#newcode66
Line 66: // The user approved a mixed content exception for the main frame's
origin.
I don't understand this part.  How do you know the user
approved a mixed content exception for the main frame's
origin?

It seems that I can understand this function without this
comment -- if a resource is unsafe, we mark both its frame's
origin and its main frame's origin as mixed content.  This
is what the ShowMixedContentTask::Run() method below does,
too.

http://codereview.chromium.org/42314/diff/39/44#newcode228
Line 228: std::string host = GURL(handler->main_frame_origin()).host();
Hmm... this line makes me wonder if main_frame_origin()
should be a GURL.

http://codereview.chromium.org/42314/diff/39/44#newcode263
Line 263: // An HTTPS response may not have a certificate for some reason.  When
that
With your recent work on the SSL tunnel CONNECT failure
handling, this should not happen.  It's fine to keep this
code just in case we change SSL tunnel CONNECT failure
handling in the future.

http://codereview.chromium.org/42314/diff/39/44#newcode266
Line 266: if (!entry->ssl().cert_id()) {
Perhaps add a NOTREACHED() assertion here?

http://codereview.chromium.org/42314/diff/39/44#newcode271
Line 271: std::string host = entry->url().host();
Nit: move this line after the following if statement
because the if statement doesn't use 'host'.

http://codereview.chromium.org/42314/diff/39/44#newcode307
Line 307: // We aren't worried about mixed content if we're loading an HTTPS,
about,
Why do you mention 'about' and 'data' here?  The code only
tests url.SchemeIsSecure().

[abarth-chromium, 2009-03-19 21:14:07]

http://codereview.chromium.org/42314/diff/65/70
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/42314/diff/65/70#newcode279
Line 279: entry->ssl().set_has_mixed_content();
After further discussion with Jay via email, he convinced me that
set_security_style here was wrong.  Fixed.

[abarth-chromium, 2009-03-19 21:34:00]

Thanks for your comments Wan-Teh.

http://codereview.chromium.org/42314/diff/39/40
File chrome/browser/ssl/ssl_manager.cc (right):

http://codereview.chromium.org/42314/diff/39/40#newcode641
Line 641: UpdateEntry(entry);
Yes.  This is because we need to update the entry on Back/Forward if our
internal state has changed.  This case is covered by a UI test.

http://codereview.chromium.org/42314/diff/39/44
File chrome/browser/ssl/ssl_policy.cc (left):

http://codereview.chromium.org/42314/diff/39/44#oldcode319
Line 319: // Default behavior for rejecting a certificate.
Fixed.  I put the comment back.  I don't think its a super insightful comment,
but its not harming anyone.

http://codereview.chromium.org/42314/diff/39/44
File chrome/browser/ssl/ssl_policy.cc (right):

http://codereview.chromium.org/42314/diff/39/44#newcode58
Line 58: static void UpdateStateForMixedContent(SSLManager::RequestInfo* info) {
Yes.  The rest is machinery to make this function work.

http://codereview.chromium.org/42314/diff/39/44#newcode66
Line 66: // The user approved a mixed content exception for the main frame's
origin.
Yeah, saying the user has approved the exception is probably a bit strong
because we automatically approve them in the default configuration.  I'll update
the comment.

http://codereview.chromium.org/42314/diff/39/44#newcode228
Line 228: std::string host = GURL(handler->main_frame_origin()).host();
It's almost a GURL, but not quite.  For example, we need to represent the
security origin "null", which isn't representable as a GURL...  Maybe we should
have a base::SecurityOrigin type?  That would make a convent place to put the
constant for "null" ...  I'll do this in a followup patch.

http://codereview.chromium.org/42314/diff/39/44#newcode263
Line 263: // An HTTPS response may not have a certificate for some reason.  When
that
This still happens.  For example, https://sdflasjdflkds.com gives back an error
page (host not found) without a proxy.  This code makes that location bar white
instead of yellow.

http://codereview.chromium.org/42314/diff/39/44#newcode271
Line 271: std::string host = entry->url().host();
Fixed.

http://codereview.chromium.org/42314/diff/39/44#newcode307
Line 307: // We aren't worried about mixed content if we're loading an HTTPS,
about,
The old code tested for these, but they don't matter any more since they're
handled internally by the renderer.  Comment removed.

[jcampan, 2009-03-19 23:26:30]

LGTM