Use system crypto in pref-pane IsPinValid()

Use system crypto in pref-pane IsPinValid()

Use system-provided HMAC library to remove dependency of Mac
pref-pane on Chrome crypto code.  This is to enable the pref-pane to be
built as a Mac universal binary, even though Chrome code is only built for
32-bit on Mac OS X.

Linking to individual files in base/ is not feasible, since any calls to the
logging facility cause many files to be pulled in as dependencies, creating a
burden for any maintainers of those files.  So we prefer to avoid using
base/ entirely, until Chrome supports building for 64-bit on Mac OS X.

BUG=125116
TEST=manual


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137278

[dcaiafa, 2012-05-14 15:04:43]

drive-by.

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

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

[Wez, 2012-05-14 16:42:52]

Can you briefly elaborate on why we want to break that dependency? Does it
trigger a cascade of unwanted dependencies, for example?

[Jamie, 2012-05-14 17:37:49]

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

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference...
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.

[Lambros, 2012-05-14 17:43:25]

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.

[Lambros, 2012-05-14 19:58:33]

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

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference...
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...
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10384140/diff/1/remoting/host/me2me_preference...
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.

[Wez, 2012-05-14 20:02:11]

On 2012/05/14 17:43:25, Lambros wrote:
> 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.

OK, so can you make sure the CL description explains these points (reducing the
dependencies, particularly to enable Universal builds), please?

[Lambros, 2012-05-14 20:35:42]

On 2012/05/14 20:02:11, Wez wrote:
> On 2012/05/14 17:43:25, Lambros wrote:
> > 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.
> 
> OK, so can you make sure the CL description explains these points (reducing
the
> dependencies, particularly to enable Universal builds), please?
Done.

[Jamie, 2012-05-14 20:59:34]

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_prefere...
File remoting/host/me2me_preference_pane.mm (right):

http://codereview.chromium.org/10384140/diff/5002/remoting/host/me2me_prefere...
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_prefere...
remoting/host/me2me_preference_pane.mm:74: LOG(ERROR) << "Failed to parse
host_secret_hash";
NSLog

[Lambros, 2012-05-15 01:08:33]

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

http://codereview.chromium.org/10384140/diff/5002/remoting/host/me2me_prefere...
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.

[commit-bot: I haz the power, 2012-05-15 21:00:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/10384140/4002

[commit-bot: I haz the power, 2012-05-15 22:49:02]

Change committed as 137278

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

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

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me...
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...
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.

[Lambros, 2012-05-16 18:48:56]

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

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me...
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...
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.

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

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

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me...
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...
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.

[Wez, 2012-05-16 20:09:26]

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

https://chromiumcodereview.appspot.com/10384140/diff/4002/remoting/host/me2me...
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...
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!