Certificate Transparency - basic UI indicator

RESOLVED FIXED in Firefox 52

Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Sergei Chernov, Assigned: Sergei Chernov)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [psm-assigned], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.116 Safari/537.36
(Assignee)

Updated

a year ago
Blocks: 1281469
Priority: -- → P1
(Assignee)

Comment 1

a year ago
Created attachment 8795781 [details]
screenshot.png

The patch adds a short description of Certificate Transparency verification status to the Page Info dialog (under "Technical Details", see the attached screenshot). The text presented is one of the following:

1. "This website does not supply Certificate Transparency audit records." - no SCTs are available.
2. "This website supplies publicly auditable Certificate Transparency records." - at least one verified SCT is present.
3. "This website claims to have Certificate Transparency audit records, but the records cannot be verified." - SCT from unknown log is present and no verified SCTs.
4. "This website supplies Certificate Transparency audit records, but the records failed verification." - all the available SCTs have failed verification.

No text status is shown in case the connection is not secure or CT is completely disabled via the settings.
Comment hidden (mozreview-request)
Assignee: nobody → sergei.cv
Whiteboard: [psm-assigned]

Comment 3

a year ago
mozreview-review
Comment on attachment 8795783 [details]
Bug 1305289 - Certificate Transparency - basic UI indicator;

https://reviewboard.mozilla.org/r/81724/#review80808

Great - the security/ parts look good to me with comments addressed. I think Dolske might be a good candidate to review the browser/ parts.

::: browser/base/content/pageinfo/security.js:113
(Diff revision 1)
> +          retval.certificateTransparency = "UnknownLog";
> +          break;
> +        case nsISSLStatus.CERTIFICATE_TRANSPARENCY_INVALID:
> +          retval.certificateTransparency = "Invalid";
> +          break;
> +        default:

I think we should have an explicit case for nsISSLStatus.CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE so it's clear that if CT is disabled, nothing will show up.

::: security/manager/ssl/SSLServerCertVerification.cpp:1299
(Diff revision 1)
>    // but it failed to parse (e.g. due to unsupported CT protocol version).
>    Telemetry::Accumulate(Telemetry::SSL_SCTS_PER_CONNECTION, sctsCount);
>  }
>  
> +void
> +SetCertificateTransparencyStatus(RefPtr<nsSSLStatus>& status,

Let's actually just make this a method on nsSSLStatus.

::: security/manager/ssl/SSLServerCertVerification.cpp:1478
(Diff revision 1)
>        status->SetServerCert(nsc, evStatus);
>        MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
>               ("AuthCertificate setting NEW cert %p\n", nsc.get()));
>      }
> +
> +    SetCertificateTransparencyStatus(status, certificateTransparencyInfo);

One concern I have is I'm not sure this will do the right thing in the case of session resumption (my understanding is we can have a successful handshake that doesn't involve calling AuthCertificate) (although tbh, I'm not sure other things like EV work for session resumption, so we can probably address this in a follow-up if it's an issue).
Attachment #8795783 - Flags: review?(dkeeler) → review+
Attachment #8795783 - Flags: review?(dolske)
How does get this UI in Chrome? I was curious if our language is consistent with theirs, but I'm not getting any of the UI shown in https://www.certificate-transparency.org/certificate-transparency-in-chrome with my current Canary. (The security info seems to be in their devtools now, but I don't see anything there about CT.)

Comment 5

a year ago
mozreview-review
Comment on attachment 8795783 [details]
Bug 1305289 - Certificate Transparency - basic UI indicator;

https://reviewboard.mozilla.org/r/81724/#review81526

::: browser/base/content/pageinfo/security.js:113
(Diff revision 1)
> +        default:
> +          // Do not display any status text.
> +          retval.certificateTransparency = null;

It would be kinda nice if we could eliminate the "undefined" case... Ideally all possible values are covered, with a plausible fallback ("None"?) if there's a bug that causes an unknown value to come through (since crashing probably isn't what we want. :).

But, eh, not a big deal.

::: security/manager/locales/en-US/chrome/pippki/pippki.properties:111
(Diff revision 1)
>  pageInfo_Privacy_Encrypted1=The page you are viewing was encrypted before being transmitted over the Internet.
>  pageInfo_Privacy_Encrypted2=Encryption makes it difficult for unauthorized people to view information traveling between computers. It is therefore unlikely that anyone read this page as it traveled across the network.
>  pageInfo_MixedContent=Connection Partially Encrypted
>  pageInfo_MixedContent2=Parts of the page you are viewing were not encrypted before being transmitted over the Internet.
>  pageInfo_WeakCipher=Your connection to this website uses weak encryption and is not private. Other people can view your information or modify the website’s behavior.
> +pageInfo_CertificateTransparency_None=This website does not supply Certificate Transparency audit records.

Part of why I was looking at Chrome's wording was that I thought CT was just a thing the CA did (and not the website) but from some quick reading it looks like it's... not that simple.

