net: add low-entropy, shared secret authentication protocol.

net: add low-entropy, shared secret authentication protocol.

BUG=none
TEST=crypto_unittests


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

[Wez, 2011-11-09 22:43:46]

Looks pretty good.  :)

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc
File crypto/mutual_auth.cc (right):

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode122
crypto/mutual_auth.cc:122: session_length[3] = session.size();
nit: Do we not have helper functions in crypto/, or base/, to marshal integers
into byte strings?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode154
crypto/mutual_auth.cc:154: " bad state " << state_;
Is this actually a sufficiently severe failure of the calling code to warrant a
CHECK()?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode183
crypto/mutual_auth.cc:183: return kResultFailed;
Similarly, is this actually a potentially severe failure of the calling code?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode200
crypto/mutual_auth.cc:200: p224::ScalarMult(Y, x_, &k);
nit: As for the P224 impl, I find this easier to read with blank lines before
the comment lines, between the sections.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode204
crypto/mutual_auth.cc:204: std::string client_masked_dh, server_masked_dh;
Add a comment here to indicate that we're just calculating hashes to verify the
client & server's idea of K to one another?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode227
crypto/mutual_auth.cc:227: server_hash_contents += k_str;
Consider having a static function to do these, since they are the same for
client and server, aside from the prefix.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h
File crypto/mutual_auth.h (right):

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode6
crypto/mutual_auth.h:6: #define CRYPTO_MUTUAL_AUTH_H_
"mutual auth" seems a very generic name for this, given that it implements a
specific algorithm.  How would you feel about crypto/p224_eke.*, for example?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode31
crypto/mutual_auth.h:31: //   kResultPending: another round is required. Call
GetMessage and repeat.
nit: Capitalize "another", and similarly for Failed and Success.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode37
crypto/mutual_auth.h:37: class CRYPTO_EXPORT SharedSecretMutualAuthentication {
Would EncryptedKeyExchange or something of that ilk be a better name for this?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode51
crypto/mutual_auth.h:51: };
nit: Should these be kPeerTypeClient and kPeerTypeServer?

The comment doesn't really explain to be what it _means_ to be the client, or
the server.  Why, as the caller, do I care, especially given it's mutual-auth?

If you has the PeerType actually be a p224::Point and defined kClient and
kServer via the M and N values then you could do away with |is_server_|...

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode60
crypto/mutual_auth.h:60: const base::StringPiece& session);
Are there any size limits that we should enforce on |password| or |session|, as
crypto::HMAC does, for example?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode64
crypto/mutual_auth.h:64: const std::string& GetMessage();
What happens if the caller calls this twice between ProcessMessage calls?

