Re: [v6 07/10] hornet: Introduce gen_sig

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Off-by-one bounds check logic in argument parsing aborts execution on valid inputs
- [High] Private key passphrase is exposed via command-line arguments
- [Medium] `write-sig.sh` does not modify the target header file as intended
- [Medium] Missing error handling for OpenSSL allocations and encoding can lead to crashes
- [Medium] `sha256()` destroys global OpenSSL error strings, breaking subsequent error reporting
- [Medium] Incomplete command-line argument parsing ignores `--help` and unrecognized flags
- [Medium] Memory leaks of OpenSSL context objects and ASN.1 structures
--

commit bf20721f572ef02a45417d08219036dd03d54e92
Author: Blaise Boscaccy <bboscaccy@linux.microsoft.com>
hornet: Introduce gen_sig

This commit introduces the gen_sig tool to create pkcs#7 signatures for
data payloads with an appended set of hashes. It also provides a script
to rewrite an autogenerated eBPF lskel header with a new signature.

> diff --git a/scripts/hornet/gen_sig.c b/scripts/hornet/gen_sig.c
> new file mode 100644
> index 000000000000..8e328b4ca120
> --- /dev/null
> +++ b/scripts/hornet/gen_sig.c
> @@ -0,0 +1,397 @@
[ ... ]
> +static void usage(const char *prog)
> +{
> +	fprintf(stderr,
> +		"Usage:\n"
> +		"  %s --data content.bin --cert signer.crt --key signer.key [-pass pass]\n"

Is it safe to accept the private key passphrase via a command-line argument?
Local users can view process arguments through the proc filesystem or tools
like ps, which could allow them to harvest the passphrase.

[ ... ]
> +static int sha256(const char *path, unsigned char out[SHA256_LEN], unsigned int *out_len)
> +{
[ ... ]
> +done:
> +	EVP_MD_CTX_free(ctx);
> +	fclose(f);
> +	ERR_free_strings();
> +	return rc;
> +}

Could clearing the error strings here cause issues with error reporting
later in the program? If a subsequent cryptographic operation fails after
this function returns, the global OpenSSL error state will be empty,
preventing tools from printing human-readable error 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);

Is it possible for HORNET_MAP_new() to return a NULL pointer? If so,
dereferencing it immediately on the next line might cause a crash.

> +	ASN1_OCTET_STRING_set(map->hash, buffer, buffer_len);
> +	sk_HORNET_MAP_push(set->maps, map);
> +}

If pushing the map onto the stack fails, will the allocated map object
be leaked since it won't be freed when the parent set is later destroyed?

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

This isn't a bug, but the short option D for data appears to be missing
from this string.

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

Should there be a handler for the help flag or unrecognized options here?
If someone passes the help flag, the program seems to silently ignore it
and proceed instead of displaying the usage information.

[ ... ]
> +		case 'A':
> +			if (strchr(optarg, ':')) {
> +				hashes[hash_count].file = strsep(&optarg, ":");
> +				hashes[hash_count].index = atoi(optarg);
> +				if (++hash_count >= MAX_HASHES) {
> +					usage(argv[0]);
> +					return EXIT_FAILURE;
> +				}

Does this check prematurely abort the program on valid input? Since the
array can hold exactly MAX_HASHES items, if a user provides exactly that
many items, incrementing hash_count to MAX_HASHES will trigger this failure
path rather than allowing the program to continue processing.

> +			} else {
> +				usage(argv[0]);
> +				return EXIT_FAILURE;
> +			}
> +		}
> +	}
[ ... ]
> +	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();

When MAP_SET_new() is called, doesn't it automatically allocate an empty
stack for the maps field? Overwriting it with sk_HORNET_MAP_new_null()
might leak that initially allocated stack.

Also, can MAP_SET_new() fail and return NULL here before set->maps is
accessed?

> +
> +	for (i = 0; i < hash_count; i++) {
> +		if (sha256(hashes[i].file, hash_buffer, &hash_len) != 0) {
> +			DIE("failed to hash input");
> +		}
> +		add_hash(set, hash_buffer, hash_len, hashes[i].index);
> +	}
> +
> +	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);

Are we sure ASN1_item_i2d() succeeded? If it fails, it can return a negative
value, which would be passed as the length to CMS_signed_add1_attr_by_OBJ().
Some OpenSSL APIs treat negative lengths as null-terminated strings and
attempt to call strlen() on a potentially NULL pointer.

> +
> +	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);
> +
> +	BIO_free(data_in);
> +	BIO_free(b_out);
> +	EVP_cleanup();
> +	ERR_free_strings();
> +	return 0;
> +}

Does this function leak the signer, pkey, and cms_out objects? I don't see
calls to X509_free(), EVP_PKEY_free(), or CMS_ContentInfo_free() before
returning.

> diff --git a/scripts/hornet/write-sig.sh b/scripts/hornet/write-sig.sh
> new file mode 100755
> index 000000000000..7eaabe3bab9a
> --- /dev/null
> +++ b/scripts/hornet/write-sig.sh
> @@ -0,0 +1,27 @@
[ ... ]
> +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

Does this script actually modify the intended header file? It looks like
the sed command is missing the in-place edit flag, so the output is merely
printed to standard output while the original file remains unchanged.

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