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 <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>

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

common.go-e

@@ -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
 	}
 }
 

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)
+	}
+}

handshake_test.go-e

@@ -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)
+	}
+}