diff --git a/crypto/err/ssl.errordata b/crypto/err/ssl.errordata index 690ea2b0..4fc89d44 100644 --- a/crypto/err/ssl.errordata +++ b/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 diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index ebede0b8..656a901c 100644 --- a/include/openssl/ssl.h +++ b/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 diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index a7e3b9c0..61f0626c 100644 --- a/ssl/ssl_lib.c +++ b/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; } diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 70743df1..56df2afb 100644 --- a/ssl/t1_lib.c +++ b/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; } diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc index 261344ae..add45aed 100644 --- a/ssl/test/bssl_shim.cc +++ b/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); } diff --git a/ssl/test/runner/common.go b/ssl/test/runner/common.go index bad3ebe5..fb78ef1c 100644 --- a/ssl/test/runner/common.go +++ b/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() { diff --git a/ssl/test/runner/handshake_server.go b/ssl/test/runner/handshake_server.go index 3902ed3e..5d376745 100644 --- a/ssl/test/runner/handshake_server.go +++ b/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 { diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index d4804bf3..ff10c059 100644 --- a/ssl/test/runner/runner.go +++ b/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() diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc index 6b831f0f..fef51ed2 100644 --- a/ssl/test/test_config.cc +++ b/ssl/test/test_config.cc @@ -68,7 +68,6 @@ const Flag 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 }, diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h index b05db165..67655f4f 100644 --- a/ssl/test/test_config.h +++ b/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;