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

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.

[dave_cheney.net, 2012-08-11 07:18:33]

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

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

[albert.strasheim, 2012-08-11 08:02:27]

Hello

On 2012/08/11 07:18:33, dfc wrote:
> Hello mailto:gustav.paul@gmail.com, mailto:agl@golang.org,
mailto:fullung@gmail.com, mailto:huin@google.com
> (cc: mailto:golang-dev@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

[dave_cheney.net, 2012-08-11 08:06:36]

> "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.

[dave_cheney.net, 2012-08-11 08:55:25]

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

Please take another look.

[dave_cheney.net, 2012-08-11 08:55:55]

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

[agl1, 2012-08-11 16:37:37]

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 {
  ...
}

[dave_cheney.net, 2012-08-11 23:57:26]

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.

[dave_cheney.net, 2012-08-11 23:57:44]

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

Please take another look.

[dave_cheney.net, 2012-08-12 22:22:59]

*** 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