global-buffer-overflow out of bounds write in recovery_cipher_finalize()

invd commented 3 years ago

The following write operation can go out of bounds since the inner strnlen() call can return 0. This allows an unsigned integer underflow after subtracting 1.

https://github.com/keepkey/keepkey-firmware/blob/a988325501ca75554a253bb7e8b2bf4511e92747/lib/firmware/recovery_cipher.c#L591

runtime error: index 18446744073709551615 out of bounds for type 'char [288]'
runtime error: addition of unsigned offset to 0x0000014b8ca0 overflowed to 0x0000014b8c9f

==23917==ERROR: AddressSanitizer: global-buffer-overflow on address 0x0000014b8c9f at pc 0x0000005a6404 bp 0x7ffe82a10db0 sp 0x7ffe82a10da8
WRITE of size 1 at 0x0000014b8c9f thread T0
    #0 0x5a6403 in recovery_cipher_finalize
    #1 0x5a50b6 in recovery_character
    #2 0x581be6 in fsm_msgCharacterAck
    #3 0x629e19 in dispatch
    #4 0x629e19 in usb_rx_helper
    #5 0x62a5ac in handle_usb_rx
    #6 0x631a3c in usbPoll
    #7 0x55a8d8 in exec()

Technical notes

AFAIK, this codepath is only reachable on uninitialized KeepKey devices.
The error behaves differently depending on compiler optimization levels, as it is common with undefined behavior.

Organizational notes

This issue has been reported confidentially to Shapeshift on 1.1.2020. It has been acknowledged by the team.
A patch has been suggested by Shapeshift engineers. It holds up well to my dynamic fuzz tests, but AFAIK it is not used in the firmware repository yet.

I suggest applying the internally proposed patch.

greatwolf commented 3 years ago

I'm interested in patching up this potentially undefined behavior bug on my fork. Where can I find the internally proposed patch for this?

Is it enough to just change the line to this?

new_mnemonic[MAX(1u, strnlen(new_mnemonic, sizeof(new_mnemonic)) - 1)] = '\0';

From what I can tell looking at the code above, new_mnemonic is a single-space delimited list of seed words. The last character of this array should be a space which gets replaced by a \0 null character.

invd commented 3 years ago

@greatwolf : this has been solved recently in the master branch through commit 04fbc47284d2a08342bf4d9a2049fa2a15a9620a . Note that there are two 1u changes on the line.

This issue is now resolved, closing.

invd commented 3 years ago

Reopening, the current patch is incorrect. Details have been communicated to KeepKey.

invd commented 3 years ago

This has been fixed through https://github.com/keepkey/keepkey-firmware/commit/974f2bee29a2542a326da6d9c2b05e8f7db74e4a , closing.

keepkey / keepkey-firmware

global-buffer-overflow out of bounds write in recovery_cipher_finalize() #237