Merge pull request #391 from rod-hynes/packet-tunnel

Packet tunnel fixes

.travis.yml

@@ -13,6 +13,7 @@ script:
 - go test -race -v ./common
 - go test -race -v ./common/osl
 - go test -race -v ./common/protocol
+- go test -race -v -run TestObfuscatedSessionTicket ./common/tls
 # TODO: enable once known race condition is addressed
 #       also, see comment below
 #- sudo -E env "PATH=$PATH" go test -race -v ./common/tun
@@ -23,6 +24,7 @@ script:
 - go test -v -covermode=count -coverprofile=common.coverprofile ./common
 - go test -v -covermode=count -coverprofile=osl.coverprofile ./common/osl
 - go test -v -covermode=count -coverprofile=protocol.coverprofile ./common/protocol
+- go test -v -covermode=count -coverprofile=tls.coverprofile -run TestObfuscatedSessionTicket ./common/tls
 # TODO: fix and reenable test, which is failing in TravisCI environment:
 # --- FAIL: TestTunneledTCPIPv4
 #    tun_test.go:226: startTestTCPClient failed: syscall.Connect failed: connection timed out

psiphon/common/tls/cipher_suites.go

@@ -407,23 +407,8 @@ const (
 	// https://tools.ietf.org/html/rfc7507.
 	TLS_FALLBACK_SCSV uint16 = 0x5600
 
-	// Psiphon suites for indistinguishable TLS.
+	// [Psiphon]
+	// TLS_..._CHACHA20_POLY1305_OLD are required for EmulateChrome.
 	TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_OLD   uint16 = 0xcc13
 	TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_OLD uint16 = 0xcc14
-	TLS_GREASE_0A0A                            uint16 = 0x1A1A
-	TLS_GREASE_1A1A                            uint16 = 0x2A2A
-	TLS_GREASE_2A2A                            uint16 = 0x3A3A
-	TLS_GREASE_3A3A                            uint16 = 0x4A4A
-	TLS_GREASE_4A4A                            uint16 = 0x5A5A
-	TLS_GREASE_5A5A                            uint16 = 0x6A6A
-	TLS_GREASE_6A6A                            uint16 = 0x7A7A
-	TLS_GREASE_7A7A                            uint16 = 0x8A8A
-	TLS_GREASE_8A8A                            uint16 = 0x9A9A
-	TLS_GREASE_9A9A                            uint16 = 0xAAAA
-	TLS_GREASE_AAAA                            uint16 = 0xBABA
-	TLS_GREASE_BABA                            uint16 = 0xCACA
-	TLS_GREASE_CACA                            uint16 = 0xDADA
-	TLS_GREASE_DADA                            uint16 = 0xEAEA
-	TLS_GREASE_EAEA                            uint16 = 0xFAFA
-	TLS_GREASE_FAFA                            uint16 = 0x0A0A
 )

psiphon/common/tls/handshake_client.go

@@ -7,12 +7,9 @@ package tls
 import (
 	"bytes"
 	"crypto"
-	"crypto/rand"
-	"crypto/sha256"
-	"math/big"
-
 	"crypto/ecdsa"
 	"crypto/rsa"
+	"crypto/sha256"
 	"crypto/subtle"
 	"crypto/x509"
 	"errors"
@@ -183,14 +180,14 @@ NextCipherSuite:
 		}
 
 		hello.supportedCurves = []CurveID{
-			CurveID(randomGREASEValue()),
+			CurveID(getGREASEValue(hello.random, greaseGroup)),
 			X25519,
 			CurveP256, // secp256r1
 			CurveP384, // secp384r1
 		}
 
 		hello.cipherSuites = []uint16{
-			randomGREASEValue(),
+			getGREASEValue(hello.random, greaseCipher),
 			TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
 			TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
 			TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
@@ -212,7 +209,7 @@ NextCipherSuite:
 			TLS_RSA_WITH_3DES_EDE_CBC_SHA,
 		}
 
