[Remote Commands] Update secure remote commands approach

After some security discussions, it was decided to wrap the RemoteCommand proto
in an additional PolicyData proto layer.  Update the code to reflect this.

Also, enable secure remote commands again (they were temporarily disabled in
crrev.com/c/1771781).

Bug: 891222
Change-Id: I6439959cf4b5562b40870df9d9455bc6e5078203
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1774621
Commit-Queue: Ivan Šandrk <isandrk@chromium.org>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691586}

components/policy/core/common/cloud/cloud_policy_client.cc

@@ -623,6 +623,8 @@ void CloudPolicyClient::FetchRemoteCommands(
   for (const auto& command_result : command_results)
     *request->add_command_results() = command_result;
 
+  request->set_send_secure_commands(true);
+
   request_jobs_.push_back(service_->CreateJob(std::move(config)));
 }
 

components/policy/core/common/cloud/cloud_policy_client_unittest.cc

@@ -245,6 +245,8 @@ class CloudPolicyClientTest : public testing::Test {
         em::RemoteCommandResult_ResultType_RESULT_SUCCESS);
     command_result->set_payload(kResultPayload);
     command_result->set_timestamp(kTimestamp);
+    remote_command_request_.mutable_remote_command_request()
+        ->set_send_secure_commands(true);
 
     em::RemoteCommand* command =
         remote_command_response_.mutable_remote_command_response()

components/policy/core/common/cloud/cloud_policy_constants.cc

@@ -83,6 +83,7 @@ const char kChromeMachineLevelUserCloudPolicyType[] =
     "google/chrome/machine-level-user";
 const char kChromeMachineLevelExtensionCloudPolicyType[] =
     "google/chrome/machine-level-extension";
+const char kChromeRemoteCommandPolicyType[] = "google/chromeos/remotecommand";
 
 }  // namespace dm_protocol
 

components/policy/core/common/cloud/cloud_policy_constants.h

@@ -66,6 +66,7 @@ POLICY_EXPORT extern const char kChromeExtensionPolicyType[];
 POLICY_EXPORT extern const char kChromeSigninExtensionPolicyType[];
 POLICY_EXPORT extern const char kChromeMachineLevelUserCloudPolicyType[];
 POLICY_EXPORT extern const char kChromeMachineLevelExtensionCloudPolicyType[];
+POLICY_EXPORT extern const char kChromeRemoteCommandPolicyType[];
 
 // These codes are sent in the |error_code| field of PolicyFetchResponse.
 enum PolicyFetchStatus {

components/policy/core/common/remote_commands/remote_command_job.h

@@ -49,10 +49,10 @@ class POLICY_EXPORT RemoteCommandJob {
   // time. It must be consistent to the same parameter passed to Run() below.
   // In order to minimize the error while estimating the command issued time,
   // this method must be called immediately after the command is received from
-  // the server. |signed_command| is passed if we're using signed commands, its
-  // format is the raw serialized command plus its signature, and it's cached in
-  // case the actual command implementation needs to pass its signature on to
-  // some other system for verification.
+  // the server. |signed_command| is passed if we're using signed commands; its
+  // format is the raw serialized command inside of policy data proto plus its
+  // signature, and it's cached in case the actual command implementation needs
+  // to pass its signature on to some other system for verification.
   bool Init(base::TimeTicks now,
             const enterprise_management::RemoteCommand& command,
             const enterprise_management::SignedData* signed_command);
@@ -161,8 +161,8 @@ class POLICY_EXPORT RemoteCommandJob {
   // The time when the command started running.
   base::TimeTicks execution_started_time_;
 
-  // Serialized command with signature in case of a signed command, otherwise
-  // empty.
+  // Serialized command inside policy data proto with signature in case of a
+  // signed command, otherwise empty.
   base::Optional<enterprise_management::SignedData> signed_command_;
 
   std::unique_ptr<ResultPayload> result_payload_;

components/policy/core/common/remote_commands/remote_commands_service.cc

@@ -15,6 +15,7 @@
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "components/policy/core/common/cloud/cloud_policy_client.h"
+#include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "components/policy/core/common/cloud/cloud_policy_store.h"
 #include "components/policy/core/common/cloud/cloud_policy_validator.h"
 #include "components/policy/core/common/remote_commands/remote_commands_factory.h"
@@ -92,9 +93,6 @@ void RemoteCommandsService::SetOnCommandAckedCallback(
 
 void RemoteCommandsService::VerifyAndEnqueueSignedCommand(
     const em::SignedData& signed_command) {
-  em::RemoteCommand command;
-  command.ParseFromString(signed_command.data());
-
   const bool valid_signature = CloudPolicyValidatorBase::VerifySignature(
       signed_command.data(), store_->policy_signature_public_key(),
       signed_command.signature(),
@@ -104,7 +102,28 @@ void RemoteCommandsService::VerifyAndEnqueueSignedCommand(
     SYSLOG(ERROR) << "Secure remote command signature verification failed";
     em::RemoteCommandResult result;
     result.set_result(em::RemoteCommandResult_ResultType_RESULT_IGNORED);
-    result.set_command_id(command.command_id());
+    unsent_results_.push_back(result);
+    return;
+  }
+
+  em::PolicyData policy_data;
+  if (!policy_data.ParseFromString(signed_command.data()) ||
+      !policy_data.has_policy_type() ||
+      policy_data.policy_type() !=
+          dm_protocol::kChromeRemoteCommandPolicyType) {
+    SYSLOG(ERROR) << "Secure remote command with wrong PolicyData type";
+    em::RemoteCommandResult result;
+    result.set_result(em::RemoteCommandResult_ResultType_RESULT_IGNORED);
+    unsent_results_.push_back(result);
+    return;
+  }
+
+  em::RemoteCommand command;
+  if (!policy_data.has_policy_value() ||
+      !command.ParseFromString(policy_data.policy_value())) {
+    SYSLOG(ERROR) << "Secure remote command invalid RemoteCommand data";
+    em::RemoteCommandResult result;
+    result.set_result(em::RemoteCommandResult_ResultType_RESULT_IGNORED);
     unsent_results_.push_back(result);
     return;
   }