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 a17e9ea12..e412f089b 100644 --- a/src/random.c +++ b/src/random.c @@ -149,6 +149,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 96dd35fae..b246730cc 100644 --- a/test/unittester.c +++ b/test/unittester.c @@ -97,6 +97,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(); @@ -232,6 +233,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); +}