Tidy up extensions stuff and drop fastradio support.

Fastradio was a trick where the ClientHello was padding to at least 1024
bytes in order to trick some mobile radios into entering high-power mode
immediately. After experimentation, the feature is being dropped.

This change also tidies up a bit of the extensions code now that
everything is using the new system.

Change-Id: Icf7892e0ac1fbe5d66a5d7b405ec455c6850a41c
Reviewed-on: https://boringssl-review.googlesource.com/5466
Reviewed-by: Adam Langley <agl@google.com>

crypto/err/ssl.errordata

@@ -50,7 +50,9 @@ SSL,147,ECC_CERT_NOT_FOR_SIGNING
 SSL,148,EMPTY_SRTP_PROTECTION_PROFILE_LIST
 SSL,276,EMS_STATE_INCONSISTENT
 SSL,149,ENCRYPTED_LENGTH_TOO_LONG
+SSL,281,ERROR_ADDING_EXTENSION
 SSL,150,ERROR_IN_RECEIVED_CIPHER_LIST
+SSL,282,ERROR_PARSING_EXTENSION
 SSL,151,EVP_DIGESTSIGNFINAL_FAILED
 SSL,152,EVP_DIGESTSIGNINIT_FAILED
 SSL,153,EXCESSIVE_MESSAGE_SIZE
@@ -73,6 +75,7 @@ SSL,168,LENGTH_MISMATCH
 SSL,169,LIBRARY_HAS_NO_CIPHERS
 SSL,170,MISSING_DH_KEY
 SSL,171,MISSING_ECDSA_SIGNING_CERT
+SSL,283,MISSING_EXTENSION
 SSL,172,MISSING_RSA_CERTIFICATE
 SSL,173,MISSING_RSA_ENCRYPTING_CERT
 SSL,174,MISSING_RSA_SIGNING_CERT

include/openssl/ssl.h

