Port randomization fixes (#239)

Param changes so that internal session specific parameters do not persist to subsequent connects. also re-visit randomized port selection on the bidirectional registrars and override systems to respect phantom subnet random port support and client randomization options.

Co-authored-by: Mingye Chen <[email protected]>

go.mod

@@ -20,7 +20,7 @@ require (
 	github.com/pion/stun v0.6.1
 	github.com/pion/transport/v2 v2.2.3
 	github.com/refraction-networking/ed25519 v0.1.2
-	github.com/refraction-networking/gotapdance v1.7.1
+	github.com/refraction-networking/gotapdance v1.7.5-0.20231007192233-6c0352155207
 	github.com/refraction-networking/obfs4 v0.1.2
 	github.com/refraction-networking/utls v1.3.3
 	github.com/sirupsen/logrus v1.9.3
@@ -53,5 +53,3 @@ require (
 replace github.com/pion/dtls/v2 => github.com/mingyech/dtls/v2 v2.0.0
 
 replace github.com/pion/transport/v2 => github.com/mingyech/transport/v2 v2.0.0
-
-replace github.com/refraction-networking/gotapdance => github.com/refraction-networking/gotapdance v1.7.3-0.20230914182507-7d8ccbce6eb2

go.sum

@@ -69,8 +69,8 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
 github.com/refraction-networking/ed25519 v0.1.2 h1:08kJZUkAlY7a7cZGosl1teGytV+QEoNxPO7NnRvAB+g=
 github.com/refraction-networking/ed25519 v0.1.2/go.mod h1:nxYLUAYt/hmNpAh64PNSQ/tQ9gTIB89wCaGKJlRtZ9I=
-github.com/refraction-networking/gotapdance v1.7.3-0.20230914182507-7d8ccbce6eb2 h1:bjm9kj/WjSHJyEi6pxNrviGv90N7+LtlrTtE/s3KIVU=
-github.com/refraction-networking/gotapdance v1.7.3-0.20230914182507-7d8ccbce6eb2/go.mod h1:svhsxN8LGv3FSL1RKLFXG74tWEXQ8m1xb9rFEZF2abk=
+github.com/refraction-networking/gotapdance v1.7.5-0.20231007192233-6c0352155207 h1:j13eRL4998NZSroZyz5E795cCQj2E/USmHLot+nr+G8=
+github.com/refraction-networking/gotapdance v1.7.5-0.20231007192233-6c0352155207/go.mod h1:w40UUCQH18Z2pWVLAGqj6L9d3t3YpAhW4w5r0YgGptY=
 github.com/refraction-networking/obfs4 v0.1.2 h1:J842O4fGSkd2W8ogYj0KN6gqVVY+Cpqodw9qFGL7wVU=
 github.com/refraction-networking/obfs4 v0.1.2/go.mod h1:wAl/+gWiLsrcykJA3nKJHx89f5/gXGM8UKvty7+mvbM=
 github.com/refraction-networking/utls v1.3.3 h1:f/TBLX7KBciRyFH3bwupp+CE4fzoYKCirhdRcC490sw=

go.work.sum

@@ -20,6 +20,7 @@ github.com/felixge/fgprof v0.9.3/go.mod h1:RdbpDgzqYVh/T9fPELJyV7EYJuHB55UTEULNu
 github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ=
 github.com/golang/glog v1.1.0 h1:/d3pCKDPWNnvIWe0vVUpNP32qc8U3PDVxySP/y360qE=
 github.com/golang/glog v1.1.0/go.mod h1:pfYeQZ3JWZoXTV5sFc986z3HTpwQs9At6P4ImfuP3NQ=
+github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg=
 github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY=
 github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/pprof v0.0.0-20211214055906-6f57359322fd h1:1FjCyPC+syAzJ5/2S8fqdZK1R22vvA0J7JZKcuOIQ7Y=
@@ -44,6 +45,8 @@ github.com/pkg/profile v1.7.0 h1:hnbDkaNWPCLMO9wGLdBFTIZvzDrDfBM2072E1S9gJkA=
 github.com/pkg/profile v1.7.0/go.mod h1:8Uer0jas47ZQMJ7VD+OHknK4YDY07LPUC6dEvqDjvNo=
 github.com/refraction-networking/gotapdance v1.6.2 h1:pDC12wdSCE4Afh31NcNAIcjIhO6Mb/qiIscwbGOhhRA=
 github.com/refraction-networking/gotapdance v1.6.2/go.mod h1:m82n2v+yNr3+E/dttji+qO2hEjcXgIYj2szuieEMTts=
+github.com/refraction-networking/gotapdance v1.7.5-0.20231007192233-6c0352155207 h1:j13eRL4998NZSroZyz5E795cCQj2E/USmHLot+nr+G8=
+github.com/refraction-networking/gotapdance v1.7.5-0.20231007192233-6c0352155207/go.mod h1:w40UUCQH18Z2pWVLAGqj6L9d3t3YpAhW4w5r0YgGptY=
 github.com/stretchr/objx v0.5.0 h1:1zr/of2m5FGMsad5YfcqgdqdWrIhu+EBEJRhR1U7z/c=
 github.com/yuin/goldmark v1.4.13 h1:fVcFKWvrslecOb/tg+Cc05dkeYx540o0FuFt3nUVDoE=
 golang.org/x/lint v0.0.0-20200302205851-738671d3881b h1:Wh+f8QHJXR411sJR8/vRBTZ7YapZaRvUcLFFJhusH0k=

internal/transport_integration_test.go

@@ -2,6 +2,7 @@ package integration_test
 
 import (
 	"bytes"
+	"context"
 	"crypto/rand"
 	"errors"
 	"flag"
@@ -116,6 +117,9 @@ func testTransportsEndToEnd(t *testing.T, builder stationBuilder, clientTranspor
 
 		err = clientTransport.SetParams(params)
 		require.Nil(t, err)
+		err = clientTransport.Prepare(context.Background(), nil)
+		require.Nil(t, err)
+
 		protoParams, err := clientTransport.GetParams()
 		require.Nil(t, err)
 		t.Logf("running %s w/ %s", clientTransport.Name(), testParams.String())

pkg/core/interfaces/interfaces.go

@@ -33,18 +33,23 @@ type Transport interface {
 	ParseParams(data *anypb.Any) (any, error)
 
 	// SetParams allows the caller to set parameters associated with the transport, returning an
-	// error if the provided generic message is not compatible. the variadic bool parameter is used
-	// to indicate whether the client should sanity check the params or just apply them. This is
-	// useful in cases where the registrar may provide options to the client that it is able to
-	// handle, but are outside of the clients sanity checks. (see prefix transport for an example)
-	SetParams(any, ...bool) error
+	// error if the provided generic message is not compatible.
+	SetParams(any) error
+
+	// SetSessionParams allows the session to apply updated params that are only used within an
+	// individual dial, returning an error if the provided generic message is not compatible. the
+	// variadic bool parameter is used to indicate whether the client should sanity check the params
+	// or just apply them. This is useful in cases where the registrar may provide options to the
+	// client that it is able to handle, but are outside of the clients sanity checks. (see prefix
+	// transport for an example)
+	SetSessionParams(incoming *anypb.Any, unchecked ...bool) error
 
 	// Prepare lets the transport use the dialer to prepare. This is called before GetParams to let the
 	// transport prepare stuff such as nat traversal.
 	Prepare(ctx context.Context, dialer func(ctx context.Context, network, laddr, raddr string) (net.Conn, error)) error
 
 	// GetDstPort returns the destination port that the client should open the phantom connection with.
-	GetDstPort(seed []byte) (uint16, error)
+	GetDstPort(seed []byte, randomizeDstPorSupported bool) (uint16, error)
 
 	// PrepareKeys provides an opportunity for the transport to integrate the station public key
 	// as well as bytes from the deterministic random generator associated with the registration

pkg/regserver/overrides/prefix_transport.go

@@ -21,6 +21,7 @@ import (
 	"github.com/refraction-networking/conjure/pkg/transports"
 	"github.com/refraction-networking/conjure/pkg/transports/wrapping/prefix"
 	pb "github.com/refraction-networking/conjure/proto"
+	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/anypb"
 )
 
@@ -200,9 +201,16 @@ func (po *PrefixOverride) Override(reg *pb.C2SWrapper, randReader io.Reader) err
 		reg.RegistrationResponse = &pb.RegistrationResponse{}
 	}
 
-	if fields.port > 0 {
-		p := uint32(fields.port)
-		reg.RegistrationResponse.DstPort = &p
+	if reg.GetRegistrationResponse().GetPhantomsSupportPortRand() {
+		if !params.GetRandomizeDstPort() || reg.GetRegistrationResponse().GetDstPort() == 0 {
+			if fields.port > 0 {
+				reg.RegistrationResponse.DstPort = proto.Uint32(uint32(fields.port))
+			}
+		}
+	} else {
+		if reg.GetRegistrationResponse().GetDstPort() != 443 {
+			reg.RegistrationResponse.DstPort = proto.Uint32(443)
+		}
 	}
 
 	anypbParams, err := anypb.New(params)
@@ -262,9 +270,16 @@ func (rpo *RandPrefixOverride) Override(reg *pb.C2SWrapper, randReader io.Reader
 	}
 
 	port := newPrefix.DstPort(reg.GetSharedSecret())
-	if port > 0 {
-		p := uint32(port)
-		reg.RegistrationResponse.DstPort = &p
+	if reg.GetRegistrationResponse().GetPhantomsSupportPortRand() {
+		if !params.GetRandomizeDstPort() || reg.GetRegistrationResponse().GetDstPort() == 0 {
+			if port > 0 {
+				reg.RegistrationResponse.DstPort = proto.Uint32(uint32(port))
+			}
+		}
+	} else {
+		if reg.GetRegistrationResponse().GetDstPort() != 443 {
+			reg.RegistrationResponse.DstPort = proto.Uint32(443)
+		}
 	}
 
 	anypbParams, err := anypb.New(params)
@@ -318,9 +333,16 @@ func (fpo *FixedPrefixOverride) Override(reg *pb.C2SWrapper, randReader io.Reade
 	}
 
 	port := fpo.p.DstPort(reg.GetSharedSecret())
-	if port > 0 {
-		p := uint32(port)
-		reg.RegistrationResponse.DstPort = &p
+	if reg.GetRegistrationResponse().GetPhantomsSupportPortRand() {
+		if !params.GetRandomizeDstPort() || reg.GetRegistrationResponse().GetDstPort() == 0 {
+			if port > 0 {
+				reg.RegistrationResponse.DstPort = proto.Uint32(uint32(port))
+			}
+		}
+	} else {
+		if reg.GetRegistrationResponse().GetDstPort() != 443 {
+			reg.RegistrationResponse.DstPort = proto.Uint32(443)
+		}
 	}
 
 	anypbParams, err := anypb.New(params)

pkg/regserver/overrides/prefix_transport_test.go

@@ -12,6 +12,7 @@ import (
 
 	pb "github.com/refraction-networking/conjure/proto"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/anypb"
 )
 
@@ -138,7 +139,7 @@ func TestPrefixOverride_Override(t *testing.T) {
 
 	c = &pb.C2SWrapper{}
 	F := false
-	params := &pb.GenericTransportParams{}
+	params := &pb.GenericTransportParams{RandomizeDstPort: proto.Bool(false)}
 	p, err := anypb.New(params)
 	require.Nil(t, err)
 
@@ -154,9 +155,12 @@ func TestPrefixOverride_Override(t *testing.T) {
 	t.Run("registration wrong tt and params", test)
 
 	ttPrefix := pb.TransportType_Prefix
-	paramsPref, _ := anypb.New(&pb.PrefixTransportParams{})
+	paramsPref, _ := anypb.New(&pb.PrefixTransportParams{RandomizeDstPort: proto.Bool(false)})
 	c.RegistrationPayload.Transport = &ttPrefix
 	c.RegistrationPayload.TransportParams = paramsPref
+	c.RegistrationResponse = &pb.RegistrationResponse{
+		PhantomsSupportPortRand: proto.Bool(true),
+	}
 
 	out = expected{true, nil, 0, []byte{}}
 	t.Run("empty prefix override set", test)
@@ -176,6 +180,48 @@ func TestPrefixOverride_Override(t *testing.T) {
 
 	out = expected{false, nil, 1024, []byte("ABC")}
 	t.Run("select prefix with port override disabled", test)
+
+	// Phantom supports random port, but user disabled it - use default port from prefix
+	conf = strings.NewReader("100 1 0x22 22 SSH")
+	po, err = ParsePrefixes(conf)
+	require.Nil(t, err)
+	c.RegistrationResponse = &pb.RegistrationResponse{
+		PhantomsSupportPortRand: proto.Bool(true),
+	}
+	paramsPref, _ = anypb.New(&pb.PrefixTransportParams{RandomizeDstPort: proto.Bool(false)})
+	c.RegistrationPayload.TransportParams = paramsPref
+	out = expected{false, nil, 22, []byte("SSH")}
+	t.Run("select prefix with user disabled port randomization", test)
+
+	// Phantom supports random port, and user enables it - use port set in the registration response
+	c.RegistrationResponse = &pb.RegistrationResponse{
+		PhantomsSupportPortRand: proto.Bool(true),
+		DstPort:                 proto.Uint32(12345),
+	}
+	paramsPref, _ = anypb.New(&pb.PrefixTransportParams{RandomizeDstPort: proto.Bool(true)})
+	c.RegistrationPayload.TransportParams = paramsPref
+	out = expected{false, nil, 12345, []byte("SSH")}
+	t.Run("select prefix with randomization already applied", test)
+
+	// Phantom does not support random port,  user enables it - use port set in the registration response
+	c.RegistrationResponse = &pb.RegistrationResponse{
+		PhantomsSupportPortRand: proto.Bool(false),
+		DstPort:                 proto.Uint32(12345),
+	}
+	paramsPref, _ = anypb.New(&pb.PrefixTransportParams{RandomizeDstPort: proto.Bool(true)})
+	c.RegistrationPayload.TransportParams = paramsPref
+	out = expected{false, nil, 443, []byte("SSH")}
+	t.Run("select prefix with where random has been applied, but phantom doesnt support", test)
+
+	// Phantom does not support random port,  user enables it - use port set in the registration response
+	c.RegistrationResponse = &pb.RegistrationResponse{
+		PhantomsSupportPortRand: proto.Bool(false),
+		DstPort:                 proto.Uint32(12345),
+	}
+	paramsPref, _ = anypb.New(&pb.PrefixTransportParams{RandomizeDstPort: proto.Bool(false)})
+	c.RegistrationPayload.TransportParams = paramsPref
+	out = expected{false, nil, 443, []byte("SSH")}
+	t.Run("select prefix with where random has been incorrectly applied, but phantom doesnt support", test)
 }
 
 func d(s string) []byte {

pkg/regserver/regprocessor/regprocessor.go

@@ -277,7 +277,7 @@ func (p *RegProcessor) processBdReq(c2sPayload *pb.C2SWrapper) (*pb.Registration
 		return nil, ErrRegProcessFailed
 	}
 
-	phantomSubnetSupportsRandPort := false
+	phantomSubnetSupportsRandPort := true
 	if c2s.GetV4Support() {
 		p.selectorMutex.RLock()
 		defer p.selectorMutex.RUnlock()
@@ -311,7 +311,7 @@ func (p *RegProcessor) processBdReq(c2sPayload *pb.C2SWrapper) (*pb.Registration
 		}
 
 		regResp.Ipv6Addr = *phantom6.IP()
-		phantomSubnetSupportsRandPort = phantom6.SupportRandomPort()
+		phantomSubnetSupportsRandPort = phantomSubnetSupportsRandPort && phantom6.SupportRandomPort()
 	}
 
 	transportType := c2s.GetTransport()
@@ -326,23 +326,6 @@ func (p *RegProcessor) processBdReq(c2sPayload *pb.C2SWrapper) (*pb.Registration
 		return nil, fmt.Errorf("failed to parse transport parameters: %w", err)
 	}
 
-	if phantomSubnetSupportsRandPort {
-		dstPort, err := t.GetDstPort(uint(c2s.GetClientLibVersion()), cjkeys.ConjureSeed, params)
-		if err != nil {
-			return nil, fmt.Errorf("error determining destination port: %w", err)
-		}
-
-		// we have to cast to uint32 because protobuf using varint for all int / uint types and doesn't
-		// have an outward facing uint16 type.
-
-		port := uint32(dstPort)
-		regResp.DstPort = &port
-	} else {
-		port := uint32(443)
-		regResp.DstPort = &port
-
-	}
-
 	// Overrides will modify the C2SWrapper and put the updated registrationResponse inside to be
 	// forwarded to the station.
 	c2sPayload.RegistrationResponse = regResp
@@ -360,6 +343,19 @@ func (p *RegProcessor) processBdReq(c2sPayload *pb.C2SWrapper) (*pb.Registration
 		regResp = c2sPayload.GetRegistrationResponse()
 	}
 
+	if phantomSubnetSupportsRandPort {
+		dstPort, err := t.GetDstPort(uint(c2s.GetClientLibVersion()), cjkeys.ConjureSeed, params)
+		if err != nil {
+			return nil, fmt.Errorf("error determining destination port: %w", err)
+		}
+
+		// we have to cast to uint32 because protobuf using varint for all int / uint types and doesn't
+		// have an outward facing uint16 type.
+		regResp.DstPort = proto.Uint32(uint32(dstPort))
+	} else {
+		regResp.DstPort = proto.Uint32(443)
+	}
+
 	return regResp, nil
 }
 

pkg/transports/client/transports_test.go

@@ -2,12 +2,15 @@ package transports
 
 import (
 	// "net/pipe"
+	"context"
 	"testing"
 
 	cj "github.com/refraction-networking/conjure/pkg/core/interfaces"
 	"github.com/refraction-networking/conjure/pkg/transports/wrapping/min"
 	pb "github.com/refraction-networking/conjure/proto"
 	"github.com/stretchr/testify/require"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/anypb"
 )
 
 func TestTransportParameterFunctionality(t *testing.T) {
@@ -28,25 +31,52 @@ func TestTransportParameterFunctionality(t *testing.T) {
 	require.Nil(t, params)
 
 	transport.Parameters = paramsRandomize
+	err = transport.Prepare(context.Background(), nil)
+	require.Nil(t, err)
 
 	// Once params are set it returns a protobuf message that can be cast and parsed or otherwise
 	// operated upon
 	params, err = transport.GetParams()
 	require.Nil(t, err)
 	require.Equal(t, true, params.(*pb.GenericTransportParams).GetRandomizeDstPort())
 
-	// We can then set the parameters if the proper parameters structure is provided even using
-	// the generic transport interface.
+	// We can then set the parameters if the proper parameters structure is provided even using the
+	// generic transport interface. This should only be done on init (i.e in
+	// NewTransport{FromID, FromName, etc.}) as it will not change the params used in active session
+	// unless Prepare is called again.
 	var gt cj.Transport = transport
 	err = gt.SetParams(paramsStatic)
 	require.Nil(t, err)
 
 	// The updated parameters are reflected when we get the parameters,  again returning a protobuf
 	// message that can be cast and parsed or otherwise operated upon.
+	params, err = transport.GetParams()
+	require.Nil(t, err)
+	require.Equal(t, true, params.(*pb.GenericTransportParams).GetRandomizeDstPort())
+
+	err = transport.Prepare(context.Background(), nil)
+	require.Nil(t, err)
+
 	params, err = transport.GetParams()
 	require.Nil(t, err)
 	require.Equal(t, false, params.(*pb.GenericTransportParams).GetRandomizeDstPort())
 
+	// We can then set the parameters if the proper parameters structure is provided even using the
+	// generic transport interface. This should only be done on init (i.e in
+	// NewTransport{FromID, FromName, etc.}) as it will not change the params used in active session
+	// unless Prepare is called again.
+	gt = transport
+	sp, err := anypb.New(&pb.GenericTransportParams{RandomizeDstPort: proto.Bool(true)})
+	require.Nil(t, err)
+	err = gt.SetSessionParams(sp)
+	require.Nil(t, err)
+
+	// The updated parameters are reflected when we get the parameters,  again returning a protobuf
+	// message that can be cast and parsed or otherwise operated upon.
+	params, err = transport.GetParams()
+	require.Nil(t, err)
+	require.Equal(t, true, params.(*pb.GenericTransportParams).GetRandomizeDstPort())
+
 	// if an improper object (any) is provided the cast in min will fail and SetParams will return
 	// an error.
 	badParams := struct{}{}