Browse Source

[tls] Do not access beyond the end of a 24-bit integer

The current implementation handles big-endian 24-bit integers (which
occur in several TLS record types) by treating them as big-endian
32-bit integers which are shifted by 8 bits.  This can result in
"Invalid read" errors when running under valgrind, if the 24-bit field
happens to be exactly at the end of an I/O buffer.

Fix by ensuring that we touch only the three bytes which comprise the
24-bit integer.

Signed-off-by: Michael Brown <mcb30@ipxe.org>
tags/v1.20.1
Michael Brown 8 years ago
parent
commit
1ac7434111
1 changed files with 29 additions and 22 deletions
  1. 29
    22
      src/net/tls.c

+ 29
- 22
src/net/tls.c View File

@@ -179,20 +179,29 @@ static void tls_clear_cipher ( struct tls_session *tls,
179 179
  ******************************************************************************
180 180
  */
181 181
 
182
+/** A TLS 24-bit integer
183
+ *
184
+ * TLS uses 24-bit integers in several places, which are awkward to
185
+ * parse in C.
186
+ */
187
+typedef struct {
188
+	/** High byte */
189
+	uint8_t high;
190
+	/** Low word */
191
+	uint16_t low;
192
+} __attribute__ (( packed )) tls24_t;
193
+
182 194
 /**
183 195
  * Extract 24-bit field value
184 196
  *
185 197
  * @v field24		24-bit field
186 198
  * @ret value		Field value
187 199
  *
188
- * TLS uses 24-bit integers in several places, which are awkward to
189
- * parse in C.
190 200
  */
191 201
 static inline __attribute__ (( always_inline )) unsigned long
192
-tls_uint24 ( const uint8_t field24[3] ) {
193
-	const uint32_t *field32 __attribute__ (( may_alias )) =
194
-		( ( const void * ) field24 );
195
-	return ( be32_to_cpu ( *field32 ) >> 8 );
202
+tls_uint24 ( const tls24_t *field24 ) {
203
+
204
+	return ( ( field24->high << 16 ) | be16_to_cpu ( field24->low ) );
196 205
 }
197 206
 
198 207
 /**
@@ -200,13 +209,11 @@ tls_uint24 ( const uint8_t field24[3] ) {
200 209
  *
201 210
  * @v field24		24-bit field
202 211
  * @v value		Field value
203
- *
204
- * The field must be pre-zeroed.
205 212
  */
206
-static void tls_set_uint24 ( uint8_t field24[3], unsigned long value ) {
207
-	uint32_t *field32 __attribute__ (( may_alias )) =
208
-		( ( void * ) field24 );
209
-	*field32 |= cpu_to_be32 ( value << 8 );
213
+static void tls_set_uint24 ( tls24_t *field24, unsigned long value ) {
214
+
215
+	field24->high = ( value >> 16 );
216
+	field24->low = cpu_to_be16 ( value );
210 217
 }
211 218
 
212 219
 /**
@@ -1038,9 +1045,9 @@ static int tls_send_client_hello ( struct tls_session *tls ) {
1038 1045
 static int tls_send_certificate ( struct tls_session *tls ) {
1039 1046
 	struct {
1040 1047
 		uint32_t type_length;
1041
-		uint8_t length[3];
1048
+		tls24_t length;
1042 1049
 		struct {
1043
-			uint8_t length[3];
1050
+			tls24_t length;
1044 1051
 			uint8_t data[ tls->cert->raw.len ];
1045 1052
 		} __attribute__ (( packed )) certificates[1];
1046 1053
 	} __attribute__ (( packed )) *certificate;
@@ -1058,9 +1065,9 @@ static int tls_send_certificate ( struct tls_session *tls ) {
1058 1065
 		( cpu_to_le32 ( TLS_CERTIFICATE ) |
1059 1066
 		  htonl ( sizeof ( *certificate ) -
1060 1067
 			  sizeof ( certificate->type_length ) ) );
1061
-	tls_set_uint24 ( certificate->length,
1068
+	tls_set_uint24 ( &certificate->length,
1062 1069
 			 sizeof ( certificate->certificates ) );
1063
-	tls_set_uint24 ( certificate->certificates[0].length,
1070
+	tls_set_uint24 ( &certificate->certificates[0].length,
1064 1071
 			 sizeof ( certificate->certificates[0].data ) );
1065 1072
 	memcpy ( certificate->certificates[0].data,
1066 1073
 		 tls->cert->raw.data,
@@ -1412,7 +1419,7 @@ static int tls_parse_chain ( struct tls_session *tls,
1412 1419
 			     const void *data, size_t len ) {
1413 1420
 	const void *end = ( data + len );
1414 1421
 	const struct {
1415
-		uint8_t length[3];
1422
+		tls24_t length;
1416 1423
 		uint8_t data[0];
1417 1424
 	} __attribute__ (( packed )) *certificate;
1418 1425
 	size_t certificate_len;
@@ -1436,7 +1443,7 @@ static int tls_parse_chain ( struct tls_session *tls,
1436 1443
 
1437 1444
 		/* Extract raw certificate data */
1438 1445
 		certificate = data;
1439
-		certificate_len = tls_uint24 ( certificate->length );
1446
+		certificate_len = tls_uint24 ( &certificate->length );
1440 1447
 		next = ( certificate->data + certificate_len );
1441 1448
 		if ( next > end ) {
1442 1449
 			DBGC ( tls, "TLS %p overlength certificate:\n", tls );
@@ -1482,10 +1489,10 @@ static int tls_parse_chain ( struct tls_session *tls,
1482 1489
 static int tls_new_certificate ( struct tls_session *tls,
1483 1490
 				 const void *data, size_t len ) {
1484 1491
 	const struct {
1485
-		uint8_t length[3];
1492
+		tls24_t length;
1486 1493
 		uint8_t certificates[0];
1487 1494
 	} __attribute__ (( packed )) *certificate = data;
1488
-	size_t certificates_len = tls_uint24 ( certificate->length );
1495
+	size_t certificates_len = tls_uint24 ( &certificate->length );
1489 1496
 	const void *end = ( certificate->certificates + certificates_len );
1490 1497
 	int rc;
1491 1498
 
@@ -1634,11 +1641,11 @@ static int tls_new_handshake ( struct tls_session *tls,
1634 1641
 	while ( data != end ) {
1635 1642
 		const struct {
1636 1643
 			uint8_t type;
1637
-			uint8_t length[3];
1644
+			tls24_t length;
1638 1645
 			uint8_t payload[0];
1639 1646
 		} __attribute__ (( packed )) *handshake = data;
1640 1647
 		const void *payload = &handshake->payload;
1641
-		size_t payload_len = tls_uint24 ( handshake->length );
1648
+		size_t payload_len = tls_uint24 ( &handshake->length );
1642 1649
 		const void *next = ( payload + payload_len );
1643 1650
 
1644 1651
 		/* Sanity check */

Loading…
Cancel
Save