From: Phil Carmody <phil@dovecot.fi>
Date: Mon, 9 Jun 2014 20:02:52 +0000 (+0300)
Subject: lib: fix numpack overflow checking
X-Git-Tag: 2.2.14.rc1~420
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a6050e85dfaa9af0a7b4e059c198df6dc0133388;p=thirdparty%2Fdovecot%2Fcore.git

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 0da3fe7aa2..921a785fd0 100644
--- 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 52352935f3..ff1e16e0be 100644
--- 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();
 }