[PATCH 1/3] patch-delta: fix oob read

From: Jann Horn <jannh@google.com>
Date: Aug 29, 2018, 20:58 UTC
Message-ID: <20180829205857.77340-1-jannh@google.com>

If `cmd` is in the range [0x01,0x7f] and `cmd > top-data`, the `memcpy(out, data, cmd)` can copy out-of-bounds data from after `delta_buf` into `dst_buf`.

This is not an exploitable bug because triggering the bug increments the `data` pointer beyond `top`, causing the `data != top` sanity check after the loop to trigger and discard the destination buffer - which means that the result of the out-of-bounds read is never used for anything.

Also, directly jump into the error handler instead of just breaking out of the loop - otherwise, data corruption would be silently ignored if the delta buffer ends with a command and the destination buffer is already full.

Signed-off-by: Jann Horn <jannh@google.com>
---
 patch-delta.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/patch-delta.c b/patch-delta.c
index 56e0a5ede..283fb4b75 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -51,13 +51,13 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 			if (unsigned_add_overflows(cp_off, cp_size) ||
 			    cp_off + cp_size > src_size ||
 			    cp_size > size)
-				break;
+				goto bad_length;
 			memcpy(out, (char *) src_buf + cp_off, cp_size);
 			out += cp_size;
 			size -= cp_size;
 		} else if (cmd) {
-			if (cmd > size)
-				break;
+			if (cmd > size || cmd > top - data)
+				goto bad_length;
 			memcpy(out, data, cmd);
 			out += cmd;
 			data += cmd;
@@ -75,6 +75,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 
 	/* sanity check */
 	if (data != top || size != 0) {
+		bad_length:
 		error("delta replay has gone wild");
 		bad:
 		free(dst_buf);

-- 
2.19.0.rc0.228.g281dcd1b4d0-goog