Change in coreboot[master]: libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}

[Alexandre Rebert (Code Review)]

Alexandre Rebert has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39177 )

Change subject: libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}
......................................................................

libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}

cbfs_get_handle() and cbfs_get_attr() are both looping over elements to
find a particular one. Each element header contains the element's
length, which is used to compute the next element's offset. Invalid or
corrupted CBFS files could lead to infinite loops where the offset would
remain constant across iterations, due to 0-length elements or integer
overflows in the computation of the next offset.

This patch makes both functions more robust by adding a check that
ensure offsets are strictly monotonic. Instead of infinite looping, the
functions are now printing an ERROR and returning a NULL value.

Change-Id: I440e82fa969b8c2aacc5800e7e26450c3b97c74a
Signed-off-by: Alex Rebert 
Found-by: Mayhem
---
M payloads/libpayload/libcbfs/cbfs_core.c
1 file changed, 17 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/39177/1

diff --git a/payloads/libpayload/libcbfs/cbfs_core.c b/payloads/libpayload/libcbfs/cbfs_core.c
index e94e1e7..54e5de8 100644
--- a/payloads/libpayload/libcbfs/cbfs_core.c
+++ b/payloads/libpayload/libcbfs/cbfs_core.c
@@ -212,9 +212,15 @@
    	}

// Move to next file.
-		offset += ntohl(file.len) + ntohl(file.offset);
-		if (offset % CBFS_ALIGNMENT)
-			offset += CBFS_ALIGNMENT - (offset % CBFS_ALIGNMENT);
+		uint32_t new_offset = offset + ntohl(file.len) + ntohl(file.offset);
+		if (new_offset % CBFS_ALIGNMENT)
+			new_offset += CBFS_ALIGNMENT - (new_offset % CBFS_ALIGNMENT);
+		// Check that offset is strictly monotonic to prevent infinite loop
+		if(new_offset <= offset) {
+			ERROR("ERROR: corrupted CBFS file header at 0x%x.\n", offset);
+			break;
+		}
+		offset = new_offset;
    }
    media->close(media);
    LOG("WARNING: '%s' not found.\n", name);
@@ -309,7 +315,14 @@
    		return NULL;
    	}
    	if (ntohl(attr.tag) != tag) {
-			offset += ntohl(attr.len);
+			uint32_t new_offset = offset + ntohl(attr.len);
+			// Check that offset is strictly monotonic to prevent infinite loop
+			if(new_offset <= offset) {
+				ERROR("ERROR: corrupted CBFS attribute at 0x%x.\n", offset);
+				m->close(m);
+				return NULL;
+			}
+			offset = new_offset;
    		continue;
    	}
    	ret = m->map(m, offset, ntohl(attr.len));

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39177 )

Change subject: libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39177/1/payloads/libpayload/libcbfs... 
File payloads/libpayload/libcbfs/cbfs_core.c:

https://review.coreboot.org/c/coreboot/+/39177/1/payloads/libpayload/libcbfs... 
PS1, Line 219: 		if(new_offset <= offset) {
space required before the open parenthesis '('

https://review.coreboot.org/c/coreboot/+/39177/1/payloads/libpayload/libcbfs... 
PS1, Line 320: 			if(new_offset <= offset) {
space required before the open parenthesis '('

[Alexandre Rebert (Code Review)]

Alexandre Rebert has uploaded a new patch set (#2). ( https://review.coreboot.org/c/coreboot/+/39177 )

Change subject: libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}
......................................................................

libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}

cbfs_get_handle() and cbfs_get_attr() are both looping over elements to
find a particular one. Each element header contains the element's
length, which is used to compute the next element's offset. Invalid or
corrupted CBFS files could lead to infinite loops where the offset would
remain constant across iterations, due to 0-length elements or integer
overflows in the computation of the next offset.

This patch makes both functions more robust by adding a check that
ensure offsets are strictly monotonic. Instead of infinite looping, the
functions are now printing an ERROR and returning a NULL value.

Change-Id: I440e82fa969b8c2aacc5800e7e26450c3b97c74a
Signed-off-by: Alex Rebert 
Found-by: Mayhem
---
M payloads/libpayload/libcbfs/cbfs_core.c
1 file changed, 17 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/77/39177/2

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39177 )

Change subject: libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}
......................................................................

Patch Set 2: Code-Review+1

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39177 )

Change subject: libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}
......................................................................

Patch Set 2: Code-Review+2

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39177 )

Change subject: libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}
......................................................................

libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}

cbfs_get_handle() and cbfs_get_attr() are both looping over elements to
find a particular one. Each element header contains the element's
length, which is used to compute the next element's offset. Invalid or
corrupted CBFS files could lead to infinite loops where the offset would
remain constant across iterations, due to 0-length elements or integer
overflows in the computation of the next offset.

This patch makes both functions more robust by adding a check that
ensure offsets are strictly monotonic. Instead of infinite looping, the
functions are now printing an ERROR and returning a NULL value.

Change-Id: I440e82fa969b8c2aacc5800e7e26450c3b97c74a
Signed-off-by: Alex Rebert 
Found-by: Mayhem
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39177
Tested-by: build bot (Jenkins) 
Reviewed-by: Paul Menzel 
Reviewed-by: Patrick Georgi 
---
M payloads/libpayload/libcbfs/cbfs_core.c
1 file changed, 17 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Georgi: Looks good to me, approved
  Paul Menzel: Looks good to me, but someone else must approve

diff --git a/payloads/libpayload/libcbfs/cbfs_core.c b/payloads/libpayload/libcbfs/cbfs_core.c
index e94e1e7..30a41f8 100644
--- a/payloads/libpayload/libcbfs/cbfs_core.c
+++ b/payloads/libpayload/libcbfs/cbfs_core.c
@@ -212,9 +212,15 @@
    	}

// Move to next file.
-		offset += ntohl(file.len) + ntohl(file.offset);
-		if (offset % CBFS_ALIGNMENT)
-			offset += CBFS_ALIGNMENT - (offset % CBFS_ALIGNMENT);
+		uint32_t next_offset = offset + ntohl(file.len) + ntohl(file.offset);
+		if (next_offset % CBFS_ALIGNMENT)
+			next_offset += CBFS_ALIGNMENT - (next_offset % CBFS_ALIGNMENT);
+		// Check that offset is strictly monotonic to prevent infinite loop
+		if (next_offset <= offset) {
+			ERROR("ERROR: corrupted CBFS file header at 0x%x.\n", offset);
+			break;
+		}
+		offset = next_offset;
    }
    media->close(media);
    LOG("WARNING: '%s' not found.\n", name);
@@ -309,7 +315,14 @@
    		return NULL;
    	}
    	if (ntohl(attr.tag) != tag) {
-			offset += ntohl(attr.len);
+			uint32_t next_offset = offset + ntohl(attr.len);
+			// Check that offset is strictly monotonic to prevent infinite loop
+			if (next_offset <= offset) {
+				ERROR("ERROR: corrupted CBFS attribute at 0x%x.\n", offset);
+				m->close(m);
+				return NULL;
+			}
+			offset = next_offset;
    		continue;
    	}
    	ret = m->map(m, offset, ntohl(attr.len));

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39177 )

Change subject: libpayload: cbfs: fix infinite loop in cbfs_get_{handle,attr}
......................................................................

Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1025
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1024
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1023

Please note: This test is under development and might not be accurate at all!