From: Todd Short Date: Mon, 22 Mar 2021 16:56:36 +0000 (-0400) Subject: Handle set_alpn_protos inputs better. X-Git-Tag: openssl-3.0.0-alpha15~117 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=feba11cf2ea1dee9cc6e30bf5953404c9c2c88c6;p=thirdparty%2Fopenssl.git Handle set_alpn_protos inputs better. It's possible to set an invalid protocol list that will be sent in a ClientHello. This validates the inputs to make sure this does not happen. Reviewed-by: Matt Caswell Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/14815) --- diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index c4394cf9e22..3d0f309fd23 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -2985,6 +2985,19 @@ void SSL_CTX_set_npn_select_cb(SSL_CTX *ctx, } #endif +static int alpn_value_ok(const unsigned char *protos, unsigned int protos_len) +{ + unsigned int idx; + + if (protos_len < 2 || protos == NULL) + return 0; + + for (idx = 0; idx < protos_len; idx += protos[idx] + 1) { + if (protos[idx] == 0) + return 0; + } + return idx == protos_len; +} /* * SSL_CTX_set_alpn_protos sets the ALPN protocol list on |ctx| to |protos|. * |protos| must be in wire-format (i.e. a series of non-empty, 8-bit @@ -2993,13 +3006,25 @@ void SSL_CTX_set_npn_select_cb(SSL_CTX *ctx, int SSL_CTX_set_alpn_protos(SSL_CTX *ctx, const unsigned char *protos, unsigned int protos_len) { - OPENSSL_free(ctx->ext.alpn); - ctx->ext.alpn = OPENSSL_memdup(protos, protos_len); - if (ctx->ext.alpn == NULL) { + unsigned char *alpn; + + if (protos_len == 0 || protos == NULL) { + OPENSSL_free(ctx->ext.alpn); + ctx->ext.alpn = NULL; ctx->ext.alpn_len = 0; + return 0; + } + /* Not valid per RFC */ + if (!alpn_value_ok(protos, protos_len)) + return 1; + + alpn = OPENSSL_memdup(protos, protos_len); + if (alpn == NULL) { ERR_raise(ERR_LIB_SSL, ERR_R_MALLOC_FAILURE); return 1; } + OPENSSL_free(ctx->ext.alpn); + ctx->ext.alpn = alpn; ctx->ext.alpn_len = protos_len; return 0; @@ -3013,13 +3038,25 @@ int SSL_CTX_set_alpn_protos(SSL_CTX *ctx, const unsigned char *protos, int SSL_set_alpn_protos(SSL *ssl, const unsigned char *protos, unsigned int protos_len) { - OPENSSL_free(ssl->ext.alpn); - ssl->ext.alpn = OPENSSL_memdup(protos, protos_len); - if (ssl->ext.alpn == NULL) { + unsigned char *alpn; + + if (protos_len == 0 || protos == NULL) { + OPENSSL_free(ssl->ext.alpn); + ssl->ext.alpn = NULL; ssl->ext.alpn_len = 0; + return 0; + } + /* Not valid per RFC */ + if (!alpn_value_ok(protos, protos_len)) + return 1; + + alpn = OPENSSL_memdup(protos, protos_len); + if (alpn == NULL) { ERR_raise(ERR_LIB_SSL, ERR_R_MALLOC_FAILURE); return 1; } + OPENSSL_free(ssl->ext.alpn); + ssl->ext.alpn = alpn; ssl->ext.alpn_len = protos_len; return 0; diff --git a/test/clienthellotest.c b/test/clienthellotest.c index 2b92395bac4..2f6d336dbc1 100644 --- a/test/clienthellotest.c +++ b/test/clienthellotest.c @@ -45,10 +45,16 @@ static const char *sessionfile = NULL; /* Dummy ALPN protocols used to pad out the size of the ClientHello */ +/* ASCII 'O' = 79 = 0x4F = EBCDIC '|'*/ +#ifdef CHARSET_EBCDIC static const char alpn_prots[] = - "0123456789012345678901234567890123456789012345678901234567890123456789" - "0123456789012345678901234567890123456789012345678901234567890123456789" - "01234567890123456789"; + "|1234567890123456789012345678901234567890123456789012345678901234567890123456789" + "|1234567890123456789012345678901234567890123456789012345678901234567890123456789"; +#else +static const char alpn_prots[] = + "O1234567890123456789012345678901234567890123456789012345678901234567890123456789" + "O1234567890123456789012345678901234567890123456789012345678901234567890123456789"; +#endif static int test_client_hello(int currtest) { diff --git a/test/sslapitest.c b/test/sslapitest.c index 624c7967da3..4625d340463 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -8616,6 +8616,77 @@ end: return testresult; } #endif +/* + * Test that setting an ALPN does not violate RFC + */ +static int test_set_alpn(void) +{ + SSL_CTX *ctx = NULL; + SSL *ssl = NULL; + int testresult = 0; + + unsigned char bad0[] = { 0x00, 'b', 'a', 'd' }; + unsigned char good[] = { 0x04, 'g', 'o', 'o', 'd' }; + unsigned char bad1[] = { 0x01, 'b', 'a', 'd' }; + unsigned char bad2[] = { 0x03, 'b', 'a', 'd', 0x00}; + unsigned char bad3[] = { 0x03, 'b', 'a', 'd', 0x01, 'b', 'a', 'd'}; + unsigned char bad4[] = { 0x03, 'b', 'a', 'd', 0x06, 'b', 'a', 'd'}; + + /* Create an initial SSL_CTX with no certificate configured */ + ctx = SSL_CTX_new_ex(libctx, NULL, TLS_server_method()); + if (!TEST_ptr(ctx)) + goto end; + + /* the set_alpn functions return 0 (false) on success, non-zero (true) on failure */ + if (!TEST_false(SSL_CTX_set_alpn_protos(ctx, NULL, 2))) + goto end; + if (!TEST_false(SSL_CTX_set_alpn_protos(ctx, good, 0))) + goto end; + if (!TEST_false(SSL_CTX_set_alpn_protos(ctx, good, sizeof(good)))) + goto end; + if (!TEST_true(SSL_CTX_set_alpn_protos(ctx, good, 1))) + goto end; + if (!TEST_true(SSL_CTX_set_alpn_protos(ctx, bad0, sizeof(bad0)))) + goto end; + if (!TEST_true(SSL_CTX_set_alpn_protos(ctx, bad1, sizeof(bad1)))) + goto end; + if (!TEST_true(SSL_CTX_set_alpn_protos(ctx, bad2, sizeof(bad2)))) + goto end; + if (!TEST_true(SSL_CTX_set_alpn_protos(ctx, bad3, sizeof(bad3)))) + goto end; + if (!TEST_true(SSL_CTX_set_alpn_protos(ctx, bad4, sizeof(bad4)))) + goto end; + + ssl = SSL_new(ctx); + if (!TEST_ptr(ssl)) + goto end; + + if (!TEST_false(SSL_set_alpn_protos(ssl, NULL, 2))) + goto end; + if (!TEST_false(SSL_set_alpn_protos(ssl, good, 0))) + goto end; + if (!TEST_false(SSL_set_alpn_protos(ssl, good, sizeof(good)))) + goto end; + if (!TEST_true(SSL_set_alpn_protos(ssl, good, 1))) + goto end; + if (!TEST_true(SSL_set_alpn_protos(ssl, bad0, sizeof(bad0)))) + goto end; + if (!TEST_true(SSL_set_alpn_protos(ssl, bad1, sizeof(bad1)))) + goto end; + if (!TEST_true(SSL_set_alpn_protos(ssl, bad2, sizeof(bad2)))) + goto end; + if (!TEST_true(SSL_set_alpn_protos(ssl, bad3, sizeof(bad3)))) + goto end; + if (!TEST_true(SSL_set_alpn_protos(ssl, bad4, sizeof(bad4)))) + goto end; + + testresult = 1; + +end: + SSL_free(ssl); + SSL_CTX_free(ctx); + return testresult; +} static int test_inherit_verify_param(void) { @@ -8908,6 +8979,7 @@ int setup_tests(void) ADD_TEST(test_sni_tls13); #endif ADD_TEST(test_inherit_verify_param); + ADD_TEST(test_set_alpn); return 1; err: