Container: Improve container component loading with bounds validation by vtcao297 · Pull Request #2687 · slimbootloader/slimbootloader

vtcao297 · 2026-04-27T20:43:41Z

Improvements to the container library component loading and decompression pathways:

Add bounded traversal functions for container header entry parsing with explicit checks
Implement explicit container bounds validation at registration time to reject malformed or incomplete container regions before processing
Add runtime input validation in decompressor metadata parsing to enforce minimum buffer sizes and NULL pointer checks in decompression GetInfo APIs
Improve flow robustness by validating component metadata before decompressor interaction
Refactor some extremely nested blocks for better readability

These changes strengthen boot-time security by enforcing defense-in-depth precondition validation across container parsing, component verification, and decompression initialization stages.

tsaikevin · 2026-04-29T01:17:10Z

+
+  // Check for overflow before addition
+  if ((CurrEntry->HashSize > (MAX_UINT64 - sizeof(COMPONENT_ENTRY) - CurrOffset)) ||
+      ((CurrOffset + sizeof(COMPONENT_ENTRY) + CurrEntry->HashSize) > HeaderLimit)) {


@vtcao297 line 164 and 165 are duplicated, the MAX value of HeaderLimit is MAX_UINT64 too. so line 165 is sufficient. fyi
btw, line 170 checking is equivalent(potentially dead code)

you're right, the first guard condition is kind of impossible since the worst-case sum is only ~ 2^32 + 267 which is much less 2^64 so it can never be true, over-defensive coding :). And yes 170 would then also be redundant as 165, so i'll just combine them. Thanks!

tsaikevin · 2026-04-29T18:53:22Z

+
+  HeaderLimit = ContainerHdr->DataOffset;
+  CurrOffset  = (UINT64)((UINT8 *)CurrEntry - (UINT8 *)ContainerHdr);
+  if (CurrOffset > HeaderLimit) {


@vtcao297 do we consider checking the "==" condition where no space left at CurrOffset?thx

I believe the == case is handled safely by the next block.

if (CurrOffset > HeaderLimit) rejects true out-of-range.

if ((HeaderLimit - CurrOffset) < sizeof (COMPONENT_ENTRY)) rejects “no room left” and partial room.

So when CurrOffset == HeaderLimit, remaining space is 0, which is < sizeof(COMPONENT_ENTRY), and it returns FALSE. Valid containers shouldn't be affected because a valid component entry must have at least sizeof(COMPONENT_ENTRY) bytes remaining.

tsaikevin · 2026-04-29T19:21:01Z

+    ScrLen = 0;
+  }
+
+  Status = DecompressGetInfo (CompressHdr->Signature, CompressHdr->Data,


@vtcao297 when line 1254 is true, DstLen and ScrLen are set 0, is it still necessary to call DecompressGetInfo in line 1259? thx

No I guess it doesn't, I was trying to flatten the previous codeblock with its nested if..elses. Thanks for catching this, it didn't manifest in my testing because the worse case scenario was just over allocation of additional 64K in the scratch buffer AllocBuf if the compression algo was LZMA. Will fix this soon

addressed in the latest revision.

tsaikevin · 2026-04-29T19:29:29Z

@vtcao297 since many changes merged in container library, it actually introduces some chance to break(Copilot suggests some but I'm unable to verify:( ) Suggest to run static analysis again after code merged. fyi

lsubash · 2026-04-29T19:36:10Z

    }
    for (Index = 0; Index < ContainerHdr->Count; Index++) {
-      CompEntry = (COMPONENT_ENTRY *)((UINT8 *)(CompEntry + 1) + CompEntry->HashSize);
+      if (!GetNextComponentEntryBounded (ContainerHdr, CompEntry, &CompEntry)) {


Looks CompEntry is repeated param. Its not passing next param.

In the old logic at ContainerLib.c, the code effectively did this:

-Read CompEntry->HashSize from the current entry.
-Compute the next pointer with raw pointer arithmetic.
-Only after that, compare the resulting offset against ContainerHdr->DataOffset.

That means it could still read fields from a partially out-of-bounds current entry before discovering the next pointer was invalid.

The replacement via GetNextComponentEntryBounded keeps the same core traversal rule:

next entry = current entry + sizeof(COMPONENT_ENTRY) + HashSize

but adds two security properties before returning the next pointer:

IsComponentEntryReadable ensures the fixed-size current entry header is fully readable inside the container header bounds before reading HashSize.

GetNextComponentEntryBounded ensures the computed next entry does not exceed DataOffset

Improvements to the container library component loading and decompression pathways: - Add bounded traversal functions for container header entry parsing with explicit checks - Implement explicit container bounds validation at registration time to reject malformed or incomplete container regions before processing - Add runtime input validation in decompressor metadata parsing to enforce minimum buffer sizes and NULL pointer checks in decompression GetInfo APIs - Improve flow robustness by validating component metadata before decompressor interaction - Refactor some extremely nested blocks for better readability These changes strengthen boot-time security by enforcing defense-in-depth precondition validation across container parsing, component verification, and decompression initialization stages. Signed-off-by: Vincent Cao <[email protected]>

A guard for FS->Ext2FsNumCylinder from commit 193129e was inadvertently removed due to commit 3e853aa. This addresses that regression but also improve on the original fix by decreasing caps the descriptor table allocation at 256 KB — comfortably within the ~3.6 (lowest) -> 22 (highest) configured MB pool budget for supported platforms. The original fix, the threshold is the wrong bound MAX_UINTN/sizeof(EXT2GD) = 288 trillion groups on 64-bit, and 67 million groups on 32-bit. That's not a meaningful filesystem limit — it only guards against a UINTN multiplication overflow, but not against a value that would potentially corrupt the pool allocator's UINT32 arithmetic. Signed-off-by: Vincent Cao <[email protected]>

vtcao297 · 2026-04-30T03:18:57Z

@vtcao297 since many changes merged in container library, it actually introduces some chance to break(Copilot suggests some but I'm unable to verify:( ) Suggest to run static analysis again after code merged. fyi

Kevin - I didn't realize static analysis is being run against PRs, can you point me to the link result so I can review if the items being caught are real and need to be addressed before merge.

bejeanmo · 2026-05-04T12:57:04Z

@vtcao297 since many changes merged in container library, it actually introduces some chance to break(Copilot suggests some but I'm unable to verify:( ) Suggest to run static analysis again after code merged. fyi

Kevin - I didn't realize static analysis is being run against PRs, can you point me to the link result so I can review if the items being caught are real and need to be addressed before merge.

It isn't. I think kevin is suggesting to check after its merged.

gdong1

Hi @vtcao297, thanks for the security hardening work here.

I think this change works perfectly fine, but the per-access bounded checks modify a significant amount of existing code and increase binary size.

How about an alternative approach: validate-once at registration time? Since SBL runs single-threaded and container memory can't be mutated between registration and use, a single structural validation pass at RegisterContainer() should be sufficient. This keeps all existing traversal code untouched.

I put together a quick implementation for reference:
[https://github.com/slimbootloader/slimbootloader/compare/master...gdong1:slimbootloader:validate-container-bounds]

Feel free to take a look and let me know if you'd like to adopt this approach or incorporate parts of it.

vtcao297 requested a review from a team as a code owner April 27, 2026 20:43

vtcao297 force-pushed the vtcao/harden_libcontainer branch 3 times, most recently from 4cff6b2 to 147d053 Compare April 28, 2026 00:56

bejeanmo approved these changes Apr 28, 2026

View reviewed changes

tsaikevin reviewed Apr 29, 2026

View reviewed changes

vtcao297 force-pushed the vtcao/harden_libcontainer branch from 147d053 to 531652a Compare April 29, 2026 04:40

tsaikevin reviewed Apr 29, 2026

View reviewed changes

lsubash reviewed Apr 29, 2026

View reviewed changes

vtcao297 added 2 commits April 29, 2026 20:09

vtcao297 force-pushed the vtcao/harden_libcontainer branch from 531652a to 5c4ee07 Compare April 30, 2026 03:14

gdong1 reviewed May 4, 2026

View reviewed changes

Conversation

vtcao297 commented Apr 27, 2026

Uh oh!

tsaikevin Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsaikevin commented Apr 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vtcao297 commented Apr 30, 2026

Uh oh!

bejeanmo commented May 4, 2026

Uh oh!

gdong1 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tsaikevin Apr 29, 2026 •

edited

Loading