Fix unconditional Return in obfs4 transport

application/lib/phantom_selector.go

@@ -1,7 +1,6 @@
 package lib
 
 import (
-	"bytes"
 	"encoding/binary"
 	"errors"
 	"fmt"
@@ -21,8 +20,9 @@ func (sc *SubnetConfig) getSubnets(seed []byte, weighted bool) []string {
 
 	if weighted {
 		// seed random with hkdf derived seed provided by client
-		seedInt, err := binary.ReadVarint(bytes.NewBuffer(seed))
-		if err != nil {
+		seedInt, n := binary.Varint(seed)
+		if n == 0 {
+			fmt.Println("failed to seed random for weighted rand")
 			return nil
 		}
 		rand.Seed(seedInt)
@@ -137,54 +137,62 @@ func (p *PhantomIPSelector) Select(seed []byte, generation uint, v6Support bool)
 		}
 	}
 
+	// Compose a list of ID Nets with min, max and network associated and count
+	// the total number of available addresses.
 	addressTotal := big.NewInt(0)
 	for _, _net := range genSubnets {
 		netMaskOnes, _ := _net.Mask.Size()
 		if ipv4net := _net.IP.To4(); ipv4net != nil {
 			_idNet := idNet{}
 			_idNet.min.Set(addressTotal)
 			addressTotal.Add(addressTotal, big.NewInt(2).Exp(big.NewInt(2), big.NewInt(int64(32-netMaskOnes)), nil))
-			addressTotal.Sub(addressTotal, big.NewInt(1))
-			_idNet.max.Set(addressTotal)
+			_idNet.max.Sub(addressTotal, big.NewInt(1))
 			_idNet.net = *_net
 			idNets = append(idNets, _idNet)
 		} else if ipv6net := _net.IP.To16(); ipv6net != nil {
 			if v6Support {
 				_idNet := idNet{}
 				_idNet.min.Set(addressTotal)
 				addressTotal.Add(addressTotal, big.NewInt(2).Exp(big.NewInt(2), big.NewInt(int64(128-netMaskOnes)), nil))
-				addressTotal.Sub(addressTotal, big.NewInt(1))
-				_idNet.max.Set(addressTotal)
+				_idNet.max.Sub(addressTotal, big.NewInt(1))
 				_idNet.net = *_net
 				idNets = append(idNets, _idNet)
 			}
 		} else {
 			return nil, fmt.Errorf("failed to parse %v", _net)
 		}
 	}
+
+	// If the total number of addresses is 0 something has gone wrong
+	if addressTotal.Cmp(big.NewInt(0)) <= 0 {
+		return nil, fmt.Errorf("no valid addresses specified")
+	}
+
+	// Pick a value using the seed in the range of between 0 and the total
+	// number of addresses.
 	id := &big.Int{}
 	id.SetBytes(seed)
-	if id.Cmp(addressTotal) > 0 {
+	if id.Cmp(addressTotal) >= 0 {
 		id.Mod(id, addressTotal)
 	}
-	if id.Cmp(addressTotal) == 0 {
-		return nil, fmt.Errorf("No valid addresses to select from")
-	}
-	if addressTotal.Cmp(big.NewInt(0)) <= 0 {
-		return nil, fmt.Errorf("No valid addresses specified")
-	}
 
+	// Find the network (ID net) that contains our random value and select a
+	// random address from that subnet.
+	// min >= id%total >= max
 	var result net.IP
 	for _, _idNet := range idNets {
-		if _idNet.max.Cmp(id) >= 0 && _idNet.min.Cmp(id) == -1 {
+		// fmt.Printf("tot:%s, seed%%tot:%s     id cmp max: %d,  id cmp min: %d %s\n", addressTotal.String(), id, _idNet.max.Cmp(id), _idNet.min.Cmp(id), _idNet.net.String())
+		if _idNet.max.Cmp(id) >= 0 && _idNet.min.Cmp(id) <= 0 {
 			result, err = SelectAddrFromSubnet(seed, &_idNet.net)
 			if err != nil {
-				return nil, fmt.Errorf("Failed to chose IP address: %v", err)
+				return nil, fmt.Errorf("failed to chose IP address: %v", err)
 			}
 		}
 	}
+
+	// We want to make it so this CANNOT happen
 	if result == nil {
-		return nil, errors.New("let's rewrite the phantom address selector")
+		return nil, errors.New("nil result should not be possible")
 	}
 	return result, nil
 }
@@ -204,14 +212,14 @@ func SelectAddrFromSubnet(seed []byte, net1 *net.IPNet) (net.IP, error) {
 		ipBigInt.SetBytes(net1.IP.To16())
 	}
 
-	seedInt, err := binary.ReadVarint(bytes.NewBuffer(seed))
-	if err != nil {
-		return nil, err
+	seedInt, n := binary.Varint(seed)
+	if n == 0 {
+		return nil, fmt.Errorf("failed to create seed ")
 	}
 
 	rand.Seed(seedInt)
 	randBytes := make([]byte, addrLen/8)
-	_, err = rand.Read(randBytes)
+	_, err := rand.Read(randBytes)
 	if err != nil {
 		return nil, err
 	}

application/lib/phantom_selector_test.go

@@ -2,6 +2,8 @@ package lib
 
 import (
 	"encoding/hex"
+	"fmt"
+	"math/rand"
 	"net"
 	"os"
 	"testing"
@@ -15,15 +17,46 @@ func TestPhantomsIPSelectionAlt(t *testing.T) {
 	seed, err := hex.DecodeString("5a87133b68da3468988a21659a12ed2ece07345c8c1a5b08459ffdea4218d12f")
 	require.Nil(t, err, "issue decoding seedStr")
 
-	//netStr := "192.122.190.0/24"
-	netStr := "2001:48a8:687f:1::/64"
-	_, net1, err := net.ParseCIDR(netStr)
-	require.Nil(t, err, "unable to parse CIDR")
+	testCases := []struct {
+		netStr   string
+		expected string
+	}{
+		{
+			netStr:   "2001:48a8:687f:1::/64",
+			expected: "2001:48a8:687f:1:5fa4:c34c:434e:ddd",
+		},
+		{
+			netStr:   "2001:48a8:687f:1::/128",
+			expected: "2001:48a8:687f:1::",
+		},
+		{
+			netStr:   "2001:48a8:687f:1::/127",
+			expected: "2001:48a8:687f:1::1",
+		},
+		{
+			netStr:   "10.0.0.0/8",
+			expected: "10.219.31.130",
+		},
+		{
+			netStr:   "10.0.0.0/32",
+			expected: "10.0.0.0",
+		},
+		{
+			netStr:   "10.0.0.0/30",
+			expected: "10.0.0.2",
+		},
+	}
+
+	for _, testCase := range testCases {
+		_, net1, err := net.ParseCIDR(testCase.netStr)
+		require.Nil(t, err, "unable to parse CIDR")
+
+		// Select address with net and seed
+		addr, err := SelectAddrFromSubnet(seed, net1)
+		require.Nil(t, err)
+		require.Equal(t, addr.String(), testCase.expected)
+	}
 
-	// Select address with predetermined gen and seed
-	addr, err := SelectAddrFromSubnet(seed, net1)
-	require.Nil(t, err)
-	require.Equal(t, addr.String(), "2001:48a8:687f:1:5fa4:c34c:434e:ddd")
 }
 
 func TestPhantomsGetSubnetsByGeneration(t *testing.T) {
@@ -67,7 +100,7 @@ func TestPhantomsSeededSelectionV4(t *testing.T) {
 
 	var newConf = &SubnetConfig{
 		WeightedSubnets: []ConjurePhantomSubnet{
-			{Weight: 9, Subnets: []string{"192.122.190.0/24", "2001:48a8:687f:1::/64"}},
+			{Weight: 9, Subnets: []string{"192.122.190.0/24", "10.0.0.0/31", "2001:48a8:687f:1::/64"}},
 			{Weight: 1, Subnets: []string{"141.219.0.0/16", "35.8.0.0/16"}},
 		},
 	}
@@ -116,3 +149,51 @@ func TestPhantomsV6OnlyFilter(t *testing.T) {
 	require.Equal(t, 2, len(testNetsParsed))
 
 }
+
+// TestPhantomsSeededSelectionV4Min ensures that minimal subnets work because
+// they re useful to test limitations (i.e. multiple clients sharing a phantom
+// address)
+func TestPhantomsSeededSelectionV4Min(t *testing.T) {
+	os.Setenv("PHANTOM_SUBNET_LOCATION", "./test/phantom_subnets_min.toml")
+	phantomSelector, err := NewPhantomIPSelector()
+	require.Nil(t, err, "Failed to create the PhantomIPSelector Object")
+
+	seed, _ := hex.DecodeString("5a87133b68ea3468988a21659a12ed2ece07345c8c1a5b08459ffdea4218d12f")
+
+	phantomAddr, err := phantomSelector.Select(seed, 1, false)
+	require.Nil(t, err)
+
+	// expectedAddr := "192.122.190.0"
+	// assert.Equal(t, expectedAddr, phantomAddr.String())
+	t.Logf("%s", phantomAddr)
+}
+
+func TestPhantomSeededSelectionFuzz(t *testing.T) {
+	phantomSelector := PhantomIPSelector{
+		Networks: make(map[uint]*SubnetConfig),
+	}
+
+	// Add generation with only one v4 subnet that has a varying mask len
+	for i := 0; i <= 32; i++ {
+		var newConf = &SubnetConfig{
+			WeightedSubnets: []ConjurePhantomSubnet{
+				{Weight: 10, Subnets: []string{"255.255.255.255/" + fmt.Sprint(i), "2001:48a8:687f:1::/64"}},
+			},
+		}
+
+		newGen := phantomSelector.AddGeneration(i, newConf)
+		var randSeed int64 = 1234
+		r := rand.New(rand.NewSource(randSeed))
+
+		var seed = make([]byte, 32)
+		for j := 0; j < 10000; j++ {
+			n, err := r.Read(seed)
+			require.Nil(t, err)
+			require.Equal(t, n, 32)
+
+			phantomAddr, err := phantomSelector.Select(seed, newGen, false)
+			require.Nil(t, err, "i=%d, j=%d, seed='%s'", i, j, hex.EncodeToString(seed))
+			require.NotNil(t, phantomAddr)
+		}
+	}
+}

application/lib/test/phantom_subnets_min.toml

@@ -0,0 +1,12 @@
+
+# USED BY:
+# * lib/phantom_selector_test.go
+# * transports/wrapping/obfs4/obfs4_test.go
+#
+# Do not change without consulting those files and ensuring tests still pass.
+[Networks]
+	[Networks.1]
+        Generation = 1
+        [[Networks.1.WeightedSubnets]]
+            Weight = 1
+            Subnets = ["192.122.190.0/32", "2001:48a8:687f:1::/128"]

application/transports/wrapping/internal/tests/tests.go

@@ -18,8 +18,14 @@ type Transport struct {
 
 var SharedSecret = []byte(`6a328b8ec2024dd92dd64332164cc0425ddbde40cb7b81e055bf7b099096d068`)
 
+// SetupPhantomConnections registers one session with the provided transport and
+// registration manager using a pre-determined kay and phantom subnet file.
 func SetupPhantomConnections(manager *dd.RegistrationManager, transport pb.TransportType) (clientToPhantom net.Conn, serverFromPhantom net.Conn, reg *dd.DecoyRegistration) {
 	testSubnetPath := os.Getenv("GOPATH") + "/src/github.com/refraction-networking/conjure/application/lib/test/phantom_subnets.toml"
+	return SetupPhantomConnectionsSecret(manager, transport, SharedSecret, testSubnetPath)
+}
+
+func SetupPhantomConnectionsSecret(manager *dd.RegistrationManager, transport pb.TransportType, sharedSecret []byte, testSubnetPath string) (clientToPhantom net.Conn, serverFromPhantom net.Conn, reg *dd.DecoyRegistration) {
 	os.Setenv("PHANTOM_SUBNET_LOCATION", testSubnetPath)
 
 	phantom := bufconn.Listen(65535)
@@ -44,7 +50,7 @@ func SetupPhantomConnections(manager *dd.RegistrationManager, transport pb.Trans
 
 	wg.Wait()
 
-	keys, err := dd.GenSharedKeys(SharedSecret)
+	keys, err := dd.GenSharedKeys(sharedSecret)
 	if err != nil {
 		log.Fatalln("failed to generate shared keys:", err)
 	}

application/transports/wrapping/obfs4/obfs4.go

@@ -3,6 +3,7 @@ package obfs4
 import (
 	"bytes"
 	"fmt"
+	"io/ioutil"
 	"net"
 
 	pt "git.torproject.org/pluggable-transports/goptlib.git"
@@ -30,18 +31,11 @@ func (Transport) WrapConnection(data *bytes.Buffer, c net.Conn, phantom net.IP,
 	var representative ntor.Representative
 	copy(representative[:ntor.RepresentativeLength], data.Bytes()[:ntor.RepresentativeLength])
 
-	// TODO: This seems to be an issue since we return unconditionally so why is it a loop?
-	// should something be checked so we continue to more registrations?
 	for _, r := range getObfs4Registrations(regManager, phantom) {
 		mark := generateMark(r.Keys.Obfs4Keys.NodeID, r.Keys.Obfs4Keys.PublicKey, &representative)
 		pos := findMarkMac(mark, data.Bytes(), ntor.RepresentativeLength+ClientMinPadLength, MaxHandshakeLength, true)
-
 		if pos == -1 {
-			// If we read up to the max handshake length and didn't find the mark, move on.
-			if data.Len() >= MaxHandshakeLength {
-				return nil, nil, transports.ErrNotTransport
-			}
-			return nil, nil, transports.ErrTryAgain
+			continue
 		}
 
 		// We found the mark in the client handshake! We found our registration!
@@ -55,7 +49,12 @@ func (Transport) WrapConnection(data *bytes.Buffer, c net.Conn, phantom net.IP,
 		args.Add("drbg-seed", seed.Hex())
 
 		t := &obfs4.Transport{}
-		factory, err := t.ServerFactory("", &args)
+		stateDir, err := ioutil.TempDir("", "")
+		if err != nil {
+			return nil, nil, fmt.Errorf("failed to create tmp-dir for WrapConn")
+		}
+
+		factory, err := t.ServerFactory(stateDir, &args)
 		if err != nil {
 			return nil, nil, fmt.Errorf("failed to create server factory: %w", err)
 		}
@@ -66,6 +65,13 @@ func (Transport) WrapConnection(data *bytes.Buffer, c net.Conn, phantom net.IP,
 		return r, wrapped, err
 	}
 
+	// If we read more than min handshake len, but less than max and didn't find
+	// the mark get more bytes until we have reached the max handshake length.
+	// If we have reached the max handshake len and didn't find it return NotTransport
+	if data.Len() < MaxHandshakeLength {
+		return nil, nil, transports.ErrTryAgain
+	}
+
 	// The only time we'll make it here is if there are no obfs4 registrations
 	// for the given phantom.
 	return nil, nil, transports.ErrNotTransport