Randomize TCP source port selection

Drop PickEphemeralPortStable which uses a hash-based algorithm from RFC 6056,
instead just use PickEphemeralPort that uses a randomized algorithm from RFC
6056 to avoid potential security concerns.

Discovered by Inon Kaplan (PhD candidate in the Hebrew University School of
Computer Science and Engineering), Ron Even (BSc graduate of Bar Ilan
University) and Amit Klein (faculty member in the Hebrew University School of
Computer Science and Engineering). Details will be provided in their paper, to
be presented in a forthcoming academic conference.

PiperOrigin-RevId: 581334218

Author: zeling

pkg/tcpip/ports/BUILD

@@ -13,7 +13,6 @@ go_library(
     ],
     visibility = ["//visibility:public"],
     deps = [
-        "//pkg/atomicbitops",
         "//pkg/rand",
         "//pkg/sync",
         "//pkg/tcpip",

pkg/tcpip/ports/ports.go

@@ -19,7 +19,6 @@ package ports
 import (
 	"math"
 
-	"gvisor.dev/gvisor/pkg/atomicbitops"
 	"gvisor.dev/gvisor/pkg/rand"
 	"gvisor.dev/gvisor/pkg/sync"
 	"gvisor.dev/gvisor/pkg/tcpip"
@@ -228,13 +227,6 @@ type PortManager struct {
 	ephemeralMu    sync.RWMutex
 	firstEphemeral uint16
 	numEphemeral   uint16
-
-	// hint is used to pick ports ephemeral ports in a stable order for
-	// a given port offset.
-	//
-	// hint must be accessed using the portHint/incPortHint helpers.
-	// TODO(gvisor.dev/issue/940): S/R this field.
-	hint atomicbitops.Uint32
 }
 
 // NewPortManager creates new PortManager.
@@ -264,38 +256,12 @@ func (pm *PortManager) PickEphemeralPort(rng rand.RNG, testPort PortTester) (por
 	return pickEphemeralPort(rng.Uint32(), firstEphemeral, numEphemeral, testPort)
 }
 
-// portHint atomically reads and returns the pm.hint value.
-func (pm *PortManager) portHint() uint32 {
-	return pm.hint.Load()
-}
-
-// incPortHint atomically increments pm.hint by 1.
-func (pm *PortManager) incPortHint() {
-	pm.hint.Add(1)
-}
-
-// PickEphemeralPortStable starts at the specified offset + pm.portHint and
-// iterates over all ephemeral ports, allowing the caller to decide whether a
-// given port is suitable for its needs and stopping when a port is found or an
-// error occurs.
-func (pm *PortManager) PickEphemeralPortStable(offset uint32, testPort PortTester) (port uint16, err tcpip.Error) {
-	pm.ephemeralMu.RLock()
-	firstEphemeral := pm.firstEphemeral
-	numEphemeral := pm.numEphemeral
-	pm.ephemeralMu.RUnlock()
-
-	p, err := pickEphemeralPort(pm.portHint()+offset, firstEphemeral, numEphemeral, testPort)
-	if err == nil {
-		pm.incPortHint()
-	}
-	return p, err
-}
-
 // pickEphemeralPort starts at the offset specified from the FirstEphemeral port
 // and iterates over the number of ports specified by count and allows the
 // caller to decide whether a given port is suitable for its needs, and stopping
 // when a port is found or an error occurs.
 func pickEphemeralPort(offset uint32, first, count uint16, testPort PortTester) (port uint16, err tcpip.Error) {
+	// This implements Algorithm 1 as per RFC 6056 Section 3.3.1.
 	for i := uint32(0); i < uint32(count); i++ {
 		port := uint16(uint32(first) + (offset+i)%uint32(count))
 		ok, err := testPort(port)

pkg/tcpip/ports/ports_test.go

@@ -16,7 +16,6 @@ package ports
 
 import (
 	"math"
-	"math/rand"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -434,70 +433,6 @@ func TestPickEphemeralPort(t *testing.T) {
 	}
 }
 
-func TestPickEphemeralPortStable(t *testing.T) {
-	const (
-		firstEphemeral    = 32000
-		numEphemeralPorts = 1000
-	)
-
-	for _, test := range []struct {
-		name     string
-		f        func(port uint16) (bool, tcpip.Error)
-		wantErr  tcpip.Error
-		wantPort uint16
-	}{
-		{
-			name: "no-port-available",
-			f: func(port uint16) (bool, tcpip.Error) {
-				return false, nil
-			},
-			wantErr: &tcpip.ErrNoPortAvailable{},
-		},
-		{
-			name: "port-tester-error",
-			f: func(port uint16) (bool, tcpip.Error) {
-				return false, &tcpip.ErrBadBuffer{}
-			},
-			wantErr: &tcpip.ErrBadBuffer{},
-		},
-		{
-			name: "only-port-16042-available",
-			f: func(port uint16) (bool, tcpip.Error) {
-				if port == firstEphemeral+42 {
-					return true, nil
-				}
-				return false, nil
-			},
-			wantPort: firstEphemeral + 42,
-		},
-		{
-			name: "only-port-under-16000-available",
-			f: func(port uint16) (bool, tcpip.Error) {
-				if port < firstEphemeral {
-					return true, nil
-				}
-				return false, nil
-			},
-			wantErr: &tcpip.ErrNoPortAvailable{},
-		},
-	} {
-		t.Run(test.name, func(t *testing.T) {
-			pm := NewPortManager()
-			if err := pm.SetPortRange(firstEphemeral, firstEphemeral+numEphemeralPorts); err != nil {
-				t.Fatalf("failed to set ephemeral port range: %s", err)
-			}
-			portOffset := uint32(rand.Int31n(int32(numEphemeralPorts)))
-			port, err := pm.PickEphemeralPortStable(portOffset, test.f)
-			if diff := cmp.Diff(test.wantErr, err); diff != "" {
-				t.Fatalf("unexpected error from PickEphemeralPort(..), (-want, +got):\n%s", diff)
-			}
-			if port != test.wantPort {
-				t.Errorf("got PickEphemeralPort(..) = (%d, nil); want (%d, nil)", port, test.wantPort)
-			}
-		})
-	}
-}
-
 // TestOverflow addresses b/183593432, wherein an overflowing uint16 causes a
 // port allocation failure.
 func TestOverflow(t *testing.T) {

pkg/tcpip/transport/tcp/endpoint.go

@@ -16,7 +16,6 @@ package tcp
 
 import (
 	"container/heap"
-	"encoding/binary"
 	"fmt"
 	"io"
 	"math"
@@ -29,7 +28,6 @@ import (
 	"gvisor.dev/gvisor/pkg/sleep"
 	"gvisor.dev/gvisor/pkg/sync"
 	"gvisor.dev/gvisor/pkg/tcpip"
-	"gvisor.dev/gvisor/pkg/tcpip/hash/jenkins"
 	"gvisor.dev/gvisor/pkg/tcpip/header"
 	"gvisor.dev/gvisor/pkg/tcpip/ports"
 	"gvisor.dev/gvisor/pkg/tcpip/seqnum"
@@ -2246,28 +2244,6 @@ func (e *endpoint) registerEndpoint(addr tcpip.FullAddress, netProto tcpip.Netwo
 		// endpoint would be trying to connect to itself).
 		sameAddr := e.TransportEndpointInfo.ID.LocalAddress == e.TransportEndpointInfo.ID.RemoteAddress
 
-		// Calculate a port offset based on the destination IP/port and
-		// src IP to ensure that for a given tuple (srcIP, destIP,
-		// destPort) the offset used as a starting point is the same to
-		// ensure that we can cycle through the port space effectively.
-		portBuf := make([]byte, 2)
-		binary.LittleEndian.PutUint16(portBuf, e.ID.RemotePort)
-
-		h := jenkins.Sum32(e.protocol.portOffsetSecret)
-		for _, s := range [][]byte{
-			e.ID.LocalAddress.AsSlice(),
-			e.ID.RemoteAddress.AsSlice(),
-			portBuf,
-		} {
-			// Per io.Writer.Write:
-			//
-			// Write must return a non-nil error if it returns n < len(p).
-			if _, err := h.Write(s); err != nil {
-				panic(err)
-			}
-		}
-		portOffset := h.Sum32()
-
 		var twReuse tcpip.TCPTimeWaitReuseOption
 		if err := e.stack.TransportProtocolOption(ProtocolNumber, &twReuse); err != nil {
 			panic(fmt.Sprintf("e.stack.TransportProtocolOption(%d, %#v) = %s", ProtocolNumber, &twReuse, err))
@@ -2284,7 +2260,7 @@ func (e *endpoint) registerEndpoint(addr tcpip.FullAddress, netProto tcpip.Netwo
 		}
 
 		bindToDevice := tcpip.NICID(e.ops.GetBindToDevice())
-		if _, err := e.stack.PickEphemeralPortStable(portOffset, func(p uint16) (bool, tcpip.Error) {
+		if _, err := e.stack.PickEphemeralPort(e.stack.SecureRNG(), func(p uint16) (bool, tcpip.Error) {
 			if sameAddr && p == e.TransportEndpointInfo.ID.RemotePort {
 				return false, nil
 			}