code review 6460075: go.crypto/ssh: improve channel max packet handling (issue 6460075)

[da...@cheney.net]

Reviewers: gpaul, agl1, albert.strasheim, huin-google,

Message:
Hello gusta...@gmail.com, a...@golang.org, ful...@gmail.com,
hu...@google.com (cc: golan...@googlegroups.com),

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


Description:
go.crypto/ssh: improve channel max packet handling

This proposal moves the check for max packet into
channel.writePacket. nb. Methods like channel.stdin.Write
still need to check that they are not passing more than
maxpacket - header worth of data.

There was some max packet handling in transport.go but it was
incorrect. This has been removed.

This proposal also cleans up session_test.go.

Please review this at http://codereview.appspot.com/6460075/

Affected files:
M ssh/channel.go
M ssh/server.go
M ssh/session_test.go
M ssh/transport.go

[ful...@gmail.com]

Hello

On 2012/08/11 07:18:33, dfc wrote:
> Hello mailto:gusta...@gmail.com, mailto:a...@golang.org,
mailto:ful...@gmail.com, mailto:hu...@google.com
> (cc: mailto:golan...@googlegroups.com),

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

I'm quite new to the SSH code, so maybe I'm on the wrong track here.

This makes me a bit nervous:

"Methods like channel.stdin.Write still need to check that they are not
passing more than maxpacket - header worth of data."

As soon as you do anything vaguely interesting like using Stdin as an
io.Writer to which you write protocol buffers, gob, or large strings,
you don't really have control over how large your writes are.

I guess you could always use io.CopyN, but I can see lots of people not
knowing to do that and having their programs blow up when handling
longer data.

Regards

Albert

http://codereview.appspot.com/6460075/

[Dave Cheney]

> "Methods like channel.stdin.Write still need to check that they are not
> passing more than maxpacket - header worth of data."
>
> As soon as you do anything vaguely interesting like using Stdin as an
> io.Writer to which you write protocol buffers, gob, or large strings,
> you don't really have control over how large your writes are.

Ahh, yes, that does need some explaining. TL;DR the contract for
stdin.Write is not changing.

What is changing is callers to channel.writePacket() need to be aware
there is a limit, and that limit is channel.maxPacket. In the case of
callers like stdin.Write, they also need to consider the 9 byte header
that precedes the data. That is also taken care of by
chanWriter.Write.

Hopefully this clarifies things.

[da...@cheney.net]

Hello gusta...@gmail.com, a...@golang.org, ful...@gmail.com,
hu...@google.com (cc: golan...@googlegroups.com),

Please take another look.


http://codereview.appspot.com/6460075/

[da...@cheney.net]

I have updated the description to clarify the restrictions on calling
writePacket.

http://codereview.appspot.com/6460075/

[a...@golang.org]

LGTM


http://codereview.appspot.com/6460075/diff/10001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6460075/diff/10001/ssh/channel.go#newcode120
ssh/channel.go:120: return fmt.Errorf("ssh: cannot write %d, maxPacket
is %d", len(b), c.maxPacket)
(nit) "ssh: cannot write %d bytes, maxPacket is %d bytes"

http://codereview.appspot.com/6460075/diff/10001/ssh/channel.go#newcode464
ssh/channel.go:464: c.maxPacket = msg.MaxPacketSize
(top-level): const headerLength = 9 (if it's not already defined
somewhere)

if msg.MaxPacketSize < headerLength {
return errors.New("ssh: invalid MaxPacketSize from peer")
}

http://codereview.appspot.com/6460075/diff/10001/ssh/channel.go#newcode492
ssh/channel.go:492: n := min(int(w.maxPacket-9), len(data))
s/9/headerLength

http://codereview.appspot.com/6460075/diff/10001/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6460075/diff/10001/ssh/server.go#newcode567
ssh/server.go:567: c := &serverChan{
if msg.MaxPacketSize < headerLength {
...
}

http://codereview.appspot.com/6460075/

[da...@cheney.net]

Thanks for your comments agl. I'll leave this review open for a while
longer to catch any additional comments.

http://codereview.appspot.com/6460075/diff/10001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6460075/diff/10001/ssh/channel.go#newcode120
ssh/channel.go:120: return fmt.Errorf("ssh: cannot write %d, maxPacket
is %d", len(b), c.maxPacket)

On 2012/08/11 16:37:37, agl1 wrote:
> (nit) "ssh: cannot write %d bytes, maxPacket is %d bytes"

Done.

http://codereview.appspot.com/6460075/diff/10001/ssh/channel.go#newcode464
ssh/channel.go:464: c.maxPacket = msg.MaxPacketSize

On 2012/08/11 16:37:37, agl1 wrote:
> (top-level): const headerLength = 9 (if it's not already defined
somewhere)

> if msg.MaxPacketSize < headerLength {
> return errors.New("ssh: invalid MaxPacketSize from peer")
> }

Done.

http://codereview.appspot.com/6460075/diff/10001/ssh/channel.go#newcode492
ssh/channel.go:492: n := min(int(w.maxPacket-9), len(data))

On 2012/08/11 16:37:37, agl1 wrote:
> s/9/headerLength

Done.

http://codereview.appspot.com/6460075/diff/10001/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6460075/diff/10001/ssh/server.go#newcode567
ssh/server.go:567: c := &serverChan{

On 2012/08/11 16:37:37, agl1 wrote:
> if msg.MaxPacketSize < headerLength {
> ...
> }

Done.

http://codereview.appspot.com/6460075/

[da...@cheney.net]

[da...@cheney.net]

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

go.crypto/ssh: improve channel max packet handling

This proposal moves the check for max packet into

channel.writePacket. Callers should be aware they cannot
pass a buffer larger than max packet. This is only a
concern to chanWriter.Write and appropriate guards are
already in place.

There was some max packet handling in transport.go but it was
incorrect. This has been removed.

This proposal also cleans up session_test.go.

R=gustav.paul, agl, fullung, huin
CC=golang-dev
http://codereview.appspot.com/6460075


http://codereview.appspot.com/6460075/