Closed Bug 1317951 Opened 8 years ago Closed 8 years ago

Certificate Transparency - basic support for disqualified logs

Categories

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

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

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

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files, 1 obsolete file)

Bug 1317951, part 1 - Certificate Transparency - extracted verification related fields from SCT to a separate struct.
58 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1317951, part 2 - Certificate Transparency - basic support for disqualified logs.
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
1. Support listing disqualified CT logs in CTKnownLogs.h and make the necessary (minor) adjustments to the SCT verification process. At this stage, from the verification standpoint, a disqualified log is treated the same as an unknown log (e.g. its SCTs are not valid). The distinction is important for the upcoming implementation of the CT Policy.

2. Note that the known logs JSON file from https://www.certificate-transparency.org/known-logs does not include the disqualified logs. The are plans to include the disqualified logs in the near future. Until then, such logs have to be added manually to CTKnownLogs.h based on https://cs.chromium.org/chromium/src/net/cert/ct_known_logs_static-inc.h
Blocks: 1281469
Component: Untriaged → Security: PSM
Product: Firefox → Core
Best to start the code review from SignedCertificateTimestamps.h and CTVerifyResult.h. Most of the changes started there and propagated to the other files.

Also, a correction - the disqualified logs were included in CTKnownLogs.h, now they are marked as such (by hand).
Comment on attachment 8811249 [details]
Bug 1317951 - Certificate Transparency - basic support for disqualified logs.

https://reviewboard.mozilla.org/r/93434/#review93924

I think this is probably fine, but it would be much easier for me to review if this were broken into (at least) two patches: one for the renaming/moving of the data types/attributes/etc. and another for actually adding the changes to support disqualified logs. If that's not too much trouble, I think that would be best. Thanks!

::: security/certverifier/CTLogVerifier.cpp:124
(Diff revision 1)
>    DigitallySigned::SignatureAlgorithm mSignatureAlgorithm;
>  };
>  
>  
>  CTLogVerifier::CTLogVerifier()
> -  : mSignatureAlgorithm(DigitallySigned::SignatureAlgorithm::Anonymous)
> +  : mSignatureAlgorithm(DigitallySigned::SignatureAlgorithm::Anonymous),

nit: commas at the beginning of the lines (under the ':') like so:

```
CTLogVerifier::CTLogVerifier()
  : mSignatureAlgorithm(DigitallySigned::SignatureAlgorithm::Anonymous)
  , mOperatorId(-1)
  , mDisqualified(false)
  , mDisqualificationTime(UINT64_MAX)
```
Attachment #8811249 - Flags: review?(dkeeler)
The patch is now available in two parts -

Diff between revisions "orig" and "2" includes the changes caused by moving data attributes from SignedCertificateTimestamps.h to CTVerifyResult.h.

Diff between "2" and "3" includes the changes related to the disqualified logs.
Sorry, I wasn't clear - I meant two different changesets (so there would be two different things to review in mozreview, rather than interdiffs).
Flags: needinfo?(sergei.cv)
Assignee: nobody → sergei.cv
Priority: -- → P1
Whiteboard: [psm-assigned]
Is there a way of submitting it the way you suggest without dropping this bug and opening a new one? Although you've cancelled the original review request and I've recreated the hg commit from scratch (erasing the review id set in the original commit message), Review Board still has all the commits listed including the original cancelled one. How can I create several review tasks in a single bugzilla bug?
Flags: needinfo?(sergei.cv)
Flags: needinfo?(dkeeler)
I think you can close a review request (go to the review summary and there should be a "close" menu). I'm assuming this discards that state so you can open up a new one next time you push to review (using the same bug). I think the trick to having multiple review tasks is to have multiple commits that are new compared to the base revision. I thought you had already done something like this in another bug, but I can't actually find one. Basically, I would reset to your base revision, make the move/rename changes, and add a commit (with a message like "bug 1317951 - part 1/2: rename..."). Then, make the disqualified logs changes and add another commit ("bug 1317951 - part 2/2: add support for disqualified logs..."). reviewboard should notice there are two new revisions compared to the public base revision.
Flags: needinfo?(dkeeler)
Attachment #8811249 - Attachment is obsolete: true
Attachment #8811249 - Flags: review?(dkeeler)
Worked great, thanks for the instructions!
Comment on attachment 8813735 [details]
Bug 1317951, part 1 - Certificate Transparency - extracted verification related fields from SCT to a separate struct.

https://reviewboard.mozilla.org/r/95128/#review96232

