crypto/tls: support CBC ciphers

crypto/tls: support CBC ciphers

This is largely based on ality's CL 2747042.

crypto/rc4: API break in order to conform to crypto/cipher's
Stream interface

cipher/cipher: promote to the default build

Since CBC differs between TLS 1.0 and 1.1, we downgrade and
support only 1.0 at the current time. 1.0 is what most of the
world uses.

Given this CL, it would be trival to add support for AES 256,
SHA 256 etc, but I haven't in order to keep the change smaller.

[agl1, 2010-12-15 14:17:31]

rsc: I also have the utility code for generating the handshake traces in
handshake_server_test.go. (It includes a Python script). I'm not sure what to do
with them. Include it as a comment in the test? Put them in a `support'
subdirectory?

[rsc, 2010-12-15 14:28:18]

> Since CBC differs between TLS 1.0 and 1.1, we downgrade and
> support only 1.0 at the current time. 1.0 is what most of the
> world uses.

What does it mean that CBC differs?
I thought CBC was defined by NIST SP 800-38A.

[rsc, 2010-12-15 14:45:05]

> rsc: I also have the utility code for generating the handshake traces in
> handshake_server_test.go. (It includes a Python script). I'm not sure
> what to do with them. Include it as a comment in the test? Put them in a
> `support' subdirectory?

I think it's fine to put it in the same directory.
The utility code that generates the system call
files is in the syscall directory, for example.

Russ

[rsc1, 2010-12-15 14:46:15]

LGTM

http://codereview.appspot.com/3659041/diff/8001/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/Makefile#newcode34
src/pkg/Makefile:34: crypto/cipher\
move down 1 line (sort)

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/rc4/rc4.go
File src/pkg/crypto/rc4/rc4.go (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/rc4/rc4.go#new...
src/pkg/crypto/rc4/rc4.go:48: // XORKeyStream will XOR each byte of the given
buffer with a byte of the
// XORKeyStream sets dst to the result of XORing src with the key stream.
// Dst and src may be the same slice but otherwise should not overlap.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/cipher_sui...
File src/pkg/crypto/tls/cipher_suites.go (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/cipher_sui...
src/pkg/crypto/tls/cipher_suites.go:15: // A cipherSuite is a specific
combination of key agreement, cipher and MAC function. All cipher suites current
assume RSA key agreement.
please wrap comment.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/cipher_sui...
src/pkg/crypto/tls/cipher_suites.go:18: keyLen, macLen, ivLen int
maybe
keyLen int
macLen int
ivLen int

gofmt isn't quite helping here

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/cipher_sui...
src/pkg/crypto/tls/cipher_suites.go:49: if want == id {
id == want?

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:118: // CipherSuites is a list of supported cipher
suites.
blank line above, to match the rest
also add

// If CipherSuites is nil, TLS uses a list of suites supported by the
implementation.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:148: if len(s) == 0 {
There's a difference between a nil slice and a length 0 slice.
(Nil slice has 0 length but so does the non-nil []uint16{}.)
I think this should be s == nil to match, say, c.RootCAs.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go#ne...
src/pkg/crypto/tls/conn.go:172: // The length of the padded data is public, so
we can use an if here
I love security.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go#ne...
src/pkg/crypto/tls/conn.go:182: good &= ^((mask & paddingLen) ^ (mask & b))
good &^= mask&paddingLen ^ mask&b

(go simplified all the precedence stuff to just
multiplication (* / % << >> & &^) and
addition (+ - | ^) so the parens can go,
and gofmt will format the expression to
clarify the precedence for readers.
the parens make it look like some 
inversion of the precedence is here,
but there isn't one.)

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go#ne...
src/pkg/crypto/tls/conn.go:192: toRemove := (good & paddingLen) + 1
same here

toRemove := good&paddingLen + 1

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go#ne...
src/pkg/crypto/tls/conn.go:197: return a + ((b - (a % b)) % b)
can drop one or two ( )

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go#ne...
src/pkg/crypto/tls/conn.go:214: switch c := hc.cipher.(type) {
cool

[agl1, 2010-12-15 15:05:28]

On Wed, Dec 15, 2010 at 9:28 AM, Russ Cox <rsc@golang.org> wrote:
> What does it mean that CBC differs?

The CBC itself is the same but, in TLS 1.1 upwards, there's a
per-record IV. In 1.0 the IV is generated from the master secret.

(For future reference, never use CBC mode if you can avoid it.)


AGL

[agl1, 2010-12-15 16:50:38]

*** Submitted as http://code.google.com/p/go/source/detail?r=062a54b81304 ***

crypto/tls: support CBC ciphers

This is largely based on ality's CL 2747042.

crypto/rc4: API break in order to conform to crypto/cipher's
Stream interface

cipher/cipher: promote to the default build

Since CBC differs between TLS 1.0 and 1.1, we downgrade and
support only 1.0 at the current time. 1.0 is what most of the
world uses.

Given this CL, it would be trival to add support for AES 256,
SHA 256 etc, but I haven't in order to keep the change smaller.

R=rsc
CC=ality, golang-dev
http://codereview.appspot.com/3659041

Committer: Adam Langley <agl@golang.org>

http://codereview.appspot.com/3659041/diff/8001/src/pkg/Makefile
File src/pkg/Makefile (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/Makefile#newcode34
src/pkg/Makefile:34: crypto/cipher\
On 2010/12/15 14:46:15, rsc1 wrote:
> move down 1 line (sort)

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/rc4/rc4.go
File src/pkg/crypto/rc4/rc4.go (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/rc4/rc4.go#new...
src/pkg/crypto/rc4/rc4.go:48: // XORKeyStream will XOR each byte of the given
buffer with a byte of the
On 2010/12/15 14:46:15, rsc1 wrote:
> // XORKeyStream sets dst to the result of XORing src with the key stream.
> // Dst and src may be the same slice but otherwise should not overlap.

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/cipher_sui...
File src/pkg/crypto/tls/cipher_suites.go (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/cipher_sui...
src/pkg/crypto/tls/cipher_suites.go:15: // A cipherSuite is a specific
combination of key agreement, cipher and MAC function. All cipher suites current
assume RSA key agreement.
On 2010/12/15 14:46:15, rsc1 wrote:
> please wrap comment.

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/cipher_sui...
src/pkg/crypto/tls/cipher_suites.go:18: keyLen, macLen, ivLen int
On 2010/12/15 14:46:15, rsc1 wrote:
> maybe
> keyLen int
> macLen int
> ivLen int
> 
> gofmt isn't quite helping here

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/cipher_sui...
src/pkg/crypto/tls/cipher_suites.go:49: if want == id {
On 2010/12/15 14:46:15, rsc1 wrote:
> id == want?

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:118: // CipherSuites is a list of supported cipher
suites.
On 2010/12/15 14:46:15, rsc1 wrote:
> blank line above, to match the rest
> also add
> 
> // If CipherSuites is nil, TLS uses a list of suites supported by the
> implementation.

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:148: if len(s) == 0 {
On 2010/12/15 14:46:15, rsc1 wrote:
> There's a difference between a nil slice and a length 0 slice.
> (Nil slice has 0 length but so does the non-nil []uint16{}.)
> I think this should be s == nil to match, say, c.RootCAs.

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go
File src/pkg/crypto/tls/conn.go (right):

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go#ne...
src/pkg/crypto/tls/conn.go:182: good &= ^((mask & paddingLen) ^ (mask & b))
On 2010/12/15 14:46:15, rsc1 wrote:
> good &^= mask&paddingLen ^ mask&b
> 
> (go simplified all the precedence stuff to just
> multiplication (* / % << >> & &^) and
> addition (+ - | ^) so the parens can go,
> and gofmt will format the expression to
> clarify the precedence for readers.
> the parens make it look like some 
> inversion of the precedence is here,
> but there isn't one.)

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go#ne...
src/pkg/crypto/tls/conn.go:192: toRemove := (good & paddingLen) + 1
On 2010/12/15 14:46:15, rsc1 wrote:
> same here
> 
> toRemove := good&paddingLen + 1

Done.

http://codereview.appspot.com/3659041/diff/8001/src/pkg/crypto/tls/conn.go#ne...
src/pkg/crypto/tls/conn.go:197: return a + ((b - (a % b)) % b)
On 2010/12/15 14:46:15, rsc1 wrote:
> can drop one or two ( )

Done.

[agl1, 2010-12-15 16:55:00]

Submitted 062a54b81304.