0

Revert "Use Mojo for communicating with web_app_shortcut_copier"

This reverts commit 3806c10c31.

Reason for revert:
[gardener]
Causes consistent failures on some official bots such as mac-chrome
https://ci.chromium.org/ui/p/chrome/builders/ci/mac-chrome
also
https://ci.chromium.org/ui/p/chrome/builders/ci/mac64
https://ci.chromium.org/ui/p/chrome/builders/ci/mac-rel-ready

On the first bot mentioned, failures started here:
https://ci.chromium.org/ui/p/chrome/builders/ci/mac-chrome/40062/overview

They are compile failures:
[8242/19922] CXX obj/chrome/browser/web_applications/os_integration/mac/web_app_shortcut_copier_lib/web_app_shortcut_copier_mac.o
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF obj/chrome/browser/web_applications/os_integration/mac/web_app_shortcut_copi...(too long)
../../chrome/browser/web_applications/os_integration/mac/web_app_shortcut_copier_mac.cc:71:40: error: invalid operands to binary expression ('const char[18]' and 'const char[6]')
   71 |           MAC_BUNDLE_IDENTIFIER_STRING + ".beta",
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~
../../chrome/browser/web_applications/os_integration/mac/web_app_shortcut_copier_mac.cc:72:40: error: invalid operands to binary expression ('const char[18]' and 'const char[8]')
   72 |           MAC_BUNDLE_IDENTIFIER_STRING + ".canary",
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~

This is the obvious cause (the only change on the blamelist that touched that file).

Original change's description:
> Use Mojo for communicating with web_app_shortcut_copier
>
> This makes it possible for web_app_shortcut_copier to reliably verify
> the identity of the process that it is performing work on behalf of
> (http://go/peer-process-validation).
>
> `WebAppShortcutCreator` moves away from passing the source and
> destination path to be copied to `web_app_shortcut_copier` via command
> line arguments. Instead a Mojo invitation is passed via the command line
> and a synchronous call is used to request the copy be performed.
>
> `web_app_shortcut_copier` is updated to recover the Mojo endpoint from
> its command line and process the `CopyWebAppShortcut` message. The
> copier exits after a message has been processed.
>
> During initialization, `web_app_shortcut_copier` sets a
> `ProcessRequirement` to prevent anyone other than the browser process
> from establishing a Mojo connection with it. Enforcement of the
> requirement by `MachPortRendezvousClientMac` is currently gated by a
> feature flag. This needs to be enabled by default before the feature
> flag that enables use of `web_app_shortcut_copier` can be enabled.
>
> Bug: 361784552
> Change-Id: I3a9b5128d21223bcd03a4acda2491bb1e42ed896
> Include-Ci-Only-Tests: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5924245
> Reviewed-by: Nasko Oskov <nasko@chromium.org>
> Reviewed-by: Alex Gough <ajgo@chromium.org>
> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
> Auto-Submit: Mark Rowe <markrowe@chromium.org>
> Commit-Queue: Mark Rowe <markrowe@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1374160}

Bug: 361784552
Change-Id: I82ee885dbec3e0d0cd1452b59e7f6d6a3e22912d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5967679
Auto-Submit: Mark Pearson <mpearson@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Mark Pearson <mpearson@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1374209}
This commit is contained in:
Mark Pearson
2024-10-26 00:21:05 +00:00
committed by Chromium LUCI CQ
parent 0daa397623
commit 0ace7a682c
8 changed files with 197 additions and 251 deletions

