search

square / subzero

Block's Bitcoin Cold Storage solution.
Apache License 2.0
683 stars 95 forks source link

[core] fix possible stack buffer overflow in no_rollback logic #609

Closed ivmaykov closed 1 year ago

ivmaykov commented 1 year ago

The parsing logic in no_rollback_check() could end up reading past the end of the stack buffer if the file contains a string with lots of leading 0s, followed by the expected magic number, followed by a dash ('-').

Since the VERSION_SIZE is 100 but we only store 2 uint32's and a dash, we always use much less than 100 bytes. So it's safe to assume that the rest of the buffer will contain NULL characters.

This change enforces this assumption on both reads and writes of the no_rollback file (dev mode) / NVRAM (ncipher mode).

I tried adding a self-check which fails ASAN with the bad input, but apparently the strtoul() function is not ASAN-instrumented (at least on MacOS). So, I couldn't actually get ASAN to crash with an out-of-bounds read, though I was able to get ASAN crashes by replacing the strtoul() with my own parsing function. Sigh.

ivmaykov commented 1 year ago

by "lots of leading 0s" I mean the character "0", not the 0 byte. strtoul() ignores leading zero characters when an explicit base is specified, so e.g. it will parse "00001" as "1".