Skip to content

fix(secure-boot): build the secure boot module, and stop it booting plaintext - #72

Merged
srpatcha merged 2 commits into
embeddedos-org:masterfrom
AshrafAhmed9:fix-secure-boot-orphaned
Sep 1, 2026
Merged

fix(secure-boot): build the secure boot module, and stop it booting plaintext#72
srpatcha merged 2 commits into
embeddedos-org:masterfrom
AshrafAhmed9:fix-secure-boot-orphaned

Conversation

@AshrafAhmed9

@AshrafAhmed9 AshrafAhmed9 commented Aug 30, 2026

Copy link
Copy Markdown

core/secure_boot.c is not in eboot_core's source list. It has never been
compiled, nothing calls eos_secure_boot(), and it has no tests — 238 lines
implementing the verification chain the bootloader exists to perform:

$ for f in core/*.c; do grep -q "$f" CMakeLists.txt || echo "orphaned: $f"; done
orphaned: core/fdt_loader.c
orphaned: core/secure_boot.c

Wiring it in also needs core/rollback.c, which it calls and which is missing
from the list too. (#69 adds rollback.c for its own reasons; the line is the
same either way.)

Compiling it turned up a policy doing the opposite of its name.
require_encryption is documented in eos_secure_boot.h as "Enforce AES-GCM
decryption"
. Step 6 read:

if (cfg->require_encryption && (hdr.flags & EOS_IMG_FLAG_ENCRYPTED)) {
    ...
    return EOS_SBOOT_ERR_DECRYPT;
}

An encrypted image reaches the body and is refused, because decryption isn't
implemented yet. A plaintext image fails the second half of the condition,
falls past the gate, and boots.

That branch was also the only failure return in the function that didn't call
attest_record(), so a refusal left no measurement in the attestation log.

The change

  • core/secure_boot.c and core/rollback.c join eboot_core
  • require_encryption alone decides whether the gate applies; an image without
    EOS_IMG_FLAG_ENCRYPTED is refused rather than booted
  • the refusal is attested, like every other branch in the function

Testing

tests/unit/test_secure_boot.c is new — the first coverage this module has had.
It follows the simulated-flash pattern the other unit tests use, and pins four
behaviours: a plaintext image under require_encryption is rejected, an
encrypted one is rejected while decryption is unimplemented, a plaintext image
still boots when the policy doesn't ask for encryption, and the refusal is
recorded.

Against the current secure_boot.c the first case fails, because the image
boots:

  test_plaintext_image_rejected_when_encryption_required [FAIL]
  test_secure_boot.c:154: rc == EOS_SBOOT_ERR_DECRYPT

With the change:

  test_plaintext_image_rejected_when_encryption_required [PASS]
  test_encrypted_image_rejected_while_decrypt_unimplemented [PASS]
  test_plaintext_image_boots_when_encryption_not_required [PASS]
  test_decrypt_failure_is_attested                   [PASS]
4 passed

Full suite, macOS 15 / Apple Clang: 18/18, up from 17.

Master doesn't compile right now — core/recovery.c has a duplicate slot_size
and core/image_verify.c a conflicting eos_crc32, which #69 repairs. I ran
the suite with #69 applied locally to get the numbers above; this branch touches
neither of those files.

While reading the source list I noticed core/boot_log.c is listed twice, and
core/fdt_loader.c is orphaned the same way secure_boot.c was. Both left
alone here as separate questions.

srpatcha
srpatcha previously approved these changes Aug 30, 2026

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a real secure-boot bypass and the fix is right. I verified both halves of
the title rather than taking them on trust.

"build the secure boot module"

secure_boot.c was referenced zero times by any CMakeLists.txt. It was not
compiled, not linked, not tested. Whatever it said about policy had no bearing on
what the bootloader did.

"stop it booting plaintext"

The gate read:

if (cfg->require_encryption && (hdr.flags & EOS_IMG_FLAG_ENCRYPTED)) {
    /* decrypt ... */
}

