New issue
Advanced search Search tips

Issue 705671 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Team-Security-UX



Sign in to add a comment

Explanation has wrong severity for SHA-1 in Security Panel

Project Member Reported by elawrence@chromium.org, Mar 27 2017

Issue description

Chrome Version: 59.0.3053.0

What steps will reproduce the problem?
(1) Visit https://sha1-2017.badssl.com/ 
(2) Open DevTools Security panel
(3) Observe: "Sha-1 certificate" explanation shown with Neutral treatment rather than Broken treatment.

This:

 if (security_info.sha1_in_chain) {
    security_style_explanations->neutral_explanations.push_back(
      
... should be

 if (security_info.sha1_in_chain) {
    security_style_explanations->insecure_explanations.push_back(

... if (security_info.cert_status & WEAK_SIGNATURE_ALGORITHM)

 
Labels: Hotlist-GoodFirstBug

Comment 2 by est...@chromium.org, Sep 13 2017

Cc: carlosil@google.com elawrence@chromium.org
Owner: est...@chromium.org
Carlos is working on this one
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/232281fc945e0318ac59421a6a5dd0097f2717f5

commit 232281fc945e0318ac59421a6a5dd0097f2717f5
Author: Carlos IL <carlosil@google.com>
Date: Wed Sep 13 16:25:57 2017

SHA1 Certificate explanation in security panel is now 'Broken'

Websites with Security errors due to SHA1 signed certificates in the chain now
show up with the broken (red triangle) indicator in the DevTools Security panel,
instead of the neutral (red circle) indicator.

R=estark@chromium.org

Bug:  705671 
Change-Id: I400936a3221ddeea65897a83f4ad04e6683e2d22
Reviewed-on: https://chromium-review.googlesource.com/664343
Reviewed-by: Emily Stark <estark@chromium.org>
Commit-Queue: Carlos Joan Rafael Ibarra Lopez <carlosil@google.com>
Cr-Commit-Position: refs/heads/master@{#501656}
[modify] https://crrev.com/232281fc945e0318ac59421a6a5dd0097f2717f5/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
[modify] https://crrev.com/232281fc945e0318ac59421a6a5dd0097f2717f5/components/security_state/content/content_utils.cc

FWIW, the proposal here was to also check that security_info.cert_status & WEAK_SIGNATURE_ALGORITHM to handle the corner case wherein the user's on a page signed with an Enterprise SHA1 certificate with the enablesha1forlocalanchors group policy set.

https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc.cc?l=622&rcl=232281fc945e0318ac59421a6a5dd0097f2717f5

(It's probably a puntable corner case, but noting for posterity).
Will start working on that corner case, just making sure I understand it correctly, in the case where the policy is set and the certificate is an Enterprise one, we should treat it as secure? or as neutral?
When the policy is set and the certificate chains to a local anchor, the /net/cert code skips setting CERT_STATUS_WEAK_SIGNATURE_ALGORITHM on the result.

The code in security_state uses the presence of SHA1 without CERT_STATUS_WEAK_SIGNATURE_ALGORITHM set to change the security state to NONE (aka neutral):
https://cs.chromium.org/chromium/src/components/security_state/core/security_state.cc?l=197&rcl=170f5d42dab16c6733cadad4a8124068e5341c76

In contrast, when the policy isn't set or the root isn't a local root, the /net/cert code sets CERT_STATUS_WEAK_SIGNATURE_ALGORITHM and the security_state code sets the state to DANGEROUS (aka insecure): https://cs.chromium.org/chromium/src/components/security_state/core/security_state.cc?l=149&rcl=170f5d42dab16c6733cadad4a8124068e5341c76
Cc: -carlosil@google.com est...@chromium.org
Owner: carlosil@chromium.org
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/56136aecba7e8d5c01bf40e44b2afb0655181396

commit 56136aecba7e8d5c01bf40e44b2afb0655181396
Author: Carlos IL <carlosil@chromium.org>
Date: Mon Sep 25 18:20:29 2017

Changed content_utils to handle case where SHA1 certificates are allowed

Changed ExplainCertificateSecurity so that when there is a SHA1
signature in the chain it now checks that cert_status is
CERT_STATUS_WEAK_SIGNATURE_ALGORITHM, if so it adds the explanation as
insecure, if not (in the case where enablesha1forlocalanchoses is set
and the certificiate is an Enterprise SHA1 one) it adds the explanation
as neutral. Also changed a test in
security_state_tab_helper_browser_tests so it matches the new behavior.

Bug:  705671 
Change-Id: I3cf775324a8df856fbc67168982e26a24c7eb6e1
Reviewed-on: https://chromium-review.googlesource.com/676198
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Eric Lawrence <elawrence@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504106}
[modify] https://crrev.com/56136aecba7e8d5c01bf40e44b2afb0655181396/chrome/browser/ssl/security_state_tab_helper_browser_tests.cc
[modify] https://crrev.com/56136aecba7e8d5c01bf40e44b2afb0655181396/components/security_state/content/content_utils.cc

Status: Fixed (was: Assigned)

Sign in to add a comment