@ -566,10 +566,7 @@ source_set("web_applications") {
}
if (is_mac) {
deps += [
"//chrome/browser/web_applications/mojom:web_app_shortcut_copier_mojom",
"//components/os_crypt/sync:os_crypt",
]
deps += [ "//components/os_crypt/sync:os_crypt" ]
}
if (is_win) {

@ -6,9 +6,3 @@ mojom("mojom_web_apps_enum") {
public_deps = []
webui_module_path = "/"
}
mojom("web_app_shortcut_copier_mojom") {
sources = [ "web_app_shortcut_copier.mojom" ]
public_deps = [ "//mojo/public/mojom/base" ]
}

@ -1,22 +0,0 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
module web_app.mojom;
import "mojo/public/mojom/base/file_path.mojom";
// Interface for delegating installation of the native app representing
// a Progressive Web Application to `web_app_shortcut_copier`.
// The implementation lives within `web_app_shortcut_copier` and is called by
// the browser process during PWA installation.
[EnableIf=is_mac]
interface WebAppShortcutCopier {
// Copy the application bundle rooted at `source_path` to
// `destination_path`. Returns true on success or false if an error occurred
// during copying.
[Sync]
CopyWebAppShortcut(mojo_base.mojom.FilePath source_path,
mojo_base.mojom.FilePath destination_path)
=> (bool result);
};

@ -6,13 +6,11 @@ import("//build/util/branding.gni")
import("//chrome/version.gni")
source_set("web_app_shortcut_copier_lib") {
sources = [ "web_app_shortcut_copier_mac.cc" ]
sources = [ "web_app_shortcut_copier_mac.mm" ]
deps = [
"//base",
"//chrome/browser/apps/app_shim:app_shim",
"//chrome/browser/web_applications/mojom:web_app_shortcut_copier_mojom",
"//chrome/common:version_header",
"//content/public/common:main_function_params",
]
}

@ -1,164 +0,0 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Installs the native app shim for a web app to its final location.
#include "base/apple/bundle_locations.h"
#include "base/apple/mach_port_rendezvous.h"
#include "base/base_paths.h"
#include "base/command_line.h"
#include "base/feature_list.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/functional/callback.h"
#include "base/logging.h"
#include "base/mac/process_requirement.h"
#include "base/message_loop/message_pump_type.h"
#include "base/metrics/field_trial.h"
#include "base/metrics/histogram_shared_memory.h"
#include "base/path_service.h"
#include "base/run_loop.h"
#include "base/task/single_thread_task_executor.h"
#include "build/branding_buildflags.h"
#include "chrome/browser/web_applications/mojom/web_app_shortcut_copier.mojom.h"
#include "chrome/common/chrome_version.h"
#include "mojo/core/embedder/configuration.h"
#include "mojo/core/embedder/embedder.h"
#include "mojo/core/embedder/scoped_ipc_support.h"
#include "mojo/public/cpp/bindings/pending_receiver.h"
#include "mojo/public/cpp/bindings/receiver.h"
#include "mojo/public/cpp/platform/platform_channel.h"
#include "mojo/public/cpp/platform/platform_channel_endpoint.h"
#include "mojo/public/cpp/system/invitation.h"
namespace {
class WebAppShortcutCopierImpl : public web_app::mojom::WebAppShortcutCopier {
public:
explicit WebAppShortcutCopierImpl(
mojo::PendingReceiver<web_app::mojom::WebAppShortcutCopier>
pending_receiver,
base::OnceClosure quit_callback)
: receiver_(this, std::move(pending_receiver)),
quit_callback_(std::move(quit_callback)) {}
void CopyWebAppShortcut(const base::FilePath& source_path,
const base::FilePath& destination_path,
CopyWebAppShortcutCallback callback) override {
if (base::CopyDirectory(source_path, destination_path, true)) {
std::move(callback).Run(true);
} else {
LOG(ERROR) << "Copying app from " << source_path << " to "
<< destination_path << " failed.";
std::move(callback).Run(false);
}
std::move(quit_callback_).Run();
}
private:
mojo::Receiver<web_app::mojom::WebAppShortcutCopier> receiver_;
base::OnceClosure quit_callback_;
};
std::optional<base::mac::ProcessRequirement> CallerProcessRequirement() {
return base::mac::ProcessRequirement::Builder()
.SignedWithSameIdentity()
.IdentifierIsOneOf({
MAC_BUNDLE_IDENTIFIER_STRING,
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
MAC_BUNDLE_IDENTIFIER_STRING + ".beta",
MAC_BUNDLE_IDENTIFIER_STRING + ".canary",
MAC_BUNDLE_IDENTIFIER_STRING + ".dev",
#endif
})
// Chrome can be dynamically valid but not match its signature on disk
// if an update is staged and waiting for the browser to be relaunched.
.CheckDynamicValidityOnly()
.Build();
}
void InitializeFeatureState() {
const auto& command_line = *base::CommandLine::ForCurrentProcess();
base::HistogramSharedMemory::InitFromLaunchParameters(command_line);
base::FieldTrialList field_trial_list;
base::FieldTrialList::CreateTrialsInChildProcess(command_line);
auto feature_list = std::make_unique<base::FeatureList>();
base::FieldTrialList::ApplyFeatureOverridesInChildProcess(feature_list.get());
base::FeatureList::SetInstance(std::move(feature_list));
}
} // namespace
extern "C" {
// The entry point into the shortcut copier process. This is not
// a user API.
__attribute__((visibility("default"))) int ChromeWebAppShortcutCopierMain(
int argc,
char** argv);
}
// Installs the native app shim for a web app to its final location.
//
// When using ad-hoc signing for web app shims, the final app shim must be
// written to disk by this helper tool. This separate helper tool exists so that
// binary authorization tools, such as Santa, can transitively trust app shims
// that it creates without trusting all files written by Chrome. This allows app
// shims to be trusted by the binary authorization tool despite having only
// ad-hoc code signatures.
int ChromeWebAppShortcutCopierMain(int argc, char** argv) {
base::CommandLine::Init(argc, argv);
// Override the path to the framework bundle so that it has a sensible value.
// This tool lives within the Helpers subdirectory of the framework, so the
// versioned path is two levels upwards.
base::FilePath executable_path =
base::PathService::CheckedGet(base::FILE_EXE);
base::apple::SetOverrideFrameworkBundlePath(
executable_path.DirName().DirName());
auto requirement = CallerProcessRequirement();
if (!requirement) {
LOG(ERROR) << "Unable to construct requirement to validate caller";
return 1;
}
base::MachPortRendezvousClientMac::SetServerProcessRequirement(
std::move(*requirement));
// Ensure that field trials and feature state matches that of Chrome.
InitializeFeatureState();
base::SingleThreadTaskExecutor main_task_executor{base::MessagePumpType::IO};
mojo::core::InitFeatures();
mojo::core::Init({.is_broker_process = true});
mojo::core::ScopedIPCSupport ipc_support(
base::SingleThreadTaskRunner::GetCurrentDefault(),
mojo::core::ScopedIPCSupport::ShutdownPolicy::CLEAN);
mojo::PlatformChannelEndpoint endpoint =
mojo::PlatformChannel::RecoverPassedEndpointFromCommandLine(
*base::CommandLine::ForCurrentProcess());
if (!endpoint.is_valid()) {
LOG(ERROR) << "Unable to recover Mojo endpoint from command line.";
return 1;
}
mojo::ScopedMessagePipeHandle pipe =
mojo::IncomingInvitation::AcceptIsolated(std::move(endpoint));
if (!pipe->is_valid()) {
LOG(ERROR) << "Unable to accept Mojo invitation";
return 1;
}
base::RunLoop run_loop;
WebAppShortcutCopierImpl copier(
mojo::PendingReceiver<web_app::mojom::WebAppShortcutCopier>(
std::move(pipe)),
run_loop.QuitWhenIdleClosure());
run_loop.Run();
return 0;
}

@ -0,0 +1,177 @@
// Copyright 2024 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
//
// Copies files from argv[1] to argv[2]
#ifdef UNSAFE_BUFFERS_BUILD
// TODO(crbug.com/40285824): Remove this and convert code to safer constructs.
#pragma allow_unsafe_buffers
#endif
#include <CoreFoundation/CoreFoundation.h>
#include <Security/Security.h>
#include <unistd.h>
#include "base/apple/bundle_locations.h"
#include "base/apple/foundation_util.h"
#include "base/apple/scoped_cftyperef.h"
#include "base/base_paths.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/mac/code_signature.h"
#include "base/path_service.h"
#include "base/strings/stringprintf.h"
#include "base/types/expected_macros.h"
#include "chrome/browser/apps/app_shim/code_signature_mac.h"
namespace {
base::apple::ScopedCFTypeRef<CFStringRef>
BuildParentAppRequirementFromFrameworkRequirementString(
CFStringRef framwork_requirement) {
// Make sure the framework bundle requirement is in the expected format.
// It should start with 'identifier "' and have at least 2 quotes. This allows
// us to easily find the end of the "identifier" portion of the requirement so
// we can remove it.
CFIndex len = CFStringGetLength(framwork_requirement);
base::apple::ScopedCFTypeRef<CFArrayRef> quote_ranges(
CFStringCreateArrayWithFindResults(nullptr, framwork_requirement,
CFSTR("\""), CFRangeMake(0, len), 0));
if (!CFStringHasPrefix(framwork_requirement, CFSTR("identifier \"")) ||
!quote_ranges || CFArrayGetCount(quote_ranges.get()) < 2) {
LOG(ERROR) << "Framework bundle requirement is malformed.";
return base::apple::ScopedCFTypeRef<CFStringRef>(nullptr);
}
// Get the index of the second quote.
CFIndex second_quote_index =
static_cast<const CFRange*>(CFArrayGetValueAtIndex(quote_ranges.get(), 1))
->location;
// Make sure there is something to read after the second quote.
if (second_quote_index + 1 >= len) {
LOG(ERROR) << "Framework bundle requirement is too short";
return base::apple::ScopedCFTypeRef<CFStringRef>(nullptr);
}
// Build the app shim requirement. Keep the data from the framework bundle
// requirement starting after second quote.
base::apple::ScopedCFTypeRef<CFStringRef> parent_app_requirement_string(
CFStringCreateWithSubstring(
nullptr, framwork_requirement,
CFRangeMake(second_quote_index + 5, len - second_quote_index - 5)));
return parent_app_requirement_string;
}
// Creates a requirement for the parent app based on the framework bundle's
// designated requirement.
//
// Returns a non-null requirement or the reason why the requirement could not
// be created.
base::expected<base::apple::ScopedCFTypeRef<SecRequirementRef>,
apps::MissingRequirementReason>
CreateParentAppRequirement() {
ASSIGN_OR_RETURN(auto framework_requirement_string,
apps::FrameworkBundleDesignatedRequirementString());
base::apple::ScopedCFTypeRef<CFStringRef> parent_requirement_string =
BuildParentAppRequirementFromFrameworkRequirementString(
framework_requirement_string.get());
if (!parent_requirement_string) {
return base::unexpected(apps::MissingRequirementReason::Error);
}
return apps::RequirementFromString(parent_requirement_string.get());
}
// Ensure that the parent process is Chromium.
// This prevents this tool from being used to bypass binary authorization tools
// such as Santa.
//
// Returns whether the parent process's code signature is trusted:
// - True if the framework bundle is unsigned (there's nothing to verify).
// - True if the parent process satisfies the constructed designated requirement
// tailored for the parent app based on the framework bundle's requirement.
// - False otherwise.
bool ValidateParentProcess(std::string_view info_plist_xml) {
base::expected<base::apple::ScopedCFTypeRef<SecRequirementRef>,
apps::MissingRequirementReason>
parent_app_requirement = CreateParentAppRequirement();
if (!parent_app_requirement.has_value()) {
switch (parent_app_requirement.error()) {
case apps::MissingRequirementReason::NoOrAdHocSignature:
// Parent validation is not required because framework bundle is not
// code-signed or is ad-hoc code-signed.
return true;
case apps::MissingRequirementReason::Error:
// Framework bundle is code-signed however we were unable to create the
// parent app requirement. Deny.
// CreateParentAppRequirement already did the
// base::debug::DumpWithoutCrashing, possibly on a previous call. We can
// return false here without any additional explanation.
return false;
}
}
// Perform dynamic validation only as Chrome.app's dynamic signature may not
// match its on-disk signature if there is an update pending.
OSStatus status = base::mac::ProcessIdIsSignedAndFulfillsRequirement_DoNotUse(
getppid(), parent_app_requirement.value().get(),
base::mac::SignatureValidationType::DynamicOnly, info_plist_xml);
return status == errSecSuccess;
}
} // namespace
extern "C" {
// The entry point into the shortcut copier process. This is not
// a user API.
__attribute__((visibility("default"))) int ChromeWebAppShortcutCopierMain(
int argc,
char** argv);
}
// Copies files from argv[1] to argv[2]
//
// When using ad-hoc signing for web app shims, the final app shim must be
// written to disk by this helper tool. This separate helper tool exists so that
// binary authorization tools, such as Santa, can transitively trust app shims
// that it creates without trusting all files written by Chrome. This allows app
// shims to be trusted by the binary authorization tool despite having only
// ad-hoc code signatures.
//
// argv[3] is the Info.plist contents of Chrome. This is needed to validate the
// dynamic code signature of the running application as the Info.plist file on
// disk may have changed if there is an update pending. The passed-in data is
// validated against a hash recorded in the code signature before being used
// during requirement validation.
int ChromeWebAppShortcutCopierMain(int argc, char** argv) {
if (argc != 4) {
return 1;
}
// Override the path to the framework value so that it has a sensible value.
// This tool lives within the Helpers subdirectory of the framework, so the
// versioned path is two levels upwards.
base::FilePath executable_path =
base::PathService::CheckedGet(base::FILE_EXE);
base::apple::SetOverrideFrameworkBundlePath(
executable_path.DirName().DirName());
if (!ValidateParentProcess(argv[3])) {
return 1;
}
base::FilePath staging_path = base::FilePath::FromUTF8Unsafe(argv[1]);
base::FilePath dst_app_path = base::FilePath::FromUTF8Unsafe(argv[2]);
if (!base::CopyDirectory(staging_path, dst_app_path, true)) {
LOG(ERROR) << "Copying app from " << staging_path << " to " << dst_app_path
<< " failed.";
return 2;
}
return 0;
}

@ -30,6 +30,7 @@
#include "base/functional/bind.h"
#include "base/functional/callback.h"
#include "base/logging.h"
#include "base/mac/info_plist_data.h"
#include "base/mac/mac_util.h"
#include "base/metrics/histogram_functions.h"
#include "base/process/launch.h"
@ -38,7 +39,6 @@
#include "base/synchronization/lock.h"
#include "base/version_info/version_info.h"
#include "chrome/browser/shortcuts/platform_util_mac.h"
#include "chrome/browser/web_applications/mojom/web_app_shortcut_copier.mojom.h"
#include "chrome/browser/web_applications/os_integration/mac/bundle_info_plist.h"
#include "chrome/browser/web_applications/os_integration/mac/icns_encoder.h"
#include "chrome/browser/web_applications/os_integration/mac/icon_utils.h"
@ -46,13 +46,8 @@
#include "chrome/browser/web_applications/os_integration/mac/web_app_shortcut_mac.h"
#include "chrome/browser/web_applications/os_integration/os_integration_test_override.h"
#import "chrome/common/mac/app_mode_common.h"
#include "components/variations/active_field_trials.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/common/content_switches.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "mojo/public/cpp/bindings/sync_call_restrictions.h"
#include "mojo/public/cpp/platform/platform_channel.h"
#include "mojo/public/cpp/system/invitation.h"
#if defined(COMPONENT_BUILD)
#include <mach-o/loader.h>
@ -90,10 +85,6 @@ BASE_FEATURE(kWebAppMaskableIconsOnMac,
"WebAppMaskableIconsOnMac",
base::FEATURE_ENABLED_BY_DEFAULT);
class WebAppShortcutCopierSyncCallHelper {
mojo::SyncCallRestrictions::ScopedAllowSyncCall scoped_allow_;
};
namespace {
// Result of creating app shortcut.
@ -252,43 +243,27 @@ bool CopyStagingBundleToDestination(bool use_ad_hoc_signing_for_web_app_shims,
base::apple::FrameworkBundlePath().Append("Helpers").Append(
"web_app_shortcut_copier");
base::CommandLine command_line(web_app_shortcut_copier_path);
base::LaunchOptions options;
mojo::PlatformChannel channel;
channel.PrepareToPassRemoteEndpoint(&options, &command_line);
command_line.AppendArgPath(staging_path);
command_line.AppendArgPath(dst_app_path);
// Ensure that the helper tool sees the same feature state as the browser.
variations::PopulateLaunchOptionsWithVariationsInfo(&command_line, &options);
// Pass NSBundle's cached copy of the app's Info.plist data to the helper tool
// for use in dynamic signature validation. The data is validated against a
// hash recorded in the code signature before being used during requirement
// validation.
std::vector<uint8_t> info_plist_data =
base::mac::OuterBundleCachedInfoPlistData();
command_line.AppendArg(base::as_string_view(info_plist_data));
base::Process copier_process = base::LaunchProcess(command_line, options);
if (!copier_process.IsValid()) {
LOG(ERROR) << "Failed to launch web_app_shortcut_copier.";
return false;
}
channel.RemoteProcessLaunchAttempted();
mojo::ScopedMessagePipeHandle pipe = mojo::OutgoingInvitation::SendIsolated(
channel.TakeLocalEndpoint(), {}, copier_process.Handle());
if (!pipe) {
LOG(ERROR) << "Failed to send Mojo invitation to web_app_shortcut_copier.";
return false;
}
mojo::PendingRemote<mojom::WebAppShortcutCopier> pending_remote(
std::move(pipe), mojom::WebAppShortcutCopier::Version_);
if (!pending_remote) {
LOG(ERROR)
<< "Failed to establish Mojo connection with web_app_shortcut_copier.";
return false;
// Synchronously wait for the copy to complete to match the semantics of
// `base::CopyDirectory`.
std::string command_output;
int exit_code;
if (base::GetAppOutputWithExitCode(command_line, &command_output,
&exit_code)) {
return !exit_code;
}
mojo::Remote<mojom::WebAppShortcutCopier> copier(std::move(pending_remote));
WebAppShortcutCopierSyncCallHelper sync_calls_allowed;
bool copy_result = false;
if (!copier->CopyWebAppShortcut(staging_path, dst_app_path, &copy_result)) {
LOG(ERROR)
<< "Failed to call CopyWebAppShortcut in web_app_shortcut_copier.";
return false;
}
return copy_result;
return false;
}
// Remove the leading . from the entries of |extensions|. Any items that do not

@ -48,12 +48,6 @@ class HostFrameSinkManager;
class HostGpuMemoryBufferManager;
} // namespace viz
#if BUILDFLAG(IS_MAC)
namespace web_app {
class WebAppShortcutCopierSyncCallHelper;
} // namespace web_app
#endif
namespace mojo {
class ScopedAllowSyncCallForTesting;
@ -140,9 +134,6 @@ class COMPONENT_EXPORT(MOJO_CPP_BINDINGS) SyncCallRestrictions {
friend class content::StreamTextureFactory;
#if BUILDFLAG(IS_WIN)
friend class content::DCOMPTextureFactory;
#endif
#if BUILDFLAG(IS_MAC)
friend class web_app::WebAppShortcutCopierSyncCallHelper;
#endif
// END ALLOWED USAGE.