code review 6490098: go.crypto/ssh: sanity check incoming packet length

go.crypto/ssh: sanity check incoming packet length

The check for a sensible packet length was removed a while ago
when the window size and channel packet size checks were moved
into channel.go. While the RFC suggests that any packet of size
less than uint32 -1 is valid, most implmentations limit the size
to a smaller value. OpenSSH chose 256kb, so that sounds like a
sensible default.

[dave_cheney.net, 2012-09-09 01:33:01]

Hello agl@golang.org, huin@google.com (cc: golang-dev@googlegroups.com),

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

[dave_cheney.net, 2012-09-09 01:34:31]

note: I don't think this will fix the breakage under win32. It will stop the
panic, but whatever test is running will continue to fail. For reasons I don't
understand, the 5 bytes of length+padding header appears to be corrupt on that
host.

[agl1, 2012-09-09 14:53:33]

LGTM.

The description mentions 245KB, but the code says 256KB.

(In time, this constant should probably be reflected in the MaxPacketSize that
we tell our peers when establishing channels. That appears to be 32KB today.)

[kardia, 2012-09-09 15:07:25]

I would suggest someone check that it is actually building the latest code. 
 When I build it on my windows box, either 32 or 64, it tests repeatedly 
just fine. Building from Go tip.

-Daniel


On Saturday, September 8, 2012 6:34:32 PM UTC-7, Dave Cheney wrote:
>
> note: I don't think this will fix the breakage under win32. It will stop 
> the panic, but whatever test is running will continue to fail. For 
> reasons I don't understand, the 5 bytes of length+padding header appears 
> to be corrupt on that host. 
>
> http://codereview.appspot.com/6490098/ 
>

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

On 2012/09/09 15:07:25, kardia wrote:
> I would suggest someone check that it is actually building the latest code. 
>  When I build it on my windows box, either 32 or 64, it tests repeatedly 
> just fine. Building from Go tip.

Yes, I don't know what is going on with those windows builders. Both are broken
in irreproducible  ways.

[dave_cheney.net, 2012-09-09 20:56:44]

Thank you. I have fixed the description.

[dave_cheney.net, 2012-09-09 20:57:29]

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

go.crypto/ssh: sanity check incoming packet length

The check for a sensible packet length was removed a while ago
when the window size and channel packet size checks were moved
into channel.go. While the RFC suggests that any packet of size
less than uint32 -1 is valid, most implmentations limit the size
to a smaller value. OpenSSH chose 256kb, so that sounds like a
sensible default.

R=agl, huin, kardianos
CC=golang-dev
http://codereview.appspot.com/6490098

[dave_cheney.net, 2012-09-09 23:10:07]

Interesting. 

http://build.golang.org/log/9a51d64e16e6571f01fe3f505e9f4df77cf345ca

ssh breaks on windows, but so does a nacl test. 

crypto/rand.(*rngReader).Read(0x11c7e0e0, 0x5e0374, 0x0, 0x0, 0x0, ...)
	C:/Users/ADMINI~1/AppData/Local/Temp/2/gobuilder/windows-386-ec2-9f6681feef6d/go/src/pkg/crypto/rand/rand_windows.go:38
+0x2a8

a []byte at address 0x5e0374 with zero length was passed to Read() ?

[brainman, 2012-09-09 23:50:21]

On 2012/09/09 23:10:07, dfc wrote:
> Interesting. 
> 
> http://build.golang.org/log/9a51d64e16e6571f01fe3f505e9f4df77cf345ca
> 

I agree, crypto/rand on windows should be fixed. The only question I have, if
Read into empty slice should return error?

Alex

[agl1, 2012-09-09 23:58:28]

On Sun, Sep 9, 2012 at 7:10 PM, <dave@cheney.net> wrote:
> a []byte at address 0x5e0374 with zero length was passed to Read() ?

Yes, it's expected to do a zero length read from rand.Reader. However,
it's not expected to fail.

See
https://codereview.appspot.com/6496099/diff/2001/src/pkg/crypto/rand/rand_win...


Cheers

AGL

[brainman, 2012-09-10 02:42:06]

On 2012/09/09 23:10:07, dfc wrote:
> Interesting. 
> 

These

--- FAIL: TestExitStatusNonZero (1.48 seconds)
session_test.go:44: 	Unable to handshake: ssh: invalid packet length, packet too
large
session_test.go:83: 	unable to dial remote side: EOF
--- FAIL: TestExitSignalAndStatus (1.39 seconds)
session_test.go:44: 	Unable to handshake: ssh: invalid packet length, packet too
large
session_test.go:83: 	unable to dial remote side: EOF
--- FAIL: TestUnknownExitSignal (1.50 seconds)
session_test.go:44: 	Unable to handshake: ssh: invalid packet length, packet too
large
session_test.go:83: 	unable to dial remote side: EOF
--- FAIL: TestInvalidServerMessage (1.43 seconds)
session_test.go:44: 	Unable to handshake: ssh: invalid packet length, packet too
large
session_test.go:83: 	unable to dial remote side: EOF
--- FAIL: TestServerZeroWindowAdjust (1.36 seconds)
session_test.go:44: 	Unable to handshake: ssh: invalid packet length, packet too
large
session_test.go:83: 	unable to dial remote side: EOF

still fail on builder. But these tests pass on my windows pc. If anyone have
some bright ideas who I can reproduce that, I will look into it.

Alex

[dave_cheney.net, 2012-09-10 02:45:50]

That is what I expected, both from the builder and your machine. I'm sure
problem lies within my code, but the brain damage of those builders does give me
pause. 

On 10/09/2012, at 12:42, alex.brainman@gmail.com wrote:

> On 2012/09/09 23:10:07, dfc wrote:
>> Interesting.
> 
> 
> These
> 
> --- FAIL: TestExitStatusNonZero (1.48 seconds)
> session_test.go:44:    Unable to handshake: ssh: invalid packet length,
> packet too large
> session_test.go:83:    unable to dial remote side: EOF
> --- FAIL: TestExitSignalAndStatus (1.39 seconds)
> session_test.go:44:    Unable to handshake: ssh: invalid packet length,
> packet too large
> session_test.go:83:    unable to dial remote side: EOF
> --- FAIL: TestUnknownExitSignal (1.50 seconds)
> session_test.go:44:    Unable to handshake: ssh: invalid packet length,
> packet too large
> session_test.go:83:    unable to dial remote side: EOF
> --- FAIL: TestInvalidServerMessage (1.43 seconds)
> session_test.go:44:    Unable to handshake: ssh: invalid packet length,
> packet too large
> session_test.go:83:    unable to dial remote side: EOF
> --- FAIL: TestServerZeroWindowAdjust (1.36 seconds)
> session_test.go:44:    Unable to handshake: ssh: invalid packet length,
> packet too large
> session_test.go:83:    unable to dial remote side: EOF
> 
> still fail on builder. But these tests pass on my windows pc. If anyone
> have some bright ideas who I can reproduce that, I will look into it.
> 
> Alex
> 
> http://codereview.appspot.com/6490098/

[brainman, 2012-09-10 02:50:54]

On 2012/09/10 02:45:50, dfc wrote:
> That is what I expected, both from the builder and your machine. I'm sure
> problem lies within my code, but the brain damage of those builders does give
me
> pause. 
> 

I am not clear what you point is. My concern is that same test FAILS on builder,
but PASSes on my computer. I am interested to know why.

Alex