From 4ce34c6c1e13c033b046a252ae55a7489e8d23c8 Mon Sep 17 00:00:00 2001 From: Henry Gabryjelski <740168+henrygab@users.noreply.github.com> Date: Thu, 13 Nov 2025 23:42:06 -0800 Subject: [PATCH] More type and bufferlen safety, and validation in bit manipulation routines. Again, these should have zero impact on resulting instructions. At the same time, this will enable static analysis to more accurately validate a buffer (here `saflock_mfc_data_t`) points to sufficiently large allocated memory. Additional validation for `extract_bits()` and `insert_bits()` will prevent accidental corruptions (and assert, if assertions are enabled). These changes also makes the code easier to follow. --- client/src/cmdhfsaflok.c | 204 +++++++++++++++++++++++---------------- 1 file changed, 122 insertions(+), 82 deletions(-) diff --git a/client/src/cmdhfsaflok.c b/client/src/cmdhfsaflok.c index a42871a6a..7b89fcdc3 100644 --- a/client/src/cmdhfsaflok.c +++ b/client/src/cmdhfsaflok.c @@ -253,30 +253,59 @@ static void saflok_encrypt(const saflok_mfc_data_t *keyCard, saflok_mfc_data_t * } } -// unsafe without static analysis hints. -// Will access data[] at all indices from: -// floor(start_bit/8) .. ceil((start_bit + num_bits - 1)/8) -// safelok_mfc_data_t* for data parameter? -static uint32_t extract_bits(const uint8_t *data, size_t start_bit, size_t num_bits) { +static uint32_t extract_bits(const saflok_mfc_data_t *data, size_t start_bit, size_t num_bits) { + static const size_t total_available_bits = ARRAYLEN(data->raw) * 8; + if (start_bit >= total_available_bits) { + // Out of bounds access + assert(!"extract_bits: Out of bounds access (start_bit)"); + return 0; + } + if (num_bits > 32) { + // Exceeds maximum supported bit extraction + assert(!"extract_bits: num_bits exceeds 32"); + return 0; + } + if (total_available_bits - start_bit < num_bits) { + // Out of bounds access + assert(!"extract_bits: Out of bounds access (num_bits)"); + return 0; + } + uint32_t result = 0; for (size_t i = 0; i < num_bits; i++) { size_t byte_index = (start_bit + i) / 8; size_t bit_index = (start_bit + i) % 8; - if (data[byte_index] & (1 << (7 - bit_index))) { + if (data->raw[byte_index] & (1 << (7 - bit_index))) { result |= (1ULL << (num_bits - 1 - i)); } } return result; } -// unsafe without static analysis hints. -// Will access data[] at all indices from: -// floor(start_bit/8) .. ceil((start_bit + num_bits - 1)/8) -// A maximum of 32 bits can be inserted by this function. -// reject values that exceed the size of safelok_mfc_data_t->data[] -// safelok_mfc_data_t* for data parameter? +static void insert_bits(saflok_mfc_data_t *data, size_t start_bit, size_t num_bits, uint32_t value) { + static const size_t total_available_bits = ARRAYLEN(data->raw) * 8; + if (start_bit >= total_available_bits) { + // Out of bounds access + assert(!"insert_bits: Out of bounds access (start_bit)"); + return; + } + if (num_bits > 32) { + // Exceeds maximum supported bit extraction + assert(!"insert_bits: num_bits exceeds 32"); + return; + } + if (total_available_bits - start_bit < num_bits) { + // Out of bounds access + assert(!"insert_bits: Out of bounds access (num_bits)"); + return; + } + if ((num_bits < 32) && ((value >> num_bits) != 0)) { + // Value exceeds the size of the specified bit field + assert(!"insert_bits: value exceeds bit field size"); + return; + } + -static void insert_bits(uint8_t *data, size_t start_bit, size_t num_bits, uint32_t value) { for (size_t i = 0; i < num_bits; i++) { size_t current_bit = start_bit + i; size_t byte_index = current_bit / 8; @@ -284,7 +313,7 @@ static void insert_bits(uint8_t *data, size_t start_bit, size_t num_bits, uint32 uint32_t bit_value = (value >> (num_bits - 1 - i)) & 1U; - data[byte_index] = (data[byte_index] & ~(1 << bit_index)) | (bit_value << bit_index); + data->raw[byte_index] = (data->raw[byte_index] & ~(1 << bit_index)) | (bit_value << bit_index); } } @@ -306,8 +335,7 @@ static char *bytes_to_hex(const uint8_t *data, size_t len) { } -// safelok_mfc_data_t* for data parameter? -static int pack_datetime_expr(char *expiration_datetime, uint8_t *data) { +static int pack_datetime_expr(char *expiration_datetime, saflok_mfc_data_t *data) { int year, month, day, hour, minute; if (sscanf(expiration_datetime, "%4d-%2d-%2dT%2d:%2d", @@ -315,15 +343,14 @@ static int pack_datetime_expr(char *expiration_datetime, uint8_t *data) { return -1; } - data[8] = ((year & 0x0F) << 4) | (month & 0x0F); - data[9] = ((day & 0x1F) << 3) | ((hour & 0x1C) >> 2); - data[10] = ((hour & 0x03) << 6) | (minute & 0x3F); + data->raw[8] = ((year & 0x0F) << 4) | (month & 0x0F); + data->raw[9] = ((day & 0x1F) << 3) | ((hour & 0x1C) >> 2); + data->raw[10] = ((hour & 0x03) << 6) | (minute & 0x3F); return 0; } -// safelok_mfc_data_t* for data parameter? -static int pack_datetime(char *datetime_str, uint8_t *data) { +static int pack_datetime(char *datetime_str, saflok_mfc_data_t *data) { int year, month, day, hour, minute; if (sscanf(datetime_str, "%4d-%2d-%2dT%2d:%2d", @@ -333,23 +360,19 @@ static int pack_datetime(char *datetime_str, uint8_t *data) { uint8_t year_offset = year - 1980; - data[11] = ((year_offset & 0x0F) << 4) | (month & 0x0F); - data[12] = ((day & 0x1F) << 3) | ((hour & 0x1C) >> 2); - data[13] = ((hour & 0x03) << 6) | (minute & 0x3F); - data[14] = (data[14] & 0x0F) | ((year_offset & 0x70) << 0); + data->raw[11] = ((year_offset & 0x0F) << 4) | (month & 0x0F); + data->raw[12] = ((day & 0x1F) << 3) | ((hour & 0x1C) >> 2); + data->raw[13] = ((hour & 0x03) << 6) | (minute & 0x3F); + data->raw[14] = (data->raw[14] & 0x0F) | ((year_offset & 0x70) << 0); return 0; } -// unsafe without static analysis hints. -// Will access all bytes of `data[len]` -// length is ALWAYS 16 for safelock_mfc_data_t ... -// safelok_mfc_data_t* for data parameter, and only access data[0..15]? -// BUGBUG -- first parameter should point to 'const' data? -static uint8_t saflok_checksum(unsigned char *data, int length) { +static uint8_t calculated_saflok_checksum(const saflok_mfc_data_t *data) { + _Static_assert(ARRAYLEN(data->raw) == 17, "saflok_mfc_data_t raw size must be 17 bytes"); int sum = 0; - for (int i = 0; i < length; i++) { - sum += data[i]; + for (int i = 0; i < 16; i++) { + sum += data->raw[i]; } sum = 255 - (sum & 0xFF); return sum & 0xFF; @@ -372,8 +395,7 @@ static void saflok_kdf(const saflok_mfc_uid_t *uid, saflok_mfc_key_t *key_out) { memcpy(key_out, &key, KEY_LENGTH); } -// unsafe! data : struct containing uint8[17] -static void saflok_decode(uint8_t *data) { +static void saflok_decode(const saflok_mfc_data_t *data) { uint32_t card_level = extract_bits(data, 0, 4); uint32_t card_type = extract_bits(data, 4, 4); @@ -390,19 +412,19 @@ static void saflok_decode(uint8_t *data) { uint32_t checksum = extract_bits(data, 128, 8); //date parsing, stolen from flipper code - uint16_t interval_year = (data[8] >> 4); - uint8_t interval_month = data[8] & 0x0F; - uint8_t interval_day = (data[9] >> 3) & 0x1F; - uint8_t interval_hour = ((data[9] & 0x07) << 2) | (data[10] >> 6); - uint8_t interval_minute = data[10] & 0x3F; + uint16_t interval_year = (data->raw[8] >> 4); + uint8_t interval_month = data->raw[8] & 0x0F; + uint8_t interval_day = (data->raw[9] >> 3) & 0x1F; + uint8_t interval_hour = ((data->raw[9] & 0x07) << 2) | (data->raw[10] >> 6); + uint8_t interval_minute = data->raw[10] & 0x3F; - uint8_t creation_year_bits = (data[14] & 0xF0); + uint8_t creation_year_bits = (data->raw[14] & 0xF0); uint16_t creation_year = - (creation_year_bits | ((data[11] & 0xF0) >> 4)) + 1980; - uint8_t creation_month = data[11] & 0x0F; - uint8_t creation_day = (data[12] >> 3) & 0x1F; - uint8_t creation_hour = ((data[12] & 0x07) << 2) | (data[13] >> 6); - uint8_t creation_minute = data[13] & 0x3F; + (creation_year_bits | ((data->raw[11] & 0xF0) >> 4)) + 1980; + uint8_t creation_month = data->raw[11] & 0x0F; + uint8_t creation_day = (data->raw[12] >> 3) & 0x1F; + uint8_t creation_hour = ((data->raw[12] & 0x07) << 2) | (data->raw[13] >> 6); + uint8_t creation_minute = data->raw[13] & 0x3F; uint16_t expire_year = creation_year + interval_year; uint8_t expire_month = creation_month + interval_month; @@ -458,16 +480,28 @@ static void saflok_decode(uint8_t *data) { expire_hour, expire_minute); PrintAndLogEx(SUCCESS, "Property ID: " _GREEN_("%u"), property_id); - PrintAndLogEx(SUCCESS, "Checksum: " _GREEN_("0x%X") " (%s)", checksum, (checksum == saflok_checksum(data, 16)) ? _GREEN_("ok") : _RED_("bad")); + PrintAndLogEx(SUCCESS, "Checksum: " _GREEN_("0x%X") " (%s)", checksum, (checksum == calculated_saflok_checksum(data)) ? _GREEN_("ok") : _RED_("bad")); PrintAndLogEx(NORMAL, ""); } -static void saflok_encode(uint8_t *data, uint32_t card_level, uint32_t card_type, uint32_t card_id, - uint32_t opening_key, uint32_t lock_id, uint32_t pass_number, - uint32_t sequence_and_combination, uint32_t deadbolt_override, - uint32_t restricted_days, uint32_t expire_date, uint32_t card_creation_date, - uint32_t property_id, char *dt_e, char *dt) { +static void saflok_encode( + saflok_mfc_data_t *data, + uint32_t card_level, + uint32_t card_type, + uint32_t card_id, + uint32_t opening_key, + uint32_t lock_id, + uint32_t pass_number, + uint32_t sequence_and_combination, + uint32_t deadbolt_override, + uint32_t restricted_days, + uint32_t expire_date, + uint32_t card_creation_date, + uint32_t property_id, + char *dt_expiration, + char *dt + ) { insert_bits(data, 0, 4, card_level); insert_bits(data, 4, 4, card_type); insert_bits(data, 8, 8, card_id); @@ -483,6 +517,7 @@ static void saflok_encode(uint8_t *data, uint32_t card_level, uint32_t card_type int year, month, day, hour, minute; + // BUGBUG -- sscanf() occurs twice: once here, and once in pack_datetime if (sscanf(dt, "%4d-%2d-%2dT%2d:%2d", &year, &month, &day, &hour, &minute) == 5) { pack_datetime(dt, data); @@ -492,20 +527,23 @@ static void saflok_encode(uint8_t *data, uint32_t card_level, uint32_t card_type //PrintAndLogEx(SUCCESS, "DT BITS INSERTED"); //} - if (sscanf(dt_e, "%4d-%2d-%2dT%2d:%2d", + // BUGBUG -- sscanf() occurs twice: once here, and once in pack_datetime_expr + if (sscanf(dt_expiration, "%4d-%2d-%2dT%2d:%2d", &year, &month, &day, &hour, &minute) == 5) { - pack_datetime_expr(dt_e, data); + pack_datetime_expr(dt_expiration, data); } //else{ //insert_bits(data, 64, 24, expire_date); //PrintAndLogEx(SUCCESS, "DTE BITS INSERTED"); //} - uint8_t checksum = saflok_checksum(data, 16); + uint8_t checksum = calculated_saflok_checksum(data); insert_bits(data, 128, 8, checksum); } +// TODO: make clearer how many bytes secdata must minimally point to, +// perhaps by creating a struct to avoid having to pass a length parameter. static int saflok_read_sector(int sector, uint8_t *secdata) { clearCommandBuffer(); SendCommandMIX(CMD_HF_ISO14443A_READER, ISO14A_CONNECT, 0, 0, NULL, 0); @@ -551,16 +589,17 @@ static int CmdHFSaflokRead(const char *Cmd) { return PM3_EFAILED; } - saflok_mfc_data_t decrypted[17]; saflok_read_sector(0, secdata); // 64 bytes + const saflok_mfc_data_t * encrypted = (const saflok_mfc_data_t *)(secdata + 16); - saflok_decrypt(encrypted, decrypted); + saflok_mfc_data_t decrypted = {0}; + saflok_decrypt(encrypted, &decrypted); PrintAndLogEx(NORMAL, ""); PrintAndLogEx(INFO, "--- " _CYAN_("Card Information")); PrintAndLogEx(SUCCESS, "Encrypted Data: " _GREEN_("%s"), bytes_to_hex(encrypted->raw, sizeof(saflok_mfc_data_t))); - saflok_decode(decrypted->raw); + saflok_decode(&decrypted); return PM3_SUCCESS; } @@ -603,7 +642,7 @@ static int CmdHFSaflokEncode(const char *Cmd) { CLIParamStrToBuf(arg_get_str(ctx, 10), (uint8_t *)dt_e, 100, &slen); - saflok_encode(decrypted.raw, + saflok_encode(&decrypted, arg_get_u32_def(ctx, 1, 0), arg_get_u32_def(ctx, 2, 0), arg_get_u32_def(ctx, 3, 0), @@ -656,7 +695,7 @@ static int CmdHFSaflokDecode(const char *Cmd) { } saflok_decrypt(&encrypted, &decrypted); - saflok_decode(decrypted.raw); + saflok_decode(&decrypted); return PM3_SUCCESS; } @@ -704,40 +743,40 @@ static int CmdHFSaflokModify(const char *Cmd) { saflok_decrypt(&encrypted, &decrypted); - uint32_t card_level = extract_bits(decrypted.raw, 0, 4); + uint32_t card_level = extract_bits(&decrypted, 0, 4); card_level = arg_get_u32_def(ctx, 1, card_level); - uint32_t card_type = extract_bits(decrypted.raw, 4, 4); + uint32_t card_type = extract_bits(&decrypted, 4, 4); card_type = arg_get_u32_def(ctx, 2, card_type); - uint32_t card_id = extract_bits(decrypted.raw, 8, 8); + uint32_t card_id = extract_bits(&decrypted, 8, 8); card_id = arg_get_u32_def(ctx, 3, card_id); - uint32_t opening_key = extract_bits(decrypted.raw, 16, 2); + uint32_t opening_key = extract_bits(&decrypted, 16, 2); opening_key = arg_get_u32_def(ctx, 4, opening_key); - uint32_t lock_id = extract_bits(decrypted.raw, 18, 14); + uint32_t lock_id = extract_bits(&decrypted, 18, 14); lock_id = arg_get_u32_def(ctx, 5, lock_id); - uint32_t pass_number = extract_bits(decrypted.raw, 32, 12); + uint32_t pass_number = extract_bits(&decrypted, 32, 12); pass_number = arg_get_u32_def(ctx, 6, pass_number); - uint32_t sequence_and_combination = extract_bits(decrypted.raw, 44, 12); + uint32_t sequence_and_combination = extract_bits(&decrypted, 44, 12); sequence_and_combination = arg_get_u32_def(ctx, 7, sequence_and_combination); - uint32_t deadbolt_override = extract_bits(decrypted.raw, 56, 1); + uint32_t deadbolt_override = extract_bits(&decrypted, 56, 1); deadbolt_override = arg_get_u32_def(ctx, 8, deadbolt_override); - uint32_t restricted_days = extract_bits(decrypted.raw, 57, 7); + uint32_t restricted_days = extract_bits(&decrypted, 57, 7); restricted_days = arg_get_u32_def(ctx, 9, restricted_days); - uint32_t expire_date = extract_bits(decrypted.raw, 64, 24); + uint32_t expire_date = extract_bits(&decrypted, 64, 24); //expire_date = arg_get_u32_def(ctx, 10, expire_date); - uint32_t card_creation_date = extract_bits(decrypted.raw, 88, 28); + uint32_t card_creation_date = extract_bits(&decrypted, 88, 28); //card_creation_date = arg_get_u32_def(ctx, 11, card_creation_date); - uint32_t property_id = extract_bits(decrypted.raw, 116, 12); + uint32_t property_id = extract_bits(&decrypted, 116, 12); property_id = arg_get_u32_def(ctx, 12, property_id); int slen = 0; @@ -748,7 +787,7 @@ static int CmdHFSaflokModify(const char *Cmd) { char dt_e[100]; CLIParamStrToBuf(arg_get_str(ctx, 10), (uint8_t *)dt_e, 100, &slen2); - saflok_encode(decrypted.raw, + saflok_encode(&decrypted, card_level, card_type, card_id, @@ -855,9 +894,9 @@ static int CmdHFSaflokChecksum(const char *Cmd) { CLIExecWithReturn(ctx, Cmd, argtable, true); - uint8_t data[17]; + saflok_mfc_data_t data = {0}; int len; - CLIGetHexWithReturn(ctx, 1, data, &len); + CLIGetHexWithReturn(ctx, 1, data.raw, &len); if (len != 16) { PrintAndLogEx(WARNING, "Expected 16 bytes. Got %d.", len); @@ -865,10 +904,10 @@ static int CmdHFSaflokChecksum(const char *Cmd) { return PM3_EINVARG; } - data[16] = saflok_checksum(data, 16); + data.raw[16] = calculated_saflok_checksum(&data); - PrintAndLogEx(SUCCESS, "Block + checksum: " _GREEN_("%s"), bytes_to_hex(data, 17)); - PrintAndLogEx(SUCCESS, "Checksum byte: " _GREEN_("0x%02X"), data[16]); + PrintAndLogEx(SUCCESS, "Block + checksum: " _GREEN_("%s"), bytes_to_hex(data.raw, 17)); + PrintAndLogEx(SUCCESS, "Checksum byte: " _GREEN_("0x%02X"), data.raw[16]); CLIParserFree(ctx); return PM3_SUCCESS; @@ -888,9 +927,9 @@ static int CmdHFSaflokProvision(const char *Cmd) { CLIExecWithReturn(ctx, Cmd, argtable, true); - uint8_t data[17]; + saflok_mfc_data_t data = {0}; int len; - CLIGetHexWithReturn(ctx, 1, data, &len); + CLIGetHexWithReturn(ctx, 1, data.raw, &len); if (len != 17) { PrintAndLogEx(WARNING, "Expected 17 bytes, got %d", len); @@ -898,6 +937,7 @@ static int CmdHFSaflokProvision(const char *Cmd) { return PM3_EINVARG; } + // buffer size for UID is eight bytes ... even though saflok only uses first four bytes uint8_t uid[8]; int uid_len; if (mf_read_uid(uid, &uid_len, NULL) != PM3_SUCCESS || uid_len < 4) { @@ -916,8 +956,8 @@ static int CmdHFSaflokProvision(const char *Cmd) { uint8_t all_F[6] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}; uint8_t block1[16]; uint8_t block2[16] = {0}; - memcpy(block1, data, 16); - block2[0] = data[16]; + memcpy(block1, data.raw, 16); + block2[0] = data.raw[16]; block2[1] = 0x00; block2[2] = 0x04; block2[3] = 0x00;