From ce9de0c5e98290ace055f708eb625da9ad365f22 Mon Sep 17 00:00:00 2001 From: bluezr Date: Thu, 6 Aug 2026 12:26:37 -0700 Subject: [PATCH] utils, random: fix slice(), and make the TESTING RNG unbuildable in release Two findings from an audit sweep, unrelated except that both are guards that were not doing what they appeared to do. slice() is LIBDOGECOIN_API and had no in-tree callers, which is why neither of its defects was noticed: strncpy(result, str + start, end - start); strncpy writes no terminator when it copies its full count, and the count here was always exactly the number of bytes copied -- so result came back unterminated for every input, and a caller printing it read past the buffer. Separately, `end - start` is size_t arithmetic, so end < start wrapped to a length near SIZE_MAX and strncpy ran until it hit the source NUL or faulted. Now measures the source, clamps end to it, rejects end <= start and a start past the end, copies with memcpy and terminates. The header records the contract the function never stated: result needs (end - start) + 1 bytes, since the signature carries no size for it. Verified the test catches the original rather than merely passing with the fix: with slice() reverted, test_utils_slice fails at the first assertion with Expect: cde Receive: cdeXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX... which is the unterminated buffer. Separately, src/random.c carries a TESTING branch that replaces the CSPRNG with srand(time(NULL)) and rand(). Nothing in the build system defines TESTING -- not CMakeLists.txt, not configure.ac -- so it is unreachable today, and this does not change that. It adds an #error when TESTING is combined with NDEBUG, so that a release build which somehow enabled it fails loudly at compile time instead of quietly generating keys guessable from the wall clock. A dormant path that swaps out the RNG should be impossible to ship, not merely unlikely to be reached. 82/82, clean under ASan. --- include/dogecoin/utils.h | 3 +++ src/random.c | 13 ++++++++++ src/utils.c | 27 +++++++++++++++++++- test/unittester.c | 2 ++ test/utils_tests.c | 53 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 97 insertions(+), 1 deletion(-) diff --git a/include/dogecoin/utils.h b/include/dogecoin/utils.h index da2d0fdf7..289296039 100644 --- a/include/dogecoin/utils.h +++ b/include/dogecoin/utils.h @@ -83,6 +83,9 @@ LIBDOGECOIN_API void print_bits(size_t const size, void const* ptr); LIBDOGECOIN_API void prepend(char* s, const char* t); LIBDOGECOIN_API void append(char* s, char* t); LIBDOGECOIN_API char* concat(char* prefix, char* suffix); +/* Copy str[start, end) into result, NUL-terminated. result must have room for + (end - start) + 1 bytes -- the function takes no size for it. Yields an empty + string if end <= start or start is past the end of str. */ LIBDOGECOIN_API void slice(const char *str, char *result, size_t start, size_t end); LIBDOGECOIN_API void replace_last_after_delim(const char *str, char* delim, char* replacement); LIBDOGECOIN_API void text_to_hex(char* in, char* out); diff --git a/src/random.c b/src/random.c index 3f8b69b4a..3173c9dbf 100644 --- a/src/random.c +++ b/src/random.c @@ -159,6 +159,19 @@ dogecoin_bool dogecoin_random_bytes(uint8_t* buf, uint32_t len, const uint8_t up } #ifdef TESTING +/* + * This branch replaces the CSPRNG with srand(time(NULL)) and rand(), which + * would make every key this library generates predictable from the wall clock. + * Nothing in the build system defines TESTING -- not CMakeLists.txt, not + * configure.ac -- so it is unreachable today. The guard is here so that it + * stays unreachable in anything shipped: a release build sets NDEBUG, and + * combining the two must fail loudly at compile time rather than quietly + * produce guessable keys. + */ +#ifdef NDEBUG +#error "TESTING replaces the RNG with srand()/rand(); it must never be enabled in a release build" +#endif + void dogecoin_random_init_internal(void) { srand(time(NULL)); diff --git a/src/utils.c b/src/utils.c index f0af3f94d..f7bef76cb 100644 --- a/src/utils.c +++ b/src/utils.c @@ -680,7 +680,32 @@ char* concat(char* prefix, char* suffix) { void slice(const char *str, char *result, size_t start, size_t end) { - strncpy(result, str + start, end - start); + size_t slen, len; + + /* + * Two problems with the strncpy this replaces. It never terminated: + * strncpy writes no NUL when it copies its full count, and the count here + * was always exactly the number of bytes copied, so result was left + * unterminated for every input. And `end - start` is size_t arithmetic, so + * end < start wrapped to a length near SIZE_MAX. + * + * result must have room for (end - start) + 1 bytes. The function takes no + * size for it, so that is the contract; see the header. + */ + if (!str || !result) { + return; + } + slen = strlen(str); + if (end <= start || start >= slen) { + result[0] = '\0'; + return; + } + if (end > slen) { + end = slen; + } + len = end - start; + memcpy(result, str + start, len); + result[len] = '\0'; } void remove_substr(char *string, char *sub) { diff --git a/test/unittester.c b/test/unittester.c index df5181155..ce9ce67e6 100644 --- a/test/unittester.c +++ b/test/unittester.c @@ -99,6 +99,7 @@ extern void test_invalid_tx_deser(); extern void test_tx_sign(); extern void test_scripts(); extern void test_utils(); +extern void test_utils_slice(); extern void test_vector(); extern void test_qr(); @@ -236,6 +237,7 @@ int main() u_run_test(test_script_parse); u_run_test(test_script_op_codeseperator); u_run_test(test_utils); + u_run_test(test_utils_slice); u_run_test(test_vector); u_run_test(test_qr); diff --git a/test/utils_tests.c b/test/utils_tests.c index af2fc90ea..51aff5dc8 100644 --- a/test/utils_tests.c +++ b/test/utils_tests.c @@ -132,3 +132,56 @@ void test_dit() debug_print("%s", "DIT test: disabled (DIT not supported)\n"); } } + + +/* + * slice() is LIBDOGECOIN_API and had no in-tree callers, which is why neither + * defect was noticed: + * + * strncpy(result, str + start, end - start); + * + * strncpy writes no terminator when it copies its full count, and the count was + * always exactly the number of bytes copied -- so result came back unterminated + * for every input, and a caller printing it read past the buffer. Separately, + * `end - start` is size_t arithmetic, so end < start wrapped to a length near + * SIZE_MAX and strncpy ran until it hit the source NUL or a fault. + */ +void test_utils_slice() +{ + char buf[64]; + + /* Ordinary slice, and it must be terminated. */ + memset(buf, 'X', sizeof(buf)); + slice("abcdefghij", buf, 2, 5); + u_assert_str_eq(buf, "cde"); + u_assert_int_eq((int)strlen(buf), 3); + + /* Whole string. */ + memset(buf, 'X', sizeof(buf)); + slice("abcdef", buf, 0, 6); + u_assert_str_eq(buf, "abcdef"); + + /* end < start used to wrap to a huge count. Must yield an empty string. */ + memset(buf, 'X', sizeof(buf)); + slice("abcdefghij", buf, 5, 2); + u_assert_str_eq(buf, ""); + + /* end == start is an empty slice, not a one-byte read. */ + memset(buf, 'X', sizeof(buf)); + slice("abcdefghij", buf, 3, 3); + u_assert_str_eq(buf, ""); + + /* start past the end of the source must not read past it. */ + memset(buf, 'X', sizeof(buf)); + slice("abc", buf, 10, 20); + u_assert_str_eq(buf, ""); + + /* end past the end of the source clamps rather than over-reading. */ + memset(buf, 'X', sizeof(buf)); + slice("abc", buf, 1, 99); + u_assert_str_eq(buf, "bc"); + + /* NULL arguments are refused rather than dereferenced. */ + slice(NULL, buf, 0, 1); + slice("abc", NULL, 0, 1); +}