Change in coreboot[master]: libpayload: Fix out-of-bounds read

[Alexandre Rebert (Code Review)]

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

Change subject: libpayload: Fix out-of-bounds read
......................................................................

libpayload: Fix out-of-bounds read

Fix an out-of-bounds read in the LZMA decoder which happens when the src
buffer is too small to contain the 13-byte LZMA header.

Change-Id: Ie442f82cd1abcf7fa18295e782cccf26a7d30079
Signed-off-by: Alex Rebert 
---
M payloads/libpayload/liblzma/lzma.c
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/39033/1

diff --git a/payloads/libpayload/liblzma/lzma.c b/payloads/libpayload/liblzma/lzma.c
index 57a8b3a..132ae84 100644
--- a/payloads/libpayload/liblzma/lzma.c
+++ b/payloads/libpayload/liblzma/lzma.c
@@ -28,6 +28,11 @@
    SizeT mallocneeds;
    unsigned char *scratchpad;

+	if (srcn < data_offset) {
+		printf("lzma: Input too small.")
+		return 0;
+	}
+
    memcpy(properties, src, LZMA_PROPERTIES_SIZE);
    memcpy(&outSize, src + LZMA_PROPERTIES_SIZE, sizeof(outSize));
    if (outSize > dstn)

[Alexandre Rebert (Code Review)]

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

Change subject: libpayload: Fix out-of-bounds read
......................................................................

libpayload: Fix out-of-bounds read

Fix an out-of-bounds read in the LZMA decoder which happens when the src
buffer is too small to contain the 13-byte LZMA header.

Change-Id: Ie442f82cd1abcf7fa18295e782cccf26a7d30079
Signed-off-by: Alex Rebert 
---
M payloads/libpayload/liblzma/lzma.c
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/39033/2

[Paul Menzel (Code Review)]

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

Change subject: libpayload: Fix out-of-bounds read
......................................................................

Patch Set 2: Code-Review+1

Please send this fix also upstream.

Did some tool find this? If so, please mention `Found-by: …` in the commit message.

[Alexandre Rebert (Code Review)]

Hello Julius Werner, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39033

to look at the new patch set (#3).

Change subject: libpayload: Fix out-of-bounds read
......................................................................

libpayload: Fix out-of-bounds read

Fix an out-of-bounds read in the LZMA decoder which happens when the src
buffer is too small to contain the 13-byte LZMA header.

Change-Id: Ie442f82cd1abcf7fa18295e782cccf26a7d30079
Signed-off-by: Alex Rebert 
Found-by: Mayhem
---
M payloads/libpayload/liblzma/lzma.c
1 file changed, 5 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/39033/3

[Alexandre Rebert (Code Review)]

Alexandre Rebert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39033 )

Change subject: libpayload: Fix out-of-bounds read
......................................................................

Patch Set 3:
> Please send this fix also upstream.

Is there any docs on how to send a patch upstream? It's my first time contributing to coreboot, so I'm still learning the ropes.
> Did some tool find this? If so, please mention `Found-by: …` in the commit message.

The out-of-bounds read was found by Mayhem, a proprietary fuzzing tool. I amended the commit message to mention it. There are a few more issues that were found that I'm planning to patch after this one.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39033 )

Change subject: libpayload: Fix out-of-bounds read
......................................................................

Patch Set 3: Code-Review+2

Wanna fix the same issue in src/lib/lzma.c as well?

[Alexandre Rebert (Code Review)]

Alexandre Rebert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39033 )

Change subject: libpayload: Fix out-of-bounds read
......................................................................

Patch Set 3:
> Patch Set 3: Code-Review+2
>
> Wanna fix the same issue in src/lib/lzma.c as well?

Definitely! I have another patch lined up once that one is submitted. Happy to include it in this patch set if you think that'd be better.

[Paul Menzel (Code Review)]

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

Change subject: libpayload: Fix out-of-bounds read
......................................................................

Patch Set 3: Code-Review+1

[Patrick Georgi (Code Review)]

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

Change subject: libpayload: Fix out-of-bounds read
......................................................................

libpayload: Fix out-of-bounds read

Fix an out-of-bounds read in the LZMA decoder which happens when the src
buffer is too small to contain the 13-byte LZMA header.

Change-Id: Ie442f82cd1abcf7fa18295e782cccf26a7d30079
Signed-off-by: Alex Rebert 
Found-by: Mayhem
Reviewed-on: https://review.coreboot.org/c/coreboot/+/39033
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
Reviewed-by: Paul Menzel 
---
M payloads/libpayload/liblzma/lzma.c
1 file changed, 5 insertions(+), 0 deletions(-)

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

diff --git a/payloads/libpayload/liblzma/lzma.c b/payloads/libpayload/liblzma/lzma.c
index 57a8b3a..1845afc 100644
--- a/payloads/libpayload/liblzma/lzma.c
+++ b/payloads/libpayload/liblzma/lzma.c
@@ -28,6 +28,11 @@
    SizeT mallocneeds;
    unsigned char *scratchpad;

+	if (srcn < data_offset) {
+		printf("lzma: Input too small.\n");
+		return 0;
+	}
+
    memcpy(properties, src, LZMA_PROPERTIES_SIZE);
    memcpy(&outSize, src + LZMA_PROPERTIES_SIZE, sizeof(outSize));
    if (outSize > dstn)

[Patrick Georgi (Code Review)]

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

Change subject: libpayload: Fix out-of-bounds read
......................................................................

Patch Set 4:
> Patch Set 3:
> The out-of-bounds read was found by Mayhem, a proprietary fuzzing tool. I amended the commit message to mention it. There are a few more issues that were found that I'm planning to patch after this one.

I'm curious, is it the one described in https://spectrum.ieee.org/computing/software/mayhem-the-machine-that-finds-s...

[Alexandre Rebert (Code Review)]

Alexandre Rebert has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39033 )

Change subject: libpayload: Fix out-of-bounds read
......................................................................

Patch Set 4:
> I'm curious, is it the one described in https://spectrum.ieee.org/computing/software/mayhem-the-machine-that-finds-s...

Yeah, that's the one! I'm a co-founder of ForAllSecure, the company behind Mayhem. We regularly use Mayhem on open source software we care about. In this case, I was curious about coreboot and its secure boot implementation. Big fan of having easily auditable open source firmware!

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39033 )

Change subject: libpayload: Fix out-of-bounds read
......................................................................

Patch Set 4:
> In this case, I was curious about coreboot and its secure boot implementation. Big fan of having easily auditable open source firmware!

Note that the core of the secure boot stuff is implemented in the separate "vboot" project, which is hosted at https://chromium.googlesource.com/chromiumos/platform/vboot_reference and checked out as a submodule under 3rdparty/vboot in your coreboot tree. It has a cleanly delimited API which might make it easier to analyze. Always appreciate extra eyes and analyzers on our code! (And if you do find something that's actually security-relevant, note that it might qualify for the rewards program at https://www.google.com/about/appsecurity/chrome-rewards/index.html.)