Problematic length calculation in yh_com_sign_ssh_certificate()

[invd]

The yh_com_sign_ssh_certificate() implicitly relies on certain yh_util_sign_ssh_certificate() lengths for response_len if yrc == YHR_SUCCESS.

Otherwise the dlen value (third parameter) of the OpenSSL BIO_write() call will be problematic and go below 0:

yubihsm-shell/src/commands.c

Lines 3128 to 3130 in c409285

	(void) BIO_write(bio, data + 4 + 256,
	argv[4].len + response_len - 4 -
	256); // TODO(adma): FIXME, unmagify

For values of argv[4].len + response_len < 260, the computation result will involve (well-defined, but likely unwanted) unsigned integer overflows:

runtime error: unsigned integer overflow: 1 - 4 cannot be represented in type 'size_t' (aka 'unsigned long')
runtime error: unsigned integer overflow: 5 - 256 cannot be represented in type 'size_t' (aka 'unsigned long')

I expect the intermediary result to be close to SIZE_MAX, so a large positive value close to 2^32 or 2^64 depending on the system/compiler.

This is related to math operation rules between size_t and int variables. Fortunately, the cast back to the OpenSSL BIO_Write() int BIO_write(BIO *b, const void *data, int dlen); , to int dlen in particular, is expected to turn this back into a negative number that represents the original non-overflowed signed int result.

For OpenSSL 3.0, the documentation specifies that

BIO_write() returns -2 if the "write" operation is not implemented by the BIO or -1 on other errors. Otherwise it returns the number of bytes written. This may be 0 if the BIO b is NULL or dlen <= 0.

This suggests the case with a dlen like -251 is gracefully handled on the OpenSSL side. However, the yubishm code does not check the return code of BIO_Write() (which is dangerous, in my opinion) and therefore blindly continues with the regular operations on a bio context that's not as expected. In my current understanding, the code gets lucky that the following line has nothing to write because bufferPtr->length == 0, because the OpenSSL behavior will be different than expected:

yubihsm-shell/src/commands.c

Line 3144 in c409285

if (fwrite(bufferPtr->data, 1, bufferPtr->length, ctx->out) !=

To summarize, this is a dangerous edge case that leads to incorrect behavior - ultimately claiming the signing operation succeeded when it didn't, and returning incomplete data for the command - but doesn't trigger memory safety issues, undefined behavior, or a denial of service, as far as I can see.

I highly recommend

• refactoring this code to make the length expectations explicit
• avoid the error-prone underflow
• start checking response codes for BIO_write() and similar functions to abort on errors

Mentioning @a-dma due to the person-specific TODO code comment.

I found this issue during fuzzing with -fsanitize=unsigned-integer-overflow (a non-standard mechanism of UBSan).

[a-dma]

Hi and thank you for the report. @aveenismail could please you look into it?