net: add DANE support for DNSSEC stapled certificates.

net/socket/ssl_client_socket_nss.cc

Index: net/socket/ssl_client_socket_nss.cc
diff --git a/net/socket/ssl_client_socket_nss.cc b/net/socket/ssl_client_socket_nss.cc
index dd3b13a16905338f08c50ec2005fa15f27bac941..acbb875b5af48fac374d8124e21cd09fc0b97396 100644
--- a/net/socket/ssl_client_socket_nss.cc
+++ b/net/socket/ssl_client_socket_nss.cc
@@ -278,6 +278,7 @@ enum DNSValidationResult {
 //    server_cert_nss: the server's leaf certificate.
 //    rrdatas: the CAA records for the current domain.
 //    port: the TCP port number that we connected to.
+// TODO(agl): remove once DANE support has been released.
 DNSValidationResult VerifyCAARecords(
     CERTCertificate* server_cert_nss,
     const std::vector<base::StringPiece>& rrdatas,
@@ -323,6 +324,55 @@ DNSValidationResult VerifyCAARecords(
   return DNSVR_FAILURE;
 }
 
+// VerifyTLSARecords processes DNSSEC validated RRDATA for a number of DNS TLSA
+// records and checks them against the given chain.
+//    server_cert_nss: the server's leaf certificate.
+//    rrdatas: the TLSA records for the current domain.
+DNSValidationResult VerifyTLSARecords(
+    CERTCertificate* server_cert_nss,
+    const std::vector<base::StringPiece>& rrdatas) {
+  std::vector<DnsTLSARecord::Match> matches;
+  DnsTLSARecord::Parse(rrdatas, &matches);

[Ryan Sleevi, 2012/10/18 23:21:13]

Should a certificate with invalid record types be accepted? I can see the
argument either way, but given that it's security relevant, I'm inclined to
suspect the answer should be "As little junk as possible should be accepted".
Thoughts?

[agl, 2012/10/29 15:41:56]

Invalid record types are future record types that we just haven't implemented
yet, right? Since the record types are a registry it seems that we need to
ignore values that we don't know in the same way that we ignore TLS extensions
that we don't know.

+
+  for (std::vector<DnsTLSARecord::Match>::const_iterator
+       i = matches.begin(); i != matches.end(); i++) {

[Ryan Sleevi, 2012/10/18 23:21:13]

style nit: pre-increment

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc...

[agl, 2012/10/29 15:41:56]

Done.

+    SECItem matched_data;
+    switch (i->target) {
+    case DnsTLSARecord::Match::CERTIFICATE:
+      matched_data = server_cert_nss->derCert;
+      break;
+    case DnsTLSARecord::Match::SUBJECT_PUBLIC_KEY_INFO:
+      matched_data = server_cert_nss->derPublicKey;
+      break;
+    default:
+      continue;
+    }

[Ryan Sleevi, 2012/10/18 23:21:13]

style nit: indentation of cases

[agl, 2012/10/29 15:41:56]

Done.

+
+    uint8 calculated_hash[SHA512_LENGTH];  // SHA512 is the largest.

[Ryan Sleevi, 2012/10/18 23:21:13]

nit: HASH_LENGTH_MAX (comes from hasht.h, the same place as HASHContext)

[agl, 2012/10/29 15:41:56]

Done.

+    SECItem processed_data;
+    if (i->algorithm == HASH_AlgNULL) {
+      processed_data = matched_data;
+    } else {
+      SECStatus rv = HASH_HashBuf(
+          static_cast<HASH_HashType>(i->algorithm),

[Ryan Sleevi, 2012/10/18 23:21:13]

DESIGN: You seem to have tightly coupled the HASH_HashType enum to your int
constant. This seems problematic, in that it's easy for the Chromium side to add
a new value that is not valid.

Perhaps a comment to the appropriate headers explaining this coupling would be
suitable.

[agl, 2012/10/29 15:41:56]

The alternative would be Yet Another Hash Type enum, no? The worry is that
Chrome adds HASH_SHA3, or some such, which isn't implemented in the system NSS?

Since that could happen, I've changed the check of rv to skip records where we
fail to hash the input.

+          calculated_hash,
+          matched_data.data,
+          matched_data.len);
+      DCHECK(rv == SECSuccess);

[Ryan Sleevi, 2012/10/18 23:21:13]

nit: DCHECK_EQ(SECSuccess, rv)

+      processed_data.data = calculated_hash;
+      processed_data.len = i->data.size();
+    }
+
+    if (processed_data.len == i->data.size() &&
+        memcmp(processed_data.data, i->data.data(), i->data.size()) == 0) {
+      return DNSVR_SUCCESS;
+    }
+  }
+
+  // No TLSA records matched so we reject the certificate.
+  return DNSVR_FAILURE;
+}
+
 // CheckDNSSECChain tries to validate a DNSSEC chain embedded in
 // |server_cert_nss|. It returns true iff a chain is found that proves the
 // value of a CAA record that contains a valid public key fingerprint.
@@ -363,27 +413,50 @@ DNSValidationResult CheckDNSSECChain(
   if (rv != SECSuccess)
     return DNSVR_CONTINUE;
 
-  base::StringPiece chain(
+  std::string chain(
       reinterpret_cast<char*>(dnssec_embedded_chain.data),
       dnssec_embedded_chain.len);
+  SECITEM_FreeItem(&dnssec_embedded_chain, PR_FALSE);
   std::string dns_hostname;
   if (!DNSDomainFromDot(hostname, &dns_hostname))
     return DNSVR_CONTINUE;
+
   DNSSECChainVerifier verifier(dns_hostname, chain);
   DNSSECChainVerifier::Error err = verifier.Verify();
+  bool is_tlsa = false;
+  if (err == DNSSECChainVerifier::BAD_TARGET) {
+    // We might have failed because this is a DANE chain, not a CAA chain.
+    // DANE stores records in a subdomain of the name that includes the port
+    // and protocol, i.e. _443._tcp.www.example.com. We have to construct
+    // such a name for |hostname|.
+    const std::string port_str = base::UintToString(port);
+    char port_label_len = 1 + port_str.size();

[Ryan Sleevi, 2012/10/18 23:21:13]

nit: I would expect a warning here from at least one of our compilers
(converting size_t to char - which, I agree, is safe for the constraints of
port_str)

[agl, 2012/10/29 15:41:56]

Added static_cast.

+    const std::string tlsa_domain =
+      std::string(&port_label_len, 1) + "_" + port_str +
+      "\x04_tcp" +
+      dns_hostname;

[Ryan Sleevi, 2012/10/18 23:21:13]

world's tiniest nit: StringPrintf / StringAppend, per the trend internally to
avoid temporary strings?

[agl, 2012/10/29 15:41:56]

While I agree in general, since |dns_hostname| has an embedded NUL, I think
StringPrintf and converting to char* is asking for trouble.

+
+    verifier = DNSSECChainVerifier(tlsa_domain, chain);
+    err = verifier.Verify();
+    is_tlsa = true;
+  }
+
   if (err != DNSSECChainVerifier::OK) {
     LOG(ERROR) << "DNSSEC chain verification failed: " << err;
     return DNSVR_CONTINUE;
   }
 
+  if (is_tlsa) {
+    if (verifier.rrtype() != kDNS_TLSA)
+      return DNSVR_CONTINUE;
+
+    return VerifyTLSARecords(server_cert_nss, verifier.rrdatas());
+  }
+
   if (verifier.rrtype() != kDNS_CAA)
     return DNSVR_CONTINUE;
 
-  DNSValidationResult r = VerifyCAARecords(
-      server_cert_nss, verifier.rrdatas(), port);
-  SECITEM_FreeItem(&dnssec_embedded_chain, PR_FALSE);
-
-  return r;
+  return VerifyCAARecords(server_cert_nss, verifier.rrdatas(), port);
 }
 
 void DestroyCertificates(CERTCertificate** certs, size_t len) {