@@ -1496,11 +1496,6 @@ OPENSSL_EXPORT void SSL_CTX_set_alpn_select_cb(
 OPENSSL_EXPORT void SSL_get0_alpn_selected(const SSL *ssl, const uint8_t **data,
                                            unsigned *len);
 
-/* SSL_enable_fastradio_padding controls whether fastradio padding is enabled
- * on |ssl|. If it is, ClientHello messages are padded to 1024 bytes. This
- * causes 3G radios to switch to DCH mode (high data rate). */
-OPENSSL_EXPORT void SSL_enable_fastradio_padding(SSL *ssl, char on_off);
-
 /* SSL_set_reject_peer_renegotiations controls whether renegotiation attempts by
  * the peer are rejected. It may be set at any point in a connection's lifetime
  * to control future renegotiations programmatically. By default, renegotiations
@@ -1743,11 +1738,6 @@ struct ssl_st {
   uint8_t *alpn_client_proto_list;
   unsigned alpn_client_proto_list_len;
 
-  /* fastradio_padding, if true, causes ClientHellos to be padded to 1024
-   * bytes. This ensures that the cellular radio is fast forwarded to DCH (high
-   * data rate) state in 3G networks. */
-  char fastradio_padding;
-
   /* accept_peer_renegotiations, if one, accepts renegotiation attempts from the
    * peer. Otherwise, they will be rejected with a fatal error. */
   char accept_peer_renegotiations;
@@ -2952,6 +2942,9 @@ OPENSSL_EXPORT const char *SSLeay_version(int unused);
 #define SSL_R_TOO_MANY_WARNING_ALERTS 278
 #define SSL_R_UNEXPECTED_EXTENSION 279
 #define SSL_R_SIGNATURE_ALGORITHMS_EXTENSION_SENT_BY_SERVER 280
+#define SSL_R_ERROR_ADDING_EXTENSION 281
+#define SSL_R_ERROR_PARSING_EXTENSION 282
+#define SSL_R_MISSING_EXTENSION 283
 #define SSL_R_SSLV3_ALERT_CLOSE_NOTIFY 1000
 #define SSL_R_SSLV3_ALERT_UNEXPECTED_MESSAGE 1010
 #define SSL_R_SSLV3_ALERT_BAD_RECORD_MAC 1020

ssl/ssl_lib.c

@@ -2824,10 +2824,6 @@ void SSL_CTX_set_dos_protection_cb(
   ctx->dos_protection_cb = cb;
 }
 
-void SSL_enable_fastradio_padding(SSL *s, char on_off) {
-  s->fastradio_padding = on_off;
-}
-
 void SSL_set_reject_peer_renegotiations(SSL *s, int reject) {
   s->accept_peer_renegotiations = !reject;
 }

ssl/t1_lib.c

@@ -2241,19 +2241,16 @@ static const struct tls_extension *tls_extension_find(uint32_t *out_index,
  * is to be done. */
 uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *const buf,
                                     uint8_t *const limit, size_t header_len) {
-  int extdatalen = 0;
-  uint8_t *ret = buf;
-  uint8_t *orig = buf;
-
   /* don't add extensions for SSLv3 unless doing secure renegotiation */
   if (s->client_version == SSL3_VERSION && !s->s3->send_connection_binding) {
-    return orig;
+    return buf;
   }
 
-  ret += 2;
-
-  if (ret >= limit) {
-    return NULL; /* should never occur. */
+  CBB cbb, extensions;
+  CBB_zero(&cbb);
+  if (!CBB_init_fixed(&cbb, buf, limit - buf) ||
+      !CBB_add_u16_length_prefixed(&cbb, &extensions)) {
+    goto err;
   }
 
   s->s3->tmp.extensions.sent = 0;
@@ -2265,32 +2262,22 @@ uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *const buf,
     }
   }
 
-  CBB cbb;
-  if (!CBB_init_fixed(&cbb, ret, limit - ret)) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-    return NULL;
-  }
-
   for (i = 0; i < kNumExtensions; i++) {
-    const size_t len_before = CBB_len(&cbb);
-    if (!kExtensions[i].add_clienthello(s, &cbb)) {
-      CBB_cleanup(&cbb);
-      OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-      return NULL;
+    const size_t len_before = CBB_len(&extensions);
+    if (!kExtensions[i].add_clienthello(s, &extensions)) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_ADDING_EXTENSION);
+      ERR_add_error_dataf("extension: %u", (unsigned)kExtensions[i].value);
+      goto err;
     }
-    const size_t len_after = CBB_len(&cbb);
 
-    if (len_after != len_before) {
+    if (CBB_len(&extensions) != len_before) {
       s->s3->tmp.extensions.sent |= (1u << i);
     }
   }
 
