From: Theo Buehler <tb@theobuehler.org>
Subject: Re: [PATCH] Fix memory leaks related to ML-KEM
To: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
Cc: tech@openbsd.org, beck@openbsd.org, jsing@openbsd.org
Date: Tue, 17 Dec 2024 08:28:28 +0100

On Tue, Dec 17, 2024 at 03:40:26PM +0900, Kenjiro Nakayama wrote:
> This patch resolves memory leaks and ensures proper resource cleanup:

Thanks

> - Call CBB_cleanup() to avoid leaks after initializing CBB.

These two are quite nasty. There are allocations in a void function.
So I added an XXX for error checking before the CBB_init_fixed().

Also, the _external_entropy() has become _external_seed() in more recent
versions of the upstream code. Did BoringSSL's switch to C++ also
involve some paradigm shift towards allocations can't fail?

> - Remove unnecessary "break" before MlkemKeygenFileTest in tests.

This seems to be a leftover from attempts at debugging an earlier
version. Also, a CBS holding a pointer to free is really not great.
Would be nice to fix this.

> - Add missing fclose() calls to release file resources in tests.

I took these, although this comes right before exit. It is weird to free
one thing but not clean up the other.

> 
> Signed-off-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
> ---
>  src/lib/libcrypto/mlkem/mlkem1024.c                           | 1 +
>  src/lib/libcrypto/mlkem/mlkem768.c                            | 1 +
>  src/regress/lib/libcrypto/mlkem/mlkem1024_keygen_tests.c      | 2 +-
>  src/regress/lib/libcrypto/mlkem/mlkem1024_nist_keygen_tests.c | 1 +
>  src/regress/lib/libcrypto/mlkem/mlkem768_keygen_tests.c       | 2 +-
>  src/regress/lib/libcrypto/mlkem/mlkem768_nist_keygen_tests.c  | 1 +
>  6 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git src/lib/libcrypto/mlkem/mlkem1024.c src/lib/libcrypto/mlkem/mlkem1024.c
> index e0a71f335..813d84f88 100644
> --- src/lib/libcrypto/mlkem/mlkem1024.c
> +++ src/lib/libcrypto/mlkem/mlkem1024.c
> @@ -878,6 +878,7 @@ MLKEM1024_generate_key_external_entropy(
>  	if (!mlkem_marshal_public_key(&cbb, &priv->pub)) {
>  		abort();
>  	}
> +	CBB_cleanup(&cbb);
>  
>  	hash_h(priv->pub.public_key_hash, out_encoded_public_key,
>  	    MLKEM1024_PUBLIC_KEY_BYTES);
> diff --git src/lib/libcrypto/mlkem/mlkem768.c src/lib/libcrypto/mlkem/mlkem768.c
> index fed4704f8..7ab48fab6 100644
> --- src/lib/libcrypto/mlkem/mlkem768.c
> +++ src/lib/libcrypto/mlkem/mlkem768.c
> @@ -877,6 +877,7 @@ MLKEM768_generate_key_external_entropy(
>  	if (!mlkem_marshal_public_key(&cbb, &priv->pub)) {
>  		abort();
>  	}
> +	CBB_cleanup(&cbb);
>  
>  	hash_h(priv->pub.public_key_hash, out_encoded_public_key,
>  	    MLKEM768_PUBLIC_KEY_BYTES);
> diff --git src/regress/lib/libcrypto/mlkem/mlkem1024_keygen_tests.c src/regress/lib/libcrypto/mlkem/mlkem1024_keygen_tests.c
> index d27d5aacf..e1a00c5b8 100644
> --- src/regress/lib/libcrypto/mlkem/mlkem1024_keygen_tests.c
> +++ src/regress/lib/libcrypto/mlkem/mlkem1024_keygen_tests.c
> @@ -112,7 +112,6 @@ main(int argc, char **argv)
>  				break;
>  			grab_data(&private_key, buf, strlen("private_key: "));
>  			state = S_START;
> -			break;
>  
>  			MlkemKeygenFileTest(&seed, &public_key, &private_key);
>  			free((void *)CBS_data(&seed));
> @@ -126,5 +125,6 @@ main(int argc, char **argv)
>  	}
>  
>  	free(buf);
> +	fclose(fp);
>  	exit(failure);
>  }
> diff --git src/regress/lib/libcrypto/mlkem/mlkem1024_nist_keygen_tests.c src/regress/lib/libcrypto/mlkem/mlkem1024_nist_keygen_tests.c
> index b9600dc6b..effa1f734 100644
> --- src/regress/lib/libcrypto/mlkem/mlkem1024_nist_keygen_tests.c
> +++ src/regress/lib/libcrypto/mlkem/mlkem1024_nist_keygen_tests.c
> @@ -133,5 +133,6 @@ main(int argc, char **argv)
>  	}
>  
>  	free(buf);
> +	fclose(fp);
>  	exit(failure);
>  }
> diff --git src/regress/lib/libcrypto/mlkem/mlkem768_keygen_tests.c src/regress/lib/libcrypto/mlkem/mlkem768_keygen_tests.c
> index b54b8c53c..f00d009fa 100644
> --- src/regress/lib/libcrypto/mlkem/mlkem768_keygen_tests.c
> +++ src/regress/lib/libcrypto/mlkem/mlkem768_keygen_tests.c
> @@ -112,7 +112,6 @@ main(int argc, char **argv)
>  				break;
>  			grab_data(&private_key, buf, strlen("private_key: "));
>  			state = S_START;
> -			break;
>  
>  			MlkemKeygenFileTest(&seed, &public_key, &private_key);
>  			free((void *)CBS_data(&seed));
> @@ -126,5 +125,6 @@ main(int argc, char **argv)
>  	}
>  
>  	free(buf);
> +	fclose(fp);
>  	exit(failure);
>  }
> diff --git src/regress/lib/libcrypto/mlkem/mlkem768_nist_keygen_tests.c src/regress/lib/libcrypto/mlkem/mlkem768_nist_keygen_tests.c
> index ee71aa262..8b87afefc 100644
> --- src/regress/lib/libcrypto/mlkem/mlkem768_nist_keygen_tests.c
> +++ src/regress/lib/libcrypto/mlkem/mlkem768_nist_keygen_tests.c
> @@ -133,5 +133,6 @@ main(int argc, char **argv)
>  	}
>  
>  	free(buf);
> +	fclose(fp);
>  	exit(failure);
>  }
> -- 
> 2.39.5 (Apple Git-154)
>