iwa: Added a reader for .swbn of unknown ID

Added a new UnsecureSignedWebBundleReader. This class should be
used to read data from unverifiable signed web bundle. The unverifiable
signed web bundle is a bundle which ID we don't know. Because of that
verification of the file against the public key stored in the file
doesn't make sense as attacker can just resign the modified file
with their own key.

UnsecureSignedWebBundleReader should be used to get info about the
file, and NOT for general usage (e.g. reading responses).

Bug: b:277233728
Test: Added a unit test and executed already existing tests.

Change-Id: I68069fde95cf73b204c4a4fe27b0b0924afc3383
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4604542
Auto-Submit: Oleksandr Peletskyi <peletskyi@google.com>
Commit-Queue: Oleksandr Peletskyi <peletskyi@google.com>
Reviewed-by: Christian Flach <cmfcmf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1158183}

chrome/browser/web_applications/isolated_web_apps/isolated_web_app_url_info.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/files/file_path.h"
+#include "base/functional/bind.h"
 #include "base/functional/callback.h"
 #include "base/functional/callback_helpers.h"
 #include "base/functional/overloaded.h"
@@ -27,59 +28,29 @@ namespace web_app {
 
 namespace {
 
+base::expected<IsolatedWebAppUrlInfo, std::string> MakeIsolatedWebAppUrlInfo(
+    base::expected<web_package::SignedWebBundleId, UnusableSwbnFileError>
+        bundle_id) {
+  return bundle_id
+      .transform([](const web_package::SignedWebBundleId& id) {
+        return IsolatedWebAppUrlInfo::CreateFromSignedWebBundleId(id);
+      })
+      .transform_error([](const UnusableSwbnFileError& error) {
+        return "Failed to read the integrity block of the signed web bundle: " +
+               error.message();
+      });
+}
+
 void GetSignedWebBundleIdByPath(
     const base::FilePath& path,
     base::OnceCallback<void(base::expected<IsolatedWebAppUrlInfo, std::string>)>
-        callback) {
-  std::unique_ptr<SignedWebBundleReader> reader =
-      SignedWebBundleReader::Create(path, /*base_url=*/absl::nullopt);
-
-  SignedWebBundleReader* reader_raw_ptr = reader.get();
-
-  auto [callback_first, callback_second] =
-      base::SplitOnceCallback(std::move(callback));
-
-  SignedWebBundleReader::IntegrityBlockReadResultCallback
-      integrity_block_result_callback = base::BindOnce(
-          [](base::OnceCallback<void(
-                 base::expected<IsolatedWebAppUrlInfo, std::string>)> callback,
-             web_package::SignedWebBundleIntegrityBlock integrity_block,
-             base::OnceCallback<void(
-                 SignedWebBundleReader::SignatureVerificationAction)>
-                 verify_callback) {
-            std::move(verify_callback)
-                .Run(SignedWebBundleReader::SignatureVerificationAction::Abort(
-                    "Stopped after reading the integrity block."));
-            web_package::SignedWebBundleId bundle_id =
-                integrity_block.signature_stack().derived_web_bundle_id();
-            std::move(callback).Run(
-                IsolatedWebAppUrlInfo::CreateFromSignedWebBundleId(bundle_id));
-          },
-          std::move(callback_first));
-
-  SignedWebBundleReader::ReadErrorCallback read_error_callback = base::BindOnce(
-      [](std::unique_ptr<SignedWebBundleReader> reader_ownership,
-         base::OnceCallback<void(
-             base::expected<IsolatedWebAppUrlInfo, std::string>)> callback,
-         base::expected<void, UnusableSwbnFileError> read_status) {
-        CHECK(!read_status.has_value());
-
-        // If the operation was aborted intentionally the reader will return
-        // kIntegrityBlockValidationError error. Strictly speaking we should
-        // have a separate error for this purpose. But because the code of this
-        // function will be soon refactored, let's use this temporary hacky
-        // solution.
-        if (read_status.error().value() !=
-            UnusableSwbnFileError::Error::kIntegrityBlockValidationError) {
-          std::move(callback).Run(base::unexpected(
-              "Failed to read the integrity block of the signed web bundle: " +
-              read_status.error().message()));
-        }
-      },
-      std::move(reader), std::move(callback_second));
-
-  reader_raw_ptr->StartReading(std::move(integrity_block_result_callback),
-                               std::move(read_error_callback));
+        url_info_ready_callback) {
+  UnsecureSignedWebBundleIdReader::WebBundleIdCallback id_read_callback =
+      base::BindOnce(&MakeIsolatedWebAppUrlInfo)
+          .Then(std::move(url_info_ready_callback));
+
+  UnsecureSignedWebBundleIdReader::GetWebBundleId(path,
+                                                  std::move(id_read_callback));
 }
 
 }  // namespace

chrome/browser/web_applications/isolated_web_apps/signed_web_bundle_reader.cc

@@ -602,4 +602,111 @@ SignedWebBundleReader::SignatureVerificationAction::SignatureVerificationAction(
 SignedWebBundleReader::SignatureVerificationAction::
     ~SignatureVerificationAction() = default;
 
+UnsecureReader::UnsecureReader(const base::FilePath& web_bundle_path)
+    : connection_(web_bundle_path, /*base_url=*/absl::nullopt) {}
+
+UnsecureReader::~UnsecureReader() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
+
+void UnsecureReader::StartReading() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  connection_.Initialize(
+      base::BindOnce(&UnsecureReader::OnConnectionInitialized, GetWeakPtr()));
+}
+
+void UnsecureReader::OnConnectionInitialized(
+    base::expected<void, UnusableSwbnFileError> init_status) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (!init_status.has_value()) {
+    ReturnError(init_status.error());
+    return;
+  }
+
+  DoReading();
+}
+
+// static
+void UnsecureSignedWebBundleIdReader::GetWebBundleId(
+    const base::FilePath& web_bundle_path,
+    WebBundleIdCallback result_callback) {
+  std::unique_ptr<UnsecureSignedWebBundleIdReader> reader =
+      base::WrapUnique(new UnsecureSignedWebBundleIdReader(web_bundle_path));
+  UnsecureSignedWebBundleIdReader* const reader_raw_ptr = reader.get();
+
+  // We pass the owning unique_ptr to the second no-op callback to keep
+  // the instance of UnsecureSignedWebBundleIdReader alive.
+  WebBundleIdCallback id_read_callback =
+      base::BindOnce(std::move(result_callback))
+          .Then(base::BindOnce(
+              [](std::unique_ptr<UnsecureSignedWebBundleIdReader> owning_ptr) {
+              },
+              std::move(reader)));
+
+  reader_raw_ptr->SetResultCallback(std::move(id_read_callback));
+  reader_raw_ptr->StartReading();
+}
+
+UnsecureSignedWebBundleIdReader::UnsecureSignedWebBundleIdReader(
+    const base::FilePath& web_bundle_path)
+    : UnsecureReader(web_bundle_path) {}
+
+void UnsecureSignedWebBundleIdReader::DoReading() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  connection_.parser_->ParseIntegrityBlock(
+      base::BindOnce(&UnsecureSignedWebBundleIdReader::OnIntegrityBlockParsed,
+                     weak_ptr_factory_.GetWeakPtr()));
+}
+
+void UnsecureSignedWebBundleIdReader::ReturnError(UnusableSwbnFileError error) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  std::move(web_bundle_id_callback_).Run(base::unexpected(std::move(error)));
+}
+
+base::WeakPtr<UnsecureReader> UnsecureSignedWebBundleIdReader::GetWeakPtr() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return weak_ptr_factory_.GetWeakPtr();
+}
+
+void UnsecureSignedWebBundleIdReader::OnIntegrityBlockParsed(
+    web_package::mojom::BundleIntegrityBlockPtr raw_integrity_block,
+    web_package::mojom::BundleIntegrityBlockParseErrorPtr error) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  if (error) {
+    ReturnError(UnusableSwbnFileError(std::move(error)));
+    return;
+  }
+
+  auto integrity_block =
+      web_package::SignedWebBundleIntegrityBlock::Create(
+          std::move(raw_integrity_block))
+          .transform_error([](std::string error) {
+            return UnusableSwbnFileError(
+                UnusableSwbnFileError::Error::kIntegrityBlockParserFormatError,
+                "Error while parsing the Signed Web Bundle's integrity "
+                "block: " +
+                    std::move(error));
+          });
+
+  if (!integrity_block.has_value()) {
+    ReturnError(std::move(integrity_block.error()));
+    return;
+  }
+
+  web_package::SignedWebBundleId bundle_id =
+      integrity_block->signature_stack().derived_web_bundle_id();
+
+  std::move(web_bundle_id_callback_).Run(std::move(bundle_id));
+}
+
+void UnsecureSignedWebBundleIdReader::SetResultCallback(
+    WebBundleIdCallback web_bundle_id_result_callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  web_bundle_id_callback_ = std::move(web_bundle_id_result_callback);
+}
+
+UnsecureSignedWebBundleIdReader::~UnsecureSignedWebBundleIdReader() = default;
+
 }  // namespace web_app

chrome/browser/web_applications/isolated_web_apps/signed_web_bundle_reader.h

@@ -16,6 +16,7 @@
 #include "chrome/browser/web_applications/isolated_web_apps/error/unusable_swbn_file_error.h"
 #include "components/web_package/mojom/web_bundle_parser.mojom-forward.h"
 #include "components/web_package/shared_file.h"
+#include "components/web_package/signed_web_bundles/signed_web_bundle_id.h"
 #include "components/web_package/signed_web_bundles/signed_web_bundle_signature_verifier.h"
 #include "net/base/net_errors.h"
 #include "services/data_decoder/public/cpp/safe_web_bundle_parser.h"
@@ -356,6 +357,69 @@ class SignedWebBundleReader {
   base::WeakPtrFactory<SignedWebBundleReader> weak_ptr_factory_{this};
 };
 
+// This is a base class for fetching an info about a unsecure .swbn file.
+// The implementation of the pure virtual functions of this class should
+// provide a logic to read a specific thing from a bundle.
+// A signed web bundle considered unsecure if the signed web bundle ID of the
+// file is not known from a trusted source. Examples of trusted source of the ID
+// are the enterprise policy, a distributor store, etc.
+// Integrity check of the .swbn file without knowing the expected ID makes no
+// sense as an attacker can resign the tampered bundle with their private key.
+class UnsecureReader {
+ public:
+  UnsecureReader(const UnsecureReader&) = delete;
+  UnsecureReader& operator=(const UnsecureReader&) = delete;
+  virtual ~UnsecureReader();
+
+ protected:
+  explicit UnsecureReader(const base::FilePath& web_bundle_path);
+  // Initializes the connection which in the end leads to
+  // |DoReading()| execution.
+  void StartReading();
+
+  // Implementation of this does real work to fetch the particular
+  // information about the .swbn file.
+  virtual void DoReading() = 0;
+  // Implementation should return an error to the caller.
+  virtual void ReturnError(UnusableSwbnFileError error) = 0;
+  virtual base::WeakPtr<UnsecureReader> GetWeakPtr() = 0;
+
+  void OnConnectionInitialized(
+      base::expected<void, UnusableSwbnFileError> init_status);
+
+  internal::SafeWebBundleParserConnection connection_;
+  SEQUENCE_CHECKER(sequence_checker_);
+};
+
+class UnsecureSignedWebBundleIdReader : public UnsecureReader {
+ public:
+  using WebBundleIdCallback = base::OnceCallback<void(
+      base::expected<web_package::SignedWebBundleId, UnusableSwbnFileError>)>;
+
+  static void GetWebBundleId(const base::FilePath& web_bundle_path,
+                             WebBundleIdCallback result_callback);
+
+  ~UnsecureSignedWebBundleIdReader() override;
+
+ protected:
+  explicit UnsecureSignedWebBundleIdReader(
+      const base::FilePath& web_bundle_path);
+
+  void DoReading() override;
+  base::WeakPtr<UnsecureReader> GetWeakPtr() override;
+  void ReturnError(UnusableSwbnFileError error) override;
+
+  void OnIntegrityBlockParsed(
+      web_package::mojom::BundleIntegrityBlockPtr raw_integrity_block,
+      web_package::mojom::BundleIntegrityBlockParseErrorPtr error);
+
+ private:
+  void SetResultCallback(WebBundleIdCallback web_bundle_id_result_callback);
+
+  WebBundleIdCallback web_bundle_id_callback_;
+  base::WeakPtrFactory<UnsecureSignedWebBundleIdReader> weak_ptr_factory_{this};
+};
+
 }  // namespace web_app
 
 #endif  // CHROME_BROWSER_WEB_APPLICATIONS_ISOLATED_WEB_APPS_SIGNED_WEB_BUNDLE_H_