GSSAPI has a single process message call that both processes the message and
returns the next message to send, if any.  Would that make sense here?  (You'd
need callers to make a start-of-day call to the function with an empty message,
which is a little ugly, of course.)

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode72
crypto/mutual_auth.h:72: const std::string& error() const;
Wot no numeric error code that calling code can switch on if necessary?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode87
crypto/mutual_auth.h:87: State state_;
nit: Blank line after |state_|, please.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode91
crypto/mutual_auth.h:91: // big-endian length prefix.
nit: Are "x" and "pw" the EKE terminology?  If so then perhaps a comment above
these fields indicating that fact would give the reader context for
understanding the names.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode94
crypto/mutual_auth.h:94: // other party.
nit: If it's the expected hash value, why not |expected_hash_|?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode98
crypto/mutual_auth.h:98: std::string error_;
nit: Consider moving these implementation-detail fields to join |is_server_| and
|state_|, rather than bundling them with the EKE fields above?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc
File crypto/mutual_auth_unittest.cc (right):

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc#...
crypto/mutual_auth_unittest.cc:13: SharedSecretMutualAuthentication* server) {
This method checks that one or other side of the authentication exchange fails;
it doesn't verify that _both_ sides fail, which I think is something we should
check given the mutual-auth goal.

We also don't have a test for a malicious peer sending a scalar chosen to b0rk
us (presumably something not on the curve?)?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc#...
crypto/mutual_auth_unittest.cc:15: std::string clients_message, servers_message;
nit: Clearer to lose the 's's, since you don't have the luxury of apostrophes.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc#...
crypto/mutual_auth_unittest.cc:33:
CHECK(SharedSecretMutualAuthentication::kResultPending == client_result);
CHECK -> CHECK_EQ?

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc#...
crypto/mutual_auth_unittest.cc:40: std::string session = "bar";
nit: Consider moving the |password| and |session| constants to static const
char* |password1| ("foo"), |password2| ("foo2") and similarly |session1|,
|session2|.

[agl, 2011-11-10 17:18:19]

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc
File crypto/mutual_auth.cc (right):

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode122
crypto/mutual_auth.cc:122: session_length[3] = session.size();
On 2011/11/09 22:43:46, Wez wrote:
> nit: Do we not have helper functions in crypto/, or base/, to marshal integers
> into byte strings?

In net/ we use htonl/ntohl and memcpy. For the sake of a couple of lines I don't
think it's worth the complexity and an #if defined(OS_WINDOWS) to get the right
header.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode154
crypto/mutual_auth.cc:154: " bad state " << state_;
On 2011/11/09 22:43:46, Wez wrote:
> Is this actually a sufficiently severe failure of the calling code to warrant
a
> CHECK()?

You're the one writing the calling code so, if you think so, then that's good
enough for me. Done.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode183
crypto/mutual_auth.cc:183: return kResultFailed;
On 2011/11/09 22:43:46, Wez wrote:
> Similarly, is this actually a potentially severe failure of the calling code?

Ditto.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode200
crypto/mutual_auth.cc:200: p224::ScalarMult(Y, x_, &k);
On 2011/11/09 22:43:46, Wez wrote:
> nit: As for the P224 impl, I find this easier to read with blank lines before
> the comment lines, between the sections.

Done.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode204
crypto/mutual_auth.cc:204: std::string client_masked_dh, server_masked_dh;
On 2011/11/09 22:43:46, Wez wrote:
> Add a comment here to indicate that we're just calculating hashes to verify
the
> client & server's idea of K to one another?

Done.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.cc#newcode227
crypto/mutual_auth.cc:227: server_hash_contents += k_str;
On 2011/11/09 22:43:46, Wez wrote:
> Consider having a static function to do these, since they are the same for
> client and server, aside from the prefix.

Done.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc
File crypto/mutual_auth_unittest.cc (right):

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc#...
crypto/mutual_auth_unittest.cc:13: SharedSecretMutualAuthentication* server) {
On 2011/11/09 22:43:46, Wez wrote:
> This method checks that one or other side of the authentication exchange
fails;
> it doesn't verify that _both_ sides fail, which I think is something we should
> check given the mutual-auth goal.

Done.

> We also don't have a test for a malicious peer sending a scalar chosen to b0rk
> us (presumably something not on the curve?)?

See fuzz test, below.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc#...
crypto/mutual_auth_unittest.cc:15: std::string clients_message, servers_message;
On 2011/11/09 22:43:46, Wez wrote:
> nit: Clearer to lose the 's's, since you don't have the luxury of apostrophes.

Done.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc#...
crypto/mutual_auth_unittest.cc:33:
CHECK(SharedSecretMutualAuthentication::kResultPending == client_result);
On 2011/11/09 22:43:46, Wez wrote:
> CHECK -> CHECK_EQ?

Done.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth_unittest.cc#...
crypto/mutual_auth_unittest.cc:40: std::string session = "bar";
On 2011/11/09 22:43:46, Wez wrote:
> nit: Consider moving the |password| and |session| constants to static const
> char* |password1| ("foo"), |password2| ("foo2") and similarly |session1|,
> |session2|.

Done.

[Wez, 2011-11-11 01:51:35]

For Chromoting Me2Me, we need to be able to store |password| at the Host, in a
form that can't easily be brute-forced.  So I think we need a way to generate a
"long-lived key" at the Host, and to subsequently use that in place of the
actual password in the authentication exchange.

[agl, 2011-11-11 16:05:17]

Yep, sorry. Missed a whole file of comments.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h
File crypto/mutual_auth.h (right):

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode6
crypto/mutual_auth.h:6: #define CRYPTO_MUTUAL_AUTH_H_
On 2011/11/09 22:43:46, Wez wrote:
> "mutual auth" seems a very generic name for this, given that it implements a
> specific algorithm.  How would you feel about crypto/p224_eke.*, for example?

Done: p224_spake.cc

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode31
crypto/mutual_auth.h:31: //   kResultPending: another round is required. Call
GetMessage and repeat.
On 2011/11/09 22:43:46, Wez wrote:
> nit: Capitalize "another", and similarly for Failed and Success.

I don't think my English teacher would agree, but whatever. Done.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode37
crypto/mutual_auth.h:37: class CRYPTO_EXPORT SharedSecretMutualAuthentication {
On 2011/11/09 22:43:46, Wez wrote:
> Would EncryptedKeyExchange or something of that ilk be a better name for this?

Well, if the filename is algorithm specific then so should this be. So it's
P224EncryptedKeyExchange because P224SPAKE looked too ALLCAPS.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode51
crypto/mutual_auth.h:51: };
On 2011/11/09 22:43:46, Wez wrote:
> nit: Should these be kPeerTypeClient and kPeerTypeServer?

Done.

> The comment doesn't really explain to be what it _means_ to be the client, or
> the server.  Why, as the caller, do I care, especially given it's mutual-auth?

I think the comment is pretty clear and I certainly don't want to expose a
P224::Point in this API.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode60
crypto/mutual_auth.h:60: const base::StringPiece& session);
On 2011/11/09 22:43:46, Wez wrote:
> Are there any size limits that we should enforce on |password| or |session|,
as
> crypto::HMAC does, for example?

