Certificate Transparency: Add the high-level interface for verifying SCTs over multiple logs

net/cert/multi_log_ct_verifier.cc

Index: net/cert/multi_log_ct_verifier.cc
diff --git a/net/cert/multi_log_ct_verifier.cc b/net/cert/multi_log_ct_verifier.cc
new file mode 100644
index 0000000000000000000000000000000000000000..37c47b8a9654d804547249d242768aa5d696843d
--- /dev/null
+++ b/net/cert/multi_log_ct_verifier.cc
@@ -0,0 +1,145 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/cert/multi_log_ct_verifier.h"
+
+#include "net/base/net_errors.h"
+#include "net/cert/ct_log_verifier.h"
+#include "net/cert/ct_objects_extractor.h"
+#include "net/cert/ct_serialization.h"
+#include "net/cert/ct_verify_result.h"
+#include "net/cert/signed_certificate_timestamp.h"

[wtc, 2013/11/21 02:05:02]

Nit: already included by the .h file.

[Eran M. (Google), 2013/11/21 20:06:02]

Done.

+#include "net/cert/x509_certificate.h"
+
+namespace net {
+
+MultiLogCTVerifier::MultiLogCTVerifier() {
+}
+
+MultiLogCTVerifier::~MultiLogCTVerifier() { }

[wtc, 2013/11/21 02:05:02]

Nit: the constructor and destructor should use the same style for an empty
function body.

[wtc, 2013/11/21 02:05:02]

Nit: the constructor and destructor should use the same style for an empty
function body.

[Eran M. (Google), 2013/11/21 20:06:02]

Done.

[Eran M. (Google), 2013/11/21 20:06:02]

Done.

+
+void MultiLogCTVerifier::AddLog(scoped_ptr<CTLogVerifier> log_verifier) {
+  std::string log_id(log_verifier->key_id());
+  logs_[log_id] = linked_ptr<CTLogVerifier>(log_verifier.release());

[wtc, 2013/11/21 02:05:02]

1. Nit: you should be able to do
  logs_[log_id].reset(log_verifier.release());

2. Nit: It seems wrong to mix scoped_ptr and linked_ptr.

[Eran M. (Google), 2013/11/21 20:06:02]

1. Done

2. Will look into changing the type of the input parameter to linked_ptr as well

[wtc, 2013/11/21 23:26:34]

You can declare the |log_verifier| parameter as a plain pointer and document
that MultiLogCTVerifier takes ownership of |log_verifier|.

Declaring |log_verifier| as a linked_ptr also seems fine.

I am not an expert in this area, so my opinion may be wrong.

[Ryan Sleevi, 2013/11/21 23:37:02]

Taking a scoped_ptr<CTLogVerifier> and turning it into a linked_ptr<> is the
"Right Way". The scoped_ptr<> clearly communicates that ownership is
transferred; this is the preferred pattern over documentation. Releasing it into
a linked_ptr<> is also fine, as the ownership is then transferred into a
linked_ptr<>.

So this code looks fine to me.

The only thing I'm not sure of is why you don't do
linked_ptr<CTLogVerifier> log(log_verifier);
logs_[log->key_id()] = log;

I checked the C++03 spec and couldn't find a guarantee that
logs_[log_verifier->key_id()] = linked_ptr<...> was safe (even though I'm 95%
sure it is), but at least with the above construct, you trade a malloc (since
the default small-string optimization size for GCC and MSVC is ~16 bytes, and
ISTR that key IDs are ~20 bytes) for an int inc/dec (to the linked_ptr's
refcount)

[Eran M. (Google), 2013/11/23 21:02:06]

Done as Ryan suggested - only modification is I've initialized log with
log_verifier.release().

[wtc, 2013/11/25 01:48:32]

I think
    logs_[log_id].reset(log_verifier.release());
is cheaper because it avoids a linked_ptr constructor and destructor call, which
is why I suggested it. It seems that the reset() method is intended exactly for
taking a plain pointer. I am curious why you prefer the assignment operator.

[wtc, 2013/11/25 23:01:59]

Thanks for the explanation.

You wrote:
So the local variable |log| in
  linked_ptr<CTLogVerifier> log(log_verifier.release());
  logs_[log->key_id()] = log;
can used by the compiler (through RVO) as the second argument for the make_pair
in the equivalent code you showed?

I am surprised the compiler can pull that off. That would indeed be cheaper than
using reset(), which doesn't require a link_ptr local variable or temporary
object but requires constructing an empty linked_ptr for make_pair.

+}
+
+int MultiLogCTVerifier::Verify(
+    X509Certificate* verified_cert,
+    const std::string& sct_list_from_ocsp,
+    const std::string& sct_list_from_tls_handshake,
+    ct::CTVerifyResult* result)  {
+  DCHECK(verified_cert);
+  DCHECK(result);
+
+  result->verified_scts.clear();
+  result->unverified_scts.clear();
+  result->unknown_logs_scts.clear();
+
+  bool has_verified_scts = false;
+
+  std::string embedded_scts;
+  if (!verified_cert->GetIntermediateCertificates().empty() &&
+      ct::ExtractEmbeddedSCTList(
+        verified_cert->os_cert_handle(),
+        &embedded_scts)) {

[wtc, 2013/11/21 02:05:02]

Nit: indent by four spaces with respect to "ct::ExtractEmbeddedSCTList(".

[Eran M. (Google), 2013/11/21 20:06:02]

Done.

+    ct::LogEntry embedded_log_entry;

[wtc, 2013/11/21 02:05:02]

Nit: if the |x509_entry| variable you declare below is also an embedded log
entry, then I suggest renaming this variable to "precert_entry".

[Eran M. (Google), 2013/11/21 20:06:02]

Done.

+
+    has_verified_scts =
+      ct::GetPrecertLogEntry(

[wtc, 2013/11/21 02:05:02]

Nit: indent by four spaces.

[Eran M. (Google), 2013/11/21 20:06:02]

Done.

+          verified_cert->os_cert_handle(),
+          verified_cert->GetIntermediateCertificates().front(),
+          &embedded_log_entry) &&
+      VerifySCTs(
+          embedded_scts,
+          embedded_log_entry,
+          ct::SignedCertificateTimestamp::SCT_EMBEDDED,
+          result);
+  }
+
+  ct::LogEntry x509_entry;
+  if (!ct::GetX509LogEntry(verified_cert->os_cert_handle(), &x509_entry)) {
+    return has_verified_scts ? OK : ERR_FAILED;

[wtc, 2013/11/21 02:05:02]

IMPORTANT: use a more informative error code than ERR_FAILED. You can add a new
error code to net/base/net_error_list.h for CT verification.

[Eran M. (Google), 2013/11/21 20:06:02]

Done - ended up adding another category for the various CT errors. PTAL and let
me know if it better fits one of the existing sections.

+  }

[wtc, 2013/11/21 02:05:02]

Nit: omit curly braces.

[Eran M. (Google), 2013/11/21 20:06:02]

Done.

+
+  has_verified_scts |= VerifySCTs(
+      sct_list_from_ocsp,
+      x509_entry,
+      ct::SignedCertificateTimestamp::SCT_FROM_OCSP_RESPONSE,
+      result);
+
+  has_verified_scts |= VerifySCTs(
+      sct_list_from_tls_handshake,
+      x509_entry,
+      ct::SignedCertificateTimestamp::SCT_FROM_TLS_HANDSHAKE,
+      result);
+
+  //XXX(eranm): Add a specific error code to indicate no presence
+  // of SCTs at all.
+  return has_verified_scts ? OK : ERR_FAILED;
+}
+
+bool MultiLogCTVerifier::VerifySCTs(
+    const std::string& encoded_sct_list,
+    const ct::LogEntry& expected_entry,
+    ct::SignedCertificateTimestamp::Origin origin,
+    ct::CTVerifyResult* result) {
+  if (logs_.empty())
+    return false;
+
+  base::StringPiece temp(encoded_sct_list);
+  std::vector<base::StringPiece> sct_list;
+
+  if (!ct::DecodeSCTList(&temp, &sct_list))
+    return false;
+
+  bool verified = false;
+  for (std::vector<base::StringPiece>::const_iterator it = sct_list.begin();
+       it != sct_list.end(); ++it) {
+    base::StringPiece encoded_sct(*it);
+    ct::SignedCertificateTimestamp decoded_sct;
+    if (!DecodeSignedCertificateTimestamp(&encoded_sct, &decoded_sct)) {
+      // XXX(rsleevi): Should we really just skip over bad SCTs?
+      continue;
+    }
+    decoded_sct.origin = origin;
+
+    verified |= VerifySingleSCT(decoded_sct, expected_entry, result);
+  }
+
+  return verified;
+}
+
+bool MultiLogCTVerifier::VerifySingleSCT(
+    const ct::SignedCertificateTimestamp& sct,
+    const ct::LogEntry& expected_entry,
+    ct::CTVerifyResult* result) {
+
+  // Assume this SCT is untrusted until proven otherwise.
+  result->unverified_scts.push_back(sct);

[wtc, 2013/11/21 02:05:02]

Nit: it seems clearer to just push |sct| to the appropriate member of |result|
before each return statement. This way the person who reads this code doesn't
need to check where we need to pop_back().

[Eran M. (Google), 2013/11/21 20:06:02]

Done - that was my attempt at defensive programming, does not sound like it's
necessary. 

+
+  IDToLogMap::iterator it = logs_.find(sct.log_id);
+  if (it == logs_.end()) {
+    DVLOG(1) << "SCT does not match any known log.";
+    result->unverified_scts.pop_back();
+    result->unknown_logs_scts.push_back(sct);
+    return false;
+  }
+
+  if (!it->second->Verify(expected_entry, sct)) {
+    DVLOG(1) << "Unable to verify SCT signature.";
+    return false;
+  }
+
+  // SCT verified ok, just make sure the timestamp is legitimate.
+  if (sct.timestamp + base::TimeDelta::FromSeconds(1) > base::Time::Now()) {

[wtc, 2013/11/21 02:05:02]

Should add a comment to explain why we need to add one second.

[Eran M. (Google), 2013/11/21 20:06:02]

Done, although I'll be the first to admit it's not a convincing one.
The basic reasoning is to not disqualify SCTs which have just been issued in
case the client and log's clocks are not in perfect sync. This is taken from
here:
https://code.google.com/p/certificate-transparency/source/browse/src/log/log_...

+    DVLOG(1) << "SCT is from the future!";
+    return false;
+  }
+
+  result->unverified_scts.pop_back();
+  result->verified_scts.push_back(sct);
+  return true;
+}
+
+} // namespace net