From a4c0e607258ae4e5c8b6758e26721ba673c1635b Mon Sep 17 00:00:00 2001 From: Julius Werner Date: Fri, 10 Sep 2021 17:14:41 -0700 Subject: [PATCH] commonlib/cbfs: Fix minor parser edge cases This patch fixes a few minor CBFS parsing edge cases that could lead to unintended behavior: the CBFS attribute parser could have run into an infinite loop if an attribute's length was (accidentally or maliciously) invalid. A length of 0 would have caused it to read the same attribute over and over again without making forward progress, while a very large length could have caused an overflow that makes it go backwards to find the next attribute. Also, the filename was not guaranteed to be null-terminated which could have resulted in out-of-bounds reads on a few error messages. Finally, clarify the validity guarantees for CBFS header fields offered by cbfs_walk() in the comment explaining cbfs_mdata. Signed-off-by: Julius Werner Change-Id: Ie569786e5bec355b522f6580f53bdd8b16a4d726 Reviewed-on: https://review.coreboot.org/c/coreboot/+/57569 Tested-by: build bot (Jenkins) Reviewed-by: Jakub Czapiga --- src/commonlib/bsd/cbfs_private.c | 8 +++++--- src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h | 5 ++++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/commonlib/bsd/cbfs_private.c b/src/commonlib/bsd/cbfs_private.c index 94a29ac929..a8c2a3a61f 100644 --- a/src/commonlib/bsd/cbfs_private.c +++ b/src/commonlib/bsd/cbfs_private.c @@ -78,6 +78,8 @@ cb_err_t cbfs_walk(cbfs_dev_t dev, cb_err_t (*walker)(cbfs_dev_t dev, size_t off if (cbfs_dev_read(dev, mdata.raw + sizeof(mdata.h), offset + sizeof(mdata.h), todo) != todo) return CB_CBFS_IO; + /* Force filename null-termination, just in case. */ + mdata.raw[attr_offset ? attr_offset - 1 : data_offset - 1] = '\0'; DEBUG("File name: '%s'\n", mdata.h.filename); if (do_hash && !empty && vb2_digest_extend(&dc, mdata.raw, data_offset)) @@ -175,9 +177,9 @@ const void *cbfs_find_attr(const union cbfs_mdata *mdata, uint32_t attr_tag, siz const uint32_t tag = be32toh(attr->tag); const uint32_t len = be32toh(attr->len); - if (offset + len > end) { - ERROR("Attribute %s[%x] overflows end of metadata\n", - mdata->h.filename, tag); + if (len < sizeof(struct cbfs_file_attribute) || len > end - offset) { + ERROR("Attribute %s[%x] invalid length: %u\n", + mdata->h.filename, tag, len); return NULL; } if (tag == attr_tag) { diff --git a/src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h b/src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h index ed5c378973..d73a68c1af 100644 --- a/src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h +++ b/src/commonlib/bsd/include/commonlib/bsd/cbfs_mdata.h @@ -8,7 +8,10 @@ #include /* - * Helper structure to allocate space for a blob of metadata on the stack. + * Helper structure to allocate space for a blob of metadata on the stack. All functions using + * a cbfs_mdata should be getting it via cbfs_walk(), and can rely on the fact that cbfs_walk() + * has already fully validated the header (range checks for `len`, `attributes_offset` and + * `offset`, and null-termination for `filename`). * NOTE: The fields in any union cbfs_mdata or any of its substructures from cbfs_serialized.h * should always remain in the same byte order as they are stored on flash (= big endian). To * avoid byte-order confusion, fields should always and only be converted to host byte order at