Skip to content

Commit

Permalink
feat: add experimental cookie encryption support (#29493)
Browse files Browse the repository at this point in the history
* feat: add experimental cookie encryption support (#27524)

* feat: add experimental cookie encryption support on macOS

* chore: fix TODO

* update patches

* feat: make cookie encryption work on windows

* chore: update cookie encryption support comments

* fix: only call OSCrypt::Init on windows

* chore: make cookie encryption work on linux

* Update shell/browser/net/system_network_context_manager.cc

Co-authored-by: Jeremy Rose <jeremya@chromium.org>

* chore: fix lint

* chore: update patches

* chore: update patches to upstreamed variants

* chore: use chrome ::switches constants

* chore: remove bad patch

* build: disable cookie encryption by default

* chore: update patches

* fix: provide std::string to NoDestructor

* chore: fix macos, nodestructor syntax

* build: fix macOS build due to mismatch in DEFINE

Co-authored-by: Electron Bot <electron@github.com>
Co-authored-by: Jeremy Rose <jeremya@chromium.org>

* chore: update patches

Co-authored-by: Electron Bot <electron@github.com>
Co-authored-by: Jeremy Rose <jeremya@chromium.org>
  • Loading branch information
3 people committed Jun 3, 2021
1 parent 6e16908 commit 2156a90
Show file tree
Hide file tree
Showing 12 changed files with 524 additions and 11 deletions.
8 changes: 7 additions & 1 deletion BUILD.gn
@@ -1,6 +1,7 @@
import("//build/config/locales.gni")
import("//build/config/ui.gni")
import("//build/config/win/manifest.gni")
import("//components/os_crypt/features.gni")
import("//components/spellcheck/spellcheck_build_features.gni")
import("//content/public/app/mac_helpers.gni")
import("//extensions/buildflags/buildflags.gni")
Expand Down Expand Up @@ -292,7 +293,7 @@ templated_file("electron_version_header") {
action("electron_fuses") {
script = "build/fuses/build.py"

inputs = [ "build/fuses/fuses.json" ]
inputs = [ "build/fuses/fuses.json5" ]

outputs = [
"$target_gen_dir/fuses.h",
Expand Down Expand Up @@ -331,6 +332,7 @@ source_set("electron_lib") {
"//components/network_hints/common:mojo_bindings",
"//components/network_hints/renderer",
"//components/network_session_configurator/common",
"//components/os_crypt",
"//components/pref_registry",
"//components/prefs",
"//components/upload_list",
Expand Down Expand Up @@ -678,6 +680,10 @@ source_set("electron_lib") {
]
libs += [ "uxtheme.lib" ]
}

if (allow_runtime_configurable_key_storage) {
defines += [ "ALLOW_RUNTIME_CONFIGURABLE_KEY_STORAGE" ]
}
}

electron_paks("packed_resources") {
Expand Down
3 changes: 3 additions & 0 deletions build/args/all.gn
Expand Up @@ -28,3 +28,6 @@ libcxx_abi_unstable = false
enable_pseudolocales = false

is_cfi = false

# Make application name configurable at runtime for cookie crypto
allow_runtime_configurable_key_storage = true
4 changes: 2 additions & 2 deletions build/fuses/build.py
Expand Up @@ -49,8 +49,8 @@
}
"""

with open(os.path.join(dir_path, "fuses.json"), 'r') as f:
fuse_defaults = json.load(f)
with open(os.path.join(dir_path, "fuses.json5"), 'r') as f:
fuse_defaults = json.loads(''.join(line for line in f.readlines() if not line.strip()[0] == "/"))

fuse_version = fuse_defaults['_version']
del fuse_defaults['_version']
Expand Down
3 changes: 2 additions & 1 deletion build/fuses/fuses.json → build/fuses/fuses.json5
Expand Up @@ -2,5 +2,6 @@
"_comment": "Modifying the fuse schema in any breaking way should result in the _version prop being incremented. NEVER remove a fuse or change its meaning, instead mark it as removed with 'r'",
"_schema": "0 == off, 1 == on, r == removed fuse",
"_version": 1,
"run_as_node": "1"
"run_as_node": "1",
"cookie_encryption": "0"
}
2 changes: 1 addition & 1 deletion docs/tutorial/fuses.md
Expand Up @@ -51,4 +51,4 @@ Somewhere in the Electron binary there will be a sequence of bytes that look lik

To flip a fuse you find its position in the fuse wire and change it to "0" or "1" depending on the state you'd like.

You can view the current schema [here](https://github.com/electron/electron/blob/master/build/fuses/fuses.json).
You can view the current schema [here](https://github.com/electron/electron/blob/master/build/fuses/fuses.json5).
2 changes: 2 additions & 0 deletions patches/chromium/.patches
Expand Up @@ -110,3 +110,5 @@ add_setter_for_browsermainloop_result_code.patch
cherry-pick-8089dbfc616f.patch
x11_fix_window_enumeration_order_when_wm_doesn_t_set.patch
build_libc_as_static_library.patch
support_runtime_configurable_key_storage_on_linux_os_crypto.patch
make_keychain_service_account_optionally_configurable_at_runtime.patch
@@ -0,0 +1,132 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Samuel Attard <samuel.r.attard@gmail.com>
Date: Fri, 7 May 2021 00:35:57 +0000
Subject: Make keychain service/account optionally configurable at runtime

This flag allows embedders to customize the service/account_name used
for cookie crypto at runtime. Currently these values are hardcoded
to Chromium/Chrome and the only way to change them is to patch this
file as part of the build process. Making these non-const and
assignable allows embedders to easily avoid colliding with the
"Chrome Safe Storage" keychain value. The const vs non-const change
is done behind a build flag so that the normal Chrome and Chromium
builds are unaffected.

I intend to follow this up with a similar CL for changes to the
linux crypto files too.

Change-Id: Id2f9456a8dfc71a004a2dd405bed46518c559ac4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2861286
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Jeremy Rose <jeremya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#880168}

diff --git a/components/os_crypt/BUILD.gn b/components/os_crypt/BUILD.gn
index 79f9744a94d79e155958ee28e93eeb2b4e65d112..b9c1cf2bd914812697b161217e09ea693d2c6e65 100644
--- a/components/os_crypt/BUILD.gn
+++ b/components/os_crypt/BUILD.gn
@@ -51,6 +51,10 @@ component("os_crypt") {

defines = [ "IS_OS_CRYPT_IMPL" ]

+ if (allow_runtime_configurable_key_storage) {
+ defines += [ "ALLOW_RUNTIME_CONFIGURABLE_KEY_STORAGE" ]
+ }
+
if ((is_posix || is_fuchsia) && !is_apple &&
(!(is_linux || is_chromeos_lacros) || is_chromecast)) {
sources += [ "os_crypt_posix.cc" ]
diff --git a/components/os_crypt/features.gni b/components/os_crypt/features.gni
index a145e0530d3cdc2a04d471f98d72c3dd30e437d9..73b6e8703298b342ba94b977c9b98da0e1739bbd 100644
--- a/components/os_crypt/features.gni
+++ b/components/os_crypt/features.gni
@@ -9,4 +9,10 @@ declare_args() {
# Whether to use libgnome-keyring (deprecated by libsecret).
# See http://crbug.com/466975 and http://crbug.com/355223.
use_gnome_keyring = (is_linux || is_chromeos_lacros) && use_glib
+
+ # Whether to make account and service names for the crypto key storage
+ # configurable at runtime for embedders.
+ #
+ # Currently only has an effect on macOS via KeychainPassword
+ allow_runtime_configurable_key_storage = false
}
diff --git a/components/os_crypt/keychain_password_mac.h b/components/os_crypt/keychain_password_mac.h
index 6fda0244667c4eb5d1abb973f4b72d9c59ed2165..40b2522b87912124c106a7c28853399d31238b81 100644
--- a/components/os_crypt/keychain_password_mac.h
+++ b/components/os_crypt/keychain_password_mac.h
@@ -9,6 +9,7 @@

#include "base/component_export.h"
#include "base/macros.h"
+#include "base/no_destructor.h"

namespace crypto {
class AppleKeychain;
@@ -16,6 +17,12 @@ class AppleKeychain;

class COMPONENT_EXPORT(OS_CRYPT) KeychainPassword {
public:
+#if defined(ALLOW_RUNTIME_CONFIGURABLE_KEY_STORAGE)
+ using KeychainNameType = base::NoDestructor<std::string>;
+#else
+ using KeychainNameType = const base::NoDestructor<std::string>;
+#endif
+
KeychainPassword(const crypto::AppleKeychain& keychain);
~KeychainPassword();

@@ -28,8 +35,8 @@ class COMPONENT_EXPORT(OS_CRYPT) KeychainPassword {
std::string GetPassword() const;

// The service and account names used in Chrome's Safe Storage keychain item.
- static COMPONENT_EXPORT(OS_CRYPT) const char service_name[];
- static COMPONENT_EXPORT(OS_CRYPT) const char account_name[];
+ static COMPONENT_EXPORT(OS_CRYPT) KeychainNameType service_name;
+ static COMPONENT_EXPORT(OS_CRYPT) KeychainNameType account_name;

private:
const crypto::AppleKeychain& keychain_;
diff --git a/components/os_crypt/keychain_password_mac.mm b/components/os_crypt/keychain_password_mac.mm
index 6654c46eb0af784a3a2ff64569d5d1931b9fae30..f8973f5ed0e213c0d242d2141091607017824ec3 100644
--- a/components/os_crypt/keychain_password_mac.mm
+++ b/components/os_crypt/keychain_password_mac.mm
@@ -48,11 +48,11 @@
// the encryption keyword. So as to not lose encrypted data when system
// locale changes we DO NOT LOCALIZE.
#if BUILDFLAG(GOOGLE_CHROME_BRANDING)
-const char KeychainPassword::service_name[] = "Chrome Safe Storage";
-const char KeychainPassword::account_name[] = "Chrome";
+KeychainPassword::KeychainNameType KeychainPassword::service_name("Chrome Safe Storage");
+KeychainPassword::KeychainNameType KeychainPassword::account_name("Chrome");
#else
-const char KeychainPassword::service_name[] = "Chromium Safe Storage";
-const char KeychainPassword::account_name[] = "Chromium";
+KeychainPassword::KeychainNameType KeychainPassword::service_name("Chromium Safe Storage");
+KeychainPassword::KeychainNameType KeychainPassword::account_name("Chromium");
#endif

KeychainPassword::KeychainPassword(const AppleKeychain& keychain)
@@ -64,8 +64,9 @@
UInt32 password_length = 0;
void* password_data = nullptr;
OSStatus error = keychain_.FindGenericPassword(
- strlen(service_name), service_name, strlen(account_name), account_name,
- &password_length, &password_data, nullptr);
+ service_name->size(), service_name->c_str(),
+ account_name->size(), account_name->c_str(), &password_length,
+ &password_data, nullptr);

if (error == noErr) {
std::string password =
@@ -75,8 +76,8 @@
}

if (error == errSecItemNotFound) {
- std::string password =
- AddRandomPasswordToKeychain(keychain_, service_name, account_name);
+ std::string password = AddRandomPasswordToKeychain(
+ keychain_, *service_name, *account_name);
return password;
}

0 comments on commit 2156a90

Please sign in to comment.