Implement authenticator based on SPAKE2 implementation in boringssl.

Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will
be enabled in host and client in a separate CL.

BUG=589698
Committed: https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}

Committed: https://crrev.com/e5aa32ee4fae3a3e4d038001ca5c5ed3e6f02b1e
Cr-Commit-Position: refs/heads/master@{#380674}

[Sergey Ulanov, 2016-03-03 23:55:52]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-03-03 23:56:02]

Patchset #1 (id:20001) has been deleted

[Sergey Ulanov, 2016-03-03 23:56:20]

Description was changed from

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. The new
authenticator will be enabled in host and client in a separate CL.

BUG=589698
==========

to

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will 
be enabled in host and client in a separate CL.

BUG=589698
==========

[Sergey Ulanov, 2016-03-03 23:58:01]

sergeyu@chromium.org changed reviewers:
+ davidben@chromium.org, kelvinp@chromium.org

[Sergey Ulanov, 2016-03-03 23:58:02]

David, please approve dependency on BoringSSL.
Kelvin, please review.

[Sergey Ulanov, 2016-03-04 00:00:36]

Patchset #1 (id:40001) has been deleted

[davidben, 2016-03-04 19:06:36]

DEPS lgtm. I didn't look too hard at the crypto and assume you already had agl
or someone else appropriate look that over since he was involved in the
BoringSSL-side review.

https://codereview.chromium.org/1759313002/diff/60001/remoting/protocol/spake...
File remoting/protocol/spake2_authenticator.cc (right):

https://codereview.chromium.org/1759313002/diff/60001/remoting/protocol/spake...
remoting/protocol/spake2_authenticator.cc:16: #include
"third_party/boringssl/src/include/openssl/curve25519.h"
Oh right. I need to go land my CLs that switch our includes to that form. For
historical reasons, it's currently <openssl/curve25519.h> throughout the code,
but I think Ryan and I both agreed this is better (despite being a mouthful...
would be nice if it were shorter) since we get DEPS and stuff.

[kelvinp, 2016-03-04 19:30:51]

lgtm with nits.

https://codereview.chromium.org/1759313002/diff/60001/remoting/protocol/spake...
File remoting/protocol/spake2_authenticator.h (right):

https://codereview.chromium.org/1759313002/diff/60001/remoting/protocol/spake...
remoting/protocol/spake2_authenticator.h:61: bool is_server,
is_server is a bit confusing.  s/is_server/is_host/ ?

[Sergey Ulanov, 2016-03-04 23:07:23]

https://codereview.chromium.org/1759313002/diff/60001/remoting/protocol/spake...
File remoting/protocol/spake2_authenticator.h (right):

https://codereview.chromium.org/1759313002/diff/60001/remoting/protocol/spake...
remoting/protocol/spake2_authenticator.h:61: bool is_server,
On 2016/03/04 19:30:51, kelvinp wrote:
> is_server is a bit confusing.  s/is_server/is_host/ ?

Done.

[Sergey Ulanov, 2016-03-04 23:10:01]

sergeyu@chromium.org changed reviewers:
+ arnarb@chromium.org

[Sergey Ulanov, 2016-03-04 23:10:01]

+arnarb@
Arnar, can you also please take a look to make sure I'm using your SPAKE2
implementation correctly?
Thanks!

[Arnar Birgisson, 2016-03-07 23:58:09]

The API is used correctly, but the key does not authenticate the server's
certificate - which presumably is used for future authentications (?).

A man-in-the-middle can swap out the certificate in the message and thus fool
the client in future authentications that they are talking to the same server as
they did the spake2 dance with.

This can be fixed simply by including the certificate in the HMAC computation.

https://codereview.chromium.org/1759313002/diff/100001/remoting/protocol/spak...
File remoting/protocol/spake2_authenticator.cc (right):

