diff --git a/storage/backup.c b/storage/backup.c index 2842eb7..9ab3de7 100644 --- a/storage/backup.c +++ b/storage/backup.c @@ -36,9 +36,15 @@ static backup_key_t to_backup_key(const key_record_t *k) { static key_record_t to_key_record(const backup_key_t *b) { key_record_t k; - k.id = b->id; - k.is_enabled = b->is_enabled; - k.is_admin = b->is_admin; + k.id = b->id; + // The backup blob is untrusted input: its is_enabled/is_admin bytes may hold + // any value, so read them as raw bytes and canonicalise. Loading a `bool` + // whose object representation is not 0/1 is undefined behaviour. + 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); k.created_at = b->created_at; k.is_checksum_valid = true; memcpy(k.name, b->name, sizeof(k.name)); diff --git a/test/harness_storage.c b/test/harness_storage.c index fd929f3..a74bd60 100644 --- a/test/harness_storage.c +++ b/test/harness_storage.c @@ -28,6 +28,7 @@ #include "pico/stdlib.h" /* PICO_FLASH_SIZE_BYTES, PICO_OK */ #include "backup.h" +#include "lfs_util.h" /* lfs_crc: matches backup.c's whole-backup checksum */ #include "storage.h" /* Must match storage.c's private layout constants. */ @@ -250,6 +251,34 @@ int main(void) { assert(backup_import(backup, (size_t)elen) == true); assert(storage_key_list(list, 16) == 0); + /* --- UBSan regression: non-bool flag byte in an otherwise-valid blob --- */ + /* to_key_record() must not load is_enabled/is_admin straight into a `bool`: + * a crafted import blob can carry any byte there, and loading a bool whose + * object representation is not 0/1 is undefined behaviour (caught by + * -fsanitize=undefined). Build a valid 1-key blob, poke a non-bool flag + * byte, refresh the header CRC so the blob still validates, and import it. + * The import must succeed and the flag must read back canonicalised. */ + { + key_record_t seed = make_key(9, "ub", true, false, 1234, 0x40); + assert(storage_key_save(&seed) == true); + uint8_t craft[sizeof backup]; + int clen = backup_export(craft, sizeof craft); + assert((size_t)clen == sizeof(backup_header_t) + sizeof(backup_key_t)); + assert(storage_key_delete(9) == true); + + backup_key_t *bk = (backup_key_t *)(craft + sizeof(backup_header_t)); + *(uint8_t *)&bk->is_admin = 67; /* non-bool byte in an otherwise-valid record */ + backup_header_t *bh = (backup_header_t *)craft; + bh->checksum = lfs_crc(0xFFFFFFFF, bk, sizeof(backup_key_t)); + + assert(backup_import(craft, (size_t)clen) == true); + key_record_t g; + assert(storage_key_get(9, &g) == true); + assert(g.is_admin == true); /* canonicalised: exactly 1, not 67 */ + assert(g.is_enabled == true); /* untouched flag survives */ + assert(storage_key_delete(9) == true); + } + printf("storage OK\n"); return 0; }