[gPXE-devel] [Patch] Fix tls handshake handling in gPXE

[Stefan Hajnoczi]

> From dcd0803f05a6c5e933da668d5ff48c7d248d9507 Mon Sep 17 00:00:00 2001
> From: tstack <tstack at tstack-dev2.(none)>

Oops, looks like the author git configuration option hasn't been configured.
Also, please sign off this patch by adding your name/email with a
Signed-off-by: line.

(See http://www.kernel.org/doc/Documentation/SubmittingPatches Section
12 "Sign your work" for details.)

> Date: Tue, 22 Jun 2010 10:22:29 -0700
> Subject: [PATCH] Fix tls handshake handling in gPXE
>
> The handshake record in TLS can contain multiple messages, however,
> gPXE was only expecting one message.  This seems to work fine with
> openssl, but the java ssl code seems to send multiple message blocks
> in a single record.  So, we need to iterate through them.
>
> For reference: http://en.wikipedia.org/wiki/Transport_Layer_Security
> ---
>  src/include/gpxe/tls.h |    7 ++++
>  src/net/tls.c          |   90 ++++++++++++++++++++++++++---------------------
>  2 files changed, 57 insertions(+), 40 deletions(-)
>
> diff --git a/src/include/gpxe/tls.h b/src/include/gpxe/tls.h
> index e2da046..5126bea 100644
> --- a/src/include/gpxe/tls.h
> +++ b/src/include/gpxe/tls.h
> @@ -52,6 +52,13 @@ struct tls_header {
>  /** Application data content type */
>  #define TLS_TYPE_DATA 23
>
> +/** Handshake message type */
> +struct tls_handshake_msg {
> +	uint8_t type;
> +	uint8_t length[3];
> +	uint8_t payload[0];
> +} __attribute__ (( packed ));
> +
>  /* Handshake message types */
>  #define TLS_HELLO_REQUEST 0
>  #define TLS_CLIENT_HELLO 1
> diff --git a/src/net/tls.c b/src/net/tls.c
> index a5b126e..ecc863f 100644
> --- a/src/net/tls.c
> +++ b/src/net/tls.c
> @@ -964,49 +964,59 @@ static int tls_new_finished ( struct tls_session *tls,
>   */
>  static int tls_new_handshake ( struct tls_session *tls,
>  			       void *data, size_t len ) {
> -	struct {
> -		uint8_t type;
> -		uint8_t length[3];
> -		uint8_t payload[0];
> -	} __attribute__ (( packed )) *handshake = data;
> -	void *payload = &handshake->payload;
> -	size_t payload_len = tls_uint24 ( handshake->length );
> -	void *end = ( payload + payload_len );
> -	int rc;
> +	void *data_end = ( data + len );
> +	int rc = 0;
> +
> +	while ( data < data_end ) {
> +		struct tls_handshake_msg *handshake = data;
> +		void *payload = &handshake->payload;
> +		size_t payload_len = tls_uint24 ( handshake->length );
> +
> +		DBGC ( tls, "TLS %p received Handshake %d %p %d\n",
> +		       tls,
> +		       handshake->type,
> +		       payload,
> +		       payload_len );

payload_len is size_t so its format specifier needs to be "%zu" (size_t
unsigned).  A similar issue recently came up and it results in a warning when
building in 64-bit mode.

At this point a sanity check is needed to ensure that payload + payload_len
(user input) is at most data_end.  Otherwise the functions called below would
try to use bogus/malicious payload_len values.

> +		
> +		switch ( handshake->type ) {
> +		case TLS_SERVER_HELLO:
> +			rc = tls_new_server_hello ( tls, payload, payload_len );
> +			break;
> +		case TLS_CERTIFICATE:
> +			rc = tls_new_certificate ( tls, payload, payload_len );
> +			break;
> +		case TLS_SERVER_HELLO_DONE:
> +			rc = tls_new_server_hello_done ( tls, payload, payload_len );
> +			break;
> +		case TLS_FINISHED:
> +			rc = tls_new_finished ( tls, payload, payload_len );
> +			break;
> +		default:
> +			DBGC ( tls, "TLS %p ignoring handshake type %d\n",
> +			       tls, handshake->type );
> +			rc = 0;
> +			break;
> +		}
> +		
> +		/* Add to handshake digest (except for Hello Requests, which
> +		 * are explicitly excluded).
> +		 */
> +		if ( handshake->type != TLS_HELLO_REQUEST )
> +			tls_add_handshake ( tls, data,
> +					    sizeof ( *handshake ) +
> +					    payload_len );
>
> -	/* Sanity check */
> -	if ( end != ( data + len ) ) {
> -		DBGC ( tls, "TLS %p received overlength Handshake\n", tls );
> -		DBGC_HD ( tls, data, len );
> -		return -EINVAL;
> +		data += sizeof ( *handshake ) + payload_len;
>  	}
>
> -	switch ( handshake->type ) {
> -	case TLS_SERVER_HELLO:
> -		rc = tls_new_server_hello ( tls, payload, payload_len );
> -		break;
> -	case TLS_CERTIFICATE:
> -		rc = tls_new_certificate ( tls, payload, payload_len );
> -		break;
> -	case TLS_SERVER_HELLO_DONE:
> -		rc = tls_new_server_hello_done ( tls, payload, payload_len );
> -		break;
> -	case TLS_FINISHED:
> -		rc = tls_new_finished ( tls, payload, payload_len );
> -		break;
> -	default:
> -		DBGC ( tls, "TLS %p ignoring handshake type %d\n",
> -		       tls, handshake->type );
> -		rc = 0;
> -		break;
> +	if ( data != data_end ) {
> +		DBGC ( tls, "TLS %p received overlength Handshake %p %p\n",
> +		       tls,
> +		       data,
> +		       data_end );
> +		DBGC_HD ( tls, data, len );
>  	}

This check is no longer needed here (see comment above).

> -
> -	/* Add to handshake digest (except for Hello Requests, which
> -	 * are explicitly excluded).
> -	 */
> -	if ( handshake->type != TLS_HELLO_REQUEST )
> -		tls_add_handshake ( tls, data, len );
> -
> +	
>  	return rc;
>  }
>
> --
> 1.6.3.3
>

Stefan