Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
184483002:
Set insecure content status also when there are other security issues. (Closed)
|
Created:
6 years, 9 months ago by joleksy Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionSSLPolicy::UpdateEntry is returning when it encounters first error.
Because of this, SSLStatus::DISPLAYED_INSECURE_CONTENT should be set before other errors are examined.
This handles the case when not all flags are set if there are some problems with the certificate on the page *and* insecure content is loaded.
BUG=348560
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259743
Patch Set 1 #Patch Set 2 : Added browser test & set insecure content flag only in secure connection. #
Total comments: 4
Patch Set 3 : Review follow up: remove bool arguments, introduce enums. #
Messages
Total messages: 17 (0 generated)
@reviewer: does this patch look OK to you?
[+rsleevi] I probably shouldn't own this code anymore. We might want to change the OWNERS file to match this one: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ssl/OWNERS If I were to review your CL, however, I would ask for a test.
On 2014/03/01 06:06:04, abarth wrote: > [+rsleevi] > > I probably shouldn't own this code anymore. We might want to change the OWNERS > file to match this one: > > http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ssl/OWNERS > > If I were to review your CL, however, I would ask for a test. abarth: jam oppose(d/s) it - https://codereview.chromium.org/22265007/ That said, I'm definitely concerned about the correctness of this, so tests are needed - and then some. For example, this CL makes it possible for SSLStatus::DISPLAYED_INSECURE_CONTENT to be set along with SECURITY_STYLE_UNAUTHENTICATED when there is no cert. I have no clue what code may be expecting that these flags may or may not be mixed in combination (but inevitably it seems the same) There also should be a BUG= line, along with a Chromium bug that explains this issue further.
On 2014/03/03 02:07:32, Ryan Sleevi wrote: > abarth: jam oppose(d/s) it - https://codereview.chromium.org/22265007/ Ok. Maybe I should just delete myself from the file then. :)
I have my doubts regarding TestRunsInsecureContentTwoTabs, see comments. https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_browser_tests.cc:1044: CheckAuthenticationBrokenState(tab1, 0, false, true, false); Note: RAN_INSECURE_CONTENT is stored at host level, see SSLHostState::HostRanInsecureContent. DISPLAYED_INSECURE_CONTENT is stored in WebContents, this is why it is not set here. Looks fine to me, but could anyone verify that this is ok?
On 2014/03/03 11:22:45, joleksy wrote: > I have my doubts regarding TestRunsInsecureContentTwoTabs, see comments. > > https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... > chrome/browser/ssl/ssl_browser_tests.cc:1044: > CheckAuthenticationBrokenState(tab1, 0, false, true, false); > Note: RAN_INSECURE_CONTENT is stored at host level, see > SSLHostState::HostRanInsecureContent. > DISPLAYED_INSECURE_CONTENT is stored in WebContents, this is why it is not set > here. > > Looks fine to me, but could anyone verify that this is ok? Note: there is also another patch related to insecure content: https://codereview.chromium.org/181253003/ When it is integrated, some tests will have to be changed for this one (and vice-versa).
https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_browser_tests.cc:159: bool interstitial) { I'm wondering whether we should change these three bools into more symbolic enums. CheckAuthenticationBrokenState(blah, error, false, true, false); CheckAuthenticationBrokenState(blah, error, true, true, true); vs CheckAuthenticationBrokenState(blah, error, RAN_INSECURE_CONTENT); CheckAuthenticationBrokenState(blah, error, DISPLAYED_INSECURE_CONTENT | RAN_INSECURE_CONTENT | SHOWING_INTERSTITIAL); https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... chrome/browser/ssl/ssl_browser_tests.cc:1044: CheckAuthenticationBrokenState(tab1, 0, false, true, false); On 2014/03/03 11:22:45, joleksy wrote: > Note: RAN_INSECURE_CONTENT is stored at host level, see > SSLHostState::HostRanInsecureContent. > DISPLAYED_INSECURE_CONTENT is stored in WebContents, this is why it is not set > here. > > Looks fine to me, but could anyone verify that this is ok? Right. Active content has the potential of corrupting the renderer / JS environment, which is why it's stored at the host level. Passive content (eg: images) is only on a per-page level, and doesn't affect the overall renderer security (mod popping libtiff / libpng or the like). That's at least the logic behind the asymmetry. https://codereview.chromium.org/184483002/diff/20001/content/browser/ssl/ssl_... File content/browser/ssl/ssl_policy.cc (right): https://codereview.chromium.org/184483002/diff/20001/content/browser/ssl/ssl_... content/browser/ssl/ssl_policy.cc:125: entry->GetSSL().content_status |= SSLStatus::DISPLAYED_INSECURE_CONTENT; I'm not sure I understand why you set this here, rather than 115. What's the reasoning? Because UNAUTHENTICATED shouldn't be combined with insecure content?
On 2014/03/11 01:46:54, Ryan Sleevi wrote: > https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... > chrome/browser/ssl/ssl_browser_tests.cc:159: bool interstitial) { > I'm wondering whether we should change these three bools into more symbolic > enums. > > CheckAuthenticationBrokenState(blah, error, false, true, false); > CheckAuthenticationBrokenState(blah, error, true, true, true); > > vs > > CheckAuthenticationBrokenState(blah, error, RAN_INSECURE_CONTENT); > CheckAuthenticationBrokenState(blah, error, > DISPLAYED_INSECURE_CONTENT | > RAN_INSECURE_CONTENT | > SHOWING_INTERSTITIAL); > +1 Would be definitely more readable. Just say a word and I will change it:-) > https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... > chrome/browser/ssl/ssl_browser_tests.cc:1044: > CheckAuthenticationBrokenState(tab1, 0, false, true, false); > On 2014/03/03 11:22:45, joleksy wrote: > > Note: RAN_INSECURE_CONTENT is stored at host level, see > > SSLHostState::HostRanInsecureContent. > > DISPLAYED_INSECURE_CONTENT is stored in WebContents, this is why it is not set > > here. > > > > Looks fine to me, but could anyone verify that this is ok? > > Right. Active content has the potential of corrupting the renderer / JS > environment, which is why it's stored at the host level. > > Passive content (eg: images) is only on a per-page level, and doesn't affect the > overall renderer security (mod popping libtiff / libpng or the like). That's at > least the logic behind the asymmetry. > Thanks! > https://codereview.chromium.org/184483002/diff/20001/content/browser/ssl/ssl_... > File content/browser/ssl/ssl_policy.cc (right): > > https://codereview.chromium.org/184483002/diff/20001/content/browser/ssl/ssl_... > content/browser/ssl/ssl_policy.cc:125: entry->GetSSL().content_status |= > SSLStatus::DISPLAYED_INSECURE_CONTENT; > I'm not sure I understand why you set this here, rather than 115. What's the > reasoning? > > Because UNAUTHENTICATED shouldn't be combined with insecure content? Yes, that was my idea. Also because of your comment: "this CL makes it possible for SSLStatus::DISPLAYED_INSECURE_CONTENT to be set along with SECURITY_STYLE_UNAUTHENTICATED when there is no cert" Anyway, this way it seems more logical to me.
On 2014/03/13 08:14:48, joleksy wrote: > On 2014/03/11 01:46:54, Ryan Sleevi wrote: > > > https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... > > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > > > > https://codereview.chromium.org/184483002/diff/20001/chrome/browser/ssl/ssl_b... > > chrome/browser/ssl/ssl_browser_tests.cc:159: bool interstitial) { > > I'm wondering whether we should change these three bools into more symbolic > > enums. > > > > CheckAuthenticationBrokenState(blah, error, false, true, false); > > CheckAuthenticationBrokenState(blah, error, true, true, true); > > > > vs > > > > CheckAuthenticationBrokenState(blah, error, RAN_INSECURE_CONTENT); > > CheckAuthenticationBrokenState(blah, error, > > DISPLAYED_INSECURE_CONTENT | > > RAN_INSECURE_CONTENT | > > SHOWING_INTERSTITIAL); > > > > +1 > Would be definitely more readable. Just say a word and I will change it:-) Yeah, in the Chromium spirit of "leave the code better than you found it" - and especially because the extra bool adds a growing readability hurdle - let's make this change in this CL (since we're adding yet-another-bool)
LGTM For OWNERS rubberstamp, abarth is needed.
On 2014/03/14 21:39:20, Ryan Sleevi wrote: > For OWNERS rubberstamp, abarth is needed. abarth, could you take a look?
I don't want to be an OWNER of this code anymore. Can we not make someone who actually works on this code an OWNER?
On 2014/03/26 16:45:52, abarth wrote: > I don't want to be an OWNER of this code anymore. Can we not make someone who > actually works on this code an OWNER? abarth: That's the thing. No one works on this code, and the most likely candidates (eg: src/chrome/browser/ssl/OWNERS ) jam@ wasn't a fan of adding to content/browser/ssl/OWNERS My hope is that either joleksy will want to apply for OWNERS in the future, or the reviews will help convince jam@ that it's better to have someone invested in the code in OWNERS :)
On 2014/03/26 17:42:33, Ryan Sleevi wrote: > abarth: That's the thing. No one works on this code, and the most likely > candidates (eg: src/chrome/browser/ssl/OWNERS ) jam@ wasn't a fan of adding to > content/browser/ssl/OWNERS I guess that's the curse of writing code that works. :) > My hope is that either joleksy will want to apply for OWNERS in the future, or > the reviews will help convince jam@ that it's better to have someone invested in > the code in OWNERS :) Ok. rslgtm. Sorry for being grumpy. It's just that I wrote this code like five years ago.
The CQ bit was checked by rsleevi@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/joleksy@opera.com/184483002/40001
Message was sent while issue was closed.
Change committed as 259743 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