The decrypt path ran only for images that are encrypted. A plaintext image on
a device configured to require encryption matched neither arm, fell past the
block, and booted. The one image the policy exists to reject was the one image it
let through.

Your structure separates the two questions correctly — first "does policy demand
encryption", then "is this image encrypted":

if (cfg->require_encryption) {
    if (!(hdr.flags & EOS_IMG_FLAG_ENCRYPTED)) {
        return EOS_SBOOT_ERR_DECRYPT;
    }
    ...
}

Verified

Applied on a base where master's build is repaired:

0 build errors
100% tests passed, 0 tests failed out of 17

Secure boot policy tests
  test_plaintext_image_rejected_when_encryption_required      [PASS]
  test_encrypted_image_rejected_while_decrypt_unimplemented   [PASS]
  test_plaintext_image_boots_when_encryption_not_required     [PASS]
  test_decrypt_failure_is_attested                            [PASS]

Then I reverted only the condition, back to require_encryption && ENCRYPTED:

test_plaintext_image_rejected_when_encryption_required
  [FAIL] test_secure_boot.c:154: rc == EOS_SBOOT_ERR_DECRYPT

So the test genuinely pins the bypass rather than describing it.

The third test is the one I would have asked for if it were missing:
test_plaintext_image_boots_when_encryption_not_required proves the fix did not
over-correct into refusing plaintext on devices that never required encryption.
And test_encrypted_image_rejected_while_decrypt_unimplemented is the right call
— an encrypted image must not boot while decryption is a stub, and "we cannot
decrypt this" has to fail closed like any other unreadable input.

One note for when you rebase

eBoot#71 namespaces every test target in tests/ as eboot_*, because eos and
eBoot both defined test_crypto and test_multicore and ebuild composes them
into one CMake project. Its configure-time guard caught your new target while I
was verifying:

CMake Error: Test target 'test_secure_boot' is not namespaced.  Name it
'eboot_test_secure_boot' ...

So after #71 lands this needs eboot_test_secure_boot as the target name. The
add_test(NAME test_secure_boot ...) label stays as it is — the collision is
between targets, not test names, so your ctest output does not change.

Approving. Merge order here is #58, then #71, then this.

@AshrafAhmed9

Copy link
Copy Markdown
Author

