code review 117100043: crypto/tls: implement tls-unique channel binding (RFC 5...

crypto/tls: implement tls-unique channel binding (RFC 5929 section 3).
Tested against GnuTLS and Python.

[andreser, 2014-07-23 01:08:41]

Hello golang-codereviews@googlegroups.com (cc: agl@google.com,
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[agl1, 2014-07-23 01:27:46]

On 2014/07/23 01:08:41, andreser wrote:
> Hello mailto:golang-codereviews@googlegroups.com (cc: mailto:agl@google.com,
> mailto:golang-codereviews@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go

The tls-unique channel binding is broken without the master-secret fix that is
currently working though the IETF[1]. It's unclear whether we should support it
in a broken state.

[1] https://tools.ietf.org/html/draft-bhargavan-tls-session-hash-01

[andreser, 2014-07-23 18:38:18]

> The tls-unique channel binding is broken without the master-secret fix that is
> currently working though the IETF[1]. It's unclear whether we should support
it
> in a broken state.
> 
> [1] https://tools.ietf.org/html/draft-bhargavan-tls-session-hash-01

As far as I understand, channel bindings work with "original" TLS sessions but
break down when resumption is used. Given that, I see the following options:

1) Implement extended master secret support now, before this patch.
2) Wait for the standardization process to move on and implement extended master
secret support then, stalling this patch.
3) Clearly document that session resumption and channel bindings together are
not secure and apply this patch without the extended master secret fix.

(1) seems like the right thing to do, but I do not know whether it is likely
that the extended master secret fix will change significantly. Should I take a
stab at implementing it now?

[agl1, 2014-07-24 22:13:00]

On Wed, Jul 23, 2014 at 11:38 AM,  <andreser@google.com> wrote:
> (1) seems like the right thing to do, but I do not know whether it is
> likely that the extended master secret fix will change significantly.
> Should I take a stab at implementing it now?

The problem is that no extension number has been assigned yet. I asked
for an expedited assignment at the meeting last Sunday and that should
happen, but "expedited" at the IETF still takes weeks or months.
Unless you have an urgent need, I think waiting for the codepoint
assignment before doing the master_secret fix would be best.


Cheers

AGL

[ashankar, 2014-07-28 17:23:47]

> The problem is that no extension number has been assigned yet. I asked
> for an expedited assignment at the meeting last Sunday and that should
> happen, but "expedited" at the IETF still takes weeks or months.
> Unless you have an urgent need, I think waiting for the codepoint
> assignment before doing the master_secret fix would be best.

