diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index d9ce0a0de..340b53555 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -13,6 +13,7 @@ name: "CodeQL" on: workflow_dispatch: + push: branches: [ master ] pull_request: diff --git a/armsrc/spiffs.h b/armsrc/spiffs.h index 7bbfb794b..e0b5868da 100644 --- a/armsrc/spiffs.h +++ b/armsrc/spiffs.h @@ -296,6 +296,7 @@ typedef struct spiffs_t { // file system configuration spiffs_config cfg; // number of logical blocks + // BUGBUG -- Should this be of type spiffs_block_ix? u32_t block_count; // cursor for free blocks, block index diff --git a/armsrc/spiffs_check.c b/armsrc/spiffs_check.c index c59fcabef..636feb769 100644 --- a/armsrc/spiffs_check.c +++ b/armsrc/spiffs_check.c @@ -535,26 +535,61 @@ static s32_t spiffs_page_consistency_check_i(spiffs *fs) { s32_t res = SPIFFS_OK; spiffs_page_ix pix_offset = 0; + // Avoid arithmetic in loop conditions (integer promotion rules can cause unintended consequences) + uint32_t block_count = fs->block_count; + uint32_t total_blocks = SPIFFS_PAGES_PER_BLOCK(fs) * block_count; + uint32_t total_blocks_plus_one_page = total_blocks + SPIFFS_PAGES_PER_BLOCK(fs); + +//#pragma region // check for overflow once, before looping + // this _should_ never happen, but prefer to see debug message / error + // rather than silently entering infinite loop. + if (block_count > ((spiffs_block_ix)(-1))) { + SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", block_count); + SPIFFS_API_CHECK_RES(fs, SPIFFS_ERR_INTERNAL); + } + // this checks for overflow of the multiplication of block_count+1 with SPIFFS_PAGES_PER_BLOCK(fs) + if (((uint32_t)(-1)) / SPIFFS_PAGES_PER_BLOCK(fs) > (block_count+1)) { + // checking with +1 block count to avoid overflow also in inner loop, which adds one page... + // would exceed value storable in uint32_t + SPIFFS_DBG("Overflow: pages per block %04x with block count "_SPIPRIbl" results in overflow\n", SPIFFS_PAGES_PER_BLOCK(fs), block_count); + SPIFFS_API_CHECK_RES(fs, SPIFFS_ERR_INTERNAL); + } + // because loop indices are using spiffs_page_ix type, + // that type can hold a large enough value + if (total_blocks > ((spiffs_page_ix)-1)) { + SPIFFS_DBG("Avoiding infinite loop, total_blocks "_SPIPRIpg" too large for spiffs_page_ix type\n", total_blocks); + SPIFFS_CHECK_RES(SPIFFS_ERR_INTERNAL); + } + // because loop indices are using spiffs_page_ix type, + // that type can hold a large enough value + if (total_blocks_plus_one_page > ((spiffs_page_ix)-1) || total_blocks_plus_one_page < total_blocks) { + SPIFFS_DBG("Avoiding infinite loop, total_blocks_plus_one_page "_SPIPRIpg" too large for spiffs_page_ix type\n", total_blocks_plus_one_page); + SPIFFS_CHECK_RES(SPIFFS_ERR_INTERNAL); + } + // RESULT: spiffs_page_ix can safely be used for loop index vs. each of + // block_count, total_blocks, and total_blocks_plus_one_page +//#pragma endregion // check for overflow once, before looping + // for each range of pages fitting into work memory - while (pix_offset < SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count) { + while (pix_offset < total_blocks) { // set this flag to abort all checks and rescan the page range u8_t restart = 0; memset(fs->work, 0, SPIFFS_CFG_LOG_PAGE_SZ(fs)); spiffs_block_ix cur_block = 0; // build consistency bitmap for id range traversing all blocks - while (!restart && cur_block < fs->block_count) { + while (!restart && cur_block < block_count) { CHECK_CB(fs, SPIFFS_CHECK_PAGE, SPIFFS_CHECK_PROGRESS, - (pix_offset * 256) / (SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count) + - ((((cur_block * pages_per_scan * 256) / (SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count))) / fs->block_count), + (pix_offset * 256) / total_blocks + + ((((cur_block * pages_per_scan * 256) / total_blocks)) / block_count), 0); // traverse each page except for lookup pages spiffs_page_ix cur_pix = SPIFFS_OBJ_LOOKUP_PAGES(fs) + SPIFFS_PAGES_PER_BLOCK(fs) * cur_block; - while (!restart && cur_pix < SPIFFS_PAGES_PER_BLOCK(fs) * (cur_block + 1)) { + while (!restart && cur_pix < SPIFFS_PAGES_PER_BLOCK(fs) * (cur_block+1)) { //if ((cur_pix & 0xff) == 0) // SPIFFS_CHECK_DBG("PA: processing pix "_SPIPRIpg", block "_SPIPRIbl" of pix "_SPIPRIpg", block "_SPIPRIbl"\n", - // cur_pix, cur_block, SPIFFS_PAGES_PER_BLOCK(fs) * fs->block_count, fs->block_count); + // cur_pix, cur_block, total_blocks, block_count); // read header spiffs_page_header p_hdr; diff --git a/armsrc/spiffs_hydrogen.c b/armsrc/spiffs_hydrogen.c index 93c7fbe89..9f44cff13 100644 --- a/armsrc/spiffs_hydrogen.c +++ b/armsrc/spiffs_hydrogen.c @@ -52,8 +52,16 @@ s32_t SPIFFS_format(spiffs *fs) { SPIFFS_LOCK(fs); + uint32_t block_count = fs->block_count; + // this _should_ never happen, but prefer to see debug message / error + // rather than silently entering infinite loop. + if (block_count > ((spiffs_block_ix)(-1))) { + SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", block_count); + SPIFFS_API_CHECK_RES_UNLOCK(fs, SPIFFS_ERR_INTERNAL); + } + spiffs_block_ix bix = 0; - while (bix < fs->block_count) { + while (bix < block_count) { fs->max_erase_count = 0; s32_t res = spiffs_erase_block(fs, bix); if (res != SPIFFS_OK) { diff --git a/armsrc/spiffs_nucleus.c b/armsrc/spiffs_nucleus.c index 2ebfabe04..cd6c708c4 100644 --- a/armsrc/spiffs_nucleus.c +++ b/armsrc/spiffs_nucleus.c @@ -364,6 +364,7 @@ static s32_t spiffs_obj_lu_scan_v( // Checks magic if enabled s32_t spiffs_obj_lu_scan( spiffs *fs) { + s32_t res; spiffs_block_ix bix; int entry; @@ -371,13 +372,22 @@ s32_t spiffs_obj_lu_scan( spiffs_block_ix unerased_bix = (spiffs_block_ix) - 1; #endif + uint32_t block_count = fs->block_count; + // this _should_ never happen, but prefer to see debug message / error + // rather than silently entering infinite loop. + if (block_count > ((spiffs_block_ix)(-1))) { + SPIFFS_DBG("Avoiding infinite loop, block_count "_SPIPRIbl" too large for spiffs_block_ix type\n", block_count); + SPIFFS_API_CHECK_RES(fs, SPIFFS_ERR_INTERNAL); + } + + // find out erase count // if enabled, check magic bix = 0; spiffs_obj_id erase_count_final; spiffs_obj_id erase_count_min = SPIFFS_OBJ_ID_FREE; spiffs_obj_id erase_count_max = 0; - while (bix < fs->block_count) { + while (bix < block_count) { #if SPIFFS_USE_MAGIC spiffs_obj_id magic; res = _spiffs_rd(fs, diff --git a/armsrc/thinfilm.c b/armsrc/thinfilm.c index c39316be5..8603ec506 100644 --- a/armsrc/thinfilm.c +++ b/armsrc/thinfilm.c @@ -105,7 +105,7 @@ static int EmSendCmdThinfilmRaw(const uint8_t *resp, uint16_t respLen) { uint16_t FpgaSendQueueDelay = 0; // send cycle - uint16_t i = 0; + size_t i = 0; for (; i < respLen;) { if (AT91C_BASE_SSC->SSC_SR & (AT91C_SSC_TXRDY)) { AT91C_BASE_SSC->SSC_THR = resp[i++]; @@ -114,8 +114,10 @@ static int EmSendCmdThinfilmRaw(const uint8_t *resp, uint16_t respLen) { } // Ensure that the FPGA Delay Queue is empty - uint8_t fpga_queued_bits = FpgaSendQueueDelay >> 3; - for (i = 0; i <= (fpga_queued_bits >> 3) + 1;) { + uint16_t fpga_queued_bits = FpgaSendQueueDelay >> 3; + fpga_queued_bits >>= 3; // divide by 8 (again?) + fpga_queued_bits += 1u; + for (i = 0; i <= fpga_queued_bits;) { if (AT91C_BASE_SSC->SSC_SR & (AT91C_SSC_TXRDY)) { AT91C_BASE_SSC->SSC_THR = SEC_F; FpgaSendQueueDelay = (uint8_t)AT91C_BASE_SSC->SSC_RHR; diff --git a/client/src/cmdhf14a.c b/client/src/cmdhf14a.c index f2d573a8c..e5207978f 100644 --- a/client/src/cmdhf14a.c +++ b/client/src/cmdhf14a.c @@ -3226,13 +3226,22 @@ int CmdHF14ANdefRead(const char *Cmd) { return PM3_EOVFLOW; } - for (uint16_t i = offset; i < ndef_size + offset; i += max_rapdu_size) { - uint16_t segment_size = max_rapdu_size < ndef_size + offset - i ? max_rapdu_size : ndef_size + offset - i; + for (size_t i = offset; i < ndef_size + offset; i += max_rapdu_size) { + size_t segment_size = max_rapdu_size < ndef_size + offset - i ? max_rapdu_size : ndef_size + offset - i; + keep_field_on = i < ndef_size + offset - max_rapdu_size; aREAD_NDEF_n = 0; param_gethex_to_eol("00b00000", 0, aREAD_NDEF, sizeof(aREAD_NDEF), &aREAD_NDEF_n); aREAD_NDEF[2] = i >> 8; aREAD_NDEF[3] = i & 0xFF; + + // Segment_size is stuffed into a single-byte field below ... so error out if overflows + if (segment_size > 0xFFu) { + PrintAndLogEx(ERR, "Segment size too large (0x%zx > 0xFF)", segment_size); + DropField(); + free(ndef_file); + return PM3_EOVFLOW; + } aREAD_NDEF[4] = segment_size; res = ExchangeAPDU14a(aREAD_NDEF, aREAD_NDEF_n + 1, activate_field, keep_field_on, response, sizeof(response), &resplen); diff --git a/client/src/cmdhfmf.c b/client/src/cmdhfmf.c index 60e2a8e1b..7997c505e 100644 --- a/client/src/cmdhfmf.c +++ b/client/src/cmdhfmf.c @@ -117,17 +117,28 @@ static char *GenerateFilename(const char *prefix, const char *suffix) { return fptr; } +// allocates `items` table entries, storing pointer to `*src` +// Each entry stores two keys (A and B), initialized to six-byte value 0xFFFFFFFFFFFF +// Each entry also stores whether the key was "found", defaults to false (0) static int initSectorTable(sector_t **src, size_t items) { + + // typedef struct { + // uint64_t Key[2]; + // uint8_t foundKey[2]; + // } sector_t; + + // This allocates based on the size of a single item (*src) = calloc(items, sizeof(sector_t)); - if (*src == NULL) + if (*src == NULL) { return PM3_EMALLOC; + } // empty e_sector for (size_t i = 0; i < items; i++) { for (uint8_t j = 0; j < 2; j++) { (*src)[i].Key[j] = 0xffffffffffff; - (*src)[i].foundKey[j] = 0; + // (*src)[i].foundKey[j] = 0; // calloc zero's these already } } return PM3_SUCCESS; diff --git a/client/src/cmdlfem410x.c b/client/src/cmdlfem410x.c index 5abbbd4d3..ecb252ffc 100644 --- a/client/src/cmdlfem410x.c +++ b/client/src/cmdlfem410x.c @@ -245,7 +245,7 @@ static int ask_em410x_binary_decode(bool verbose, uint32_t *hi, uint64_t *lo, ui else if (ans == -4) PrintAndLogEx(DEBUG, "DEBUG: Error - Em410x preamble not found"); else if (ans == -5) - PrintAndLogEx(DEBUG, "DEBUG: Error - Em410x Size not correct: %zu", size); + PrintAndLogEx(DEBUG, "DEBUG: Error - Em410x Size not correct: %zu", *size); else if (ans == -6) PrintAndLogEx(DEBUG, "DEBUG: Error - Em410x parity failed"); diff --git a/client/src/cmdlfem4x50.c b/client/src/cmdlfem4x50.c index 2b1dd5497..e1ee8181f 100644 --- a/client/src/cmdlfem4x50.c +++ b/client/src/cmdlfem4x50.c @@ -30,10 +30,12 @@ static int CmdHelp(const char *Cmd); -static void em4x50_prepare_result(const uint8_t *data, int fwr, int lwr, em4x50_word_t *words) { +// Each record is 4 bytes long ... a single line in the dump output +// Reads each record from `data`, reverses the four bytes, and writes to `words` +static void em4x50_prepare_result(const uint8_t *data, int first_record_inclusive, int last_record_inclusive, em4x50_word_t *words) { // restructure received result in "em4x50_word_t" structure - for (int i = fwr; i <= lwr; i++) { + for (int i = first_record_inclusive; i <= last_record_inclusive; i++) { for (int j = 0; j < 4; j++) { words[i].byte[j] = data[i * 4 + (3 - j)]; } @@ -779,6 +781,12 @@ static int CmdEM4x50Reader(const char *Cmd) { // iceman, misuse of return status code. int now = resp.status; + // prevent massive stack corruption if unexpected results from device. + if (now > EM4X50_NO_WORDS) { + PrintAndLogEx(WARNING, "word count was: %d, limiting to %d", now, EM4X50_NO_WORDS); + now = EM4X50_NO_WORDS; + } + if (now > 0) { em4x50_word_t words[EM4X50_NO_WORDS];