This looks good. Thanks for breaking up the changes - this was a lot easier to review.

::: security/certverifier/MultiLogCTVerifier.cpp:153
(Diff revision 1)
>      }
>    }
>  
>    if (!matchingLog) {
>      // SCT does not match any known log.
> -    return StoreVerifiedSct(result, Move(sct),
> +    return StoreVerifiedSct(result,

nit: we can probably re-break these lines

::: security/certverifier/MultiLogCTVerifier.cpp:159
(Diff revision 1)
> -      SignedCertificateTimestamp::VerificationStatus::UnknownLog);
> +      Move(verifiedSct), VerifiedSCT::Status::UnknownLog);
>    }
>  
>    if (!matchingLog->SignatureParametersMatch(sct.signature)) {
>      // SCT signature parameters do not match the log's.
> -    return StoreVerifiedSct(result, Move(sct),
> +    return StoreVerifiedSct(result,

(same)

::: security/certverifier/MultiLogCTVerifier.cpp:166
(Diff revision 1)
>    }
>  
>    Result rv = matchingLog->Verify(expectedEntry, sct);
>    if (rv != Success) {
>      if (rv == Result::ERROR_BAD_SIGNATURE) {
> -      return StoreVerifiedSct(result, Move(sct),
> +      return StoreVerifiedSct(result,

(same)

::: security/certverifier/MultiLogCTVerifier.cpp:180
(Diff revision 1)
> +  // it does not matter.
>    Time sctTime = TimeFromEpochInSeconds((sct.timestamp + 999u) / 1000u);
> -
> -  // SCT verified ok, just make sure the timestamp is legitimate.
>    if (sctTime > time) {
> -    return StoreVerifiedSct(result, Move(sct),
> +    return StoreVerifiedSct(result,

(same)

::: security/certverifier/MultiLogCTVerifier.cpp:185
(Diff revision 1)
> -    return StoreVerifiedSct(result, Move(sct),
> -      SignedCertificateTimestamp::VerificationStatus::InvalidTimestamp);
> +    return StoreVerifiedSct(result,
> +      Move(verifiedSct), VerifiedSCT::Status::InvalidTimestamp);
>    }
>  
> -  return StoreVerifiedSct(result, Move(sct),
> -    SignedCertificateTimestamp::VerificationStatus::OK);
> +  // SCT verified ok.
> +  return StoreVerifiedSct(result,

(same)

::: security/certverifier/tests/gtest/MultiLogCTVerifierTest.cpp:52
(Diff revision 1)
>      // Set the current time making sure all test timestamps are in the past.
>      mNow = TimeFromEpochInSeconds(1451606400u); // Date.parse("2016-01-01")/1000
>    }
>  
> -  void CheckForSingleVerifiedSCTInResult(const CTVerifyResult& result,
> -    SignedCertificateTimestamp::Origin origin)
> +  void CheckForSingleValidSCTInResult(const CTVerifyResult& result,
> +    VerifiedSCT::Origin origin)

nit: maybe indent this to line up with the argument from the previous line
Attachment #8813735 - Flags: review?(dkeeler) → review+
Comment on attachment 8813736 [details]
Bug 1317951, part 2 - Certificate Transparency - basic support for disqualified logs.

https://reviewboard.mozilla.org/r/95130/#review96574

This looks good, but I think we need a slightly different approach for noting disqualified logs in CTKnownLogs.h (see comment - really my concern is about manually editing the data). I'm thinking it might be best to address the other review comments here, take out the manually edited parts of CTKnownLogs.h, and then add another commit to this series that implements noting disqualified logs. Does that sound like a good approach?

::: security/certverifier/CTKnownLogs.h:8
(Diff revision 2)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* This file was automatically generated by getCTKnownLogs.py. */
> +/* Note: manually updated the disqualified logs using the info from */

I'm concerned about manually editing an automatically generated file. Is there a way to separate the information so we can have one manually-edited one and one automatically-generated one? Or maybe the extra information can be another input to the generator?

::: security/certverifier/SignedCertificateTimestamp.h:95
(Diff revision 2)
> +// Signed integer sufficient to store the numeric ID of CT log operators
> +// as assigned at https://www.certificate-transparency.org/known-logs .
> +// The assigned IDs are 0-based positive integers.
> +// While not directly related to SCTs, the log operator ID is used when
> +// enforcing the CT Policy on connections.
> +typedef int32_t CTLogOperatorId;

Could we maybe just make this an enum class?

::: security/certverifier/tests/gtest/MultiLogCTVerifierTest.cpp:236
(Diff revision 2)
> +{
> +  MultiLogCTVerifier verifier;
> +  CTLogVerifier log;
> +  const uint64_t disqualificationTime = 12345u;
> +  ASSERT_EQ(Success, log.Init(InputForBuffer(GetTestPublicKey()),
> +    mLogOperatorID, true /*disqualified log*/, disqualificationTime));

For things like this, it might be nice to use an enum class rather than a bool to indicate if a log is disqualified or not.

::: toolkit/components/telemetry/Histograms.json:8566
(Diff revision 2)
>      "expires_in_version": "never",
>      "kind": "enumerated",
>      "n_values": 10,
>      "bug_numbers": [1293231],
>      "releaseChannelCollection": "opt-out",
> -    "description": "Verification status of Signed Certificate Timestamps received (0=Decoding error, 1=SCT verified, 2=SCT from unknown log, 3=Invalid SCT signature, 4=SCT timestamp is in the future)"
> +    "description": "Verification status of Signed Certificate Timestamps received (0=Decoding error, 1=Valid SCT, 2=SCT from unknown log, 3=Invalid SCT signature, 4=SCT timestamp is in the future, 5=Valid SCT from a disqualified log)"

Would we want to separate SCTs from a disqualified log that are older than the disqualification date vs. newer than the disqualification date?
Attachment #8813736 - Flags: review?(dkeeler)
Comment on attachment 8813736 [details]
Bug 1317951, part 2 - Certificate Transparency - basic support for disqualified logs.

https://reviewboard.mozilla.org/r/95130/#review96574

> I'm concerned about manually editing an automatically generated file. Is there a way to separate the information so we can have one manually-edited one and one automatically-generated one? Or maybe the extra information can be another input to the generator?

OK, I get your point. Let's see what we have here:

1. We must avoid editing an automatically generated file.
2. Eran said they are planning to include the disqualification status for the logs in https://www.certificate-transparency.org/known-logs/log_list.json , no ETA yet.
3. I understand that Mozilla's future policy on CT logs qualification is currently being discussed, and while we are bootstrapping with the known logs list from Chrome, the lists might (or might not) differ in the future.

So to get things going on our side, I suggest we add log_list.json to the source tree, manually edit it to match https://cs.chromium.org/chromium/src/net/cert/ct_known_logs_static-inc.h (which means removing a couple of pending logs and marking some others as disqualified), and change getCTKnownLogs.py to use the updated format.

What do you think? I'll drop this review and upload the updated code so you can take a look.

> Could we maybe just make this an enum class?

That would be nice, but the problem is that the names for the enum would have to come from the automatically generated file, which would need to be included everywhere. Besides, it would require the operator names in the json (text strings) to never be updated (so we can generate consistently named enums), which is not an obvious assumption when looking at the current format of the json file.

Since currently, we only need to be able to tell whether the logs we are looking at are run by the same (or different) operators, I suggest we keep the IDs as numbers at this stage.

> For things like this, it might be nice to use an enum class rather than a bool to indicate if a log is disqualified or not.

Will fix.

> Would we want to separate SCTs from a disqualified log that are older than the disqualification date vs. newer than the disqualification date?

For this specific probe, we only want to count such "disqualified" SCTs separately from others. The logic of whether an SCT from a disqualified log can be treated as valid depends on many things (see the CT Policy at http://dev.chromium.org/Home/chromium-security/root-ca-policy/CTPolicyMay2016edition.pdf). When we implement the policy, we will add more telemetry probes (Bug 1320567) which will take into account the condition you mention.
Comment on attachment 8813736 [details]
Bug 1317951, part 2 - Certificate Transparency - basic support for disqualified logs.

https://reviewboard.mozilla.org/r/95130/#review96864

Ok - this looks great!

::: security/manager/tools/getCTKnownLogs.py:76
(Diff revisions 2 - 3)
>  
>  #endif // $include_guard
>  """
>  
>  
> +def get_disqualification_time(str):

nit: maybe call the argument 'time_string' or 'time_str' or something?
Attachment #8813736 - Flags: review?(dkeeler) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0d8eb74cce6f
part 1 - Certificate Transparency - extracted verification related fields from SCT to a separate struct. r=keeler
https://hg.mozilla.org/integration/autoland/rev/adf193b5d6c9
part 2 - Certificate Transparency - basic support for disqualified logs. r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d8eb74cce6f
https://hg.mozilla.org/mozilla-central/rev/adf193b5d6c9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: