Closed Bug 1320566 Opened 8 years ago Closed 7 years ago

Certificate Transparency - implement CT Policy

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: sergei.cv, Assigned: sergei.cv)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

Bug 1320566 - Certificate Transparency - implement CT Policy.
58 bytes, text/x-review-board-request
keeler
: review+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.98 Safari/537.36
Blocks: 1281469
Priority: -- → P2
Whiteboard: [psm-backlog]
The implemented policy is based on the Chromium CT Policy of May 2016 (http://dev.chromium.org/Home/chromium-security/root-ca-policy/CTPolicyMay2016edition.pdf) with the exception of the diversity requirement: SCTs are required to be from CT logs run by two distinct operators, rather than from one Google-operated CT log and one CT log not operated by Google.

Telemetry reports will be implemented in a separate patch (see bug 1320567). In this patch, we check the policy on secure connections and report the results in the UI (Page Info -> Security).
For reference, here is the implementation of the CT Policy in Chromium that our code is based upon:
https://cs.chromium.org/chromium/src/net/cert/ct_policy_enforcer.cc
The informational message displayed on the Security tab of the Page Info dialog is one of the following:

* This website complies with the Certificate Transparency policy.

* This website does not comply with the Certificate Transparency policy because it did not supply enough audit records.

* This website does not comply with the Certificate Transparency policy because the number of its audit record issuers is not sufficient.
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

https://reviewboard.mozilla.org/r/97194/#review98432

Sorry this took a while to get to. This looks really good, but I'm wary of landing a policy before we have a more definite idea of what Mozilla's CT policy will be (see the first comment). We're actively working on that, though, and we should have more details soon. In the meantime, though, there are a couple of things I think we should change in how this works.

::: security/certverifier/CTPolicyEnforcer.h:33
(Diff revision 1)
> +  NotDiverseScts,
> +};
> +
> +// Checks whether a TLS connection complies with the current CT policy.
> +// The implemented policy is based on the Chromium CT Policy of May 2016
> +// (http://dev.chromium.org/Home/chromium-security/root-ca-policy/

We actually had a meeting about this last Friday, and while the Mozilla policy is still very similar to the Chromium policy, it might be a good idea to hold off on landing any specific policy code until we have a more concrete idea of what the policy will be (in particular, we might be going with requiring 3 distinct CT operators).

::: security/certverifier/CTPolicyEnforcer.cpp:81
(Diff revision 1)
> +// Whether a valid embedded SCT is present in the list.
> +static bool
> +HasValidEmbeddedSct(const VerifiedSCTList& verifiedScts)
> +{
> +  for (const VerifiedSCT& verifiedSct : verifiedScts) {
> +    if (verifiedSct.status == VerifiedSCT::Status::Valid && 

nit: watch out for trailing whitespace

::: security/certverifier/CertVerifier.cpp:319
(Diff revision 1)
>               result.decodingErrors));
>    }
>  
> +  PRTime notBefore;
> +  PRTime notAfter;
> +  if (CERT_GetCertTimes(endEntity, &notBefore, &notAfter) != SECSuccess) {

We don't want to use CERT_GetCertTimes here - we should extract the notBefore/notAfter values using mozilla::pkix (we should be able to get the values in NSSCertDBTrustDomain and pass them to this function - see CheckValidityIsAcceptable).

::: security/certverifier/MultiLogCTVerifier.cpp:69
(Diff revision 1)
>      if (rv != Success) {
>        return rv;
>      }
>    }
>  
> +  if (sctListFromOCSPResponse.GetLength() == 0 &&

This is an optimization to avoid calling GetX509LogEntry, right? If so, I don't think we need to add this - the code is more clear without it.

::: security/manager/ssl/nsSSLStatus.cpp:338
(Diff revision 1)
> -    mCertificateTransparencyStatus =
> +  mCertificateTransparencyStatus =
> -      nsISSLStatus::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE;
> +    nsISSLStatus::CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE;
> -    return;
> -  }
>  
> -  if (!info.processedSCTs) {
> +  if (!info.enabled) {

If I'm understanding correctly, this patch puts the page info dialog into a slightly strange situation where if a site doesn't send any CT info at all, it will show nothing, but if it does send CT info, it will mention the CT policy and whether or not the information was sufficient. Maybe we should keep the "this site did not provide CT info" indicator for now?
Attachment #8816440 - Flags: review?(dkeeler)
OK, I understand the situation. Some questions:
1. Do you have an estimation on when the browser-side CT Policy (which affects the code in this patch) will be finalized?
2. Do you think the final browser-side CT Policy might turn out significantly different from what we have now? Or the changes would be relatively minor (such as the number of logs required and other parameters)?

I am asking since following this patch, we have a telemetry patch which will take time to complete and pass all the necessary reviews. The telemetry patch will report the result of the policy evaluation, so it is more or less independent from the policy itself. If we do not anticipate major changes in the policy, it might make sense to ship it as it is now, proceed with the telemetry patch, and fix the policy later in a separate patch.

After all, releasing a work-in-progress policy (assuming it won't change much) will only affect a single line of text in a seldom viewed place, but will unblock the other work we have.

What do you think?
Flags: needinfo?(dkeeler)
Attachment #8816440 - Flags: review?(eranm)
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

https://reviewboard.mozilla.org/r/97194/#review98896

::: browser/base/content/pageinfo/security.js:101
(Diff revision 1)
>        // Select status text to display for Certificate Transparency.
>        switch (status.certificateTransparencyStatus) {
>          case nsISSLStatus.CERTIFICATE_TRANSPARENCY_NOT_APPLICABLE:
>            // CT compliance checks were not performed,
>            // do not display any status text.
>            retval.certificateTransparency = null;

If we adopt David's suggestion, a string would go in here to say that CT info is not present at all, right?
Attachment #8816440 - Flags: review?(eranm)
Since we are now reporting on policy compliance of secure connections (not SCT presence like before), I think it should go like this:

* No status text will be shown if CT is not applicable to the connection (it is not secure, CT is disabled in the settings).

* For secure connections, policy compliance will be reported. Specifically, if no SCTs are present, then the connection is not compliant and the corresponding message will be shown (see Comment 4).
(In reply to Eran Messeri from comment #7)
> FYI the current Mozilla CT policy is here:
> https://docs.google.com/document/d/1rnqYYwscAx8WhS-
> MCdTiNzYQus9e37HuVyafQvEeNro/edit

Oh - I didn't realize that was public yet. In any case, now we have something more concrete to discuss.

(In reply to Sergei Chernov from comment #6)
> OK, I understand the situation. Some questions:
> 1. Do you have an estimation on when the browser-side CT Policy (which
> affects the code in this patch) will be finalized?

I think soon, but I don't want to commit to a timetable on someone else's behalf (Gerv, now cc'd, is drafting the policy).

> 2. Do you think the final browser-side CT Policy might turn out
> significantly different from what we have now? Or the changes would be
> relatively minor (such as the number of logs required and other parameters)?

Almost certainly minor. In particular, one difference I can see right now is the document currently doesn't specify a log operator diversity requirement.

> I am asking since following this patch, we have a telemetry patch which will
> take time to complete and pass all the necessary reviews. The telemetry
> patch will report the result of the policy evaluation, so it is more or less
> independent from the policy itself. If we do not anticipate major changes in
> the policy, it might make sense to ship it as it is now, proceed with the
> telemetry patch, and fix the policy later in a separate patch.
> 
> After all, releasing a work-in-progress policy (assuming it won't change
> much) will only affect a single line of text in a seldom viewed place, but
> will unblock the other work we have.
> 
> What do you think?

Since the draft policy is public, let's keep moving forward (seems like the next step is to update this patch to match the policy as-is).

(In reply to Sergei Chernov from comment #9)
> Since we are now reporting on policy compliance of secure connections (not
> SCT presence like before), I think it should go like this:
> 
> * No status text will be shown if CT is not applicable to the connection (it
> is not secure, CT is disabled in the settings).
> 
> * For secure connections, policy compliance will be reported. Specifically,
> if no SCTs are present, then the connection is not compliant and the
> corresponding message will be shown (see Comment 4).

My worry here is that we do have some users who would be concerned if Firefox said sites they visit aren't complying with some policy just because they haven't implemented CT yet, so while internally we can treat not sending CT information as not compliant with the policy, let's keep the string and case for where the site didn't send anything at all.
Flags: needinfo?(dkeeler)
The policy will receive a significant update next week, based on recent discussions within Mozilla, which will certainly affect the direction here. You may want to wait until that happens before making further code changes.

Gerv
https://docs.google.com/document/d/1rnqYYwscAx8WhS-MCdTiNzYQus9e37HuVyafQvEeNro/edit# has now been updated with the new proposed direction for our CT policy.

As you can see, if we proceed down this route, we would need to attach "ownership" metadata to both root certificates and CT logs, work out the relevant "ownership" of the cert (based on its root) and its SCTs (based on the logs they come from) and then do a calculation to see if our log independence requirement has been met.

We would also, if we don't already, need the ability to mark a log as having a termination date - i.e. it was once qualified, and SCTs it issues before a certain date are still valid, but newer ones are not.

We might also want to write the code to check the "CT Stamped" requirement as well, although we would not enable this until October at the earliest. (Having said that, this is a new idea, so it may not survive review if it turns out there's a very good reason not to do it.)

Please let me know if you have questions.

Gerv
Submitted the updated CT Policy implementation. The method for filtering out CA-dependent log operators is currently a placeholder.
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

https://reviewboard.mozilla.org/r/97194/#review103974

I'm not quite done with this (still have to look at the tests), but I wanted to give you some feedback since it's been taking me a while.
Also, there's the question of what to do now that it's not as clear that the Mozilla CT policy is as near finalization as we thought it was. Perhaps what we can do is implement the policy as proposed but not expose it in the UI anywhere (and essentially keep the old UI). That way, we can move forward without giving users any potentially confusing or misleading messages. What do you think?

::: browser/base/content/pageinfo/security.js:107
(Diff revision 2)
> -        case nsISSLStatus.CERTIFICATE_TRANSPARENCY_UNKNOWN_LOG:
> -          retval.certificateTransparency = "UnknownLog";
> -          break;
> -        case nsISSLStatus.CERTIFICATE_TRANSPARENCY_INVALID:
> -          retval.certificateTransparency = "Invalid";
>            break;

Do we want to include a default case here for old cached values?

::: security/certverifier/CTPolicyEnforcer.cpp:72
(Diff revision 2)
> +CountIndependentLogOperatorsForSelectedScts(const VerifiedSCTList& verifiedScts,
> +  const CTLogOperatorList& dependentOperators,
> +  size_t& count,
> +  SelectFunc selected)
> +{
> +  Vector<CTLogOperatorId, 8> operatorIds;

nit: I think we can use the CTLogOperatorList typedef here

::: security/certverifier/CTPolicyEnforcer.cpp:115
(Diff revision 2)
> +                         size_t& count,
> +                         SelectFunc selected)
> +{
> +  // Keep pointers to log ids (of type Buffer) from |verifiedScts| to save on
> +  // memory allocations.
> +  Vector<const Buffer*, 8> logIds;

Let's use references instead of pointers if we can.

::: security/certverifier/CTPolicyEnforcer.cpp:124
(Diff revision 2)
> +    }
> +
> +    const Buffer* sctLogId = &verifiedSct.sct.logId;
> +    // Check if |sctLogId| points to data already in |logIds|...
> +    bool alreadyAdded = false;
> +    for (const Buffer* logId : logIds) {

Seems like we could factor out this add-if-not-added functionality.

::: security/certverifier/CTPolicyEnforcer.cpp:349
(Diff revision 2)
> +    MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
> +            ("CT Policy: embedded case satisfied\n"));
> +  }
> +
> +  if (nonEmbeddedCaseOK || embeddedCaseOK) {
> +    compliance = diversityOK ?

nit: we often format long ternary ifs like so:

    compliance = diversityOK ? CTPolicyCompliance::Compliant
                             : CTPolicyCompliance::NotDiverseScts;

::: security/certverifier/CertVerifier.cpp:345
(Diff revision 2)
>               result.decodingErrors));
>    }
>  
> +  PRTime notBefore;
> +  PRTime notAfter;
> +  if (CERT_GetCertTimes(endEntity, &notBefore, &notAfter) != SECSuccess) {

We still don't want to use CERT_GetCerTimes here (there was a previous review comment on this, but I don't know how to link to that, so I'll just reproduce it here: "... we should extract the notBefore/notAfter values using mozilla::pkix (we should be able to get the values in NSSCertDBTrustDomain and pass them to this function - see CheckValidityIsAcceptable).")
Attachment #8816440 - Flags: review?(dkeeler)
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

https://reviewboard.mozilla.org/r/97194/#review103974

> Do we want to include a default case here for old cached values?

Note that retval.certificateTransparency has a default value set in line 66, so if there is case we don't handle, the default value will stay (meaning no message will be displayed).

> Seems like we could factor out this add-if-not-added functionality.

You are right of course, but the question is where to put this function. Note it needs to be templated, so it is not related to CT as itself. It can implement "add-if-not-added", but basically "find" would be enough. "find" seems to belong to Vector.h, but I prefer not to touch Vector.h, so the logic is currently duplicated (although it's not too much).
What do you think?

> We still don't want to use CERT_GetCerTimes here (there was a previous review comment on this, but I don't know how to link to that, so I'll just reproduce it here: "... we should extract the notBefore/notAfter values using mozilla::pkix (we should be able to get the values in NSSCertDBTrustDomain and pass them to this function - see CheckValidityIsAcceptable).")

Yes, but note that in the new code CERT_GetCertTimes function is used in CertVerifier.cpp (which already uses NSS a lot).
The issue with CheckValidityIsAcceptable is that notBefore/notAfter are pkix::Time (which is opaque) and we need to find the number of calendar months in that period. CERT_GetCerTimes returns PRTime which can be converted to year/month/day values.
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

https://reviewboard.mozilla.org/r/97194/#review103974

> Note that retval.certificateTransparency has a default value set in line 66, so if there is case we don't handle, the default value will stay (meaning no message will be displayed).

Ah, right - ok.

> You are right of course, but the question is where to put this function. Note it needs to be templated, so it is not related to CT as itself. It can implement "add-if-not-added", but basically "find" would be enough. "find" seems to belong to Vector.h, but I prefer not to touch Vector.h, so the logic is currently duplicated (although it's not too much).
> What do you think?

Yeah, it's not a big deal. I was thinking of just having a helper function here, but for only 2 instances it's not quite worth it.

> Yes, but note that in the new code CERT_GetCertTimes function is used in CertVerifier.cpp (which already uses NSS a lot).
> The issue with CheckValidityIsAcceptable is that notBefore/notAfter are pkix::Time (which is opaque) and we need to find the number of calendar months in that period. CERT_GetCerTimes returns PRTime which can be converted to year/month/day values.

Ok - since there are preexisting instances of CERT_GetCertTimes, I guess we can go ahead with this and address them all later.
Assignee: nobody → sergei.cv
Priority: P2 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

https://reviewboard.mozilla.org/r/97194/#review103974

What do think of the following -

I've updated the CTPolicyEnforcer (take a look), so now it is effectively using the policy before the recent changes (i.e. like Chrome's but requiring 2 different operators). If the site complies, you will see "This website complies with the Certificate Transparency policy"; nothing will be shown otherwise.

Another option could be "This site has presented valid Certificate Transparency audit records" in case we have at least 1 valid SCT from a qualified log; and nothing otherwise.

I just think we better not bring back the code needed for all the cases of the old messages...

> Let's use references instead of pointers if we can.

We would probably need to use std::reference_wrapper for Vector to hold references to Buffer. I didn't see it used anywhere else, maybe best to just keep using pointers?
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

https://reviewboard.mozilla.org/r/97194/#review106096

Great! Looks good to me - just the two comments.

::: browser/base/content/pageinfo/security.js:95
(Diff revision 3)
>          case nsISSLStatus.TLS_VERSION_1_3:
>            retval.version = "TLS 1.3"
>            break;
>        }
>  
> -      // Select status text to display for Certificate Transparency.
> +      // Select the status text to display for Certificate Transparency.

Technically you'll need a browser peer to r+ this, but that shouldn't take long.

::: security/certverifier/CertVerifier.cpp:164
(Diff revision 3)
>    return rv;
>  }
>  
> +// Returns the certificate validity period in calendar months (rounded down).
> +static Result
> +GetCertLifetimeInFullMonths(PRTime certNotBefore,

I think it would be best to have a couple of unit tests for this function (I think if you don't declare this static and have an extern definition in a gtest it should work without having to actually declare it in a header file, since we don't want to expose it).
Attachment #8816440 - Flags: review?(dkeeler) → review+
(In reply to Sergei Chernov from comment #19)
> Comment on attachment 8816440 [details]
> Bug 1320566 - Certificate Transparency - implement CT Policy.
> 
> https://reviewboard.mozilla.org/r/97194/#review103974
> 
> What do think of the following -
> 
> I've updated the CTPolicyEnforcer (take a look), so now it is effectively
> using the policy before the recent changes (i.e. like Chrome's but requiring
> 2 different operators). If the site complies, you will see "This website
> complies with the Certificate Transparency policy"; nothing will be shown
> otherwise.

This sounds good.

> Another option could be "This site has presented valid Certificate
> Transparency audit records" in case we have at least 1 valid SCT from a
> qualified log; and nothing otherwise.
> 
> I just think we better not bring back the code needed for all the cases of
> the old messages...
> 
> > Let's use references instead of pointers if we can.
> 
> We would probably need to use std::reference_wrapper for Vector to hold
> references to Buffer. I didn't see it used anywhere else, maybe best to just
> keep using pointers?

Yeah, this should be fine.
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

Dolske, any chance you could have a quick look at the changes in browser/base/content/pageinfo/security.js? I think you're the browser peer most familiar with this code. Thanks!
Attachment #8816440 - Flags: review?(dolske)
Comment on attachment 8816440 [details]
Bug 1320566 - Certificate Transparency - implement CT Policy.

https://reviewboard.mozilla.org/r/97194/#review107636

It's a little curious that this will now either show the "compliant" message or nothing, but also sounds like this is still an evolving space. And so I can understand the hesitancy to be indicating a problem when it's… not that straightforward. But I trust you're all on top of it, and they'll return when suitable.
Attachment #8816440 - Flags: review?(dolske) → review+
Rebased and fixed MSVC issues with GetCertLifetimeInFullMonths (CertVerifier.cpp).
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9faf10e794e6
Certificate Transparency - implement CT Policy. r=Dolske,keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9faf10e794e6
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: