From d2463c0cfee066111c3a72d188cd897957aa2988 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 10 Sep 2014 23:57:31 -0400 Subject: [PATCH 1/6] Avoid overflows and underflows in sscanf and friends (Patch from teor on 13104) --- src/common/util.c | 23 ++++-- src/test/test_util.c | 167 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 171 insertions(+), 19 deletions(-) diff --git a/src/common/util.c b/src/common/util.c index 2d7893b38a..297ae7806e 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2804,10 +2804,14 @@ scan_unsigned(const char **bufp, unsigned long *out, int width, int base) while (**bufp && (hex?TOR_ISXDIGIT(**bufp):TOR_ISDIGIT(**bufp)) && scanned_so_far < width) { int digit = hex?hex_decode_digit(*(*bufp)++):digit_to_num(*(*bufp)++); - unsigned long new_result = result * base + digit; - if (new_result < result) - return -1; /* over/underflow. */ - result = new_result; + // Check for overflow beforehand, without actually causing any overflow + // This preserves functionality on compilers that don't wrap overflow + // (i.e. that trap or optimise away overflow) + // result * base + digit > ULONG_MAX + // result * base > ULONG_MAX - digit + if (result > (ULONG_MAX - digit)/base) + return -1; /* Processing this digit would overflow */ + result = result * base + digit; ++scanned_so_far; } @@ -2842,10 +2846,17 @@ scan_signed(const char **bufp, long *out, int width) if (scan_unsigned(bufp, &result, width, 10) < 0) return -1; - if (neg) { + if (neg && result > 0) { if (result > ((unsigned long)LONG_MAX) + 1) return -1; /* Underflow */ - *out = -(long)result; + // Avoid overflow on the cast to signed long when result is LONG_MIN + // by subtracting 1 from the unsigned long positive value, + // then, after it has been cast to signed and negated, + // subtracting the original 1 (the double-subtraction is intentional). + // Otherwise, the cast to signed could cause a temporary long + // to equal LONG_MAX + 1, which is undefined. + // We avoid underflow on the subtraction by treating -0 as positive. + *out = (-(long)(result - 1)) - 1; } else { if (result > LONG_MAX) return -1; /* Overflow */ diff --git a/src/test/test_util.c b/src/test/test_util.c index 151ec69127..16a2927e4d 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1672,6 +1672,7 @@ static void test_util_sscanf(void) { unsigned u1, u2, u3; + unsigned long ulng; char s1[20], s2[10], s3[10], ch; int r; long lng1,lng2; @@ -1713,11 +1714,6 @@ test_util_sscanf(void) test_eq(0, tor_sscanf("", "%u", &u1)); /* absent number */ test_eq(0, tor_sscanf("A", "%u", &u1)); /* bogus number */ test_eq(0, tor_sscanf("-1", "%u", &u1)); /* negative number */ - test_eq(1, tor_sscanf("4294967295", "%u", &u1)); /* UINT32_MAX should work */ - test_eq(4294967295u, u1); - test_eq(0, tor_sscanf("4294967296", "%u", &u1)); /* But not at 32 bits */ - test_eq(1, tor_sscanf("4294967296", "%9u", &u1)); /* but parsing only 9... */ - test_eq(429496729u, u1); /* Numbers with size (eg. %2u) */ test_eq(0, tor_sscanf("-1", "%2u", &u1)); @@ -1812,46 +1808,191 @@ test_util_sscanf(void) test_eq(int2, -1); #if SIZEOF_INT == 4 + /* %u */ + /* UINT32_MAX should work */ + test_eq(1, tor_sscanf("4294967295", "%u", &u1)); + test_eq(4294967295U, u1); + + /* But UINT32_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("4294967296", "%u", &u1)); + /* but parsing only 9... */ + test_eq(1, tor_sscanf("4294967296", "%9u", &u1)); + test_eq(429496729U, u1); + + /* %x */ + /* UINT32_MAX should work */ + test_eq(1, tor_sscanf("FFFFFFFF", "%x", &u1)); + test_eq(0xFFFFFFFF, u1); + + /* But UINT32_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("100000000", "%x", &u1)); + + /* %d */ + /* INT32_MIN and INT32_MAX should work */ r = tor_sscanf("-2147483648. 2147483647.", "%d. %d.", &int1, &int2); test_eq(r,2); - test_eq(int1, -2147483647-1); + test_eq(int1, -2147483647 - 1); test_eq(int2, 2147483647); - r = tor_sscanf("-2147483679.", "%d.", &int1); + /* But INT32_MIN - 1 and INT32_MAX + 1 shouldn't work */ + r = tor_sscanf("-2147483649.", "%d.", &int1); test_eq(r,0); - r = tor_sscanf("2147483678.", "%d.", &int1); + r = tor_sscanf("2147483648.", "%d.", &int1); + test_eq(r,0); + + /* and the first failure stops further processing */ + r = tor_sscanf("-2147483648. 2147483648.", + "%d. %d.", &int1, &int2); + test_eq(r,1); + + r = tor_sscanf("-2147483649. 2147483647.", + "%d. %d.", &int1, &int2); + test_eq(r,0); + + r = tor_sscanf("2147483648. -2147483649.", + "%d. %d.", &int1, &int2); test_eq(r,0); #elif SIZEOF_INT == 8 + /* %u */ + /* UINT64_MAX should work */ + test_eq(1, tor_sscanf("18446744073709551615", "%u", &u1)); + test_eq(18446744073709551615U, u1); + + /* But UINT64_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("18446744073709551616", "%u", &u1)); + /* but parsing only 19... */ + test_eq(1, tor_sscanf("18446744073709551616", "%19u", &u1)); + test_eq(1844674407370955161U, u1); + + /* %x */ + /* UINT64_MAX should work */ + test_eq(1, tor_sscanf("FFFFFFFFFFFFFFFF", "%x", &u1)); + test_eq(0xFFFFFFFFFFFFFFFF, u1); + + /* But UINT64_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("10000000000000000", "%x", &u1)); + + /* %d */ + /* INT64_MIN and INT64_MAX should work */ r = tor_sscanf("-9223372036854775808. 9223372036854775807.", "%d. %d.", &int1, &int2); test_eq(r,2); - test_eq(int1, -9223372036854775807-1); + test_eq(int1, -9223372036854775807 - 1); test_eq(int2, 9223372036854775807); + /* But INT64_MIN - 1 and INT64_MAX + 1 shouldn't work */ r = tor_sscanf("-9223372036854775809.", "%d.", &int1); test_eq(r,0); r = tor_sscanf("9223372036854775808.", "%d.", &int1); test_eq(r,0); + + /* and the first failure stops further processing */ + r = tor_sscanf("-9223372036854775808. 9223372036854775808.", + "%d. %d.", &int1, &int2); + test_eq(r,1); + + r = tor_sscanf("-9223372036854775809. 9223372036854775807.", + "%d. %d.", &int1, &int2); + test_eq(r,0); + + r = tor_sscanf("9223372036854775808. -9223372036854775809.", + "%d. %d.", &int1, &int2); + test_eq(r,0); #endif #if SIZEOF_LONG == 4 + /* %lu */ + /* UINT32_MAX should work */ + test_eq(1, tor_sscanf("4294967295", "%lu", &ulng)); + test_eq(4294967295UL, ulng); + + /* But UINT32_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("4294967296", "%lu", &ulng)); + /* but parsing only 9... */ + test_eq(1, tor_sscanf("4294967296", "%9lu", &ulng)); + test_eq(429496729UL, ulng); + + /* %lx */ + /* UINT32_MAX should work */ + test_eq(1, tor_sscanf("FFFFFFFF", "%lx", &ulng)); + test_eq(0xFFFFFFFFUL, ulng); + + /* But UINT32_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("100000000", "%lx", &ulng)); + + /* %ld */ + /* INT32_MIN and INT32_MAX should work */ r = tor_sscanf("-2147483648. 2147483647.", "%ld. %ld.", &lng1, &lng2); test_eq(r,2); - test_eq(lng1, -2147483647 - 1); - test_eq(lng2, 2147483647); + test_eq(lng1, -2147483647L - 1L); + test_eq(lng2, 2147483647L); + + /* But INT32_MIN - 1 and INT32_MAX + 1 shouldn't work */ + r = tor_sscanf("-2147483649.", "%ld.", &lng1); + test_eq(r,0); + + r = tor_sscanf("2147483648.", "%ld.", &lng1); + test_eq(r,0); + + /* and the first failure stops further processing */ + r = tor_sscanf("-2147483648. 2147483648.", + "%ld. %ld.", &lng1, &lng2); + test_eq(r,1); + + r = tor_sscanf("-2147483649. 2147483647.", + "%ld. %ld.", &lng1, &lng2); + test_eq(r,0); + + r = tor_sscanf("2147483648. -2147483649.", + "%ld. %ld.", &lng1, &lng2); + test_eq(r,0); #elif SIZEOF_LONG == 8 + /* %lu */ + /* UINT64_MAX should work */ + test_eq(1, tor_sscanf("18446744073709551615", "%lu", &ulng)); + test_eq(18446744073709551615UL, ulng); + + /* But UINT64_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("18446744073709551616", "%lu", &ulng)); + /* but parsing only 19... */ + test_eq(1, tor_sscanf("18446744073709551616", "%19lu", &ulng)); + test_eq(1844674407370955161UL, ulng); + + /* %lx */ + /* UINT64_MAX should work */ + test_eq(1, tor_sscanf("FFFFFFFFFFFFFFFF", "%lx", &ulng)); + test_eq(0xFFFFFFFFFFFFFFFFUL, ulng); + + /* But UINT64_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("10000000000000000", "%lx", &ulng)); + + /* %ld */ + /* INT64_MIN and INT64_MAX should work */ r = tor_sscanf("-9223372036854775808. 9223372036854775807.", "%ld. %ld.", &lng1, &lng2); test_eq(r,2); - test_eq(lng1, -9223372036854775807L - 1); + test_eq(lng1, -9223372036854775807L - 1L); test_eq(lng2, 9223372036854775807L); + /* But INT64_MIN - 1 and INT64_MAX + 1 shouldn't work */ + r = tor_sscanf("-9223372036854775809.", "%ld.", &lng1); + test_eq(r,0); + + r = tor_sscanf("9223372036854775808.", "%ld.", &lng1); + test_eq(r,0); + + /* and the first failure stops further processing */ r = tor_sscanf("-9223372036854775808. 9223372036854775808.", "%ld. %ld.", &lng1, &lng2); test_eq(r,1); - r = tor_sscanf("-9223372036854775809. 9223372036854775808.", + + r = tor_sscanf("-9223372036854775809. 9223372036854775807.", + "%ld. %ld.", &lng1, &lng2); + test_eq(r,0); + + r = tor_sscanf("9223372036854775808. -9223372036854775809.", "%ld. %ld.", &lng1, &lng2); test_eq(r,0); #endif From 5126bc2ebd9ae67449e3c4f18828acb895d86308 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 10 Sep 2014 23:58:02 -0400 Subject: [PATCH 2/6] Extra tests for tor_memeq and memcmp (Patch from teor; part of 13104) --- src/test/test_util.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/test/test_util.c b/src/test/test_util.c index 16a2927e4d..f091fc95a1 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -3251,6 +3251,25 @@ test_util_di_ops(void) test_eq(neq1, !eq1); } + /* exhaustively white-box test tor_memeq + * against each possible (single-byte) bit difference + * some arithmetic bugs only appear with certain bit patterns */ + const uint8_t z = 0; + uint8_t ii = 0; + for (i = 0; i < 256; i++) { + ii = (uint8_t)i; + test_eq(tor_memeq(&z, &ii, 1), z == ii); + } + + /* exhaustively white-box test tor_memcmp + * against each possible single-byte numeric difference + * some arithmetic bugs only appear with certain bit patterns */ + for (i = 0; i < 256; i++) { + ii = (uint8_t)i; + test_eq(tor_memcmp(&z, &ii, 1) > 0 ? GT : EQ, z > ii ? GT : EQ); + test_eq(tor_memcmp(&ii, &z, 1) < 0 ? LT : EQ, ii < z ? LT : EQ); + } + tt_int_op(1, ==, safe_mem_is_zero("", 0)); tt_int_op(1, ==, safe_mem_is_zero("", 1)); tt_int_op(0, ==, safe_mem_is_zero("a", 1)); From 59f9a5c78695213442599e8f2e8a535e8a9cc666 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Wed, 10 Sep 2014 23:59:21 -0400 Subject: [PATCH 3/6] Avoid divide by zero and NaNs in scale_array_elements_to_u64 Patch from teor; part of 13104 --- src/or/routerlist.c | 5 +++-- src/test/test_dir.c | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/or/routerlist.c b/src/or/routerlist.c index 32cbe19379..0df411b822 100644 --- a/src/or/routerlist.c +++ b/src/or/routerlist.c @@ -1802,7 +1802,7 @@ scale_array_elements_to_u64(u64_dbl_t *entries, int n_entries, uint64_t *total_out) { double total = 0.0; - double scale_factor; + double scale_factor = 0.0; int i; /* big, but far away from overflowing an int64_t */ #define SCALE_TO_U64_MAX (INT64_MAX / 4) @@ -1810,7 +1810,8 @@ scale_array_elements_to_u64(u64_dbl_t *entries, int n_entries, for (i = 0; i < n_entries; ++i) total += entries[i].dbl; - scale_factor = SCALE_TO_U64_MAX / total; + if (total > 0.0) + scale_factor = SCALE_TO_U64_MAX / total; for (i = 0; i < n_entries; ++i) entries[i].u64 = tor_llround(entries[i].dbl * scale_factor); diff --git a/src/test/test_dir.c b/src/test/test_dir.c index c03b63be27..1b56bcdd3d 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -1749,10 +1749,37 @@ test_dir_scale_bw(void *testdata) tt_assert(total <= (U64_LITERAL(1)<<62)); for (i=0; i<8; ++i) { + /* vals[2].u64 is the scaled value of 1.0 */ double ratio = ((double)vals[i].u64) / vals[2].u64; tt_double_op(fabs(ratio - v[i]), <, .00001); } + /* test handling of no entries */ + total = 1; + scale_array_elements_to_u64(vals, 0, &total); + tt_assert(total == 0); + + /* make sure we don't read the array when we have no entries + * may require compiler flags to catch NULL dereferences */ + total = 1; + scale_array_elements_to_u64(NULL, 0, &total); + tt_assert(total == 0); + + scale_array_elements_to_u64(NULL, 0, NULL); + + /* test handling of zero totals */ + total = 1; + vals[0].dbl = 0.0; + scale_array_elements_to_u64(vals, 1, &total); + tt_assert(total == 0); + tt_assert(vals[0].u64 == 0); + + vals[0].dbl = 0.0; + vals[1].dbl = 0.0; + scale_array_elements_to_u64(vals, 2, NULL); + tt_assert(vals[0].u64 == 0); + tt_assert(vals[1].u64 == 0); + done: ; } From 284cc9a22405bf320944b34ac08be74c2d897fc3 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 11 Sep 2014 00:00:13 -0400 Subject: [PATCH 4/6] Avoid an overflow on negation in format_helper_exit_status Part of 13104; patch from teor. --- src/common/util.c | 8 +++++++- src/test/test_util.c | 23 +++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/src/common/util.c b/src/common/util.c index 297ae7806e..3acae1c118 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -3555,7 +3555,13 @@ format_helper_exit_status(unsigned char child_state, int saved_errno, /* Convert errno to be unsigned for hex conversion */ if (saved_errno < 0) { - unsigned_errno = (unsigned int) -saved_errno; + // Avoid overflow on the cast to unsigned int when result is INT_MIN + // by adding 1 to the signed int negative value, + // then, after it has been negated and cast to unsigned, + // adding the original 1 back (the double-addition is intentional). + // Otherwise, the cast to signed could cause a temporary int + // to equal INT_MAX + 1, which is undefined. + unsigned_errno = ((unsigned int) -(saved_errno + 1)) + 1; } else { unsigned_errno = (unsigned int) saved_errno; } diff --git a/src/test/test_util.c b/src/test/test_util.c index f091fc95a1..eed37ca817 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -2561,12 +2561,17 @@ test_util_exit_status(void *ptr) int n; (void)ptr; + + clear_hex_errno(hex_errno); + test_streq("", hex_errno); clear_hex_errno(hex_errno); n = format_helper_exit_status(0, 0, hex_errno); test_streq("0/0\n", hex_errno); test_eq(n, strlen(hex_errno)); +#if SIZEOF_INT == 4 + clear_hex_errno(hex_errno); n = format_helper_exit_status(0, 0x7FFFFFFF, hex_errno); test_streq("0/7FFFFFFF\n", hex_errno); @@ -2577,6 +2582,21 @@ test_util_exit_status(void *ptr) test_streq("FF/-80000000\n", hex_errno); test_eq(n, strlen(hex_errno)); test_eq(n, HEX_ERRNO_SIZE); + +#elif SIZEOF_INT == 8 + + clear_hex_errno(hex_errno); + n = format_helper_exit_status(0, 0x7FFFFFFFFFFFFFFF, hex_errno); + test_streq("0/7FFFFFFFFFFFFFFF\n", hex_errno); + test_eq(n, strlen(hex_errno)); + + clear_hex_errno(hex_errno); + n = format_helper_exit_status(0xFF, -0x8000000000000000, hex_errno); + test_streq("FF/-8000000000000000\n", hex_errno); + test_eq(n, strlen(hex_errno)); + test_eq(n, HEX_ERRNO_SIZE); + +#endif clear_hex_errno(hex_errno); n = format_helper_exit_status(0x7F, 0, hex_errno); @@ -2587,6 +2607,9 @@ test_util_exit_status(void *ptr) n = format_helper_exit_status(0x08, -0x242, hex_errno); test_streq("8/-242\n", hex_errno); test_eq(n, strlen(hex_errno)); + + clear_hex_errno(hex_errno); + test_streq("", hex_errno); done: ; From 9e595a95a8aae83381246f012157107c9f6dde5e Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 11 Sep 2014 00:05:56 -0400 Subject: [PATCH 5/6] Add a changes file for bug 13104 --- changes/bug13104 | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changes/bug13104 diff --git a/changes/bug13104 b/changes/bug13104 new file mode 100644 index 0000000000..331db64ccc --- /dev/null +++ b/changes/bug13104 @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Fix several instances of possible integer overflow/underflow/NaN. + Fixes bug 13104; bugfix on 0.2.3.1-alpha and later. Patches from + "teor". From 2491eadf002e993bee11aa76597ec7f361d8f6e5 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 11 Sep 2014 00:10:53 -0400 Subject: [PATCH 6/6] C90 compliance for #13104 fixes --- src/test/test_util.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/test_util.c b/src/test/test_util.c index eed37ca817..e105435f05 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -3277,6 +3277,7 @@ test_util_di_ops(void) /* exhaustively white-box test tor_memeq * against each possible (single-byte) bit difference * some arithmetic bugs only appear with certain bit patterns */ + { const uint8_t z = 0; uint8_t ii = 0; for (i = 0; i < 256; i++) { @@ -3292,6 +3293,7 @@ test_util_di_ops(void) test_eq(tor_memcmp(&z, &ii, 1) > 0 ? GT : EQ, z > ii ? GT : EQ); test_eq(tor_memcmp(&ii, &z, 1) < 0 ? LT : EQ, ii < z ? LT : EQ); } + } tt_int_op(1, ==, safe_mem_is_zero("", 0)); tt_int_op(1, ==, safe_mem_is_zero("", 1));