Revert of remoting: use VerifyHostPinHash() in place on IsPinValid() (issue 1543303002 by lukasza@chromium.org)

[luk...@chromium.org]

Reviewers: Sergey Ulanov, tfarina
CL: https://codereview.chromium.org/1543303002/

Message:
Created Revert of remoting: use VerifyHostPinHash() in place on IsPinValid()

Description:
Revert of remoting: use VerifyHostPinHash() in place on IsPinValid()
(patchset
#4 id:60001 of https://codereview.chromium.org/1547533002/ )

Reason for revert:
This seems to break the build on Mac:

https://build.chromium.org/p/chromium/builders/Mac/builds/10276/steps/compile/logs/stdio:

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/remoting/host/mac/remoting_host_prefpane.me2me_preference_pane.x86_64.o.d
-DBINARY_CORE=1 -DBINARY_DESKTOP=2 -DBINARY_HOST_ME2ME=3
-DBINARY_NATIVE_MESSAGING_HOST=4 -DBINARY_REMOTE_ASSISTANCE_HOST=5
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2
-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DCHROMIUM_BUILD
-DCR_CLANG_REVISION=255169-1 -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN
-DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
-DDONT_EMBED_BUILD_METADATA -DFIELDTRIAL_TESTING_ENABLED
-DENABLE_TASK_MANAGER=1
-DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_BACKGROUND=1 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DUSE_BROWSER_SPELLCHECKER=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1
-DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1 -DENABLE_SUPERVISED_USERS=1
-DENABLE_SERVICE_DISCOVERY=1 -DV8_USE_EXTERNAL_STARTUP_DATA
-DFULL_SAFE_BROWSING
-DSAFE_BROWSING_CSD -DSAFE_BROWSING_DB_LOCAL
'-DHOST_BUNDLE_NAME="ChromotingHost.bundle"'
'-DPREFPANE_BUNDLE_NAME="Chromoting.prefPane"' -DJSON_USE_EXCEPTION=0
-DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DUSE_LIBPCI=1 -DUSE_OPENSSL=1
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -I../.. -Igen
-I../../third_party/jsoncpp/overrides/include
-I../../third_party/jsoncpp/source/include
-I../../third_party/jsoncpp/source/src/lib_json -I../../third_party/khronos
-I../../gpu -I../../skia/config -I../../third_party/protobuf
-I../../third_party/protobuf/src -Igen/protoc_out -isysroot
/Applications/Xcode5.1.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
-O2 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
-mmacosx-version-min=10.6 -arch x86_64 -Wall -Wextra -Wno-unused-parameter
-Wno-missing-field-initializers -Wno-selector-type-mismatch
-Wpartial-availability -Wheader-hygiene -Wno-char-subscripts
-Wno-unneeded-internal-declaration -Wno-covered-switch-default
-Wstring-conversion -Wno-c++11-narrowing -Wno-deprecated-register
-Wno-inconsistent-missing-override -Wno-shift-negative-value
-Wno-bitfield-width
-std=c++11 -stdlib=libc++ -fno-rtti -fno-exceptions
-fvisibility-inlines-hidden
-fno-threadsafe-statics -Xclang -load -Xclang
/b/build/slave/Mac/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -fcolor-diagnostics
-fno-strict-aliasing -fobjc-gc -Wobjc-missing-property-synthesis
-fobjc-call-cxx-cdtors -c ../../remoting/host/mac/me2me_preference_pane.mm
-o
obj/remoting/host/mac/remoting_host_prefpane.me2me_preference_pane.x86_64.o
../../remoting/host/mac/me2me_preference_pane.mm:299:8: error: use of
undeclared
identifier 'VerifyHostPinHash'; did you mean 'remoting::VerifyHostPinHash'?
if (!VerifyHostPinHash(pin_utf8, host_id, host_secret_hash)) {
^~~~~~~~~~~~~~~~~
remoting::VerifyHostPinHash


Original issue's description:
> remoting: use VerifyHostPinHash() in place on IsPinValid()

> For over a year, we have been shipping a 64-bit version of Chrome for Mac.
> So with this statement, this patch makes the switch to
> VerifyHostPinHash() function from remoting/host/pin_hash.h, fixing the
> lambroslambrou's TODO.

> BUG=None
> R=ser...@chromium.org

> Committed: https://crrev.com/7acd392e4018d121977e2738b6e3c59f5cf418f1
> Cr-Commit-Position: refs/heads/master@{#366762}

TBR=ser...@chromium.org,tfa...@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=None

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+62, -5 lines):
M remoting/host/DEPS
M remoting/host/mac/me2me_preference_pane.mm
M remoting/remoting_host_mac.gypi


Index: remoting/host/DEPS
diff --git a/remoting/host/DEPS b/remoting/host/DEPS
index
052a0c5ca705fd2987e3891f9665f35242638fb5..3abc8545dbe42bf8933ae6066acb8037b5c824d2
100644
--- a/remoting/host/DEPS
+++ b/remoting/host/DEPS
@@ -10,6 +10,7 @@
"+remoting/signaling",
"+remoting/tools",
"+third_party/jsoncpp",
+ "+third_party/modp_b64",
"+third_party/skia",
"+third_party/webrtc",
"+ui",
Index: remoting/host/mac/me2me_preference_pane.mm
diff --git a/remoting/host/mac/me2me_preference_pane.mm
b/remoting/host/mac/me2me_preference_pane.mm
index
0d67bffc24fbedec0d999aa6f391d66a3c6c9670..de33b8bf500127561dea1a3514b6e68ca5487f81
100644
--- a/remoting/host/mac/me2me_preference_pane.mm
+++ b/remoting/host/mac/me2me_preference_pane.mm
@@ -21,11 +21,11 @@
#include "base/posix/eintr_wrapper.h"
#include "remoting/host/constants_mac.h"
#include "remoting/host/host_config.h"
-#include "remoting/host/pin_hash.h"
#import "remoting/host/mac/me2me_preference_pane_confirm_pin.h"
#import "remoting/host/mac/me2me_preference_pane_disable.h"
#include "third_party/jsoncpp/source/include/json/reader.h"
#include "third_party/jsoncpp/source/include/json/writer.h"
+#include "third_party/modp_b64/modp_b64.h"

namespace {

@@ -44,6 +44,51 @@
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) {
+ // 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") {
+ NSLog(@"Authentication method '%s' not supported", method.c_str());
+ return false;
+ }
+
+ 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));
+
+ // modp_b64_decode_len() returns at least 1, so hash[0] is safe here.
+ int hash_size = modp_b64_decode(&(hash[0]), hash_base64.data(),
+ hash_base64_size);
+ if (hash_size < 0) {
+ NSLog(@"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]));
+
+ // Normally, a constant-time comparison function would be used, but it is
+ // unnecessary here as the "secret" is already readable by the user
+ // supplying input to this routine.
+ return computed_hash == hash;
}

} // namespace
@@ -296,7 +341,7 @@
[self showError];
return;
}
- if (!VerifyHostPinHash(pin_utf8, host_id, host_secret_hash)) {
+ if (!IsPinValid(pin_utf8, host_id, host_secret_hash)) {
[self showIncorrectPinMessage];
return;
}
Index: remoting/remoting_host_mac.gypi
diff --git a/remoting/remoting_host_mac.gypi
b/remoting/remoting_host_mac.gypi
index
258a6883636e331fa074276e153f889432acd763..dce3cf08b6d79024b3c02f0ff5b44dd785a9eb60
100644
--- a/remoting/remoting_host_mac.gypi
+++ b/remoting/remoting_host_mac.gypi
@@ -178,10 +178,7 @@
'prefpane_bundle_name': '<!(python <(version_py_path) -f
<(branding_path) -t "@MAC_PREFPANE_BUNDLE_NAME@")',
},
'dependencies': [
- 'remoting_base',
- 'remoting_host',
'remoting_infoplist_strings',
- '<(DEPTH)/third_party/jsoncpp/jsoncpp.gyp:jsoncpp',
],
'defines': [
'HOST_BUNDLE_NAME="<(host_bundle_name)"',
@@ -193,7 +190,21 @@
'../third_party/jsoncpp/source/include/',
'../third_party/jsoncpp/source/src/lib_json/',
],
+
+ # These source files are included directly, instead of adding target
+ # dependencies, because the targets are not yet built for 64-bit on
+ # Mac OS X - http://crbug.com/125116.
+ #
+ # TODO(lambroslambrou): Fix this when Chrome supports building for
+ # Mac OS X 64-bit - http://crbug.com/128122.
'sources': [
+ '../third_party/jsoncpp/overrides/src/lib_json/json_value.cpp',
+ '../third_party/jsoncpp/overrides/src/lib_json/json_reader.cpp',
+ '../third_party/jsoncpp/source/src/lib_json/json_writer.cpp',
+ '../third_party/modp_b64/modp_b64.cc',
+ 'host/constants_mac.cc',
+ 'host/constants_mac.h',
+ 'host/host_config_constants.cc',
'host/mac/me2me_preference_pane.h',
'host/mac/me2me_preference_pane.mm',
'host/mac/me2me_preference_pane_confirm_pin.h',