-  ret += CBB_len(&cbb);
-  CBB_cleanup(&cbb);
-
   if (header_len > 0) {
     size_t clienthello_minsize = 0;
-    header_len += ret - orig;
+    header_len += CBB_len(&extensions);
     if (header_len > 0xff && header_len < 0x200) {
       /* Add padding to workaround bugs in F5 terminators. See
        * https://tools.ietf.org/html/draft-agl-tls-padding-03
@@ -2299,15 +2286,6 @@ uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *const buf,
        * it MUST always appear last. */
       clienthello_minsize = 0x200;
     }
-    if (s->fastradio_padding) {
-      /* Pad the ClientHello record to 1024 bytes to fast forward the radio
-       * into DCH (high data rate) state in 3G networks. Note that when
-       * fastradio_padding is enabled, even if the header_len is less than 255
-       * bytes, the padding will be applied regardless. This is slightly
-       * different from the TLS padding extension suggested in
-       * https://tools.ietf.org/html/draft-agl-tls-padding-03 */
-      clienthello_minsize = 0x400;
-    }
     if (header_len < clienthello_minsize) {
       size_t padding_len = clienthello_minsize - header_len;
       /* Extensions take at least four bytes to encode. Always include least
@@ -2319,46 +2297,51 @@ uint8_t *ssl_add_clienthello_tlsext(SSL *s, uint8_t *const buf,
         padding_len = 1;
       }
 
-      if (limit - ret - 4 - (long)padding_len < 0) {
-        return NULL;
+      uint8_t *padding_bytes;
+      if (!CBB_add_u16(&extensions, TLSEXT_TYPE_padding) ||
+          !CBB_add_u16(&extensions, padding_len) ||
+          !CBB_add_space(&extensions, &padding_bytes, padding_len)) {
+        goto err;
       }
 
-      s2n(TLSEXT_TYPE_padding, ret);
-      s2n(padding_len, ret);
-      memset(ret, 0, padding_len);
-      ret += padding_len;
+      memset(padding_bytes, 0, padding_len);
     }
   }
 
-  extdatalen = ret - orig - 2;
-  if (extdatalen == 0) {
-    return orig;
+  if (!CBB_flush(&cbb)) {
+    goto err;
   }
 
-  s2n(extdatalen, orig);
+  uint8_t *ret = buf;
+  const size_t cbb_len = CBB_len(&cbb);
+  /* If only two bytes have been written then the extensions are actually empty
+   * and those two bytes are the zero length. In that case, we don't bother
+   * sending the extensions length. */
+  if (cbb_len > 2) {
+    ret += cbb_len;
+  }
+
+  CBB_cleanup(&cbb);
   return ret;
+
+err:
+  CBB_cleanup(&cbb);
+  OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+  return NULL;
 }
 
 uint8_t *ssl_add_serverhello_tlsext(SSL *s, uint8_t *const buf,
                                     uint8_t *const limit) {
-  int extdatalen = 0;
-  uint8_t *orig = buf;
-  uint8_t *ret = buf;
-
   /* don't add extensions for SSLv3, unless doing secure renegotiation */
   if (s->version == SSL3_VERSION && !s->s3->send_connection_binding) {
-    return orig;
+    return buf;
   }
 
-  ret += 2;
-  if (ret >= limit) {
-    return NULL; /* should never happen. */
-  }
-
-  CBB cbb;
-  if (!CBB_init_fixed(&cbb, ret, limit - ret)) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-    return NULL;
+  CBB cbb, extensions;
+  CBB_zero(&cbb);
+  if (!CBB_init_fixed(&cbb, buf, limit - buf) ||
+      !CBB_add_u16_length_prefixed(&cbb, &extensions)) {
+    goto err;
   }
 
   unsigned i;
@@ -2368,28 +2351,36 @@ uint8_t *ssl_add_serverhello_tlsext(SSL *s, uint8_t *const buf,
       continue;
     }
 
-    if (!kExtensions[i].add_serverhello(s, &cbb)) {
-      CBB_cleanup(&cbb);
-      OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
-      return NULL;
+    if (!kExtensions[i].add_serverhello(s, &extensions)) {
+      OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_ADDING_EXTENSION);
+      ERR_add_error_dataf("extension: %u", (unsigned)kExtensions[i].value);
+      goto err;
     }
   }
 
-  ret += CBB_len(&cbb);
-  CBB_cleanup(&cbb);
+  if (!CBB_flush(&cbb)) {
+    goto err;
+  }
 
-  extdatalen = ret - orig - 2;
-  if (extdatalen == 0) {
-    return orig;
+  uint8_t *ret = buf;
+  const size_t cbb_len = CBB_len(&cbb);
+  /* If only two bytes have been written then the extensions are actually empty
+   * and those two bytes are the zero length. In that case, we don't bother
+   * sending the extensions length. */
+  if (cbb_len > 2) {
+    ret += cbb_len;
   }
 
-  s2n(extdatalen, orig);
+  CBB_cleanup(&cbb);
   return ret;
+
+err:
+  CBB_cleanup(&cbb);
+  OPENSSL_PUT_ERROR(SSL, ERR_R_INTERNAL_ERROR);
+  return NULL;
 }
 
 static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert) {
-  CBS extensions;
-
   size_t i;
   for (i = 0; i < kNumExtensions; i++) {
     if (kExtensions[i].init != NULL) {
@@ -2404,51 +2395,53 @@ static int ssl_scan_clienthello_tlsext(SSL *s, CBS *cbs, int *out_alert) {
   assert(kExtensions[0].value == TLSEXT_TYPE_renegotiate);
 
   /* There may be no extensions. */
-  if (CBS_len(cbs) == 0) {
-    goto no_extensions;
-  }
-
-  /* Decode the extensions block and check it is valid. */
-  if (!CBS_get_u16_length_prefixed(cbs, &extensions) ||
-      !tls1_check_duplicate_extensions(&extensions)) {
-    *out_alert = SSL_AD_DECODE_ERROR;
-    return 0;
-  }
-
-  while (CBS_len(&extensions) != 0) {
-    uint16_t type;
-    CBS extension;
-
-    /* Decode the next extension. */
-    if (!CBS_get_u16(&extensions, &type) ||
-        !CBS_get_u16_length_prefixed(&extensions, &extension)) {
+  if (CBS_len(cbs) != 0) {
+    /* Decode the extensions block and check it is valid. */
+    CBS extensions;
+    if (!CBS_get_u16_length_prefixed(cbs, &extensions) ||
+        !tls1_check_duplicate_extensions(&extensions)) {
       *out_alert = SSL_AD_DECODE_ERROR;
       return 0;
     }
 
-    unsigned ext_index;
-    const struct tls_extension *const ext =
-        tls_extension_find(&ext_index, type);
+    while (CBS_len(&extensions) != 0) {
+      uint16_t type;
+      CBS extension;
+
+      /* Decode the next extension. */
+      if (!CBS_get_u16(&extensions, &type) ||
+          !CBS_get_u16_length_prefixed(&extensions, &extension)) {
+        *out_alert = SSL_AD_DECODE_ERROR;
+        return 0;
+      }
+
+      unsigned ext_index;
+      const struct tls_extension *const ext =
+          tls_extension_find(&ext_index, type);
+
+      if (ext == NULL) {
+        continue;
+      }
 
-    if (ext != NULL) {
       s->s3->tmp.extensions.received |= (1u << ext_index);
       uint8_t alert = SSL_AD_DECODE_ERROR;
       if (!ext->parse_clienthello(s, &alert, &extension)) {
         *out_alert = alert;
+        OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_PARSING_EXTENSION);
+        ERR_add_error_dataf("extension: %u", (unsigned)type);
         return 0;
       }
-
-      continue;
     }
   }
 
-no_extensions:
   for (i = 0; i < kNumExtensions; i++) {
     if (!(s->s3->tmp.extensions.received & (1u << i))) {
       /* Extension wasn't observed so call the callback with a NULL
        * parameter. */
       uint8_t alert = SSL_AD_DECODE_ERROR;
       if (!kExtensions[i].parse_clienthello(s, &alert, NULL)) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_EXTENSION);
+        ERR_add_error_dataf("extension: %u", (unsigned)kExtensions[i].value);
         *out_alert = alert;
         return 0;
       }
@@ -2474,47 +2467,41 @@ int ssl_parse_clienthello_tlsext(SSL *s, CBS *cbs) {
 }
 
 static int ssl_scan_serverhello_tlsext(SSL *s, CBS *cbs, int *out_alert) {
-  CBS extensions;
-
   uint32_t received = 0;
-  size_t i;
   assert(kNumExtensions <= sizeof(received) * 8);
 
-  /* There may be no extensions. */
-  if (CBS_len(cbs) == 0) {
-    goto no_extensions;
-  }
-
-  /* Decode the extensions block and check it is valid. */
-  if (!CBS_get_u16_length_prefixed(cbs, &extensions) ||
-      !tls1_check_duplicate_extensions(&extensions)) {
-    *out_alert = SSL_AD_DECODE_ERROR;
-    return 0;
-  }
-
-  while (CBS_len(&extensions) != 0) {
-    uint16_t type;
-    CBS extension;
-
-    /* Decode the next extension. */
-    if (!CBS_get_u16(&extensions, &type) ||
-        !CBS_get_u16_length_prefixed(&extensions, &extension)) {
+  if (CBS_len(cbs) != 0) {
+    /* Decode the extensions block and check it is valid. */
+    CBS extensions;
+    if (!CBS_get_u16_length_prefixed(cbs, &extensions) ||
+        !tls1_check_duplicate_extensions(&extensions)) {
       *out_alert = SSL_AD_DECODE_ERROR;
       return 0;
     }
 
-    unsigned ext_index;
-    const struct tls_extension *const ext =
-        tls_extension_find(&ext_index, type);
 
-    /* While we have extensions that don't use tls_extension this conditional
-     * needs to be guarded on |ext != NULL|. In the future, ext being NULL will
-     * be fatal. */
-    if (ext != NULL) {
-      if (!(s->s3->tmp.extensions.sent & (1u << ext_index))) {
-        /* Received an extension that was never sent. */
+    while (CBS_len(&extensions) != 0) {
+      uint16_t type;
+      CBS extension;
+
+      /* Decode the next extension. */
+      if (!CBS_get_u16(&extensions, &type) ||
+          !CBS_get_u16_length_prefixed(&extensions, &extension)) {
+        *out_alert = SSL_AD_DECODE_ERROR;
+        return 0;
+      }
+
+      unsigned ext_index;
+      const struct tls_extension *const ext =
+          tls_extension_find(&ext_index, type);
+
+      if (/* If ext == NULL then an unknown extension was received. Since we
+           * cannot have sent an unknown extension, this is illegal. */
+          ext == NULL ||
+          /* If the extension was never sent then it is also illegal. */
+          !(s->s3->tmp.extensions.sent & (1u << ext_index))) {
         OPENSSL_PUT_ERROR(SSL, SSL_R_UNEXPECTED_EXTENSION);
-        ERR_add_error_dataf("ext:%u", (unsigned) type);
+        ERR_add_error_dataf("extension :%u", (unsigned)type);
         *out_alert = SSL_AD_DECODE_ERROR;
         return 0;
       }
@@ -2523,21 +2510,23 @@ static int ssl_scan_serverhello_tlsext(SSL *s, CBS *cbs, int *out_alert) {
 
       uint8_t alert = SSL_AD_DECODE_ERROR;
       if (!ext->parse_serverhello(s, &alert, &extension)) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_ERROR_PARSING_EXTENSION);
+        ERR_add_error_dataf("extension: %u", (unsigned)type);
         *out_alert = alert;
         return 0;
       }
-
-      continue;
     }
   }
 
-no_extensions:
+  size_t i;
   for (i = 0; i < kNumExtensions; i++) {
     if (!(received & (1u << i))) {
       /* Extension wasn't observed so call the callback with a NULL
        * parameter. */
       uint8_t alert = SSL_AD_DECODE_ERROR;
       if (!kExtensions[i].parse_serverhello(s, &alert, NULL)) {
+        OPENSSL_PUT_ERROR(SSL, SSL_R_MISSING_EXTENSION);
+        ERR_add_error_dataf("extension: %u", (unsigned)kExtensions[i].value);
         *out_alert = alert;
         return 0;
       }

ssl/test/bssl_shim.cc

@@ -921,7 +921,6 @@ static bool DoExchange(ScopedSSL_SESSION *out_session, SSL_CTX *ssl_ctx,
       !SSL_enable_signed_cert_timestamps(ssl.get())) {
     return false;
   }
-  SSL_enable_fastradio_padding(ssl.get(), config->fastradio_padding);
   if (config->min_version != 0) {
     SSL_set_min_version(ssl.get(), (uint16_t)config->min_version);
   }

ssl/test/runner/common.go

@@ -621,10 +621,6 @@ type ProtocolBugs struct {
 	// across a renego.
 	RequireSameRenegoClientVersion bool
 
-	// RequireFastradioPadding, if true, requires that ClientHello messages
-	// be at least 1000 bytes long.
-	RequireFastradioPadding bool
-
 	// ExpectInitialRecordVersion, if non-zero, is the expected
 	// version of the records before the version is determined.
 	ExpectInitialRecordVersion uint16
@@ -736,6 +732,10 @@ type ProtocolBugs struct {
 	// ExpectNewTicket, if true, causes the client to abort if it does not
 	// receive a new ticket.
 	ExpectNewTicket bool
+
+	// RequireClientHelloSize, if not zero, is the required length in bytes
+	// of the ClientHello /record/. This is checked by the server.
+	RequireClientHelloSize int
 }
 
 func (c *Config) serverInit() {

ssl/test/runner/handshake_server.go

@@ -139,8 +139,8 @@ func (hs *serverHandshakeState) readClientHello() (isResume bool, err error) {
 		c.sendAlert(alertUnexpectedMessage)
 		return false, unexpectedMessageError(hs.clientHello, msg)
 	}
-	if config.Bugs.RequireFastradioPadding && len(hs.clientHello.raw) < 1000 {
-		return false, errors.New("tls: ClientHello record size should be larger than 1000 bytes when padding enabled.")
+	if size := config.Bugs.RequireClientHelloSize; size != 0 && len(hs.clientHello.raw) != size {
+		return false, fmt.Errorf("tls: ClientHello record size is %d, but expected %d", len(hs.clientHello.raw), size)
 	}
 
 	if c.isDTLS && !config.Bugs.SkipHelloVerifyRequest {

ssl/test/runner/runner.go

@@ -2997,6 +2997,19 @@ func addExtensionTests() {
 			base64.StdEncoding.EncodeToString(testSCTList),
 		},
 	})
+	testCases = append(testCases, testCase{
+		testType: clientTest,
+		name:     "ClientHelloPadding",
+		config: Config{
+			Bugs: ProtocolBugs{
+				RequireClientHelloSize: 512,
+			},
+		},
+		// This hostname just needs to be long enough to push the
+		// ClientHello into F5's danger zone between 256 and 511 bytes
+		// long.
+		flags: []string{"-host-name", "01234567890123456789012345678901234567890123456789012345678901234567890123456789.com"},
+	})
 }
 
 func addResumptionVersionTests() {
@@ -3263,29 +3276,6 @@ func addDTLSReplayTests() {
 	})
 }
 
-func addFastRadioPaddingTests() {
-	testCases = append(testCases, testCase{
-		protocol: tls,
-		name:     "FastRadio-Padding",
-		config: Config{
-			Bugs: ProtocolBugs{
-				RequireFastradioPadding: true,
-			},
-		},
-		flags: []string{"-fastradio-padding"},
-	})
-	testCases = append(testCases, testCase{
-		protocol: dtls,
-		name:     "FastRadio-Padding-DTLS",
-		config: Config{
-			Bugs: ProtocolBugs{
-				RequireFastradioPadding: true,
-			},
-		},
-		flags: []string{"-fastradio-padding"},
-	})
-}
-
 var testHashes = []struct {
 	name string
 	id   uint8
@@ -3730,7 +3720,6 @@ func main() {
 	addRenegotiationTests()
 	addDTLSReplayTests()
 	addSigningHashTests()
-	addFastRadioPaddingTests()
 	addDTLSRetransmitTests()
 	addExportKeyingMaterialTests()
 	addTLSUniqueTests()

ssl/test/test_config.cc

@@ -68,7 +68,6 @@ const Flag<bool> kBoolFlags[] = {
   { "-enable-ocsp-stapling", &TestConfig::enable_ocsp_stapling },
   { "-enable-signed-cert-timestamps",
     &TestConfig::enable_signed_cert_timestamps },
-  { "-fastradio-padding", &TestConfig::fastradio_padding },
   { "-implicit-handshake", &TestConfig::implicit_handshake },
   { "-use-early-callback", &TestConfig::use_early_callback },
   { "-fail-early-callback", &TestConfig::fail_early_callback },

ssl/test/test_config.h

@@ -59,7 +59,6 @@ struct TestConfig {
   std::string expected_ocsp_response;
   bool enable_signed_cert_timestamps = false;
   std::string expected_signed_cert_timestamps;
-  bool fastradio_padding = false;
   int min_version = 0;
   int max_version = 0;
   int mtu = 0;