Issue metadata
Sign in to add a comment
|
Superfish check computes cert fingerprints twice |
||||||||||||||||||||||||
Issue descriptionIn ssl_error_handler.cc, IsSuperfish() computes Sha256 of all certs in the chain. It's called twice, once from RecordSuperfishUMA() and then from ShowSSLInterstitial(). That seems wasteful, can we just do it once? More specifically, can we call only RecordSuperfishUMA() in SSLErrorHandlerDelegateImpl::ShowSSLInterstitial()? That'll change the histogram a bit, but perhaps that's acceptable if we ignore bad clock + superfish or captive portal + superfish scenarios.
,
Oct 6 2017
Actually, the CL that I thought was going to fix this didn't. I'll hold on to this as a starter bug for Chris.
,
Oct 9 2017
Sounds good. Chris: Please CC me on the CL! :-)
,
Oct 19 2017
,
Oct 19 2017
I've uploaded a CL to address this in https://chromium-review.googlesource.com/c/chromium/src/+/728923. A slight difference from the discussion here: Moving the UMA logging up the call hierarchy to HandleSSLError() places it next to the first call to IsSuperfish(), and avoids the complication of missing logging if combined with bad clock/captive portal.
,
Oct 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c2cecaab73ba5e7fad8659a4ec195cdac6fd4efb commit c2cecaab73ba5e7fad8659a4ec195cdac6fd4efb Author: Christopher Thompson <cthomp@chromium.org> Date: Fri Oct 20 00:20:49 2017 Remove duplicate IsSuperfish() call Previously we were calling IsSuperfish() (which computes the Sha256 of all certs in the chain) twice. This moves the call and the UMA logging up into SSLErrorHandler::HandleSSLError() to combine both flows (the result is needed for UMA logging and for the options_mask passed to the delegate). Bug: 770843 Change-Id: Idda41d5ad388ed49c63c7997ef897d27636dcd5f Reviewed-on: https://chromium-review.googlesource.com/728923 Reviewed-by: Mustafa Emre Acer <meacer@chromium.org> Commit-Queue: Christopher Thompson <cthomp@chromium.org> Cr-Commit-Position: refs/heads/master@{#510260} [modify] https://crrev.com/c2cecaab73ba5e7fad8659a4ec195cdac6fd4efb/chrome/browser/ssl/ssl_error_handler.cc
,
Oct 23 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by est...@chromium.org
, Oct 3 2017Owner: lgar...@chromium.org
Status: Assigned (was: Untriaged)