Re: [PATCH v5 07/10] hornet: Introduce gen_sig

[sashiko-bot@kernel.org]

Sashiko AI review found 5 potential issue(s):
- [High] Infinite loop during argument parsing on architectures with unsigned char.
- [Medium] Ignored return value of `sha256()` causes use of uninitialized memory.
- [Medium] Stack buffer overflow when parsing '--add' arguments.
- [Medium] Missing NULL pointer checks for OpenSSL allocations.
- [Medium] Unchecked return value of `ASN1_item_i2d()`.
--

commit 72bf19e54c723e2a7111cbc219bbeb6e98977318
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>

hornet: Introduce gen_sig

This commit introduces the gen_sig tool. It creates a pkcs#7 signature of a
data payload and appends a signed attribute containing a set of hashes.

> diff --git a/scripts/hornet/gen_sig.c b/scripts/hornet/gen_sig.c
> index 0000000000000..f966516ebc99b
> --- /dev/null
> +++ b/scripts/hornet/gen_sig.c

[ ... ]

> +static void add_hash(MAP_SET *set, unsigned char *buffer, int buffer_len, int index)
> +{
> +	HORNET_MAP *map = NULL;
> +
> +	map = HORNET_MAP_new();
> +	ASN1_INTEGER_set(map->index, index);

map is allocated via HORNET_MAP_new(), but the return value is not checked
for NULL before dereferencing it in ASN1_INTEGER_set(). Should we handle
potential allocation failures here?

[ ... ]

> +int main(int argc, char **argv)
> +{

[ ... ]

> +	int i;
> +	char opt;
> +
> +	const char *short_opts = "C:K:P:O:A:Sh";
> +
> +	static const struct option long_opts[] = {
> +		{"cert", required_argument, 0, 'C'},
> +		{"key",  required_argument, 0, 'K'},
> +		{"pass",  required_argument, 0, 'P'},
> +		{"out",  required_argument, 0, 'O'},
> +		{"data",  required_argument, 0, 'D'},
> +		{"add",  required_argument, 0, 'A'},
> +		{"help",    no_argument,       0, 'h'},
> +		{0, 0, 0, 0}
> +	};
> +
> +	while ((opt = getopt_long_only(argc, argv, short_opts, long_opts, NULL)) != -1) {

getopt_long_only() returns an int. If it returns -1, and opt is a char,
architectures where char is unsigned by default (like ARM or PowerPC) will
evaluate opt as 255.

Since 255 != -1, will this cause an infinite loop during argument parsing on
those platforms?

> +		switch (opt) {
> +		case 'C':
> +			cert_path = optarg;
> +			break;

[ ... ]

> +		case 'A':
> +			if (strchr(optarg, ':')) {
> +				hashes[hash_count].file = strsep(&optarg, ":");
> +				hashes[hash_count].index = atoi(optarg);
> +				hash_count++;

The hashes array is defined with a fixed size of MAX_HASHES. Does this code
need to check if hash_count is less than MAX_HASHES before writing to the
array to prevent a stack buffer overflow if too many arguments are provided?

[ ... ]

> +	si = CMS_add1_signer(cms_out, signer, pkey, EVP_sha256(), flags);
> +	ERR(!si, "add signer failed");
> +
> +	set = MAP_SET_new();
> +	set->maps = sk_HORNET_MAP_new_null();

MAP_SET_new() and sk_HORNET_MAP_new_null() can return NULL if memory
allocation fails. Since set->maps dereferences set immediately, can this
result in a segmentation fault under memory pressure?

> +
> +	for (i = 0; i < hash_count; i++) {
> +		sha256(hashes[i].file, hash_buffer, &hash_len);
> +		add_hash(set, hash_buffer, hash_len, hashes[i].index);
> +	}

The return value of sha256() is ignored here. If the file cannot be opened,
sha256() will fail, leaving hash_buffer and hash_len uninitialized.

Will this lead to an out-of-bounds read of uninitialized stack memory when
passed to add_hash()?

> +
> +	oid = OBJ_txt2obj("2.25.316487325684022475439036912669789383960", 1);
> +	if (!oid) {
> +		ERR_print_errors_fp(stderr);
> +		DIE("create oid failed");
> +	}
> +
> +	der_len = ASN1_item_i2d((ASN1_VALUE *)set, &der, ASN1_ITEM_rptr(MAP_SET));
> +	CMS_signed_add1_attr_by_OBJ(si, oid, V_ASN1_SEQUENCE, der, der_len);

ASN1_item_i2d() returns a negative value on failure. Since der_len is passed
directly to CMS_signed_add1_attr_by_OBJ() without checking for errors, could
this cause issues by passing a negative length to the OpenSSL routine?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420212653.438685-1-bboscaccy@linux.microsoft.com?part=7