Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
794023002:
Remember user's decisions on SSL errors. (Closed)
|
Created:
6 years ago by hush (inactive) Modified:
6 years ago CC:
chromium-reviews, android-webview-reviews_chromium.org, benm (inactive), jww Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemember user's decisions on SSL errors.
After CL: https://codereview.chromium.org/369703002, Android
WebView just has a NULL SSLHostStateDelegate. So it does not remember
any user decisions on SSL errors. This is a regression of behavior from
L (m37).
History:
JB behavior: Larger error codes are assumed to have higher severity. And
if the user has allowed an SSL error with a high severity, the user
won't be prompted for a lower severity SSL error.
K and L behavior: A specific SSL error will be allowed only if the error
bit field is a subset of previously allowed error codes.
trunk behavior for webview (without this patch): We don't remember
user's decision at all.
This CL:
Maintain the same behavior with K and L.
BUG=441065
Committed: https://crrev.com/8e5bcae56618fb6f5d5ad677b0206b943221193a
Cr-Commit-Position: refs/heads/master@{#308160}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : comments #Patch Set 3 : Implement with SSLHostStateDelegate #
Total comments: 12
Patch Set 4 : Added another test #Patch Set 5 : comments #
Total comments: 26
Patch Set 6 : comments #
Messages
Total messages: 22 (6 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
hush@chromium.org changed reviewers: + sgurun@chromium.org
PTAL https://codereview.chromium.org/794023002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/794023002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:67: return sb.toString().hashCode(); this is for the index in hashMap. Is it okay to do so? (there is indeed a small chance that different errors collide into the same hashCode)
https://codereview.chromium.org/794023002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/794023002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:67: return sb.toString().hashCode(); That would work, but it would be cleaner to do something like: { return host.hashCode() ^ port ^ errorCode ^ Arrays.hashCode(certBytes); }
https://codereview.chromium.org/794023002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java (right): https://codereview.chromium.org/794023002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContentsClientBridge.java:67: return sb.toString().hashCode(); On 2014/12/11 19:31:08, paulmiller wrote: > That would work, but it would be cleaner to do something like: > { return host.hashCode() ^ port ^ errorCode ^ Arrays.hashCode(certBytes); } That's cool. I should have used that....
cc-ing jww@, who did a lot of work around this area.
Hi Selim, Like we discussed, I implemented using SSLHostStateDelegate on the native side in PS3.
Patchset #4 (id:100001) has been deleted
jww@chromium.org changed reviewers: + jww@chromium.org
Drive by adding myself as a reviewer. https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... File android_webview/browser/aw_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. See comment on Copyright in aw_ssl_host_state_delegate.h. https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.cc:39: allowed_[cert.fingerprint()] = error; This should store the entire chain, not just the end cert fingerprint. You can use net::X509Certificate::CalculateChainFingerprint256 as shown in https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl..., to do this. Actually, you might consider making "GetKey()" from that file into a general utility function somewhere to share the code. https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.cc:43: AwSSLHostStateDelegate::AwSSLHostStateDelegate() { Nit: This constructor and destructor be inline in the header https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... File android_webview/browser/aw_ssl_host_state_delegate.h (right): https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: I think this should be "Copyright (c) 2014" https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.h:36: std::map<net::SHA1HashValue, net::CertStatus, net::SHA1HashValueLessThan> As a rule, we should never use SHA1 hashes for security decisions as they are know to be weak. Fortunately, we have SHA256 fingerprints on cert chains in net/ now. See https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl... to see how this is implemented for the main browser.
Thanks Joel for the comments! https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... File android_webview/browser/aw_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/12/12 01:48:41, jww wrote: > See comment on Copyright in aw_ssl_host_state_delegate.h. Done. https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.cc:39: allowed_[cert.fingerprint()] = error; On 2014/12/12 01:48:41, jww wrote: > This should store the entire chain, not just the end cert fingerprint. You can > use net::X509Certificate::CalculateChainFingerprint256 as shown in > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl..., > to do this. Actually, you might consider making "GetKey()" from that file into a > general utility function somewhere to share the code. I just used "CalculateChainFingerprint256" to get the chain fingerprint, then use it as the hashmap key without mixing the error code with it, because in our case here, the error code is the "value" in the |allowed_| hashmap. As a result, I don't really have to extract the getKey() from chrome/ to somewhere else. Does this sound good? https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.cc:43: AwSSLHostStateDelegate::AwSSLHostStateDelegate() { On 2014/12/12 01:48:41, jww wrote: > Nit: This constructor and destructor be inline in the header Sorry, I tried to do this. But I got a compiler error: "[chromium-style] Complex constructor has an inlined body." Looking at tools/clang/plugins/FindBadConstructsConsumer.cpp, I guess people just don't want us to inline constructors. https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... File android_webview/browser/aw_ssl_host_state_delegate.h (right): https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2014/12/12 01:48:41, jww wrote: > nit: I think this should be "Copyright (c) 2014" Done. https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.h:36: std::map<net::SHA1HashValue, net::CertStatus, net::SHA1HashValueLessThan> On 2014/12/12 01:48:41, jww wrote: > As a rule, we should never use SHA1 hashes for security decisions as they are > know to be weak. Fortunately, we have SHA256 fingerprints on cert chains in net/ > now. See > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl... > to see how this is implemented for the main browser. Done.
LGTM for the SSLHostStateDelegate files (I didn't look at any of the Java files). https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... File android_webview/browser/aw_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.cc:39: allowed_[cert.fingerprint()] = error; On 2014/12/12 02:42:37, hush wrote: > On 2014/12/12 01:48:41, jww wrote: > > This should store the entire chain, not just the end cert fingerprint. You can > > use net::X509Certificate::CalculateChainFingerprint256 as shown in > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl..., > > to do this. Actually, you might consider making "GetKey()" from that file into > a > > general utility function somewhere to share the code. > > I just used "CalculateChainFingerprint256" to get the chain fingerprint, then > use it as the hashmap key without mixing the error code with it, because in our > case here, the error code is the "value" in the |allowed_| hashmap. > As a result, I don't really have to extract the getKey() from chrome/ to > somewhere else. > Does this sound good? Yup, sounds great (And I misspoke in my original comment. You clearly don't need to store the entire chain; you just need the *hash* of the entire chain, which you did). https://codereview.chromium.org/794023002/diff/80001/android_webview/browser/... android_webview/browser/aw_ssl_host_state_delegate.cc:43: AwSSLHostStateDelegate::AwSSLHostStateDelegate() { On 2014/12/12 02:42:37, hush wrote: > On 2014/12/12 01:48:41, jww wrote: > > Nit: This constructor and destructor be inline in the header > > Sorry, I tried to do this. But I got a compiler error: "[chromium-style] Complex > constructor has an inlined body." > Looking at tools/clang/plugins/FindBadConstructsConsumer.cpp, I guess people > just don't want us to inline constructors. Okay, my bad.
https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_browser_context.cc:353: nit: remove empty line https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... File android_webview/browser/aw_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:12: namespace internal { nit: add a line between namespaces https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:38: nit: remove the empty line https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:47: nit: remove the empty line https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:65: return !!ran_insecure_content_hosts_.count(BrokenHostEntry(host, pid)); We are changing behavior here compared to returning a null delegate, Further looking at the code of how it is used in content and net layer, I don't think it is meaningful for webview. jww can correct me but I think you can return plain false here and drop any baggage that comes with it. https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... File android_webview/browser/aw_ssl_host_state_delegate.h (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.h:18: namespace internal { nit: add a line between namespaces https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.h:20: // permitted for a particular purpose. particular purpose is pretty vague. say: // This class maintains the policy for storing actions on certificate errors. https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:519: public void testClearSslPreferences() throws Throwable { There is a great deal of overlap between tests, try not to duplicate the code? https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:552: public void testDenySslErrorNotRememebered() throws Throwable { nit: Remembered https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:575: // Now clear the ssl preferences then load the same url again. Expect to see himm, don't think this part adds any value here. https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java:28: private final OnReceivedSslErrorHelper mOnReceivedSslErrorHelper; CallbackHelper https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java:72: public static class OnReceivedSslErrorHelper extends CallbackHelper {} drop this and simply use callbackhelper.
On 2014/12/12 04:23:11, sgurun wrote: > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > File android_webview/browser/aw_browser_context.cc (right): > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > android_webview/browser/aw_browser_context.cc:353: > nit: remove empty line > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > File android_webview/browser/aw_ssl_host_state_delegate.cc (right): > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > android_webview/browser/aw_ssl_host_state_delegate.cc:12: namespace internal { > nit: add a line between namespaces > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > android_webview/browser/aw_ssl_host_state_delegate.cc:38: > nit: remove the empty line > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > android_webview/browser/aw_ssl_host_state_delegate.cc:47: > nit: remove the empty line > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > android_webview/browser/aw_ssl_host_state_delegate.cc:65: return > !!ran_insecure_content_hosts_.count(BrokenHostEntry(host, pid)); > We are changing behavior here compared to returning a null delegate, Further > looking at the code of how it is used in content and net layer, I don't think it > is meaningful for webview. jww can correct me but I think you can return plain > false here and drop any baggage that comes with it. > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > File android_webview/browser/aw_ssl_host_state_delegate.h (right): > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > android_webview/browser/aw_ssl_host_state_delegate.h:18: namespace internal { > nit: add a line between namespaces > > https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... > android_webview/browser/aw_ssl_host_state_delegate.h:20: // permitted for a > particular purpose. > particular purpose is pretty vague. > say: // This class maintains the policy for storing actions on certificate > errors. > > https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... > File > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java > (right): > > https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:519: > public void testClearSslPreferences() throws Throwable { > There is a great deal of overlap between tests, try not to duplicate the code? > > https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:552: > public void testDenySslErrorNotRememebered() throws Throwable { > nit: Remembered > > https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:575: > // Now clear the ssl preferences then load the same url again. Expect to see > himm, don't think this part adds any value here. > > https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... > File > android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java > (right): > > https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java:28: > private final OnReceivedSslErrorHelper mOnReceivedSslErrorHelper; > CallbackHelper > > https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... > android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java:72: > public static class OnReceivedSslErrorHelper extends CallbackHelper {} > drop this and simply use callbackhelper. thanks Jww for your quick review!
https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_browser_context.cc:353: On 2014/12/12 04:23:09, sgurun wrote: > nit: remove empty line Done. https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... File android_webview/browser/aw_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:12: namespace internal { On 2014/12/12 04:23:09, sgurun wrote: > nit: add a line between namespaces Done. https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:35: !(~(allowed_iter->second & error) ^ ~error)) { I copied this if statement from m37 code, and I think it is equivalent to (allowed_iter->second & error) == error https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:38: On 2014/12/12 04:23:09, sgurun wrote: > nit: remove the empty line Done. https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:47: On 2014/12/12 04:23:09, sgurun wrote: > nit: remove the empty line Done. https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:65: return !!ran_insecure_content_hosts_.count(BrokenHostEntry(host, pid)); On 2014/12/12 04:23:09, sgurun wrote: > We are changing behavior here compared to returning a null delegate, Further > looking at the code of how it is used in content and net layer, I don't think it > is meaningful for webview. jww can correct me but I think you can return plain > false here and drop any baggage that comes with it. Yes. I did some investigations too. This method is only used here: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ss... which updates the entry with the flags: SECURITY_STYLE_AUTHENTICATION_BROKEN and SSLStatus::RAN_INSECURE_CONTENT. These 2 flags are only read by chrome/ layer, so probably not related to WebView. I will just go ahead and remove this thing. https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... File android_webview/browser/aw_ssl_host_state_delegate.h (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.h:18: namespace internal { On 2014/12/12 04:23:10, sgurun wrote: > nit: add a line between namespaces Done. https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.h:20: // permitted for a particular purpose. On 2014/12/12 04:23:10, sgurun wrote: > particular purpose is pretty vague. > say: // This class maintains the policy for storing actions on certificate > errors. Done. https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:519: public void testClearSslPreferences() throws Throwable { On 2014/12/12 04:23:10, sgurun wrote: > There is a great deal of overlap between tests, try not to duplicate the code? Ok. I just merged the 2 tests. https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:552: public void testDenySslErrorNotRememebered() throws Throwable { On 2014/12/12 04:23:10, sgurun wrote: > nit: Remembered Done. https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:575: // Now clear the ssl preferences then load the same url again. Expect to see On 2014/12/12 04:23:10, sgurun wrote: > himm, don't think this part adds any value here. removed. https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java:28: private final OnReceivedSslErrorHelper mOnReceivedSslErrorHelper; On 2014/12/12 04:23:10, sgurun wrote: > CallbackHelper Done. https://codereview.chromium.org/794023002/diff/140001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/TestAwContentsClient.java:72: public static class OnReceivedSslErrorHelper extends CallbackHelper {} On 2014/12/12 04:23:10, sgurun wrote: > drop this and simply use callbackhelper. Done.
lgtm
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/794023002/160001
https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... File android_webview/browser/aw_ssl_host_state_delegate.cc (right): https://codereview.chromium.org/794023002/diff/140001/android_webview/browser... android_webview/browser/aw_ssl_host_state_delegate.cc:65: return !!ran_insecure_content_hosts_.count(BrokenHostEntry(host, pid)); On 2014/12/12 19:43:39, hush wrote: > On 2014/12/12 04:23:09, sgurun wrote: > > We are changing behavior here compared to returning a null delegate, Further > > looking at the code of how it is used in content and net layer, I don't think > it > > is meaningful for webview. jww can correct me but I think you can return plain > > false here and drop any baggage that comes with it. > > Yes. I did some investigations too. This method is only used here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ss... > which updates the entry with the flags: SECURITY_STYLE_AUTHENTICATION_BROKEN and > SSLStatus::RAN_INSECURE_CONTENT. > These 2 flags are only read by chrome/ layer, so probably not related to > WebView. > > I will just go ahead and remove this thing. I'm surprised there's no way to extract this information from a WebView. That is, it seems like an embedder of a WebView might want to know if the HTTPS content inside the WebView ran insecure content. That having been said, it does seem that that's not possible with WebView, so if you never expect it to be present, then indeed this is not useful information to store.
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/8e5bcae56618fb6f5d5ad677b0206b943221193a Cr-Commit-Position: refs/heads/master@{#308160} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