https://codereview.chromium.org/1759313002/diff/100001/remoting/protocol/spak...
remoting/protocol/spake2_authenticator.cc:299: !hmac.Sign((from_host ? "host" :
"client") + local_id + " " + remote_id,
This must assume local_id and remote_id are either fixed length strings or
cannot contain spaces.

Safer is to prefix them with their length, rather than concatenating with a
separator.

https://codereview.chromium.org/1759313002/diff/100001/remoting/protocol/spak...
remoting/protocol/spake2_authenticator.cc:303: return result;
This verification hash should sign over the server's certificate as well.

Otherwise a MitM can trivially swap out the certificate with their own public
key without breaking the spake authentication.

[Sergey Ulanov, 2016-03-08 01:53:44]

The cert there is not really intended for authentication. See my comment below.

https://codereview.chromium.org/1759313002/diff/100001/remoting/protocol/spak...
File remoting/protocol/spake2_authenticator.cc (right):

https://codereview.chromium.org/1759313002/diff/100001/remoting/protocol/spak...
remoting/protocol/spake2_authenticator.cc:299: !hmac.Sign((from_host ? "host" :
"client") + local_id + " " + remote_id,
On 2016/03/07 23:58:09, Arnar Birgisson wrote:
> This must assume local_id and remote_id are either fixed length strings or
> cannot contain spaces.
> 
> Safer is to prefix them with their length, rather than concatenating with a
> separator.

Done.

https://codereview.chromium.org/1759313002/diff/100001/remoting/protocol/spak...
remoting/protocol/spake2_authenticator.cc:303: return result;
On 2016/03/07 23:58:09, Arnar Birgisson wrote:
> This verification hash should sign over the server's certificate as well.
> 
> Otherwise a MitM can trivially swap out the certificate with their own public
> key without breaking the spake authentication.

The auth_key_ generated by spake is passed to SslHmacChannelAuthenticator. The
key is used there to authenticate the TLS connection. It will prevent MitM in
the transport connection. That's the same mechanism we used previously with
P224EncryptedKeyExchange. See
https://code.google.com/p/chromium/codesearch#chromium/src/remoting/protocol/...
.
In the future the certificate that's passed from host to client won't be used at
all (see https://codereview.chromium.org/1739503003/). For the new webrtc-based
protocol the plan is to sign handshake using the key generated by SPAKE2.

[Sergey Ulanov, 2016-03-08 21:38:58]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-03-08 21:38:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1759313002/#ps120001
(title: " ")

[commit-bot: I haz the power, 2016-03-08 21:39:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759313002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759313002/120001

[commit-bot: I haz the power, 2016-03-08 22:15:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-08 22:15:47]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Arnar Birgisson, 2016-03-08 22:37:32]

Thanks for the clarification!

If the certificate isn't used at all, I'd suggest removing it from the message,
or authenticating it. Future CLs might assume the certificate is safe to use,
and start relying on it.

Just for my curiosity: Is spake used on every connection, or do you store the
key long-term for using with SslHmacChannelAuthenticator?

[Sergey Ulanov, 2016-03-08 22:44:38]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-03-08 22:44:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1759313002/#ps140001
(title: " ")

[commit-bot: I haz the power, 2016-03-08 22:45:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759313002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759313002/140001

[commit-bot: I haz the power, 2016-03-08 23:50:21]

Description was changed from

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will 
be enabled in host and client in a separate CL.

BUG=589698
==========

to

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will 
be enabled in host and client in a separate CL.

BUG=589698
==========

[commit-bot: I haz the power, 2016-03-08 23:50:24]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-03-08 23:52:31]

Description was changed from

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will 
be enabled in host and client in a separate CL.

BUG=589698
==========

to

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will
be enabled in host and client in a separate CL.

BUG=589698

Committed: https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}
==========

[commit-bot: I haz the power, 2016-03-08 23:52:31]

Patchset 5 (id:??) landed as
https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}

[Sergey Ulanov, 2016-03-09 18:13:15]

On 2016/03/08 22:37:32, Arnar Birgisson wrote:
> Thanks for the clarification!
> 
> If the certificate isn't used at all, I'd suggest removing it from the
message,
> or authenticating it. Future CLs might assume the certificate is safe to use,
> and start relying on it.

It will be gone once we finish switching to WebRTC-based protocol.

> 
> Just for my curiosity: Is spake used on every connection, or do you store the
> key long-term for using with SslHmacChannelAuthenticator?

SPAKE2 is used on every connection and the generated key is never stored. For
paired host/client (when user clicks "Don't ask PIN" check box) we generate a
random secret that's stored on both ends and used as password for SPAKE2.

[chrishall, 2016-03-10 05:05:15]

A revert of this CL (patchset #5 id:140001) has been created in
https://codereview.chromium.org/1778223003/ by chrishall@chromium.org.

The reason for reverting is: This patch added 2 more tests which are now failing
on Linux MSan Tests as of build #14316 

https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Test...


Spake2AuthenticatorTest.InvalidSecret (run #1):
[ RUN      ] Spake2AuthenticatorTest.InvalidSecret
==10538==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x14bb0fd in ?? third_party/modp_b64/modp_b64.cc:91:20
    #1 0xdcf2f6 in Base64Encode base/base64.cc:18:24
    #2 0x12d6e6c in EncodeBinaryValueToXml
remoting/protocol/spake2_authenticator.cc:47:3


and


Spake2AuthenticatorTest.SuccessfulAuth (run #1):
[ RUN      ] Spake2AuthenticatorTest.SuccessfulAuth
==10537==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x14bb0fd in ?? third_party/modp_b64/modp_b64.cc:91:20
    #1 0xdcf2f6 in Base64Encode base/base64.cc:18:24
    #2 0x12d6e6c in EncodeBinaryValueToXml
remoting/protocol/spake2_authenticator.cc:47:3



.

[Sergey Ulanov, 2016-03-10 22:10:35]

Description was changed from

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will
be enabled in host and client in a separate CL.

BUG=589698

Committed: https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}
==========

to

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will
be enabled in host and client in a separate CL.

BUG=589698

Committed: https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}
==========

[Sergey Ulanov, 2016-03-11 17:31:21]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2016-03-11 17:32:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1759313002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1759313002/140001

[commit-bot: I haz the power, 2016-03-11 18:21:59]

Description was changed from

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will
be enabled in host and client in a separate CL.

BUG=589698

Committed: https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}
==========

to

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will
be enabled in host and client in a separate CL.

BUG=589698

Committed: https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}
==========

[commit-bot: I haz the power, 2016-03-11 18:22:00]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-03-11 18:24:11]

Description was changed from

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will
be enabled in host and client in a separate CL.

BUG=589698

Committed: https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}
==========

to

==========
Implement authenticator based on SPAKE2 implementation in boringssl.

The new authenticator uses SPAKE2 over Curve25519. It will
be enabled in host and client in a separate CL.

BUG=589698

Committed: https://crrev.com/bf336334ba59ae7cd150e9cb36a9b248d174a4eb
Cr-Commit-Position: refs/heads/master@{#379972}

Committed: https://crrev.com/e5aa32ee4fae3a3e4d038001ca5c5ed3e6f02b1e
Cr-Commit-Position: refs/heads/master@{#380674}
==========

[commit-bot: I haz the power, 2016-03-11 18:24:12]

Patchset 5 (id:??) landed as
https://crrev.com/e5aa32ee4fae3a3e4d038001ca5c5ed3e6f02b1e
Cr-Commit-Position: refs/heads/master@{#380674}