From fc030cda2601da86f606d0f8d45ac9b27c8eca5d Mon Sep 17 00:00:00 2001
From: Andrew Paseltiner <[email protected]>
Date: Tue, 21 Apr 2026 08:32:13 -0700
Subject: [PATCH] Fix hash desynchronization in download resumption

This CL fixes a logic error where a stale SHA-256 hash state could
persist when an interrupted download was restarted from offset 0. This
resulted in an incorrect final hash calculation, which could be used to
bypass Safe Browsing hash-based blocklist checks.

Key changes:

- In DownloadItemImpl::ResumeInterruptedDownload, explicitly reset the
  hash state if the download offset is clamped to 0 due to
  insufficient validation data.
- In HandleSuccessfulServerResponse, ensure the hash state is reset
  whenever a full response (200 OK) is received for a partial
  request, regardless of the expected offset. This fixes a bypass
  where the existing failsafe was skipped if the offset was already 0.
- In BaseFile::Open, reset the cryptographic hash object to a fresh
  SHA-256 state if the physical file is truncated to the beginning.

Regression tests added to base_file_unittest.cc,
download_item_impl_unittest.cc, and download_utils_unittest.cc.

Fixed: 500510384
Change-Id: I3c28887981a6e04daca12e344afaf6513f3036b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7762205
Reviewed-by: Xinghui (xing) Lu <[email protected]>
Reviewed-by: Min Qin <[email protected]>
Commit-Queue: Andrew Paseltiner <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1618214}
---

diff --git a/components/download/internal/common/base_file.cc b/components/download/internal/common/base_file.cc
index 62c2ffd5..fc570b5d 100644
--- a/components/download/internal/common/base_file.cc
+++ b/components/download/internal/common/base_file.cc
@@ -463,6 +463,12 @@
       ClearFile();
       return LogSystemError("Truncating to last known offset", error);
     }
+
+    // If the file was truncated to the beginning, the hash state is no longer
+    // valid.
+    if (bytes_so_far_ == 0) {
+      secure_hash_ = crypto::SecureHash::Create(crypto::SecureHash::SHA256);
+    }
   } else if (file_size < bytes_so_far_) {
     // The file is shorter than we expected.  Our hashes won't be valid.
     *bytes_wasted = bytes_so_far_;
diff --git a/components/download/internal/common/base_file_unittest.cc b/components/download/internal/common/base_file_unittest.cc
index aec606f..001f9805 100644
--- a/components/download/internal/common/base_file_unittest.cc
+++ b/components/download/internal/common/base_file_unittest.cc
@@ -802,4 +802,45 @@
   base_file_->Finish();
 }
 
+// Regression test for crbug.com/500510384. Truncating a file to 0 should reset
+// the hash state.
+TEST_F(BaseFileTest, TruncateToZeroResetsHash) {
+  ASSERT_TRUE(InitializeFile());
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
+  base::FilePath path = base_file_->full_path();
+
+  // "Finish" to get the hash state.
+  std::unique_ptr<crypto::SecureHash> hash_state = base_file_->Finish();
+  base_file_->Detach();
+
+  // Now "resume" the download from 0, but provide the stale hash state.
+  // This simulates what happens in DownloadFileImpl::Initialize when
+  // DownloadItemImpl::ResumeInterruptedDownload clamps the offset to 0
+  // but passes along the moved hash_state_.
+  base_file_ = std::make_unique<BaseFile>(DownloadItem::kInvalidId);
+  set_expected_data("");
+  DownloadInterruptReason result = base_file_->Initialize(
+      path, base::FilePath(), base::File(), 0, std::string(),
+      std::move(hash_state), false, &kTestDataBytesWasted);
+  ASSERT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, result);
+
+  // Write some new data.
+  ASSERT_TRUE(AppendDataToFile(kTestData2));
+
+  // The final hash should be just SHA256(kTestData2).
+  std::unique_ptr<crypto::SecureHash> final_hash_state = base_file_->Finish();
+
+  std::array<uint8_t, crypto::hash::kSha256Size> actual_hash;
+  final_hash_state->Finish(actual_hash);
+
+  std::unique_ptr<crypto::SecureHash> expected_hash_provider =
+      crypto::SecureHash::Create(crypto::SecureHash::SHA256);
+  expected_hash_provider->Update(base::as_byte_span(kTestData2));
+  std::array<uint8_t, crypto::hash::kSha256Size> expected_hash;
+  expected_hash_provider->Finish(expected_hash);
+
+  // Verification that the hash state was correctly reset.
+  EXPECT_EQ(expected_hash, actual_hash);
+}
+
 }  // namespace download
