Disallow chrome-untrusted from spawning crosh

Step 2 of 2 to disallow terminal from spawning crosh.
* crrev.com/c/2094066
* crrev.com/c/2089558

Adds a new terminalPrivate.openVmshellProcess function
which is to be used by terminal.  This function can
open vmshell, but not crosh.

Terminal CL must be landed first which supports calling
either openVmshellProcess or openTerminalProcess.

Bug: 1056049
Change-Id: I0e4b54741aafcb3d6683f8ec668e94eaeec78ce8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089558Reviewed-by: Jason Lin <lxj@google.com>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749032}

chrome/browser/chromeos/web_applications/terminal_source.cc

@@ -64,8 +64,7 @@ void ReadFile(const std::string& relative_path,
              "fill='red'/></svg>"},
             {"html/terminal.html", "<script src='/js/terminal.js'></script>"},
             {"js/terminal.js",
-             "chrome.terminalPrivate.openTerminalProcess("
-             "'vmshell', [], () => {})"},
+             "chrome.terminalPrivate.openVmshellProcess([], () => {})"},
         });
     auto it = kTestFiles->find(relative_path);
     if (it != kTestFiles->end()) {

chrome/browser/extensions/api/terminal/terminal_private_api.cc

@@ -49,6 +49,8 @@ namespace OnTerminalResize =
     extensions::api::terminal_private::OnTerminalResize;
 namespace OpenTerminalProcess =
     extensions::api::terminal_private::OpenTerminalProcess;
+namespace OpenVmshellProcess =
+    extensions::api::terminal_private::OpenVmshellProcess;
 namespace CloseTerminalProcess =
     extensions::api::terminal_private::CloseTerminalProcess;
 namespace SendInput = extensions::api::terminal_private::SendInput;
@@ -184,6 +186,13 @@ TerminalPrivateOpenTerminalProcessFunction::Run() {
       OpenTerminalProcess::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
+  return OpenProcess(params->process_name, std::move(params->args));
+}
+
+ExtensionFunction::ResponseAction
+TerminalPrivateOpenTerminalProcessFunction::OpenProcess(
+    const std::string& process_name,
+    std::unique_ptr<std::vector<std::string>> args) {
   const std::string& user_id_hash =
       extensions::ExtensionsBrowserClient::Get()->GetUserIdHashFromContext(
           browser_context());
@@ -211,7 +220,7 @@ TerminalPrivateOpenTerminalProcessFunction::Run() {
     OpenProcess(user_id_hash, tab_id,
                 {command_line->GetSwitchValueASCII(switches::kCroshCommand)});
 
-  } else if (params->process_name == kCroshName) {
+  } else if (process_name == kCroshName) {
     // command=crosh: use '/usr/bin/crosh' on a device, 'cat' otherwise.
     if (base::SysInfo::IsRunningOnChromeOS()) {
       OpenProcess(user_id_hash, tab_id, {kCroshCommand});
@@ -219,7 +228,7 @@ TerminalPrivateOpenTerminalProcessFunction::Run() {
       OpenProcess(user_id_hash, tab_id, {kStubbedCroshCommand});
     }
 
-  } else if (params->process_name == kVmShellName) {
+  } else if (process_name == kVmShellName) {
     // Ensure crostini is allowed before starting terminal.
     Profile* profile = Profile::FromBrowserContext(browser_context());
     if (!crostini::CrostiniFeatures::Get()->IsAllowed(profile))
@@ -228,10 +237,10 @@ TerminalPrivateOpenTerminalProcessFunction::Run() {
     // command=vmshell: ensure --owner_id, --vm_name, and --target_container are
     // set and the specified vm/container is running.
     base::CommandLine vmshell_cmd({kVmShellCommand});
-    std::vector<std::string> args = {kVmShellCommand};
-    if (params->args)
-      args.insert(args.end(), params->args->begin(), params->args->end());
-    base::CommandLine params_args(args);
+    if (!args)
+      args = std::make_unique<std::vector<std::string>>();
+    args->insert(args->begin(), kVmShellCommand);
+    base::CommandLine params_args(*args);
     std::string owner_id =
         GetSwitch(&params_args, &vmshell_cmd, kSwitchOwnerId, user_id_hash);
     std::string vm_name = GetSwitch(&params_args, &vmshell_cmd, kSwitchVmName,
@@ -264,7 +273,7 @@ TerminalPrivateOpenTerminalProcessFunction::Run() {
         observer_ptr);
   } else {
     // command=[unrecognized].
-    return RespondNow(Error("Invalid process name: " + params->process_name));
+    return RespondNow(Error("Invalid process name: " + process_name));
   }
   return RespondLater();
 }
@@ -328,6 +337,19 @@ void TerminalPrivateOpenTerminalProcessFunction::OpenOnRegistryTaskRunner(
                  base::BindOnce(callback, success, terminal_id));
 }
 
+TerminalPrivateOpenVmshellProcessFunction::
+    ~TerminalPrivateOpenVmshellProcessFunction() = default;
+
+ExtensionFunction::ResponseAction
+TerminalPrivateOpenVmshellProcessFunction::Run() {
+  std::unique_ptr<OpenVmshellProcess::Params> params(
+      OpenVmshellProcess::Params::Create(*args_));
+  EXTENSION_FUNCTION_VALIDATE(params.get());
+
+  // Only opens 'vmshell'.
+  return OpenProcess(kVmShellName, std::move(params->args));
+}
+
 TerminalPrivateSendInputFunction::~TerminalPrivateSendInputFunction() = default;
 
 void TerminalPrivateOpenTerminalProcessFunction::RespondOnUIThread(

chrome/browser/extensions/api/terminal/terminal_private_api.h

@@ -53,6 +53,11 @@ class TerminalPrivateOpenTerminalProcessFunction : public ExtensionFunction {
 
   ExtensionFunction::ResponseAction Run() override;
 
+  // Open the specified |process_name| with supplied |args|.
+  ExtensionFunction::ResponseAction OpenProcess(
+      const std::string& process_name,
+      std::unique_ptr<std::vector<std::string>> args);
+
  private:
   // Callback for when starting crostini is complete.
   void OnCrostiniRestarted(
@@ -79,6 +84,19 @@ class TerminalPrivateOpenTerminalProcessFunction : public ExtensionFunction {
   void RespondOnUIThread(bool success, const std::string& terminal_id);
 };
 
+// Opens new vmshell process. Returns the new terminal id.
+class TerminalPrivateOpenVmshellProcessFunction
+    : public TerminalPrivateOpenTerminalProcessFunction {
+ public:
+  DECLARE_EXTENSION_FUNCTION("terminalPrivate.openVmshellProcess",
+                             TERMINALPRIVATE_OPENVMSHELLPROCESS)
+
+ protected:
+  ~TerminalPrivateOpenVmshellProcessFunction() override;
+
+  ExtensionFunction::ResponseAction Run() override;
+};
+
 // Send input to the terminal process specified by the terminal ID, which is set
 // as an argument.
 class TerminalPrivateSendInputFunction : public ExtensionFunction {

chrome/browser/extensions/api/terminal/terminal_private_browsertest.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+
+#include "chrome/browser/chromeos/crostini/fake_crostini_features.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/test/browser_test_utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace extensions {
+
+class TerminalPrivateBrowserTest : public InProcessBrowserTest {
+ public:
+  TerminalPrivateBrowserTest() = default;
+
+ protected:
+  void ExpectJsResult(const std::string& script, const std::string& expected) {
+    content::WebContents* web_contents =
+        browser()->tab_strip_model()->GetActiveWebContents();
+    content::EvalJsResult eval_result =
+        EvalJs(web_contents, script, content::EXECUTE_SCRIPT_DEFAULT_OPTIONS,
+               /*world_id=*/1);
+    EXPECT_EQ(eval_result.value.GetString(), expected);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TerminalPrivateBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(TerminalPrivateBrowserTest, OpenTerminalProcessChecks) {
+  ui_test_utils::NavigateToURL(
+      browser(), GURL("chrome-untrusted://terminal/html/terminal.html"));
+
+  const std::string script = R"(new Promise((resolve) => {
+    chrome.terminalPrivate.openVmshellProcess([], () => {
+      const lastError = chrome.runtime.lastError;
+      resolve(lastError ? lastError.message : "success");
+    })}))";
+
+  // 'vmshell not allowed' when crostini is not allowed.
+  crostini::FakeCrostiniFeatures crostini_features;
+  crostini_features.set_allowed(false);
+  ExpectJsResult(script, "vmshell not allowed");
+
+  // 'Error starting crostini for terminal: 22' when crostini is allowed.
+  // This is the specific error we expect to see in a test environment
+  // (LOAD_COMPONENT_FAILED), but the point of the test is to see that we get
+  // past the vmshell allowed checks.
+  crostini_features.set_allowed(true);
+  ExpectJsResult(script, "Error starting crostini for terminal: 22");
+
+  // openTerminalProcess not defined.
+  ExpectJsResult("typeof chrome.terminalPrivate.openTerminalProcess",
+                 "undefined");
+}
+
+}  // namespace extensions

chrome/common/extensions/api/_api_features.json

@@ -758,9 +758,12 @@
     ],
     "platforms": ["chromeos"]
   }],
+  // chrome.terminalPrivate allowed for crosh/nassh extensions,
+  // and chrome-untrusted://terminal.
   "terminalPrivate": [{
     "dependencies": ["permission:terminalPrivate"],
     "contexts": ["blessed_extension"],
+    "default_parent": true,
     "platforms": ["chromeos"]
   }, {
     "channel": "stable",
@@ -770,6 +773,11 @@
     ],
     "platforms": ["chromeos"]
   }],
+  // terminalPrivate.openTerminalProcess only allowed for
+  // crosh/nassh extensions.
+  "terminalPrivate.openTerminalProcess": {
+    "contexts": ["blessed_extension"]
+  },
   "topSites": {
     "dependencies": ["permission:topSites"],
     "contexts": ["blessed_extension"]

chrome/common/extensions/api/terminal_private.json

@@ -52,10 +52,39 @@
           }
         ]
       },
