Re: [PATCH 3/4] tls: DHE_RSA key exchange implementation server side

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2069 bytes --]

Hi Andrew,

On 01/09/2019 04:43 AM, Andrew Zaborowski wrote:
> Add the handler for the ClientKeyExchange message and a builder callback
> for the ServerKeyExchange message for DHE_RSA.
> ---
>   ell/tls-suites.c | 187 +++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 187 insertions(+)

<snip>

> +
> +	params->server_dh_params_buf = ptr;

This part seems a bit hacky..

> +
> +	/* RFC 5246, Section 7.4.3 */
> +
> +	l_put_be16(params->prime_len, ptr);
> +	memcpy(ptr + 2, prime_buf, params->prime_len);
> +	ptr += 2 + params->prime_len;
> +
> +	l_put_be16(1, ptr);
> +	memcpy(ptr + 2, &generator_buf, 1);
> +	ptr += 2 + 1;
> +
> +	l_put_be16(public_len - zeros, ptr);
> +	memcpy(ptr + 2, public_buf + zeros, public_len - zeros);
> +	ptr += 2 + public_len - zeros;
> +
> +	params->server_dh_params_len = ptr - params->server_dh_params_buf;
> +	tls->pending.key_xchg_params = params;
> +
> +	sign_len = tls->pending.cipher_suite->key_xchg->sign(tls, ptr,
> +					buf + sizeof(buf) - ptr,
> +					tls_get_server_dh_params_hash);

The way you did it in ecdh seems to be much cleaner.  Perhaps the 
sign/verify operation should be receiving the data to be hashed directly 
(maybe as an iovec), instead of having get_hash function lookup the hash 
id, create the checksum, run it, check for errors, etc).  You are 
already duplicating these steps in tls_get_server_ecdh_params_hash and 
tls_get_server_dh_params_hash.  Admittedly, tls_get_prev_digest_by_id is 
a bit of a special case.

At the very least can we zero out the server_dh_params & 
server_dh_params_len after you're done here (and similarly in patch 2)?

> +	if (sign_len < 0)
> +		return false;
> +
> +	ptr += sign_len;
> +	tls_tx_handshake(tls, TLS_SERVER_KEY_EXCHANGE, buf, ptr - buf);
> +	return true;
> +
> +free_params:
> +	l_key_free(params->prime);
> +	l_key_free(params->generator);
> +	l_key_free(params->private);
> +	l_free(params);
> +	return false;
> +}
> +

Anyway, I applied all 4 of these.

Regards,
-Denis