diff --git a/bstring/bstraux.c b/bstring/bstraux.c index 92d57fe..6b268ef 100644 --- a/bstring/bstraux.c +++ b/bstring/bstraux.c @@ -311,13 +311,22 @@ bNetStr2Bstr(const char * buff) return NULL; } x = 0; - for (i = 0; buff[i] != ':'; ++i) { + for (i = 0; i < 11 && buff[i] != ':'; ++i) { unsigned int v = buff[i] - '0'; if (v > 9 || x > ((INT_MAX - (signed int)v) / 10)) { return NULL; } x = (x * 10) + v; } + if (buff[i] != ':') { + return NULL; + } + /* Ensure the buffer spans at least i+1+x+1 bytes (digits, ':', data, ',') + * without scanning past that bound. A null byte before the expected comma + * position means the netstring is truncated. */ + if (memchr(buff, '\0', (size_t)i + 2 + (size_t)x) != NULL) { + return NULL; + } /* This thing has to be properly terminated */ if (buff[i + 1 + x] != ',') { return NULL; diff --git a/tests/testaux.c b/tests/testaux.c index ff80d96..0d52abe 100644 --- a/tests/testaux.c +++ b/tests/testaux.c @@ -483,6 +483,30 @@ START_TEST(core_013) } END_TEST +START_TEST(core_015) +{ + bstring b; + + /* + * Craft a buffer where: + * - The netstring claims length 5 ("5:abc"), but the actual content + * after ':' is only "abc" (3 bytes, null-terminated at index 5). + * - A ',' is planted at buff[7], which is exactly buff[i+1+x] + * (i=1, x=5) — the position the un-fixed code reads without bounds + * checking. + * + * Without the fix: buff[7]=',' passes the terminator check and the + * function returns a non-NULL bstring containing out-of-bounds data. + * With the fix: the bounds check catches i+1+x (7) >= blen (5) first + * and returns NULL. + */ + char crafted[16] = "5:abc"; + crafted[7] = ','; + b = bNetStr2Bstr(crafted); + ck_assert(b == NULL); +} +END_TEST + START_TEST(core_014) { bstring b; @@ -532,6 +556,7 @@ main(void) tcase_add_test(core, core_012); tcase_add_test(core, core_013); tcase_add_test(core, core_014); + tcase_add_test(core, core_015); suite_add_tcase(suite, core); /* Run tests */ SRunner *runner = srunner_create(suite);