Limit lifetime of self-signed certificate used for TLS on Cast channel

extensions/browser/api/cast_channel/cast_socket.cc

 // Copyright 2014 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 "extensions/browser/api/cast_channel/cast_socket.h"
 
 #include <stdlib.h>
 #include <string.h>
 
 #include "base/bind.h"
@@ skipping 145 matching lines @@
 bool CastSocket::ExtractPeerCert(std::string* cert) {
   DCHECK(cert);
   DCHECK(peer_cert_.empty());
   net::SSLInfo ssl_info;
   if (!socket_->GetSSLInfo(&ssl_info) || !ssl_info.cert.get()) {
     return false;
   }
 
   logger_->LogSocketEvent(channel_id_, proto::SSL_INFO_OBTAINED);
 
+  // we need to check that the peer cert (which is self-signed) doesn't have an

[mark a. foltz, 2014/11/01 20:17:34]

Please write a function in either cast_auth_util.cc (if the same code is
applicable to NSS and openssl) or NSS and openssl implementations written in
cast_auth_util_{nss,openssl}.cc

[dougsteed, 2014/11/03 21:06:11]

I set out to do this as you suggested, but it seems to require a
disproportionate amount of new code to verify a simple time comparison check. I
got so far as to add a function to cast_auth_util, but the body of the function
is literally two lines long (I realized that the code had been more complicated
than was necessary, and is simpler in the second draft). In order to add a unit
test for this simple function, I would need to create and import one or two
specially constructed X.509 certificates so that I can extract their expiry
times and do a comparison with a known time. Note also that there are already
unit tests for extracting the expiry time of a certificate in
net/cert/x509_certificate_unittest.cc. I will do this if you still think it
would be worthwhile.

+  // excessive life-time (i.e. no more than 48 hrs).
+  net::X509Certificate::OSCertHandles no_intermediates;
+  scoped_refptr<net::X509Certificate> peer_cert =
+      net::X509Certificate::CreateFromHandle(ssl_info.cert->os_cert_handle(),
+                                             no_intermediates);
+
+  base::Time expiry = peer_cert->valid_expiry();
+  if (expiry.is_null()) {
+    return false;
+  }
+
+  base::Time should_have_expired_by =
+      base::Time::Now() + base::TimeDelta::FromDays(2);

[mark a. foltz, 2014/11/01 20:17:34]

This is not really a security feature as the system clock can be trivially set
to bypass this check.

[dougsteed, 2014/11/03 21:06:11]

Yes, we know this is not a strong security check, and can be overridden by the
user. However it is a simple change which should, in conjunction with some
receiver changes, help mitigate a particular exploit, so we are making this
change on all senders. There will likely be other changes.

+
+  if (expiry > should_have_expired_by) {
+    LOG(ERROR) << "Peer cert has excessive lifetime. notAfter=" << expiry;
+    return false;
+  }
+
   bool result = net::X509Certificate::GetDEREncoded(
      ssl_info.cert->os_cert_handle(), cert);
   if (result) {
     VLOG_WITH_CONNECTION(1) << "Successfully extracted peer certificate: "
                             << *cert;
   }
 
   logger_->LogSocketEventWithRv(
       channel_id_, proto::DER_ENCODED_CERT_OBTAIN, result ? 1 : 0);
   return result;
@@ skipping 701 matching lines @@
 }
 
 CastSocket::WriteRequest::~WriteRequest() {
 }
 
 }  // namespace cast_channel
 }  // namespace core_api
 }  // namespace extensions
 
 #undef VLOG_WITH_CONNECTION