+      {
+        "name": "openVmshellProcess",
+        "type": "function",
+        "description": "Starts new vmshell process.",
+        "parameters": [
+          {
+            "type": "array",
+            "items": {
+              "type": "string"
+            },
+            "name": "args",
+            "optional": true,
+            "description": "Command line arguments to pass to vmshell."
+          },
+          {
+            "type": "function",
+            "name": "callback",
+            "optional": false,
+            "description": "Returns id of the launched vmshell process. If no process was launched returns -1.",
+            "parameters": [
+              {
+                "name": "id",
+                "description": "Id of the launched vmshell process.",
+                "type": "string"
+              }
+            ]
+          }
+        ]
+      },
       {
         "name": "closeTerminalProcess",
         "type": "function",
-        "description": "Closes previously opened process.",
+        "description": "Closes previously opened process from either openTerminalProcess or openVmshellProcess.",
         "parameters": [
           {
             "name": "id",

chrome/test/BUILD.gn

@@ -1871,6 +1871,7 @@ if (!is_android) {
           "../browser/accessibility/accessibility_extension_api_browsertest.cc",
           "../browser/apps/platform_apps/api/arc_apps_private/arc_apps_private_apitest.cc",
           "../browser/extensions/api/system_display/system_display_chromeos_apitest.cc",
+          "../browser/extensions/api/terminal/terminal_private_browsertest.cc",
           "../browser/extensions/clipboard_extension_apitest_chromeos.cc",
         ]
       }

extensions/browser/extension_function_histogram_value.h

@@ -1521,6 +1521,7 @@ enum HistogramValue {
   PASSWORDSPRIVATE_STARTPASSWORDCHECK = 1458,
   PASSWORDSPRIVATE_STOPPASSWORDCHECK = 1459,
   PASSWORDSPRIVATE_GETPASSWORDCHECKSTATUS = 1460,
+  TERMINALPRIVATE_OPENVMSHELLPROCESS = 1461,
   // Last entry: Add new entries above, then run:
   // python tools/metrics/histograms/update_extension_histograms.py
   ENUM_BOUNDARY

tools/metrics/histograms/enums.xml

@@ -22490,6 +22490,7 @@ to ensure that the crash string is shown properly on the user-facing crash UI.
   <int value="1458" label="PASSWORDSPRIVATE_STARTPASSWORDCHECK"/>
   <int value="1459" label="PASSWORDSPRIVATE_STOPPASSWORDCHECK"/>
   <int value="1460" label="PASSWORDSPRIVATE_GETPASSWORDCHECKSTATUS"/>
+  <int value="1461" label="TERMINALPRIVATE_OPENVMSHELLPROCESS"/>
 </enum>
 
 <enum name="ExtensionIconState">