From: Matthias Schiffer <mschiffer@universe-factory.net>
Date: Sat, 16 May 2020 20:29:24 +0000 (+0200)
Subject: Fix length checks in cert_load()
X-Git-Url: https://git.librecmc.org/?p=oweals%2Fucert.git;a=commitdiff_plain;h=96c42c5ed3207b8ad1ce836a4426c8700c13b655

Fix length checks in cert_load()

cert_load() iterates over multiple blobs, so the length argument to
blob_parse_untrusted() needs to be updated to prevent out-of-bounds
accesses.

Some other checks have become redundant and are removed, as
blob_parse_untrusted() already ensures that all attrs are contained in
the passed buffer.

Note that this issue currently does not pose a security threat, as an
over-restrictive check in blob_parse_untrusted() broke parsing of
buffers with multiple blobs completely.

Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
---

diff --git a/ucert.c b/ucert.c
index 208d5f6..24349c4 100644
--- a/ucert.c
+++ b/ucert.c
@@ -164,9 +164,8 @@ static int cert_load(const char *certfile, struct list_head *chain) {
 	struct blob_attr *certtb[CERT_ATTR_MAX];
 	struct blob_attr *bufpt;
 	struct cert_object *cobj;
-	char filebuf[CERT_BUF_LEN];
-	int ret = 0, pret = 0;
-	size_t pos = 0;
+	char filebuf[CERT_BUF_LEN], *end;
+	int ret = 1;
 	ssize_t len;
 
 	len = read_file(certfile, filebuf, sizeof(filebuf) - 1, 0);
@@ -177,17 +176,16 @@ static int cert_load(const char *certfile, struct list_head *chain) {
 	}
 
 	bufpt = (struct blob_attr *)filebuf;
-	do {
-		pret = blob_parse_untrusted(bufpt, len, certtb, cert_policy, CERT_ATTR_MAX);
-		if (pret <= 0)
-			/* no attributes found */
+	end = filebuf + len;
+
+	while (true) {
+		len = end - (char *)bufpt;
+		if (len <= 0)
 			break;
 
-		if (pos + blob_pad_len(bufpt) > (size_t) len)
-			/* blob exceeds filebuffer */
+		if (blob_parse_untrusted(bufpt, len, certtb, cert_policy, CERT_ATTR_MAX) <= 0)
+			/* no attributes found */
 			break;
-		else
-			pos += blob_pad_len(bufpt);
 
 		if (!certtb[CERT_ATTR_SIGNATURE])
 			/* no signature -> drop */
@@ -199,11 +197,17 @@ static int cert_load(const char *certfile, struct list_head *chain) {
 			cobj->cert[CERT_ATTR_PAYLOAD] = blob_memdup(certtb[CERT_ATTR_PAYLOAD]);
 
 		list_add_tail(&cobj->list, chain);
-		ret += pret;
-	/* repeat parsing while there is still enough remaining data in buffer */
-	} while((size_t) len > pos + sizeof(struct blob_attr) && (bufpt = blob_next(bufpt)));
+		ret = 0;
+
+		/* Repeat parsing while there is still enough remaining data in buffer
+		 *
+		 * Note that blob_next() is only valid for untrusted data because blob_parse_untrusted()
+		 * verified that the buffer contains at least one blob, and that it is completely contained
+		 * in the buffer */
+		bufpt = blob_next(bufpt);
+	}
 
-	return (ret <= 0);
+	return ret;
 }
 
 #ifdef UCERT_FULL