Use system crypto in pref-pane IsPinValid()

remoting/host/me2me_preference_pane.mm

 // 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.
 
 #import "remoting/host/me2me_preference_pane.h"
 
 #import <Cocoa/Cocoa.h>
+#include <CommonCrypto/CommonHMAC.h>
 #include <launch.h>
 #import <PreferencePanes/PreferencePanes.h>
 #import <SecurityInterface/SFAuthorizationView.h>
 
 #include "base/eintr_wrapper.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "base/mac/authorization_util.h"
 #include "base/mac/foundation_util.h"
 #include "base/mac/launchd.h"
 #include "base/mac/mac_logging.h"
 #include "base/mac/scoped_launch_data.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/stringprintf.h"
 #include "base/sys_string_conversions.h"
 #include "remoting/host/host_config.h"
 #include "remoting/host/json_host_config.h"
-#include "remoting/protocol/me2me_host_authenticator_factory.h"
+#include "third_party/modp_b64/modp_b64.h"
 
 namespace {
 // The name of the Remoting Host service that is registered with launchd.
 #define kServiceName "org.chromium.chromoting"
 #define kConfigDir "/Library/PrivilegedHelperTools/"
 
 // This helper script is executed as root.  It is passed a command-line option
 // (--enable or --disable), which causes it to create or remove a file that
 // informs the host's launch script of whether the host is enabled or disabled.
 const char kHelperTool[] = kConfigDir kServiceName ".me2me.sh";
 
 bool GetTemporaryConfigFilePath(FilePath* path) {
   if (!file_util::GetTempDir(path))
     return false;
   *path = path->Append(kServiceName ".json");
   return true;
 }
 
 bool IsConfigValid(const remoting::JsonHostConfig* config) {
   std::string value;
   return (config->GetString(remoting::kHostIdConfigPath, &value) &&
           config->GetString(remoting::kHostSecretHashConfigPath, &value) &&
           config->GetString(remoting::kXmppLoginConfigPath, &value));
 }
 
 bool IsPinValid(const std::string& pin, const std::string& host_id,
                 const std::string& host_secret_hash) {
-  remoting::protocol::SharedSecretHash hash;
-  if (!hash.Parse(host_secret_hash)) {
-    LOG(ERROR) << "Invalid host_secret_hash.";
+  // TODO(lambroslambrou): Once the "base" target supports building for 64-bit
+  // on Mac OS X, remove this code and replace it with |VerifyHostPinHash()|
+  // from host/pin_hash.h.
+  size_t separator = host_secret_hash.find(':');
+  if (separator == std::string::npos)
+    return false;
+
+  std::string method = host_secret_hash.substr(0, separator);
+  if (method != "hmac") {
+    LOG(ERROR) << "Authentication method '" << method << "' not supported";
     return false;
   }
-  std::string result =
-      remoting::protocol::AuthenticationMethod::ApplyHashFunction(
-          hash.hash_function, host_id, pin);
-  return result == hash.value;
+
+  std::string hash_base64 = host_secret_hash.substr(separator + 1);
+
+  // Convert |hash_base64| to |hash|, based on code from base/base64.cc.
+  int hash_base64_size = static_cast<int>(hash_base64.size());
+  std::string hash;
+  hash.resize(modp_b64_decode_len(hash_base64_size));

[Ryan Sleevi, 2012/05/16 03:10:34]

Security? You don't check if hash.empty() before attempting to do &hash[0]. As
far as STL goes, this is undefined, and typically debug STLs will assert here,
to prevent scribbling into memory you don't own.

[Lambros, 2012/05/16 18:48:57]

Ah, good point!  I lifted this from base/base64.cc, and it looks like that might
have the same problem.  Does that need patching as well?

[Ryan Sleevi, 2012/05/16 19:26:14]

Ah, looks like modp_b64_decode_len will always return at least 1, so short of
overflow issues, this should be fine. Subtle, but fine :)

[Wez, 2012/05/16 20:09:27]

Sufficiently subtle that a short comment to explain the fact is probably
advisable. :)

+  int hash_size = modp_b64_decode(&(hash[0]), hash_base64.data(),
+                                  hash_base64_size);
+  if (hash_size < 0) {
+    LOG(ERROR) << "Failed to parse host_secret_hash";
+    return false;
+  }
+  hash.resize(hash_size);
+
+  std::string computed_hash;
+  computed_hash.resize(CC_SHA256_DIGEST_LENGTH);
+
+  CCHmac(kCCHmacAlgSHA256,
+         host_id.data(), host_id.size(),
+         pin.data(), pin.size(),
+         &(computed_hash[0]));
+
+  return computed_hash == hash;

[Ryan Sleevi, 2012/05/16 03:10:34]

Security? This is not a constant time comparison, as implemented by crypto/, and
thus leaks timing information.

Please see the referenced information in crypto/ about why this is generally
bad.

[Lambros, 2012/05/16 18:48:57]

Thanks.  At the moment, the hash is already readable for the user supplying
input to this routine, so I don't know if a constant-time operation gains us
anything here?  I'll see if I can improve this anyway.

[Ryan Sleevi, 2012/05/16 19:26:14]

Nope, should be fine then.

[Wez, 2012/05/16 20:09:27]

Again, as discussed offline, a short comment to explain why this is OK would
help keep reviewers happy!

 }
 
 }  // namespace
 
 
 @implementation Me2MePreferencePane
 
 - (void)mainViewDidLoad {
   [authorization_view_ setDelegate:self];
   [authorization_view_ setString:kAuthorizationRightExecute];
@@ skipping 321 matching lines @@
   int error = launch_data_get_errno(response.get());
   if (error) {
     LOG(ERROR) << "launchd returned error: " << error;
     [self showError];
     return NO;
   }
   return YES;
 }
 
 @end