Use system crypto in pref-pane IsPinValid() (issue 10384140)

0 views
Skip to first unread message

lambros...@chromium.org

unread,
May 11, 2012, 9:52:24 PM5/11/12
to jamie...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
Reviewers: Jamie,

Description:
Use system crypto in pref-pane IsPinValid()

Use system-provided HMAC library to remove dependency of Mac pref-pane on
Chrome crypto code.


BUG=125116
TEST=manual


Please review this at http://codereview.chromium.org/10384140/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M 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..966097771ae3de9da801858694a73fca256caa3a
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,39 @@ 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.";
+ 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));
+ 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(32);
+
+ CCHmac(kCCHmacAlgSHA256,
+ host_id.data(), host_id.size(),
+ pin.data(), pin.size(),
+ &(computed_hash[0]));
+
+ return computed_hash == hash;
}

} // namespace


dca...@chromium.org

unread,
May 14, 2012, 11:04:43 AM5/14/12
to lambros...@chromium.org, jamie...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
drive-by.


http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference_pane.mm#newcode80
remoting/host/me2me_preference_pane.mm:80: computed_hash.resize(32);
s/32/CC_SHA256_DIGEST_LENGTH/

http://codereview.chromium.org/10384140/

w...@chromium.org

unread,
May 14, 2012, 12:42:53 PM5/14/12
to lambros...@chromium.org, jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
Can you briefly elaborate on why we want to break that dependency? Does it
trigger a cascade of unwanted dependencies, for example?

http://codereview.chromium.org/10384140/

jamie...@chromium.org

unread,
May 14, 2012, 1:37:49 PM5/14/12
to lambros...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (left):

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference_pane.mm#oldcode54
remoting/host/me2me_preference_pane.mm:54:
remoting::protocol::SharedSecretHash hash;
Would it be possible to define a local class with the same interface as
SharedSecretHash? That would minimize the changes to the rest of the
file and greatly simplify the task of undoing this change once we have
64-bit support in the core codebase.

http://codereview.chromium.org/10384140/

lambros...@chromium.org

unread,
May 14, 2012, 1:43:25 PM5/14/12
to jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
On 2012/05/14 16:42:52, Wez wrote:
> Can you briefly elaborate on why we want to break that dependency? Does it
> trigger a cascade of unwanted dependencies, for example?

crypto/hmac.cc contains "NOTREACHED()", which brings in the whole logging
framework. This pulls in a significant amount of code from base (threading,
synchronization, string-conversions, base/debug). See
http://codereview.chromium.org/10383143/ for the full list. Even that list
requires dead-code-stripping to link successfully on Mac OS X - the list
would
be far longer otherwise.

Directly linking to so many files creates a maintenance burden to other
Chrome
developers that they probably would not welcome :)

The code in "base" is only being built for 32-bit on Mac OS X, yet we need
to
build our pref-pane as a universal binary. So we have to pull Chrome source
files directly, or eliminate all dependencies on Chrome code. I'm
following the
latter approach here.

http://codereview.chromium.org/10384140/

lambros...@chromium.org

unread,
May 14, 2012, 3:58:33 PM5/14/12
to jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (left):

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference_pane.mm#oldcode54
remoting/host/me2me_preference_pane.mm:54:
remoting::protocol::SharedSecretHash hash;
On 2012/05/14 17:37:49, Jamie wrote:
> Would it be possible to define a local class with the same interface
as
> SharedSecretHash? That would minimize the changes to the rest of the
file and
> greatly simplify the task of undoing this change once we have 64-bit
support in
> the core codebase.

The SharedSecretHash/AuthenticationMethod interfaces don't seem very
convenient for embedding into here.

Everything is encapsulated into the IsPinValid() method here. Once we
have 64-bit support, all we need to do is replace this implementation
with a simple call to VerifyHostPinHash() in remoting/host/pin_hash.h.

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference_pane.mm#newcode80
remoting/host/me2me_preference_pane.mm:80: computed_hash.resize(32);
On 2012/05/14 15:04:43, dcaiafa wrote:
> s/32/CC_SHA256_DIGEST_LENGTH/

Thanks for that! Done.

http://codereview.chromium.org/10384140/

w...@chromium.org

unread,
May 14, 2012, 4:02:12 PM5/14/12
to lambros...@chromium.org, jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
OK, so can you make sure the CL description explains these points (reducing
the
dependencies, particularly to enable Universal builds), please?

http://codereview.chromium.org/10384140/

lambros...@chromium.org

unread,
May 14, 2012, 4:35:42 PM5/14/12
to jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
Done.


http://codereview.chromium.org/10384140/

jamie...@chromium.org

unread,
May 14, 2012, 4:59:34 PM5/14/12
to lambros...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
lgtm, but I'm not an expert on HMAC. If the code here is pretty much
cut-and-paste from elsewhere then I think it's fine, otherwise you should
get a
review from someone else.

Also, can you add a TODO somewhere to remove this code once we have 64-bit
support elsewhere?


