[gnutls-devel] GnuTLS | added clientHello extension permutation (!1737)

[Read-only notification of GnuTLS library development activities]

Merge request https://gitlab.com/gnutls/gnutls/-/merge_requests/1737 was reviewed by Daiki Ueno

--
  
Daiki Ueno started a new discussion on lib/hello_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1737#note_1337594311

>  #include "intprops.h"
>  
> +#include <gnutls/gnutls.h>

This shouldn't be necessary; for library source files `<gnutls/gnutls.h>` is included through `"gnutls_int.h"`.

--
  
Daiki Ueno started a new discussion on lib/hello_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1737#note_1337594312

> +
> +	/* genrating random permute */
> +	gnutls_global_init();

We shouldn't call `gnutls_global_init` (and `_deinit`) in the library code; it should be implicitly called through ELF [constructor](https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Common-Function-Attributes.html#index-constructor-function-attribute).

--
  
Daiki Ueno started a new discussion on lib/hello_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1737#note_1337594313

> +				     sizeof(uint32_t));
> +		if (ret < 0)
> +			return -1;

Let's just return `ret` (which is be one of the `GNUTLS_E_*` values and represents the reason of the failure).

--
  
Daiki Ueno started a new discussion on lib/hello_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1737#note_1337594315

> +	}
> +
> +	/* genrating random permute */

Typo: `genrating` → `generating`. Also I'd suggest `permute` → `permutation`.

--
  
Daiki Ueno started a new discussion on lib/hello_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1737#note_1337594318

> +	for (uint32_t i = sz - 1; i > 0; i--) {
> +		int ret = gnutls_rnd(GNUTLS_RND_RANDOM, (void *)&rnd_n,
> +				     sizeof(uint32_t));

We could reduce the size of random value, as we are sending at most 64 extensions at a time.

--
  
Daiki Ueno started a new discussion on lib/hello_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1737#note_1337594320

> +	/* To shuffle extension sending order */
> +	uint32_t shuffled_n[MAX_EXT_TYPES];
> +	ret = shuffle_arr(shuffled_n, MAX_EXT_TYPES);

As mentioned in the issue description, `pre_shared_key` exntesion (and maybe `padding`) must always be processed at last, because it needs to [calculate](https://www.rfc-editor.org/rfc/rfc8446#section-4.2.11) the hash of ClientHello content for binders.

--
  
Daiki Ueno started a new discussion on lib/hello_ext.c: https://gitlab.com/gnutls/gnutls/-/merge_requests/1737#note_1337594321

> +		uint32_t temp = arr[i];
> +		arr[i] = arr[j];
> +		arr[j] = temp;

This algorithm seems to be equivalent to the modern version of the Fisher–Yates shuffle as in [Wikipedia](https://en.wikipedia.org/wiki/Fisher%E2%80%93Yates_shuffle); kudos you ended up with it :-)


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