From cbdb2c61b1f753834cedf2ebe68cbc335dadca52 Mon Sep 17 00:00:00 2001 From: Zeling Feng Date: Fri, 10 Nov 2023 12:19:27 -0800 Subject: [PATCH] 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 --- pkg/tcpip/ports/BUILD | 1 - pkg/tcpip/ports/ports.go | 36 +--------------- pkg/tcpip/ports/ports_test.go | 65 ----------------------------- pkg/tcpip/transport/tcp/endpoint.go | 26 +----------- 4 files changed, 2 insertions(+), 126 deletions(-) diff --git a/pkg/tcpip/ports/BUILD b/pkg/tcpip/ports/BUILD index b3f752062..fbd6ae08c 100644 --- a/pkg/tcpip/ports/BUILD +++ b/pkg/tcpip/ports/BUILD @@ -13,7 +13,6 @@ go_library( ], visibility = ["//visibility:public"], deps = [ - "//pkg/atomicbitops", "//pkg/rand", "//pkg/sync", "//pkg/tcpip", diff --git a/pkg/tcpip/ports/ports.go b/pkg/tcpip/ports/ports.go index 1642e142f..1e9040e4e 100644 --- a/pkg/tcpip/ports/ports.go +++ b/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) diff --git a/pkg/tcpip/ports/ports_test.go b/pkg/tcpip/ports/ports_test.go index c688bfd8f..49002fdb5 100644 --- a/pkg/tcpip/ports/ports_test.go +++ b/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) { diff --git a/pkg/tcpip/transport/tcp/endpoint.go b/pkg/tcpip/transport/tcp/endpoint.go index 5ce0ab9ab..7eada75e4 100644 --- a/pkg/tcpip/transport/tcp/endpoint.go +++ b/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 }