commit 42ba3997d64591822411fbbedd51a240dbbb5fab Author: José M. Guisado guigom@riseup.net Date: Wed Sep 18 13:28:29 2019 +0200

Check memunit parsing for overflow in confparse

Before, when parsing memunits, if overflow occured it failed silently. Use nowrap u64 math to detect overflow, compare to INT64_MAX and if greater tell user and fail accordingly.

15000000.5 TB fails double check as it a greater floating number than (double)INT64_MAX

8388608.1 TB passes double check because it falls in the same value as (double)INT64_MAX (which is 2^63), but will fail the int check because (uint64_t)d, which is 2^63, is strictly greater than 2^63-1 (INT64_MAX).

Fixes #30920 Signed-off-by: José M. Guisado guigom@riseup.net --- changes/ticket30920 | 3 +++ src/lib/confmgt/.may_include | 1 + src/lib/confmgt/unitparse.c | 35 +++++++++++++++++++++++++++++++---- src/test/test_confparse.c | 17 ++++++++++++++--- 4 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/changes/ticket30920 b/changes/ticket30920 new file mode 100644 index 000000000..d2fd8c9da --- /dev/null +++ b/changes/ticket30920 @@ -0,0 +1,3 @@ + o Minor bugfix (configuration): + - Check for multiplication overflow when parsing memory units inside + configuration. Fixes bug 30920; bugfix on 0.0.9rc1~46. diff --git a/src/lib/confmgt/.may_include b/src/lib/confmgt/.may_include index 256413391..5ff949f10 100644 --- a/src/lib/confmgt/.may_include +++ b/src/lib/confmgt/.may_include @@ -4,6 +4,7 @@ lib/conf/*.h lib/confmgt/*.h lib/container/*.h lib/encoding/*.h +lib/intmath/*.h lib/log/*.h lib/malloc/*.h lib/string/*.h diff --git a/src/lib/confmgt/unitparse.c b/src/lib/confmgt/unitparse.c index c3ed8285a..8cbf9903e 100644 --- a/src/lib/confmgt/unitparse.c +++ b/src/lib/confmgt/unitparse.c @@ -15,6 +15,7 @@ #include "lib/log/util_bug.h" #include "lib/string/parse_int.h" #include "lib/string/util_string.h" +#include "lib/intmath/muldiv.h"

#include <string.h>

@@ -109,6 +110,7 @@ const struct unit_table_t time_msec_units[] = { * table <b>u</b>, then multiply the number by the unit multiplier. * On success, set *<b>ok</b> to 1 and return this product. * Otherwise, set *<b>ok</b> to 0. + * Warns user when overflow or a negative value is detected. */ uint64_t config_parse_units(const char *val, const unit_table_t *u, int *ok) @@ -142,10 +144,35 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok)

for ( ;u->unit;++u) { if (!strcasecmp(u->unit, cp)) { - if (use_float) - v = (uint64_t)(u->multiplier * d); - else - v *= u->multiplier; + if (use_float) { + d = u->multiplier * d; + + if (d < 0) { + log_warn(LD_CONFIG, "Negative value arised when parsing %s %s", + val, u->unit); + *ok = 0; + goto done; + } + + // Some compilers may warn about casting a double to an unsigned type + // because they don't know if d is >= 0 + if (d >= 0 && (d > (double)INT64_MAX || (uint64_t)d > INT64_MAX)) { + log_warn(LD_CONFIG, "Overflow detected parsing %s %s", val, u->unit); + *ok = 0; + goto done; + } + + v = (uint64_t) d; + } else { + v = tor_mul_u64_nowrap(v, u->multiplier); + + if (v > INT64_MAX) { + log_warn(LD_CONFIG, "Overflow detected parsing %s %s", val, u->unit); + *ok = 0; + goto done; + } + } + *ok = 1; goto done; } diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 5f29a22c1..e0c9b3f63 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -906,11 +906,22 @@ test_confparse_unitparse(void *args) tt_assert(ok);

/* u64 overflow */ - /* XXXX our implementation does not currently detect this. See bug 30920. */ - /* tt_u64_op(config_parse_memunit("20000000 TB", &ok), OP_EQ, 0); tt_assert(!ok); - */ + // This test fails the double check as the float representing 15000000.5 TB + // is greater than (double) INT64_MAX + tt_u64_op(config_parse_memunit("15000000.5 TB", &ok), OP_EQ, 0); + tt_assert(!ok); + // 8388608.1 TB passes double check because it falls in the same float + // value as (double)INT64_MAX (which is 2^63) due to precision. + // But will fail the int check because the unsigned representation of + // the float, which is 2^63, is strictly greater than INT64_MAX (2^63-1) + tt_u64_op(config_parse_memunit("8388608.1 TB", &ok), OP_EQ, 0); + tt_assert(!ok); + + /* negative float */ + tt_u64_op(config_parse_memunit("-1.5 GB", &ok), OP_EQ, 0); + tt_assert(!ok);

/* i32 overflow */ tt_int_op(config_parse_interval("1000 months", &ok), OP_EQ, -1);