Repository: electron/electron
Pull request: fix: deprecated API and -Wunsafe-buffer-usage warnings in AsarFileValidator

#### Description of Change

Part 13 in a [series](https://github.com/electron/electron/pull/43477) to fix `-Wunsafe-buffer-usage` warnings in `shell/`. This one migrates a handful of deprecated / unsafe crypto API calls in `AsarFileValidator`.

Warning fixed by this PR:

```
2024-10-01T01:27:56.0161997Z ../../electron/shell/browser/net/asar/asar_file_validator.cc:63:34: error: unsafe pointer arithmetic [-Werror,-Wunsafe-buffer-usage]
2024-10-01T01:27:56.0162901Z    63 |     current_hash_->Update(buffer.data() + bytes_added, bytes_to_hash);
2024-10-01T01:27:56.0163362Z       |                           ~~~~~~~^~~~~~
2024-10-01T01:27:56.0164170Z ../../electron/shell/browser/net/asar/asar_file_validator.cc:63:34: note: See //docs/unsafe_buffers.md for help.
```

It also removes use of [these deprecated SecureHash methods](https://crbug.com/364687923):

```c++
    // Deprecated non-span APIs - do not add new uses of them, and please remove
    // existing uses.
    // TODO(https://crbug.com/364687923): Delete these.
    void Update(const void* input, size_t len);
    void Finish(void* output, size_t len);
```

#### Checklist

- [x] PR description included and stakeholders cc'd
- [x] `npm test` passes
- [x] [PR release notes](https://github.com/electron/clerk/blob/main/README.md) describe the change in a way relevant to app developers, and are [capitalized, punctuated, and past tense](https://github.com/electron/clerk/blob/main/README.md#examples).

#### Release Notes

Notes: none.

Changed files:
filename: shell/browser/net/asar/asar_file_validator.cc
patch: @@ -5,10 +5,12 @@
 #include "shell/browser/net/asar/asar_file_validator.h"
 
 #include <algorithm>
+#include <array>
 #include <string>
 #include <utility>
 #include <vector>
 
+#include "base/containers/span.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -25,50 +27,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<char> 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;
-
-    if (current_hash_byte_count_ == block_size && !FinishBlock()) {
-      LOG(FATAL) << "Failed to validate block while streaming ASAR file: "
-                 << current_block_;
-    }
+    EnsureBlockHashExists();
+
+    // 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;
   }
 }
 
@@ -85,8 +88,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
@@ -103,21 +104,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<uint8_t, crypto::kSHA256Length>{};
+  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_++;
 

filename: shell/browser/net/asar/asar_file_validator.h
patch: @@ -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<crypto::SecureHash> current_hash_;
 };