From 86d86768b3dc4ba093e63269b556543515f50d41 Mon Sep 17 00:00:00 2001 From: "trop[bot]" <37223003+trop[bot]@users.noreply.github.com> Date: Wed, 2 Oct 2024 19:13:34 -0500 Subject: [PATCH] fix: deprecated API and -Wunsafe-buffer-usage warnings in AsarFileValidator (#44105) * refactor: const correctness Co-authored-by: Charles Kerr * refactor: extract-method AsarFileValidator::EnsureHashExists() Co-authored-by: Charles Kerr * refactor: replace use of deprecated crypto API https://crbug.com/364687923 Co-authored-by: Charles Kerr * refactor: use span API in AsarFileValidator::OnRead() Co-authored-by: Charles Kerr * refactor: replace use of deprecated crypto API https://crbug.com/364687923 Co-authored-by: Charles Kerr * fixup! refactor: use span API in AsarFileValidator::OnRead() fix: electron-ia32-testing FTBFS Co-authored-by: Charles Kerr --------- Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com> Co-authored-by: Charles Kerr --- shell/browser/net/asar/asar_file_validator.cc | 92 +++++++++---------- shell/browser/net/asar/asar_file_validator.h | 4 +- 2 files changed, 48 insertions(+), 48 deletions(-) diff --git a/shell/browser/net/asar/asar_file_validator.cc b/shell/browser/net/asar/asar_file_validator.cc index 342635a5f54c..52aa6ca82c29 100644 --- a/shell/browser/net/asar/asar_file_validator.cc +++ b/shell/browser/net/asar/asar_file_validator.cc @@ -5,10 +5,12 @@ #include "shell/browser/net/asar/asar_file_validator.h" #include +#include #include #include #include +#include "base/containers/span.h" #include "base/logging.h" #include "base/notreached.h" #include "base/strings/string_number_conversions.h" @@ -26,50 +28,51 @@ AsarFileValidator::AsarFileValidator(IntegrityPayload integrity, AsarFileValidator::~AsarFileValidator() = default; +void AsarFileValidator::EnsureBlockHashExists() { + if (current_hash_) + return; + + current_hash_byte_count_ = 0U; + switch (integrity_.algorithm) { + case HashAlgorithm::kSHA256: + current_hash_ = crypto::SecureHash::Create(crypto::SecureHash::SHA256); + break; + case HashAlgorithm::kNone: + CHECK(false); + break; + } +} + void AsarFileValidator::OnRead(base::span buffer, mojo::FileDataSource::ReadResult* result) { DCHECK(!done_reading_); - uint64_t buffer_size = result->bytes_read; + const uint32_t block_size = integrity_.block_size; - // Compute how many bytes we should hash, and add them to the current hash. - uint32_t block_size = integrity_.block_size; - uint64_t bytes_added = 0; - while (bytes_added < buffer_size) { - if (current_block_ > max_block_) { - LOG(FATAL) - << "Unexpected number of blocks while validating ASAR file stream"; - } + // |buffer| contains the read buffer. |result->bytes_read| is the actual + // bytes number that |source| read that should be less than buffer.size(). + auto hashme = base::as_bytes(buffer.subspan(0U, result->bytes_read)); - // Create a hash if we don't have one yet - if (!current_hash_) { - current_hash_byte_count_ = 0; - switch (integrity_.algorithm) { - case HashAlgorithm::kSHA256: - current_hash_ = - crypto::SecureHash::Create(crypto::SecureHash::SHA256); - break; - case HashAlgorithm::kNone: - CHECK(false); - break; - } - } + while (!std::empty(hashme)) { + if (current_block_ > max_block_) + LOG(FATAL) << "Unexpected block count while validating ASAR file stream"; - // Compute how many bytes we should hash, and add them to the current hash. - // We need to either add just enough bytes to fill up a block (block_size - - // current_bytes) or use every remaining byte (buffer_size - bytes_added) - int bytes_to_hash = std::min(block_size - current_hash_byte_count_, - buffer_size - bytes_added); - DCHECK_GT(bytes_to_hash, 0); - current_hash_->Update(buffer.data() + bytes_added, bytes_to_hash); - bytes_added += bytes_to_hash; - current_hash_byte_count_ += bytes_to_hash; - total_hash_byte_count_ += bytes_to_hash; + EnsureBlockHashExists(); - if (current_hash_byte_count_ == block_size && !FinishBlock()) { - LOG(FATAL) << "Failed to validate block while streaming ASAR file: " - << current_block_; - } + // hash as many bytes as will fit in the current block. + const auto n_left_in_block = block_size - current_hash_byte_count_; + const auto n_now = std::min(n_left_in_block, uint64_t{std::size(hashme)}); + DCHECK_GT(n_now, 0U); + const auto [hashme_now, hashme_next] = hashme.split_at(n_now); + + current_hash_->Update(hashme_now); + current_hash_byte_count_ += n_now; + total_hash_byte_count_ += n_now; + + if (current_hash_byte_count_ == block_size && !FinishBlock()) + LOG(FATAL) << "Streaming ASAR file block hash failed: " << current_block_; + + hashme = hashme_next; } } @@ -86,8 +89,6 @@ bool AsarFileValidator::FinishBlock() { current_hash_ = crypto::SecureHash::Create(crypto::SecureHash::SHA256); } - uint8_t actual[crypto::kSHA256Length]; - // If the file reader is done we need to make sure we've either read up to the // end of the file (the check below) or up to the end of a block_size byte // boundary. If the below check fails we compute the next block boundary, how @@ -104,21 +105,18 @@ bool AsarFileValidator::FinishBlock() { if (!file_.ReadAndCheck(offset, abandoned_buffer)) { LOG(FATAL) << "Failed to read required portion of streamed ASAR archive"; } - - current_hash_->Update(&abandoned_buffer.front(), bytes_needed); + current_hash_->Update(abandoned_buffer); } - current_hash_->Finish(actual, sizeof(actual)); + auto actual = std::array{}; + current_hash_->Finish(actual); current_hash_.reset(); current_hash_byte_count_ = 0; - const std::string expected_hash = integrity_.blocks[current_block_]; - const std::string actual_hex_hash = - base::ToLowerASCII(base::HexEncode(actual, sizeof(actual))); - - if (expected_hash != actual_hex_hash) { + const auto& expected_hash = integrity_.blocks[current_block_]; + const auto actual_hex_hash = base::ToLowerASCII(base::HexEncode(actual)); + if (expected_hash != actual_hex_hash) return false; - } current_block_++; diff --git a/shell/browser/net/asar/asar_file_validator.h b/shell/browser/net/asar/asar_file_validator.h index aa5bf2a041b8..48c9c432e1f3 100644 --- a/shell/browser/net/asar/asar_file_validator.h +++ b/shell/browser/net/asar/asar_file_validator.h @@ -36,6 +36,8 @@ class AsarFileValidator : public mojo::FilteredDataSource::Filter { bool FinishBlock(); private: + void EnsureBlockHashExists(); + base::File file_; IntegrityPayload integrity_; @@ -52,7 +54,7 @@ class AsarFileValidator : public mojo::FilteredDataSource::Filter { bool done_reading_ = false; int current_block_; int max_block_; - uint64_t current_hash_byte_count_ = 0; + uint64_t current_hash_byte_count_ = 0U; uint64_t total_hash_byte_count_ = 0; std::unique_ptr current_hash_; };