[gnutls-devel] GnuTLS | GOST key exchange support (!1097)

Sat Nov 2 21:48:08 CET 2019

Merge request https://gitlab.com/gnutls/gnutls/merge_requests/1097 was reviewed by Nikos Mavrogiannopoulos

--

Nikos Mavrogiannopoulos started a new discussion on lib/nettle/pk.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245801

> +		}
> +
> +		if (nonce == NULL)

There seems to be a leak on this point. `ecc_pub` and `ecc_priv`.

--

Nikos Mavrogiannopoulos started a new discussion on lib/nettle/gost_keywrap.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245802

> +		gnutls_assert();
> +		_gnutls_free_temp_key_datum(cek);
> +		return GNUTLS_E_INTERNAL_ERROR;

Is it really an internal error, or something on the paramters?

--

Nikos Mavrogiannopoulos started a new discussion on lib/vko.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245803

> +	}
> +
> +	ret = gnutls_hash_init(&dig, digalg);

If performance could be an issue here, `_gnutls_hash_fast` may be more suitable.

--

Nikos Mavrogiannopoulos started a new discussion on lib/vko.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245805

> +
> +	ret = _gnutls_x509_write_value(kx, "sessionEncryptedKey.encryptedKey", &enc);
> +	if (ret != ASN1_SUCCESS) {

`ret < 0`

--

Nikos Mavrogiannopoulos started a new discussion on lib/vko.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245806

> +
> +	ret = _gnutls_x509_write_value(kx, "sessionEncryptedKey.maskKey", &zero_data);
> +	if (ret != ASN1_SUCCESS) {

`ret < 0` as well

--

Nikos Mavrogiannopoulos started a new discussion on lib/vko.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245807

> +	}
> +	ret = _gnutls_x509_write_value(kx, "sessionEncryptedKey.macKey", &imit);
> +	if (ret != ASN1_SUCCESS) {

same here

--

Nikos Mavrogiannopoulos started a new discussion on lib/vko.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245808

> +	if (ret != ASN1_SUCCESS) {
> +		gnutls_assert();
> +		ret = _gnutls_asn2err(ret);

that's not necessary; maybe use different variable names for asn1 return values to easier distinguish them.

--

Nikos Mavrogiannopoulos started a new discussion on lib/vko.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245809

> +/* Returns 1 if decode used ephemeral key */
> +int
> +_gnutls_gost_keytrans_decrypt(gnutls_pk_params_st *pub,

Would you like to add some description on the key transport, either in the commit message or this file? Some high level info on how it supposed to work and the rfc/draft section it is specified on. While I can read what it does not seem easy to go back to the specification to check what is it expected.

--

Nikos Mavrogiannopoulos started a new discussion on lib/vko.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245810

> +		gnutls_assert();
> +		_gnutls_free_datum(&ukm2);
> +		ret = GNUTLS_E_ASN1_DER_ERROR;

Why is it a der error here?

--

Nikos Mavrogiannopoulos started a new discussion on lib/algorithms/groups.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245811

> +#ifdef ENABLE_GOST
> +	{
> +	 .name = "GC256A",

Should we link the rfc defining the numbers?

--

Nikos Mavrogiannopoulos started a new discussion on lib/algorithms.h: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245813

>  	unsigned gost_curve;
>  	bool supported;
> +	gnutls_group_t group;

Is that sufficient? There may be several places where this mapping is used in a direct way. Shouldn't we replace them to follow that new mapping?

--

Nikos Mavrogiannopoulos started a new discussion on lib/algorithms/ecc.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245814

> + * Returns: the group associated with the named curve or %GNUTLS_GROUP_INVALID.
> + *
> + * Since: 3.6.9

3.6.10

--

Nikos Mavrogiannopoulos started a new discussion on lib/algorithms/ecc.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245815

> + * Since: 3.6.9
> + */
> +gnutls_group_t _gnutls_ecc_curve_get_group(gnutls_ecc_curve_t curve)

There is an underscore in the name here, however it is not in the description.

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245816

> +calc_ukm(gnutls_session_t session, gnutls_datum_t *ukm)
> +{
> +	gnutls_digest_algorithm_t digalg = GNUTLS_DIG_STREEBOG_256;

Looks like a const would be suitable.

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245817

> +
> +static int
> +calc_ukm(gnutls_session_t session, gnutls_datum_t *ukm)

Given that the output size is fixed here to 8 bytes would it make sense to avoid the malloc here?

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245818

> +	ret = asn1_get_length_der(&data[i], data_size, &len);
> +	if (ret < 0)
> +		return gnutls_assert_val(_gnutls_asn2err(ret));

`ret` here cannot be converted to error. It is not a standard libtasn1 error.

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245819

> +	i += 1;
> +
> +	ret = asn1_get_length_der(&data[i], data_size, &len);

I wish I could unsee that asn.1/tls mix!

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245820

> +		return gnutls_assert_val(ret);
> +
> +	if (!privkey || privkey->type != GNUTLS_PRIVKEY_X509) {

Wouldn;t it be simpler if it was earlier (simpler in the sense that no cleanup is necessary)

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245821

> +	}
> +
> +	ret = _gnutls_gost_keytrans_decrypt(has_pcert ? &peer_cert.pubkey->params : NULL,

I think having two separate code paths here would make it much easier to read. I.e., without `has_pcert` variable.

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245822

> +
> +	if (ret == 0)
> +		session->internals.hsk_flags &= ~HSK_CRT_VRFY_EXPECTED;

Is that necessary? It is not done by any other auth.

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245823

> +	uint8_t priv_buf[512/8];
> +	char buf[512 / 4 + 1];
> +

Given that this is about about printing would it make sense to make it conditional on the same condition as the hard_log function so that it returns early?

--

Nikos Mavrogiannopoulos started a new discussion on lib/auth/vko_gost.c: https://gitlab.com/gnutls/gnutls/merge_requests/1097#note_239245824

> +		has_pcert = 1;
> +	}
> +

Are there decryption oracles we should worry about? I.e., should that be constant time/memory access?

-- 
Reply to this email directly or view it on GitLab: https://gitlab.com/gnutls/gnutls/merge_requests/1097
You're receiving this email because of your account on gitlab.com.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.gnupg.org/pipermail/gnutls-devel/attachments/20191102/274c22af/attachment-0001.html>