Disallow launching unknown apps via browser client.

CVE-2018-1000006

atom/app/atom_library_main.h

@@ -10,7 +10,7 @@
 #if defined(OS_MACOSX)
 extern "C" {
 __attribute__((visibility("default")))
-int AtomMain(int argc, const char* argv[]);
+int AtomMain(int argc, char* argv[]);
 
 __attribute__((visibility("default")))
 int AtomInitializeICUandStartNode(int argc, char *argv[]);

atom/app/atom_library_main.mm

@@ -15,11 +15,11 @@
 #include "content/public/app/content_main.h"
 
 #if defined(OS_MACOSX)
-int AtomMain(int argc, const char* argv[]) {
+int AtomMain(int argc, char* argv[]) {
   atom::AtomMainDelegate delegate;
   content::ContentMainParams params(&delegate);
   params.argc = argc;
-  params.argv = argv;
+  params.argv = const_cast<const char**>(argv);
   atom::AtomCommandLine::Init(argc, argv);
   return content::ContentMain(params);
 }

atom/app/atom_main.cc

@@ -4,7 +4,8 @@
 
 #include "atom/app/atom_main.h"
 
-#include <stdlib.h>
+#include <cstdlib>
+#include <vector>
 
 #if defined(OS_WIN)
 #include <windows.h>  // windows.h must be included first
@@ -15,9 +16,11 @@
 #include <tchar.h>
 
 #include "atom/app/atom_main_delegate.h"
+#include "atom/app/command_line_args.h"
 #include "atom/common/crash_reporter/win/crash_service_main.h"
 #include "base/environment.h"
 #include "base/process/launch.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/win/windows_version.h"
 #include "content/public/app/sandbox_helper_win.h"
 #include "sandbox/win/src/sandbox_types.h"
@@ -52,18 +55,23 @@ bool IsEnvSet(const char* name) {
 
 #if defined(OS_WIN)
 int APIENTRY wWinMain(HINSTANCE instance, HINSTANCE, wchar_t* cmd, int) {
-  int argc = 0;
+  struct Arguments {
-  wchar_t** wargv = ::CommandLineToArgvW(::GetCommandLineW(), &argc);
+    int argc = 0;
+    wchar_t** argv = ::CommandLineToArgvW(::GetCommandLineW(), &argc);
 
-  bool run_as_node = IsEnvSet(kRunAsNode);
+    ~Arguments() { LocalFree(argv); }
+  } arguments;
+
+  if (!arguments.argv)
+    return -1;
 
 #ifdef _DEBUG
   // Don't display assert dialog boxes in CI test runs
   static const auto kCI = "ELECTRON_CI";
   bool is_ci = IsEnvSet(kCI);
   if (!is_ci) {
-    for (int i = 0; i < argc; ++i) {
+    for (int i = 0; i < arguments.argc; ++i) {
-      if (!_wcsicmp(wargv[i], L"--ci")) {
+      if (!_wcsicmp(arguments.argv[i], L"--ci")) {
         is_ci = true;
         _putenv_s(kCI, "1");  // set flag for child processes
         break;
@@ -81,44 +89,12 @@ int APIENTRY wWinMain(HINSTANCE instance, HINSTANCE, wchar_t* cmd, int) {
   }
 #endif
 
+  bool run_as_node = IsEnvSet(kRunAsNode);
+
   // Make sure the output is printed to console.
   if (run_as_node || !IsEnvSet("ELECTRON_NO_ATTACH_CONSOLE"))
     base::RouteStdioToConsole(false);
 
-  // Convert argv to to UTF8
-  char** argv = new char*[argc];
-  for (int i = 0; i < argc; i++) {
-    // Compute the size of the required buffer
-    DWORD size = WideCharToMultiByte(CP_UTF8,
-                                     0,
-                                     wargv[i],
-                                     -1,
-                                     NULL,
-                                     0,
-                                     NULL,
-                                     NULL);
-    if (size == 0) {
-      // This should never happen.
-      fprintf(stderr, "Could not convert arguments to utf8.");
-      exit(1);
-    }
-    // Do the actual conversion
-    argv[i] = new char[size];
-    DWORD result = WideCharToMultiByte(CP_UTF8,
-                                       0,
-                                       wargv[i],
-                                       -1,
-                                       argv[i],
-                                       size,
-                                       NULL,
-                                       NULL);
-    if (result == 0) {
-      // This should never happen.
-      fprintf(stderr, "Could not convert arguments to utf8.");
-      exit(1);
-    }
-  }
-
 #ifndef DEBUG
   // Chromium has its own TLS subsystem which supports automatic destruction
   // of thread-local data, and also depends on memory allocation routines
@@ -139,14 +115,23 @@ int APIENTRY wWinMain(HINSTANCE instance, HINSTANCE, wchar_t* cmd, int) {
 #endif
 
   if (run_as_node) {
-    // Now that argv conversion is done, we can finally start.
+    std::vector<char*> argv(arguments.argc);
+    std::transform(
+        arguments.argv, arguments.argv + arguments.argc, argv.begin(),
+        [](auto& a) { return _strdup(base::WideToUTF8(a).c_str()); });
+
     base::AtExitManager atexit_manager;
     base::i18n::InitializeICU();
-    return atom::NodeMain(argc, argv);
+    auto ret = atom::NodeMain(argv.size(), argv.data());
+    std::for_each(argv.begin(), argv.end(), free);
+    return ret;
   } else if (IsEnvSet("ELECTRON_INTERNAL_CRASH_SERVICE")) {
     return crash_service::Main(cmd);
   }
 
+  if (!atom::CheckCommandLineArguments(arguments.argc, arguments.argv))
+    return -1;
+
   sandbox::SandboxInterfaceInfo sandbox_info = {0};
   content::InitializeSandboxInfo(&sandbox_info);
   atom::AtomMainDelegate delegate;
@@ -154,33 +139,32 @@ int APIENTRY wWinMain(HINSTANCE instance, HINSTANCE, wchar_t* cmd, int) {
   content::ContentMainParams params(&delegate);
   params.instance = instance;
   params.sandbox_info = &sandbox_info;
-  atom::AtomCommandLine::Init(argc, argv);
+  atom::AtomCommandLine::Init(arguments.argc, arguments.argv);
-  atom::AtomCommandLine::InitW(argc, wargv);
   return content::ContentMain(params);
 }
 
 #elif defined(OS_LINUX)  // defined(OS_WIN)
 
-int main(int argc, const char* argv[]) {
+int main(int argc, char* argv[]) {
   if (IsEnvSet(kRunAsNode)) {
     base::i18n::InitializeICU();
     base::AtExitManager atexit_manager;
-    return atom::NodeMain(argc, const_cast<char**>(argv));
+    return atom::NodeMain(argc, argv);
   }
 
   atom::AtomMainDelegate delegate;
   content::ContentMainParams params(&delegate);
   params.argc = argc;
-  params.argv = argv;
+  params.argv = const_cast<const char**>(argv);
   atom::AtomCommandLine::Init(argc, argv);
   return content::ContentMain(params);
 }
 
 #else  // defined(OS_LINUX)
 
-int main(int argc, const char* argv[]) {
+int main(int argc, char* argv[]) {
   if (IsEnvSet(kRunAsNode)) {
-    return AtomInitializeICUandStartNode(argc, const_cast<char**>(argv));
+    return AtomInitializeICUandStartNode(argc, argv);
   }
 
   return AtomMain(argc, argv);

atom/app/command_line_args.h

@@ -0,0 +1,17 @@
+// Copyright (c) 2018 GitHub, Inc.
+// Use of this source code is governed by the MIT license that can be
+// found in the LICENSE file.
+
+#ifndef ATOM_APP_COMMAND_LINE_ARGS_H_
+#define ATOM_APP_COMMAND_LINE_ARGS_H_
+
+#include "base/command_line.h"
+
+namespace atom {
+
+bool CheckCommandLineArguments(int argc, base::CommandLine::CharType** argv);
+
+}  // namespace atom
+
+#endif  // ATOM_APP_COMMAND_LINE_ARGS_H_
+

atom/browser/api/atom_api_app.cc

@@ -897,11 +897,7 @@ bool App::Relaunch(mate::Arguments* js_args) {
   }
 
   if (!override_argv) {
-#if defined(OS_WIN)
-    const relauncher::StringVector& argv = atom::AtomCommandLine::wargv();
-#else
     const relauncher::StringVector& argv = atom::AtomCommandLine::argv();
-#endif
     return relauncher::RelaunchApp(argv);
   }
 

atom/browser/atom_browser_client.cc

@@ -38,6 +38,7 @@
 #include "content/public/browser/resource_dispatcher_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/content_paths.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/resource_request_body.h"
 #include "content/public/common/url_constants.h"
@@ -237,6 +238,11 @@ void AtomBrowserClient::OverrideSiteInstanceForNavigation(
 void AtomBrowserClient::AppendExtraCommandLineSwitches(
     base::CommandLine* command_line,
     int process_id) {
+  // Make sure we're about to launch a known executable
+  base::FilePath child_path;
+  PathService::Get(content::CHILD_PROCESS_EXE, &child_path);
+  CHECK(base::MakeAbsoluteFilePath(command_line->GetProgram()) == child_path);
+
   std::string process_type =
       command_line->GetSwitchValueASCII(::switches::kProcessType);
   if (process_type != ::switches::kRendererProcess)

atom/browser/relauncher.cc

@@ -140,11 +140,7 @@ bool RelaunchAppWithHelper(const base::FilePath& helper,
 }
 
 int RelauncherMain(const content::MainFunctionParams& main_parameters) {
-#if defined(OS_WIN)
-  const StringVector& argv = atom::AtomCommandLine::wargv();
-#else
   const StringVector& argv = atom::AtomCommandLine::argv();
-#endif
 
   if (argv.size() < 4 || argv[1] != internal::kRelauncherTypeArg) {
     LOG(ERROR) << "relauncher process invoked with unexpected arguments";

atom/common/atom_command_line.cc

@@ -10,31 +10,22 @@
 namespace atom {
 
 // static
-std::vector<std::string> AtomCommandLine::argv_;
+base::CommandLine::StringVector AtomCommandLine::argv_;
-
-#if defined(OS_WIN)
-// static
-std::vector<std::wstring> AtomCommandLine::wargv_;
-#endif
 
 // static
-void AtomCommandLine::Init(int argc, const char* const* argv) {
+void AtomCommandLine::Init(int argc, base::CommandLine::CharType** argv) {
+  DCHECK(argv_.empty());
+
+  // NOTE: uv_setup_args does nothing on Windows, so we don't need to call it.
+  // Otherwise we'd have to convert the arguments from UTF16.
+#if !defined(OS_WIN)
   // Hack around with the argv pointer. Used for process.title = "blah"
-  char** new_argv = uv_setup_args(argc, const_cast<char**>(argv));
+  argv = uv_setup_args(argc, argv);
-  for (int i = 0; i < argc; ++i) {
-    argv_.push_back(new_argv[i]);
-  }
-}
-
-#if defined(OS_WIN)
-// static
-void AtomCommandLine::InitW(int argc, const wchar_t* const* argv) {
-  for (int i = 0; i < argc; ++i) {
-    wargv_.push_back(argv[i]);
-  }
-}
 #endif
 
+  argv_.assign(argv, argv + argc);
+}
+
 #if defined(OS_LINUX)
 // static
 void AtomCommandLine::InitializeFromCommandLine() {

atom/common/atom_command_line.h

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "base/command_line.h"
 #include "base/macros.h"
 #include "build/build_config.h"
 
@@ -16,13 +17,9 @@ namespace atom {
 // Singleton to remember the original "argc" and "argv".
 class AtomCommandLine {
  public:
-  static void Init(int argc, const char* const* argv);
+  static const base::CommandLine::StringVector& argv() { return argv_; }
-  static std::vector<std::string> argv() { return argv_; }
 
-#if defined(OS_WIN)
+  static void Init(int argc, base::CommandLine::CharType** argv);
-  static void InitW(int argc, const wchar_t* const* argv);
-  static std::vector<std::wstring> wargv() { return wargv_; }
-#endif
 
 #if defined(OS_LINUX)
   // On Linux the command line has to be read from base::CommandLine since
@@ -31,11 +28,7 @@ class AtomCommandLine {
 #endif
 
  private:
-  static std::vector<std::string> argv_;
+  static base::CommandLine::StringVector argv_;
-
-#if defined(OS_WIN)
-  static std::vector<std::wstring> wargv_;
-#endif
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(AtomCommandLine);
 };

atom/common/node_bindings.cc

@@ -4,6 +4,7 @@
 
 #include "atom/common/node_bindings.h"
 
+#include <algorithm>
 #include <string>
 #include <vector>
 
@@ -17,6 +18,7 @@
 #include "base/files/file_path.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "content/public/browser/browser_thread.h"
@@ -179,7 +181,14 @@ void NodeBindings::Initialize() {
 
 node::Environment* NodeBindings::CreateEnvironment(
     v8::Handle<v8::Context> context) {
+#if defined(OS_WIN)
+  auto& atom_args = AtomCommandLine::argv();
+  std::vector<std::string> args(atom_args.size());
+  std::transform(atom_args.cbegin(), atom_args.cend(), args.begin(),
+                 [](auto& a) { return base::WideToUTF8(a); });
+#else
   auto args = AtomCommandLine::argv();
+#endif
 
   // Feed node the path to initialization script.
   base::FilePath::StringType process_type;
@@ -199,8 +208,7 @@ node::Environment* NodeBindings::CreateEnvironment(
       resources_path.Append(FILE_PATH_LITERAL("electron.asar"))
                     .Append(process_type)
                     .Append(FILE_PATH_LITERAL("init.js"));
-  std::string script_path_str = script_path.AsUTF8Unsafe();
+  args.insert(args.begin() + 1, script_path.AsUTF8Unsafe());
-  args.insert(args.begin() + 1, script_path_str.c_str());
 
   std::unique_ptr<const char*[]> c_argv = StringVectorToArgArray(args);
   node::Environment* env = node::CreateEnvironment(

docs/api/client-request.md

@@ -32,7 +32,7 @@ the hostname and the port number 'hostname:port'.
   * `redirect` String (optional) - The redirect mode for this request. Should be
 one of `follow`, `error` or `manual`. Defaults to `follow`. When mode is `error`,
 any redirection will be aborted. When mode is `manual` the redirection will be
-deferred until [`request.followRedirect`](#requestfollowRedirect) is invoked. Listen for the [`redirect`](#event-redirect) event in
+deferred until [`request.followRedirect`](#requestfollowredirect) is invoked. Listen for the [`redirect`](#event-redirect) event in
 this mode to get more details about the redirect request.
 
 `options` properties such as `protocol`, `host`, `hostname`, `port` and `path`
@@ -137,7 +137,7 @@ Returns:
 * `responseHeaders` Object
 
 Emitted when there is redirection and the mode is `manual`. Calling
-[`request.followRedirect`](#requestfollowRedirect) will continue with the redirection.
+[`request.followRedirect`](#requestfollowredirect) will continue with the redirection.
 
 ### Instance Properties
 

docs/api/web-contents.md

@@ -291,7 +291,7 @@ Calling `event.preventDefault` will prevent the page `keydown`/`keyup` events
 and the menu shortcuts.
 
 To only prevent the menu shortcuts, use
-[`setIgnoreMenuShortcuts`](#contentssetignoremenushortcuts):
+[`setIgnoreMenuShortcuts`](#contentssetignoremenushortcutsignore-experimental):
 
 ```javascript
 const {BrowserWindow} = require('electron')

docs/api/webview-tag.md

@@ -528,7 +528,7 @@ can be obtained by subscribing to [`found-in-page`](webview-tag.md#event-found-i
 ### `<webview>.stopFindInPage(action)`
 
 * `action` String - Specifies the action to take place when ending
-  [`<webview>.findInPage`](webview-tag.md#webviewtagfindinpage) request.
+  [`<webview>.findInPage`](#webviewfindinpagetext-options) request.
   * `clearSelection` - Clear the selection.
   * `keepSelection` - Translate the selection into a normal selection.
   * `activateSelection` - Focus and click the selection node.
@@ -579,7 +579,7 @@ Send an asynchronous message to renderer process via `channel`, you can also
 send arbitrary arguments. The renderer process can handle the message by
 listening to the `channel` event with the [`ipcRenderer`](ipc-renderer.md) module.
 
-See [webContents.send](web-contents.md#webcontentssendchannel-args) for
+See [webContents.send](web-contents.md#contentssendchannel-arg1-arg2-) for
 examples.
 
 ### `<webview>.sendInputEvent(event)`
@@ -588,7 +588,7 @@ examples.
 
 Sends an input `event` to the page.
 
-See [webContents.sendInputEvent](web-contents.md#webcontentssendinputeventevent)
+See [webContents.sendInputEvent](web-contents.md#contentssendinputeventevent)
 for detailed description of `event` object.
 
 ### `<webview>.setZoomFactor(factor)`
@@ -752,7 +752,7 @@ Returns:
   * `finalUpdate` Boolean
 
 Fired when a result is available for
-[`webview.findInPage`](webview-tag.md#webviewtagfindinpage) request.
+[`webview.findInPage`](#webviewfindinpagetext-options) request.
 
 ```javascript
 const webview = document.querySelector('webview')

docs/development/testing.md

@@ -15,7 +15,7 @@ To ensure that your JavaScript is in compliance with the Electron coding
 style, run `npm run lint-js`, which will run `standard` against both
 Electron itself as well as the unit tests. If you are using an editor
 with a plugin/addon system, you might want to use one of the many
-[StandardJS addons](standard-addons) to be informed of coding style
+[StandardJS addons][standard-addons] to be informed of coding style
 violations before you ever commit them.
 
 To run `standard` with parameters, run `npm run lint-js --` followed by

filenames.gypi

@@ -99,6 +99,8 @@
       'atom/app/atom_main_delegate.cc',
       'atom/app/atom_main_delegate.h',
       'atom/app/atom_main_delegate_mac.mm',
+      'atom/app/command_line_args.cc',
+      'atom/app/command_line_args.h',
       'atom/app/node_main.cc',
       'atom/app/node_main.h',
       'atom/app/uv_task_runner.cc',

spec/api-app-spec.js

@@ -634,6 +634,54 @@ describe('app module', () => {
     })
   })
 
+  describe('app launch through uri', () => {
+    before(function () {
+      if (process.platform !== 'win32') {
+        this.skip()
+      }
+    })
+
+    it('does not launch for blacklisted argument', function (done) {
+      const appPath = path.join(__dirname, 'fixtures', 'api', 'quit-app')
+      // App should exit with non 123 code.
+      const first = ChildProcess.spawn(remote.process.execPath, [appPath, 'electron-test://?', '--no-sandbox', '--gpu-launcher=cmd.exe /c start calc'])
+      first.once('exit', (code) => {
+        assert.notEqual(code, 123)
+        done()
+      })
+    })
+
+    it('launches successfully for multiple uris in cmd args', function (done) {
+      const appPath = path.join(__dirname, 'fixtures', 'api', 'quit-app')
+      // App should exit with code 123.
+      const first = ChildProcess.spawn(remote.process.execPath, [appPath, 'http://electronjs.org', 'electron-test://testdata'])
+      first.once('exit', (code) => {
+        assert.equal(code, 123)
+        done()
+      })
+    })
+
+    it('does not launch for encoded space', function (done) {
+      const appPath = path.join(__dirname, 'fixtures', 'api', 'quit-app')
+      // App should exit with non 123 code.
+      const first = ChildProcess.spawn(remote.process.execPath, [appPath, 'electron-test://?', '--no-sandbox', '--gpu-launcher%20"cmd.exe /c start calc'])
+      first.once('exit', (code) => {
+        assert.notEqual(code, 123)
+        done()
+      })
+    })
+
+    it('launches successfully for argnames similar to blacklisted ones', function (done) {
+      const appPath = path.join(__dirname, 'fixtures', 'api', 'quit-app')
+      // inspect is blacklisted, but inspector should work, and app launch should succeed
+      const first = ChildProcess.spawn(remote.process.execPath, [appPath, 'electron-test://?', '--inspector'])
+      first.once('exit', (code) => {
+        assert.equal(code, 123)
+        done()
+      })
+    })
+  })
+
   describe('getFileIcon() API', () => {
     const iconPath = path.join(__dirname, 'fixtures/assets/icon.ico')
     const sizes = {