Thanks Adam.
If I understand correctly, you're saying that we hold off on this change till
after the codepoint assignment and then update this change with the
master_secret fix?
Sounds fair, though it would be nice to ensure that channel-binding doesn't have
to wait another 6+ months for Go1.5 in case the timing of the release cut (for
1.4) and the codepoint assignment don't align.
So, please do advise on how to ensure that (maybe this change goes in as-is
before the freeze on Sept 1?
https://docs.google.com/a/google.com/document/d/1sYiPs2KWhNkygrmj-kJuQ1X1uYQF...),
or if you consider the wait for 1.5 a non-issue.

Thanks.

[andreser, 2014-08-02 00:14:43]

Hello golang-codereviews@googlegroups.com, agl@golang.org, ashankar@google.com
(cc: agl@google.com, golang-codereviews@googlegroups.com),

Please take another look.

[agl1, 2014-08-04 18:42:58]

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/common...
src/pkg/crypto/tls/common.go:167: ChannelBinding             []byte             
  // tls-unique channel binding (rfc 5929 section 3)
Go comment style suggests a comment like this when splitting lines, prior to the
variable:

// TLSUnique contains the "tls-unique" channel binding value. See RFC 5929,
section 3. For resumed sessions this value will be nil because resumption does
not include enough context (see https://secure-resumption.com/#channelbindings).
This will change in future versions of Go once the TLS master-secret fix has
been standardized and implemented.

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/common...
src/pkg/crypto/tls/common.go:167: ChannelBinding             []byte             
  // tls-unique channel binding (rfc 5929 section 3)
I'd call this TLSUnique.

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/conn.g...
src/pkg/crypto/tls/conn.go:46: firstFinished []byte
Comment style requires that the comment start with the variable name. Also, we
can use an array here.

// firstFinished contains the first Finished has sent during the handshake. This
is the "tls-unique" channel binding value.
firstFinished [12]byte

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/conn.g...
src/pkg/crypto/tls/conn.go:1000: state.ChannelBinding = c.firstFinished
Will need to be:

state.TLSUnique = c.firstFinished[:]

once firstFinished is an array.

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/handsh...
File src/pkg/crypto/tls/handshake_client.go (right):

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/handsh...
src/pkg/crypto/tls/handshake_client.go:177: if c.firstFinished, err =
hs.readFinished(); err != nil {
Rather than an additional return value, one can pass in c.firstFinished[:] or
nil once firstFinished is an array. Then {read|send}Finished can copy() to it.

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/handsh...
File src/pkg/crypto/tls/handshake_server.go (right):

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/handsh...
src/pkg/crypto/tls/handshake_server.go:60: if c.firstFinished, err =
hs.sendFinished(); err != nil {
ditto.

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/tls_te...
File src/pkg/crypto/tls/tls_test.go (right):

https://codereview.appspot.com/117100043/diff/60001/src/pkg/crypto/tls/tls_te...
src/pkg/crypto/tls/tls_test.go:245: var serr error
serr is never used.

[andreser, 2014-08-04 22:38:38]

Hello golang-codereviews@googlegroups.com, agl@golang.org, ashankar@google.com
(cc: agl@google.com, golang-codereviews@googlegroups.com),

Please take another look.

[agl1, 2014-08-05 00:30:57]

LGTM with nits.

https://codereview.appspot.com/117100043/diff/80001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

https://codereview.appspot.com/117100043/diff/80001/src/pkg/crypto/tls/common...
src/pkg/crypto/tls/common.go:168: // TLSUnique contains the "tls-unique" channel
binding value. See RFC 5929,
wrap to 80 chars because most other comments here do.

https://codereview.appspot.com/117100043/diff/80001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/117100043/diff/80001/src/pkg/crypto/tls/conn.g...
src/pkg/crypto/tls/conn.go:45: // firstFinished contains the first Finished has
sent during the handshake.
80 chars

[andreser, 2014-08-05 16:51:08]

Hello golang-codereviews@googlegroups.com, agl@golang.org, ashankar@google.com
(cc: agl@google.com, golang-codereviews@googlegroups.com),

Please take another look.

[ashankar, 2014-08-05 17:01:31]

https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn....
src/pkg/crypto/tls/conn.go:45: // firstFinished contains the first Finished has
sent during the
s/has/hash/?

https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn....
src/pkg/crypto/tls/conn.go:1001: state.TLSUnique = c.firstFinished[:]
By doing this (as opposed to copying the bytes), clients of the library can
change the "firstFinished" value:

conn.ConnectionState().TLSUnique[3] = 0xA;
// for example.

Dunno if we care about that, if we do, might be safer to make a copy?
Adam?

[agl1, 2014-08-11 20:23:32]

https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn....
src/pkg/crypto/tls/conn.go:1001: state.TLSUnique = c.firstFinished[:]
On 2014/08/05 17:01:31, ashankar wrote:
> Dunno if we care about that, if we do, might be safer to make a copy?
> Adam?

They can, but making a copy for every caller would mean a lot of allocing for
something that 99.9% of callers don't care about. I think it's ok to assume that
the caller doesn't mutate the structure because, if they do, there are several
other things in here that would be a problem.

[andreser, 2014-08-11 22:01:34]

On 2014/08/11 20:23:32, agl1 wrote:
>
https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn.go
> File src/pkg/crypto/tls/conn.go (right):
> 
>
https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn....
> src/pkg/crypto/tls/conn.go:1001: state.TLSUnique = c.firstFinished[:]
> On 2014/08/05 17:01:31, ashankar wrote:
> > Dunno if we care about that, if we do, might be safer to make a copy?
> > Adam?
> 
> They can, but making a copy for every caller would mean a lot of allocing for
> something that 99.9% of callers don't care about. I think it's ok to assume
that
> the caller doesn't mutate the structure because, if they do, there are several
> other things in here that would be a problem.

Is there anything else you would like me to change here?

[agl1, 2014-08-11 23:40:50]

*** Submitted as https://code.google.com/p/go/source/detail?r=b3759654d42d ***

crypto/tls: implement tls-unique channel binding (RFC 5929 section 3).
Tested against GnuTLS and Python.

LGTM=agl
R=golang-codereviews, agl, ashankar
CC=agl, golang-codereviews
https://codereview.appspot.com/117100043

Committer: Adam Langley <agl@golang.org>

[agl1, 2014-08-11 23:41:27]

On 2014/08/11 22:01:34, andreser wrote:
> On 2014/08/11 20:23:32, agl1 wrote:
> >
>
https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn.go
> > File src/pkg/crypto/tls/conn.go (right):
> > 
> >
>
https://codereview.appspot.com/117100043/diff/100001/src/pkg/crypto/tls/conn....
> > src/pkg/crypto/tls/conn.go:1001: state.TLSUnique = c.firstFinished[:]
> > On 2014/08/05 17:01:31, ashankar wrote:
> > > Dunno if we care about that, if we do, might be safer to make a copy?
> > > Adam?
> > 
> > They can, but making a copy for every caller would mean a lot of allocing
for
> > something that 99.9% of callers don't care about. I think it's ok to assume
> that
> > the caller doesn't mutate the structure because, if they do, there are
several
> > other things in here that would be a problem.
> 
> Is there anything else you would like me to change here?

Sorry, this was waiting in a terminal for me to submit. Done.