[gnutls-devel] GnuTLS | support non-NULL-terminated PSKs (!917)

[Development of GNU's TLS library]

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

--
  
Nikos Mavrogiannopoulos started a new discussion on lib/auth/psk.h: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626720

>  
> +inline static
> +void _gnutls_copy_psk_auth_info(psk_auth_info_t info, const gnutls_datum_t *username)

what about naming it `_gnutls_copy_psk_username`? Reading the name made me think that this copies something more than the username.

--
  
Nikos Mavrogiannopoulos commented on a discussion on lib/auth/psk.h: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626724

> -	gnutls_psk_server_credentials_function *pwd_callback;
> +	union {
> +		gnutls_psk_server_credentials_function *cb1;

@juaristi did you see this one?

--
  
Nikos Mavrogiannopoulos started a new discussion on lib/auth/psk_passwd.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626728

> +			return gnutls_assert_val(retval);
> +
> +		retval = memcmp(username->data, hex_username.data, username->size);

Shouldn't we compare the sizes before the memcmp for equality?

--
  
Nikos Mavrogiannopoulos started a new discussion on lib/ext/pre_shared_key.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626730

>  			 * return its error code in that case */
> -			ret = _gnutls_psk_pwd_find_entry(session, identity_str, &key);
> +			ret = _gnutls_psk_pwd_find_entry(session, (const char *) psk.identity.data, psk.identity.size, &key);

is the `const char*` cast necessary?

--
  
Nikos Mavrogiannopoulos started a new discussion on lib/psk.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626736

> +
> +/**
> + * gnutls_psk_set_server_credentials_function:

typo: 2 is missing here

--
  
Nikos Mavrogiannopoulos started a new discussion on lib/psk.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626738

>  
> +/**
> + * gnutls_psk_set_client_credentials_function:

typo: 2 is missing

--
  
Nikos Mavrogiannopoulos started a new discussion on lib/psk.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626742

> +username_has_embedded_nulls(psk_auth_info_t info)
> +{
> +	for (uint16_t i = 0; i < info->len; i++) {

There is also the `has_embedded_null` macro which could be used here.

--
  
Nikos Mavrogiannopoulos started a new discussion on lib/psk.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626745

> +
> +	if (info->username[0] != 0 && info->len > 0)
> +		return _gnutls_set_datum(out, info->username, info->len);

Would it make sense to provide the pointer to the username instead of allocating memory? This will be more in par with the original function this replaces.

An example of a function that works like that with datums is `gnutls_session_get_random`.

--
  
Nikos Mavrogiannopoulos started a new discussion on lib/psk.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626747

> +		return GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE;
> +
> +	if (info->username[0] != 0 && info->len > 0)

What if the username is '\x00'?

--
  
Nikos Mavrogiannopoulos started a new discussion on tests/psk.passwd: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626748

>  jas:9e32cf7786321a828ef7668f09fb35db
>  non-hex:9e32cf7786321a828ef7668f09fb35dbxx
> + at deadbeef:9e32cf7786321a828ef7668f09fb35db

+1

I'd also add `@00` and `@0000aa00`, to ensure that embedded nulls work well.

--
  
Nikos Mavrogiannopoulos started a new discussion on tests/pskself2.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626751

> +
> +	gnutls_psk_allocate_client_credentials(&pskcred);
> +	gnutls_psk_set_client_credentials2(pskcred, &user, &key,

we should check the error code here, to ensure this is succeeding as expected.

--
  
Nikos Mavrogiannopoulos started a new discussion on tests/pskself2.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626754

> +static gnutls_dh_params_t dh_params;
> +
> +static int generate_dh_params(void)

The DH parameters are not necessary in new tests

--
  
Nikos Mavrogiannopoulos started a new discussion on tests/pskself2.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626757

> +		success("server: Handshake was completed\n");
> +
> +		if (gnutls_psk_server_get_username(session))

+1

--
  
Nikos Mavrogiannopoulos started a new discussion on tests/pskself2.c: https://gitlab.com/gnutls/gnutls/merge_requests/917#note_165626758

> +	generate_dh_params();
> +
> +	run_test("NORMAL:-VERS-ALL:+VERS-TLS1.2:-KX-ALL:+PSK", 1);

We most likely do not need to test all combinations here. TLS1.2 and TLS1.3 should be the minimum. An idea could be however, to pass the username here on `run_test` so that multiple names can be tested.


-- 
Reply to this email directly or view it on GitLab: https://gitlab.com/gnutls/gnutls/merge_requests/917
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/20190430/d4891439/attachment-0001.html>