Navigation Menu

Skip to content

Commit

Permalink
[tls] Avoid potential out-of-bound reads in length fields
Browse files Browse the repository at this point in the history 
Many TLS records contain variable-length fields.  We currently
validate the overall record length, but do so only after reading the
length of the variable-length field.  If the record is too short to
even contain the length field, then we may read uninitialised data
from beyond the end of the record.

This is harmless in practice (since the subsequent overall record
length check would fail regardless of the value read from the
uninitialised length field), but causes warnings from some analysis
tools.

Fix by validating that the overall record length is sufficient to
contain the length field before reading from the length field.

Signed-off-by: Michael Brown <mcb30@ipxe.org>
  • Loading branch information
@mcb30
mcb30 committed Mar 11, 2016
1 parent e303a6b commit 05dcb07
Showing 1 changed file with 67 additions and 44 deletions.
111 changes: 67 additions & 44 deletions src/net/tls.c
View file
Expand Up @@ -1271,10 +1271,9 @@ static int tls_new_alert ( struct tls_session *tls, const void *data,
uint8_t description;
char next[0];
} __attribute__ (( packed )) *alert = data;
const void *end = alert->next;

/* Sanity check */
if ( end != ( data + len ) ) {
if ( sizeof ( *alert ) != len ) {
DBGC ( tls, "TLS %p received overlength Alert\n", tls );
DBGC_HD ( tls, data, len );
return -EINVAL_ALERT;
Expand Down Expand Up @@ -1310,24 +1309,28 @@ static int tls_new_server_hello ( struct tls_session *tls,
uint16_t version;
uint8_t random[32];
uint8_t session_id_len;
char next[0];
uint8_t session_id[0];
} __attribute__ (( packed )) *hello_a = data;
const uint8_t *session_id;
const struct {
uint8_t session_id[hello_a->session_id_len];
uint16_t cipher_suite;
uint8_t compression_method;
char next[0];
} __attribute__ (( packed )) *hello_b = ( void * ) &hello_a->next;
const void *end = hello_b->next;
} __attribute__ (( packed )) *hello_b;
uint16_t version;
int rc;

/* Sanity check */
if ( end > ( data + len ) ) {
/* Parse header */
if ( ( sizeof ( *hello_a ) > len ) ||
( hello_a->session_id_len > ( len - sizeof ( *hello_a ) ) ) ||
( sizeof ( *hello_b ) > ( len - sizeof ( *hello_a ) -
hello_a->session_id_len ) ) ) {
DBGC ( tls, "TLS %p received underlength Server Hello\n", tls );
DBGC_HD ( tls, data, len );
return -EINVAL_HELLO;
}
session_id = hello_a->session_id;
hello_b = ( ( void * ) ( session_id + hello_a->session_id_len ) );

/* Check and store protocol version */
version = ntohs ( hello_a->version );
Expand Down Expand Up @@ -1380,14 +1383,7 @@ static int tls_new_server_hello ( struct tls_session *tls,
*/
static int tls_parse_chain ( struct tls_session *tls,
const void *data, size_t len ) {
const void *end = ( data + len );
const struct {
tls24_t length;
uint8_t data[0];
} __attribute__ (( packed )) *certificate;
size_t certificate_len;
struct x509_certificate *cert;
const void *next;
size_t remaining = len;
int rc;

/* Free any existing certificate chain */
Expand All @@ -1402,39 +1398,53 @@ static int tls_parse_chain ( struct tls_session *tls,
}

/* Add certificates to chain */
while ( data < end ) {

/* Extract raw certificate data */
certificate = data;
while ( remaining ) {
const struct {
tls24_t length;
uint8_t data[0];
} __attribute__ (( packed )) *certificate = data;
size_t certificate_len;
size_t record_len;
struct x509_certificate *cert;

/* Parse header */
if ( sizeof ( *certificate ) > remaining ) {
DBGC ( tls, "TLS %p underlength certificate:\n", tls );
DBGC_HDA ( tls, 0, data, remaining );
rc = -EINVAL_CERTIFICATE;
goto err_underlength;
}
certificate_len = tls_uint24 ( &certificate->length );
next = ( certificate->data + certificate_len );
if ( next > end ) {
if ( certificate_len > ( remaining - sizeof ( *certificate ) )){
DBGC ( tls, "TLS %p overlength certificate:\n", tls );
DBGC_HDA ( tls, 0, data, ( end - data ) );
DBGC_HDA ( tls, 0, data, remaining );
rc = -EINVAL_CERTIFICATE;
goto err_overlength;
}
record_len = ( sizeof ( *certificate ) + certificate_len );

/* Add certificate to chain */
if ( ( rc = x509_append_raw ( tls->chain, certificate->data,
certificate_len ) ) != 0 ) {
DBGC ( tls, "TLS %p could not append certificate: %s\n",
tls, strerror ( rc ) );
DBGC_HDA ( tls, 0, data, ( end - data ) );
DBGC_HDA ( tls, 0, data, remaining );
goto err_parse;
}
cert = x509_last ( tls->chain );
DBGC ( tls, "TLS %p found certificate %s\n",
tls, x509_name ( cert ) );

/* Move to next certificate in list */
data = next;
data += record_len;
remaining -= record_len;
}

return 0;

err_parse:
err_overlength:
err_underlength:
x509_chain_put ( tls->chain );
tls->chain = NULL;
err_alloc_chain:
Expand All @@ -1455,12 +1465,18 @@ static int tls_new_certificate ( struct tls_session *tls,
tls24_t length;
uint8_t certificates[0];
} __attribute__ (( packed )) *certificate = data;
size_t certificates_len = tls_uint24 ( &certificate->length );
const void *end = ( certificate->certificates + certificates_len );
size_t certificates_len;
int rc;

/* Sanity check */
if ( end != ( data + len ) ) {
/* Parse header */
if ( sizeof ( *certificate ) > len ) {
DBGC ( tls, "TLS %p received underlength Server Certificate\n",
tls );
DBGC_HD ( tls, data, len );
return -EINVAL_CERTIFICATES;
}
certificates_len = tls_uint24 ( &certificate->length );
if ( certificates_len > ( len - sizeof ( *certificate ) ) ) {
DBGC ( tls, "TLS %p received overlength Server Certificate\n",
tls );
DBGC_HD ( tls, data, len );
Expand Down Expand Up @@ -1521,11 +1537,10 @@ static int tls_new_server_hello_done ( struct tls_session *tls,
const struct {
char next[0];
} __attribute__ (( packed )) *hello_done = data;
const void *end = hello_done->next;
int rc;

/* Sanity check */
if ( end != ( data + len ) ) {
if ( sizeof ( *hello_done ) != len ) {
DBGC ( tls, "TLS %p received overlength Server Hello Done\n",
tls );
DBGC_HD ( tls, data, len );
Expand Down Expand Up @@ -1557,12 +1572,11 @@ static int tls_new_finished ( struct tls_session *tls,
uint8_t verify_data[12];
char next[0];
} __attribute__ (( packed )) *finished = data;
const void *end = finished->next;
uint8_t digest_out[ digest->digestsize ];
uint8_t verify_data[ sizeof ( finished->verify_data ) ];

/* Sanity check */
if ( end != ( data + len ) ) {
if ( sizeof ( *finished ) != len ) {
DBGC ( tls, "TLS %p received overlength Finished\n", tls );
DBGC_HD ( tls, data, len );
return -EINVAL_FINISHED;
Expand Down Expand Up @@ -1598,27 +1612,37 @@ static int tls_new_finished ( struct tls_session *tls,
*/
static int tls_new_handshake ( struct tls_session *tls,
const void *data, size_t len ) {
const void *end = ( data + len );
size_t remaining = len;
int rc;

while ( data != end ) {
while ( remaining ) {
const struct {
uint8_t type;
tls24_t length;
uint8_t payload[0];
} __attribute__ (( packed )) *handshake = data;
const void *payload = &handshake->payload;
size_t payload_len = tls_uint24 ( &handshake->length );
const void *next = ( payload + payload_len );
const void *payload;
size_t payload_len;
size_t record_len;

/* Sanity check */
if ( next > end ) {
/* Parse header */
if ( sizeof ( *handshake ) > remaining ) {
DBGC ( tls, "TLS %p received underlength Handshake\n",
tls );
DBGC_HD ( tls, data, remaining );
return -EINVAL_HANDSHAKE;
}
payload_len = tls_uint24 ( &handshake->length );
if ( payload_len > ( remaining - sizeof ( *handshake ) ) ) {
DBGC ( tls, "TLS %p received overlength Handshake\n",
tls );
DBGC_HD ( tls, data, len );
return -EINVAL_HANDSHAKE;
}
payload = &handshake->payload;
record_len = ( sizeof ( *handshake ) + payload_len );

/* Handle payload */
switch ( handshake->type ) {
case TLS_SERVER_HELLO:
rc = tls_new_server_hello ( tls, payload, payload_len );
Expand Down Expand Up @@ -1648,16 +1672,15 @@ static int tls_new_handshake ( struct tls_session *tls,
* which are explicitly excluded).
*/
if ( handshake->type != TLS_HELLO_REQUEST )
tls_add_handshake ( tls, data,
sizeof ( *handshake ) +
payload_len );
tls_add_handshake ( tls, data, record_len );

/* Abort on failure */
if ( rc != 0 )
return rc;

/* Move to next handshake record */
data = next;
data += record_len;
remaining -= record_len;
}

return 0;
Expand Down

0 comments on commit 05dcb07

Please sign in to comment.