Use system crypto in pref-pane IsPinValid()

remoting/host/me2me_preference_pane.mm

Index: remoting/host/me2me_preference_pane.mm
diff --git a/remoting/host/me2me_preference_pane.mm b/remoting/host/me2me_preference_pane.mm
index d092983ba081d82ff257c5f6970fa9519e77be2f..aec017a59c1c29530d08447e5d737463bc6f1124 100644
--- a/remoting/host/me2me_preference_pane.mm
+++ b/remoting/host/me2me_preference_pane.mm
@@ -5,6 +5,7 @@
 #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>
@@ -23,7 +24,7 @@
 #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.
@@ -51,15 +52,42 @@ bool IsConfigValid(const remoting::JsonHostConfig* config) {
 
 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