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 208d5f67e10d11238736ae290b222c5f9c4f765a..24349c41349b0d444d98e42acfd544eae86d7974 100644 (file)
--- 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