From c78caca803c95773f48a844d3dcab04b9bc4d6dd Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Tue, 4 Apr 2017 17:27:40 +0200 Subject: [PATCH] ssh: reject RekeyThresholds over MaxInt64 This fixes weirdness when users use int64(-1) as sentinel value. Also, really use cipher specific default thresholds. These were added in a59c127441a8ae2ad9b0fb300ab36a6558bba697, but weren't taking effect. Add a test. Fixes golang/go#19639 Change-Id: Ie9518a0ff12fded2fca35465abb427d7a9f84340 Reviewed-on: https://go-review.googlesource.com/39431 Run-TryBot: Han-Wen Nienhuys TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick --- ssh/common.go | 12 +++++++----- ssh/handshake_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/ssh/common.go b/ssh/common.go index aa5535e2aa..dc39e4d231 100644 --- a/ssh/common.go +++ b/ssh/common.go @@ -9,6 +9,7 @@ import ( "crypto/rand" "fmt" "io" + "math" "sync" _ "crypto/sha1" @@ -186,7 +187,7 @@ type Config struct { // The maximum number of bytes sent or received after which a // new key is negotiated. It must be at least 256. If - // unspecified, 1 gigabyte is used. + // unspecified, a size suitable for the chosen cipher is used. RekeyThreshold uint64 // The allowed key exchanges algorithms. If unspecified then a @@ -230,11 +231,12 @@ func (c *Config) SetDefaults() { } if c.RekeyThreshold == 0 { - // RFC 4253, section 9 suggests rekeying after 1G. - c.RekeyThreshold = 1 << 30 - } - if c.RekeyThreshold < minRekeyThreshold { + // cipher specific default + } else if c.RekeyThreshold < minRekeyThreshold { c.RekeyThreshold = minRekeyThreshold + } else if c.RekeyThreshold >= math.MaxInt64 { + // Avoid weirdness if somebody uses -1 as a threshold. + c.RekeyThreshold = math.MaxInt64 } } diff --git a/ssh/handshake_test.go b/ssh/handshake_test.go index 77d1aac765..51a4c5adec 100644 --- a/ssh/handshake_test.go +++ b/ssh/handshake_test.go @@ -526,3 +526,31 @@ func TestDisconnect(t *testing.T) { t.Errorf("readPacket 3 succeeded") } } + +func TestHandshakeRekeyDefault(t *testing.T) { + clientConf := &ClientConfig{ + Config: Config{ + Ciphers: []string{"aes128-ctr"}, + }, + HostKeyCallback: InsecureIgnoreHostKey(), + } + trC, trS, err := handshakePair(clientConf, "addr", false) + if err != nil { + t.Fatalf("handshakePair: %v", err) + } + defer trC.Close() + defer trS.Close() + + trC.writePacket([]byte{msgRequestSuccess, 0, 0}) + trC.Close() + + rgb := (1024 + trC.readBytesLeft) >> 30 + wgb := (1024 + trC.writeBytesLeft) >> 30 + + if rgb != 64 { + t.Errorf("got rekey after %dG read, want 64G", rgb) + } + if wgb != 64 { + t.Errorf("got rekey after %dG write, want 64G", wgb) + } +}