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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Missing error checks for memory allocations and OpenSSL APIs can lead to crashes or silently corrupted outputs.
- [Medium] Off-by-one error limits the maximum number of hashes.
- [Low] Command-line option parsing errors due to mismatched `short_opts` and missing switch cases.
- [Low] Premature teardown of global OpenSSL error strings inside a loop.
- [Low] Unquoted variables in `write-sig.sh` can cause word splitting and unexpected file reads.
--

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

hornet: Introduce gen_sig

This introduces the gen_sig tool to create a pkcs#7 signature of a data
payload and append a signed attribute containing a set of hashes.

> diff --git a/scripts/hornet/gen_sig.c b/scripts/hornet/gen_sig.c
> new file mode 100644
> index 0000000000000..4d59b70f5fff4
> --- /dev/null
> +++ b/scripts/hornet/gen_sig.c
[ ... ]
> +static int sha256(const char *path, unsigned char out[SHA256_LEN], unsigned int *out_len)
> +{
[ ... ]
> +	ERR_load_crypto_strings();
> +
> +	rc = -3;
> +	ctx = EVP_MD_CTX_new();
[ ... ]
> +done:
> +	EVP_MD_CTX_free(ctx);
> +	fclose(f);
> +	ERR_free_strings();
> +	return rc;
> +}

Because sha256() is called inside a loop in main(), will calling
ERR_free_strings() here prematurely free the global error string table? This
might cause subsequent OpenSSL errors to only print raw numeric codes instead
of human-readable messages.

> +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);

Should we check if map is NULL before passing it to ASN1_INTEGER_set()? If
HORNET_MAP_new() fails during an out-of-memory condition, this could lead to a
segmentation fault.

[ ... ]
> +int main(int argc, char **argv)
> +{
[ ... ]
> +	const char *short_opts = "C:K:P:O:A:Sh";

This isn't a bug, but short_opts appears to be missing D: for the data
argument. Will this cause the short -D option to be rejected even though it
is defined in long_opts?

> +	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) {
> +		switch (opt) {
[ ... ]
> +		case 'A':
> +			if (strchr(optarg, ':')) {
> +				hashes[hash_count].file = strsep(&optarg, ":");
> +				hashes[hash_count].index = atoi(optarg);
> +				if (++hash_count >= MAX_HASHES) {

Does this condition inadvertently limit the maximum number of hashes to 63
instead of 64? If exactly 64 hashes are provided, hash_count increments to 64,
the condition evaluates to true, and it immediately aborts.

> +					usage(argv[0]);
> +					return EXIT_FAILURE;
> +				}
> +			} else {
> +				usage(argv[0]);
> +				return EXIT_FAILURE;
> +			}
> +			break;
> +		default:
> +			usage(argv[0]);
> +			return EXIT_FAILURE;
> +		}
> +	}

Passing --help maps to 'h', but there is no case for 'h' in the switch
statement. Will this fall through to the default case and incorrectly return
EXIT_FAILURE instead of EXIT_SUCCESS?

[ ... ]
> +	set = MAP_SET_new();
> +	set->maps = sk_HORNET_MAP_new_null();

Could MAP_SET_new() return NULL here, causing a crash when set->maps is
subsequently accessed?

[ ... ]
> +	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() can return a negative value on failure. Could passing this
unchecked der_len into CMS_signed_add1_attr_by_OBJ() cause an out-of-bounds
read or crash?

Additionally, should the return value of CMS_signed_add1_attr_by_OBJ() be
checked for errors?

[ ... ]
> +	err = CMS_final(cms_out, data_in, NULL, CMS_NOCERTS | CMS_BINARY);
> +	ERR(!err, "cms final failed");
> +
> +	OPENSSL_free(der);
> +	MAP_SET_free(set);
> +
> +	b_out = bio_open_wr(out_path);
> +	ERR(!b_out, "opening output path failed");
> +
> +	i2d_CMS_bio_stream(b_out, cms_out, NULL, 0);

Is it possible for i2d_CMS_bio_stream() to fail, for example if there is no
space left on the device? Since the return value is ignored, could the tool
exit successfully while leaving a corrupted or empty signature file?

> +	BIO_free(data_in);
> +	BIO_free(b_out);
> +	EVP_cleanup();
> +	ERR_free_strings();
> +	return 0;
> +}
> diff --git a/scripts/hornet/write-sig.sh b/scripts/hornet/write-sig.sh
> new file mode 100755
> index 0000000000000..7eaabe3bab9aa
> --- /dev/null
> +++ b/scripts/hornet/write-sig.sh
[ ... ]
> +if [ $ARGC -ne $EXPECTED_ARGS ] ; then
> +    usage
> +else
> +    SIG=$(xxd -p $2 | tr -d '\n' | sed 's/\(..\)/\\\\x\1/g')
> +    sed '/const char opts_sig/,/;/c\\tstatic const char opts_sig[] __attribute__((__aligned__(8))) = "\\\n'"$(printf '%s\n' "$SIG")"'\";' $1
> +fi

This isn't a bug, but could leaving the variables $1 and $2 unquoted cause
issues if the provided filenames contain spaces? This might result in word
splitting and cause xxd or sed to read unintended files.

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