diff --git a/components/download/internal/common/download_item_impl.cc b/components/download/internal/common/download_item_impl.cc
index 2c2d4e8..7de45e83 100644
--- a/components/download/internal/common/download_item_impl.cc
+++ b/components/download/internal/common/download_item_impl.cc
@@ -2619,6 +2619,7 @@
       // There is not enough data for validation, simply overwrites the
       // existing data from the beginning.
       download_params->set_offset(0);
+      download_params->set_hash_state(nullptr);
     }
   }
 
diff --git a/components/download/internal/common/download_item_impl_unittest.cc b/components/download/internal/common/download_item_impl_unittest.cc
index 34a8cbd..7af8432 100644
--- a/components/download/internal/common/download_item_impl_unittest.cc
+++ b/components/download/internal/common/download_item_impl_unittest.cc
@@ -21,6 +21,7 @@
 #include "base/functional/callback_helpers.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/raw_ptr.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_view_util.h"
 #include "base/task/single_thread_task_runner.h"
@@ -46,6 +47,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
+using ::testing::AnyNumber;
 using ::testing::ByMove;
 using ::testing::DoAll;
 using ::testing::InvokeWithoutArgs;
@@ -2504,5 +2506,60 @@
   task_environment_.RunUntilIdle();
 }
 
+// Regression test for crbug.com/500510384. ResumeInterruptedDownload should
+// reset the hash state when clamping the offset to 0.
+TEST_F(DownloadItemTest, ResumptionClampingClearsHashState) {
+  base::test::ScopedFeatureList feature_list;
+  std::map<std::string, std::string> params;
+  params["download_validation_length"] = "1024";
+  feature_list.InitAndEnableFeatureWithParameters(
+      features::kAllowDownloadResumptionWithoutStrongValidators, params);
+
+  // 1. Create an interrupted download with some hash state.
+  create_info()->etag.clear();
+  create_info()->last_modified.clear();
+  DownloadItemImpl* item = CreateDownloadItem();
+
+  // We need to start the download to get it into a state where it can be
+  // interrupted.
+  MockDownloadFile* download_file =
+      DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
+  ASSERT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
+
+  std::unique_ptr<crypto::SecureHash> hash_state =
+      crypto::SecureHash::Create(crypto::SecureHash::SHA256);
+  hash_state->Update(kHashOfTestData1);
+
+  // 2. Resume the download.
+  // We expect ResumeInterruptedDownload to be called on the delegate.
+  // We want to inspect the DownloadUrlParameters it receives.
+  // Note: For FILE_TOO_SHORT, auto-resumption might happen immediately.
+  int64_t captured_offset = -1;
+  bool captured_has_hash_state = false;
+  base::RunLoop run_loop;
+  EXPECT_CALL(*mock_delegate(), MockResumeInterruptedDownload(_))
+      .WillOnce([&](DownloadUrlParameters* params) {
+        captured_offset = params->offset();
+        captured_has_hash_state =
+            (params->TakeSaveInfo().hash_state != nullptr);
+        run_loop.Quit();
+      });
+
+  // Interrupt the download at offset 500 (which is < 1024).
+  // Use FILE_TOO_SHORT which forces a RESTART.
+  EXPECT_CALL(*download_file, Cancel()).Times(AnyNumber());
+  EXPECT_CALL(*download_file, Detach()).Times(AnyNumber());
+  item->DestinationObserverAsWeakPtr()->DestinationError(
+      DOWNLOAD_INTERRUPT_REASON_FILE_TOO_SHORT, 500, std::move(hash_state));
+  run_loop.Run();
+
+  // The offset should be clamped to 0 because 500 < 1024 and no strong
+  // validators.
+  EXPECT_EQ(0, captured_offset);
+
+  // Verification that the hash state was cleared.
+  EXPECT_FALSE(captured_has_hash_state);
+}
+
 }  // namespace
 }  // namespace download
