From 1648665733ed9c26ce1f23acb5fa1362623de6c6 Mon Sep 17 00:00:00 2001 From: User Robin Steuber Date: Thu, 30 May 2024 15:14:51 +0800 Subject: [PATCH] CVE-2023-29532 Conflict:NA Reference:https://bug1806394.bmoattachments.org/attachment.cgi?id=9318233 --- modules/libmar/src/mar.h | 63 ++++- modules/libmar/src/mar_extract.c | 4 +- modules/libmar/src/mar_read.c | 259 +++++++++++++----- modules/libmar/tool/mar.c | 9 +- modules/libmar/verify/mar_verify.c | 143 +++++----- toolkit/mozapps/update/UpdateService.jsm | 32 ++- toolkit/mozapps/update/common/updatererrors.h | 6 + .../mozapps/update/tests/browser/browser.ini | 3 + ...rowser_memory_allocation_error_fallback.js | 81 ++++++ .../mozapps/update/updater/archivereader.cpp | 9 +- toolkit/mozapps/update/updater/updater.cpp | 6 + 11 files changed, 454 insertions(+), 161 deletions(-) create mode 100644 toolkit/mozapps/update/tests/browser/browser_memory_allocation_error_fallback.js diff --git a/modules/libmar/src/mar.h b/modules/libmar/src/mar.h index 4f9ae487e7..74ab7657f8 100644 --- a/modules/libmar/src/mar.h +++ b/modules/libmar/src/mar.h @@ -55,7 +55,8 @@ typedef struct SeenIndex_ { * Mozilla ARchive (MAR) file data structure */ struct MarFile_ { - FILE* fp; /* file pointer to the archive */ + unsigned char* buffer; /* file buffer containing the entire MAR */ + size_t data_len; /* byte count of the data in the buffer */ MarItem* item_table[TABLESIZE]; /* hash table of files in the archive */ SeenIndex* index_list; /* file indexes processed */ int item_table_is_valid; /* header and index validation flag */ @@ -72,16 +73,29 @@ typedef struct MarFile_ MarFile; */ typedef int (*MarItemCallback)(MarFile* mar, const MarItem* item, void* data); +enum MarReadResult_ { + MAR_READ_SUCCESS, + MAR_IO_ERROR, + MAR_MEM_ERROR, + MAR_FILE_TOO_BIG_ERROR, +}; + +typedef enum MarReadResult_ MarReadResult; + /** * Open a MAR file for reading. * @param path Specifies the path to the MAR file to open. This path must * be compatible with fopen. + * @param out_mar Out-parameter through which the created MarFile structure is + * returned. Guaranteed to be a valid structure if + * MAR_READ_SUCCESS is returned. Otherwise NULL will be + * assigned. * @return NULL if an error occurs. */ -MarFile* mar_open(const char* path); +MarReadResult mar_open(const char* path, MarFile** out_mar); #ifdef XP_WIN -MarFile* mar_wopen(const wchar_t* path); +MarReadResult mar_wopen(const wchar_t* path, MarFile** out_mar); #endif /** @@ -90,6 +104,49 @@ MarFile* mar_wopen(const wchar_t* path); */ void mar_close(MarFile* mar); +/** + * Reads the specified amount of data from the buffer in MarFile that contains + * the entirety of the MAR file data. + * @param mar The MAR file to read from. + * @param dest The buffer to read into. + * @param position The byte index to start reading from the MAR at. + * On success, position will be incremented by size. + * @param size The number of bytes to read. + * @return 0 If the specified amount of data was read. + * -1 If the buffer MAR is not large enough to read the + * specified amount of data at the specified position. + */ +int mar_read_buffer(MarFile* mar, void* dest, size_t* position, size_t size); + +/** + * Reads the specified amount of data from the buffer in MarFile that contains + * the entirety of the MAR file data. If there isn't that much data remaining, + * reads as much as possible. + * @param mar The MAR file to read from. + * @param dest The buffer to read into. + * @param position The byte index to start reading from the MAR at. + * This function will increment position by the number of bytes + * copied. + * @param size The maximum number of bytes to read. + * @return The number of bytes copied into dest. + */ +int mar_read_buffer_max(MarFile* mar, void* dest, size_t* position, + size_t size); + +/** + * Increments position by distance. Checks that the resulting position is still + * within the bounds of the buffer. Much like fseek, this will allow position to + * be successfully placed just after the end of the buffer. + * @param mar The MAR file to read from. + * @param position The byte index to start reading from the MAR at. + * On success, position will be incremented by size. + * @param distance The number of bytes to move forward by. + * @return 0 If position was successfully moved. + * -1 If moving position by distance would move it outside the + * bounds of the buffer. + */ +int mar_buffer_seek(MarFile* mar, size_t* position, size_t distance); + /** * Find an item in the MAR file by name. * @param mar The MarFile object to query. diff --git a/modules/libmar/src/mar_extract.c b/modules/libmar/src/mar_extract.c index 28693e24b0..9b4a3fa26d 100644 --- a/modules/libmar/src/mar_extract.c +++ b/modules/libmar/src/mar_extract.c @@ -75,8 +75,8 @@ int mar_extract(const char* path) { MarFile* mar; int rv; - mar = mar_open(path); - if (!mar) { + MarReadResult result = mar_open(path, &mar); + if (result != MAR_READ_SUCCESS) { return -1; } diff --git a/modules/libmar/src/mar_read.c b/modules/libmar/src/mar_read.c index 883760903d..fb743151f4 100644 --- a/modules/libmar/src/mar_read.c +++ b/modules/libmar/src/mar_read.c @@ -112,25 +112,28 @@ static int mar_consume_index(MarFile* mar, char** buf, const char* buf_end) { static int mar_read_index(MarFile* mar) { char id[MAR_ID_SIZE], *buf, *bufptr, *bufend; uint32_t offset_to_index, size_of_index; + size_t mar_position = 0; /* verify MAR ID */ - fseek(mar->fp, 0, SEEK_SET); - if (fread(id, MAR_ID_SIZE, 1, mar->fp) != 1) { + if (mar_read_buffer(mar, id, &mar_position, MAR_ID_SIZE) != 0) { return -1; } if (memcmp(id, MAR_ID, MAR_ID_SIZE) != 0) { return -1; } - if (fread(&offset_to_index, sizeof(uint32_t), 1, mar->fp) != 1) { + if (mar_read_buffer(mar, &offset_to_index, &mar_position, + sizeof(uint32_t)) != 0) { return -1; } offset_to_index = ntohl(offset_to_index); - if (fseek(mar->fp, offset_to_index, SEEK_SET)) { + mar_position = 0; + if (mar_buffer_seek(mar, &mar_position, offset_to_index) != 0) { return -1; } - if (fread(&size_of_index, sizeof(uint32_t), 1, mar->fp) != 1) { + if (mar_read_buffer(mar, &size_of_index, &mar_position, + sizeof(uint32_t)) != 0) { return -1; } size_of_index = ntohl(size_of_index); @@ -139,7 +142,7 @@ static int mar_read_index(MarFile* mar) { if (!buf) { return -1; } - if (fread(buf, size_of_index, 1, mar->fp) != 1) { + if (mar_read_buffer(mar, buf, &mar_position, size_of_index) != 0) { free(buf); return -1; } @@ -210,50 +213,96 @@ static int mar_insert_offset(MarFile* mar, uint32_t offset, uint32_t length) { /** * Internal shared code for mar_open and mar_wopen. - * On failure, will fclose(fp). + * Reads the entire MAR into memory. Fails if it is bigger than + * MAX_SIZE_OF_MAR_FILE bytes. */ -static MarFile* mar_fpopen(FILE* fp) { +static MarReadResult mar_fpopen(FILE* fp, MarFile** out_mar) { + *out_mar = NULL; MarFile* mar; mar = (MarFile*)malloc(sizeof(*mar)); if (!mar) { - fclose(fp); - return NULL; + return MAR_MEM_ERROR; + } + + off_t buffer_size = -1; + if (fseeko(fp, 0, SEEK_END) == 0) { + buffer_size = ftello(fp); + } + rewind(fp); + if (buffer_size < 0) { + fprintf(stderr, "Warning: MAR size could not be determined\n"); + buffer_size = MAX_SIZE_OF_MAR_FILE; + } + if (buffer_size > MAX_SIZE_OF_MAR_FILE) { + fprintf(stderr, "ERROR: MAR exceeds maximum size (%lli)\n", + (long long int)buffer_size); + free(mar); + return MAR_FILE_TOO_BIG_ERROR; + } + + mar->buffer = malloc(buffer_size); + if (!mar->buffer) { + fprintf(stderr, "ERROR: MAR buffer could not be allocated\n"); + free(mar); + return MAR_MEM_ERROR; + } + mar->data_len = fread(mar->buffer, 1, buffer_size, fp); + if (fgetc(fp) != EOF) { + fprintf(stderr, "ERROR: File is larger than buffer (%lli)\n", + (long long int)buffer_size); + free(mar->buffer); + free(mar); + return MAR_IO_ERROR; + } + if (ferror(fp)) { + fprintf(stderr, "ERROR: Failed to read MAR\n"); + free(mar->buffer); + free(mar); + return MAR_IO_ERROR; } - mar->fp = fp; mar->item_table_is_valid = 0; memset(mar->item_table, 0, sizeof(mar->item_table)); mar->index_list = NULL; - return mar; + *out_mar = mar; + return MAR_READ_SUCCESS; } -MarFile* mar_open(const char* path) { +MarReadResult mar_open(const char* path, MarFile** out_mar) { + *out_mar = NULL; + FILE* fp; fp = fopen(path, "rb"); if (!fp) { fprintf(stderr, "ERROR: could not open file in mar_open()\n"); perror(path); - return NULL; + return MAR_IO_ERROR; } - return mar_fpopen(fp); + MarReadResult result = mar_fpopen(fp, out_mar); + fclose(fp); + return result; } #ifdef XP_WIN -MarFile* mar_wopen(const wchar_t* path) { +MarReadResult mar_wopen(const wchar_t* path, MarFile** out_mar) { + *out_mar = NULL; + FILE* fp; _wfopen_s(&fp, path, L"rb"); if (!fp) { fprintf(stderr, "ERROR: could not open file in mar_wopen()\n"); _wperror(path); - return NULL; + return MAR_IO_ERROR; } - return mar_fpopen(fp); + MarReadResult result = mar_fpopen(fp, out_mar); + fclose(fp); + return result; } #endif @@ -262,7 +311,7 @@ void mar_close(MarFile* mar) { SeenIndex* index; int i; - fclose(mar->fp); + free(mar->buffer); for (i = 0; i < TABLESIZE; ++i) { item = mar->item_table[i]; @@ -282,10 +331,61 @@ void mar_close(MarFile* mar) { free(mar); } +int mar_read_buffer(MarFile* mar, void* dest, size_t* position, size_t size) { + // size may be provided by the MAR, which we may not have finished validating + // the signature on yet. Make sure not to trust it in a way that could + // cause an overflow. + if (size > mar->data_len) { + return -1; + } + if (*position > mar->data_len - size) { + return -1; + } + memcpy(dest, mar->buffer + *position, size); + *position += size; + return 0; +} + +int mar_read_buffer_max(MarFile* mar, void* dest, size_t* position, + size_t size) { + // size may be provided by the MAR, which we may not have finished validating + // the signature on yet. Make sure not to trust it in a way that could + // cause an overflow. + if (mar->data_len <= *position) { + return 0; + } + size_t read_count = mar->data_len - *position; + if (read_count > size) { + read_count = size; + } + memcpy(dest, mar->buffer + *position, read_count); + *position += read_count; + return read_count; +} + +int mar_buffer_seek(MarFile* mar, size_t* position, size_t distance) { + // distance may be provided by the MAR, which we may not have finished + // validating the signature on yet. Make sure not to trust it in a way that + // could cause an overflow. + if (distance > mar->data_len) { + return -1; + } + if (*position > mar->data_len - distance) { + return -1; + } + *position += distance; + return 0; +} + /** * Determines the MAR file information. * - * @param fp An opened MAR file in read mode. + * @param mar An open MAR file. + * @param mar_position The current position in the MAR. + * Its value will be updated to the current + * position in the MAR after the function exits. + * Since its initial value will never actually be + * used, this is effectively an outparam. * @param hasSignatureBlock Optional out parameter specifying if the MAR * file has a signature block or not. * @param numSignatures Optional out parameter for storing the number @@ -300,10 +400,11 @@ void mar_close(MarFile* mar) { * hasAdditionalBlocks is not equal to 0. * @return 0 on success and non-zero on failure. */ -int get_mar_file_info_fp(FILE* fp, int* hasSignatureBlock, - uint32_t* numSignatures, int* hasAdditionalBlocks, - uint32_t* offsetAdditionalBlocks, - uint32_t* numAdditionalBlocks) { +int get_open_mar_file_info(MarFile* mar, size_t* mar_position, + int* hasSignatureBlock, uint32_t* numSignatures, + int* hasAdditionalBlocks, + uint32_t* offsetAdditionalBlocks, + uint32_t* numAdditionalBlocks) { uint32_t offsetToIndex, offsetToContent, signatureCount, signatureLen, i; /* One of hasSignatureBlock or hasAdditionalBlocks must be non NULL */ @@ -312,24 +413,27 @@ int get_mar_file_info_fp(FILE* fp, int* hasSignatureBlock, } /* Skip to the start of the offset index */ - if (fseek(fp, MAR_ID_SIZE, SEEK_SET)) { + *mar_position = 0; + if (mar_buffer_seek(mar, mar_position, MAR_ID_SIZE) != 0) { return -1; } /* Read the offset to the index. */ - if (fread(&offsetToIndex, sizeof(offsetToIndex), 1, fp) != 1) { + if (mar_read_buffer(mar, &offsetToIndex, mar_position, + sizeof(offsetToIndex)) != 0) { return -1; } offsetToIndex = ntohl(offsetToIndex); if (numSignatures) { /* Skip past the MAR file size field */ - if (fseek(fp, sizeof(uint64_t), SEEK_CUR)) { + if (mar_buffer_seek(mar, mar_position, sizeof(uint64_t)) != 0) { return -1; } /* Read the number of signatures field */ - if (fread(numSignatures, sizeof(*numSignatures), 1, fp) != 1) { + if (mar_read_buffer(mar, numSignatures, mar_position, + sizeof(*numSignatures)) != 0) { return -1; } *numSignatures = ntohl(*numSignatures); @@ -337,17 +441,19 @@ int get_mar_file_info_fp(FILE* fp, int* hasSignatureBlock, /* Skip to the first index entry past the index size field We do it in 2 calls because offsetToIndex + sizeof(uint32_t) - could oerflow in theory. */ - if (fseek(fp, offsetToIndex, SEEK_SET)) { + could overflow in theory. */ + *mar_position = 0; + if (mar_buffer_seek(mar, mar_position, offsetToIndex) != 0) { return -1; } - if (fseek(fp, sizeof(uint32_t), SEEK_CUR)) { + if (mar_buffer_seek(mar, mar_position, sizeof(uint32_t)) != 0) { return -1; } /* Read the first offset to content field. */ - if (fread(&offsetToContent, sizeof(offsetToContent), 1, fp) != 1) { + if (mar_read_buffer(mar, &offsetToContent, mar_position, + sizeof(offsetToContent)) != 0) { return -1; } offsetToContent = ntohl(offsetToContent); @@ -368,12 +474,14 @@ int get_mar_file_info_fp(FILE* fp, int* hasSignatureBlock, } /* Skip to the start of the signature block */ - if (fseeko(fp, SIGNATURE_BLOCK_OFFSET, SEEK_SET)) { + *mar_position = 0; + if (mar_buffer_seek(mar, mar_position, SIGNATURE_BLOCK_OFFSET) != 0) { return -1; } /* Get the number of signatures */ - if (fread(&signatureCount, sizeof(signatureCount), 1, fp) != 1) { + if (mar_read_buffer(mar, &signatureCount, mar_position, + sizeof(signatureCount)) != 0) { return -1; } signatureCount = ntohl(signatureCount); @@ -387,38 +495,50 @@ int get_mar_file_info_fp(FILE* fp, int* hasSignatureBlock, /* Skip past the whole signature block */ for (i = 0; i < signatureCount; i++) { /* Skip past the signature algorithm ID */ - if (fseek(fp, sizeof(uint32_t), SEEK_CUR)) { + if (mar_buffer_seek(mar, mar_position, sizeof(uint32_t)) != 0) { return -1; } /* Read the signature length and skip past the signature */ - if (fread(&signatureLen, sizeof(uint32_t), 1, fp) != 1) { + if (mar_read_buffer(mar, &signatureLen, mar_position, + sizeof(uint32_t)) != 0) { return -1; } signatureLen = ntohl(signatureLen); - if (fseek(fp, signatureLen, SEEK_CUR)) { + if (mar_buffer_seek(mar, mar_position, signatureLen) != 0) { return -1; } } - if ((int64_t)ftell(fp) == (int64_t)offsetToContent) { + if (*mar_position <= (size_t)INT64_MAX && + (int64_t)mar_position == (int64_t)offsetToContent) { *hasAdditionalBlocks = 0; } else { if (numAdditionalBlocks) { /* We have an additional block, so read in the number of additional blocks and set the offset. */ *hasAdditionalBlocks = 1; - if (fread(numAdditionalBlocks, sizeof(uint32_t), 1, fp) != 1) { + if (mar_read_buffer(mar, numAdditionalBlocks, mar_position, + sizeof(uint32_t)) != 0) { return -1; } *numAdditionalBlocks = ntohl(*numAdditionalBlocks); if (offsetAdditionalBlocks) { - *offsetAdditionalBlocks = ftell(fp); + if (*mar_position > (size_t)UINT32_MAX) { + return -1; + } + *offsetAdditionalBlocks = (uint32_t)*mar_position; } } else if (offsetAdditionalBlocks) { /* numAdditionalBlocks is not specified but offsetAdditionalBlocks is, so fill it! */ - *offsetAdditionalBlocks = ftell(fp) + sizeof(uint32_t); + if (mar_buffer_seek(mar, mar_position, sizeof(uint32_t)) != 0) { + return -1; + } + if (*mar_position > (size_t)UINT32_MAX) { + return -1; + } + *offsetAdditionalBlocks = (uint32_t)*mar_position; } } @@ -436,16 +556,15 @@ int get_mar_file_info_fp(FILE* fp, int* hasSignatureBlock, int read_product_info_block(char* path, struct ProductInformationBlock* infoBlock) { int rv; - MarFile mar; - mar.fp = fopen(path, "rb"); - if (!mar.fp) { + MarFile* mar; + MarReadResult result = mar_open(path, &mar); + if (result != MAR_READ_SUCCESS) { fprintf(stderr, "ERROR: could not open file in read_product_info_block()\n"); - perror(path); return -1; } - rv = mar_read_product_info_block(&mar, infoBlock); - fclose(mar.fp); + rv = mar_read_product_info_block(mar, infoBlock); + mar_close(mar); return rv; } @@ -462,13 +581,14 @@ int mar_read_product_info_block(MarFile* mar, uint32_t offsetAdditionalBlocks, numAdditionalBlocks, additionalBlockSize, additionalBlockID; int hasAdditionalBlocks; + size_t mar_position = 0; /* The buffer size is 97 bytes because the MAR channel name < 64 bytes, and product version < 32 bytes + 3 NULL terminator bytes. */ char buf[MAXADDITIONALBLOCKSIZE + 1] = {'\0'}; - if (get_mar_file_info_fp(mar->fp, NULL, NULL, &hasAdditionalBlocks, - &offsetAdditionalBlocks, - &numAdditionalBlocks) != 0) { + if (get_open_mar_file_info(mar, &mar_position, NULL, NULL, &hasAdditionalBlocks, + &offsetAdditionalBlocks, + &numAdditionalBlocks) != 0) { return -1; } @@ -476,8 +596,8 @@ int mar_read_product_info_block(MarFile* mar, in a MAR file so check if any exist and process the first found */ if (numAdditionalBlocks > 0) { /* Read the additional block size */ - if (fread(&additionalBlockSize, sizeof(additionalBlockSize), 1, mar->fp) != - 1) { + if (mar_read_buffer(mar, &additionalBlockSize, &mar_position, + sizeof(additionalBlockSize)) != 0) { return -1; } additionalBlockSize = ntohl(additionalBlockSize) - @@ -490,7 +610,8 @@ int mar_read_product_info_block(MarFile* mar, } /* Read the additional block ID */ - if (fread(&additionalBlockID, sizeof(additionalBlockID), 1, mar->fp) != 1) { + if (mar_read_buffer(mar, &additionalBlockID, &mar_position, + sizeof(additionalBlockID)) != 0) { return -1; } additionalBlockID = ntohl(additionalBlockID); @@ -499,7 +620,7 @@ int mar_read_product_info_block(MarFile* mar, const char* location; int len; - if (fread(buf, additionalBlockSize, 1, mar->fp) != 1) { + if (mar_read_buffer(mar, buf, &mar_position, additionalBlockSize) != 0) { return -1; } @@ -528,7 +649,7 @@ int mar_read_product_info_block(MarFile* mar, return 0; } else { /* This is not the additional block you're looking for. Move along. */ - if (fseek(mar->fp, additionalBlockSize, SEEK_CUR)) { + if (mar_buffer_seek(mar, &mar_position, additionalBlockSize) != 0) { return -1; } } @@ -601,6 +722,7 @@ int mar_enum_items(MarFile* mar, MarItemCallback callback, void* closure) { int mar_read(MarFile* mar, const MarItem* item, int offset, uint8_t* buf, int bufsize) { int nr; + size_t mar_position = 0; if (offset == (int)item->length) { return 0; @@ -613,12 +735,16 @@ int mar_read(MarFile* mar, const MarItem* item, int offset, uint8_t* buf, if (nr > bufsize) { nr = bufsize; } - - if (fseek(mar->fp, item->offset + offset, SEEK_SET)) { + + // Avoid adding item->offset and offset directly, just in case of overflow. + if (mar_buffer_seek(mar, &mar_position, item->offset)) { + return -1; + } + if (mar_buffer_seek(mar, &mar_position, offset)) { return -1; } - return fread(buf, 1, nr, mar->fp); + return mar_read_buffer_max(mar, buf, &mar_position, nr); } /** @@ -644,17 +770,18 @@ int get_mar_file_info(const char* path, int* hasSignatureBlock, uint32_t* offsetAdditionalBlocks, uint32_t* numAdditionalBlocks) { int rv; - FILE* fp = fopen(path, "rb"); - if (!fp) { - fprintf(stderr, "ERROR: could not open file in get_mar_file_info()\n"); - perror(path); + MarFile* mar; + size_t mar_position = 0; + MarReadResult result = mar_open(path, &mar); + if (result != MAR_READ_SUCCESS) { + fprintf(stderr, "ERROR: could not read file in get_mar_file_info()\n"); return -1; } - rv = get_mar_file_info_fp(fp, hasSignatureBlock, numSignatures, - hasAdditionalBlocks, offsetAdditionalBlocks, - numAdditionalBlocks); + rv = get_open_mar_file_info(mar, &mar_position, hasSignatureBlock, + numSignatures, hasAdditionalBlocks, + offsetAdditionalBlocks, numAdditionalBlocks); - fclose(fp); + mar_close(mar); return rv; } diff --git a/modules/libmar/tool/mar.c b/modules/libmar/tool/mar.c index 0bf2cb4bd1..449c9f7efc 100644 --- a/modules/libmar/tool/mar.c +++ b/modules/libmar/tool/mar.c @@ -111,8 +111,8 @@ static int mar_test_callback(MarFile* mar, const MarItem* item, void* unused) { static int mar_test(const char* path) { MarFile* mar; - mar = mar_open(path); - if (!mar) { + MarReadResult result = mar_open(path, &mar); + if (result != MAR_READ_SUCCESS) { return -1; } @@ -393,8 +393,9 @@ int main(int argc, char** argv) { } if (!rv) { - MarFile* mar = mar_open(argv[2]); - if (mar) { + MarFile* mar; + MarReadResult result = mar_open(argv[2], &mar); + if (result == MAR_READ_SUCCESS) { rv = mar_verify_signatures(mar, certBuffers, fileSizes, certCount); mar_close(mar); } else { diff --git a/modules/libmar/verify/mar_verify.c b/modules/libmar/verify/mar_verify.c index 2ec17bbf7f..f08721c323 100644 --- a/modules/libmar/verify/mar_verify.c +++ b/modules/libmar/verify/mar_verify.c @@ -56,35 +56,38 @@ int mar_read_entire_file(const char* filePath, uint32_t maxSize, return result; } -int mar_extract_and_verify_signatures_fp(FILE* fp, - CryptoX_ProviderHandle provider, - CryptoX_PublicKey* keys, - uint32_t keyCount); -int mar_verify_signatures_for_fp(FILE* fp, CryptoX_ProviderHandle provider, - CryptoX_PublicKey* keys, - const uint8_t* const* extractedSignatures, - uint32_t keyCount, uint32_t* numVerified); +int mar_extract_and_verify_signatures(MarFile* mar, + CryptoX_ProviderHandle provider, + CryptoX_PublicKey* keys, + uint32_t keyCount); +int mar_verify_extracted_signatures(MarFile* mar, + CryptoX_ProviderHandle provider, + CryptoX_PublicKey* keys, + const uint8_t* const* extractedSignatures, + uint32_t keyCount, uint32_t* numVerified); /** - * Reads the specified number of bytes from the file pointer and + * Reads the specified number of bytes from the MAR buffer and * stores them in the passed buffer. * - * @param fp The file pointer to read from. + * @param mar An opened MAR + * @param mar_position + * Our current position within the MAR file buffer. * @param buffer The buffer to store the read results. * @param size The number of bytes to read, buffer must be * at least of this size. * @param ctxs Pointer to the first element in an array of verify context. * @param count The number of elements in ctxs * @param err The name of what is being written to in case of error. - * @return 0 on success - * -1 on read error - * -2 on verify update error + * @return CryptoX_Success on success + * CryptoX_Error on error */ -int ReadAndUpdateVerifyContext(FILE* fp, void* buffer, uint32_t size, - CryptoX_SignatureHandle* ctxs, uint32_t count, - const char* err) { +CryptoX_Result ReadAndUpdateVerifyContext(MarFile* mar, size_t* mar_position, + void* buffer, uint32_t size, + CryptoX_SignatureHandle* ctxs, + uint32_t count, const char* err) { uint32_t k; - if (!fp || !buffer || !ctxs || count == 0 || !err) { + if (!mar || !mar_position || !buffer || !ctxs || count == 0 || !err) { fprintf(stderr, "ERROR: Invalid parameter specified.\n"); return CryptoX_Error; } @@ -93,7 +96,7 @@ int ReadAndUpdateVerifyContext(FILE* fp, void* buffer, uint32_t size, return CryptoX_Success; } - if (fread(buffer, size, 1, fp) != 1) { + if (mar_read_buffer(mar, buffer, mar_position, size) != 0) { fprintf(stderr, "ERROR: Could not read %s\n", err); return CryptoX_Error; } @@ -101,7 +104,7 @@ int ReadAndUpdateVerifyContext(FILE* fp, void* buffer, uint32_t size, for (k = 0; k < count; k++) { if (CryptoX_Failed(CryptoX_VerifyUpdate(&ctxs[k], buffer, size))) { fprintf(stderr, "ERROR: Could not update verify context for %s\n", err); - return -2; + return CryptoX_Error; } } return CryptoX_Success; @@ -136,11 +139,6 @@ int mar_verify_signatures(MarFile* mar, const uint8_t* const* certData, goto failure; } - if (!mar->fp) { - fprintf(stderr, "ERROR: MAR file is not open.\n"); - goto failure; - } - if (CryptoX_Failed(CryptoX_InitCryptoProvider(&provider))) { fprintf(stderr, "ERROR: Could not init crytpo library.\n"); goto failure; @@ -154,7 +152,7 @@ int mar_verify_signatures(MarFile* mar, const uint8_t* const* certData, } } - rv = mar_extract_and_verify_signatures_fp(mar->fp, provider, keys, certCount); + rv = mar_extract_and_verify_signatures(mar, provider, keys, certCount); failure: @@ -169,50 +167,41 @@ failure: /** * Extracts each signature from the specified MAR file, - * then calls mar_verify_signatures_for_fp to verify each signature. + * then calls mar_verify_extracted_signatures to verify each signature. * - * @param fp An opened MAR file handle + * @param mar An opened MAR * @param provider A library provider * @param keys The public keys to use to verify the MAR * @param keyCount The number of keys pointed to by keys * @return 0 on success */ -int mar_extract_and_verify_signatures_fp(FILE* fp, - CryptoX_ProviderHandle provider, - CryptoX_PublicKey* keys, - uint32_t keyCount) { +int mar_extract_and_verify_signatures(MarFile* mar, + CryptoX_ProviderHandle provider, + CryptoX_PublicKey* keys, + uint32_t keyCount) { uint32_t signatureCount, signatureLen, numVerified = 0; uint32_t signatureAlgorithmIDs[MAX_SIGNATURES]; uint8_t* extractedSignatures[MAX_SIGNATURES]; uint32_t i; + size_t mar_position = 0; memset(signatureAlgorithmIDs, 0, sizeof(signatureAlgorithmIDs)); memset(extractedSignatures, 0, sizeof(extractedSignatures)); - if (!fp) { + if (!mar) { fprintf(stderr, "ERROR: Invalid file pointer passed.\n"); return CryptoX_Error; } - /* To protect against invalid MAR files, we assumes that the MAR file - size is less than or equal to MAX_SIZE_OF_MAR_FILE. */ - if (fseeko(fp, 0, SEEK_END)) { - fprintf(stderr, "ERROR: Could not seek to the end of the MAR file.\n"); - return CryptoX_Error; - } - if (ftello(fp) > MAX_SIZE_OF_MAR_FILE) { - fprintf(stderr, "ERROR: MAR file is too large to be verified.\n"); - return CryptoX_Error; - } - /* Skip to the start of the signature block */ - if (fseeko(fp, SIGNATURE_BLOCK_OFFSET, SEEK_SET)) { + if (mar_buffer_seek(mar, &mar_position, SIGNATURE_BLOCK_OFFSET) != 0) { fprintf(stderr, "ERROR: Could not seek to the signature block.\n"); return CryptoX_Error; } /* Get the number of signatures */ - if (fread(&signatureCount, sizeof(signatureCount), 1, fp) != 1) { + if (mar_read_buffer(mar, &signatureCount, &mar_position, + sizeof(signatureCount)) != 0) { fprintf(stderr, "ERROR: Could not read number of signatures.\n"); return CryptoX_Error; } @@ -228,19 +217,21 @@ int mar_extract_and_verify_signatures_fp(FILE* fp, for (i = 0; i < signatureCount; i++) { /* Get the signature algorithm ID */ - if (fread(&signatureAlgorithmIDs[i], sizeof(uint32_t), 1, fp) != 1) { + if (mar_read_buffer(mar, &signatureAlgorithmIDs[i], &mar_position, + sizeof(uint32_t)) != 0) { fprintf(stderr, "ERROR: Could not read signatures algorithm ID.\n"); return CryptoX_Error; } signatureAlgorithmIDs[i] = ntohl(signatureAlgorithmIDs[i]); - if (fread(&signatureLen, sizeof(uint32_t), 1, fp) != 1) { + if (mar_read_buffer(mar, &signatureLen, &mar_position, + sizeof(uint32_t)) != 0) { fprintf(stderr, "ERROR: Could not read signatures length.\n"); return CryptoX_Error; } signatureLen = ntohl(signatureLen); - /* To protected against invalid input make sure the signature length + /* To protect against invalid input make sure the signature length isn't too big. */ if (signatureLen > MAX_SIGNATURE_LENGTH) { fprintf(stderr, "ERROR: Signature length is too large to verify.\n"); @@ -249,10 +240,11 @@ int mar_extract_and_verify_signatures_fp(FILE* fp, extractedSignatures[i] = malloc(signatureLen); if (!extractedSignatures[i]) { - fprintf(stderr, "ERROR: Could allocate buffer for signature.\n"); + fprintf(stderr, "ERROR: Could not allocate buffer for signature.\n"); return CryptoX_Error; } - if (fread(extractedSignatures[i], signatureLen, 1, fp) != 1) { + if (mar_read_buffer(mar, extractedSignatures[i], &mar_position, + signatureLen) != 0) { fprintf(stderr, "ERROR: Could not read extracted signature.\n"); for (i = 0; i < signatureCount; ++i) { free(extractedSignatures[i]); @@ -270,11 +262,8 @@ int mar_extract_and_verify_signatures_fp(FILE* fp, } } - if (ftello(fp) == -1) { - return CryptoX_Error; - } - if (mar_verify_signatures_for_fp( - fp, provider, keys, (const uint8_t* const*)extractedSignatures, + if (mar_verify_extracted_signatures( + mar, provider, keys, (const uint8_t* const*)extractedSignatures, signatureCount, &numVerified) == CryptoX_Error) { return CryptoX_Error; } @@ -304,7 +293,7 @@ int mar_extract_and_verify_signatures_fp(FILE* fp, * certificate given, etc. The signature count must exactly match the number of * certificates given, and all signature verifications must succeed. * - * @param fp An opened MAR file handle + * @param mar An opened MAR * @param provider A library provider * @param keys A pointer to the first element in an * array of keys. @@ -315,18 +304,19 @@ int mar_extract_and_verify_signatures_fp(FILE* fp, * the number of verified signatures. * This information can be useful for printing * error messages. - * @return 0 on success, *numVerified == signatureCount. + * @return CryptoX_Success on success, *numVerified == signatureCount. */ -int mar_verify_signatures_for_fp(FILE* fp, CryptoX_ProviderHandle provider, - CryptoX_PublicKey* keys, - const uint8_t* const* extractedSignatures, - uint32_t signatureCount, - uint32_t* numVerified) { +CryptoX_Result mar_verify_extracted_signatures( + MarFile* mar, CryptoX_ProviderHandle provider, CryptoX_PublicKey* keys, + const uint8_t* const* extractedSignatures, + uint32_t signatureCount, + uint32_t* numVerified) { CryptoX_SignatureHandle signatureHandles[MAX_SIGNATURES]; char buf[BLOCKSIZE]; uint32_t signatureLengths[MAX_SIGNATURES]; uint32_t i; int rv = CryptoX_Error; + size_t mar_position = 0; memset(signatureHandles, 0, sizeof(signatureHandles)); memset(signatureLengths, 0, sizeof(signatureLengths)); @@ -355,19 +345,13 @@ int mar_verify_signatures_for_fp(FILE* fp, CryptoX_ProviderHandle provider, } } - /* Skip to the start of the file */ - if (fseeko(fp, 0, SEEK_SET)) { - fprintf(stderr, "ERROR: Could not seek to start of the file\n"); - goto failure; - } - /* Bytes 0-3: MAR1 Bytes 4-7: index offset Bytes 8-15: size of entire MAR */ if (CryptoX_Failed(ReadAndUpdateVerifyContext( - fp, buf, SIGNATURE_BLOCK_OFFSET + sizeof(uint32_t), signatureHandles, - signatureCount, "signature block"))) { + mar, &mar_position, buf, SIGNATURE_BLOCK_OFFSET + sizeof(uint32_t), + signatureHandles, signatureCount, "signature block"))) { goto failure; } @@ -375,14 +359,14 @@ int mar_verify_signatures_for_fp(FILE* fp, CryptoX_ProviderHandle provider, for (i = 0; i < signatureCount; i++) { /* Get the signature algorithm ID */ if (CryptoX_Failed(ReadAndUpdateVerifyContext( - fp, &buf, sizeof(uint32_t), signatureHandles, signatureCount, - "signature algorithm ID"))) { + mar, &mar_position, &buf, sizeof(uint32_t), signatureHandles, + signatureCount, "signature algorithm ID"))) { goto failure; } if (CryptoX_Failed(ReadAndUpdateVerifyContext( - fp, &signatureLengths[i], sizeof(uint32_t), signatureHandles, - signatureCount, "signature length"))) { + mar, &mar_position, &signatureLengths[i], sizeof(uint32_t), + signatureHandles, signatureCount, "signature length"))) { goto failure; } signatureLengths[i] = ntohl(signatureLengths[i]); @@ -392,20 +376,15 @@ int mar_verify_signatures_for_fp(FILE* fp, CryptoX_ProviderHandle provider, } /* Skip past the signature itself as those are not included */ - if (fseeko(fp, signatureLengths[i], SEEK_CUR)) { + if (mar_buffer_seek(mar, &mar_position, signatureLengths[i]) != 0) { fprintf(stderr, "ERROR: Could not seek past signature.\n"); goto failure; } } /* Read the rest of the file after the signature block */ - while (!feof(fp)) { - int numRead = fread(buf, 1, BLOCKSIZE, fp); - if (ferror(fp)) { - fprintf(stderr, "ERROR: Error reading data block.\n"); - goto failure; - } - + while (mar_position < mar->data_len) { + int numRead = mar_read_buffer_max(mar, buf, &mar_position, BLOCKSIZE); for (i = 0; i < signatureCount; i++) { if (CryptoX_Failed( CryptoX_VerifyUpdate(&signatureHandles[i], buf, numRead))) { diff --git a/toolkit/mozapps/update/UpdateService.jsm b/toolkit/mozapps/update/UpdateService.jsm index 4d1b1c59ef..e7cb58d0a0 100644 --- a/toolkit/mozapps/update/UpdateService.jsm +++ b/toolkit/mozapps/update/UpdateService.jsm @@ -1943,6 +1943,22 @@ function updateIsAtLeastAsOldAs(update, version, buildID) { ); } +/** + * This function determines whether the error represented by the passed error + * code is the result of the updater failing to allocate memory. This is + * relevant when staging because, since Firefox is also running, we may not be + * able to allocate much memory. Thus, if we fail to stage an update, we may + * succeed at updating without staging. + * + * @param An integer error code from the update.status file. Should be one of + * the codes enumerated in updatererrors.h. + * @returns true if the code represents a memory allocation error. + * Otherwise, false. + */ +function isMemoryAllocationErrorCode(errorCode) { + return errorCode >= 10 && errorCode <= 14; +} + /** * Update Patch * @param patch @@ -4485,12 +4501,26 @@ UpdateManager.prototype = { cleanUpReadyUpdateDir(false); if (update.state == STATE_FAILED && parts[1]) { + let isMemError = isMemoryAllocationErrorCode(parts[1]); if ( parts[1] == DELETE_ERROR_STAGING_LOCK_FILE || - parts[1] == UNEXPECTED_STAGING_ERROR + parts[1] == UNEXPECTED_STAGING_ERROR || + isMemError ) { update.state = getBestPendingState(); writeStatusFile(getReadyUpdateDir(), update.state); + if (isMemError) { + LOG( + `UpdateManager:refreshUpdateStatus - Updater failed to ` + + `allocate enough memory to successfully stage. Setting ` + + `status to "${update.state}"` + ); + } else { + LOG( + `UpdateManager:refreshUpdateStatus - Unexpected staging error. ` + + `Setting status to "${update.state}"` + ); + } } else if (!handleUpdateFailure(update, parts[1])) { await handleFallbackToCompleteUpdate(true); } diff --git a/toolkit/mozapps/update/common/updatererrors.h b/toolkit/mozapps/update/common/updatererrors.h index c4388cf3fd..a5304fceb3 100644 --- a/toolkit/mozapps/update/common/updatererrors.h +++ b/toolkit/mozapps/update/common/updatererrors.h @@ -23,11 +23,17 @@ #define WRITE_ERROR 7 // #define UNEXPECTED_ERROR 8 // Replaced with errors 38-42 #define ELEVATION_CANCELED 9 + +// Error codes 10-14 are related to memory allocation failures. +// Note: If more memory allocation error codes are added, the implementation of +// isMemoryAllocationErrorCode in UpdateService.jsm should be updated to account +// for them. #define READ_STRINGS_MEM_ERROR 10 #define ARCHIVE_READER_MEM_ERROR 11 #define BSPATCH_MEM_ERROR 12 #define UPDATER_MEM_ERROR 13 #define UPDATER_QUOTED_PATH_MEM_ERROR 14 + #define BAD_ACTION_ERROR 15 #define STRING_CONVERSION_ERROR 16 diff --git a/toolkit/mozapps/update/tests/browser/browser.ini b/toolkit/mozapps/update/tests/browser/browser.ini index edb9e2260e..00935bca0d 100644 --- a/toolkit/mozapps/update/tests/browser/browser.ini +++ b/toolkit/mozapps/update/tests/browser/browser.ini @@ -99,3 +99,6 @@ reason = test must be able to prevent file deletion. # Telemetry Update Ping Tests [browser_telemetry_updatePing_downloaded_ready.js] [browser_telemetry_updatePing_staged_ready.js] + +# Memory Fallback Tests +[browser_memory_allocation_error_fallback.js] diff --git a/toolkit/mozapps/update/tests/browser/browser_memory_allocation_error_fallback.js b/toolkit/mozapps/update/tests/browser/browser_memory_allocation_error_fallback.js new file mode 100644 index 0000000000..1bc075ef5e --- /dev/null +++ b/toolkit/mozapps/update/tests/browser/browser_memory_allocation_error_fallback.js @@ -0,0 +1,81 @@ +/* Any copyright is dedicated to the Public Domain. + * http://creativecommons.org/publicdomain/zero/1.0/ */ + +"use strict"; + +/** + * When the updater fails with a memory allocation error, we should fall back to + * updating without staging. + */ + +const READ_STRINGS_MEM_ERROR = 10; +const ARCHIVE_READER_MEM_ERROR = 11; +const BSPATCH_MEM_ERROR = 12; +const UPDATER_MEM_ERROR = 13; +const UPDATER_QUOTED_PATH_MEM_ERROR = 14; + +const EXPECTED_STATUS = + AppConstants.platform == "win" ? STATE_PENDING_SVC : STATE_PENDING; + +add_setup(async function setup() { + await SpecialPowers.pushPrefEnv({ + set: [ + [PREF_APP_UPDATE_STAGING_ENABLED, true], + [PREF_APP_UPDATE_SERVICE_ENABLED, true], + ], + }); + + registerCleanupFunction(() => { + Services.env.set("MOZ_FORCE_ERROR_CODE", ""); + }); +}); + +async function memAllocErrorFallback(errorCode) { + Services.env.set("MOZ_FORCE_ERROR_CODE", errorCode.toString()); + + // Since the partial should be successful specify an invalid size for the + // complete update. + let params = { + queryString: "&invalidCompleteSize=1", + backgroundUpdate: true, + continueFile: CONTINUE_STAGING, + waitForUpdateState: EXPECTED_STATUS, + }; + await runAboutPrefsUpdateTest(params, [ + { + panelId: "apply", + checkActiveUpdate: { state: EXPECTED_STATUS }, + continueFile: null, + }, + ]); +} + +function cleanup() { + reloadUpdateManagerData(true); + removeUpdateFiles(true); +} + +add_task(async function memAllocErrorFallback_READ_STRINGS_MEM_ERROR() { + await memAllocErrorFallback(READ_STRINGS_MEM_ERROR); + cleanup(); +}); + +add_task(async function memAllocErrorFallback_ARCHIVE_READER_MEM_ERROR() { + await memAllocErrorFallback(ARCHIVE_READER_MEM_ERROR); + cleanup(); +}); + +add_task(async function memAllocErrorFallback_BSPATCH_MEM_ERROR() { + await memAllocErrorFallback(BSPATCH_MEM_ERROR); + cleanup(); +}); + +add_task(async function memAllocErrorFallback_UPDATER_MEM_ERROR() { + await memAllocErrorFallback(UPDATER_MEM_ERROR); + cleanup(); +}); + +add_task(async function memAllocErrorFallback_UPDATER_QUOTED_PATH_MEM_ERROR() { + await memAllocErrorFallback(UPDATER_QUOTED_PATH_MEM_ERROR); + cleanup(); +}); diff --git a/toolkit/mozapps/update/updater/archivereader.cpp b/toolkit/mozapps/update/updater/archivereader.cpp index e166310b13..f73c24bd98 100644 --- a/toolkit/mozapps/update/updater/archivereader.cpp +++ b/toolkit/mozapps/update/updater/archivereader.cpp @@ -205,12 +205,15 @@ int ArchiveReader::Open(const NS_tchar* path) { } } +MarReadResult result = #ifdef XP_WIN - mArchive = mar_wopen(path); + mar_wopen(path, &mArchive); #else - mArchive = mar_open(path); + mar_open(path, &mArchive); #endif - if (!mArchive) { + if (result == MAR_MEM_ERROR) { + return ARCHIVE_READER_MEM_ERROR; + } else if (result != MAR_READ_SUCCESS) { return READ_ERROR; } diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp index 0295d2435d..2b0a94a6d1 100644 --- a/toolkit/mozapps/update/updater/updater.cpp +++ b/toolkit/mozapps/update/updater/updater.cpp @@ -2586,6 +2586,12 @@ static void UpdateThreadFunc(void* param) { putenv(const_cast("MOZ_TEST_PROCESS_UPDATES=")); #endif } else { +#ifdef TEST_UPDATER + const char* forceErrorCodeString = getenv("MOZ_FORCE_ERROR_CODE"); + if (forceErrorCodeString && *forceErrorCodeString) { + rv = atoi(forceErrorCodeString); + } +#endif if (rv) { LOG(("failed: %d", rv)); } else { -- 2.27.0