chrome/browser/web_applications/isolated_web_apps/signed_web_bundle_reader_unittest.cc

@@ -757,4 +757,89 @@ INSTANTIATE_TEST_SUITE_P(All,
                          ::testing::Values(absl::nullopt,
                                            "https://example.com"));
 
+class UnsecureSignedWebBundleReaderTest : public testing::Test {
+ protected:
+  void SetUp() override {
+    parser_factory_ =
+        std::make_unique<web_package::MockWebBundleParserFactory>();
+
+    web_package::mojom::BundleIntegrityBlockSignatureStackEntryPtr
+        signature_stack_entry =
+            web_package::mojom::BundleIntegrityBlockSignatureStackEntry::New();
+    signature_stack_entry->public_key = web_package::Ed25519PublicKey::Create(
+        base::make_span(kEd25519PublicKey));
+    signature_stack_entry->signature = web_package::Ed25519Signature::Create(
+        base::make_span(kEd25519Signature));
+
+    std::vector<web_package::mojom::BundleIntegrityBlockSignatureStackEntryPtr>
+        signature_stack;
+    signature_stack.push_back(std::move(signature_stack_entry));
+
+    integrity_block_ = web_package::mojom::BundleIntegrityBlock::New();
+    integrity_block_->size = 123;
+    integrity_block_->signature_stack = std::move(signature_stack);
+
+    EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
+    EXPECT_TRUE(
+        CreateTemporaryFileInDir(temp_dir_.GetPath(), &temp_file_path_));
+
+    in_process_data_decoder_.service()
+        .SetWebBundleParserFactoryBinderForTesting(base::BindRepeating(
+            &web_package::MockWebBundleParserFactory::AddReceiver,
+            base::Unretained(parser_factory_.get())));
+  }
+
+  void TearDown() override {
+    // Allow cleanup tasks posted by the destructor of `web_package::SharedFile`
+    // to run.
+    task_environment_.RunUntilIdle();
+  }
+
+  content::BrowserTaskEnvironment task_environment_;
+  data_decoder::test::InProcessDataDecoder in_process_data_decoder_;
+  base::ScopedTempDir temp_dir_;
+  base::FilePath temp_file_path_;
+
+  std::unique_ptr<web_package::MockWebBundleParserFactory> parser_factory_;
+  web_package::mojom::BundleIntegrityBlockPtr integrity_block_;
+};
+
+TEST_F(UnsecureSignedWebBundleReaderTest, ReadValidId) {
+  base::test::TestFuture<
+      base::expected<web_package::SignedWebBundleId, UnusableSwbnFileError>>
+      read_web_bundle_id_future;
+
+  UnsecureSignedWebBundleIdReader::GetWebBundleId(
+      temp_file_path_, read_web_bundle_id_future.GetCallback());
+  parser_factory_->RunIntegrityBlockCallback(integrity_block_->Clone());
+
+  base::expected<web_package::SignedWebBundleId, UnusableSwbnFileError>
+      bundle_id_result = read_web_bundle_id_future.Take();
+
+  ASSERT_TRUE(bundle_id_result.has_value());
+  web_package::Ed25519PublicKey public_key =
+      web_package::Ed25519PublicKey::Create(base::make_span(kEd25519PublicKey));
+  EXPECT_THAT(
+      bundle_id_result.value(),
+      web_package::SignedWebBundleId::CreateForEd25519PublicKey(public_key));
+}
+
+TEST_F(UnsecureSignedWebBundleReaderTest, ErrorId) {
+  base::test::TestFuture<
+      base::expected<web_package::SignedWebBundleId, UnusableSwbnFileError>>
+      read_web_bundle_id_future;
+
+  UnsecureSignedWebBundleIdReader::GetWebBundleId(
+      temp_file_path_, read_web_bundle_id_future.GetCallback());
+  parser_factory_->RunIntegrityBlockCallback(
+      nullptr, web_package::mojom::BundleIntegrityBlockParseError::New());
+
+  base::expected<web_package::SignedWebBundleId, UnusableSwbnFileError>
+      bundle_id_result = read_web_bundle_id_future.Take();
+
+  ASSERT_FALSE(bundle_id_result.has_value());
+  EXPECT_EQ(bundle_id_result.error().value(),
+            UnusableSwbnFileError::Error::kIntegrityBlockParserInternalError);
+}
+
 }  // namespace web_app