From 90eff6a340f9983792d700df3b1ea0203aced207 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 15 Mar 2017 09:56:03 -0700 Subject: Revert "Print SHA-1 in hex for corrupted blocks" This reverts commit bb0cd75a0e1f6760bdf96bd141f3a546ffa45fbc. Broke the 'free' command that deletes a stash. Bug: 36242722 Test: The previously failed incremental applies successfully. Change-Id: I1237cb0a33adfbeea57e0465b629704862ba13aa --- updater/blockimg.cpp | 143 ++------------------------------------------------- 1 file changed, 3 insertions(+), 140 deletions(-) diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp index c2897a83e..03ce4136e 100644 --- a/updater/blockimg.cpp +++ b/updater/blockimg.cpp @@ -66,21 +66,6 @@ struct RangeSet { size_t count; // Limit is INT_MAX. size_t size; std::vector pos; // Actual limit is INT_MAX. - - // Get the block number for the ith(starting from 0) block in the range set. - int get_block(size_t idx) const { - if (idx >= size) { - LOG(ERROR) << "index: " << idx << " is greater than range set size: " << size; - return -1; - } - for (size_t i = 0; i < pos.size(); i += 2) { - if (idx < pos[i + 1] - pos[i]) { - return pos[i] + idx; - } - idx -= (pos[i + 1] - pos[i]); - } - return -1; - } }; static CauseCode failure_type = kNoCause; @@ -456,117 +441,6 @@ static int LoadSrcTgtVersion1(CommandParameters& params, RangeSet& tgt, size_t& return rc; } -// Print the hash in hex for corrupted source blocks (excluding the stashed blocks which is -// handled separately). -static void PrintHashForCorruptedSourceBlocks(const CommandParameters& params, - const std::vector& buffer) { - LOG(INFO) << "unexpected contents of source blocks in cmd:\n" << params.cmdline; - if (params.version < 3) { - // TODO handle version 1,2 - LOG(WARNING) << "version number " << params.version << " is not supported to print hashes"; - return; - } - - CHECK(params.tokens[0] == "move" || params.tokens[0] == "bsdiff" || - params.tokens[0] == "imgdiff"); - - size_t pos = 0; - // Command example: - // move [ ] - // bsdiff - // [ ] - if (params.tokens[0] == "move") { - // src_range for move starts at the 4th position. - if (params.tokens.size() < 5) { - LOG(ERROR) << "failed to parse source range in cmd:\n" << params.cmdline; - return; - } - pos = 4; - } else { - // src_range for diff starts at the 7th position. - if (params.tokens.size() < 8) { - LOG(ERROR) << "failed to parse source range in cmd:\n" << params.cmdline; - return; - } - pos = 7; - } - - // Source blocks in stash only, no work to do. - if (params.tokens[pos] == "-") { - return; - } - - RangeSet src = parse_range(params.tokens[pos++]); - - RangeSet locs; - // If there's no stashed blocks, content in the buffer is consecutive and has the same - // order as the source blocks. - if (pos == params.tokens.size()) { - locs.count = 1; - locs.size = src.size; - locs.pos = { 0, src.size }; - } else { - // Otherwise, the next token is the offset of the source blocks in the target range. - // Example: for the tokens <4,63946,63947,63948,63979> <4,6,7,8,39> ; - // We want to print SHA-1 for the data in buffer[6], buffer[8], buffer[9] ... buffer[38]; - // this corresponds to the 32 src blocks #63946, #63948, #63949 ... #63978. - locs = parse_range(params.tokens[pos++]); - CHECK_EQ(src.size, locs.size); - CHECK_EQ(locs.pos.size() % 2, static_cast(0)); - } - - LOG(INFO) << "printing hash in hex for " << src.size << " source blocks"; - for (size_t i = 0; i < src.size; i++) { - int block_num = src.get_block(i); - CHECK_NE(block_num, -1); - int buffer_index = locs.get_block(i); - CHECK_NE(buffer_index, -1); - CHECK_LE((buffer_index + 1) * BLOCKSIZE, buffer.size()); - - uint8_t digest[SHA_DIGEST_LENGTH]; - SHA1(buffer.data() + buffer_index * BLOCKSIZE, BLOCKSIZE, digest); - std::string hexdigest = print_sha1(digest); - LOG(INFO) << " block number: " << block_num << ", SHA-1: " << hexdigest; - } -} - -// If the calculated hash for the whole stash doesn't match the stash id, print the SHA-1 -// in hex for each block. -static void PrintHashForCorruptedStashedBlocks(const std::string& id, - const std::vector& buffer, - const RangeSet& src) { - LOG(INFO) << "printing hash in hex for stash_id: " << id; - CHECK_EQ(src.size * BLOCKSIZE, buffer.size()); - - for (size_t i = 0; i < src.size; i++) { - int block_num = src.get_block(i); - CHECK_NE(block_num, -1); - - uint8_t digest[SHA_DIGEST_LENGTH]; - SHA1(buffer.data() + i * BLOCKSIZE, BLOCKSIZE, digest); - std::string hexdigest = print_sha1(digest); - LOG(INFO) << " block number: " << block_num << ", SHA-1: " << hexdigest; - } -} - -// If the stash file doesn't exist, read the source blocks this stash contains and print the -// SHA-1 for these blocks. -static void PrintHashForMissingStashedBlocks(const std::string& id, int fd) { - if (stash_map.find(id) == stash_map.end()) { - LOG(ERROR) << "No stash saved for id: " << id; - return; - } - - LOG(INFO) << "print hash in hex for source blocks in missing stash: " << id; - const RangeSet& src = stash_map[id]; - std::vector buffer(src.size * BLOCKSIZE); - if (ReadBlocks(src, buffer, fd) == -1) { - LOG(ERROR) << "failed to read source blocks for stash: " << id; - return; - } - PrintHashForCorruptedStashedBlocks(id, buffer, src); -} - static int VerifyBlocks(const std::string& expected, const std::vector& buffer, const size_t blocks, bool printerror) { uint8_t digest[SHA_DIGEST_LENGTH]; @@ -699,7 +573,6 @@ static int LoadStash(CommandParameters& params, const std::string& base, const s } if (VerifyBlocks(id, buffer, src.size, true) != 0) { LOG(ERROR) << "failed to verify loaded source blocks in stash map."; - PrintHashForCorruptedStashedBlocks(id, buffer, src); return -1; } return 0; @@ -724,7 +597,6 @@ static int LoadStash(CommandParameters& params, const std::string& base, const s if (res == -1) { if (errno != ENOENT || printnoent) { PLOG(ERROR) << "stat \"" << fn << "\" failed"; - PrintHashForMissingStashedBlocks(id, params.fd); } return -1; } @@ -752,13 +624,6 @@ static int LoadStash(CommandParameters& params, const std::string& base, const s if (verify && VerifyBlocks(id, buffer, *blocks, true) != 0) { LOG(ERROR) << "unexpected contents in " << fn; - if (stash_map.find(id) == stash_map.end()) { - LOG(ERROR) << "failed to find source blocks number for stash " << id - << " when executing command: " << params.cmdname; - } else { - const RangeSet& src = stash_map[id]; - PrintHashForCorruptedStashedBlocks(id, buffer, src); - } DeleteFile(fn, nullptr); return -1; } @@ -930,7 +795,6 @@ static int SaveStash(CommandParameters& params, const std::string& base, return -1; } blocks = src.size; - stash_map[id] = src; if (usehash && VerifyBlocks(id, buffer, blocks, true) != 0) { // Source blocks have unexpected contents. If we actually need this @@ -941,8 +805,9 @@ static int SaveStash(CommandParameters& params, const std::string& base, return 0; } - // In verify mode, we don't need to stash any blocks. + // In verify mode, save source range_set instead of stashing blocks. if (!params.canwrite && usehash) { + stash_map[id] = src; return 0; } @@ -1161,8 +1026,6 @@ static int LoadSrcTgtVersion3(CommandParameters& params, RangeSet& tgt, size_t& // Valid source data not available, update cannot be resumed LOG(ERROR) << "partition has unexpected contents"; - PrintHashForCorruptedSourceBlocks(params, params.buffer); - params.isunresumable = true; return -1; @@ -1231,7 +1094,7 @@ static int PerformCommandFree(CommandParameters& params) { const std::string& id = params.tokens[params.cpos++]; - if (stash_map.find(id) != stash_map.end()) { + if (!params.canwrite && stash_map.find(id) != stash_map.end()) { stash_map.erase(id); return 0; } -- cgit v1.2.3 From ec8272f6e3a1e1697469b0f2cb7fd86a120bf3f6 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 15 Mar 2017 17:39:01 -0700 Subject: updater: Minor clean up to EnumerateStash(). Test: Apply an incremental BBOTA package with the new updater. Test: Resume an interrupted BBOTA (so it cleans up the partial stash). Change-Id: I620cc57ee6366845bcffbc19210f7a01e2196052 --- updater/blockimg.cpp | 123 +++++++++++++++++++++------------------------------ 1 file changed, 50 insertions(+), 73 deletions(-) diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp index 03ce4136e..6227154e5 100644 --- a/updater/blockimg.cpp +++ b/updater/blockimg.cpp @@ -32,6 +32,7 @@ #include #include +#include #include #include #include @@ -473,88 +474,54 @@ static std::string GetStashFileName(const std::string& base, const std::string& return fn; } -typedef void (*StashCallback)(const std::string&, void*); +// Does a best effort enumeration of stash files. Ignores possible non-file items in the stash +// directory and continues despite of errors. Calls the 'callback' function for each file. +static void EnumerateStash(const std::string& dirname, + const std::function& callback) { + if (dirname.empty()) return; -// Does a best effort enumeration of stash files. Ignores possible non-file -// items in the stash directory and continues despite of errors. Calls the -// 'callback' function for each file and passes 'data' to the function as a -// parameter. + std::unique_ptr directory(opendir(dirname.c_str()), closedir); -static void EnumerateStash(const std::string& dirname, StashCallback callback, void* data) { - if (dirname.empty() || callback == nullptr) { - return; + if (directory == nullptr) { + if (errno != ENOENT) { + PLOG(ERROR) << "opendir \"" << dirname << "\" failed"; } - - std::unique_ptr directory(opendir(dirname.c_str()), closedir); - - if (directory == nullptr) { - if (errno != ENOENT) { - PLOG(ERROR) << "opendir \"" << dirname << "\" failed"; - } - return; - } - - struct dirent* item; - while ((item = readdir(directory.get())) != nullptr) { - if (item->d_type != DT_REG) { - continue; - } - - std::string fn = dirname + "/" + std::string(item->d_name); - callback(fn, data); - } -} - -static void UpdateFileSize(const std::string& fn, void* data) { - if (fn.empty() || !data) { return; } - struct stat sb; - if (stat(fn.c_str(), &sb) == -1) { - PLOG(ERROR) << "stat \"" << fn << "\" failed"; - return; + dirent* item; + while ((item = readdir(directory.get())) != nullptr) { + if (item->d_type != DT_REG) continue; + callback(dirname + "/" + item->d_name); } - - size_t* size = static_cast(data); - *size += sb.st_size; } // Deletes the stash directory and all files in it. Assumes that it only // contains files. There is nothing we can do about unlikely, but possible // errors, so they are merely logged. +static void DeleteFile(const std::string& fn) { + if (fn.empty()) return; -static void DeleteFile(const std::string& fn, void* /* data */) { - if (!fn.empty()) { - LOG(INFO) << "deleting " << fn; - - if (unlink(fn.c_str()) == -1 && errno != ENOENT) { - PLOG(ERROR) << "unlink \"" << fn << "\" failed"; - } - } -} + LOG(INFO) << "deleting " << fn; -static void DeletePartial(const std::string& fn, void* data) { - if (android::base::EndsWith(fn, ".partial")) { - DeleteFile(fn, data); - } + if (unlink(fn.c_str()) == -1 && errno != ENOENT) { + PLOG(ERROR) << "unlink \"" << fn << "\" failed"; + } } static void DeleteStash(const std::string& base) { - if (base.empty()) { - return; - } + if (base.empty()) return; - LOG(INFO) << "deleting stash " << base; + LOG(INFO) << "deleting stash " << base; - std::string dirname = GetStashFileName(base, "", ""); - EnumerateStash(dirname, DeleteFile, nullptr); + std::string dirname = GetStashFileName(base, "", ""); + EnumerateStash(dirname, DeleteFile); - if (rmdir(dirname.c_str()) == -1) { - if (errno != ENOENT && errno != ENOTDIR) { - PLOG(ERROR) << "rmdir \"" << dirname << "\" failed"; - } + if (rmdir(dirname.c_str()) == -1) { + if (errno != ENOENT && errno != ENOTDIR) { + PLOG(ERROR) << "rmdir \"" << dirname << "\" failed"; } + } } static int LoadStash(CommandParameters& params, const std::string& base, const std::string& id, @@ -624,7 +591,7 @@ static int LoadStash(CommandParameters& params, const std::string& base, const s if (verify && VerifyBlocks(id, buffer, *blocks, true) != 0) { LOG(ERROR) << "unexpected contents in " << fn; - DeleteFile(fn, nullptr); + DeleteFile(fn); return -1; } @@ -750,13 +717,24 @@ static int CreateStash(State* state, size_t maxblocks, const std::string& blockd LOG(INFO) << "using existing stash " << dirname; - // If the directory already exists, calculate the space already allocated to - // stash files and check if there's enough for all required blocks. Delete any - // partially completed stash files first. + // If the directory already exists, calculate the space already allocated to stash files and check + // if there's enough for all required blocks. Delete any partially completed stash files first. + EnumerateStash(dirname, [](const std::string& fn) { + if (android::base::EndsWith(fn, ".partial")) { + DeleteFile(fn); + } + }); - EnumerateStash(dirname, DeletePartial, nullptr); size_t existing = 0; - EnumerateStash(dirname, UpdateFileSize, &existing); + EnumerateStash(dirname, [&existing](const std::string& fn) { + if (fn.empty()) return; + struct stat sb; + if (stat(fn.c_str(), &sb) == -1) { + PLOG(ERROR) << "stat \"" << fn << "\" failed"; + return; + } + existing += static_cast(sb.st_size); + }); if (max_stash_size > existing) { size_t needed = max_stash_size - existing; @@ -817,14 +795,13 @@ static int SaveStash(CommandParameters& params, const std::string& base, } static int FreeStash(const std::string& base, const std::string& id) { - if (base.empty() || id.empty()) { - return -1; - } + if (base.empty() || id.empty()) { + return -1; + } - std::string fn = GetStashFileName(base, id, ""); - DeleteFile(fn, nullptr); + DeleteFile(GetStashFileName(base, id, "")); - return 0; + return 0; } static void MoveRange(std::vector& dest, const RangeSet& locs, -- cgit v1.2.3 From 12b90553d784b9d4ddd1b48300af6345bdf1085f Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Tue, 7 Mar 2017 14:44:14 -0800 Subject: More cleanup to imgdiff & imgpatch Also remove the utils in applypatch and replace them with the corresponding libbase functions. Test: recovery tests pass. Change-Id: I77254c141bd3e7d3d6894c23b60e866009516f81 --- applypatch/Android.mk | 15 ++-- applypatch/imgdiff.cpp | 185 ++++++++++++++++++++++----------------- applypatch/imgpatch.cpp | 25 ++++-- applypatch/utils.cpp | 65 -------------- applypatch/utils.h | 31 ------- tests/component/imgdiff_test.cpp | 9 +- 6 files changed, 133 insertions(+), 197 deletions(-) delete mode 100644 applypatch/utils.cpp delete mode 100644 applypatch/utils.h diff --git a/applypatch/Android.mk b/applypatch/Android.mk index 8be5c36be..a7412d238 100644 --- a/applypatch/Android.mk +++ b/applypatch/Android.mk @@ -21,8 +21,7 @@ LOCAL_SRC_FILES := \ applypatch.cpp \ bspatch.cpp \ freecache.cpp \ - imgpatch.cpp \ - utils.cpp + imgpatch.cpp LOCAL_MODULE := libapplypatch LOCAL_MODULE_TAGS := eng LOCAL_C_INCLUDES := \ @@ -46,8 +45,7 @@ include $(BUILD_STATIC_LIBRARY) include $(CLEAR_VARS) LOCAL_SRC_FILES := \ bspatch.cpp \ - imgpatch.cpp \ - utils.cpp + imgpatch.cpp LOCAL_MODULE := libimgpatch LOCAL_C_INCLUDES := \ $(LOCAL_PATH)/include \ @@ -56,6 +54,7 @@ LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include LOCAL_STATIC_LIBRARIES := \ libcrypto \ libbspatch \ + libbase \ libbz \ libz LOCAL_CFLAGS := \ @@ -68,8 +67,7 @@ include $(BUILD_STATIC_LIBRARY) include $(CLEAR_VARS) LOCAL_SRC_FILES := \ bspatch.cpp \ - imgpatch.cpp \ - utils.cpp + imgpatch.cpp LOCAL_MODULE := libimgpatch LOCAL_MODULE_HOST_OS := linux LOCAL_C_INCLUDES := \ @@ -79,6 +77,7 @@ LOCAL_EXPORT_C_INCLUDE_DIRS := $(LOCAL_PATH)/include LOCAL_STATIC_LIBRARIES := \ libcrypto \ libbspatch \ + libbase \ libbz \ libz LOCAL_CFLAGS := \ @@ -123,9 +122,7 @@ LOCAL_SHARED_LIBRARIES := \ LOCAL_CFLAGS := -Werror include $(BUILD_EXECUTABLE) -libimgdiff_src_files := \ - imgdiff.cpp \ - utils.cpp +libimgdiff_src_files := imgdiff.cpp # libbsdiff is compiled with -D_FILE_OFFSET_BITS=64. libimgdiff_cflags := \ diff --git a/applypatch/imgdiff.cpp b/applypatch/imgdiff.cpp index fba74e836..41d73ab98 100644 --- a/applypatch/imgdiff.cpp +++ b/applypatch/imgdiff.cpp @@ -145,12 +145,22 @@ #include #include -#include "utils.h" - using android::base::get_unaligned; static constexpr auto BUFFER_SIZE = 0x8000; +// If we use this function to write the offset and length (type size_t), their values should not +// exceed 2^63; because the signed bit will be casted away. +static inline bool Write8(int fd, int64_t value) { + return android::base::WriteFully(fd, &value, sizeof(int64_t)); +} + +// Similarly, the value should not exceed 2^31 if we are casting from size_t (e.g. target chunk +// size). +static inline bool Write4(int fd, int32_t value) { + return android::base::WriteFully(fd, &value, sizeof(int32_t)); +} + class ImageChunk { public: static constexpr auto WINDOWBITS = -15; // 32kb window; negative to indicate a raw stream. @@ -163,11 +173,12 @@ class ImageChunk { start_(start), input_file_ptr_(file_content), raw_data_len_(raw_data_len), - entry_name_(""), compress_level_(6), source_start_(0), source_len_(0), - source_uncompressed_len_(0) {} + source_uncompressed_len_(0) { + CHECK(file_content != nullptr) << "input file container can't be nullptr"; + } int GetType() const { return type_; @@ -199,7 +210,8 @@ class ImageChunk { } size_t GetHeaderSize(size_t patch_size) const; - size_t WriteHeaderToFile(FILE* f, const std::vector patch, size_t offset); + // Return the offset of the next patch into the patch data. + size_t WriteHeaderToFd(int fd, const std::vector& patch, size_t offset); /* * Cause a gzip chunk to be treated as a normal chunk (ie, as a blob @@ -222,9 +234,9 @@ class ImageChunk { void MergeAdjacentNormal(const ImageChunk& other); private: - int type_; // CHUNK_NORMAL, CHUNK_DEFLATE, CHUNK_RAW - size_t start_; // offset of chunk in the original input file - const std::vector* input_file_ptr_; // pointer to the full content of original input file + int type_; // CHUNK_NORMAL, CHUNK_DEFLATE, CHUNK_RAW + size_t start_; // offset of chunk in the original input file + const std::vector* input_file_ptr_; // ptr to the full content of original input file size_t raw_data_len_; // --- for CHUNK_DEFLATE chunks only: --- @@ -280,11 +292,11 @@ void ImageChunk::SetSourceInfo(const ImageChunk& src) { } void ImageChunk::SetEntryName(std::string entryname) { - entry_name_ = entryname; + entry_name_ = std::move(entryname); } void ImageChunk::SetUncompressedData(std::vector data) { - uncompressed_data_ = data; + uncompressed_data_ = std::move(data); } bool ImageChunk::SetBonusData(const std::vector& bonus_data) { @@ -295,7 +307,7 @@ bool ImageChunk::SetBonusData(const std::vector& bonus_data) { return true; } -// Convert CHUNK_NORMAL & CHUNK_DEFLATE to CHUNK_RAW if the terget size is +// Convert CHUNK_NORMAL & CHUNK_DEFLATE to CHUNK_RAW if the target size is // smaller. Also take the header size into account during size comparison. bool ImageChunk::ChangeChunkToRaw(size_t patch_size) { if (type_ == CHUNK_RAW) { @@ -310,6 +322,7 @@ bool ImageChunk::ChangeChunkToRaw(size_t patch_size) { void ImageChunk::ChangeDeflateChunkToNormal() { if (type_ != CHUNK_DEFLATE) return; type_ = CHUNK_NORMAL; + entry_name_.clear(); uncompressed_data_.clear(); } @@ -317,7 +330,7 @@ void ImageChunk::ChangeDeflateChunkToNormal() { // header_type 4 bytes // CHUNK_NORMAL 8*3 = 24 bytes // CHUNK_DEFLATE 8*5 + 4*5 = 60 bytes -// CHUNK_RAW 4 bytes +// CHUNK_RAW 4 bytes + patch_size size_t ImageChunk::GetHeaderSize(size_t patch_size) const { switch (type_) { case CHUNK_NORMAL: @@ -327,43 +340,43 @@ size_t ImageChunk::GetHeaderSize(size_t patch_size) const { case CHUNK_RAW: return 4 + 4 + patch_size; default: - printf("unexpected chunk type: %d\n", type_); // should not reach here. - CHECK(false); + CHECK(false) << "unexpected chunk type: " << type_; // Should not reach here. return 0; } } -size_t ImageChunk::WriteHeaderToFile(FILE* f, const std::vector patch, size_t offset) { - Write4(type_, f); +size_t ImageChunk::WriteHeaderToFd(int fd, const std::vector& patch, size_t offset) { + Write4(fd, type_); switch (type_) { case CHUNK_NORMAL: printf("normal (%10zu, %10zu) %10zu\n", start_, raw_data_len_, patch.size()); - Write8(source_start_, f); - Write8(source_len_, f); - Write8(offset, f); + Write8(fd, static_cast(source_start_)); + Write8(fd, static_cast(source_len_)); + Write8(fd, static_cast(offset)); return offset + patch.size(); case CHUNK_DEFLATE: printf("deflate (%10zu, %10zu) %10zu %s\n", start_, raw_data_len_, patch.size(), entry_name_.c_str()); - Write8(source_start_, f); - Write8(source_len_, f); - Write8(offset, f); - Write8(source_uncompressed_len_, f); - Write8(uncompressed_data_.size(), f); - Write4(compress_level_, f); - Write4(METHOD, f); - Write4(WINDOWBITS, f); - Write4(MEMLEVEL, f); - Write4(STRATEGY, f); + Write8(fd, static_cast(source_start_)); + Write8(fd, static_cast(source_len_)); + Write8(fd, static_cast(offset)); + Write8(fd, static_cast(source_uncompressed_len_)); + Write8(fd, static_cast(uncompressed_data_.size())); + Write4(fd, compress_level_); + Write4(fd, METHOD); + Write4(fd, WINDOWBITS); + Write4(fd, MEMLEVEL); + Write4(fd, STRATEGY); return offset + patch.size(); case CHUNK_RAW: printf("raw (%10zu, %10zu)\n", start_, raw_data_len_); - Write4(patch.size(), f); - fwrite(patch.data(), 1, patch.size(), f); + Write4(fd, static_cast(patch.size())); + if (!android::base::WriteFully(fd, patch.data(), patch.size())) { + CHECK(false) << "failed to write " << patch.size() <<" bytes patch"; + } return offset; default: - printf("unexpected chunk type: %d\n", type_); - CHECK(false); + CHECK(false) << "unexpected chunk type: " << type_; return offset; } } @@ -480,20 +493,21 @@ static bool GetZipFileSize(const std::vector& zip_file, size_t* input_f static bool ReadZip(const char* filename, std::vector* chunks, std::vector* zip_file, bool include_pseudo_chunk) { - CHECK(zip_file != nullptr); + CHECK(chunks != nullptr && zip_file != nullptr); + + android::base::unique_fd fd(open(filename, O_RDONLY)); + if (fd == -1) { + printf("failed to open \"%s\" %s\n", filename, strerror(errno)); + return false; + } struct stat st; - if (stat(filename, &st) != 0) { + if (fstat(fd, &st) != 0) { printf("failed to stat \"%s\": %s\n", filename, strerror(errno)); return false; } size_t sz = static_cast(st.st_size); zip_file->resize(sz); - android::base::unique_fd fd(open(filename, O_RDONLY)); - if (fd == -1) { - printf("failed to open \"%s\" %s\n", filename, strerror(errno)); - return false; - } if (!android::base::ReadFully(fd, zip_file->data(), sz)) { printf("failed to read \"%s\" %s\n", filename, strerror(errno)); return false; @@ -596,20 +610,21 @@ static bool ReadZip(const char* filename, std::vector* chunks, // Read the given file and break it up into chunks, and putting the data in to a vector. static bool ReadImage(const char* filename, std::vector* chunks, std::vector* img) { - CHECK(img != nullptr); + CHECK(chunks != nullptr && img != nullptr); + + android::base::unique_fd fd(open(filename, O_RDONLY)); + if (fd == -1) { + printf("failed to open \"%s\" %s\n", filename, strerror(errno)); + return false; + } struct stat st; - if (stat(filename, &st) != 0) { + if (fstat(fd, &st) != 0) { printf("failed to stat \"%s\": %s\n", filename, strerror(errno)); return false; } size_t sz = static_cast(st.st_size); img->resize(sz); - android::base::unique_fd fd(open(filename, O_RDONLY)); - if (fd == -1) { - printf("failed to open \"%s\" %s\n", filename, strerror(errno)); - return false; - } if (!android::base::ReadFully(fd, img->data(), sz)) { printf("failed to read \"%s\" %s\n", filename, strerror(errno)); return false; @@ -618,9 +633,8 @@ static bool ReadImage(const char* filename, std::vector* chunks, size_t pos = 0; while (pos < sz) { - if (sz - pos >= 4 && img->at(pos) == 0x1f && img->at(pos + 1) == 0x8b && - img->at(pos + 2) == 0x08 && // deflate compression - img->at(pos + 3) == 0x00) { // no header flags + // 0x00 no header flags, 0x08 deflate compression, 0x1f8b gzip magic number + if (sz - pos >= 4 && get_unaligned(img->data() + pos) == 0x00088b1f) { // 'pos' is the offset of the start of a gzip chunk. size_t chunk_offset = pos; @@ -695,7 +709,7 @@ static bool ReadImage(const char* filename, std::vector* chunks, // the uncompressed data. Double-check to make sure that it // matches the size of the data we got when we actually did // the decompression. - size_t footer_size = Read4(img->data() + pos - 4); + size_t footer_size = get_unaligned(img->data() + pos - 4); if (footer_size != body.DataLengthForPatch()) { printf("Error: footer size %zu != decompressed size %zu\n", footer_size, body.GetRawDataLength()); @@ -708,9 +722,8 @@ static bool ReadImage(const char* filename, std::vector* chunks, // Scan forward until we find a gzip header. size_t data_len = 0; while (data_len + pos < sz) { - if (data_len + pos + 4 <= sz && img->at(pos + data_len) == 0x1f && - img->at(pos + data_len + 1) == 0x8b && img->at(pos + data_len + 2) == 0x08 && - img->at(pos + data_len + 3) == 0x00) { + if (data_len + pos + 4 <= sz && + get_unaligned(img->data() + pos + data_len) == 0x00088b1f) { break; } data_len++; @@ -759,13 +772,19 @@ static bool MakePatch(const ImageChunk* src, ImageChunk* tgt, std::vector(st.st_size); + // Change the chunk type to raw if the patch takes less space that way. if (tgt->ChangeChunkToRaw(sz)) { unlink(ptemp); size_t patch_size = tgt->DataLengthForPatch(); @@ -773,12 +792,6 @@ static bool MakePatch(const ImageChunk* src, ImageChunk* tgt, std::vectorDataForPatch(), tgt->DataForPatch() + patch_size, patch_data->begin()); return true; } - - android::base::unique_fd patch_fd(open(ptemp, O_RDONLY)); - if (patch_fd == -1) { - printf("failed to open %s: %s\n", ptemp, strerror(errno)); - return false; - } patch_data->resize(sz); if (!android::base::ReadFully(patch_fd, patch_data->data(), sz)) { printf("failed to read \"%s\" %s\n", ptemp, strerror(errno)); @@ -845,18 +858,19 @@ int imgdiff(int argc, const char** argv) { std::vector bonus_data; if (argc >= 3 && strcmp(argv[1], "-b") == 0) { + android::base::unique_fd fd(open(argv[2], O_RDONLY)); + if (fd == -1) { + printf("failed to open bonus file %s: %s\n", argv[2], strerror(errno)); + return 1; + } struct stat st; - if (stat(argv[2], &st) != 0) { + if (fstat(fd, &st) != 0) { printf("failed to stat bonus file %s: %s\n", argv[2], strerror(errno)); return 1; } + size_t bonus_size = st.st_size; bonus_data.resize(bonus_size); - android::base::unique_fd fd(open(argv[2], O_RDONLY)); - if (fd == -1) { - printf("failed to open bonus file %s: %s\n", argv[2], strerror(errno)); - return 1; - } if (!android::base::ReadFully(fd, bonus_data.data(), bonus_size)) { printf("failed to read bonus file %s: %s\n", argv[2], strerror(errno)); return 1; @@ -999,9 +1013,15 @@ int imgdiff(int argc, const char** argv) { ImageChunk* src; if (tgt_chunks[i].GetType() == CHUNK_DEFLATE && (src = FindChunkByName(tgt_chunks[i].GetEntryName(), src_chunks))) { - MakePatch(src, &tgt_chunks[i], &patch_data[i], nullptr); + if (!MakePatch(src, &tgt_chunks[i], &patch_data[i], nullptr)) { + printf("Failed to generate patch for target chunk %zu: ", i); + return 1; + } } else { - MakePatch(&src_chunks[0], &tgt_chunks[i], &patch_data[i], &bsdiff_cache); + if (!MakePatch(&src_chunks[0], &tgt_chunks[i], &patch_data[i], &bsdiff_cache)) { + printf("Failed to generate patch for target chunk %zu: ", i); + return 1; + } } } else { if (i == 1 && !bonus_data.empty()) { @@ -1009,7 +1029,10 @@ int imgdiff(int argc, const char** argv) { src_chunks[i].SetBonusData(bonus_data); } - MakePatch(&src_chunks[i], &tgt_chunks[i], &patch_data[i], nullptr); + if (!MakePatch(&src_chunks[i], &tgt_chunks[i], &patch_data[i], nullptr)) { + printf("Failed to generate patch for target chunk %zu: ", i); + return 1; + } } printf("patch %3zu is %zu bytes (of %zu)\n", i, patch_data[i].size(), src_chunks[i].GetRawDataLength()); @@ -1030,28 +1053,32 @@ int imgdiff(int argc, const char** argv) { size_t offset = total_header_size; - FILE* f = fopen(argv[3], "wb"); - if (f == nullptr) { + android::base::unique_fd patch_fd(open(argv[3], O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR)); + if (patch_fd == -1) { printf("failed to open \"%s\": %s\n", argv[3], strerror(errno)); + return 1; } // Write out the headers. - - fwrite("IMGDIFF2", 1, 8, f); - Write4(static_cast(tgt_chunks.size()), f); + if (!android::base::WriteStringToFd("IMGDIFF2", patch_fd)) { + printf("failed to write \"IMGDIFF2\" to \"%s\": %s\n", argv[3], strerror(errno)); + return 1; + } + Write4(patch_fd, static_cast(tgt_chunks.size())); for (size_t i = 0; i < tgt_chunks.size(); ++i) { printf("chunk %zu: ", i); - offset = tgt_chunks[i].WriteHeaderToFile(f, patch_data[i], offset); + offset = tgt_chunks[i].WriteHeaderToFd(patch_fd, patch_data[i], offset); } // Append each chunk's bsdiff patch, in order. for (size_t i = 0; i < tgt_chunks.size(); ++i) { if (tgt_chunks[i].GetType() != CHUNK_RAW) { - fwrite(patch_data[i].data(), 1, patch_data[i].size(), f); + if (!android::base::WriteFully(patch_fd, patch_data[i].data(), patch_data[i].size())) { + CHECK(false) << "failed to write " << patch_data[i].size() << " bytes patch for chunk " + << i; + } } } - fclose(f); - return 0; } diff --git a/applypatch/imgpatch.cpp b/applypatch/imgpatch.cpp index 8f4a2a42b..adcc61fd6 100644 --- a/applypatch/imgpatch.cpp +++ b/applypatch/imgpatch.cpp @@ -31,10 +31,17 @@ #include #include +#include #include #include -#include "utils.h" +static inline int64_t Read8(const void *address) { + return android::base::get_unaligned(address); +} + +static inline int32_t Read4(const void *address) { + return android::base::get_unaligned(address); +} int ApplyImagePatch(const unsigned char* old_data, ssize_t old_size, const unsigned char* patch_data, ssize_t patch_size, @@ -86,9 +93,9 @@ int ApplyImagePatch(const unsigned char* old_data, ssize_t old_size, const Value return -1; } - size_t src_start = Read8(normal_header); - size_t src_len = Read8(normal_header + 8); - size_t patch_offset = Read8(normal_header + 16); + size_t src_start = static_cast(Read8(normal_header)); + size_t src_len = static_cast(Read8(normal_header + 8)); + size_t patch_offset = static_cast(Read8(normal_header + 16)); if (src_start + src_len > static_cast(old_size)) { printf("source data too short\n"); @@ -125,11 +132,11 @@ int ApplyImagePatch(const unsigned char* old_data, ssize_t old_size, const Value return -1; } - size_t src_start = Read8(deflate_header); - size_t src_len = Read8(deflate_header + 8); - size_t patch_offset = Read8(deflate_header + 16); - size_t expanded_len = Read8(deflate_header + 24); - size_t target_len = Read8(deflate_header + 32); + size_t src_start = static_cast(Read8(deflate_header)); + size_t src_len = static_cast(Read8(deflate_header + 8)); + size_t patch_offset = static_cast(Read8(deflate_header + 16)); + size_t expanded_len = static_cast(Read8(deflate_header + 24)); + size_t target_len = static_cast(Read8(deflate_header + 32)); int level = Read4(deflate_header + 40); int method = Read4(deflate_header + 44); int windowBits = Read4(deflate_header + 48); diff --git a/applypatch/utils.cpp b/applypatch/utils.cpp deleted file mode 100644 index 450dc8d76..000000000 --- a/applypatch/utils.cpp +++ /dev/null @@ -1,65 +0,0 @@ -/* - * Copyright (C) 2009 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#include - -#include "utils.h" - -/** Write a 4-byte value to f in little-endian order. */ -void Write4(int value, FILE* f) { - fputc(value & 0xff, f); - fputc((value >> 8) & 0xff, f); - fputc((value >> 16) & 0xff, f); - fputc((value >> 24) & 0xff, f); -} - -/** Write an 8-byte value to f in little-endian order. */ -void Write8(int64_t value, FILE* f) { - fputc(value & 0xff, f); - fputc((value >> 8) & 0xff, f); - fputc((value >> 16) & 0xff, f); - fputc((value >> 24) & 0xff, f); - fputc((value >> 32) & 0xff, f); - fputc((value >> 40) & 0xff, f); - fputc((value >> 48) & 0xff, f); - fputc((value >> 56) & 0xff, f); -} - -int Read2(const void* pv) { - const unsigned char* p = reinterpret_cast(pv); - return (int)(((unsigned int)p[1] << 8) | - (unsigned int)p[0]); -} - -int Read4(const void* pv) { - const unsigned char* p = reinterpret_cast(pv); - return (int)(((unsigned int)p[3] << 24) | - ((unsigned int)p[2] << 16) | - ((unsigned int)p[1] << 8) | - (unsigned int)p[0]); -} - -int64_t Read8(const void* pv) { - const unsigned char* p = reinterpret_cast(pv); - return (int64_t)(((uint64_t)p[7] << 56) | - ((uint64_t)p[6] << 48) | - ((uint64_t)p[5] << 40) | - ((uint64_t)p[4] << 32) | - ((uint64_t)p[3] << 24) | - ((uint64_t)p[2] << 16) | - ((uint64_t)p[1] << 8) | - (uint64_t)p[0]); -} diff --git a/applypatch/utils.h b/applypatch/utils.h deleted file mode 100644 index c7c8e90e2..000000000 --- a/applypatch/utils.h +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright (C) 2009 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef _BUILD_TOOLS_APPLYPATCH_UTILS_H -#define _BUILD_TOOLS_APPLYPATCH_UTILS_H - -#include -#include - -// Read and write little-endian values of various sizes. - -void Write4(int value, FILE* f); -void Write8(int64_t value, FILE* f); -int Read2(const void* p); -int Read4(const void* p); -int64_t Read8(const void* p); - -#endif // _BUILD_TOOLS_APPLYPATCH_UTILS_H diff --git a/tests/component/imgdiff_test.cpp b/tests/component/imgdiff_test.cpp index be2dd385b..2f648501c 100644 --- a/tests/component/imgdiff_test.cpp +++ b/tests/component/imgdiff_test.cpp @@ -18,13 +18,14 @@ #include #include +#include #include #include #include #include #include -#include "applypatch/utils.h" +using android::base::get_unaligned; static ssize_t MemorySink(const unsigned char* data, ssize_t len, void* token) { std::string* s = static_cast(token); @@ -41,7 +42,7 @@ static void verify_patch_header(const std::string& patch, size_t* num_normal, si ASSERT_GE(size, 12U); ASSERT_EQ("IMGDIFF2", std::string(data, 8)); - const int num_chunks = Read4(data + 8); + const int num_chunks = get_unaligned(data + 8); ASSERT_GE(num_chunks, 0); size_t normal = 0; @@ -51,7 +52,7 @@ static void verify_patch_header(const std::string& patch, size_t* num_normal, si size_t pos = 12; for (int i = 0; i < num_chunks; ++i) { ASSERT_LE(pos + 4, size); - int type = Read4(data + pos); + int type = get_unaligned(data + pos); pos += 4; if (type == CHUNK_NORMAL) { pos += 24; @@ -59,7 +60,7 @@ static void verify_patch_header(const std::string& patch, size_t* num_normal, si normal++; } else if (type == CHUNK_RAW) { ASSERT_LE(pos + 4, size); - ssize_t data_len = Read4(data + pos); + ssize_t data_len = get_unaligned(data + pos); ASSERT_GT(data_len, 0); pos += 4 + data_len; ASSERT_LE(pos, size); -- cgit v1.2.3 From 56ebe620a241577ee2573649e57cc79c9931145b Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Thu, 16 Mar 2017 00:48:21 -0700 Subject: Add a test to perform block_image_update Add the following tests: stash src bspatch stashed_src tgt free stashed_src (expected a successful update) stash src free stashed_src fail_the_update (expected stashed_src freed) Bug: 36242722 Test: Test identified unfreed stashes correctly. Change-Id: I5a136e8dc31774367972fbfe8c63cbc1ddb3a113 --- tests/Android.mk | 9 ++- tests/component/updater_test.cpp | 120 +++++++++++++++++++++++++++++++++++++-- 2 files changed, 123 insertions(+), 6 deletions(-) diff --git a/tests/Android.mk b/tests/Android.mk index ec971b38c..65f736d13 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -81,7 +81,10 @@ include $(BUILD_NATIVE_TEST) # Component tests include $(CLEAR_VARS) -LOCAL_CFLAGS := -Werror +LOCAL_CFLAGS := \ + -Werror \ + -D_FILE_OFFSET_BITS=64 + LOCAL_ADDITIONAL_DEPENDENCIES := $(LOCAL_PATH)/Android.mk LOCAL_MODULE := recovery_component_test LOCAL_C_INCLUDES := bootable/recovery @@ -136,6 +139,10 @@ LOCAL_STATIC_LIBRARIES := \ libz \ libbase \ libtune2fs \ + libfec \ + libfec_rs \ + libsquashfs_utils \ + libcutils \ $(tune2fs_static_libraries) testdata_files := $(call find-subdir-files, testdata/*) diff --git a/tests/component/updater_test.cpp b/tests/component/updater_test.cpp index 8c4bdbaa4..4f8349e2f 100644 --- a/tests/component/updater_test.cpp +++ b/tests/component/updater_test.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include @@ -27,12 +28,17 @@ #include #include #include +#include #include #include +#include #include "common/test_constants.h" #include "edify/expr.h" #include "error_code.h" +#include "otautil/SysUtil.h" +#include "print_sha1.h" +#include "updater/blockimg.h" #include "updater/install.h" #include "updater/updater.h" @@ -64,12 +70,19 @@ static void expect(const char* expected, const char* expr_str, CauseCode cause_c ASSERT_EQ(cause_code, state.cause_code); } +static std::string get_sha1(const std::string& content) { + uint8_t digest[SHA_DIGEST_LENGTH]; + SHA1(reinterpret_cast(content.c_str()), content.size(), digest); + return print_sha1(digest); +} + class UpdaterTest : public ::testing::Test { - protected: - virtual void SetUp() { - RegisterBuiltins(); - RegisterInstallFunctions(); - } + protected: + virtual void SetUp() override { + RegisterBuiltins(); + RegisterInstallFunctions(); + RegisterBlockImageFunctions(); + } }; TEST_F(UpdaterTest, getprop) { @@ -447,3 +460,100 @@ TEST_F(UpdaterTest, show_progress) { // recovery-updater protocol expects 3 tokens ("progress "). ASSERT_EQ(3U, android::base::Split(cmd, " ").size()); } + +TEST_F(UpdaterTest, block_image_update) { + // Create a zip file with new_data and patch_data. + TemporaryFile zip_file; + FILE* zip_file_ptr = fdopen(zip_file.fd, "wb"); + ZipWriter zip_writer(zip_file_ptr); + + // Add a dummy new data. + ASSERT_EQ(0, zip_writer.StartEntry("new_data", 0)); + ASSERT_EQ(0, zip_writer.FinishEntry()); + + // Generate and add the patch data. + std::string src_content = std::string(4096, 'a') + std::string(4096, 'c'); + std::string tgt_content = std::string(4096, 'b') + std::string(4096, 'd'); + TemporaryFile patch_file; + ASSERT_EQ(0, bsdiff::bsdiff(reinterpret_cast(src_content.data()), + src_content.size(), reinterpret_cast(tgt_content.data()), + tgt_content.size(), patch_file.path, nullptr)); + std::string patch_content; + ASSERT_TRUE(android::base::ReadFileToString(patch_file.path, &patch_content)); + ASSERT_EQ(0, zip_writer.StartEntry("patch_data", 0)); + ASSERT_EQ(0, zip_writer.WriteBytes(patch_content.data(), patch_content.size())); + ASSERT_EQ(0, zip_writer.FinishEntry()); + + // Add two transfer lists. The first one contains a bsdiff; and we expect the update to succeed. + std::string src_hash = get_sha1(src_content); + std::string tgt_hash = get_sha1(tgt_content); + std::vector transfer_list = { + "4", + "2", + "0", + "2", + "stash " + src_hash + " 2,0,2", + android::base::StringPrintf("bsdiff 0 %zu %s %s 2,0,2 2 - %s:2,0,2", patch_content.size(), + src_hash.c_str(), tgt_hash.c_str(), src_hash.c_str()), + "free " + src_hash, + }; + ASSERT_EQ(0, zip_writer.StartEntry("transfer_list", 0)); + std::string commands = android::base::Join(transfer_list, '\n'); + ASSERT_EQ(0, zip_writer.WriteBytes(commands.data(), commands.size())); + ASSERT_EQ(0, zip_writer.FinishEntry()); + + // Stash and free some blocks, then fail the 2nd update intentionally. + std::vector fail_transfer_list = { + "4", + "2", + "0", + "2", + "stash " + tgt_hash + " 2,0,2", + "free " + tgt_hash, + "fail", + }; + ASSERT_EQ(0, zip_writer.StartEntry("fail_transfer_list", 0)); + std::string fail_commands = android::base::Join(fail_transfer_list, '\n'); + ASSERT_EQ(0, zip_writer.WriteBytes(fail_commands.data(), fail_commands.size())); + ASSERT_EQ(0, zip_writer.FinishEntry()); + ASSERT_EQ(0, zip_writer.Finish()); + ASSERT_EQ(0, fclose(zip_file_ptr)); + + MemMapping map; + ASSERT_EQ(0, sysMapFile(zip_file.path, &map)); + ZipArchiveHandle handle; + ASSERT_EQ(0, OpenArchiveFromMemory(map.addr, map.length, zip_file.path, &handle)); + + // Set up the handler, command_pipe, patch offset & length. + UpdaterInfo updater_info; + updater_info.package_zip = handle; + TemporaryFile temp_pipe; + updater_info.cmd_pipe = fopen(temp_pipe.path, "wb"); + updater_info.package_zip_addr = map.addr; + updater_info.package_zip_len = map.length; + + // Execute the commands in the 1st transfer list. + TemporaryFile update_file; + ASSERT_TRUE(android::base::WriteStringToFile(src_content, update_file.path)); + std::string script = "block_image_update(\"" + std::string(update_file.path) + + R"(", package_extract_file("transfer_list"), "new_data", "patch_data"))"; + expect("t", script.c_str(), kNoCause, &updater_info); + // The update_file should be patched correctly. + std::string updated_content; + ASSERT_TRUE(android::base::ReadFileToString(update_file.path, &updated_content)); + ASSERT_EQ(tgt_hash, get_sha1(updated_content)); + + // Expect the 2nd update to fail, but expect the stashed blocks to be freed. + script = "block_image_update(\"" + std::string(update_file.path) + + R"(", package_extract_file("fail_transfer_list"), "new_data", "patch_data"))"; + expect("", script.c_str(), kNoCause, &updater_info); + // Updater generates the stash name based on the input file name. + std::string name_digest = get_sha1(update_file.path); + std::string stash_base = "/cache/recovery/" + name_digest; + ASSERT_EQ(0, access(stash_base.c_str(), F_OK)); + ASSERT_EQ(-1, access((stash_base + tgt_hash).c_str(), F_OK)); + ASSERT_EQ(0, rmdir(stash_base.c_str())); + + ASSERT_EQ(0, fclose(updater_info.cmd_pipe)); + CloseArchive(handle); +} -- cgit v1.2.3 From 2cd36ba52265c824e226378bdcfc66969411c1b0 Mon Sep 17 00:00:00 2001 From: Tianjie Xu Date: Wed, 15 Mar 2017 23:52:46 +0000 Subject: Revert "Revert "Print SHA-1 in hex for corrupted blocks"" This reverts commit 90eff6a340f9983792d700df3b1ea0203aced207. Also fix the bug where stashed blocks are not freed. Bug: 21124445 Test: Previous failed update succeeded on bullhead Change-Id: I23d232331a2beb51b6dcc82c957c87bc247d0268 --- updater/blockimg.cpp | 146 ++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 140 insertions(+), 6 deletions(-) diff --git a/updater/blockimg.cpp b/updater/blockimg.cpp index 6227154e5..0fa83d9d5 100644 --- a/updater/blockimg.cpp +++ b/updater/blockimg.cpp @@ -67,6 +67,21 @@ struct RangeSet { size_t count; // Limit is INT_MAX. size_t size; std::vector pos; // Actual limit is INT_MAX. + + // Get the block number for the ith(starting from 0) block in the range set. + int get_block(size_t idx) const { + if (idx >= size) { + LOG(ERROR) << "index: " << idx << " is greater than range set size: " << size; + return -1; + } + for (size_t i = 0; i < pos.size(); i += 2) { + if (idx < pos[i + 1] - pos[i]) { + return pos[i] + idx; + } + idx -= (pos[i + 1] - pos[i]); + } + return -1; + } }; static CauseCode failure_type = kNoCause; @@ -442,6 +457,117 @@ static int LoadSrcTgtVersion1(CommandParameters& params, RangeSet& tgt, size_t& return rc; } +// Print the hash in hex for corrupted source blocks (excluding the stashed blocks which is +// handled separately). +static void PrintHashForCorruptedSourceBlocks(const CommandParameters& params, + const std::vector& buffer) { + LOG(INFO) << "unexpected contents of source blocks in cmd:\n" << params.cmdline; + if (params.version < 3) { + // TODO handle version 1,2 + LOG(WARNING) << "version number " << params.version << " is not supported to print hashes"; + return; + } + + CHECK(params.tokens[0] == "move" || params.tokens[0] == "bsdiff" || + params.tokens[0] == "imgdiff"); + + size_t pos = 0; + // Command example: + // move [ ] + // bsdiff + // [ ] + if (params.tokens[0] == "move") { + // src_range for move starts at the 4th position. + if (params.tokens.size() < 5) { + LOG(ERROR) << "failed to parse source range in cmd:\n" << params.cmdline; + return; + } + pos = 4; + } else { + // src_range for diff starts at the 7th position. + if (params.tokens.size() < 8) { + LOG(ERROR) << "failed to parse source range in cmd:\n" << params.cmdline; + return; + } + pos = 7; + } + + // Source blocks in stash only, no work to do. + if (params.tokens[pos] == "-") { + return; + } + + RangeSet src = parse_range(params.tokens[pos++]); + + RangeSet locs; + // If there's no stashed blocks, content in the buffer is consecutive and has the same + // order as the source blocks. + if (pos == params.tokens.size()) { + locs.count = 1; + locs.size = src.size; + locs.pos = { 0, src.size }; + } else { + // Otherwise, the next token is the offset of the source blocks in the target range. + // Example: for the tokens <4,63946,63947,63948,63979> <4,6,7,8,39> ; + // We want to print SHA-1 for the data in buffer[6], buffer[8], buffer[9] ... buffer[38]; + // this corresponds to the 32 src blocks #63946, #63948, #63949 ... #63978. + locs = parse_range(params.tokens[pos++]); + CHECK_EQ(src.size, locs.size); + CHECK_EQ(locs.pos.size() % 2, static_cast(0)); + } + + LOG(INFO) << "printing hash in hex for " << src.size << " source blocks"; + for (size_t i = 0; i < src.size; i++) { + int block_num = src.get_block(i); + CHECK_NE(block_num, -1); + int buffer_index = locs.get_block(i); + CHECK_NE(buffer_index, -1); + CHECK_LE((buffer_index + 1) * BLOCKSIZE, buffer.size()); + + uint8_t digest[SHA_DIGEST_LENGTH]; + SHA1(buffer.data() + buffer_index * BLOCKSIZE, BLOCKSIZE, digest); + std::string hexdigest = print_sha1(digest); + LOG(INFO) << " block number: " << block_num << ", SHA-1: " << hexdigest; + } +} + +// If the calculated hash for the whole stash doesn't match the stash id, print the SHA-1 +// in hex for each block. +static void PrintHashForCorruptedStashedBlocks(const std::string& id, + const std::vector& buffer, + const RangeSet& src) { + LOG(INFO) << "printing hash in hex for stash_id: " << id; + CHECK_EQ(src.size * BLOCKSIZE, buffer.size()); + + for (size_t i = 0; i < src.size; i++) { + int block_num = src.get_block(i); + CHECK_NE(block_num, -1); + + uint8_t digest[SHA_DIGEST_LENGTH]; + SHA1(buffer.data() + i * BLOCKSIZE, BLOCKSIZE, digest); + std::string hexdigest = print_sha1(digest); + LOG(INFO) << " block number: " << block_num << ", SHA-1: " << hexdigest; + } +} + +// If the stash file doesn't exist, read the source blocks this stash contains and print the +// SHA-1 for these blocks. +static void PrintHashForMissingStashedBlocks(const std::string& id, int fd) { + if (stash_map.find(id) == stash_map.end()) { + LOG(ERROR) << "No stash saved for id: " << id; + return; + } + + LOG(INFO) << "print hash in hex for source blocks in missing stash: " << id; + const RangeSet& src = stash_map[id]; + std::vector buffer(src.size * BLOCKSIZE); + if (ReadBlocks(src, buffer, fd) == -1) { + LOG(ERROR) << "failed to read source blocks for stash: " << id; + return; + } + PrintHashForCorruptedStashedBlocks(id, buffer, src); +} + static int VerifyBlocks(const std::string& expected, const std::vector& buffer, const size_t blocks, bool printerror) { uint8_t digest[SHA_DIGEST_LENGTH]; @@ -540,6 +666,7 @@ static int LoadStash(CommandParameters& params, const std::string& base, const s } if (VerifyBlocks(id, buffer, src.size, true) != 0) { LOG(ERROR) << "failed to verify loaded source blocks in stash map."; + PrintHashForCorruptedStashedBlocks(id, buffer, src); return -1; } return 0; @@ -564,6 +691,7 @@ static int LoadStash(CommandParameters& params, const std::string& base, const s if (res == -1) { if (errno != ENOENT || printnoent) { PLOG(ERROR) << "stat \"" << fn << "\" failed"; + PrintHashForMissingStashedBlocks(id, params.fd); } return -1; } @@ -591,6 +719,13 @@ static int LoadStash(CommandParameters& params, const std::string& base, const s if (verify && VerifyBlocks(id, buffer, *blocks, true) != 0) { LOG(ERROR) << "unexpected contents in " << fn; + if (stash_map.find(id) == stash_map.end()) { + LOG(ERROR) << "failed to find source blocks number for stash " << id + << " when executing command: " << params.cmdname; + } else { + const RangeSet& src = stash_map[id]; + PrintHashForCorruptedStashedBlocks(id, buffer, src); + } DeleteFile(fn); return -1; } @@ -773,6 +908,7 @@ static int SaveStash(CommandParameters& params, const std::string& base, return -1; } blocks = src.size; + stash_map[id] = src; if (usehash && VerifyBlocks(id, buffer, blocks, true) != 0) { // Source blocks have unexpected contents. If we actually need this @@ -783,9 +919,8 @@ static int SaveStash(CommandParameters& params, const std::string& base, return 0; } - // In verify mode, save source range_set instead of stashing blocks. + // In verify mode, we don't need to stash any blocks. if (!params.canwrite && usehash) { - stash_map[id] = src; return 0; } @@ -1003,6 +1138,8 @@ static int LoadSrcTgtVersion3(CommandParameters& params, RangeSet& tgt, size_t& // Valid source data not available, update cannot be resumed LOG(ERROR) << "partition has unexpected contents"; + PrintHashForCorruptedSourceBlocks(params, params.buffer); + params.isunresumable = true; return -1; @@ -1071,9 +1208,8 @@ static int PerformCommandFree(CommandParameters& params) { const std::string& id = params.tokens[params.cpos++]; - if (!params.canwrite && stash_map.find(id) != stash_map.end()) { + if (stash_map.find(id) != stash_map.end()) { stash_map.erase(id); - return 0; } if (params.createdstash || params.canwrite) { @@ -1215,10 +1351,8 @@ static int PerformCommandDiff(CommandParameters& params) { if (params.canwrite) { if (status == 0) { LOG(INFO) << "patching " << blocks << " blocks to " << tgt.size; - Value patch_value(VAL_BLOB, std::string(reinterpret_cast(params.patch_start + offset), len)); - RangeSinkState rss(tgt); rss.fd = params.fd; rss.p_block = 0; -- cgit v1.2.3 From 9468fc0429cb076dc5ef7c4cda84f6efac3eb333 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Fri, 17 Mar 2017 00:57:37 -0700 Subject: Add the missing #include of . For the use of std::function and std::bind. They were relying on the transitive inclusion from . Test: mmma bootable/recovery Change-Id: Ia138e1cbdd035b11d6cdca9e16c5591303b6ee13 --- minui/events.cpp | 2 ++ ui.cpp | 1 + 2 files changed, 3 insertions(+) diff --git a/minui/events.cpp b/minui/events.cpp index fa44033d2..0e1fd44a0 100644 --- a/minui/events.cpp +++ b/minui/events.cpp @@ -24,6 +24,8 @@ #include #include +#include + #include "minui/minui.h" #define MAX_DEVICES 16 diff --git a/ui.cpp b/ui.cpp index 3ecd6d125..a796461c8 100644 --- a/ui.cpp +++ b/ui.cpp @@ -30,6 +30,7 @@ #include #include +#include #include #include -- cgit v1.2.3 From 5e535014dd7961fbf812abeaa27f3339775031f1 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Thu, 16 Mar 2017 17:37:38 -0700 Subject: Drop the dependency on 'ui' in verify_file(). verify_file() has a dependency on the global variable of 'ui' for posting the verification progress, which requires the users of libverifier to provide a UI instance. This CL adds an optional argument to verify_file() so that it can post the progress through the provided callback function. As a result, we can drop the MockUI class in verifier_test.cpp. Test: recovery_component_test passes. Test: verify_file() posts progress update when installing an OTA. Change-Id: I8b87d0f0d99777ea755d33d6dbbe2b6d44243bf1 --- install.cpp | 40 ++--- tests/component/verifier_test.cpp | 149 ++++++----------- verifier.cpp | 329 +++++++++++++++++++------------------- verifier.h | 14 +- 4 files changed, 235 insertions(+), 297 deletions(-) diff --git a/install.cpp b/install.cpp index ce89e0dc0..9123fc276 100644 --- a/install.cpp +++ b/install.cpp @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -578,23 +579,24 @@ install_package(const char* path, bool* wipe_cache, const char* install_file, } bool verify_package(const unsigned char* package_data, size_t package_size) { - std::vector loadedKeys; - if (!load_keys(PUBLIC_KEYS_FILE, loadedKeys)) { - LOG(ERROR) << "Failed to load keys"; - return false; - } - LOG(INFO) << loadedKeys.size() << " key(s) loaded from " << PUBLIC_KEYS_FILE; - - // Verify package. - ui->Print("Verifying update package...\n"); - auto t0 = std::chrono::system_clock::now(); - int err = verify_file(const_cast(package_data), package_size, loadedKeys); - std::chrono::duration duration = std::chrono::system_clock::now() - t0; - ui->Print("Update package verification took %.1f s (result %d).\n", duration.count(), err); - if (err != VERIFY_SUCCESS) { - LOG(ERROR) << "Signature verification failed"; - LOG(ERROR) << "error: " << kZipVerificationFailure; - return false; - } - return true; + std::vector loadedKeys; + if (!load_keys(PUBLIC_KEYS_FILE, loadedKeys)) { + LOG(ERROR) << "Failed to load keys"; + return false; + } + LOG(INFO) << loadedKeys.size() << " key(s) loaded from " << PUBLIC_KEYS_FILE; + + // Verify package. + ui->Print("Verifying update package...\n"); + auto t0 = std::chrono::system_clock::now(); + int err = verify_file(const_cast(package_data), package_size, loadedKeys, + std::bind(&RecoveryUI::SetProgress, ui, std::placeholders::_1)); + std::chrono::duration duration = std::chrono::system_clock::now() - t0; + ui->Print("Update package verification took %.1f s (result %d).\n", duration.count(), err); + if (err != VERIFY_SUCCESS) { + LOG(ERROR) << "Signature verification failed"; + LOG(ERROR) << "error: " << kZipVerificationFailure; + return false; + } + return true; } diff --git a/tests/component/verifier_test.cpp b/tests/component/verifier_test.cpp index b740af96b..460585d22 100644 --- a/tests/component/verifier_test.cpp +++ b/tests/component/verifier_test.cpp @@ -22,93 +22,34 @@ #include #include -#include #include #include -#include - +#include #include -#include +#include -#include "common.h" #include "common/test_constants.h" #include "otautil/SysUtil.h" -#include "ui.h" #include "verifier.h" -RecoveryUI* ui = NULL; - -class MockUI : public RecoveryUI { - bool Init(const std::string&) override { - return true; - } - void SetStage(int, int) override {} - void SetBackground(Icon /*icon*/) override {} - void SetSystemUpdateText(bool /*security_update*/) override {} - - void SetProgressType(ProgressType /*determinate*/) override {} - void ShowProgress(float /*portion*/, float /*seconds*/) override {} - void SetProgress(float /*fraction*/) override {} - - void ShowText(bool /*visible*/) override {} - bool IsTextVisible() override { - return false; - } - bool WasTextEverVisible() override { - return false; - } - void Print(const char* fmt, ...) override { - va_list ap; - va_start(ap, fmt); - vfprintf(stderr, fmt, ap); - va_end(ap); - } - void PrintOnScreenOnly(const char* fmt, ...) override { - va_list ap; - va_start(ap, fmt); - vfprintf(stderr, fmt, ap); - va_end(ap); - } - void ShowFile(const char*) override {} - - void StartMenu(const char* const* /*headers*/, const char* const* /*items*/, - int /*initial_selection*/) override {} - int SelectMenu(int /*sel*/) override { - return 0; - } - void EndMenu() override {} -}; - -void -ui_print(const char* format, ...) { - va_list ap; - va_start(ap, format); - vfprintf(stdout, format, ap); - va_end(ap); -} - class VerifierTest : public testing::TestWithParam> { - public: - MemMapping memmap; - std::vector certs; - - virtual void SetUp() { - std::vector args = GetParam(); - std::string package = from_testdata_base(args[0]); - if (sysMapFile(package.c_str(), &memmap) != 0) { - FAIL() << "Failed to mmap " << package << ": " << strerror(errno) << "\n"; - } - - for (auto it = ++(args.cbegin()); it != args.cend(); ++it) { - std::string public_key_file = from_testdata_base("testkey_" + *it + ".txt"); - ASSERT_TRUE(load_keys(public_key_file.c_str(), certs)); - } + protected: + void SetUp() override { + std::vector args = GetParam(); + std::string package = from_testdata_base(args[0]); + if (sysMapFile(package.c_str(), &memmap) != 0) { + FAIL() << "Failed to mmap " << package << ": " << strerror(errno) << "\n"; } - static void SetUpTestCase() { - ui = new MockUI(); + for (auto it = ++args.cbegin(); it != args.cend(); ++it) { + std::string public_key_file = from_testdata_base("testkey_" + *it + ".txt"); + ASSERT_TRUE(load_keys(public_key_file.c_str(), certs)); } + } + + MemMapping memmap; + std::vector certs; }; class VerifierSuccessTest : public VerifierTest { @@ -118,47 +59,47 @@ class VerifierFailureTest : public VerifierTest { }; TEST_P(VerifierSuccessTest, VerifySucceed) { - ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs), VERIFY_SUCCESS); + ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs, nullptr), VERIFY_SUCCESS); } TEST_P(VerifierFailureTest, VerifyFailure) { - ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs), VERIFY_FAILURE); + ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs, nullptr), VERIFY_FAILURE); } INSTANTIATE_TEST_CASE_P(SingleKeySuccess, VerifierSuccessTest, - ::testing::Values( - std::vector({"otasigned_v1.zip", "v1"}), - std::vector({"otasigned_v2.zip", "v2"}), - std::vector({"otasigned_v3.zip", "v3"}), - std::vector({"otasigned_v4.zip", "v4"}), - std::vector({"otasigned_v5.zip", "v5"}))); + ::testing::Values( + std::vector({"otasigned_v1.zip", "v1"}), + std::vector({"otasigned_v2.zip", "v2"}), + std::vector({"otasigned_v3.zip", "v3"}), + std::vector({"otasigned_v4.zip", "v4"}), + std::vector({"otasigned_v5.zip", "v5"}))); INSTANTIATE_TEST_CASE_P(MultiKeySuccess, VerifierSuccessTest, - ::testing::Values( - std::vector({"otasigned_v1.zip", "v1", "v2"}), - std::vector({"otasigned_v2.zip", "v5", "v2"}), - std::vector({"otasigned_v3.zip", "v5", "v1", "v3"}), - std::vector({"otasigned_v4.zip", "v5", "v1", "v4"}), - std::vector({"otasigned_v5.zip", "v4", "v1", "v5"}))); + ::testing::Values( + std::vector({"otasigned_v1.zip", "v1", "v2"}), + std::vector({"otasigned_v2.zip", "v5", "v2"}), + std::vector({"otasigned_v3.zip", "v5", "v1", "v3"}), + std::vector({"otasigned_v4.zip", "v5", "v1", "v4"}), + std::vector({"otasigned_v5.zip", "v4", "v1", "v5"}))); INSTANTIATE_TEST_CASE_P(WrongKey, VerifierFailureTest, - ::testing::Values( - std::vector({"otasigned_v1.zip", "v2"}), - std::vector({"otasigned_v2.zip", "v1"}), - std::vector({"otasigned_v3.zip", "v5"}), - std::vector({"otasigned_v4.zip", "v5"}), - std::vector({"otasigned_v5.zip", "v3"}))); + ::testing::Values( + std::vector({"otasigned_v1.zip", "v2"}), + std::vector({"otasigned_v2.zip", "v1"}), + std::vector({"otasigned_v3.zip", "v5"}), + std::vector({"otasigned_v4.zip", "v5"}), + std::vector({"otasigned_v5.zip", "v3"}))); INSTANTIATE_TEST_CASE_P(WrongHash, VerifierFailureTest, - ::testing::Values( - std::vector({"otasigned_v1.zip", "v3"}), - std::vector({"otasigned_v2.zip", "v4"}), - std::vector({"otasigned_v3.zip", "v1"}), - std::vector({"otasigned_v4.zip", "v2"}))); + ::testing::Values( + std::vector({"otasigned_v1.zip", "v3"}), + std::vector({"otasigned_v2.zip", "v4"}), + std::vector({"otasigned_v3.zip", "v1"}), + std::vector({"otasigned_v4.zip", "v2"}))); INSTANTIATE_TEST_CASE_P(BadPackage, VerifierFailureTest, - ::testing::Values( - std::vector({"random.zip", "v1"}), - std::vector({"fake-eocd.zip", "v1"}), - std::vector({"alter-metadata.zip", "v1"}), - std::vector({"alter-footer.zip", "v1"}))); + ::testing::Values( + std::vector({"random.zip", "v1"}), + std::vector({"fake-eocd.zip", "v1"}), + std::vector({"alter-metadata.zip", "v1"}), + std::vector({"alter-footer.zip", "v1"}))); diff --git a/verifier.cpp b/verifier.cpp index 44098f70e..00e13aa7d 100644 --- a/verifier.cpp +++ b/verifier.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -108,201 +109,193 @@ static bool read_pkcs7(uint8_t* pkcs7_der, size_t pkcs7_der_len, uint8_t** sig_d return *sig_der != NULL; } -// Look for an RSA signature embedded in the .ZIP file comment given -// the path to the zip. Verify it matches one of the given public -// keys. -// -// Return VERIFY_SUCCESS, VERIFY_FAILURE (if any error is encountered -// or no key matches the signature). - -int verify_file(unsigned char* addr, size_t length, - const std::vector& keys) { - ui->SetProgress(0.0); - - // An archive with a whole-file signature will end in six bytes: - // - // (2-byte signature start) $ff $ff (2-byte comment size) - // - // (As far as the ZIP format is concerned, these are part of the - // archive comment.) We start by reading this footer, this tells - // us how far back from the end we have to start reading to find - // the whole comment. +/* + * Looks for an RSA signature embedded in the .ZIP file comment given the path to the zip. Verifies + * that it matches one of the given public keys. A callback function can be optionally provided for + * posting the progress. + * + * Returns VERIFY_SUCCESS or VERIFY_FAILURE (if any error is encountered or no key matches the + * signature). + */ +int verify_file(unsigned char* addr, size_t length, const std::vector& keys, + const std::function& set_progress) { + if (set_progress) { + set_progress(0.0); + } + + // An archive with a whole-file signature will end in six bytes: + // + // (2-byte signature start) $ff $ff (2-byte comment size) + // + // (As far as the ZIP format is concerned, these are part of the archive comment.) We start by + // reading this footer, this tells us how far back from the end we have to start reading to find + // the whole comment. #define FOOTER_SIZE 6 - if (length < FOOTER_SIZE) { - LOG(ERROR) << "not big enough to contain footer"; - return VERIFY_FAILURE; - } + if (length < FOOTER_SIZE) { + LOG(ERROR) << "not big enough to contain footer"; + return VERIFY_FAILURE; + } - unsigned char* footer = addr + length - FOOTER_SIZE; + unsigned char* footer = addr + length - FOOTER_SIZE; - if (footer[2] != 0xff || footer[3] != 0xff) { - LOG(ERROR) << "footer is wrong"; - return VERIFY_FAILURE; - } + if (footer[2] != 0xff || footer[3] != 0xff) { + LOG(ERROR) << "footer is wrong"; + return VERIFY_FAILURE; + } - size_t comment_size = footer[4] + (footer[5] << 8); - size_t signature_start = footer[0] + (footer[1] << 8); - LOG(INFO) << "comment is " << comment_size << " bytes; signature is " << signature_start - << " bytes from end"; + size_t comment_size = footer[4] + (footer[5] << 8); + size_t signature_start = footer[0] + (footer[1] << 8); + LOG(INFO) << "comment is " << comment_size << " bytes; signature is " << signature_start + << " bytes from end"; - if (signature_start <= FOOTER_SIZE) { - LOG(ERROR) << "Signature start is in the footer"; - return VERIFY_FAILURE; - } + if (signature_start <= FOOTER_SIZE) { + LOG(ERROR) << "Signature start is in the footer"; + return VERIFY_FAILURE; + } #define EOCD_HEADER_SIZE 22 - // The end-of-central-directory record is 22 bytes plus any - // comment length. - size_t eocd_size = comment_size + EOCD_HEADER_SIZE; + // The end-of-central-directory record is 22 bytes plus any comment length. + size_t eocd_size = comment_size + EOCD_HEADER_SIZE; - if (length < eocd_size) { - LOG(ERROR) << "not big enough to contain EOCD"; - return VERIFY_FAILURE; - } + if (length < eocd_size) { + LOG(ERROR) << "not big enough to contain EOCD"; + return VERIFY_FAILURE; + } - // Determine how much of the file is covered by the signature. - // This is everything except the signature data and length, which - // includes all of the EOCD except for the comment length field (2 - // bytes) and the comment data. - size_t signed_len = length - eocd_size + EOCD_HEADER_SIZE - 2; + // Determine how much of the file is covered by the signature. This is everything except the + // signature data and length, which includes all of the EOCD except for the comment length field + // (2 bytes) and the comment data. + size_t signed_len = length - eocd_size + EOCD_HEADER_SIZE - 2; - unsigned char* eocd = addr + length - eocd_size; + unsigned char* eocd = addr + length - eocd_size; - // If this is really is the EOCD record, it will begin with the - // magic number $50 $4b $05 $06. - if (eocd[0] != 0x50 || eocd[1] != 0x4b || - eocd[2] != 0x05 || eocd[3] != 0x06) { - LOG(ERROR) << "signature length doesn't match EOCD marker"; - return VERIFY_FAILURE; - } + // If this is really is the EOCD record, it will begin with the magic number $50 $4b $05 $06. + if (eocd[0] != 0x50 || eocd[1] != 0x4b || eocd[2] != 0x05 || eocd[3] != 0x06) { + LOG(ERROR) << "signature length doesn't match EOCD marker"; + return VERIFY_FAILURE; + } - for (size_t i = 4; i < eocd_size-3; ++i) { - if (eocd[i ] == 0x50 && eocd[i+1] == 0x4b && - eocd[i+2] == 0x05 && eocd[i+3] == 0x06) { - // if the sequence $50 $4b $05 $06 appears anywhere after - // the real one, libziparchive will find the later (wrong) one, - // which could be exploitable. Fail verification if - // this sequence occurs anywhere after the real one. - LOG(ERROR) << "EOCD marker occurs after start of EOCD"; - return VERIFY_FAILURE; - } + for (size_t i = 4; i < eocd_size-3; ++i) { + if (eocd[i ] == 0x50 && eocd[i+1] == 0x4b && eocd[i+2] == 0x05 && eocd[i+3] == 0x06) { + // If the sequence $50 $4b $05 $06 appears anywhere after the real one, libziparchive will + // find the later (wrong) one, which could be exploitable. Fail the verification if this + // sequence occurs anywhere after the real one. + LOG(ERROR) << "EOCD marker occurs after start of EOCD"; + return VERIFY_FAILURE; } + } - bool need_sha1 = false; - bool need_sha256 = false; - for (const auto& key : keys) { - switch (key.hash_len) { - case SHA_DIGEST_LENGTH: need_sha1 = true; break; - case SHA256_DIGEST_LENGTH: need_sha256 = true; break; - } + bool need_sha1 = false; + bool need_sha256 = false; + for (const auto& key : keys) { + switch (key.hash_len) { + case SHA_DIGEST_LENGTH: need_sha1 = true; break; + case SHA256_DIGEST_LENGTH: need_sha256 = true; break; } + } - SHA_CTX sha1_ctx; - SHA256_CTX sha256_ctx; - SHA1_Init(&sha1_ctx); - SHA256_Init(&sha256_ctx); - - double frac = -1.0; - size_t so_far = 0; - while (so_far < signed_len) { - // On a Nexus 5X, experiment showed 16MiB beat 1MiB by 6% faster for a - // 1196MiB full OTA and 60% for an 89MiB incremental OTA. - // http://b/28135231. - size_t size = std::min(signed_len - so_far, 16 * MiB); - - if (need_sha1) SHA1_Update(&sha1_ctx, addr + so_far, size); - if (need_sha256) SHA256_Update(&sha256_ctx, addr + so_far, size); - so_far += size; - - double f = so_far / (double)signed_len; - if (f > frac + 0.02 || size == so_far) { - ui->SetProgress(f); - frac = f; - } + SHA_CTX sha1_ctx; + SHA256_CTX sha256_ctx; + SHA1_Init(&sha1_ctx); + SHA256_Init(&sha256_ctx); + + double frac = -1.0; + size_t so_far = 0; + while (so_far < signed_len) { + // On a Nexus 5X, experiment showed 16MiB beat 1MiB by 6% faster for a + // 1196MiB full OTA and 60% for an 89MiB incremental OTA. + // http://b/28135231. + size_t size = std::min(signed_len - so_far, 16 * MiB); + + if (need_sha1) SHA1_Update(&sha1_ctx, addr + so_far, size); + if (need_sha256) SHA256_Update(&sha256_ctx, addr + so_far, size); + so_far += size; + + if (set_progress) { + double f = so_far / (double)signed_len; + if (f > frac + 0.02 || size == so_far) { + set_progress(f); + frac = f; + } } + } - uint8_t sha1[SHA_DIGEST_LENGTH]; - SHA1_Final(sha1, &sha1_ctx); - uint8_t sha256[SHA256_DIGEST_LENGTH]; - SHA256_Final(sha256, &sha256_ctx); - - uint8_t* sig_der = nullptr; - size_t sig_der_length = 0; - - uint8_t* signature = eocd + eocd_size - signature_start; - size_t signature_size = signature_start - FOOTER_SIZE; - - LOG(INFO) << "signature (offset: " << std::hex << (length - signature_start) << ", length: " - << signature_size << "): " << print_hex(signature, signature_size); + uint8_t sha1[SHA_DIGEST_LENGTH]; + SHA1_Final(sha1, &sha1_ctx); + uint8_t sha256[SHA256_DIGEST_LENGTH]; + SHA256_Final(sha256, &sha256_ctx); - if (!read_pkcs7(signature, signature_size, &sig_der, &sig_der_length)) { - LOG(ERROR) << "Could not find signature DER block"; - return VERIFY_FAILURE; - } + uint8_t* sig_der = nullptr; + size_t sig_der_length = 0; - /* - * Check to make sure at least one of the keys matches the signature. Since - * any key can match, we need to try each before determining a verification - * failure has happened. - */ - size_t i = 0; - for (const auto& key : keys) { - const uint8_t* hash; - int hash_nid; - switch (key.hash_len) { - case SHA_DIGEST_LENGTH: - hash = sha1; - hash_nid = NID_sha1; - break; - case SHA256_DIGEST_LENGTH: - hash = sha256; - hash_nid = NID_sha256; - break; - default: - continue; - } + uint8_t* signature = eocd + eocd_size - signature_start; + size_t signature_size = signature_start - FOOTER_SIZE; - // The 6 bytes is the "(signature_start) $ff $ff (comment_size)" that - // the signing tool appends after the signature itself. - if (key.key_type == Certificate::KEY_TYPE_RSA) { - if (!RSA_verify(hash_nid, hash, key.hash_len, sig_der, - sig_der_length, key.rsa.get())) { - LOG(INFO) << "failed to verify against RSA key " << i; - continue; - } + LOG(INFO) << "signature (offset: " << std::hex << (length - signature_start) << ", length: " + << signature_size << "): " << print_hex(signature, signature_size); - LOG(INFO) << "whole-file signature verified against RSA key " << i; - free(sig_der); - return VERIFY_SUCCESS; - } else if (key.key_type == Certificate::KEY_TYPE_EC - && key.hash_len == SHA256_DIGEST_LENGTH) { - if (!ECDSA_verify(0, hash, key.hash_len, sig_der, - sig_der_length, key.ec.get())) { - LOG(INFO) << "failed to verify against EC key " << i; - continue; - } + if (!read_pkcs7(signature, signature_size, &sig_der, &sig_der_length)) { + LOG(ERROR) << "Could not find signature DER block"; + return VERIFY_FAILURE; + } - LOG(INFO) << "whole-file signature verified against EC key " << i; - free(sig_der); - return VERIFY_SUCCESS; - } else { - LOG(INFO) << "Unknown key type " << key.key_type; - } - i++; - } + // Check to make sure at least one of the keys matches the signature. Since any key can match, + // we need to try each before determining a verification failure has happened. + size_t i = 0; + for (const auto& key : keys) { + const uint8_t* hash; + int hash_nid; + switch (key.hash_len) { + case SHA_DIGEST_LENGTH: + hash = sha1; + hash_nid = NID_sha1; + break; + case SHA256_DIGEST_LENGTH: + hash = sha256; + hash_nid = NID_sha256; + break; + default: + continue; + } + + // The 6 bytes is the "(signature_start) $ff $ff (comment_size)" that the signing tool appends + // after the signature itself. + if (key.key_type == Certificate::KEY_TYPE_RSA) { + if (!RSA_verify(hash_nid, hash, key.hash_len, sig_der, sig_der_length, key.rsa.get())) { + LOG(INFO) << "failed to verify against RSA key " << i; + continue; + } + + LOG(INFO) << "whole-file signature verified against RSA key " << i; + free(sig_der); + return VERIFY_SUCCESS; + } else if (key.key_type == Certificate::KEY_TYPE_EC && key.hash_len == SHA256_DIGEST_LENGTH) { + if (!ECDSA_verify(0, hash, key.hash_len, sig_der, sig_der_length, key.ec.get())) { + LOG(INFO) << "failed to verify against EC key " << i; + continue; + } + + LOG(INFO) << "whole-file signature verified against EC key " << i; + free(sig_der); + return VERIFY_SUCCESS; + } else { + LOG(INFO) << "Unknown key type " << key.key_type; + } + i++; + } - if (need_sha1) { - LOG(INFO) << "SHA-1 digest: " << print_hex(sha1, SHA_DIGEST_LENGTH); - } - if (need_sha256) { - LOG(INFO) << "SHA-256 digest: " << print_hex(sha256, SHA256_DIGEST_LENGTH); - } - free(sig_der); - LOG(ERROR) << "failed to verify whole-file signature"; - return VERIFY_FAILURE; + if (need_sha1) { + LOG(INFO) << "SHA-1 digest: " << print_hex(sha1, SHA_DIGEST_LENGTH); + } + if (need_sha256) { + LOG(INFO) << "SHA-256 digest: " << print_hex(sha256, SHA256_DIGEST_LENGTH); + } + free(sig_der); + LOG(ERROR) << "failed to verify whole-file signature"; + return VERIFY_FAILURE; } std::unique_ptr parse_rsa_key(FILE* file, uint32_t exponent) { diff --git a/verifier.h b/verifier.h index 58083fe14..067dab554 100644 --- a/verifier.h +++ b/verifier.h @@ -17,6 +17,7 @@ #ifndef _RECOVERY_VERIFIER_H #define _RECOVERY_VERIFIER_H +#include #include #include @@ -58,13 +59,14 @@ struct Certificate { std::unique_ptr ec; }; -/* addr and length define a an update package file that has been - * loaded (or mmap'ed, or whatever) into memory. Verify that the file - * is signed and the signature matches one of the given keys. Return - * one of the constants below. +/* + * 'addr' and 'length' define an update package file that has been loaded (or mmap'ed, or + * whatever) into memory. Verifies that the file is signed and the signature matches one of the + * given keys. It optionally accepts a callback function for posting the progress to. Returns one + * of the constants of VERIFY_SUCCESS and VERIFY_FAILURE. */ -int verify_file(unsigned char* addr, size_t length, - const std::vector& keys); +int verify_file(unsigned char* addr, size_t length, const std::vector& keys, + const std::function& set_progress = nullptr); bool load_keys(const char* filename, std::vector& certs); -- cgit v1.2.3 From 7b22c92ac1265f439f8e2623c4a1d6b45bbb05f2 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Thu, 16 Mar 2017 17:37:38 -0700 Subject: Drop the dependency on 'ui' in verify_file(). verify_file() has a dependency on the global variable of 'ui' for posting the verification progress, which requires the users of libverifier to provide a UI instance. This CL adds an optional argument to verify_file() so that it can post the progress through the provided callback function. As a result, we can drop the MockUI class in verifier_test.cpp. Test: recovery_component_test passes. Test: verify_file() posts progress update when installing an OTA. Change-Id: I8b87d0f0d99777ea755d33d6dbbe2b6d44243bf1 (cherry picked from commit 5e535014dd7961fbf812abeaa27f3339775031f1) --- install.cpp | 40 ++--- tests/component/verifier_test.cpp | 151 ++++++----------- verifier.cpp | 339 +++++++++++++++++++------------------- verifier.h | 14 +- 4 files changed, 241 insertions(+), 303 deletions(-) diff --git a/install.cpp b/install.cpp index f51bbf05c..68d3f54bc 100644 --- a/install.cpp +++ b/install.cpp @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -578,23 +579,24 @@ install_package(const char* path, bool* wipe_cache, const char* install_file, } bool verify_package(const unsigned char* package_data, size_t package_size) { - std::vector loadedKeys; - if (!load_keys(PUBLIC_KEYS_FILE, loadedKeys)) { - LOG(ERROR) << "Failed to load keys"; - return false; - } - LOG(INFO) << loadedKeys.size() << " key(s) loaded from " << PUBLIC_KEYS_FILE; - - // Verify package. - ui->Print("Verifying update package...\n"); - auto t0 = std::chrono::system_clock::now(); - int err = verify_file(const_cast(package_data), package_size, loadedKeys); - std::chrono::duration duration = std::chrono::system_clock::now() - t0; - ui->Print("Update package verification took %.1f s (result %d).\n", duration.count(), err); - if (err != VERIFY_SUCCESS) { - LOG(ERROR) << "Signature verification failed"; - LOG(ERROR) << "error: " << kZipVerificationFailure; - return false; - } - return true; + std::vector loadedKeys; + if (!load_keys(PUBLIC_KEYS_FILE, loadedKeys)) { + LOG(ERROR) << "Failed to load keys"; + return false; + } + LOG(INFO) << loadedKeys.size() << " key(s) loaded from " << PUBLIC_KEYS_FILE; + + // Verify package. + ui->Print("Verifying update package...\n"); + auto t0 = std::chrono::system_clock::now(); + int err = verify_file(const_cast(package_data), package_size, loadedKeys, + std::bind(&RecoveryUI::SetProgress, ui, std::placeholders::_1)); + std::chrono::duration duration = std::chrono::system_clock::now() - t0; + ui->Print("Update package verification took %.1f s (result %d).\n", duration.count(), err); + if (err != VERIFY_SUCCESS) { + LOG(ERROR) << "Signature verification failed"; + LOG(ERROR) << "error: " << kZipVerificationFailure; + return false; + } + return true; } diff --git a/tests/component/verifier_test.cpp b/tests/component/verifier_test.cpp index 4294d90eb..03829f393 100644 --- a/tests/component/verifier_test.cpp +++ b/tests/component/verifier_test.cpp @@ -22,93 +22,34 @@ #include #include -#include #include #include -#include - +#include #include -#include +#include -#include "common.h" #include "common/test_constants.h" #include "otautil/SysUtil.h" -#include "ui.h" #include "verifier.h" -RecoveryUI* ui = NULL; - -class MockUI : public RecoveryUI { - bool Init(const std::string&) override { - return true; - } - void SetStage(int, int) override {} - void SetBackground(Icon /*icon*/) override {} - void SetSystemUpdateText(bool /*security_update*/) override {} - - void SetProgressType(ProgressType /*determinate*/) override {} - void ShowProgress(float /*portion*/, float /*seconds*/) override {} - void SetProgress(float /*fraction*/) override {} - - void ShowText(bool /*visible*/) override {} - bool IsTextVisible() override { - return false; - } - bool WasTextEverVisible() override { - return false; - } - void Print(const char* fmt, ...) override { - va_list ap; - va_start(ap, fmt); - vfprintf(stderr, fmt, ap); - va_end(ap); - } - void PrintOnScreenOnly(const char* fmt, ...) override { - va_list ap; - va_start(ap, fmt); - vfprintf(stderr, fmt, ap); - va_end(ap); - } - void ShowFile(const char*) override {} - - void StartMenu(const char* const* /*headers*/, const char* const* /*items*/, - int /*initial_selection*/) override {} - int SelectMenu(int /*sel*/) override { - return 0; - } - void EndMenu() override {} -}; - -void -ui_print(const char* format, ...) { - va_list ap; - va_start(ap, format); - vfprintf(stdout, format, ap); - va_end(ap); -} - class VerifierTest : public testing::TestWithParam> { - public: - MemMapping memmap; - std::vector certs; - - virtual void SetUp() { - std::vector args = GetParam(); - std::string package = from_testdata_base(args[0]); - if (sysMapFile(package.c_str(), &memmap) != 0) { - FAIL() << "Failed to mmap " << package << ": " << strerror(errno) << "\n"; - } - - for (auto it = ++(args.cbegin()); it != args.cend(); ++it) { - std::string public_key_file = from_testdata_base("testkey_" + *it + ".txt"); - ASSERT_TRUE(load_keys(public_key_file.c_str(), certs)); - } + protected: + void SetUp() override { + std::vector args = GetParam(); + std::string package = from_testdata_base(args[0]); + if (sysMapFile(package.c_str(), &memmap) != 0) { + FAIL() << "Failed to mmap " << package << ": " << strerror(errno) << "\n"; } - static void SetUpTestCase() { - ui = new MockUI(); + for (auto it = ++args.cbegin(); it != args.cend(); ++it) { + std::string public_key_file = from_testdata_base("testkey_" + *it + ".txt"); + ASSERT_TRUE(load_keys(public_key_file.c_str(), certs)); } + } + + MemMapping memmap; + std::vector certs; }; class VerifierSuccessTest : public VerifierTest { @@ -118,48 +59,48 @@ class VerifierFailureTest : public VerifierTest { }; TEST_P(VerifierSuccessTest, VerifySucceed) { - ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs), VERIFY_SUCCESS); + ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs, nullptr), VERIFY_SUCCESS); } TEST_P(VerifierFailureTest, VerifyFailure) { - ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs), VERIFY_FAILURE); + ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs, nullptr), VERIFY_FAILURE); } INSTANTIATE_TEST_CASE_P(SingleKeySuccess, VerifierSuccessTest, - ::testing::Values( - std::vector({"otasigned_v1.zip", "v1"}), - std::vector({"otasigned_v2.zip", "v2"}), - std::vector({"otasigned_v3.zip", "v3"}), - std::vector({"otasigned_v4.zip", "v4"}), - std::vector({"otasigned_v5.zip", "v5"}))); + ::testing::Values( + std::vector({"otasigned_v1.zip", "v1"}), + std::vector({"otasigned_v2.zip", "v2"}), + std::vector({"otasigned_v3.zip", "v3"}), + std::vector({"otasigned_v4.zip", "v4"}), + std::vector({"otasigned_v5.zip", "v5"}))); INSTANTIATE_TEST_CASE_P(MultiKeySuccess, VerifierSuccessTest, - ::testing::Values( - std::vector({"otasigned_v1.zip", "v1", "v2"}), - std::vector({"otasigned_v2.zip", "v5", "v2"}), - std::vector({"otasigned_v3.zip", "v5", "v1", "v3"}), - std::vector({"otasigned_v4.zip", "v5", "v1", "v4"}), - std::vector({"otasigned_v5.zip", "v4", "v1", "v5"}))); + ::testing::Values( + std::vector({"otasigned_v1.zip", "v1", "v2"}), + std::vector({"otasigned_v2.zip", "v5", "v2"}), + std::vector({"otasigned_v3.zip", "v5", "v1", "v3"}), + std::vector({"otasigned_v4.zip", "v5", "v1", "v4"}), + std::vector({"otasigned_v5.zip", "v4", "v1", "v5"}))); INSTANTIATE_TEST_CASE_P(WrongKey, VerifierFailureTest, - ::testing::Values( - std::vector({"otasigned_v1.zip", "v2"}), - std::vector({"otasigned_v2.zip", "v1"}), - std::vector({"otasigned_v3.zip", "v5"}), - std::vector({"otasigned_v4.zip", "v5"}), - std::vector({"otasigned_v5.zip", "v3"}))); + ::testing::Values( + std::vector({"otasigned_v1.zip", "v2"}), + std::vector({"otasigned_v2.zip", "v1"}), + std::vector({"otasigned_v3.zip", "v5"}), + std::vector({"otasigned_v4.zip", "v5"}), + std::vector({"otasigned_v5.zip", "v3"}))); INSTANTIATE_TEST_CASE_P(WrongHash, VerifierFailureTest, - ::testing::Values( - std::vector({"otasigned_v1.zip", "v3"}), - std::vector({"otasigned_v2.zip", "v4"}), - std::vector({"otasigned_v3.zip", "v1"}), - std::vector({"otasigned_v4.zip", "v2"}))); + ::testing::Values( + std::vector({"otasigned_v1.zip", "v3"}), + std::vector({"otasigned_v2.zip", "v4"}), + std::vector({"otasigned_v3.zip", "v1"}), + std::vector({"otasigned_v4.zip", "v2"}))); INSTANTIATE_TEST_CASE_P(BadPackage, VerifierFailureTest, - ::testing::Values( - std::vector({"random.zip", "v1"}), - std::vector({"fake-eocd.zip", "v1"}), - std::vector({"alter-metadata.zip", "v1"}), - std::vector({"alter-footer.zip", "v1"}), - std::vector({"signature-boundary.zip", "v1"}))); + ::testing::Values( + std::vector({"random.zip", "v1"}), + std::vector({"fake-eocd.zip", "v1"}), + std::vector({"alter-metadata.zip", "v1"}), + std::vector({"alter-footer.zip", "v1"}), + std::vector({"signature-boundary.zip", "v1"}))); diff --git a/verifier.cpp b/verifier.cpp index 82454867b..582c498fb 100644 --- a/verifier.cpp +++ b/verifier.cpp @@ -21,6 +21,7 @@ #include #include +#include #include #include @@ -108,207 +109,199 @@ static bool read_pkcs7(uint8_t* pkcs7_der, size_t pkcs7_der_len, uint8_t** sig_d return *sig_der != NULL; } -// Look for an RSA signature embedded in the .ZIP file comment given -// the path to the zip. Verify it matches one of the given public -// keys. -// -// Return VERIFY_SUCCESS, VERIFY_FAILURE (if any error is encountered -// or no key matches the signature). - -int verify_file(unsigned char* addr, size_t length, - const std::vector& keys) { - ui->SetProgress(0.0); - - // An archive with a whole-file signature will end in six bytes: - // - // (2-byte signature start) $ff $ff (2-byte comment size) - // - // (As far as the ZIP format is concerned, these are part of the - // archive comment.) We start by reading this footer, this tells - // us how far back from the end we have to start reading to find - // the whole comment. +/* + * Looks for an RSA signature embedded in the .ZIP file comment given the path to the zip. Verifies + * that it matches one of the given public keys. A callback function can be optionally provided for + * posting the progress. + * + * Returns VERIFY_SUCCESS or VERIFY_FAILURE (if any error is encountered or no key matches the + * signature). + */ +int verify_file(unsigned char* addr, size_t length, const std::vector& keys, + const std::function& set_progress) { + if (set_progress) { + set_progress(0.0); + } + + // An archive with a whole-file signature will end in six bytes: + // + // (2-byte signature start) $ff $ff (2-byte comment size) + // + // (As far as the ZIP format is concerned, these are part of the archive comment.) We start by + // reading this footer, this tells us how far back from the end we have to start reading to find + // the whole comment. #define FOOTER_SIZE 6 - if (length < FOOTER_SIZE) { - LOG(ERROR) << "not big enough to contain footer"; - return VERIFY_FAILURE; - } + if (length < FOOTER_SIZE) { + LOG(ERROR) << "not big enough to contain footer"; + return VERIFY_FAILURE; + } - unsigned char* footer = addr + length - FOOTER_SIZE; + unsigned char* footer = addr + length - FOOTER_SIZE; - if (footer[2] != 0xff || footer[3] != 0xff) { - LOG(ERROR) << "footer is wrong"; - return VERIFY_FAILURE; - } + if (footer[2] != 0xff || footer[3] != 0xff) { + LOG(ERROR) << "footer is wrong"; + return VERIFY_FAILURE; + } - size_t comment_size = footer[4] + (footer[5] << 8); - size_t signature_start = footer[0] + (footer[1] << 8); - LOG(INFO) << "comment is " << comment_size << " bytes; signature is " << signature_start - << " bytes from end"; + size_t comment_size = footer[4] + (footer[5] << 8); + size_t signature_start = footer[0] + (footer[1] << 8); + LOG(INFO) << "comment is " << comment_size << " bytes; signature is " << signature_start + << " bytes from end"; - if (signature_start > comment_size) { - LOG(ERROR) << "signature start: " << signature_start << " is larger than comment size: " - << comment_size; - return VERIFY_FAILURE; - } + if (signature_start > comment_size) { + LOG(ERROR) << "signature start: " << signature_start << " is larger than comment size: " + << comment_size; + return VERIFY_FAILURE; + } - if (signature_start <= FOOTER_SIZE) { - LOG(ERROR) << "Signature start is in the footer"; - return VERIFY_FAILURE; - } + if (signature_start <= FOOTER_SIZE) { + LOG(ERROR) << "Signature start is in the footer"; + return VERIFY_FAILURE; + } #define EOCD_HEADER_SIZE 22 - // The end-of-central-directory record is 22 bytes plus any - // comment length. - size_t eocd_size = comment_size + EOCD_HEADER_SIZE; + // The end-of-central-directory record is 22 bytes plus any comment length. + size_t eocd_size = comment_size + EOCD_HEADER_SIZE; - if (length < eocd_size) { - LOG(ERROR) << "not big enough to contain EOCD"; - return VERIFY_FAILURE; - } + if (length < eocd_size) { + LOG(ERROR) << "not big enough to contain EOCD"; + return VERIFY_FAILURE; + } - // Determine how much of the file is covered by the signature. - // This is everything except the signature data and length, which - // includes all of the EOCD except for the comment length field (2 - // bytes) and the comment data. - size_t signed_len = length - eocd_size + EOCD_HEADER_SIZE - 2; + // Determine how much of the file is covered by the signature. This is everything except the + // signature data and length, which includes all of the EOCD except for the comment length field + // (2 bytes) and the comment data. + size_t signed_len = length - eocd_size + EOCD_HEADER_SIZE - 2; - unsigned char* eocd = addr + length - eocd_size; + unsigned char* eocd = addr + length - eocd_size; - // If this is really is the EOCD record, it will begin with the - // magic number $50 $4b $05 $06. - if (eocd[0] != 0x50 || eocd[1] != 0x4b || - eocd[2] != 0x05 || eocd[3] != 0x06) { - LOG(ERROR) << "signature length doesn't match EOCD marker"; - return VERIFY_FAILURE; - } + // If this is really is the EOCD record, it will begin with the magic number $50 $4b $05 $06. + if (eocd[0] != 0x50 || eocd[1] != 0x4b || eocd[2] != 0x05 || eocd[3] != 0x06) { + LOG(ERROR) << "signature length doesn't match EOCD marker"; + return VERIFY_FAILURE; + } - for (size_t i = 4; i < eocd_size-3; ++i) { - if (eocd[i ] == 0x50 && eocd[i+1] == 0x4b && - eocd[i+2] == 0x05 && eocd[i+3] == 0x06) { - // if the sequence $50 $4b $05 $06 appears anywhere after - // the real one, libziparchive will find the later (wrong) one, - // which could be exploitable. Fail verification if - // this sequence occurs anywhere after the real one. - LOG(ERROR) << "EOCD marker occurs after start of EOCD"; - return VERIFY_FAILURE; - } + for (size_t i = 4; i < eocd_size-3; ++i) { + if (eocd[i ] == 0x50 && eocd[i+1] == 0x4b && eocd[i+2] == 0x05 && eocd[i+3] == 0x06) { + // If the sequence $50 $4b $05 $06 appears anywhere after the real one, libziparchive will + // find the later (wrong) one, which could be exploitable. Fail the verification if this + // sequence occurs anywhere after the real one. + LOG(ERROR) << "EOCD marker occurs after start of EOCD"; + return VERIFY_FAILURE; } + } - bool need_sha1 = false; - bool need_sha256 = false; - for (const auto& key : keys) { - switch (key.hash_len) { - case SHA_DIGEST_LENGTH: need_sha1 = true; break; - case SHA256_DIGEST_LENGTH: need_sha256 = true; break; - } + bool need_sha1 = false; + bool need_sha256 = false; + for (const auto& key : keys) { + switch (key.hash_len) { + case SHA_DIGEST_LENGTH: need_sha1 = true; break; + case SHA256_DIGEST_LENGTH: need_sha256 = true; break; } + } - SHA_CTX sha1_ctx; - SHA256_CTX sha256_ctx; - SHA1_Init(&sha1_ctx); - SHA256_Init(&sha256_ctx); - - double frac = -1.0; - size_t so_far = 0; - while (so_far < signed_len) { - // On a Nexus 5X, experiment showed 16MiB beat 1MiB by 6% faster for a - // 1196MiB full OTA and 60% for an 89MiB incremental OTA. - // http://b/28135231. - size_t size = std::min(signed_len - so_far, 16 * MiB); - - if (need_sha1) SHA1_Update(&sha1_ctx, addr + so_far, size); - if (need_sha256) SHA256_Update(&sha256_ctx, addr + so_far, size); - so_far += size; - - double f = so_far / (double)signed_len; - if (f > frac + 0.02 || size == so_far) { - ui->SetProgress(f); - frac = f; - } + SHA_CTX sha1_ctx; + SHA256_CTX sha256_ctx; + SHA1_Init(&sha1_ctx); + SHA256_Init(&sha256_ctx); + + double frac = -1.0; + size_t so_far = 0; + while (so_far < signed_len) { + // On a Nexus 5X, experiment showed 16MiB beat 1MiB by 6% faster for a + // 1196MiB full OTA and 60% for an 89MiB incremental OTA. + // http://b/28135231. + size_t size = std::min(signed_len - so_far, 16 * MiB); + + if (need_sha1) SHA1_Update(&sha1_ctx, addr + so_far, size); + if (need_sha256) SHA256_Update(&sha256_ctx, addr + so_far, size); + so_far += size; + + if (set_progress) { + double f = so_far / (double)signed_len; + if (f > frac + 0.02 || size == so_far) { + set_progress(f); + frac = f; + } } + } - uint8_t sha1[SHA_DIGEST_LENGTH]; - SHA1_Final(sha1, &sha1_ctx); - uint8_t sha256[SHA256_DIGEST_LENGTH]; - SHA256_Final(sha256, &sha256_ctx); - - uint8_t* sig_der = nullptr; - size_t sig_der_length = 0; - - uint8_t* signature = eocd + eocd_size - signature_start; - size_t signature_size = signature_start - FOOTER_SIZE; - - LOG(INFO) << "signature (offset: " << std::hex << (length - signature_start) << ", length: " - << signature_size << "): " << print_hex(signature, signature_size); + uint8_t sha1[SHA_DIGEST_LENGTH]; + SHA1_Final(sha1, &sha1_ctx); + uint8_t sha256[SHA256_DIGEST_LENGTH]; + SHA256_Final(sha256, &sha256_ctx); - if (!read_pkcs7(signature, signature_size, &sig_der, &sig_der_length)) { - LOG(ERROR) << "Could not find signature DER block"; - return VERIFY_FAILURE; - } + uint8_t* sig_der = nullptr; + size_t sig_der_length = 0; - /* - * Check to make sure at least one of the keys matches the signature. Since - * any key can match, we need to try each before determining a verification - * failure has happened. - */ - size_t i = 0; - for (const auto& key : keys) { - const uint8_t* hash; - int hash_nid; - switch (key.hash_len) { - case SHA_DIGEST_LENGTH: - hash = sha1; - hash_nid = NID_sha1; - break; - case SHA256_DIGEST_LENGTH: - hash = sha256; - hash_nid = NID_sha256; - break; - default: - continue; - } + uint8_t* signature = eocd + eocd_size - signature_start; + size_t signature_size = signature_start - FOOTER_SIZE; - // The 6 bytes is the "(signature_start) $ff $ff (comment_size)" that - // the signing tool appends after the signature itself. - if (key.key_type == Certificate::KEY_TYPE_RSA) { - if (!RSA_verify(hash_nid, hash, key.hash_len, sig_der, - sig_der_length, key.rsa.get())) { - LOG(INFO) << "failed to verify against RSA key " << i; - continue; - } + LOG(INFO) << "signature (offset: " << std::hex << (length - signature_start) << ", length: " + << signature_size << "): " << print_hex(signature, signature_size); - LOG(INFO) << "whole-file signature verified against RSA key " << i; - free(sig_der); - return VERIFY_SUCCESS; - } else if (key.key_type == Certificate::KEY_TYPE_EC - && key.hash_len == SHA256_DIGEST_LENGTH) { - if (!ECDSA_verify(0, hash, key.hash_len, sig_der, - sig_der_length, key.ec.get())) { - LOG(INFO) << "failed to verify against EC key " << i; - continue; - } + if (!read_pkcs7(signature, signature_size, &sig_der, &sig_der_length)) { + LOG(ERROR) << "Could not find signature DER block"; + return VERIFY_FAILURE; + } - LOG(INFO) << "whole-file signature verified against EC key " << i; - free(sig_der); - return VERIFY_SUCCESS; - } else { - LOG(INFO) << "Unknown key type " << key.key_type; - } - i++; - } + // Check to make sure at least one of the keys matches the signature. Since any key can match, + // we need to try each before determining a verification failure has happened. + size_t i = 0; + for (const auto& key : keys) { + const uint8_t* hash; + int hash_nid; + switch (key.hash_len) { + case SHA_DIGEST_LENGTH: + hash = sha1; + hash_nid = NID_sha1; + break; + case SHA256_DIGEST_LENGTH: + hash = sha256; + hash_nid = NID_sha256; + break; + default: + continue; + } + + // The 6 bytes is the "(signature_start) $ff $ff (comment_size)" that the signing tool appends + // after the signature itself. + if (key.key_type == Certificate::KEY_TYPE_RSA) { + if (!RSA_verify(hash_nid, hash, key.hash_len, sig_der, sig_der_length, key.rsa.get())) { + LOG(INFO) << "failed to verify against RSA key " << i; + continue; + } + + LOG(INFO) << "whole-file signature verified against RSA key " << i; + free(sig_der); + return VERIFY_SUCCESS; + } else if (key.key_type == Certificate::KEY_TYPE_EC && key.hash_len == SHA256_DIGEST_LENGTH) { + if (!ECDSA_verify(0, hash, key.hash_len, sig_der, sig_der_length, key.ec.get())) { + LOG(INFO) << "failed to verify against EC key " << i; + continue; + } + + LOG(INFO) << "whole-file signature verified against EC key " << i; + free(sig_der); + return VERIFY_SUCCESS; + } else { + LOG(INFO) << "Unknown key type " << key.key_type; + } + i++; + } - if (need_sha1) { - LOG(INFO) << "SHA-1 digest: " << print_hex(sha1, SHA_DIGEST_LENGTH); - } - if (need_sha256) { - LOG(INFO) << "SHA-256 digest: " << print_hex(sha256, SHA256_DIGEST_LENGTH); - } - free(sig_der); - LOG(ERROR) << "failed to verify whole-file signature"; - return VERIFY_FAILURE; + if (need_sha1) { + LOG(INFO) << "SHA-1 digest: " << print_hex(sha1, SHA_DIGEST_LENGTH); + } + if (need_sha256) { + LOG(INFO) << "SHA-256 digest: " << print_hex(sha256, SHA256_DIGEST_LENGTH); + } + free(sig_der); + LOG(ERROR) << "failed to verify whole-file signature"; + return VERIFY_FAILURE; } std::unique_ptr parse_rsa_key(FILE* file, uint32_t exponent) { diff --git a/verifier.h b/verifier.h index 58083fe14..067dab554 100644 --- a/verifier.h +++ b/verifier.h @@ -17,6 +17,7 @@ #ifndef _RECOVERY_VERIFIER_H #define _RECOVERY_VERIFIER_H +#include #include #include @@ -58,13 +59,14 @@ struct Certificate { std::unique_ptr ec; }; -/* addr and length define a an update package file that has been - * loaded (or mmap'ed, or whatever) into memory. Verify that the file - * is signed and the signature matches one of the given keys. Return - * one of the constants below. +/* + * 'addr' and 'length' define an update package file that has been loaded (or mmap'ed, or + * whatever) into memory. Verifies that the file is signed and the signature matches one of the + * given keys. It optionally accepts a callback function for posting the progress to. Returns one + * of the constants of VERIFY_SUCCESS and VERIFY_FAILURE. */ -int verify_file(unsigned char* addr, size_t length, - const std::vector& keys); +int verify_file(unsigned char* addr, size_t length, const std::vector& keys, + const std::function& set_progress = nullptr); bool load_keys(const char* filename, std::vector& certs); -- cgit v1.2.3 From d7bf82eb5358df367daaac12562fe5aa4e87ba63 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Sat, 18 Mar 2017 09:24:11 -0700 Subject: Remove the dead #include's in verifier.cpp. A follow-up to commit 5e535014dd7961fbf812abeaa27f3339775031f1. Also clean up Android.mk, since libverifier no longer needs anything from libminui. Test: mmma bootable/recovery Test: recovery_component_test passes. Change-Id: I1c11e4bbeef67ca34a2054debf1f5b280d509217 --- Android.mk | 5 +---- tests/Android.mk | 1 - verifier.cpp | 4 +--- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/Android.mk b/Android.mk index f8e5ac24a..58b8a2240 100644 --- a/Android.mk +++ b/Android.mk @@ -144,15 +144,12 @@ include $(BUILD_EXECUTABLE) # libverifier (static library) # =============================== include $(CLEAR_VARS) -LOCAL_CLANG := true LOCAL_MODULE := libverifier LOCAL_MODULE_TAGS := tests LOCAL_SRC_FILES := \ asn1_decoder.cpp \ - verifier.cpp \ - ui.cpp + verifier.cpp LOCAL_STATIC_LIBRARIES := \ - libminui \ libcrypto_utils \ libcrypto \ libbase diff --git a/tests/Android.mk b/tests/Android.mk index 65f736d13..ff6e14c9b 100644 --- a/tests/Android.mk +++ b/tests/Android.mk @@ -120,7 +120,6 @@ LOCAL_STATIC_LIBRARIES := \ libupdater \ libbootloader_message \ libverifier \ - libminui \ libotautil \ libmounts \ libdivsufsort \ diff --git a/verifier.cpp b/verifier.cpp index 00e13aa7d..3beaa6e02 100644 --- a/verifier.cpp +++ b/verifier.cpp @@ -21,8 +21,8 @@ #include #include -#include #include +#include #include #include @@ -31,9 +31,7 @@ #include #include "asn1_decoder.h" -#include "common.h" #include "print_sha1.h" -#include "ui.h" static constexpr size_t MiB = 1024 * 1024; -- cgit v1.2.3 From 3116ce46510e9b209e3b40dfeb577e18f51e8f95 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Thu, 16 Mar 2017 17:37:38 -0700 Subject: Add testcases for load_keys(). Test: recovery_component_test passes. Change-Id: I6276b59981c87c50736d69d4af7647c8ed892965 --- tests/component/verifier_test.cpp | 57 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/tests/component/verifier_test.cpp b/tests/component/verifier_test.cpp index 460585d22..07a8c960f 100644 --- a/tests/component/verifier_test.cpp +++ b/tests/component/verifier_test.cpp @@ -58,6 +58,63 @@ class VerifierSuccessTest : public VerifierTest { class VerifierFailureTest : public VerifierTest { }; +TEST(VerifierTest, load_keys_multiple_keys) { + std::string testkey_v4; + ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v4.txt"), &testkey_v4)); + + std::string testkey_v3; + ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v3.txt"), &testkey_v3)); + + std::string keys = testkey_v4 + "," + testkey_v3 + "," + testkey_v4; + TemporaryFile key_file1; + ASSERT_TRUE(android::base::WriteStringToFile(keys, key_file1.path)); + std::vector certs; + ASSERT_TRUE(load_keys(key_file1.path, certs)); + ASSERT_EQ(3U, certs.size()); +} + +TEST(VerifierTest, load_keys_invalid_keys) { + std::vector certs; + ASSERT_FALSE(load_keys("/doesntexist", certs)); + + // Empty file. + TemporaryFile key_file1; + ASSERT_FALSE(load_keys(key_file1.path, certs)); + + // Invalid contents. + ASSERT_TRUE(android::base::WriteStringToFile("invalid", key_file1.path)); + ASSERT_FALSE(load_keys(key_file1.path, certs)); + + std::string testkey_v4; + ASSERT_TRUE(android::base::ReadFileToString(from_testdata_base("testkey_v4.txt"), &testkey_v4)); + + // Invalid key version: "v4 ..." => "v6 ...". + std::string invalid_key2(testkey_v4); + invalid_key2[1] = '6'; + TemporaryFile key_file2; + ASSERT_TRUE(android::base::WriteStringToFile(invalid_key2, key_file2.path)); + ASSERT_FALSE(load_keys(key_file2.path, certs)); + + // Invalid key content: inserted extra bytes ",2209831334". + std::string invalid_key3(testkey_v4); + invalid_key3.insert(invalid_key2.size() - 2, ",2209831334"); + TemporaryFile key_file3; + ASSERT_TRUE(android::base::WriteStringToFile(invalid_key3, key_file3.path)); + ASSERT_FALSE(load_keys(key_file3.path, certs)); + + // Invalid key: the last key must not end with an extra ','. + std::string invalid_key4 = testkey_v4 + ","; + TemporaryFile key_file4; + ASSERT_TRUE(android::base::WriteStringToFile(invalid_key4, key_file4.path)); + ASSERT_FALSE(load_keys(key_file4.path, certs)); + + // Invalid key separator. + std::string invalid_key5 = testkey_v4 + ";" + testkey_v4; + TemporaryFile key_file5; + ASSERT_TRUE(android::base::WriteStringToFile(invalid_key5, key_file5.path)); + ASSERT_FALSE(load_keys(key_file5.path, certs)); +} + TEST_P(VerifierSuccessTest, VerifySucceed) { ASSERT_EQ(verify_file(memmap.addr, memmap.length, certs, nullptr), VERIFY_SUCCESS); } -- cgit v1.2.3 From 76fdb2419bfec0e747db2530379484a3dc571f34 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Mon, 20 Mar 2017 17:09:13 -0700 Subject: verify_file: Add constness to a few addresses. We should not touch any data while verifying packages (or parsing the in-memory ASN.1 structures). Test: mmma bootable/recovery Test: recovery_component_test passes. Test: recovery_unit_test passes. Change-Id: Ie990662c6451ec066a1807b3081c9296afbdb0bf --- asn1_decoder.cpp | 12 ++--- asn1_decoder.h | 6 +-- install.cpp | 2 +- tests/unit/asn1_decoder_test.cpp | 28 +++++----- verifier.cpp | 107 +++++++++++++++++++-------------------- verifier.h | 2 +- 6 files changed, 78 insertions(+), 79 deletions(-) diff --git a/asn1_decoder.cpp b/asn1_decoder.cpp index e7aef781c..ca4ee5267 100644 --- a/asn1_decoder.cpp +++ b/asn1_decoder.cpp @@ -22,9 +22,9 @@ typedef struct asn1_context { - size_t length; - uint8_t* p; - int app_type; + size_t length; + const uint8_t* p; + int app_type; } asn1_context_t; @@ -38,7 +38,7 @@ static const int kTagSequence = 0x30; static const int kTagSet = 0x31; static const int kTagConstructed = 0xA0; -asn1_context_t* asn1_context_new(uint8_t* buffer, size_t length) { +asn1_context_t* asn1_context_new(const uint8_t* buffer, size_t length) { asn1_context_t* ctx = (asn1_context_t*) calloc(1, sizeof(asn1_context_t)); if (ctx == NULL) { return NULL; @@ -168,7 +168,7 @@ bool asn1_sequence_next(asn1_context_t* ctx) { return true; } -bool asn1_oid_get(asn1_context_t* ctx, uint8_t** oid, size_t* length) { +bool asn1_oid_get(asn1_context_t* ctx, const uint8_t** oid, size_t* length) { if (get_byte(ctx) != kTagOid) { return false; } @@ -179,7 +179,7 @@ bool asn1_oid_get(asn1_context_t* ctx, uint8_t** oid, size_t* length) { return true; } -bool asn1_octet_string_get(asn1_context_t* ctx, uint8_t** octet_string, size_t* length) { +bool asn1_octet_string_get(asn1_context_t* ctx, const uint8_t** octet_string, size_t* length) { if (get_byte(ctx) != kTagOctetString) { return false; } diff --git a/asn1_decoder.h b/asn1_decoder.h index b17141c44..fbd118f90 100644 --- a/asn1_decoder.h +++ b/asn1_decoder.h @@ -22,7 +22,7 @@ typedef struct asn1_context asn1_context_t; -asn1_context_t* asn1_context_new(uint8_t* buffer, size_t length); +asn1_context_t* asn1_context_new(const uint8_t* buffer, size_t length); void asn1_context_free(asn1_context_t* ctx); asn1_context_t* asn1_constructed_get(asn1_context_t* ctx); bool asn1_constructed_skip_all(asn1_context_t* ctx); @@ -30,7 +30,7 @@ int asn1_constructed_type(asn1_context_t* ctx); asn1_context_t* asn1_sequence_get(asn1_context_t* ctx); asn1_context_t* asn1_set_get(asn1_context_t* ctx); bool asn1_sequence_next(asn1_context_t* seq); -bool asn1_oid_get(asn1_context_t* ctx, uint8_t** oid, size_t* length); -bool asn1_octet_string_get(asn1_context_t* ctx, uint8_t** octet_string, size_t* length); +bool asn1_oid_get(asn1_context_t* ctx, const uint8_t** oid, size_t* length); +bool asn1_octet_string_get(asn1_context_t* ctx, const uint8_t** octet_string, size_t* length); #endif /* ASN1_DECODER_H_ */ diff --git a/install.cpp b/install.cpp index 9123fc276..db8fb97db 100644 --- a/install.cpp +++ b/install.cpp @@ -589,7 +589,7 @@ bool verify_package(const unsigned char* package_data, size_t package_size) { // Verify package. ui->Print("Verifying update package...\n"); auto t0 = std::chrono::system_clock::now(); - int err = verify_file(const_cast(package_data), package_size, loadedKeys, + int err = verify_file(package_data, package_size, loadedKeys, std::bind(&RecoveryUI::SetProgress, ui, std::placeholders::_1)); std::chrono::duration duration = std::chrono::system_clock::now() - t0; ui->Print("Update package verification took %.1f s (result %d).\n", duration.count(), err); diff --git a/tests/unit/asn1_decoder_test.cpp b/tests/unit/asn1_decoder_test.cpp index af96d87d2..997639d8a 100644 --- a/tests/unit/asn1_decoder_test.cpp +++ b/tests/unit/asn1_decoder_test.cpp @@ -39,7 +39,7 @@ TEST_F(Asn1DecoderTest, Empty_Failure) { EXPECT_EQ(NULL, asn1_set_get(ctx)); EXPECT_FALSE(asn1_sequence_next(ctx)); - uint8_t* junk; + const uint8_t* junk; size_t length; EXPECT_FALSE(asn1_oid_get(ctx, &junk, &length)); EXPECT_FALSE(asn1_octet_string_get(ctx, &junk, &length)); @@ -68,7 +68,7 @@ TEST_F(Asn1DecoderTest, ConstructedGet_TooSmallForChild_Failure) { asn1_context_t* ptr = asn1_constructed_get(ctx); ASSERT_NE((asn1_context_t*)NULL, ptr); EXPECT_EQ(5, asn1_constructed_type(ptr)); - uint8_t* oid; + const uint8_t* oid; size_t length; EXPECT_FALSE(asn1_oid_get(ptr, &oid, &length)); asn1_context_free(ptr); @@ -81,7 +81,7 @@ TEST_F(Asn1DecoderTest, ConstructedGet_Success) { asn1_context_t* ptr = asn1_constructed_get(ctx); ASSERT_NE((asn1_context_t*)NULL, ptr); EXPECT_EQ(5, asn1_constructed_type(ptr)); - uint8_t* oid; + const uint8_t* oid; size_t length; ASSERT_TRUE(asn1_oid_get(ptr, &oid, &length)); EXPECT_EQ(1U, length); @@ -103,7 +103,7 @@ TEST_F(Asn1DecoderTest, ConstructedSkipAll_Success) { 0x06, 0x01, 0xA5, }; asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); ASSERT_TRUE(asn1_constructed_skip_all(ctx)); - uint8_t* oid; + const uint8_t* oid; size_t length; ASSERT_TRUE(asn1_oid_get(ctx, &oid, &length)); EXPECT_EQ(1U, length); @@ -123,7 +123,7 @@ TEST_F(Asn1DecoderTest, SequenceGet_TooSmallForChild_Failure) { asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); asn1_context_t* ptr = asn1_sequence_get(ctx); ASSERT_NE((asn1_context_t*)NULL, ptr); - uint8_t* oid; + const uint8_t* oid; size_t length; EXPECT_FALSE(asn1_oid_get(ptr, &oid, &length)); asn1_context_free(ptr); @@ -135,7 +135,7 @@ TEST_F(Asn1DecoderTest, SequenceGet_Success) { asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); asn1_context_t* ptr = asn1_sequence_get(ctx); ASSERT_NE((asn1_context_t*)NULL, ptr); - uint8_t* oid; + const uint8_t* oid; size_t length; ASSERT_TRUE(asn1_oid_get(ptr, &oid, &length)); EXPECT_EQ(1U, length); @@ -156,7 +156,7 @@ TEST_F(Asn1DecoderTest, SetGet_TooSmallForChild_Failure) { asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); asn1_context_t* ptr = asn1_set_get(ctx); ASSERT_NE((asn1_context_t*)NULL, ptr); - uint8_t* oid; + const uint8_t* oid; size_t length; EXPECT_FALSE(asn1_oid_get(ptr, &oid, &length)); asn1_context_free(ptr); @@ -168,7 +168,7 @@ TEST_F(Asn1DecoderTest, SetGet_Success) { asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); asn1_context_t* ptr = asn1_set_get(ctx); ASSERT_NE((asn1_context_t*)NULL, ptr); - uint8_t* oid; + const uint8_t* oid; size_t length; ASSERT_TRUE(asn1_oid_get(ptr, &oid, &length)); EXPECT_EQ(1U, length); @@ -180,7 +180,7 @@ TEST_F(Asn1DecoderTest, SetGet_Success) { TEST_F(Asn1DecoderTest, OidGet_LengthZero_Failure) { uint8_t data[] = { 0x06, 0x00, 0x01, }; asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); - uint8_t* oid; + const uint8_t* oid; size_t length; EXPECT_FALSE(asn1_oid_get(ctx, &oid, &length)); asn1_context_free(ctx); @@ -189,7 +189,7 @@ TEST_F(Asn1DecoderTest, OidGet_LengthZero_Failure) { TEST_F(Asn1DecoderTest, OidGet_TooSmall_Failure) { uint8_t data[] = { 0x06, 0x01, }; asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); - uint8_t* oid; + const uint8_t* oid; size_t length; EXPECT_FALSE(asn1_oid_get(ctx, &oid, &length)); asn1_context_free(ctx); @@ -198,7 +198,7 @@ TEST_F(Asn1DecoderTest, OidGet_TooSmall_Failure) { TEST_F(Asn1DecoderTest, OidGet_Success) { uint8_t data[] = { 0x06, 0x01, 0x99, }; asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); - uint8_t* oid; + const uint8_t* oid; size_t length; ASSERT_TRUE(asn1_oid_get(ctx, &oid, &length)); EXPECT_EQ(1U, length); @@ -209,7 +209,7 @@ TEST_F(Asn1DecoderTest, OidGet_Success) { TEST_F(Asn1DecoderTest, OctetStringGet_LengthZero_Failure) { uint8_t data[] = { 0x04, 0x00, 0x55, }; asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); - uint8_t* string; + const uint8_t* string; size_t length; ASSERT_FALSE(asn1_octet_string_get(ctx, &string, &length)); asn1_context_free(ctx); @@ -218,7 +218,7 @@ TEST_F(Asn1DecoderTest, OctetStringGet_LengthZero_Failure) { TEST_F(Asn1DecoderTest, OctetStringGet_TooSmall_Failure) { uint8_t data[] = { 0x04, 0x01, }; asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); - uint8_t* string; + const uint8_t* string; size_t length; ASSERT_FALSE(asn1_octet_string_get(ctx, &string, &length)); asn1_context_free(ctx); @@ -227,7 +227,7 @@ TEST_F(Asn1DecoderTest, OctetStringGet_TooSmall_Failure) { TEST_F(Asn1DecoderTest, OctetStringGet_Success) { uint8_t data[] = { 0x04, 0x01, 0xAA, }; asn1_context_t* ctx = asn1_context_new(data, sizeof(data)); - uint8_t* string; + const uint8_t* string; size_t length; ASSERT_TRUE(asn1_octet_string_get(ctx, &string, &length)); EXPECT_EQ(1U, length); diff --git a/verifier.cpp b/verifier.cpp index 3beaa6e02..fa344d746 100644 --- a/verifier.cpp +++ b/verifier.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -60,51 +61,53 @@ static constexpr size_t MiB = 1024 * 1024; * SEQUENCE (SignatureAlgorithmIdentifier) * OCTET STRING (SignatureValue) */ -static bool read_pkcs7(uint8_t* pkcs7_der, size_t pkcs7_der_len, uint8_t** sig_der, - size_t* sig_der_length) { - asn1_context_t* ctx = asn1_context_new(pkcs7_der, pkcs7_der_len); - if (ctx == NULL) { - return false; - } +static bool read_pkcs7(const uint8_t* pkcs7_der, size_t pkcs7_der_len, + std::vector* sig_der) { + CHECK(sig_der != nullptr); + sig_der->clear(); + + asn1_context_t* ctx = asn1_context_new(pkcs7_der, pkcs7_der_len); + if (ctx == NULL) { + return false; + } - asn1_context_t* pkcs7_seq = asn1_sequence_get(ctx); - if (pkcs7_seq != NULL && asn1_sequence_next(pkcs7_seq)) { - asn1_context_t *signed_data_app = asn1_constructed_get(pkcs7_seq); - if (signed_data_app != NULL) { - asn1_context_t* signed_data_seq = asn1_sequence_get(signed_data_app); - if (signed_data_seq != NULL - && asn1_sequence_next(signed_data_seq) - && asn1_sequence_next(signed_data_seq) - && asn1_sequence_next(signed_data_seq) - && asn1_constructed_skip_all(signed_data_seq)) { - asn1_context_t *sig_set = asn1_set_get(signed_data_seq); - if (sig_set != NULL) { - asn1_context_t* sig_seq = asn1_sequence_get(sig_set); - if (sig_seq != NULL - && asn1_sequence_next(sig_seq) - && asn1_sequence_next(sig_seq) - && asn1_sequence_next(sig_seq) - && asn1_sequence_next(sig_seq)) { - uint8_t* sig_der_ptr; - if (asn1_octet_string_get(sig_seq, &sig_der_ptr, sig_der_length)) { - *sig_der = (uint8_t*) malloc(*sig_der_length); - if (*sig_der != NULL) { - memcpy(*sig_der, sig_der_ptr, *sig_der_length); - } - } - asn1_context_free(sig_seq); - } - asn1_context_free(sig_set); - } - asn1_context_free(signed_data_seq); + asn1_context_t* pkcs7_seq = asn1_sequence_get(ctx); + if (pkcs7_seq != NULL && asn1_sequence_next(pkcs7_seq)) { + asn1_context_t *signed_data_app = asn1_constructed_get(pkcs7_seq); + if (signed_data_app != NULL) { + asn1_context_t* signed_data_seq = asn1_sequence_get(signed_data_app); + if (signed_data_seq != NULL + && asn1_sequence_next(signed_data_seq) + && asn1_sequence_next(signed_data_seq) + && asn1_sequence_next(signed_data_seq) + && asn1_constructed_skip_all(signed_data_seq)) { + asn1_context_t *sig_set = asn1_set_get(signed_data_seq); + if (sig_set != NULL) { + asn1_context_t* sig_seq = asn1_sequence_get(sig_set); + if (sig_seq != NULL + && asn1_sequence_next(sig_seq) + && asn1_sequence_next(sig_seq) + && asn1_sequence_next(sig_seq) + && asn1_sequence_next(sig_seq)) { + const uint8_t* sig_der_ptr; + size_t sig_der_length; + if (asn1_octet_string_get(sig_seq, &sig_der_ptr, &sig_der_length)) { + sig_der->resize(sig_der_length); + std::copy(sig_der_ptr, sig_der_ptr + sig_der_length, sig_der->begin()); } - asn1_context_free(signed_data_app); + asn1_context_free(sig_seq); + } + asn1_context_free(sig_set); } - asn1_context_free(pkcs7_seq); + asn1_context_free(signed_data_seq); + } + asn1_context_free(signed_data_app); } - asn1_context_free(ctx); + asn1_context_free(pkcs7_seq); + } + asn1_context_free(ctx); - return *sig_der != NULL; + return !sig_der->empty(); } /* @@ -115,7 +118,7 @@ static bool read_pkcs7(uint8_t* pkcs7_der, size_t pkcs7_der_len, uint8_t** sig_d * Returns VERIFY_SUCCESS or VERIFY_FAILURE (if any error is encountered or no key matches the * signature). */ -int verify_file(unsigned char* addr, size_t length, const std::vector& keys, +int verify_file(const unsigned char* addr, size_t length, const std::vector& keys, const std::function& set_progress) { if (set_progress) { set_progress(0.0); @@ -136,7 +139,7 @@ int verify_file(unsigned char* addr, size_t length, const std::vector sig_der; + if (!read_pkcs7(signature, signature_size, &sig_der)) { LOG(ERROR) << "Could not find signature DER block"; return VERIFY_FAILURE; } @@ -262,22 +263,21 @@ int verify_file(unsigned char* addr, size_t length, const std::vector& keys, +int verify_file(const unsigned char* addr, size_t length, const std::vector& keys, const std::function& set_progress = nullptr); bool load_keys(const char* filename, std::vector& certs); -- cgit v1.2.3 From edec27a5bd08ff2779d980dc5fa7b8b7dca34fa0 Mon Sep 17 00:00:00 2001 From: Dan Willemsen Date: Tue, 21 Mar 2017 16:41:14 -0700 Subject: Fix updater include generation w/installclean Since this was putting the intermediate file in obj/PACKAGING, every installclean was removing it and triggering updater to rebuild. Instead, use the standard generated-sources-dir. The dep file can also be removed now that ninja will re-run the generator if the command line changes. Test: m -j updater; m installclean; m -j updater Test: Only change to aosp_fugu updater before/after is the debug info Change-Id: I20928bd2049d4a3d4e21f83fd64d16cfdc541958 --- updater/Android.mk | 20 ++++---------------- 1 file changed, 4 insertions(+), 16 deletions(-) diff --git a/updater/Android.mk b/updater/Android.mk index 3a47dacd5..a113fe86c 100644 --- a/updater/Android.mk +++ b/updater/Android.mk @@ -110,21 +110,11 @@ LOCAL_STATIC_LIBRARIES := \ # any subsidiary static libraries required for your registered # extension libs. -inc := $(call intermediates-dir-for,PACKAGING,updater_extensions)/register.inc - -# Encode the value of TARGET_RECOVERY_UPDATER_LIBS into the filename of the dependency. -# So if TARGET_RECOVERY_UPDATER_LIBS is changed, a new dependency file will be generated. -# Note that we have to remove any existing depency files before creating new one, -# so no obsolete dependecy file gets used if you switch back to an old value. -inc_dep_file := $(inc).dep.$(subst $(space),-,$(sort $(TARGET_RECOVERY_UPDATER_LIBS))) -$(inc_dep_file): stem := $(inc).dep -$(inc_dep_file) : - $(hide) mkdir -p $(dir $@) - $(hide) rm -f $(stem).* - $(hide) touch $@ +LOCAL_MODULE_CLASS := EXECUTABLES +inc := $(call local-generated-sources-dir)/register.inc $(inc) : libs := $(TARGET_RECOVERY_UPDATER_LIBS) -$(inc) : $(inc_dep_file) +$(inc) : $(hide) mkdir -p $(dir $@) $(hide) echo "" > $@ $(hide) $(foreach lib,$(libs),echo "extern void Register_$(lib)(void);" >> $@;) @@ -132,11 +122,9 @@ $(inc) : $(inc_dep_file) $(hide) $(foreach lib,$(libs),echo " Register_$(lib)();" >> $@;) $(hide) echo "}" >> $@ -$(call intermediates-dir-for,EXECUTABLES,updater,,,$(TARGET_PREFER_32_BIT))/updater.o : $(inc) -LOCAL_C_INCLUDES += $(dir $(inc)) +LOCAL_GENERATED_SOURCES := $(inc) inc := -inc_dep_file := LOCAL_FORCE_STATIC_EXECUTABLE := true -- cgit v1.2.3