Do not depend on internals of the SSL state machine.HEADandroid-n-mr2-preview-2android-n-iot-release-smart-display-r2android-n-iot-release-smart-displayandroid-n-iot-release-polk-at1android-n-iot-release-lg-thinq-wk7android-n-iot-release-ihome-igv1android-n-iot-preview-4nougat-iot-releasen-iot-preview-4mastermain

tlsdate has a "time_is_an_illusion" parameter which uses the server's
reported time (within some bounds) to check the certificate against. It
does this by configuring the time on the SSL's X509_VERIFY_PARAM when
one of the SSL3_ST_CR_SRVR_HELLO_A and SSL3_ST_CR_SRVR_HELLO_B states
passes.

In addition to depending on quirks of the OpenSSL state machine which
BoringSSL would otherwise need to emulate, this code is wrong. It needs
to run at a point after the server_random is filled in.  In the original
OpenSSL code, SSL3_ST_CR_SRVR_HELLO_A is when the message header is
read, so this is too early. The _B also wouldn't work in a non-blocking
socket because state mcahine might pause halfway through reading the
body. This probably only worked because it only uses blocking BIOs.

This also depends on OpenSSL's info_callback hacking the state
transitions so SSL_state returned the previous state during the
callback.

Rather than ossify all these bugs, use SSL_CTX_set_cert_verify_callback.
This overrides OpenSSL's call to X509_verify_cert. By looking up the
server random immediately before verification, we are guaranteed
server_random is filled in. At this point we also have an X509_STORE_CTX
available, so we may set the time on it directly.

Change-Id: I0a830984539d7e9e53c78891dea07f27f71edcbf
Test: mma

2 files changed, 34 insertions, 36 deletions

diff --git a/src/tlsdate-helper.c b/src/tlsdate-helper.c
index 319497f..3960458 100644
--- a/src/tlsdate-helper.c
+++ b/src/tlsdate-helper.c
@@ -354,39 +354,38 @@ xfree (void *ptr)
   free(ptr);
 }
 
-void
-openssl_time_callback (const SSL* ssl, int where, int ret)
+static int
+verify_with_server_time (X509_STORE_CTX *store_ctx, void *arg)
 {
-  if (where == SSL_CB_CONNECT_LOOP &&
-      (SSL_state(ssl) == SSL3_ST_CR_SRVR_HELLO_A ||
-       SSL_state(ssl) == SSL3_ST_CR_SRVR_HELLO_B))
+  SSL *ssl = X509_STORE_CTX_get_ex_data(
+      store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
+
+  // XXX TODO: If we want to trust the remote system for time,
+  // can we just read that time out of the remote system and if the
+  // cert verifies, decide that the time is reasonable?
+  // Such a process seems to indicate that a once valid cert would be
+  // forever valid - we stopgap that by ensuring it isn't less than
+  // the latest compiled_time and isn't above max_reasonable_time...
+  // XXX TODO: Solve eternal question about the Chicken and the Egg...
+  uint32_t compiled_time = RECENT_COMPILE_DATE;
+  uint32_t max_reasonable_time = MAX_REASONABLE_TIME;
+  uint32_t server_time;
+  verb("V: freezing time for x509 verification");
+  SSL_get_server_random(ssl, (unsigned char*)&server_time, sizeof(uint32_t));
+  if (compiled_time < ntohl(server_time)
+      &&
+      ntohl(server_time) < max_reasonable_time)
   {
-    // XXX TODO: If we want to trust the remote system for time,
-    // can we just read that time out of the remote system and if the
-    // cert verifies, decide that the time is reasonable?
-    // Such a process seems to indicate that a once valid cert would be
-    // forever valid - we stopgap that by ensuring it isn't less than
-    // the latest compiled_time and isn't above max_reasonable_time...
-    // XXX TODO: Solve eternal question about the Chicken and the Egg...
-    uint32_t compiled_time = RECENT_COMPILE_DATE;
-    uint32_t max_reasonable_time = MAX_REASONABLE_TIME;
-    uint32_t server_time;
-    verb("V: freezing time for x509 verification");
-    SSL_get_server_random(ssl, (unsigned char*)&server_time, sizeof(uint32_t));
-    if (compiled_time < ntohl(server_time)
-        &&
-        ntohl(server_time) < max_reasonable_time)
-    {
-      verb("V: remote peer provided: %d, preferred over compile time: %d",
-            ntohl(server_time), compiled_time);
-      verb("V: freezing time with X509_VERIFY_PARAM_set_time");
-      X509_VERIFY_PARAM_set_time(SSL_get0_param((SSL*)ssl),
-                                 (time_t) ntohl(server_time) + 86400);
-    } else {
-      die("V: the remote server is a false ticker! server: %d compile: %d",
-           ntohl(server_time), compiled_time);
-    }
+    verb("V: remote peer provided: %d, preferred over compile time: %d",
+          ntohl(server_time), compiled_time);
+    verb("V: freezing time with X509_VERIFY_PARAM_set_time");
+    X509_STORE_CTX_set_time(store_ctx, 0, (time_t) ntohl(server_time) + 86400);
+  } else {
+    die("V: the remote server is a false ticker! server: %d compile: %d",
+         ntohl(server_time), compiled_time);
   }
+
+  return X509_verify_cert(store_ctx);
 }
 
 static const char *
@@ -1165,6 +1164,11 @@ run_ssl (uint32_t *time_map, int time_is_an_illusion, int http)
     }
   }
 
+  if (time_is_an_illusion)
+  {
+    SSL_CTX_set_cert_verify_callback(ctx, verify_with_server_time, NULL);
+  }
+
   if (NULL == (s_bio = BIO_new(BIO_s_connect())))
     die ("connect BIO setup failed");
   setup_proxy(s_bio);
@@ -1172,11 +1176,6 @@ run_ssl (uint32_t *time_map, int time_is_an_illusion, int http)
     die ("SSL setup failed");
   SSL_set_bio(ssl, s_bio, s_bio);
 
-  if (time_is_an_illusion)
-  {
-    SSL_set_info_callback(ssl, openssl_time_callback);
-  }
-
   SSL_set_mode(ssl, SSL_MODE_AUTO_RETRY);
   verb("V: opening socket to %s:%s", host, port);
   if ( (1 != BIO_set_conn_hostname(s_bio, host)) ||
diff --git a/src/tlsdate-helper.h b/src/tlsdate-helper.h
index 64e4092..fa9861a 100644
--- a/src/tlsdate-helper.h
+++ b/src/tlsdate-helper.h
@@ -124,7 +124,6 @@ static char *proxy;
 
 static const char *ca_cert_container;
 #ifndef USE_POLARSSL
-void openssl_time_callback (const SSL* ssl, int where, int ret);
 uint32_t get_certificate_keybits (EVP_PKEY *public_key);
 uint32_t check_cn (SSL *ssl, const char *hostname);
 uint32_t check_san (SSL *ssl, const char *hostname);