Restore the stack canary #1273

bigspider · 2025-10-13T15:35:49Z

Description

Restores the functionality of HAVE_BOLOS_APP_STACK_CANARY.

Initializes the canary on os_io_init, and checks it on receive/sends.

This restores a behaviour similar to how it used to work in the old io_exchange: all apps call os_io_init in the preamble (the legacy code used io_seproxyhal_init instead), and we check the canary every time the io send/receive functions are called.

Since os_io_init, os_io_rx_evt and os_io_tx_cmd in os_io.c are only used if USE_OS_IO_STACK is not defined, the same initialization and checks are also implemented for the analogous weak functions defined in syscalls.c.

Changes include

Bugfix (non-breaking change that solves an issue)
New feature (non-breaking change that adds functionality)
Breaking change (change that is not backwards-compatible and/or changes current functionality)
Tests
Documentation
Other (for changes that might not fit in any category)

Auto cherry-pick in API_LEVEL

[x] TARGET_API_LEVEL: API_LEVEL_24
[x] TARGET_API_LEVEL: API_LEVEL_25

codecov-commenter · 2025-10-13T15:36:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.41%. Comparing base (b68cbcf) to head (a4c52f6).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1273   +/-   ##
=======================================
  Coverage   62.41%   62.41%           
=======================================
  Files          14       14           
  Lines        1868     1868           
=======================================
  Hits         1166     1166           
  Misses        702      702

Flag	Coverage Δ
unittests	`62.41% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bigspider · 2025-10-14T08:40:20Z

I verified on a locally built clone of the boilerplate:

the app crashes before trying to send theresponse if the canary is overwritten and DEFINES += HAVE_BOLOS_APP_STACK_CANARY is added to the Makefile
the app doesn't crash if the canary is overwritten without HAVE_BOLOS_APP_STACK_CANARY.

This will allow it to be accessible from several source files.

This restores a behaviour similar to how it used to work in the old io_exchange: all apps call os_io_init in the preamble (the legacy code used io_seproxyhal_init instead), and we check the canary every time the io send/receive functions are called. Since os_io_init, os_io_rx_evt and os_io_tx_cmd in os_io.c are only used if USE_OS_IO_STACK is not defined, the same initialization and checks are also implemented for the analogous weak functions defined in syscalls.c.

This allows to exit with a recognizable value.

bigspider · 2025-10-14T09:05:40Z

In a4c52f6, I replaced os_reset with os_sched_exit, which allows to cleanly exit the app with a recognizable exit value (at least on speculos; on the real device it exits to the dashboard with no response). The constant 42 was chosen for obvious reasons.

fbeutin-ledger · 2025-10-14T09:09:53Z

io_legacy/src/os_io_legacy.c

-#ifdef HAVE_BOLOS_APP_STACK_CANARY
-#define APP_STACK_CANARY_MAGIC 0xDEAD0031
-#endif  // HAVE_BOLOS_APP_STACK_CANARY


We'll need to check if this causes issues with some applications still using legacy IO and the canary

Nothing should change for them. These are moved in os_io.h, which is imported by os_io_legacy.
The only difference is that this define is now visible in anything that imports os_io.h, so it could only conflict if they have a different define with the same name.

fbeutin-ledger · 2025-10-14T09:10:29Z

src/syscalls.c

+#ifdef HAVE_BOLOS_APP_STACK_CANARY
+    // if the canary is corrupted, reset the device
+    if (app_stack_canary != APP_STACK_CANARY_MAGIC) {
+        os_sched_exit(APP_STACK_CANARY_CORRUPTED_EXIT_VALUE);
+    }
+#endif


Maybe we can factorize this snippet

I was worried about introducing an additional function because if we know the stack is already corrupted, we should use it as little as possible.
Perhaps we can use a macro.

io_legacy/src/os_io_legacy.c

iartemov-ledger · 2025-10-14T09:33:02Z

io/src/os_io.c

+#ifdef HAVE_BOLOS_APP_STACK_CANARY
+    // if the canary is corrupted, reset the device
+    if (app_stack_canary != APP_STACK_CANARY_MAGIC) {
+        os_sched_exit(APP_STACK_CANARY_CORRUPTED_EXIT_VALUE);


In the perfect world it should be automatically tested against an app with and without legacy IO stack.
But we do not seem to have an infrastructure for this.
Might be https://github.com/LedgerHQ/speculos/tree/master/tests/c, but also it does not seem to fit.

io/src/os_io.c

jarevalo-ledger · 2025-10-14T10:16:41Z

src/syscalls.c

+#ifdef HAVE_BOLOS_APP_STACK_CANARY
+    app_stack_canary = APP_STACK_CANARY_MAGIC;
+#endif  // HAVE_BOLOS_APP_STACK_CANARY
+


I would prefer to not modify the syscall APIs.
Why not init in an early app start entrypoint?

the syscalls APIs are the only place used for io for most apps (that is, after io revamp and defining USE_OS_IO_STACK). So even if you find a different place for initialization, you still need to modify syscalls.c to check for canary corruption on os_io_rx_evt/os_io_tx_cmd.

(I chose os_io_init because I checked that both the C and the Rust SDK app preamble calls it before any real application code.)

Syscalls are not called directly by apps, there is SDK functions that can be used. Let's discuss to find a better solution.

bboilot-ledger

LGTM from a security standpoint. I agree with other comments especially about modifying syscalls APIs. Unfortunately, I do not see neither where we could put those snippet so it used by apps by default without any snippet needed in the app's codebase.
Is there any impact regarding apps built with zxlib/sdk?

bigspider changed the title ~~Stack canary~~ Restore the stack canary Oct 13, 2025

bigspider force-pushed the stack_canary branch from e51c779 to 77c50d1 Compare October 13, 2025 15:37

bigspider marked this pull request as ready for review October 14, 2025 08:38

bigspider added 2 commits October 14, 2025 10:40

Move APP_STACK_CANARY_MAGIC constant to os_io.h

c831c4d

This will allow it to be accessible from several source files.

bigspider force-pushed the stack_canary branch from 99b04dd to dbdf70b Compare October 14, 2025 08:41

Use os_sched_exit instead of os_reset when the stack canary is corrupted

a4c52f6

This allows to exit with a recognizable value.

bigspider force-pushed the stack_canary branch from dbdf70b to a4c52f6 Compare October 14, 2025 09:01

bigspider requested review from bboilot-ledger, fbeutin-ledger, jarevalo-ledger and tdejoigny-ledger October 14, 2025 09:08

fbeutin-ledger reviewed Oct 14, 2025

View reviewed changes

iartemov-ledger reviewed Oct 14, 2025

View reviewed changes

jarevalo-ledger reviewed Oct 14, 2025

View reviewed changes

bboilot-ledger reviewed Oct 16, 2025

View reviewed changes

Restore the stack canary #1273

Are you sure you want to change the base?

Restore the stack canary #1273

Uh oh!

Conversation

bigspider commented Oct 13, 2025

Description

Changes include

Auto cherry-pick in API_LEVEL

Uh oh!

codecov-commenter commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bigspider commented Oct 14, 2025

Uh oh!

bigspider commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fbeutin-ledger Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

bigspider Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

fbeutin-ledger Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

bigspider Oct 14, 2025 • 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!

Uh oh!

iartemov-ledger Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jarevalo-ledger Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

bigspider Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

jarevalo-ledger Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

bboilot-ledger left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Oct 13, 2025 •

edited

Loading

bigspider commented Oct 14, 2025 •

edited

Loading

bigspider Oct 14, 2025 •

edited

Loading