decimal: fix buffer overflow in decimal_unpack()

decimal_unpack() and hence mp_decode_decimal() didn't check length of
digits array, overflowing the output structure.

Fix that and a couple of typos in comments as well.

Closes https://github.com/tarantool/security/issues/17

NO_DOC=security
NO_CHANGELOG=security

(cherry picked from commit 142c324e)

src/lib/core/decimal.c

@@ -47,7 +47,7 @@ static __thread decContext decimal_context = {
 	/* Maximum precision during operations. */
 	DECIMAL_MAX_DIGITS,
 	/*
-	 * Maximum decimal lagarithm of the number.
+	 * Maximum decimal logarithm of the number.
 	 * Allows for precision = DECIMAL_MAX_DIGITS
 	 */
 	DECIMAL_MAX_DIGITS - 1,
@@ -62,7 +62,7 @@ static __thread decContext decimal_context = {
 	DECIMAL_ROUNDING,
 	/* Turn off signalling for failed operations. */
 	0,
-	/* Status holding occured events. Initially empty. */
+	/* Status holding occurred events. Initially empty. */
 	0,
 	/* Turn off exponent clamping. */
 	0
@@ -449,6 +449,33 @@ decimal_pack(char *data, const decimal_t *dec)
 	return data;
 }
 
+/*
+ * Amount of digits the structure can actually hold. Might be bigger than
+ * DECIMAL_MAX_DIGITS if DECIMAL_MAX_DIGITS is not divisible by DECDPUN.
+ */
+#define DECIMAL_DIGIT_CAPACITY (DECNUMUNITS * DECDPUN)
+static_assert(DECIMAL_DIGIT_CAPACITY >= DECIMAL_MAX_DIGITS,
+	      "DECIMAL_DIGIT_CAPACITY must be big enough to hold "
+	      "DECIMAL_MAX_DIGITS");
+
+/*
+ * Packed decimal representation is a BCD array (binary coded decimal),
+ * containing 2 digits per byte, except one last nibble containing the sign.
+ * So an input string of length L might fill up 2 * L - 1 digits.
+ *
+ * If DECIMAL_DIGIT_CAPACITY (CAP for short) is odd, we're fine. This is true
+ * for now, because CAP = DECDPUN(3) * DECNUMUNITS(13) = 39, and maximum string
+ * length (20) holds exactly 39 digits.
+ *
+ * OTOH, if CAP becomes even, this means max safe input will be of length
+ * (CAP + 1) / 2 == CAP / 2, allowing CAP - 1 digits at max. Hence
+ * CAP - 1 must be >= DECIMAL_MAX_DIGITS.
+ */
+static_assert(DECIMAL_DIGIT_CAPACITY % 2 == 1 ||
+	      DECIMAL_DIGIT_CAPACITY - 1 >= DECIMAL_MAX_DIGITS,
+	      "DECIMAL_DIGIT_CAPACITY got even, now it must be strictly "
+	      "greater than DECIMAL_MAX_DIGITS");
+
 decimal_t *
 decimal_unpack(const char **data, uint32_t len, decimal_t *dec)
 {
@@ -472,10 +499,23 @@ decimal_unpack(const char **data, uint32_t len, decimal_t *dec)
 	}
 
 	len -= *data - svp;
+	/* First check that there is enough space to strore the digits. */
+	if (len > (DECIMAL_DIGIT_CAPACITY + 1) / 2) {
+		*data = svp;
+		return NULL;
+	}
+	/* No digits to decode. */
+	if (len == 0) {
+		*data = svp;
+		return NULL;
+	}
 	decimal_t *res = decPackedToNumber((uint8_t *)*data, len, &scale, dec);
-	if (res)
+	/* Now check that the resulting number fits in our limits. */
+	if (res != NULL && decimal_precision(res) <= DECIMAL_MAX_DIGITS) {
 		*data += len;
-	else
+	} else {
+		res = NULL;
 		*data = svp;
+	}
 	return res;
 }

test/unit/decimal.c

@@ -89,7 +89,7 @@
 			  #end);\
 })
 
-char buf[32];
+char buf[64];
 
 #define test_mpdec(str) ({\
 	decimal_t dec;\
@@ -146,10 +146,37 @@ char buf[32];
 	is(val, num, "Conversion back to "#type" correct");\
 })
 
+struct canary {
+	decimal_t dec;
+	uint32_t val;
+};
+
+const uint32_t magic = 0xdecdecde;
+
+#define test_unpack(str, len, expected, exp_val) ({\
+	struct canary canary = {\
+		.dec = {0},\
+		.val = magic,\
+	};\
+	const char *bb = str;\
+	is(decimal_unpack((const char **)&bb, len, &canary.dec),\
+	   expected(&canary.dec), "Decode "#expected);\
+	is(canary.val, magic, "Canary is intact");\
+	if (expected(&canary.dec) != NULL) {\
+		is(bb, str + len, "Whole string is processed");\
+		decimal_t dec;\
+		decimal_from_string(&dec, exp_val);\
+		is(decimal_compare(&canary.dec, &dec), 0,\
+		   "Decoding is correct");\
+	} else {\
+		is(bb, str, "Buffer position is restored");\
+	} \
+})
+
 static int
 test_pack_unpack(void)
 {
-	plan(151);
+	plan(187);
 
 	test_decpack("0");
 	test_decpack("-0");
@@ -191,6 +218,51 @@ test_pack_unpack(void)
 	is(decimal_unpack(&bb, 3, &dec), NULL, "unpack malformed decimal fails");
 	is(bb, buf, "decode malformed decimal preserves buffer position");
 
+	/* Test buffer overflows on unpack. */
+	/* Only scale, no digits. */
+	b = "\x00";
+	test_unpack(b, 1, failure, "");
+	b = "\x00\x9c";
+	test_unpack(b, 2, success, "9");
+	/*     V - scale 39 overflows any number. */
+	b = "\x27\x0c";
+	test_unpack(b, 2, failure, "");
+	/*     V   V - scale -38 overflows any number */
+	b = "\xd0\xda\x0c";
+	test_unpack(b, 3, failure, "");
+	b = "\x26\x09\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x9c";
+	test_unpack(b, 21, success, "0.99999999999999999999999999999999999999");
+	b = "\x00\x09\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x9c";
+	test_unpack(b, 21, success, "99999999999999999999999999999999999999");
+	/* Missing nibble. */
+	b = "\x00\x09\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x99";
+	test_unpack(b, 21, failure, "");
+	/*         V - 39th digit overflows the buffer. */
+	b = "\x00\x99\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x9c";
+	test_unpack(b, 21, failure, "");
+	/*     V - scale -1 multiplies the number by 10 and overflows. */
+	b = "\xff\x09\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x9c";
+	test_unpack(b, 21, failure, "");
+	/* Too long, non-empty. */
+	b = "\x00\x99\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x99\x99\x99\x99"
+	    "\x99\x99\x99\x99\x99\x9c";
+	test_unpack(b, 22, failure, "");
+	/* Too long, empty. Still fails. */
+	b = "\x00\x00\x00\x00\x00\x00\x00\x00"
+	    "\x00\x00\x00\x00\x00\x00\x00\x00"
+	    "\x00\x00\x00\x00\x00\x0c";
+	test_unpack(b, 22, failure, "");
 	return check_plan();
 }
 

test/unit/decimal.result

@@ -345,7 +345,7 @@ ok 278 - decimal_sqrt(-10) - error on wrong operands.
     ok 65 - Conversion of -607 to decimal and back to int64 successful
     ok 66 - Conversion back to int64 correct
 ok 279 - subtests
-    1..151
+    1..187
     ok 1 - decimal_len(0)
     ok 2 - decimal_len(0) == len(decimal_pack(0)
     ok 3 - decimal_unpack(decimal_pack(0))
@@ -497,6 +497,42 @@ ok 279 - subtests
     ok 149 - positive exponent number is packed/unpacked correctly
     ok 150 - unpack malformed decimal fails
     ok 151 - decode malformed decimal preserves buffer position
+    ok 152 - Decode failure
+    ok 153 - Canary is intact
+    ok 154 - Buffer position is restored
+    ok 155 - Decode success
+    ok 156 - Canary is intact
+    ok 157 - Whole string is processed
+    ok 158 - Decoding is correct
+    ok 159 - Decode failure
+    ok 160 - Canary is intact
+    ok 161 - Buffer position is restored
+    ok 162 - Decode failure
+    ok 163 - Canary is intact
+    ok 164 - Buffer position is restored
+    ok 165 - Decode success
+    ok 166 - Canary is intact
+    ok 167 - Whole string is processed
+    ok 168 - Decoding is correct
+    ok 169 - Decode success
+    ok 170 - Canary is intact
+    ok 171 - Whole string is processed
+    ok 172 - Decoding is correct
+    ok 173 - Decode failure
+    ok 174 - Canary is intact
+    ok 175 - Buffer position is restored
+    ok 176 - Decode failure
+    ok 177 - Canary is intact
+    ok 178 - Buffer position is restored
+    ok 179 - Decode failure
+    ok 180 - Canary is intact
+    ok 181 - Buffer position is restored
+    ok 182 - Decode failure
+    ok 183 - Canary is intact
+    ok 184 - Buffer position is restored
+    ok 185 - Decode failure
+    ok 186 - Canary is intact
+    ok 187 - Buffer position is restored
 ok 280 - subtests
     1..216
     ok 1 - mp_sizeof_decimal(0)