Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add experimental cookie encryption support #29493

Merged
merged 2 commits into from Jun 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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;
}