dovecot-2.2: lib: fix numpack overflow checking

[dovecot at dovecot.org]

details:   http://hg.dovecot.org/dovecot-2.2/rev/1c276f2e59a5
changeset: 17450:1c276f2e59a5
user:      Phil Carmody <phil at dovecot.fi>
date:      Mon Jun 09 23:02:52 2014 +0300
description:
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 at dovecot.fi>

diffstat:

 src/lib/numpack.c      |   8 +++-----
 src/lib/test-numpack.c |  39 ++++++++++++++++++++++++++++++---------
 2 files changed, 33 insertions(+), 14 deletions(-)

diffs (93 lines):

diff -r bc4f09a5cb11 -r 1c276f2e59a5 src/lib/numpack.c
--- a/src/lib/numpack.c	Mon Jun 09 23:02:52 2014 +0300
+++ b/src/lib/numpack.c	Mon Jun 09 23:02:52 2014 +0300
@@ -21,7 +21,7 @@
 	uint64_t value = 0;
 	unsigned int bits = 0;
 
-	for (;;) {
+	while (bits < 64) {
 		if (c == end)
 			return -1;
 
@@ -33,11 +33,9 @@
 		c++;
 	}
 
-	if (bits >= 64) {
-		/* overflow */
-		*p = end;
+	bits += bits_required8(*c);
+	if (bits > 64) /* overflow */
 		return -1;
-	}
 
 	*p = c + 1;
 	*num_r = value;
diff -r bc4f09a5cb11 -r 1c276f2e59a5 src/lib/test-numpack.c
--- a/src/lib/test-numpack.c	Mon Jun 09 23:02:52 2014 +0300
+++ b/src/lib/test-numpack.c	Mon Jun 09 23:02:52 2014 +0300
@@ -10,7 +10,7 @@
 	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 @@
 	{ 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 @@
 	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();
 }