From 92130f77ea7302c45045805470b49f9ffcf80745 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 03:28:56 +0200 Subject: [PATCH 1/2] fix(L6): reject non-canonical base64 padding Co-Authored-By: Claude Opus 4.8 (1M context) --- libs/base64/base64.c | 107 ++++++++++++++++++++++++++++++------------ test/harness_base64.c | 18 +++++++ 2 files changed, 96 insertions(+), 29 deletions(-) diff --git a/libs/base64/base64.c b/libs/base64/base64.c index 9b4a99b..056fa02 100644 --- a/libs/base64/base64.c +++ b/libs/base64/base64.c @@ -1,24 +1,23 @@ #include "base64.h" -static const char B64_CHARS[] = - "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; +static const char B64_CHARS[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; void base64_encode(const unsigned char *in, size_t in_len, char *out) { size_t i = 0, j = 0; while (i + 2 < in_len) { - out[j++] = B64_CHARS[(in[i] >> 2)]; - out[j++] = B64_CHARS[(in[i] & 0x03) << 4 | (in[i+1] >> 4)]; - out[j++] = B64_CHARS[(in[i+1] & 0x0F) << 2 | (in[i+2] >> 6)]; - out[j++] = B64_CHARS[(in[i+2] & 0x3F)]; + out[j++] = B64_CHARS[(in[i] >> 2)]; + out[j++] = B64_CHARS[(in[i] & 0x03) << 4 | (in[i + 1] >> 4)]; + out[j++] = B64_CHARS[(in[i + 1] & 0x0F) << 2 | (in[i + 2] >> 6)]; + out[j++] = B64_CHARS[(in[i + 2] & 0x3F)]; i += 3; } if (i < in_len) { out[j++] = B64_CHARS[(in[i] >> 2)]; if (i + 1 < in_len) { - out[j++] = B64_CHARS[(in[i] & 0x03) << 4 | (in[i+1] >> 4)]; - out[j++] = B64_CHARS[(in[i+1] & 0x0F) << 2]; + out[j++] = B64_CHARS[(in[i] & 0x03) << 4 | (in[i + 1] >> 4)]; + out[j++] = B64_CHARS[(in[i + 1] & 0x0F) << 2]; } else { out[j++] = B64_CHARS[(in[i] & 0x03) << 4]; out[j++] = '='; @@ -30,40 +29,90 @@ void base64_encode(const unsigned char *in, size_t in_len, char *out) { } static int b64_val(char c) { - if (c >= 'A' && c <= 'Z') return c - 'A'; - if (c >= 'a' && c <= 'z') return c - 'a' + 26; - if (c >= '0' && c <= '9') return c - '0' + 52; - if (c == '+') return 62; - if (c == '/') return 63; - if (c == '=') return 0; + if (c >= 'A' && c <= 'Z') + return c - 'A'; + if (c >= 'a' && c <= 'z') + return c - 'a' + 26; + if (c >= '0' && c <= '9') + return c - '0' + 52; + if (c == '+') + return 62; + if (c == '/') + return 63; + if (c == '=') + return 0; return -1; } int base64_decode(const char *in, size_t in_len, unsigned char *out, size_t out_cap) { - if (in_len % 4 != 0) return -1; + if (in_len % 4 != 0) + return -1; size_t out_len = 0; for (size_t i = 0; i < in_len; i += 4) { - int a = b64_val(in[i]); - int b = b64_val(in[i+1]); - int c = b64_val(in[i+2]); - int d = b64_val(in[i+3]); + int is_final = (i + 4 == in_len); + int pad2 = (in[i + 2] == '='); + int pad3 = (in[i + 3] == '='); + + /* '=' is only ever valid as trailing padding in the FINAL quad, at + * position 3 (one pad) or positions 2 and 3 (two pads). A pad in any + * non-final quad, or at position 2 without one at position 3, is + * non-canonical and rejected. */ + if ((pad2 || pad3) && !is_final) + return -1; + if (pad2 && !pad3) + return -1; + + /* '=' in positions 0 or 1 is never valid (b64_val('=') is 0, so the + * generic value check below would not catch it). */ + if (in[i] == '=' || in[i + 1] == '=') + return -1; - if (a < 0 || b < 0 || c < 0 || d < 0) return -1; + int a = b64_val(in[i]); + int b = b64_val(in[i + 1]); + if (a < 0 || b < 0) + return -1; - // Bound every write by the caller-supplied capacity: with no '=' in the - // final quad the decoder emits in_len/4*3 bytes, which the caller may + // Every write is bounded by the caller-supplied capacity: with no '=' in + // the final quad the decoder emits in_len/4*3 bytes, which the caller may // have under-sized by one (see ISSUES.md M5). Reject rather than write // past the buffer. - if (out_len >= out_cap) return -1; - out[out_len++] = (a << 2) | (b >> 4); - if (in[i+2] != '=') { - if (out_len >= out_cap) return -1; + if (pad2) { + /* "XX==": one output byte; the low 4 bits of the last data symbol + * (b) are unused and must be zero for canonical input. */ + if (b & 0x0F) + return -1; + if (out_len >= out_cap) + return -1; + out[out_len++] = (a << 2) | (b >> 4); + } else if (pad3) { + /* "XXX=": two output bytes; the low 2 bits of the last data symbol + * (c) are unused and must be zero for canonical input. */ + int c = b64_val(in[i + 2]); + if (c < 0) + return -1; + if (c & 0x03) + return -1; + if (out_len >= out_cap) + return -1; + out[out_len++] = (a << 2) | (b >> 4); + if (out_len >= out_cap) + return -1; out[out_len++] = (b << 4) | (c >> 2); - } - if (in[i+3] != '=') { - if (out_len >= out_cap) return -1; + } else { + int c = b64_val(in[i + 2]); + int d = b64_val(in[i + 3]); + if (c < 0 || d < 0) + return -1; + if (out_len >= out_cap) + return -1; + out[out_len++] = (a << 2) | (b >> 4); + if (out_len >= out_cap) + return -1; + out[out_len++] = (b << 4) | (c >> 2); + if (out_len >= out_cap) + return -1; out[out_len++] = (c << 6) | d; } } diff --git a/test/harness_base64.c b/test/harness_base64.c index 9d97f49..962ab1d 100644 --- a/test/harness_base64.c +++ b/test/harness_base64.c @@ -95,6 +95,24 @@ int main(void) { /* M5: decoded output exceeding out_cap must be rejected, not written OOB. */ reject_over_capacity(); + /* Non-canonical padding (L6): '=' is only valid as trailing pad in the + * final quad; the last data symbol's unused low bits must be zero. */ + assert(base64_decode("AA==AAAA", 8, scratch, sizeof scratch) == -1); /* pad in non-final quad */ + assert(base64_decode("AA=A", 4, scratch, sizeof scratch) == -1); /* pad at pos 2 not pos 3 */ + assert(base64_decode("=AAA", 4, scratch, sizeof scratch) == -1); /* pad at pos 0 */ + assert(base64_decode("A=AA", 4, scratch, sizeof scratch) == -1); /* pad at pos 1 */ + assert(base64_decode("AB==", 4, scratch, sizeof scratch) == -1); /* nonzero trailing bits */ + assert(base64_decode("AB=A", 4, scratch, sizeof scratch) == -1); /* pad at 2 w/o 3, nonzero */ + assert(base64_decode("ABC=", 4, scratch, sizeof scratch) == -1); /* 'C'=2, low 2 bits set */ + + /* Canonical padding still decodes correctly. */ + assert(base64_decode("AA==", 4, scratch, sizeof scratch) == 1 && scratch[0] == 0x00); + assert(base64_decode("Zg==", 4, scratch, sizeof scratch) == 1 && scratch[0] == 'f'); /* "f" */ + assert(base64_decode("Zm8=", 4, scratch, sizeof scratch) == 2 && + memcmp(scratch, "fo", 2) == 0); /* "fo" */ + assert(base64_decode("AAA=", 4, scratch, sizeof scratch) == 2 && scratch[0] == 0x00 && + scratch[1] == 0x00); + printf("base64 roundtrip OK\n"); return 0; } From af847dd36a14a54b32ed299a6c66963a96aff09f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 20:26:42 +0200 Subject: [PATCH 2/2] fix(cmd_status): function-scope keys so the coverage/firmware build compiles Rebasing onto current master pulled in a latent build break: cmd_status declares `keys` inside the `if (commands_is_admin())` block but scrubs it via `secure_wipe(keys, sizeof(keys))` at function scope, so `keys` is undeclared there. asan_commands links serial/commands.c (not commands_system.c) so it never compiled this TU, but `make -C test coverage` compiles the whole first-party surface and fails here (`keys undeclared`) -- as does the real firmware build. Restore `keys` to function scope, matching the scrub's intent of always clearing the key DB from BSS (incl. the non-admin path where it stays zero-initialised). Same fix as PR #15 (M1). Co-Authored-By: Claude Opus 4.8 (1M context) --- serial/commands_system.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/serial/commands_system.c b/serial/commands_system.c index 4b92f9e..bdd3cac 100644 --- a/serial/commands_system.c +++ b/serial/commands_system.c @@ -64,11 +64,13 @@ void cmd_status(int argc, char **argv) { printf("ntp: not synced\r\n"); } - // Keys (admin only: key inventory is target-selection data) + // Keys (admin only: key inventory is target-selection data). Declared at + // function scope so the scrub below always runs, even on the non-admin path + // where the array stays zero-initialised. + static key_record_t keys[BACKUP_MAX_KEYS]; if (commands_is_admin()) { - static key_record_t keys[BACKUP_MAX_KEYS]; - int count = storage_key_list(keys, BACKUP_MAX_KEYS); - int enabled = 0, corrupt = 0; + int count = storage_key_list(keys, BACKUP_MAX_KEYS); + int enabled = 0, corrupt = 0; for (int i = 0; i < count; i++) { if (!keys[i].is_checksum_valid) corrupt++;