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

sashiko-bot
2026-04-16T21:33:56Z

Sashiko AI review found 5 potential issue(s):
- [High] Infinite loop during argument parsing on architectures with unsigned `char`.
- [High] Unchecked `sha256()` return value leads to usage of uninitialized stack memory.
- [Medium] Stack buffer overflow when parsing `--add` arguments.
- [Medium] Unchecked return values for OpenSSL serialization and file output lead to silent failures.
- [Medium] `write-sig.sh` outputs to stdout instead of modifying the target file in-place.
--

commit 5e34aa795f3e17cbd08a08fff14bbdb70263937f
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. It additionally appends 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..f966516ebc99b
> --- /dev/null
> +++ b/scripts/hornet/gen_sig.c

[ ... ]

> +int main(int argc, char **argv)
> +{
> + const char *cert_path = NULL;
> + const char *key_path = NULL;
> + const char *data_path = NULL;
> + const char *out_path = NULL;
> +
> + X509 *signer;
> + EVP_PKEY *pkey;
> + BIO *data_in;
> + CMS_ContentInfo *cms_out;
> + struct hash_spec hashes[MAX_HASHES];
> + int hash_count = 0;
> + int flags;
> + CMS_SignerInfo *si;
> + MAP_SET *set;
> + unsigned char hash_buffer[SHA256_LEN];
> + unsigned int hash_len;
> + ASN1_OBJECT *oid;
> + unsigned char *der = NULL;
> + int der_len;
> + int err;
> + BIO *b_out;
> + int i;
> + char opt;

Could this cause an infinite loop on certain architectures?

Because opt is a char, on platforms where char is unsigned by default (such as
ARM and PowerPC), it cannot hold the value -1.

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

When getopt_long_only() returns -1, it would be converted to 255. The loop
condition 255 != -1 would evaluate to true, resulting in an infinite loop that
could hang the build process.

> + switch (opt) {

[ ... ]

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

Is it possible to overflow the hashes array here?

The hashes array is allocated with a fixed capacity of MAX_HASHES (64). Since
hash_count is incremented without bounds checking, providing more than 64 --add
flags could write out of bounds on the stack.

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

[ ... ]

> + set = MAP_SET_new();
> + set->maps = sk_HORNET_MAP_new_null();
> +
> + for (i = 0; i < hash_count; i++) {
> + sha256(hashes[i].file, hash_buffer, &hash_len);

What happens if sha256() fails here?

If sha256() returns a negative error code (for instance, if the file cannot be
opened), the local variables hash_buffer and hash_len remain uninitialized.

> + add_hash(set, hash_buffer, hash_len, hashes[i].index);

Since the return value of sha256() is unchecked, this could blindly pass
uninitialized stack memory to add_hash(). The garbage value in hash_len might
be very large, causing out of bounds reads from the stack and potentially
embedding uninitialized memory into the generated signature.

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

Can ASN1_item_i2d() fail and return a negative length?

If der_len is negative upon failure, passing it to CMS_signed_add1_attr_by_OBJ()
might cause memory issues inside OpenSSL.

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

Does this silently ignore write failures?

If writing the final signature to the output file fails (e.g., due to a full
disk or an I/O error), ignoring the return value of i2d_CMS_bio_stream() means
the tool will still exit with a 0 status code. This could silently fail to
produce the necessary signature artifacts without breaking the build.

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

Should this use the -i flag for in-place modification?

Currently, sed prints the modified content to standard output instead of
modifying the target header file, which appears to leave the original file
unchanged.

> +fi

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