diff --git a/components/download/internal/common/download_utils.cc b/components/download/internal/common/download_utils.cc
index 97726a7..dc3aa5f4 100644
--- a/components/download/internal/common/download_utils.cc
+++ b/components/download/internal/common/download_utils.cc
@@ -293,7 +293,7 @@
   }

diff --git a/components/download/internal/common/base_file_unittest.cc b/components/download/internal/common/base_file_unittest.cc
index aec606f..001f9805 100644
--- a/components/download/internal/common/base_file_unittest.cc
+++ b/components/download/internal/common/base_file_unittest.cc
@@ -802,4 +802,45 @@
   base_file_->Finish();
 }
 
+// Regression test for crbug.com/500510384. Truncating a file to 0 should reset
+// the hash state.
+TEST_F(BaseFileTest, TruncateToZeroResetsHash) {
+  ASSERT_TRUE(InitializeFile());
+  ASSERT_TRUE(AppendDataToFile(kTestData1));
+  base::FilePath path = base_file_->full_path();
+
+  // "Finish" to get the hash state.
+  std::unique_ptr<crypto::SecureHash> hash_state = base_file_->Finish();
+  base_file_->Detach();
+
+  // Now "resume" the download from 0, but provide the stale hash state.
+  // This simulates what happens in DownloadFileImpl::Initialize when
+  // DownloadItemImpl::ResumeInterruptedDownload clamps the offset to 0
+  // but passes along the moved hash_state_.
+  base_file_ = std::make_unique<BaseFile>(DownloadItem::kInvalidId);
+  set_expected_data("");
+  DownloadInterruptReason result = base_file_->Initialize(
+      path, base::FilePath(), base::File(), 0, std::string(),
+      std::move(hash_state), false, &kTestDataBytesWasted);
+  ASSERT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE, result);
+
+  // Write some new data.
+  ASSERT_TRUE(AppendDataToFile(kTestData2));
+
+  // The final hash should be just SHA256(kTestData2).
+  std::unique_ptr<crypto::SecureHash> final_hash_state = base_file_->Finish();
+
+  std::array<uint8_t, crypto::hash::kSha256Size> actual_hash;
+  final_hash_state->Finish(actual_hash);
+
+  std::unique_ptr<crypto::SecureHash> expected_hash_provider =
+      crypto::SecureHash::Create(crypto::SecureHash::SHA256);
+  expected_hash_provider->Update(base::as_byte_span(kTestData2));
+  std::array<uint8_t, crypto::hash::kSha256Size> expected_hash;
+  expected_hash_provider->Finish(expected_hash);
+
+  // Verification that the hash state was correctly reset.
+  EXPECT_EQ(expected_hash, actual_hash);
+}
+
 }  // namespace download
diff --git a/components/download/internal/common/download_item_impl_unittest.cc b/components/download/internal/common/download_item_impl_unittest.cc
index 34a8cbd..7af8432 100644
--- a/components/download/internal/common/download_item_impl_unittest.cc
+++ b/components/download/internal/common/download_item_impl_unittest.cc
@@ -21,6 +21,7 @@
 #include "base/functional/callback_helpers.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/raw_ptr.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_view_util.h"
 #include "base/task/single_thread_task_runner.h"
@@ -46,6 +47,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using ::testing::_;
+using ::testing::AnyNumber;
 using ::testing::ByMove;
 using ::testing::DoAll;
 using ::testing::InvokeWithoutArgs;
@@ -2504,5 +2506,60 @@
   task_environment_.RunUntilIdle();
 }
 
