[Chromoting] Let the Windows daemon controller read the unprivileged part of the config without ele…

remoting/host/elevated_controller_win.cc

 // Copyright (c) 2012 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 "remoting/host/elevated_controller_win.h"
 
 #include <sddl.h>
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_writer.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/stringize_macros.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "base/win/scoped_handle.h"
 #include "remoting/host/branding.h"
+#include "remoting/host/daemon_controller_common_win.h"
 #include "remoting/host/elevated_controller_resource.h"
 #include "remoting/host/verify_config_window_win.h"
 
 namespace {
 
 // The host configuration file name.
 const FilePath::CharType kConfigFileName[] = FILE_PATH_LITERAL("host.json");
 
 // The extension for the temporary file.
 const FilePath::CharType kTempFileExtension[] = FILE_PATH_LITERAL("json~");
 
 // The host configuration file security descriptor that enables full access to
 // Local System and built-in administrators only.
 const char16 kConfigFileSecurityDescriptor[] =
     TO_L_STRING("O:BAG:BAD:(A;;GA;;;SY)(A;;GA;;;BA)");
 
-// The maximum size of the configuration file. "1MB ought to be enough" for any
-// reasonable configuration we will ever need. 1MB is low enough to make
-// the probability of out of memory situation fairly low. OOM is still possible
-// and we will crash if it occurs.
-const size_t kMaxConfigFileSize = 1024 * 1024;
+const char16 kUnprivilegedConfigFileSecurityDescriptor[] =
+    TO_L_STRING("O:BAG:BAD:(A;;GA;;;SY)(A;;GA;;;BA)(A;;GR;;;AU)");
 
-// ReadConfig() filters the configuration file stripping all variables except of
-// the following two.
+// Configuration keys.
 const char kHostId[] = "host_id";
 const char kXmppLogin[] = "xmpp_login";
+const char kHostSecretHash[] = "host_secret_hash";
 
 // The configuration keys that cannot be specified in UpdateConfig().
 const char* const kReadonlyKeys[] = { kHostId, kXmppLogin };
 
+// The configuration keys whose values may be read by GetConfig().
+const char* const kUnprivilegedConfigKeys[] = { kHostId, kXmppLogin };

[alexeypa (please no reviews), 2012/04/23 17:35:10]

kReadonlyKeys and kUnprivilegedConfigKeys strangely match. It does not seem to
be a coincidence.  

[simonmorris, 2012/04/23 19:39:49]

Yes, but is it clear that the two sets will always be equal, even if we later
add new keys?

[alexeypa (please no reviews), 2012/04/24 16:54:56]

As per our discussion these lists are not the same. Both are while lists. Please
file a bug so we will not forget to make sure it is correctly documented and
documented in the code.

[simonmorris, 2012/04/25 00:33:32]

crbug.com/124825

+
 // Reads and parses the configuration file up to |kMaxConfigFileSize| in
 // size.
 HRESULT ReadConfig(const FilePath& filename,

[alexeypa (please no reviews), 2012/04/23 17:35:10]

Is ReadConfig used now, when GetCOnfig is implemented in the plugin?

[simonmorris, 2012/04/23 19:39:49]

No, but there's no need to use this CL to remove it.

I've added a TODO.

[alexeypa (please no reviews), 2012/04/24 16:54:56]

Please include this into the COM interfaces cleanup bug and assign it to me.

[simonmorris, 2012/04/25 00:33:32]

crbug.com/124937

                    scoped_ptr<base::DictionaryValue>* config_out) {
 
   // Read raw data from the configuration file.
   base::win::ScopedHandle file(
       CreateFileW(filename.value().c_str(),
                   GENERIC_READ,
                   FILE_SHARE_READ | FILE_SHARE_WRITE,
                   NULL,
                   OPEN_EXISTING,
                   FILE_FLAG_SEQUENTIAL_SCAN,
                   NULL));
 
   if (!file.IsValid()) {
     DWORD error = GetLastError();
     LOG_GETLASTERROR(ERROR)
         << "Failed to open '" << filename.value() << "'";
     return HRESULT_FROM_WIN32(error);
   }
 
-  scoped_array<char> buffer(new char[kMaxConfigFileSize]);
-  DWORD size = kMaxConfigFileSize;
+  scoped_array<char> buffer(new char[remoting::kMaxConfigFileSize]);
+  DWORD size = remoting::kMaxConfigFileSize;
   if (!::ReadFile(file, &buffer[0], size, &size, NULL)) {
     DWORD error = GetLastError();
     LOG_GETLASTERROR(ERROR)
         << "Failed to read '" << filename.value() << "'";
     return HRESULT_FROM_WIN32(error);
   }
 
   // Parse the JSON configuration, expecting it to contain a dictionary.
   std::string file_content(buffer.get(), size);
   scoped_ptr<base::Value> value(
       base::JSONReader::Read(file_content, base::JSON_ALLOW_TRAILING_COMMAS));
 
   base::DictionaryValue* dictionary;
   if (value.get() == NULL || !value->GetAsDictionary(&dictionary)) {
     LOG(ERROR) << "Failed to read '" << filename.value() << "'.";
     return E_FAIL;
   }
 
   value.release();
   config_out->reset(dictionary);
   return S_OK;
 }
 
-// Writes the configuration file up to |kMaxConfigFileSize| in size.
-HRESULT WriteConfig(const FilePath& filename,
-                    const char* content,
-                    size_t length) {
-  if (length > kMaxConfigFileSize) {
-      return E_FAIL;
-  }
-
-  // Extract the configuration data that the user will verify.
-  scoped_ptr<base::Value> config_value(base::JSONReader::Read(content));
-  if (!config_value.get()) {
-    return E_FAIL;
-  }
-  base::DictionaryValue* config_dict = NULL;
-  if (!config_value->GetAsDictionary(&config_dict)) {
-    return E_FAIL;
-  }
-  std::string email, host_id, host_secret_hash;
-  if (!config_dict->GetString("xmpp_login", &email) ||
-      !config_dict->GetString("host_id", &host_id) ||
-      !config_dict->GetString("host_secret_hash", &host_secret_hash)) {
-    return E_FAIL;
-  }
-
-  // Ask the user to verify the configuration.
-  remoting::VerifyConfigWindowWin verify_win(email, host_id, host_secret_hash);
-  if (!verify_win.Run()) {
-    return E_FAIL;
-  }
-
+HRESULT WriteConfigFile(bool privileged, const char* content, size_t length) {
   // Create a security descriptor for the configuration file.
   SECURITY_ATTRIBUTES security_attributes;
   security_attributes.nLength = sizeof(security_attributes);
   security_attributes.bInheritHandle = FALSE;
 
   ULONG security_descriptor_length = 0;
+  const char16* descriptor = privileged ?

[alexeypa (please no reviews), 2012/04/23 17:35:10]

I somehow feel that passing ACL and filename to this function is better than
passing a flag.

[simonmorris, 2012/04/23 19:39:49]

Done.

+                             kConfigFileSecurityDescriptor :
+                             kUnprivilegedConfigFileSecurityDescriptor;
   if (!ConvertStringSecurityDescriptorToSecurityDescriptorW(
-           kConfigFileSecurityDescriptor,
+           descriptor,
            SDDL_REVISION_1,
            reinterpret_cast<PSECURITY_DESCRIPTOR*>(
                &security_attributes.lpSecurityDescriptor),
            &security_descriptor_length)) {
     DWORD error = GetLastError();
     LOG_GETLASTERROR(ERROR) <<
         "Failed to create a security descriptor for the configuration file";
     return HRESULT_FROM_WIN32(error);
   }
 
   // Create a temporary file and write configuration to it.
+  const FilePath::CharType* basename = privileged ?
+                                       kConfigFileName :
+                                       remoting::kUnprivilegedConfigFileName;
+  FilePath filename = remoting::GetConfigDir().Append(basename);
   FilePath tempname = filename.ReplaceExtension(kTempFileExtension);
   {
     base::win::ScopedHandle file(
         CreateFileW(tempname.value().c_str(),
                     GENERIC_WRITE,
                     0,
                     &security_attributes,
                     CREATE_ALWAYS,
                     FILE_FLAG_SEQUENTIAL_SCAN,
                     NULL));
 
     if (!file.IsValid()) {
       DWORD error = GetLastError();
       LOG_GETLASTERROR(ERROR)
           << "Failed to create '" << filename.value() << "'";
       return HRESULT_FROM_WIN32(error);
     }
 
     DWORD written;
     if (!WriteFile(file, content, static_cast<DWORD>(length), &written, NULL)) {
       DWORD error = GetLastError();
       LOG_GETLASTERROR(ERROR)
           << "Failed to write to '" << filename.value() << "'";
       return HRESULT_FROM_WIN32(error);
     }
   }
 
   // Now that the configuration is stored successfully replace the actual
   // configuration file.
   if (!MoveFileExW(tempname.value().c_str(),

[alexeypa (please no reviews), 2012/04/23 17:35:10]

Both MoveFileEx should the last thing to happen (unless you want to use NTFS
transactions). I.e. the sequence should be CreateTempFile1, CreateTempFile2,
MoveFileEx, MoveFileEx.

[simonmorris, 2012/04/23 19:39:49]

I'll make this change when we're sure that the unprivileged config file should
be written here.

[alexeypa (please no reviews), 2012/04/24 16:54:56]

Just to document what we discussed. Both files will be written by the privileged
code. The name of the unprivileged file will be a part of the interface as a
temporary solution. We will file a bug (the COM interface cleanup one) to
address it later. 

[simonmorris, 2012/04/25 00:33:32]

Done.

                    filename.value().c_str(),
                    MOVEFILE_REPLACE_EXISTING)) {
       DWORD error = GetLastError();
       LOG_GETLASTERROR(ERROR)
           << "Failed to rename '" << tempname.value() << "' to '"
           << filename.value() << "'";
       return HRESULT_FROM_WIN32(error);
   }
 
   return S_OK;
 }
 
+// Writes the configuration file up to |kMaxConfigFileSize| in size.
+HRESULT WriteConfig(const char* content, size_t length) {
+  if (length > remoting::kMaxConfigFileSize) {
+      return E_FAIL;
+  }
+
+  // Extract the configuration data that the user will verify.
+  scoped_ptr<base::Value> config_value(base::JSONReader::Read(content));
+  if (!config_value.get()) {
+    return E_FAIL;
+  }
+  base::DictionaryValue* config_dict = NULL;
+  if (!config_value->GetAsDictionary(&config_dict)) {
+    return E_FAIL;
+  }
+  std::string email, host_id, host_secret_hash;
+  if (!config_dict->GetString(kXmppLogin, &email) ||
+      !config_dict->GetString(kHostId, &host_id) ||
+      !config_dict->GetString(kHostSecretHash, &host_secret_hash)) {
+    return E_FAIL;
+  }
+
+  // Ask the user to verify the configuration.
+  remoting::VerifyConfigWindowWin verify_win(email, host_id, host_secret_hash);
+  if (!verify_win.Run()) {
+    return E_FAIL;
+  }
+
+  // Write the full configuration file.
+  HRESULT hr = WriteConfigFile(true, content, length);
+  if (FAILED(hr)) {
+    return hr;
+  }
+
+  // Extract the unprivileged fields from the configuration.
+  base::DictionaryValue unprivileged_config_dict;
+  for (int i = 0; i < arraysize(kUnprivilegedConfigKeys); ++i) {
+    const char* key = kUnprivilegedConfigKeys[i];
+    string16 value;
+    if (config_dict->GetString(key, &value)) {
+      unprivileged_config_dict.SetString(key, value);
+    }
+  }
+  std::string unprivileged_config_str;
+  base::JSONWriter::Write(&unprivileged_config_dict, &unprivileged_config_str);
+
+  // Write the unprivileged configuration file.
+  hr = WriteConfigFile(false,
+                       unprivileged_config_str.data(),
+                       unprivileged_config_str.size());
+  if (FAILED(hr)) {
+    return hr;
+  }
+
+  return S_OK;
+}
 
 } // namespace
 
 namespace remoting {
 
 ElevatedControllerWin::ElevatedControllerWin() {
 }
 
 HRESULT ElevatedControllerWin::FinalConstruct() {
   return S_OK;
@@ skipping 39 matching lines @@
 STDMETHODIMP ElevatedControllerWin::SetConfig(BSTR config) {
   // Determine the config directory path and create it if necessary.
   FilePath config_dir = remoting::GetConfigDir();
   if (!file_util::CreateDirectory(config_dir)) {
     return HRESULT_FROM_WIN32(ERROR_ACCESS_DENIED);
   }
 
   std::string file_content = UTF16ToUTF8(
     string16(static_cast<char16*>(config), ::SysStringLen(config)));
 
-  return WriteConfig(config_dir.Append(kConfigFileName),
-                     file_content.c_str(),
-                     file_content.size());
+  return WriteConfig(file_content.c_str(), file_content.size());
 }
 
 STDMETHODIMP ElevatedControllerWin::StartDaemon() {
   ScopedScHandle service;
   HRESULT hr = OpenService(&service);
   if (FAILED(hr)) {
     return hr;
   }
 
   // Change the service start type to 'auto'.
@@ skipping 92 matching lines @@
   scoped_ptr<base::DictionaryValue> config_old;
   HRESULT hr = ReadConfig(config_dir.Append(kConfigFileName), &config_old);
   if (FAILED(hr)) {
     return hr;
   }
   // Merge items from the given config into the old config.
   config_old->MergeDictionary(config_dict);
   // Write the updated config.
   std::string config_updated_str;
   base::JSONWriter::Write(config_old.get(), &config_updated_str);
-  return WriteConfig(config_dir.Append(kConfigFileName),
-                     config_updated_str.c_str(),
-                     config_updated_str.size());
+  return WriteConfig(config_updated_str.c_str(), config_updated_str.size());
 }
 
 HRESULT ElevatedControllerWin::OpenService(ScopedScHandle* service_out) {
   DWORD error;
 
   ScopedScHandle scmanager(
       ::OpenSCManagerW(NULL, SERVICES_ACTIVE_DATABASE,
                        SC_MANAGER_CONNECT | SC_MANAGER_ENUMERATE_SERVICE));
   if (!scmanager.IsValid()) {
     error = GetLastError();
@@ skipping 13 matching lines @@
         << "Failed to open to the '" << kWindowsServiceName << "' service";
 
     return HRESULT_FROM_WIN32(error);
   }
 
   service_out->Set(service.Take());
   return S_OK;
 }
 
 } // namespace remoting