Nope

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode64
crypto/mutual_auth.h:64: const std::string& GetMessage();
On 2011/11/09 22:43:46, Wez wrote:
> What happens if the caller calls this twice between ProcessMessage calls?

It LOG(FATAL)s.


> GSSAPI has a single process message call that both processes the message and
> returns the next message to send, if any.  Would that make sense here?  

Seems more ugly.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode72
crypto/mutual_auth.h:72: const std::string& error() const;
On 2011/11/09 22:43:46, Wez wrote:
> Wot no numeric error code that calling code can switch on if necessary?

Nope, because the higher level code should't act differently based on the error.
This is just a debugging aid.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode87
crypto/mutual_auth.h:87: State state_;
On 2011/11/09 22:43:46, Wez wrote:
> nit: Blank line after |state_|, please.

moot after moving.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode91
crypto/mutual_auth.h:91: // big-endian length prefix.
On 2011/11/09 22:43:46, Wez wrote:
> nit: Are "x" and "pw" the EKE terminology?  If so then perhaps a comment above
> these fields indicating that fact would give the reader context for
> understanding the names.

Done.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode94
crypto/mutual_auth.h:94: // other party.
On 2011/11/09 22:43:46, Wez wrote:
> nit: If it's the expected hash value, why not |expected_hash_|?

Because it's less specific.

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode98
crypto/mutual_auth.h:98: std::string error_;
On 2011/11/09 22:43:46, Wez wrote:
> nit: Consider moving these implementation-detail fields to join |is_server_|
and
> |state_|, rather than bundling them with the EKE fields above?

Done.

[agl, 2011-11-11 16:13:30]

On Thu, Nov 10, 2011 at 8:51 PM,  <wez@chromium.org> wrote:
> For Chromoting Me2Me, we need to be able to store |password| at the Host, in
> a
> form that can't easily be brute-forced.  So I think we need a way to
> generate a
> "long-lived key" at the Host, and to subsequently use that in place of the
> actual password in the authentication exchange.

You mean password stretching? (i.e. applying a function to the
password and storing f(x), where f is an expensive function?)

You need to be clear about your threat model here. Is the aim simply
that a leak of f(x) doesn't leak x, because x may have been used
elsewhere? If is it that a leak of f(x) doesn't allow one to
authenticate with f(x)? Which is more complex.


Cheers

AGL

[Wez, 2011-11-12 00:50:23]

LGTM!

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h
File crypto/mutual_auth.h (right):

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode6
crypto/mutual_auth.h:6: #define CRYPTO_MUTUAL_AUTH_H_
On 2011/11/11 16:05:17, agl wrote:
> On 2011/11/09 22:43:46, Wez wrote:
> > "mutual auth" seems a very generic name for this, given that it implements a
> > specific algorithm.  How would you feel about crypto/p224_eke.*, for
example?
> 
> Done: p224_spake.cc

Technically SPAKE2...? ;)

http://codereview.chromium.org/8499032/diff/1/crypto/mutual_auth.h#newcode72
crypto/mutual_auth.h:72: const std::string& error() const;
On 2011/11/11 16:05:17, agl wrote:
> On 2011/11/09 22:43:46, Wez wrote:
> > Wot no numeric error code that calling code can switch on if necessary?
> 
> Nope, because the higher level code should't act differently based on the
error.
> This is just a debugging aid.

nit: Add the ", to aid debugging" to the comment, then?

[Wez, 2011-11-12 00:50:59]

Adding sergeyu@ as FYI.

[Wez, 2011-11-12 00:57:27]

On 11 November 2011 08:13, Adam Langley <agl@chromium.org> wrote:

> On Thu, Nov 10, 2011 at 8:51 PM,  <wez@chromium.org> wrote:
> > For Chromoting Me2Me, we need to be able to store |password| at the
> Host, in
> > a
> > form that can't easily be brute-forced.  So I think we need a way to
> > generate a
> > "long-lived key" at the Host, and to subsequently use that in place of
> the
> > actual password in the authentication exchange.
>
> You mean password stretching? (i.e. applying a function to the
> password and storing f(x), where f is an expensive function?)
>
> You need to be clear about your threat model here. Is the aim simply
> that a leak of f(x) doesn't leak x, because x may have been used
> elsewhere? If is it that a leak of f(x) doesn't allow one to
> authenticate with f(x)? Which is more complex.
>

The former, I think.

When we store f(x), it will be alongside the GAIA token the Host uses to
communicate with Talk, so if they have used x across multiple hosts, a
compromise of one Host could mean trivial compromise of the others.

[agl, 2011-11-15 00:16:32]

On Fri, Nov 11, 2011 at 7:57 PM, Wez <wez@chromium.org> wrote:
> When we store f(x), it will be alongside the GAIA token the Host uses to
> communicate with Talk, so if they have used x across multiple hosts, a
> compromise of one Host could mean trivial compromise of the others.

Then we need a slow hash function. There are a number to choose from:
PBKDF2, bcrypt and scrypt are the main ones in order of increasing
difficulty of brute-forcing with hardware.

scrypt would be best, but it's also complex. PBKDF2 is simple to
implement and reasonably sufficient with a high iteration count.


Cheers

AGL

[Wez, 2011-11-15 00:21:32]

On 14 November 2011 16:16, Adam Langley <agl@chromium.org> wrote:

> scrypt would be best, but it's also complex. PBKDF2 is simple to
> implement and reasonably sufficient with a high iteration count.
>

If we're happy that that will be sufficient then great.  Is there a risk
here that we're stitching together Frankenstein's Authentication Monster,
though?

[commit-bot: I haz the power, 2011-11-22 00:16:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/8499032/19001

[commit-bot: I haz the power, 2011-11-22 01:29:38]

Change committed as 111070