"This website" would still be ok, since a hyperprecise description of who is supplying the CT record isn't really the important detail here.

I suppose you could phrase it without a particular party, like "No CT audit record was supplied" / "A CT audit record was supplied but failed verification" / etc. But again it doesn't really matter much.
Attachment #8795783 - Flags: review?(dolske) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
mozreview-review-reply
Comment on attachment 8795783 [details]
Bug 1305289 - Certificate Transparency - basic UI indicator;

https://reviewboard.mozilla.org/r/81724/#review81526

> It would be kinda nice if we could eliminate the "undefined" case... Ideally all possible values are covered, with a plausible fallback ("None"?) if there's a bug that causes an unknown value to come through (since crashing probably isn't what we want. :).
> 
> But, eh, not a big deal.

Fixed. All possible values are now covered, an unknown enum value will result in retval.certificateTransparency staying "undefined", which is handled by securityOnLoad() in the same way as "null" (i.e. nothing is displayed).

> Part of why I was looking at Chrome's wording was that I thought CT was just a thing the CA did (and not the website) but from some quick reading it looks like it's... not that simple.
> 
> "This website" would still be ok, since a hyperprecise description of who is supplying the CT record isn't really the important detail here.
> 
> I suppose you could phrase it without a particular party, like "No CT audit record was supplied" / "A CT audit record was supplied but failed verification" / etc. But again it doesn't really matter much.

Currently, the strings are similar to what they've used to be in Chrome once (see https://www.certificate-transparency.org/certificate-transparency-in-chrome). Eventually, instead of indicating whether the site has valid "CT audit records", we will show the user if the site is "CT compliant" or not. But for that, we need to define the CT policy for Firefox, which will take some time.
(Assignee)

Comment 8

a year ago
mozreview-review-reply
Comment on attachment 8795783 [details]
Bug 1305289 - Certificate Transparency - basic UI indicator;

https://reviewboard.mozilla.org/r/81724/#review80808

> Let's actually just make this a method on nsSSLStatus.

You mean to move the whole CertificateTransparencyInfo method from SSLServerCertVerification.cpp to nsSSLStatus.cpp, or just add a setter function for mCertificateTransparencyStatus?

Note that to move the whole method, we'll need to include CertVerifier.h in nsSSLStatus.h (or in nsSSLStatus.cpp and forward-declare in CertVerifier.h) since CertificateTransparencyInfo class is defined there. Or we can extract CertificateTransparencyInfo to a separate header file.
(In reply to Sergei Chernov from comment #8)
> > Let's actually just make this a method on nsSSLStatus.
> 
> You mean to move the whole CertificateTransparencyInfo method from
> SSLServerCertVerification.cpp to nsSSLStatus.cpp, or just add a setter
> function for mCertificateTransparencyStatus?

Yes, I meant move the whole method.

> Note that to move the whole method, we'll need to include CertVerifier.h in
> nsSSLStatus.h (or in nsSSLStatus.cpp and forward-declare in CertVerifier.h)
> since CertificateTransparencyInfo class is defined there. Or we can extract
> CertificateTransparencyInfo to a separate header file.

I think that's fine - CertVerifier.h is included in other header files in that directory.

Thanks!
Comment hidden (mozreview-request)
(Assignee)

Comment 12

a year ago
Seems like we're ready for checkin. Thanks everyone!
Keywords: checkin-needed

Comment 13

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1f6f908368e5
Certificate Transparency - basic UI indicator; r=Dolske,keeler
Keywords: checkin-needed
backed out for gtest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4767161&repo=autoland
Flags: needinfo?(sergei.cv)

Comment 15

a year ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f93ca43637c1
Backed out changeset 1f6f908368e5 for gtest failures
Passing by, since I've seen the strings landing and being backed out. I confess that status 3 and 4 sound exactly the same to me: "the records cannot be verified" and "the records failed verification".

If I understand correctly, 3 means "I'm unable to verify these records", and 4 means "I verified these records and they're invalid"? If that's the case, maybe it's worth to reword the 4th case a little differently?
(Assignee)

Comment 18

a year ago
Good point, fixed the wording. The messages are now

"This website claims to have Certificate Transparency audit records, but the records were issued by an unknown party and cannot be verified."

vs

"This website supplies Certificate Transparency audit records, but the records failed verification."
No longer depends on: 1248628
Comment hidden (mozreview-request)
(Assignee)

Comment 20

a year ago
Added ad-hoc stream format versioning to nsSSLStatus following discussion with David. This allows reading older streams and keeps the failing psm_DeserializeCert gtest happy. For more details, see bug 1248628.
The relevant changes in this review request are in nsSSLStatus.cpp, the others are due to rebasing.
Flags: needinfo?(sergei.cv)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 22

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bc220b980c08
Certificate Transparency - basic UI indicator; r=Dolske,keeler
Keywords: checkin-needed

Comment 23

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc220b980c08
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.