From 320bea4c287a04287c798cf177be7c657b672564 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Thu, 5 Aug 2021 10:50:11 -0700 Subject: [PATCH] feat: add fuses for NODE_OPTIONS and --inspect (#30190) * feat: add fuses for NODE_OPTIONS and --inspect * chore: add node patch to ensure NODE_OPTIONS are never parsed when fuse is disabledd * chore: fix lint * chore: flip boolean logic * chore: update patches * chore: add trailing _ to static member * Update add_should_read_node_options_from_env_option_to_disable_node_options.patch * chore: update patches Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com> --- build/fuses/fuses.json5 | 4 +- patches/node/.patches | 1 + ...m_env_option_to_disable_node_options.patch | 68 ++++++++++++++++ shell/common/node_bindings.cc | 80 ++++++++++++------- 4 files changed, 121 insertions(+), 32 deletions(-) create mode 100644 patches/node/add_should_read_node_options_from_env_option_to_disable_node_options.patch diff --git a/build/fuses/fuses.json5 b/build/fuses/fuses.json5 index dfc518ffa3..bb5ec6c1f2 100644 --- a/build/fuses/fuses.json5 +++ b/build/fuses/fuses.json5 @@ -3,5 +3,7 @@ "_schema": "0 == off, 1 == on, r == removed fuse", "_version": 1, "run_as_node": "1", - "cookie_encryption": "0" + "cookie_encryption": "0", + "node_options": "1", + "node_cli_inspect": "1" } diff --git a/patches/node/.patches b/patches/node/.patches index ac583a1bca..f401653119 100644 --- a/patches/node/.patches +++ b/patches/node/.patches @@ -25,3 +25,4 @@ src_allow_embedders_to_provide_a_custom_pageallocator_to.patch fix_crypto_tests_to_run_with_bssl.patch fix_account_for_debugger_agent_race_condition.patch fix_use_new_v8_error_message_property_access_format.patch +add_should_read_node_options_from_env_option_to_disable_node_options.patch diff --git a/patches/node/add_should_read_node_options_from_env_option_to_disable_node_options.patch b/patches/node/add_should_read_node_options_from_env_option_to_disable_node_options.patch new file mode 100644 index 0000000000..45fc669dc5 --- /dev/null +++ b/patches/node/add_should_read_node_options_from_env_option_to_disable_node_options.patch @@ -0,0 +1,68 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Samuel Attard +Date: Wed, 21 Jul 2021 13:40:59 -0700 +Subject: add should_read_node_options_from_env option to disable NODE_OPTIONS + parsing at runtime + +We can remove the NODE_OPTIONS environment variable but it in theory could be injected / re-inserted at runtime and be used for workers. In order to ensure the fuse is respected we need a hard runtime toggle for NODE_OPTION support. + +diff --git a/src/env.cc b/src/env.cc +index 1cc7da1ce15f43905ce607adcc1a23ed9d92948a..16af6aec3791df1363682f1ed024c52208b9a8f6 100644 +--- a/src/env.cc ++++ b/src/env.cc +@@ -329,6 +329,9 @@ std::string GetExecPath(const std::vector& argv) { + return exec_path; + } + ++/* static */ ++bool Environment::should_read_node_options_from_env_ = true; ++ + Environment::Environment(IsolateData* isolate_data, + Isolate* isolate, + const std::vector& args, +diff --git a/src/env.h b/src/env.h +index 45210f074a0ca4d57f9fdc5019e8e82540b28b72..c0da6c53028bc9459065caf25c1221f556b22d68 100644 +--- a/src/env.h ++++ b/src/env.h +@@ -1143,6 +1143,8 @@ class Environment : public MemoryRetainer { + inline double trigger_async_id(); + inline double get_default_trigger_async_id(); + ++ static bool should_read_node_options_from_env_; ++ + // List of id's that have been destroyed and need the destroy() cb called. + inline std::vector* destroy_async_id_list(); + +diff --git a/src/node.cc b/src/node.cc +index 6302bb925339d709a54151a8fc8b58f1a2253881..48a9eedfaf6350fc05fb104a1f44e85b75878f9a 100644 +--- a/src/node.cc ++++ b/src/node.cc +@@ -882,7 +882,7 @@ int InitializeNodeWithArgs(std::vector* argv, + #if !defined(NODE_WITHOUT_NODE_OPTIONS) + std::string node_options; + +- if (credentials::SafeGetenv("NODE_OPTIONS", &node_options)) { ++ if (Environment::should_read_node_options_from_env_ && credentials::SafeGetenv("NODE_OPTIONS", &node_options)) { + std::vector env_argv = + ParseNodeOptionsEnvVar(node_options, errors); + +diff --git a/src/node_worker.cc b/src/node_worker.cc +index 43a3862cc69dc39b95b329f713691b19de3b8d7e..7782e8488f797a8cf83b1e02f012797ef256afe7 100644 +--- a/src/node_worker.cc ++++ b/src/node_worker.cc +@@ -457,6 +457,7 @@ void Worker::New(const FunctionCallbackInfo& args) { + }); + + #ifndef NODE_WITHOUT_NODE_OPTIONS ++ if (Environment::should_read_node_options_from_env_) { + MaybeLocal maybe_node_opts = + env_vars->Get(isolate, OneByteString(isolate, "NODE_OPTIONS")); + Local node_opts; +@@ -487,6 +488,7 @@ void Worker::New(const FunctionCallbackInfo& args) { + return; + } + } ++ } + #endif + } + diff --git a/shell/common/node_bindings.cc b/shell/common/node_bindings.cc index 6ff87e7962..96123651a6 100644 --- a/shell/common/node_bindings.cc +++ b/shell/common/node_bindings.cc @@ -25,6 +25,7 @@ #include "content/public/browser/browser_thread.h" #include "content/public/common/content_paths.h" #include "electron/buildflags/buildflags.h" +#include "electron/fuses.h" #include "shell/browser/api/electron_api_app.h" #include "shell/common/api/electron_bindings.h" #include "shell/common/electron_command_line.h" @@ -188,16 +189,26 @@ void ErrorMessageListener(v8::Local message, } } +const std::unordered_set +GetAllowedDebugOptions() { + if (electron::fuses::IsNodeCliInspectEnabled()) { + // Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode + return { + "--inspect", "--inspect-brk", + "--inspect-port", "--debug", + "--debug-brk", "--debug-port", + "--inspect-brk-node", "--inspect-publish-uid", + }; + } + // If node CLI inspect support is disabled, allow no debug options. + return {}; +} + // Initialize Node.js cli options to pass to Node.js // See https://nodejs.org/api/cli.html#cli_options void SetNodeCliFlags() { - // Only allow DebugOptions in non-ELECTRON_RUN_AS_NODE mode - const std::unordered_set allowed = { - "--inspect", "--inspect-brk", - "--inspect-port", "--debug", - "--debug-brk", "--debug-port", - "--inspect-brk-node", "--inspect-publish-uid", - }; + const std::unordered_set allowed = + GetAllowedDebugOptions(); const auto argv = base::CommandLine::ForCurrentProcess()->argv(); std::vector args; @@ -231,7 +242,7 @@ void SetNodeCliFlags() { } else if (!errors.empty()) { LOG(ERROR) << err_str << base::JoinString(errors, " "); } -} // namespace +} // Initialize NODE_OPTIONS to pass to Node.js // See https://nodejs.org/api/cli.html#cli_node_options_options @@ -246,34 +257,39 @@ void SetNodeOptions(base::Environment* env) { "--http-parser"}; if (env->HasVar("NODE_OPTIONS")) { - std::string options; - env->GetVar("NODE_OPTIONS", &options); - std::vector parts = base::SplitString( - options, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); + if (electron::fuses::IsNodeOptionsEnabled()) { + std::string options; + env->GetVar("NODE_OPTIONS", &options); + std::vector parts = base::SplitString( + options, " ", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); - bool is_packaged_app = electron::api::App::IsPackaged(); + bool is_packaged_app = electron::api::App::IsPackaged(); - for (const auto& part : parts) { - // Strip off values passed to individual NODE_OPTIONs - std::string option = part.substr(0, part.find('=')); + for (const auto& part : parts) { + // Strip off values passed to individual NODE_OPTIONs + std::string option = part.substr(0, part.find('=')); - if (is_packaged_app && - allowed_in_packaged.find(option) == allowed_in_packaged.end()) { - // Explicitly disallow majority of NODE_OPTIONS in packaged apps - LOG(ERROR) << "Most NODE_OPTIONs are not supported in packaged apps." - << " See documentation for more details."; - options.erase(options.find(option), part.length()); - } else if (disallowed.find(option) != disallowed.end()) { - // Remove NODE_OPTIONS specifically disallowed for use in Node.js - // through Electron owing to constraints like BoringSSL. - LOG(ERROR) << "The NODE_OPTION " << option - << " is not supported in Electron"; - options.erase(options.find(option), part.length()); + if (is_packaged_app && + allowed_in_packaged.find(option) == allowed_in_packaged.end()) { + // Explicitly disallow majority of NODE_OPTIONS in packaged apps + LOG(ERROR) << "Most NODE_OPTIONs are not supported in packaged apps." + << " See documentation for more details."; + options.erase(options.find(option), part.length()); + } else if (disallowed.find(option) != disallowed.end()) { + // Remove NODE_OPTIONS specifically disallowed for use in Node.js + // through Electron owing to constraints like BoringSSL. + LOG(ERROR) << "The NODE_OPTION " << option + << " is not supported in Electron"; + options.erase(options.find(option), part.length()); + } } - } - // overwrite new NODE_OPTIONS without unsupported variables - env->SetVar("NODE_OPTIONS", options); + // overwrite new NODE_OPTIONS without unsupported variables + env->SetVar("NODE_OPTIONS", options); + } else { + LOG(ERROR) << "NODE_OPTIONS have been disabled in this app"; + env->SetVar("NODE_OPTIONS", ""); + } } } @@ -364,6 +380,8 @@ void NodeBindings::Initialize() { auto env = base::Environment::Create(); SetNodeOptions(env.get()); + node::Environment::should_read_node_options_from_env_ = + fuses::IsNodeOptionsEnabled(); std::vector argv = {"electron"}; std::vector exec_argv;