code review 6483052: go.crypto/ssh: Prevent server from sending too large of...

go.crypto/ssh: prevent server from sending more than maxPacket

Fixes issue 4003.

[kardia, 2012-08-24 16:42:55]

Hello agl@chromium.org, dave@cheney.net (cc: golang-dev@googlegroups.com),

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

[agl1, 2012-08-24 17:04:22]

LGTM, although hopefully dfc can take a look too.

https://codereview.appspot.com/6483052/diff/1002/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/6483052/diff/1002/ssh/channel.go#newcode354
ssh/channel.go:354: var space = uint32(min(int(c.maxPacket-(1+4+4)), len(data)))
space := ?

[kardia, 2012-08-24 18:09:14]

Hello agl@chromium.org, dave@cheney.net, agl@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2012-08-24 23:23:08]

Thank you for fixing this, there are a few more things to do before this can be
submitted.

1. address the comments below
2. add this logic to the edc write on line 246 
3. fix TestServerStdoutRespectsMaxPacketSize

http://codereview.appspot.com/6483052/diff/6001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6483052/diff/6001/ssh/channel.go#newcode354
ssh/channel.go:354: space := uint32(min(int(c.maxPacket-(1+4+4)), len(data)))
This value is defined as a constant on line 512, please move that value to the
top of the file and reference it here.

[kardia, 2012-08-24 23:38:17]

> 2. add this logic to the edc write on line 246 
Added, Thanks.

> 3. fix TestServerStdoutRespectsMaxPacketSize

I don't know what's wrong with that test. Looks like it is a client test, not a
server test.

> ssh/channel.go:354: space := uint32(min(int(c.maxPacket-(1+4+4)), len(data)))
> This value is defined as a constant on line 512, please move that value to the
> top of the file and reference it here.

And have another constant for the EDC? This seems wrong as it is message
specific, even between the std and extra channel.

[dave_cheney.net, 2012-08-24 23:52:57]

>> 3. fix TestServerStdoutRespectsMaxPacketSize
>
>
> I don't know what's wrong with that test. Looks like it is a client
> test, not a server test.

Yes, you are correct. This test will always fail because we are using
writePacket, not *chan.Write().

>> ssh/channel.go:354: space := uint32(min(int(c.maxPacket-(1+4+4)),
>
> len(data)))
>>
>> This value is defined as a constant on line 512, please move that
>
> value to the
>>
>> top of the file and reference it here.
>
>
> And have another constant for the EDC? This seems wrong as it is message
> specific, even between the std and extra channel.

Fair enough. For the moment, please follow the pattern on line 512 and
add a local constant.

>
>
> http://codereview.appspot.com/6483052/

[kardia, 2012-08-25 00:05:31]

On 2012/08/24 23:52:57, dfc wrote:
> >> 3. fix TestServerStdoutRespectsMaxPacketSize
> >
> >
> > I don't know what's wrong with that test. Looks like it is a client
> > test, not a server test.
> 
> Yes, you are correct. This test will always fail because we are using
> writePacket, not *chan.Write().

On my machine I ended up getting the following once:
 runtime error: makeslice: len out of range
on some of those tests, but not always. I'm leaving for now as I think it's out
side the scope of this CL.

> >> ssh/channel.go:354: space := uint32(min(int(c.maxPacket-(1+4+4)),
> >
> > len(data)))
> >>
> >> This value is defined as a constant on line 512, please move that
> >
> > value to the
> >>
> >> top of the file and reference it here.
> >
> >
> > And have another constant for the EDC? This seems wrong as it is message
> > specific, even between the std and extra channel.
> 
> Fair enough. For the moment, please follow the pattern on line 512 and
> add a local constant.

Done.

[dave_cheney.net, 2012-08-25 00:12:13]

> On my machine I ended up getting the following once:
>  runtime error: makeslice: len out of range
> on some of those tests, but not always. I'm leaving for now as I think
> it's out side the scope of this CL.

Yes, that one is on my dance card as
http://code.google.com/p/go/issues/detail?id=3989

[dave_cheney.net, 2012-08-25 00:14:41]

LGTM. Please address the final two comments, then I'll submit this CL.

http://codereview.appspot.com/6483052/diff/1004/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6483052/diff/1004/ssh/channel.go#newcode242
ssh/channel.go:242: const headerLength = 1 + 4 + 4 + 4 // 1 byte message type, 4
bytes remoteId, 4 bytes extended message type, 4 bytes data length
const headerLength = 13

http://codereview.appspot.com/6483052/diff/1004/ssh/channel.go#newcode354
ssh/channel.go:354: const headerLength = 1 + 4 + 4 // 1 byte message type, 4
bytes remoteId, 4 bytes data length
same as above, but 9

[dave_cheney.net, 2012-08-25 00:29:16]

Oh, sorry one more thing. Could you please fix the description, as a suggestion:

go.crypto/ssh: prevent server from sending more than maxPacket

[kardia, 2012-08-25 04:49:33]

On 2012/08/25 00:14:41, dfc wrote:
> LGTM. Please address the final two comments, then I'll submit this CL.
> 
> http://codereview.appspot.com/6483052/diff/1004/ssh/channel.go
> File ssh/channel.go (right):
> 
> http://codereview.appspot.com/6483052/diff/1004/ssh/channel.go#newcode242
> ssh/channel.go:242: const headerLength = 1 + 4 + 4 + 4 // 1 byte message type,
4
> bytes remoteId, 4 bytes extended message type, 4 bytes data length
> const headerLength = 13
> 
> http://codereview.appspot.com/6483052/diff/1004/ssh/channel.go#newcode354
> ssh/channel.go:354: const headerLength = 1 + 4 + 4 // 1 byte message type, 4
> bytes remoteId, 4 bytes data length
> same as above, but 9

Sure, done.

[dave_cheney.net, 2012-08-25 04:49:59]

Thank you. Submitting.

[dave_cheney.net, 2012-08-25 04:53:00]

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

go.crypto/ssh: prevent server from sending more than maxPacket

Fixes issue 4003.

R=agl, dave, agl
CC=golang-dev
http://codereview.appspot.com/6483052

Committer: Dave Cheney <dave@cheney.net>