Skip to content

openssl: Fix heap buffer overflow in TLS PSK callbacks #25134

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
bukka wants to merge 1 commit into from
Closed

openssl: Fix heap buffer overflow in TLS PSK callbacks #25134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions ext/openssl/tests/tls_psk_oversized_identity.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
--TEST--
TLS PSK - oversized identity from an unserialized Psk is rejected
--EXTENSIONS--
openssl
--SKIPIF--
<?php
if (!function_exists("proc_open")) die("skip no proc_open");
if (OPENSSL_VERSION_NUMBER < 0x10101000) die("skip OpenSSL >= 1.1.1 required");
?>
--FILE--
<?php
$serverCode = <<<'CODE'
$serverCtx = stream_context_create(['ssl' => [
'crypto_method' => STREAM_CRYPTO_METHOD_TLSv1_2_SERVER,
'ciphers' => 'PSK',
'psk_server_cb' => function ($stream, string $identity): ?Openssl\Psk {
return new Openssl\Psk(str_repeat("\x42", 16));
},
]]);
$server = stream_socket_server('tls://127.0.0.1:0', $errno, $errstr,
STREAM_SERVER_BIND | STREAM_SERVER_LISTEN, $serverCtx);
phpt_notify_server_start($server);
@stream_socket_accept($server, 3);
CODE;

$clientCode = <<<'CODE'
// unserialize bypasses the constructor length check, so the Psk carries a
// 512-byte identity; the callback must reject it, not overflow the buffer.
$identity = str_repeat('A', 512);
$payload = sprintf('O:11:"Openssl\Psk":2:{s:3:"psk";s:4:"pass";s:8:"identity";s:%d:"%s";}',
strlen($identity), $identity);

$clientCtx = stream_context_create(['ssl' => [
'crypto_method' => STREAM_CRYPTO_METHOD_TLSv1_2_CLIENT,
'ciphers' => 'PSK',
'verify_peer' => false,
'verify_peer_name' => false,
'psk_client_cb' => function ($stream) use ($payload): ?Openssl\Psk {
return unserialize($payload);
},
]]);
try {
@stream_socket_client('tls://{{ ADDR }}', $errno, $errstr,
30, STREAM_CLIENT_CONNECT, $clientCtx);
echo "no exception\n";
} catch (ValueError $e) {
echo "caught: ", $e->getMessage(), "\n";
}
CODE;

include __DIR__ . '/ServerClientTestCase.inc';
ServerClientTestCase::getInstance()->run($clientCode, $serverCode);
?>
--EXPECTF--
caught: PSK identity must be shorter than %d bytes
31 changes: 27 additions & 4 deletions ext/openssl/xp_ssl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1772,8 +1772,6 @@ static unsigned int php_openssl_psk_client_cb(SSL *ssl, const char *hint,
unsigned char *psk_out, unsigned int max_psk_len)
{
(void)hint; /* identity hint deliberately not exposed as it is useless */
(void)max_identity_len;
(void)max_psk_len;

php_stream *stream = (php_stream *)SSL_get_ex_data(ssl,
php_openssl_get_ssl_stream_data_index());
Expand Down Expand Up @@ -1812,6 +1810,18 @@ static unsigned int php_openssl_psk_client_cb(SSL *ssl, const char *hint,
return 0;
}

/* Returned lengths are untrusted, so bound them to the buffers. */
if (ZSTR_LEN(identity_str) >= max_identity_len) {
zend_value_error("PSK identity must be shorter than %u bytes", max_identity_len);
zval_ptr_dtor(&result);
return 0;
}
if (ZSTR_LEN(psk_str) > max_psk_len) {
zend_value_error("PSK key must not exceed %u bytes", max_psk_len);
zval_ptr_dtor(&result);
return 0;
}

memcpy(identity_out, ZSTR_VAL(identity_str), ZSTR_LEN(identity_str));
identity_out[ZSTR_LEN(identity_str)] = '\0';
memcpy(psk_out, ZSTR_VAL(psk_str), ZSTR_LEN(psk_str));
Expand All @@ -1824,8 +1834,6 @@ static unsigned int php_openssl_psk_client_cb(SSL *ssl, const char *hint,
static unsigned int php_openssl_psk_server_cb(SSL *ssl, const char *identity,
unsigned char *psk_out, unsigned int max_psk_len)
{
(void)max_psk_len;

php_stream *stream = (php_stream *)SSL_get_ex_data(ssl,
php_openssl_get_ssl_stream_data_index());
if (stream == NULL) {
Expand Down Expand Up @@ -1865,6 +1873,13 @@ static unsigned int php_openssl_psk_server_cb(SSL *ssl, const char *identity,
return 0;
}

/* Returned length is untrusted, so bound it to the buffer. */
if (ZSTR_LEN(psk_str) > max_psk_len) {
zend_value_error("PSK key must not exceed %u bytes", max_psk_len);
zval_ptr_dtor(&result);
return 0;
}

memcpy(psk_out, ZSTR_VAL(psk_str), ZSTR_LEN(psk_str));
unsigned int psk_len = (unsigned int)ZSTR_LEN(psk_str);

Expand Down Expand Up @@ -1930,6 +1945,14 @@ static int php_openssl_psk_use_session_cb(SSL *ssl, const EVP_MD *md,
return 0;
}

/* Returned length is untrusted, so bound it to the buffer. */
if (ZSTR_LEN(identity_str) > PHP_OPENSSL_PSK_MAX_IDENTITY_LEN) {
zend_value_error("PSK identity must not exceed %d bytes", PHP_OPENSSL_PSK_MAX_IDENTITY_LEN);
SSL_SESSION_free(session);
zval_ptr_dtor(&result);
return 0;
}

/* Identity buffer must outlive this callback. */
if (sslsock->psk_identity_buf == NULL) {
sslsock->psk_identity_buf = emalloc(PHP_OPENSSL_PSK_MAX_IDENTITY_LEN);
Expand Down
Loading