code review 6448128: go.crypto/ssh: never send more data than maxpacket

go.crypto/ssh: never send more data than maxpacket

RFC 4254 s5.2 is clear that a client must never send a data
packet larger than the value of maximum packet supplied by the
remote side during channel setup. The client was not honoring
this value, in fact it wasn't even recording it.

Thanks to Albert Strasheim for the bug report.

[dave_cheney.net, 2012-08-08 10:57:35]

Hello agl@golang.org, fullung@gmail.com (cc: golang-dev@googlegroups.com),

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

[agl1, 2012-08-08 14:30:11]

LGTM

http://codereview.appspot.com/6448128/diff/3/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6448128/diff/3/ssh/client.go#newcode502
ssh/client.go:502: c.maxPacketSize = msg.MaxPacketSize
It might be worth checking that msg.MaxPacketSize < 2^31. Otherwise we'll end up
with a negative number further down and bad things will occur.

http://codereview.appspot.com/6448128/diff/3/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6448128/diff/3/ssh/server.go#newcode569
ssh/server.go:569: conn:          s,
I guess that any checks on MaxPacketSize would have to be duplicated here.

[albert.strasheim, 2012-08-08 14:41:08]

My test works now. Thanks.

On 2012/08/08 10:57:35, dfc wrote:
> Hello mailto:agl@golang.org, mailto:fullung@gmail.com (cc:
mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://code.google.com/p/go.crypto

[dave_cheney.net, 2012-08-09 00:20:18]

Hello agl@golang.org, fullung@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-08-09 00:21:33]

Thank you for your comments. I'll follow up with some additional CLs.

http://codereview.appspot.com/6448128/diff/3/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6448128/diff/3/ssh/client.go#newcode502
ssh/client.go:502: c.maxPacketSize = msg.MaxPacketSize
On 2012/08/08 14:30:11, agl1 wrote:
> It might be worth checking that msg.MaxPacketSize < 2^31. Otherwise we'll end
up
> with a negative number further down and bad things will occur.

I've added a TODO in both the client and the server as neither were checking
this parameter. I'll address it in a followup CL.

http://codereview.appspot.com/6448128/diff/3/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6448128/diff/3/ssh/server.go#newcode569
ssh/server.go:569: conn:          s,
On 2012/08/08 14:30:11, agl1 wrote:
> I guess that any checks on MaxPacketSize would have to be duplicated here.

See previous comment.

[dave_cheney.net, 2012-08-09 00:22:05]

*** Submitted as
http://code.google.com/p/go/source/detail?r=5140f139859b&repo=crypto ***

go.crypto/ssh: never send more data than maxpacket

RFC 4254 s5.2 is clear that a client must never send a data
packet larger than the value of maximum packet supplied by the
remote side during channel setup. The client was not honoring
this value, in fact it wasn't even recording it.

Thanks to Albert Strasheim for the bug report.

R=agl, fullung
CC=golang-dev
http://codereview.appspot.com/6448128