@srpatcha both follow-ups from your review are pushed, see the two new commits (the eboot_test_secure_boot rename here, and the poll-granularity note on eos#96). I can't re-request a review as an outside contributor, hence the ping.

srpatcha
srpatcha previously approved these changes Aug 30, 2026

@srpatcha srpatcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-approving — my earlier review was auto-dismissed when you pushed the
namespacing commit, not withdrawn.

eboot_test_secure_boot is exactly right, and keeping add_test(NAME test_secure_boot ...) unchanged is the detail I was hoping you would catch: the
collision CMake rejects is between targets, so the ctest output stays as it was.

Re-verified on #71's branch, which carries the namespace guard:

0 build errors
100% tests passed, 0 tests failed out of 17

Everything in my previous review stands. The important part, for anyone reading
this later: secure_boot.c was compiled by nothing at all, and the policy gate
read require_encryption && (flags & ENCRYPTED) — so the decrypt path ran only
for images that were encrypted, and a plaintext image on a device requiring
encryption fell straight past it and booted. Reverting just that condition turns
your test red:

test_plaintext_image_rejected_when_encryption_required
  [FAIL] test_secure_boot.c:154: rc == EOS_SBOOT_ERR_DECRYPT

Merge order remains #58, then #71, then this.

…laintext

core/secure_boot.c is not in eboot_core's source list, so it has never
been compiled, has no callers, and has no tests. It is the module that
implements the verification chain the bootloader exists to perform.

Wiring it in required core/rollback.c too, which it calls and which is
also missing from the list.

Compiling it turned up a policy that does the opposite of its name.
require_encryption is documented as "Enforce AES-GCM decryption", and
step 6 read:

    if (cfg->require_encryption && (hdr.flags & EOS_IMG_FLAG_ENCRYPTED))

An encrypted image reaches the body and is refused, because decryption is
not implemented yet. A plaintext image fails the second half of the
condition, falls past the gate, and boots. So the single image the policy
exists to reject was the one case that skipped the check. Now
require_encryption alone decides whether the gate applies, and an image
without the flag is refused.

That path was also the only failure return in the function that did not
call attest_record(), so a refusal left no measurement behind. It records
one now, like every other branch.

Adds tests/unit/test_secure_boot.c, the first coverage this module has
had: a plaintext image under require_encryption is rejected, an encrypted
one is rejected while decryption is unimplemented, a plaintext image boots
when the policy does not ask for encryption, and the refusal is attested.

Against the current secure_boot.c the first case fails — the image boots.
With the fix all four pass, and the suite goes from 17 to 18.

core/fdt_loader.c is orphaned from the build in the same way. Left alone
here; it is a separate module and a separate question.
eos and eBoot both define test_crypto and test_multicore, and ebuild
composes them into one CMake project, so embeddedos-org#71 namespaces every eBoot test
target as eboot_*. Adopt that convention here now rather than after embeddedos-org#71
lands, so the two merge in either order.

The add_test() name stays test_secure_boot: the collision is between
targets, not test names, so ctest output is unchanged.
@AshrafAhmed9
AshrafAhmed9 force-pushed the fix-secure-boot-orphaned branch from 407c6db to c26c1d8 Compare August 30, 2026 22:43
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 1, 2026
…ul boot

cfg.lock_debug asks for SWD/JTAG to be closed before the verified image runs.
Step 7 of eos_secure_boot() honoured that request like this:

    if (cfg->lock_debug) {
        eos_secure_boot_lock_debug();
    }

    /* ---- Step 8: Record successful attestation ---- */
    attest_record(2, hdr.image_version, hdr.hash, NULL, EOS_SBOOT_OK);
    return EOS_SBOOT_OK;

eos_secure_boot_lock_debug() returned void and discarded the result of the OTP
write that actually blows the fuse. So when the write failed, boot continued,
attestation recorded EOS_SBOOT_OK, and the device ran the image with its debug
port open -- the exact condition the policy existed to prevent, reported as a
clean secure boot.

The interesting case is not a flaky fuse. eos_hal_otp_write() returns
EOS_ERR_NOT_SUPPORTED when the board provides no otp_write hook at all, so on
every such board `lock_debug: true` was silently a no-op. That is the default
configuration, not an edge case.

eos_secure_boot_lock_debug() now returns int, and a caller that asked for the
lock and did not get it fails with EOS_SBOOT_ERR_POLICY -- a code that already
existed for exactly this ("Boot policy violation") -- with the failure recorded
in the attestation log rather than a success.

Why this was never observable: core/secure_boot.c is not in CMakeLists.txt.
The module has never been compiled, so this path could not run and could not be
tested. Added the one line that builds it -- the same line embeddedos-org#72 adds, written
identically so whichever lands first leaves the other a trivial rebase.

tests/unit/test_secure_boot_policy.c covers the three outcomes: the fuse
written, the write failing, and a board with no otp_write. Kept in its own file
so it does not collide with the test_secure_boot.c embeddedos-org#72 introduces.

Against master the new test does not compile -- `invalid operands to binary
expression ('void' and 'int')` -- because there is no result to check. That is
the defect stated as a compile error.

Verified on this branch: build clean, ctest 20/20, pytest 30 passed.

Stacked on embeddedos-org#77 (master's test suite does not compile without it).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@srpatcha
srpatcha merged commit 2955938 into embeddedos-org:master Sep 1, 2026
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 1, 2026
…ul boot

cfg.lock_debug asks for SWD/JTAG to be closed before the verified image runs.
Step 7 of eos_secure_boot() honoured that request like this:

    if (cfg->lock_debug) {
        eos_secure_boot_lock_debug();
    }

    /* ---- Step 8: Record successful attestation ---- */
    attest_record(2, hdr.image_version, hdr.hash, NULL, EOS_SBOOT_OK);
    return EOS_SBOOT_OK;

eos_secure_boot_lock_debug() returned void and discarded the result of the OTP
write that actually blows the fuse. So when the write failed, boot continued,
attestation recorded EOS_SBOOT_OK, and the device ran the image with its debug
port open -- the exact condition the policy existed to prevent, reported as a
clean secure boot.

The interesting case is not a flaky fuse. eos_hal_otp_write() returns
EOS_ERR_NOT_SUPPORTED when the board provides no otp_write hook at all, so on
every such board `lock_debug: true` was silently a no-op. That is the default
configuration, not an edge case.

eos_secure_boot_lock_debug() now returns int, and a caller that asked for the
lock and did not get it fails with EOS_SBOOT_ERR_POLICY -- a code that already
existed for exactly this ("Boot policy violation") -- with the failure recorded
in the attestation log rather than a success.

Why this was never observable: core/secure_boot.c is not in CMakeLists.txt.
The module has never been compiled, so this path could not run and could not be
tested. Added the one line that builds it -- the same line embeddedos-org#72 adds, written
identically so whichever lands first leaves the other a trivial rebase.

tests/unit/test_secure_boot_policy.c covers the three outcomes: the fuse
written, the write failing, and a board with no otp_write. Kept in its own file
so it does not collide with the test_secure_boot.c embeddedos-org#72 introduces.

Against master the new test does not compile -- `invalid operands to binary
expression ('void' and 'int')` -- because there is no result to check. That is
the defect stated as a compile error.

Verified on this branch: build clean, ctest 20/20, pytest 30 passed.

Stacked on embeddedos-org#77 (master's test suite does not compile without it).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Kartikey1306 added a commit to Kartikey1306/eBoot that referenced this pull request Sep 3, 2026
…ul boot

cfg.lock_debug asks for SWD/JTAG to be closed before the verified image runs.
Step 7 of eos_secure_boot() honoured that request like this:

    if (cfg->lock_debug) {
        eos_secure_boot_lock_debug();
    }

    /* ---- Step 8: Record successful attestation ---- */
    attest_record(2, hdr.image_version, hdr.hash, NULL, EOS_SBOOT_OK);
    return EOS_SBOOT_OK;

eos_secure_boot_lock_debug() returned void and discarded the result of the OTP
write that actually blows the fuse. So when the write failed, boot continued,
attestation recorded EOS_SBOOT_OK, and the device ran the image with its debug
port open -- the exact condition the policy existed to prevent, reported as a
clean secure boot.

The interesting case is not a flaky fuse. eos_hal_otp_write() returns
EOS_ERR_NOT_SUPPORTED when the board provides no otp_write hook at all, so on
every such board `lock_debug: true` was silently a no-op. That is the default
configuration, not an edge case.

eos_secure_boot_lock_debug() now returns int, and a caller that asked for the
lock and did not get it fails with EOS_SBOOT_ERR_POLICY -- a code that already
existed for exactly this ("Boot policy violation") -- with the failure recorded
in the attestation log rather than a success.

Why this was never observable: core/secure_boot.c is not in CMakeLists.txt.
The module has never been compiled, so this path could not run and could not be
tested. Added the one line that builds it -- the same line embeddedos-org#72 adds, written
identically so whichever lands first leaves the other a trivial rebase.

tests/unit/test_secure_boot_policy.c covers the three outcomes: the fuse
written, the write failing, and a board with no otp_write. Kept in its own file
so it does not collide with the test_secure_boot.c embeddedos-org#72 introduces.

Against master the new test does not compile -- `invalid operands to binary
expression ('void' and 'int')` -- because there is no result to check. That is
the defect stated as a compile error.

Verified on this branch: build clean, ctest 20/20, pytest 30 passed.

Stacked on embeddedos-org#77 (master's test suite does not compile without it).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants