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>
diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata
index 690ea2b..4fc89d4 100644
--- a/crypto/err/ssl.errordata
+++ b/crypto/err/ssl.errordata
@@ -50,7 +50,9 @@
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,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
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index ebede0b..656a901 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1496,11 +1496,6 @@
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 @@
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 @@
#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
diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
index a7e3b9c..61f0626 100644
--- a/ssl/ssl_lib.c
+++ b/ssl/ssl_lib.c
@@ -2824,10 +2824,6 @@
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;
}
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 70743df..56df2af 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -2241,19 +2241,16 @@
* 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 @@
}
}
- 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 @@
* 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 @@
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 @@
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);
-
- 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;
}
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 @@
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;
- if (ext != NULL) {
+ /* 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;
+ }
+
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 @@
}
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 @@
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;
}
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 261344a..add45ae 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -921,7 +921,6 @@
!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);
}
diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go
index bad3ebe..fb78ef1 100644
--- a/ssl/test/runner/common.go
+++ b/ssl/test/runner/common.go
@@ -621,10 +621,6 @@
// 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 @@
// 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() {
diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go
index 3902ed3..5d37674 100644
--- a/ssl/test/runner/handshake_server.go
+++ b/ssl/test/runner/handshake_server.go
@@ -139,8 +139,8 @@
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 {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index d4804bf..ff10c05 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -2997,6 +2997,19 @@
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 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 @@
addRenegotiationTests()
addDTLSReplayTests()
addSigningHashTests()
- addFastRadioPaddingTests()
addDTLSRetransmitTests()
addExportKeyingMaterialTests()
addTLSUniqueTests()
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 6b831f0..fef51ed 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -68,7 +68,6 @@
{ "-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 },
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index b05db16..67655f4 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -59,7 +59,6 @@
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;