+// Regression test for crbug.com/500510384. ResumeInterruptedDownload should
+// reset the hash state when clamping the offset to 0.
+TEST_F(DownloadItemTest, ResumptionClampingClearsHashState) {
+  base::test::ScopedFeatureList feature_list;
+  std::map<std::string, std::string> params;
+  params["download_validation_length"] = "1024";
+  feature_list.InitAndEnableFeatureWithParameters(
+      features::kAllowDownloadResumptionWithoutStrongValidators, params);
+
+  // 1. Create an interrupted download with some hash state.
+  create_info()->etag.clear();
+  create_info()->last_modified.clear();
+  DownloadItemImpl* item = CreateDownloadItem();
+
+  // We need to start the download to get it into a state where it can be
+  // interrupted.
+  MockDownloadFile* download_file =
+      DoIntermediateRename(item, DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
+  ASSERT_EQ(DownloadItem::IN_PROGRESS, item->GetState());
+
+  std::unique_ptr<crypto::SecureHash> hash_state =
+      crypto::SecureHash::Create(crypto::SecureHash::SHA256);
+  hash_state->Update(kHashOfTestData1);
+
+  // 2. Resume the download.
+  // We expect ResumeInterruptedDownload to be called on the delegate.
+  // We want to inspect the DownloadUrlParameters it receives.
+  // Note: For FILE_TOO_SHORT, auto-resumption might happen immediately.
+  int64_t captured_offset = -1;
+  bool captured_has_hash_state = false;
+  base::RunLoop run_loop;
+  EXPECT_CALL(*mock_delegate(), MockResumeInterruptedDownload(_))
+      .WillOnce([&](DownloadUrlParameters* params) {
+        captured_offset = params->offset();
+        captured_has_hash_state =
+            (params->TakeSaveInfo().hash_state != nullptr);
+        run_loop.Quit();
+      });
+
+  // Interrupt the download at offset 500 (which is < 1024).
+  // Use FILE_TOO_SHORT which forces a RESTART.
+  EXPECT_CALL(*download_file, Cancel()).Times(AnyNumber());
+  EXPECT_CALL(*download_file, Detach()).Times(AnyNumber());
+  item->DestinationObserverAsWeakPtr()->DestinationError(
+      DOWNLOAD_INTERRUPT_REASON_FILE_TOO_SHORT, 500, std::move(hash_state));
+  run_loop.Run();
+
+  // The offset should be clamped to 0 because 500 < 1024 and no strong
+  // validators.
+  EXPECT_EQ(0, captured_offset);
+
+  // Verification that the hash state was cleared.
+  EXPECT_FALSE(captured_has_hash_state);
+}
+
 }  // namespace
 }  // namespace download
diff --git a/components/download/internal/common/download_utils_unittest.cc b/components/download/internal/common/download_utils_unittest.cc
index b4d0406..e0effef 100644
--- a/components/download/internal/common/download_utils_unittest.cc
+++ b/components/download/internal/common/download_utils_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/test/scoped_feature_list.h"
 #include "components/download/public/common/download_features.h"
+#include "crypto/secure_hash.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
@@ -142,5 +143,22 @@
   EXPECT_EQ(resource_request->permissions_policy, std::nullopt);
 }
 
+// Regression test for crbug.com/500510384.
+// Scenario: Resumption was attempted, but offset was clamped to 0.
+TEST(DownloadUtilsTest, HandleServerResponse200_ClampedOffsetClearsHash) {
+  scoped_refptr<net::HttpResponseHeaders> headers(
+      new net::HttpResponseHeaders("HTTP/1.1 200 OK"));
+  DownloadSaveInfo save_info;
+  save_info.offset = 0;
+  save_info.hash_state = crypto::SecureHash::Create(crypto::SecureHash::SHA256);
+
+  EXPECT_EQ(DOWNLOAD_INTERRUPT_REASON_NONE,
+            HandleSuccessfulServerResponse(*headers, &save_info,
+                                           /*fetch_error_body=*/false));
+
+  // Verification that the hash state was cleared.
+  EXPECT_EQ(nullptr, save_info.hash_state);
+}
+
 }  // namespace
 }  // namespace download