http://codereview.chromium.org/10384140/diff/5002/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10384140/diff/5002/remoting/host/me2me_preference_pane.mm#newcode61
remoting/host/me2me_preference_pane.mm:61: LOG(ERROR) << "Authentication
method '" << method << "' not supported";
I think you want NSLog, otherwise you're going to have to follow-up with
a CL to change all the logging.

http://codereview.chromium.org/10384140/diff/5002/remoting/host/me2me_preference_pane.mm#newcode74
remoting/host/me2me_preference_pane.mm:74: LOG(ERROR) << "Failed to
parse host_secret_hash";
NSLog

http://codereview.chromium.org/10384140/

lambros...@chromium.org

unread,
May 14, 2012, 9:08:33 PM5/14/12
to jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

http://codereview.chromium.org/10384140/diff/5002/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10384140/diff/5002/remoting/host/me2me_preference_pane.mm#newcode61
remoting/host/me2me_preference_pane.mm:61: LOG(ERROR) << "Authentication
method '" << method << "' not supported";
On 2012/05/14 20:59:34, Jamie wrote:
> I think you want NSLog, otherwise you're going to have to follow-up
with a CL to
> change all the logging.
I'll have to do this anyway for all the LOGs in this file. Might as
well keep it consistent for now.

http://codereview.chromium.org/10384140/

commi...@chromium.org

unread,
May 15, 2012, 5:00:59 PM5/15/12
to lambros...@chromium.org, jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambros...@chromium.org/10384140/4002


https://chromiumcodereview.appspot.com/10384140/

commi...@chromium.org

unread,
May 15, 2012, 6:49:02 PM5/15/12
to lambros...@chromium.org, jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org
Change committed as 137278

https://chromiumcodereview.appspot.com/10384140/

rsl...@chromium.org

unread,
May 15, 2012, 11:10:34 PM5/15/12
to lambros...@chromium.org, jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm#newcode73
remoting/host/me2me_preference_pane.mm:73:
hash.resize(modp_b64_decode_len(hash_base64_size));
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.

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm#newcode90
remoting/host/me2me_preference_pane.mm:90: return computed_hash == hash;
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.

https://chromiumcodereview.appspot.com/10384140/

lambros...@chromium.org

unread,
May 16, 2012, 2:48:57 PM5/16/12
to jamie...@chromium.org, dca...@chromium.org, rsl...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm#newcode73
remoting/host/me2me_preference_pane.mm:73:
hash.resize(modp_b64_decode_len(hash_base64_size));
On 2012/05/16 03:10:34, Ryan Sleevi wrote:
> 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.
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?

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm#newcode90
remoting/host/me2me_preference_pane.mm:90: return computed_hash == hash;
On 2012/05/16 03:10:34, Ryan Sleevi wrote:
> 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.
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.

https://chromiumcodereview.appspot.com/10384140/

rsl...@chromium.org

unread,
May 16, 2012, 3:26:14 PM5/16/12
to lambros...@chromium.org, jamie...@chromium.org, dca...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm#newcode73
remoting/host/me2me_preference_pane.mm:73:
hash.resize(modp_b64_decode_len(hash_base64_size));
On 2012/05/16 18:48:57, Lambros wrote:
> On 2012/05/16 03:10:34, Ryan Sleevi wrote:
> > 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.
> 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?

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 :)

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm#newcode90
remoting/host/me2me_preference_pane.mm:90: return computed_hash == hash;
On 2012/05/16 18:48:57, Lambros wrote:
> On 2012/05/16 03:10:34, Ryan Sleevi wrote:
> > 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.
> 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.

Nope, should be fine then.

https://chromiumcodereview.appspot.com/10384140/

w...@chromium.org

unread,
May 16, 2012, 4:09:27 PM5/16/12
to lambros...@chromium.org, jamie...@chromium.org, dca...@chromium.org, rsl...@chromium.org, chromium...@chromium.org, jamiewal...@chromium.org, dcaiaf...@chromium.org, simonmor...@chromium.org, hclam...@chromium.org, wez+...@chromium.org, am...@chromium.org, sanj...@chromium.org, garyka...@chromium.org, lambroslam...@chromium.org, alexeyp...@chromium.org, sergey...@chromium.org

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm
File remoting/host/me2me_preference_pane.mm (right):

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm#newcode73
remoting/host/me2me_preference_pane.mm:73:
hash.resize(modp_b64_decode_len(hash_base64_size));
On 2012/05/16 19:26:14, Ryan Sleevi wrote:
> On 2012/05/16 18:48:57, Lambros wrote:
> > On 2012/05/16 03:10:34, Ryan Sleevi wrote:
> > > 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.
> > 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?

> 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 :)

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

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me_preference_pane.mm#newcode90
remoting/host/me2me_preference_pane.mm:90: return computed_hash == hash;
On 2012/05/16 19:26:14, Ryan Sleevi wrote:
> On 2012/05/16 18:48:57, Lambros wrote:
> > On 2012/05/16 03:10:34, Ryan Sleevi wrote:
> > > 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.
> > 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.

> Nope, should be fine then.

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

https://chromiumcodereview.appspot.com/10384140/
Reply all
Reply to author
Forward
0 new messages