-		// From: https://github.com/google/boringssl/blob/7d7554b6b3c79e707e25521e61e066ce2b996e4c/ssl/t1_lib.c#L442
+		// From: https://github.com/google/boringssl/blob/46db7af2c998cf8514d606408546d9be9699f03c/ssl/t1_lib.c#L442
 		// TODO: handle RSA-PSS (0x08)
 		hello.signatureAndHashes = []signatureAndHash{
 			{hashSHA256, signatureECDSA},
@@ -244,7 +241,7 @@ NextCipherSuite:
 
 		// In BoringSSL, the session ID is a SHA256 digest of the
 		// session ticket:
-		// https://github.com/google/boringssl/blob/33fe4a0d1406f423e7424ea7367e1d1a51c2edc1/ssl/handshake_client.c#L1901-L1908
+		// https://github.com/google/boringssl/blob/46db7af2c998cf8514d606408546d9be9699f03c/ssl/handshake_client.c#L1895-L1902
 		if session != nil {
 			hello.sessionTicket = session.sessionTicket
 			sessionId := sha256.Sum256(session.sessionTicket)
@@ -356,10 +353,20 @@ NextCipherSuite:
 	return nil
 }
 
-func randomGREASEValue() uint16 {
-	values := []uint16{0x0A0A, 0x1A1A, 0x2A2A, 0x3A3A, 0x4A4A, 0x5A5A, 0x6A6A, 0x7A7A, 0x8A8A, 0x9A9A, 0xAAAA, 0xBABA, 0xCACA, 0xDADA, 0xEAEA, 0xFAFA}
-	i, _ := rand.Int(rand.Reader, big.NewInt(int64(len(values))))
-	return values[int(i.Int64())]
+// From: https://github.com/google/boringssl/blob/46db7af2c998cf8514d606408546d9be9699f03c/ssl/internal.h#L1225-L1231
+const (
+	greaseCipher     = 0
+	greaseGroup      = 1
+	greaseExtension1 = 2
+	greaseExtension2 = 3
+)
+
+func getGREASEValue(random []byte, index int) uint16 {
+	// From: https://github.com/google/boringssl/blob/46db7af2c998cf8514d606408546d9be9699f03c/ssl/handshake_client.c#L545-L555
+	value := uint16(random[index])
+	value = (value & 0xf0) | 0x0a
+	value |= value << 8
+	return value
 }
 
 func (hs *clientHandshakeState) doFullHandshake() error {

psiphon/common/tls/handshake_messages.go

@@ -138,20 +138,21 @@ func (m *clientHelloMsg) marshal() []byte {
 	// Padding extension required for EmulateChrome.
 	// Logic from:
 	//
-	// https://github.com/google/boringssl/blob/7d7554b6b3c79e707e25521e61e066ce2b996e4c/ssl/t1_lib.c#L2803
+	// https://github.com/google/boringssl/blob/46db7af2c998cf8514d606408546d9be9699f03c/ssl/t1_lib.c#L2803
 	// https://github.com/google/boringssl/blob/master/LICENSE
-
-	unpaddedLength := length + 2 + 4*numExtensions + extensionsLength
 	paddingLength := uint16(0)
-	if unpaddedLength > 0xff && unpaddedLength < 0x200 {
-		paddingLength = 0x200 - uint16(unpaddedLength)
-		if paddingLength >= 4+1 {
-			paddingLength -= 4
-		} else {
-			paddingLength = 1
+	if m.emulateChrome {
+		unpaddedLength := length + 2 + 4*numExtensions + extensionsLength
+		if unpaddedLength > 0xff && unpaddedLength < 0x200 {
+			paddingLength = 0x200 - uint16(unpaddedLength)
+			if paddingLength >= 4+1 {
+				paddingLength -= 4
+			} else {
+				paddingLength = 1
+			}
+			extensionsLength += int(paddingLength)
+			numExtensions++
 		}
-		extensionsLength += int(paddingLength)
-		numExtensions++
 	}
 
 	if numExtensions > 0 {
@@ -393,8 +394,8 @@ func (m *clientHelloMsg) marshal() []byte {
 		// of extensions as required for EmulateChrome is handled
 		// in Conn.clientHandshae().
 
-		value := randomGREASEValue()
-		marshalGREASE(value, true)
+		greaseValue := getGREASEValue(m.random, greaseExtension1)
+		marshalGREASE(greaseValue, true)
 
 		if m.secureRenegotiationSupported {
 			marshalRenegotiationInfo()
@@ -433,11 +434,14 @@ func (m *clientHelloMsg) marshal() []byte {
 			marshalSupportedCurves()
 		}
 
-		previousGREASEValue := value
-		for value == previousGREASEValue {
-			value = randomGREASEValue()
+		previousValue := greaseValue
+		greaseValue = getGREASEValue(m.random, greaseExtension2)
+		if greaseValue == previousValue {
+			// See: https://github.com/google/boringssl/blob/46db7af2c998cf8514d606408546d9be9699f03c/ssl/t1_lib.c#L2787-L2792
+			greaseValue ^= 0x1010
 		}
-		marshalGREASE(value, false)
+
+		marshalGREASE(greaseValue, false)
 
 		if paddingLength > 0 {
 			marshalPadding(paddingLength)

psiphon/common/tls/obfuscated_test.go

@@ -37,6 +37,14 @@ import (
 // [Psiphon]
 // TestObfuscatedSessionTicket exercises the Obfuscated Session Tickets facility.
 func TestObfuscatedSessionTicket(t *testing.T) {
+	runObfuscatedSessionTicket(t, false)
+}
+
+func TestObfuscatedSessionTicketEmulateChrome(t *testing.T) {
+	runObfuscatedSessionTicket(t, true)
+}
+
+func runObfuscatedSessionTicket(t *testing.T, emulateChrome bool) {
 
 	var standardSessionTicketKey [32]byte
 	rand.Read(standardSessionTicketKey[:])
@@ -46,7 +54,8 @@ func TestObfuscatedSessionTicket(t *testing.T) {
 
 	// Note: SNI and certificate CN don't match
 	clientConfig := &Config{
-		ServerName: "www.example.com",
+		ServerName:    "www.example.com",
+		EmulateChrome: emulateChrome,
 		ClientSessionCache: NewObfuscatedClientSessionCache(
 			obfuscatedSessionTicketSharedSecret),
 	}
@@ -72,9 +81,14 @@ func TestObfuscatedSessionTicket(t *testing.T) {
 
 	result := make(chan error, 1)
 
+	listening := make(chan struct{}, 1)
+
 	go func() {
 
 		listener, err := Listen("tcp", serverAddress, serverConfig)
+		defer listener.Close()
+
+		listening <- *new(struct{})
 
 		var conn net.Conn
 		if err == nil {
@@ -103,6 +117,8 @@ func TestObfuscatedSessionTicket(t *testing.T) {
 
 	go func() {
 
+		<-listening
+
 		conn, err := Dial("tcp", serverAddress, clientConfig)
 
 		if err == nil {

psiphon/common/tun/tun.go

@@ -529,6 +529,13 @@ func (server *Server) runSessionReaper() {
 	// sessions. This action, removing the index from server.indexToSession,
 	// releases the IP addresses assigned  to the session.
 
+	// TODO: As-is, this will discard sessions for live SSH tunnels,
+	// as long as the SSH channel for such a session has been idle for
+	// a sufficient period. Should the session be retained as long as
+	// the SSH tunnel is alive (e.g., expose and call session.touch()
+	// on keepalive events)? Or is it better to free up resources held
+	// by idle sessions?
+
 	idleExpiry := server.sessionIdleExpiry()
 
 	ticker := time.NewTicker(idleExpiry / 2)

psiphon/controller.go

@@ -692,7 +692,7 @@ loop:
 
 			// Set the new tunnel as the transport for the packet tunnel. The packet tunnel
 			// client remains up when reestablishing, but no packets are relayed while there
-			// is no connected tunnel. UseNewTunnel will establish a new packet tunnel SSH
+			// is no connected tunnel. UseTunnel will establish a new packet tunnel SSH
 			// channel over the new SSH tunnel and configure the packet tunnel client to use
 			// the new SSH channel as its transport.
 			//
@@ -702,7 +702,7 @@ loop:
 			// the packet tunnel is used.
 
 			if controller.packetTunnelTransport != nil {
-				controller.packetTunnelTransport.UseNewTunnel(establishedTunnel)
+				controller.packetTunnelTransport.UseTunnel(establishedTunnel)
 			}
 
 			// TODO: design issue -- might not be enough server entries with region/caps to ever fill tunnel slots;

psiphon/packetTunnelTransport.go

@@ -71,6 +71,14 @@ func NewPacketTunnelTransport() *PacketTunnelTransport {
 		stopRunning:  stopRunning,
 		workers:      new(sync.WaitGroup),
 		channelReady: sync.NewCond(new(sync.Mutex)),
+
+		// Initialize lastReadComplete to now to avoid a false positive
+		// in monitoring: lastWriteComplete.Sub(lastReadComplete) will
+		// easily exceed PACKET_TUNNEL_PROBE_SLOW_READ when a write
+		// completes before any read. If lastReadComplete were to be
+		// used by logic other than the monitoring heuristics, this
+		// initial value may need to be revisited.
+		lastReadComplete: int64(monotime.Now()),
 	}
 
 	// The monitor worker will signal the tunnel channel when it
@@ -172,10 +180,10 @@ func (p *PacketTunnelTransport) Close() error {
 	return nil
 }
 
-// UseNewTunnel sets the PacketTunnelTransport to use a new transport channel within
-// the specified tunnel. UseNewTunnel does not block on the open channel call; it spawns
+// UseTunnel sets the PacketTunnelTransport to use a new transport channel within
+// the specified tunnel. UseTunnel does not block on the open channel call; it spawns
 // a worker that calls tunnel.DialPacketTunnelChannel and uses the resulting channel.
-func (p *PacketTunnelTransport) UseNewTunnel(tunnel *Tunnel) {
+func (p *PacketTunnelTransport) UseTunnel(tunnel *Tunnel) {
 
 	p.workers.Add(1)
 	go func(tunnel *Tunnel) {
@@ -207,7 +215,7 @@ func (p *PacketTunnelTransport) setChannel(
 	p.channelMutex.Lock()
 
 	// Concurrency note: this check is within the mutex to ensure that a
-	// UseNewTunnel call concurrent with a Close call doesn't leave a channel
+	// UseTunnel call concurrent with a Close call doesn't leave a channel
 	// set.
 	select {
 	case <-p.runContext.Done():
@@ -282,6 +290,15 @@ func (p *PacketTunnelTransport) failedChannel(
 		p.channelTunnel = nil
 	}
 	p.channelMutex.Unlock()
+
+	// Try to establish a new channel within the current tunnel. If this
+	// fails, a port forward failure probe will be triggered which will
+	// ultimately trigger a SSH keep alive probe.
+	//
+	// One case where this is necessary is when the server closes an idle
+	// packet tunnel port forward for a live SSH tunnel.
+
+	p.UseTunnel(channelTunnel)
 }
 
 func (p *PacketTunnelTransport) monitor() {

psiphon/tunnel.go

@@ -891,7 +891,17 @@ func dialSsh(
 			sshConn, sshAddress, sshClientConfig)
 		var sshClient *ssh.Client
 		if err == nil {
-			sshClient = ssh.NewClient(sshClientConn, sshChannels, nil)
+
+			// sshRequests is handled by operateTunnel.
+			// ssh.NewClient also expects to handle the sshRequests
+			// value from ssh.NewClientConn and will spawn a goroutine
+			// to handle the  <-chan *ssh.Request, so we must provide
+			// a closed channel to ensure that goroutine halts instead
+			// of hanging on a nil channel.
+			noRequests := make(chan *ssh.Request)
+			close(noRequests)
+
+			sshClient = ssh.NewClient(sshClientConn, sshChannels, noRequests)
 		}
 		resultChannel <- &sshNewClientResult{sshClient, sshRequests, err}
 	}()