Skip to content

Fix undefined behaviour reading bool flags from untrusted backup blobs#8

Open
d33mobile wants to merge 1 commit into
hakierspejs:masterfrom
d33mobile:fix/backup-nonbool-flags
Open

Fix undefined behaviour reading bool flags from untrusted backup blobs#8
d33mobile wants to merge 1 commit into
hakierspejs:masterfrom
d33mobile:fix/backup-nonbool-flags

Conversation

@d33mobile

Copy link
Copy Markdown

The bug

to_key_record() in storage/backup.c copies an imported backup_key_t into a key_record_t by loading the is_enabled / is_admin fields (declared bool) directly:

k.is_enabled = b->is_enabled;
k.is_admin   = b->is_admin;

The backup blob is untrusted input: it arrives over the serial import-keys command as a base64 payload. Its flag bytes can hold any value, not just 0 or 1. Loading a bool whose object representation is not 0/1 is undefined behaviour in C. A crafted backup whose flag byte is e.g. 67 but whose header CRC is valid reaches this load and trips the sanitizer:

storage/backup.c:41: runtime error: load of value 67, which is not a valid value for type '_Bool'

The fix

Read the two flag bytes as raw uint8_t via memcpy and canonicalise with != 0, so the resulting bool always has a valid representation:

uint8_t enabled_raw, admin_raw;
memcpy(&enabled_raw, &b->is_enabled, sizeof(enabled_raw));
memcpy(&admin_raw, &b->is_admin, sizeof(admin_raw));
k.is_enabled = (enabled_raw != 0);
k.is_admin   = (admin_raw != 0);

Regression test

Added a case to the existing test/harness_storage.c (RAM-backed littlefs harness that runs storage.c + backup.c under make -C test asan, i.e. -fsanitize=address,undefined). It builds a valid 1-key backup, pokes a non-bool byte (67) into a flag, refreshes the header CRC so the blob still validates, imports it, and asserts the import succeeds and the flag reads back canonicalised (exactly 1). With the fix make -C test asan is green; temporarily reverting the fix makes the same test fail with the UBSan not a valid value for type '_Bool' error, confirming the guard works.

Severity

Low practical impact: a non-canonical bool byte is still truthy, so there is no auth bypass or logic change from this path. The value of the fix is removing the undefined behaviour and canonicalising the stored flags.

Co-Authored-By: Claude Opus 4.8 (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.

1 participant