Fix unsafe slice shuffles

psiphon/TCPConn.go

@@ -259,6 +259,7 @@ func tcpDial(ctx context.Context, addr string, config *DialConfig) (net.Conn, er
 	// Controller.establishCandidateGenerator will retry a candidate
 	// tunnel server dials.
 
+	// Don't shuffle or otherwise mutate the slice returned by ResolveIP.
 	permutedIndexes := prng.Perm(len(ipAddrs))
 
 	lastErr := errors.TraceNew("unknown error")

psiphon/UDPConn.go

@@ -118,12 +118,12 @@ func NewUDPConn(
 		// "udp4" or "udp6" was specified, so pick from either IPv4 or IPv6
 		//  candidates.
 
-		prng.Shuffle(len(ipAddrs), func(i, j int) {
-			ipAddrs[i], ipAddrs[j] = ipAddrs[j], ipAddrs[i]
-		})
-		for _, nextIPAddr := range ipAddrs {
-			if (network == "udp6") == (nextIPAddr.To4() == nil) {
-				ipAddr = nextIPAddr
+		// Don't shuffle or otherwise mutate the slice returned by ResolveIP.
+		permutedIndexes := prng.Perm(len(ipAddrs))
+
+		for _, i := range permutedIndexes {
+			if (network == "udp6") == (ipAddrs[i].To4() == nil) {
+				ipAddr = ipAddrs[i]
 				break
 			}
 		}

psiphon/common/resolver/resolver.go

@@ -473,26 +473,24 @@ func (r *Resolver) ResolveAddress(
 		return "", errors.Trace(err)
 	}
 
-	copyIPs := append([]net.IP(nil), IPs...)
-	prng.Shuffle(len(copyIPs), func(i, j int) {
-		copyIPs[i], copyIPs[j] = copyIPs[j], copyIPs[i]
-	})
+	// Don't shuffle or otherwise mutate the slice returned by ResolveIP.
+	permutedIndexes := prng.Perm(len(IPs))
 
 	index := 0
 
 	switch network {
 	case "ip4", "tcp4", "udp4":
 		index = -1
-		for i, IP := range IPs {
-			if IP.To4() != nil {
+		for _, i := range permutedIndexes {
+			if IPs[i].To4() != nil {
 				index = i
 				break
 			}
 		}
 	case "ip6", "tcp6", "udp6":
 		index = -1
-		for i, IP := range IPs {
-			if IP.To4() == nil {
+		for _, i := range permutedIndexes {
+			if IPs[i].To4() == nil {
 				index = i
 				break
 			}
@@ -625,6 +623,8 @@ func (r *Resolver) ResolveIP(
 	// logic.
 	IPs := r.getCache(hostname)
 	if IPs != nil {
+		// TODO: it would be safer to make and return a copy of the cached
+		// slice, instead of depending on all callers to not mutate the slice.
 		return IPs, nil
 	}
 

psiphon/dataStore.go

@@ -2196,13 +2196,13 @@ func SetNetworkReplayParameters[R any](networkID, replayID string, replayParams
 }
 
 // ShuffleAndGetNetworkReplayParameters takes a list of candidate objects and
-// selects one. The candidates are shuffled. The first post-shuffle candidate
-// with a valid replay record is returned, along with its replay parameters.
-// The caller provides isValidReplay which should indicate if replay
-// parameters remain valid; the caller should check for expiry and changes to
-// the underlhying tactics. When no valid replay parameters are found,
-// ShuffleAndGetNetworkReplayParameters returns a candidate and nil replay
-// parameters.
+// selects one. The candidates are considered in random order. The first
+// candidate with a valid replay record is returned, along with its replay
+// parameters. The caller provides isValidReplay which should indicate if
+// replay parameters remain valid; the caller should check for expiry and
+// changes to the underlhying tactics. When no valid replay parameters are
+// found, ShuffleAndGetNetworkReplayParameters returns a candidate and nil
+// replay parameters.
 func ShuffleAndGetNetworkReplayParameters[C, R any](
 	networkID string,
 	replayEnabled bool,
@@ -2214,11 +2214,11 @@ func ShuffleAndGetNetworkReplayParameters[C, R any](
 		return nil, nil, errors.TraceNew("no candidates")
 	}
 
-	prng.Shuffle(
-		len(candidates),
-		func(i, j int) { candidates[i], candidates[j] = candidates[j], candidates[i] })
+	// Don't shuffle or otherwise mutate the candidates slice, which may be a
+	// tactics parameter.
+	permutedIndexes := prng.Perm(len(candidates))
 
-	candidate := candidates[0]
+	candidate := candidates[permutedIndexes[0]]
 	var replay *R
 
 	if !replayEnabled {
@@ -2231,7 +2231,8 @@ func ShuffleAndGetNetworkReplayParameters[C, R any](
 
 		bucket := tx.bucket(datastoreNetworkReplayParametersBucket)
 
-		for _, c := range candidates {
+		for _, i := range permutedIndexes {
+			c := candidates[i]
 			key := makeNetworkReplayParametersKey[R](networkID, getReplayID(c))
 			value := bucket.get(key)
 			if value == nil {