Browse Source

[tls] Avoid potential out-of-bound reads in length fields

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>
tags/v1.20.1
Michael Brown 8 years ago
parent
commit
05dcb07cb2
1 changed files with 67 additions and 44 deletions
  1. 67
    44
      src/net/tls.c

+ 67
- 44
src/net/tls.c View File

@@ -1271,10 +1271,9 @@ static int tls_new_alert ( struct tls_session *tls, const void *data,
1271 1271
 		uint8_t description;
1272 1272
 		char next[0];
1273 1273
 	} __attribute__ (( packed )) *alert = data;
1274
-	const void *end = alert->next;
1275 1274
 
1276 1275
 	/* Sanity check */
1277
-	if ( end != ( data + len ) ) {
1276
+	if ( sizeof ( *alert ) != len ) {
1278 1277
 		DBGC ( tls, "TLS %p received overlength Alert\n", tls );
1279 1278
 		DBGC_HD ( tls, data, len );
1280 1279
 		return -EINVAL_ALERT;
@@ -1310,24 +1309,28 @@ static int tls_new_server_hello ( struct tls_session *tls,
1310 1309
 		uint16_t version;
1311 1310
 		uint8_t random[32];
1312 1311
 		uint8_t session_id_len;
1313
-		char next[0];
1312
+		uint8_t session_id[0];
1314 1313
 	} __attribute__ (( packed )) *hello_a = data;
1314
+	const uint8_t *session_id;
1315 1315
 	const struct {
1316
-		uint8_t session_id[hello_a->session_id_len];
1317 1316
 		uint16_t cipher_suite;
1318 1317
 		uint8_t compression_method;
1319 1318
 		char next[0];
1320
-	} __attribute__ (( packed )) *hello_b = ( void * ) &hello_a->next;
1321
-	const void *end = hello_b->next;
1319
+	} __attribute__ (( packed )) *hello_b;
1322 1320
 	uint16_t version;
1323 1321
 	int rc;
1324 1322
 
1325
-	/* Sanity check */
1326
-	if ( end > ( data + len ) ) {
1323
+	/* Parse header */
1324
+	if ( ( sizeof ( *hello_a ) > len ) ||
1325
+	     ( hello_a->session_id_len > ( len - sizeof ( *hello_a ) ) ) ||
1326
+	     ( sizeof ( *hello_b ) > ( len - sizeof ( *hello_a ) -
1327
+				       hello_a->session_id_len ) ) ) {
1327 1328
 		DBGC ( tls, "TLS %p received underlength Server Hello\n", tls );
1328 1329
 		DBGC_HD ( tls, data, len );
1329 1330
 		return -EINVAL_HELLO;
1330 1331
 	}
1332
+	session_id = hello_a->session_id;
1333
+	hello_b = ( ( void * ) ( session_id + hello_a->session_id_len ) );
1331 1334
 
1332 1335
 	/* Check and store protocol version */
1333 1336
 	version = ntohs ( hello_a->version );
@@ -1380,14 +1383,7 @@ static int tls_new_server_hello ( struct tls_session *tls,
1380 1383
  */
1381 1384
 static int tls_parse_chain ( struct tls_session *tls,
1382 1385
 			     const void *data, size_t len ) {
1383
-	const void *end = ( data + len );
1384
-	const struct {
1385
-		tls24_t length;
1386
-		uint8_t data[0];
1387
-	} __attribute__ (( packed )) *certificate;
1388
-	size_t certificate_len;
1389
-	struct x509_certificate *cert;
1390
-	const void *next;
1386
+	size_t remaining = len;
1391 1387
 	int rc;
1392 1388
 
1393 1389
 	/* Free any existing certificate chain */
@@ -1402,25 +1398,37 @@ static int tls_parse_chain ( struct tls_session *tls,
1402 1398
 	}
1403 1399
 
1404 1400
 	/* Add certificates to chain */
1405
-	while ( data < end ) {
1406
-
1407
-		/* Extract raw certificate data */
1408
-		certificate = data;
1401
+	while ( remaining ) {
1402
+		const struct {
1403
+			tls24_t length;
1404
+			uint8_t data[0];
1405
+		} __attribute__ (( packed )) *certificate = data;
1406
+		size_t certificate_len;
1407
+		size_t record_len;
1408
+		struct x509_certificate *cert;
1409
+
1410
+		/* Parse header */
1411
+		if ( sizeof ( *certificate ) > remaining ) {
1412
+			DBGC ( tls, "TLS %p underlength certificate:\n", tls );
1413
+			DBGC_HDA ( tls, 0, data, remaining );
1414
+			rc = -EINVAL_CERTIFICATE;
1415
+			goto err_underlength;
1416
+		}
1409 1417
 		certificate_len = tls_uint24 ( &certificate->length );
1410
-		next = ( certificate->data + certificate_len );
1411
-		if ( next > end ) {
1418
+		if ( certificate_len > ( remaining - sizeof ( *certificate ) )){
1412 1419
 			DBGC ( tls, "TLS %p overlength certificate:\n", tls );
1413
-			DBGC_HDA ( tls, 0, data, ( end - data ) );
1420
+			DBGC_HDA ( tls, 0, data, remaining );
1414 1421
 			rc = -EINVAL_CERTIFICATE;
1415 1422
 			goto err_overlength;
1416 1423
 		}
1424
+		record_len = ( sizeof ( *certificate ) + certificate_len );
1417 1425
 
1418 1426
 		/* Add certificate to chain */
1419 1427
 		if ( ( rc = x509_append_raw ( tls->chain, certificate->data,
1420 1428
 					      certificate_len ) ) != 0 ) {
1421 1429
 			DBGC ( tls, "TLS %p could not append certificate: %s\n",
1422 1430
 			       tls, strerror ( rc ) );
1423
-			DBGC_HDA ( tls, 0, data, ( end - data ) );
1431
+			DBGC_HDA ( tls, 0, data, remaining );
1424 1432
 			goto err_parse;
1425 1433
 		}
1426 1434
 		cert = x509_last ( tls->chain );
@@ -1428,13 +1436,15 @@ static int tls_parse_chain ( struct tls_session *tls,
1428 1436
 		       tls, x509_name ( cert ) );
1429 1437
 
1430 1438
 		/* Move to next certificate in list */
1431
-		data = next;
1439
+		data += record_len;
1440
+		remaining -= record_len;
1432 1441
 	}
1433 1442
 
1434 1443
 	return 0;
1435 1444
 
1436 1445
  err_parse:
1437 1446
  err_overlength:
1447
+ err_underlength:
1438 1448
 	x509_chain_put ( tls->chain );
1439 1449
 	tls->chain = NULL;
1440 1450
  err_alloc_chain:
@@ -1455,12 +1465,18 @@ static int tls_new_certificate ( struct tls_session *tls,
1455 1465
 		tls24_t length;
1456 1466
 		uint8_t certificates[0];
1457 1467
 	} __attribute__ (( packed )) *certificate = data;
1458
-	size_t certificates_len = tls_uint24 ( &certificate->length );
1459
-	const void *end = ( certificate->certificates + certificates_len );
1468
+	size_t certificates_len;
1460 1469
 	int rc;
1461 1470
 
1462
-	/* Sanity check */
1463
-	if ( end != ( data + len ) ) {
1471
+	/* Parse header */
1472
+	if ( sizeof ( *certificate ) > len ) {
1473
+		DBGC ( tls, "TLS %p received underlength Server Certificate\n",
1474
+		       tls );
1475
+		DBGC_HD ( tls, data, len );
1476
+		return -EINVAL_CERTIFICATES;
1477
+	}
1478
+	certificates_len = tls_uint24 ( &certificate->length );
1479
+	if ( certificates_len > ( len - sizeof ( *certificate ) ) ) {
1464 1480
 		DBGC ( tls, "TLS %p received overlength Server Certificate\n",
1465 1481
 		       tls );
1466 1482
 		DBGC_HD ( tls, data, len );
@@ -1521,11 +1537,10 @@ static int tls_new_server_hello_done ( struct tls_session *tls,
1521 1537
 	const struct {
1522 1538
 		char next[0];
1523 1539
 	} __attribute__ (( packed )) *hello_done = data;
1524
-	const void *end = hello_done->next;
1525 1540
 	int rc;
1526 1541
 
1527 1542
 	/* Sanity check */
1528
-	if ( end != ( data + len ) ) {
1543
+	if ( sizeof ( *hello_done ) != len ) {
1529 1544
 		DBGC ( tls, "TLS %p received overlength Server Hello Done\n",
1530 1545
 		       tls );
1531 1546
 		DBGC_HD ( tls, data, len );
@@ -1557,12 +1572,11 @@ static int tls_new_finished ( struct tls_session *tls,
1557 1572
 		uint8_t verify_data[12];
1558 1573
 		char next[0];
1559 1574
 	} __attribute__ (( packed )) *finished = data;
1560
-	const void *end = finished->next;
1561 1575
 	uint8_t digest_out[ digest->digestsize ];
1562 1576
 	uint8_t verify_data[ sizeof ( finished->verify_data ) ];
1563 1577
 
1564 1578
 	/* Sanity check */
1565
-	if ( end != ( data + len ) ) {
1579
+	if ( sizeof ( *finished ) != len ) {
1566 1580
 		DBGC ( tls, "TLS %p received overlength Finished\n", tls );
1567 1581
 		DBGC_HD ( tls, data, len );
1568 1582
 		return -EINVAL_FINISHED;
@@ -1598,27 +1612,37 @@ static int tls_new_finished ( struct tls_session *tls,
1598 1612
  */
1599 1613
 static int tls_new_handshake ( struct tls_session *tls,
1600 1614
 			       const void *data, size_t len ) {
1601
-	const void *end = ( data + len );
1615
+	size_t remaining = len;
1602 1616
 	int rc;
1603 1617
 
1604
-	while ( data != end ) {
1618
+	while ( remaining ) {
1605 1619
 		const struct {
1606 1620
 			uint8_t type;
1607 1621
 			tls24_t length;
1608 1622
 			uint8_t payload[0];
1609 1623
 		} __attribute__ (( packed )) *handshake = data;
1610
-		const void *payload = &handshake->payload;
1611
-		size_t payload_len = tls_uint24 ( &handshake->length );
1612
-		const void *next = ( payload + payload_len );
1624
+		const void *payload;
1625
+		size_t payload_len;
1626
+		size_t record_len;
1613 1627
 
1614
-		/* Sanity check */
1615
-		if ( next > end ) {
1628
+		/* Parse header */
1629
+		if ( sizeof ( *handshake ) > remaining ) {
1630
+			DBGC ( tls, "TLS %p received underlength Handshake\n",
1631
+			       tls );
1632
+			DBGC_HD ( tls, data, remaining );
1633
+			return -EINVAL_HANDSHAKE;
1634
+		}
1635
+		payload_len = tls_uint24 ( &handshake->length );
1636
+		if ( payload_len > ( remaining - sizeof ( *handshake ) ) ) {
1616 1637
 			DBGC ( tls, "TLS %p received overlength Handshake\n",
1617 1638
 			       tls );
1618 1639
 			DBGC_HD ( tls, data, len );
1619 1640
 			return -EINVAL_HANDSHAKE;
1620 1641
 		}
1642
+		payload = &handshake->payload;
1643
+		record_len = ( sizeof ( *handshake ) + payload_len );
1621 1644
 
1645
+		/* Handle payload */
1622 1646
 		switch ( handshake->type ) {
1623 1647
 		case TLS_SERVER_HELLO:
1624 1648
 			rc = tls_new_server_hello ( tls, payload, payload_len );
@@ -1648,16 +1672,15 @@ static int tls_new_handshake ( struct tls_session *tls,
1648 1672
 		 * which are explicitly excluded).
1649 1673
 		 */
1650 1674
 		if ( handshake->type != TLS_HELLO_REQUEST )
1651
-			tls_add_handshake ( tls, data,
1652
-					    sizeof ( *handshake ) +
1653
-					    payload_len );
1675
+			tls_add_handshake ( tls, data, record_len );
1654 1676
 
1655 1677
 		/* Abort on failure */
1656 1678
 		if ( rc != 0 )
1657 1679
 			return rc;
1658 1680
 
1659 1681
 		/* Move to next handshake record */
1660
-		data = next;
1682
+		data += record_len;
1683
+		remaining -= record_len;
1661 1684
 	}
1662 1685
 
1663 1686
 	return 0;

Loading…
Cancel
Save