lib: fix numpack overflow checking

As on broken input, bits may grow without limit, so << bits becomes
Undefined Behaviour. Add a simple check to the while loop to prevent
this.

Also, the (presumably) final byte adds something to the bit length,
so include that in the tally. If we didn't get to a final byte due
to the above while() condition, then this extra addition does no harm

Now we can precisely check for overflow conditions. Note that 64 bits
is perfectly OK, only 65+ is an overflow.

Note - no longer moving *p if there was a decode error.

Expand the test suite to check for overflow cases. Also checked for
short-input cases too, while I was there.

Signed-off-by: Phil Carmody <phil@dovecot.fi>

diff --git a/src/lib/numpack.c b/src/lib/numpack.c
index 0da3fe7aa20c6748215ae8f546c7270accdf4c14..921a785fd0d9fc7a9d8f9d99b964b7fbe2ab0534 100644 (file)
--- a/src/lib/numpack.c
+++ b/src/lib/numpack.c
@@ -21,7 +21,7 @@ int numpack_decode(const uint8_t **p, const uint8_t *end, uint64_t *num_r)
        uint64_t value = 0;
        unsigned int bits = 0;
 
-       for (;;) {
+       while (bits < 64) {
                if (c == end)
                        return -1;
 
@@ -33,11 +33,9 @@ int numpack_decode(const uint8_t **p, const uint8_t *end, uint64_t *num_r)
                c++;
        }
 
-       if (bits >= 64) {
-               /* overflow */
-               *p = end;
+       bits += bits_required8(*c);
+       if (bits > 64) /* overflow */
                return -1;
-       }
 
        *p = c + 1;
        *num_r = value;

diff --git a/src/lib/test-numpack.c b/src/lib/test-numpack.c
index 52352935f3109a196235c625bc5a7ce495e2c110..ff1e16e0bed06a13dcaee503e18ed09d50ab9691 100644 (file)
--- a/src/lib/test-numpack.c
+++ b/src/lib/test-numpack.c
@@ -10,7 +10,7 @@ static struct test {
        uint64_t input;
        uint8_t output[10];
        unsigned int output_size;
-} tests[] = {
+} enc_tests[] = {
        { 0xffffffff, { 0xff, 0xff, 0xff, 0xff, 0xf }, 5 },
        { 0, { 0 }, 1 },
        { 0x7f, { 0x7f }, 1 },
@@ -20,6 +20,16 @@ static struct test {
        { 0xffffffffffffffff, { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 1 }, 10 },
        { 0xfffffffe, { 0xfe, 0xff, 0xff, 0xff, 0xf }, 5 },
 };
+static struct fail {
+       uint8_t input[11];
+       unsigned int input_size;
+} dec_fails[] = {
+       { { 0 }, 0 },    /* has no termination byte */
+       { { 0x80 }, 1 },  /* ditto */
+       { { 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80 }, 10 }, /* ditto*/
+       { { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 2 }, 10 }, /* overflow */
+       { { 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f }, 11 }, /* ditto */
+};
 
 void test_numpack(void)
 {
@@ -27,18 +37,29 @@ void test_numpack(void)
        unsigned int i;
        const uint8_t *p, *end;
        uint64_t num;
+       uint64_t magic=0x9669699669969669;
 
-       test_begin("numpack");
-       for (i = 0; i < N_ELEMENTS(tests); i++) {
+       test_begin("numpack (good)");
+       for (i = 0; i < N_ELEMENTS(enc_tests); i++) {
                buffer_set_used_size(buf, 0);
-               numpack_encode(buf, tests[i].input);
-               test_assert(buf->used == tests[i].output_size &&
-                           memcmp(buf->data, tests[i].output,
-                                  tests[i].output_size) == 0);
+               numpack_encode(buf, enc_tests[i].input);
+               test_assert_idx(buf->used == enc_tests[i].output_size, i);
+               test_assert_idx(memcmp(buf->data, enc_tests[i].output,
+                                    enc_tests[i].output_size) == 0,
+                             i);
 
                p = buf->data; end = p + buf->used;
-               test_assert(numpack_decode(&p, end, &num) == 0);
-               test_assert(num == tests[i].input);
+               test_assert_idx(numpack_decode(&p, end, &num) == 0, i);
+               test_assert_idx(num == enc_tests[i].input, i);
+       }
+       test_end();
+
+       test_begin("numpack (bad)");
+       for (i = 0; i < N_ELEMENTS(dec_fails); i++) {
+               p = dec_fails[i].input; end = p + dec_fails[i].input_size;
+               num = magic;
+               test_assert_idx(numpack_decode(&p, end, &num) == -1, i);
+               test_assert_idx(p == dec_fails[i].input && num == magic, i);
        }
        test_end();
 }