Integration changes in progress

- Add sudo option for executing network
  config commands when ambient capabilities
  are not available (Linux kernel < 4.3)
- Document os.File race condition

psiphon/common/tun/tun.go

@@ -153,6 +153,16 @@ type ServerConfig struct {
 	// Logger is used for logging events and metrics.
 	Logger common.Logger
 
+	// SudoNetworkConfigCommands specifies whether to use "sudo"
+	// when executing network configuration commands. This is required
+	// when the packet tunnel server is not run as root and when
+	// process capabilities are not available (only Linux kernel 4.3+
+	// has the required capabilities support). The host sudoers file
+	// must be configured to allow the tunnel server process user to
+	// execute the commands invoked in configureServerInterface; see
+	// the implementation for the appropriate platform.
+	SudoNetworkConfigCommands bool
+
 	// EgressInterface is the interface to which client traffic is
 	// masqueraded/NATed. For example, "eth0". If blank, a platform-
 	// appropriate default is used.
@@ -1130,6 +1140,11 @@ type ClientConfig struct {
 	// Logger is used for logging events and metrics.
 	Logger common.Logger
 
+	// SudoNetworkConfigCommands specifies whether to use "sudo"
+	// when executing network configuration commands. See description
+	// for ServerConfig.SudoNetworkConfigCommands.
+	SudoNetworkConfigCommands bool
+
 	// MTU is the packet MTU value to use; this value
 	// should be obtained from the packet tunnel server.
 	// When MTU is 0, a default value is used.
@@ -1996,6 +2011,55 @@ func (device *Device) WritePacket(packet []byte) error {
 // Close interrupts any blocking Read/Write calls and
 // tears down the tun device.
 func (device *Device) Close() error {
+
+	// TODO: dangerous data race exists until Go 1.9
+	//
+	// https://github.com/golang/go/issues/7970
+	//
+	// Unlike net.Conns, os.File doesn't use the poller and
+	// it's not correct to use Close() cannot to interrupt
+	// blocking reads and writes. This changes in Go 1.9,
+	// which changes os.File to use the poller.
+	//
+	// Severity may be high since there's a remote possibility
+	// that a Write could send a packet to wrong fd, including
+	// sending as plaintext to a network socket.
+	//
+	// As of this writing, we do not expect to put this
+	// code into production before Go 1.9 is released. Since
+	// interrupting blocking Read/Writes is necessary, the
+	// race condition is left as-is.
+	//
+	// This appears running tun_test with the race detector
+	// enabled:
+	//
+	// ==================
+	// WARNING: DATA RACE
+	// Write at 0x00c4200ce220 by goroutine 16:
+	//   os.(*file).close()
+	//       /usr/local/go/src/os/file_unix.go:143 +0x10a
+	//   os.(*File).Close()
+	//       /usr/local/go/src/os/file_unix.go:132 +0x55
+	//   _/root/psiphon-tunnel-core/psiphon/common/tun.(*Device).Close()
+	//       /root/psiphon-tunnel-core/psiphon/common/tun/tun.go:1999 +0x53
+	//   _/root/psiphon-tunnel-core/psiphon/common/tun.(*Client).Stop()
+	//       /root/psiphon-tunnel-core/psiphon/common/tun/tun.go:1314 +0x1a8
+	//   _/root/psiphon-tunnel-core/psiphon/common/tun.(*testClient).stop()
+	//       /root/psiphon-tunnel-core/psiphon/common/tun/tun_test.go:426 +0x77
+	//   _/root/psiphon-tunnel-core/psiphon/common/tun.testTunneledTCP.func1()
+	//       /root/psiphon-tunnel-core/psiphon/common/tun/tun_test.go:172 +0x550
+	//
+	// Previous read at 0x00c4200ce220 by goroutine 100:
+	//   os.(*File).Read()
+	//       /usr/local/go/src/os/file.go:98 +0x70
+	//   _/root/psiphon-tunnel-core/psiphon/common/tun.(*Device).readTunPacket()
+	//       /root/psiphon-tunnel-core/psiphon/common/tun/tun_linux.go:109 +0x84
+	//   _/root/psiphon-tunnel-core/psiphon/common/tun.(*Device).ReadPacket()
+	//       /root/psiphon-tunnel-core/psiphon/common/tun/tun.go:1974 +0x3c
+	//   _/root/psiphon-tunnel-core/psiphon/common/tun.(*Client).Start.func1()
+	//       /root/psiphon-tunnel-core/psiphon/common/tun/tun.go:1224 +0xaf
+	// ==================
+
 	return device.deviceIO.Close()
 }
 

psiphon/common/tun/tun_darwin.go

@@ -223,7 +223,7 @@ func (device *Device) writeTunPacket(packet []byte) error {
 	return nil
 }
 
-func configureSubprocessCapabilities() error {
+func configureNetworkConfigSubprocessCapabilities() error {
 	// Not supported on Darwin
 	return nil
 }
@@ -245,8 +245,9 @@ func configureServerInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ifconfig",
 		tunDeviceName,
 		IPv4Address, IPv4Address, IPv4Netmask,
@@ -261,8 +262,9 @@ func configureServerInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ifconfig",
 		tunDeviceName,
 		"inet6", IPv6Address, "prefixlen", IPv6Prefixlen)
@@ -280,16 +282,18 @@ func configureServerInterface(
 		egressInterface = DEFAULT_PUBLIC_INTERFACE_NAME
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"sysctl",
 		"net.inet.ip.forwarding=1")
 	if err != nil {
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"sysctl",
 		"net.inet6.ip6.forwarding=1")
 	if err != nil {
@@ -328,14 +332,16 @@ func configureServerInterface(
 	}).Debug("pf.conf")
 
 	// Disable first to avoid "pfctl: pf already enabled"
-	_ = runCommand(
+	_ = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"pfctl",
 		"-q",
 		"-d")
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"pfctl",
 		"-q",
 		"-e",
@@ -358,8 +364,9 @@ func configureClientInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ifconfig",
 		tunDeviceName,
 		IPv4Address, IPv4Address,
@@ -375,8 +382,9 @@ func configureClientInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ifconfig",
 		tunDeviceName,
 		"inet6", IPv6Address, "prefixlen", IPv6Prefixlen)
@@ -391,8 +399,9 @@ func configureClientInterface(
 
 		// TODO: IPv6
 
-		err = runCommand(
+		err = runNetworkConfigCommand(
 			config.Logger,
+			config.SudoNetworkConfigCommands,
 			"route",
 			"add",
 			"-ifscope", tunDeviceName,
@@ -406,7 +415,7 @@ func configureClientInterface(
 	return nil
 }
 
-func fixBindToDevice(_ common.Logger, _ string) error {
+func fixBindToDevice(_ common.Logger, _ bool, _ string) error {
 	// Not required on Darwin
 	return nil
 }

psiphon/common/tun/tun_linux.go

@@ -124,11 +124,13 @@ func (device *Device) writeTunPacket(packet []byte) error {
 	return nil
 }
 
-func configureSubprocessCapabilities() error {
+func configureNetworkConfigSubprocessCapabilities() error {
 
 	// If this process has CAP_NET_ADMIN, make it available to be inherited
 	// be child processes via ambient mechanism described here:
 	// https://github.com/torvalds/linux/commit/58319057b7847667f0c9585b9de0e8932b0fdb08
+	//
+	// The ambient mechanim is available in Linux kernel 4.3 and later.
 
 	// When using capabilities, this process should have CAP_NET_ADMIN in order
 	// to create tun devices. And the subprocess operations such as using "ifconfig"
@@ -164,8 +166,9 @@ func resetNATTables(
 	// the already unlikely event that there's still in-flight traffic when the address is
 	// recycled.
 
-	err := runCommand(
+	err := runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"conntrack",
 		"--delete",
 		"--src-nat",
@@ -189,8 +192,9 @@ func configureServerInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ifconfig",
 		tunDeviceName,
 		IPv4Address, "netmask", IPv4Netmask,
@@ -200,8 +204,9 @@ func configureServerInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ifconfig",
 		tunDeviceName,
 		"add", serverIPv6AddressCIDR)
@@ -218,8 +223,9 @@ func configureServerInterface(
 
 	// TODO: appear to not need sysctl net.ipv4.conf.[all|<device>].forwarding=1?
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"iptables",
 		"-t", "nat",
 		"-A", "POSTROUTING",
@@ -230,8 +236,9 @@ func configureServerInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ip6tables",
 		"-t", "nat",
 		"-A", "POSTROUTING",
@@ -256,8 +263,9 @@ func configureClientInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ifconfig",
 		tunDeviceName,
 		IPv4Address,
@@ -268,8 +276,9 @@ func configureClientInterface(
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		config.Logger,
+		config.SudoNetworkConfigCommands,
 		"ifconfig",
 		tunDeviceName,
 		"add", config.IPv6AddressCIDR)
@@ -301,8 +310,9 @@ func configureClientInterface(
 		// Note: use "replace" instead of "add" as route from
 		// previous run (e.g., tun_test case) may not yet be cleared.
 
-		err = runCommand(
+		err = runNetworkConfigCommand(
 			config.Logger,
+			config.SudoNetworkConfigCommands,
 			"ip",
 			"-6",
 			"route", "replace",
@@ -316,29 +326,32 @@ func configureClientInterface(
 	return nil
 }
 
-func fixBindToDevice(logger common.Logger, tunDeviceName string) error {
+func fixBindToDevice(logger common.Logger, useSudo bool, tunDeviceName string) error {
 
 	// Fix the problem described here:
 	// https://stackoverflow.com/questions/24011205/cant-perform-tcp-handshake-through-a-nat-between-two-nics-with-so-bindtodevice/
 
-	err := runCommand(
+	err := runNetworkConfigCommand(
 		logger,
+		useSudo,
 		"sysctl",
 		"net.ipv4.conf.all.accept_local=1")
 	if err != nil {
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		logger,
+		useSudo,
 		"sysctl",
 		"net.ipv4.conf.all.rp_filter=0")
 	if err != nil {
 		return common.ContextError(err)
 	}
 
-	err = runCommand(
+	err = runNetworkConfigCommand(
 		logger,
+		useSudo,
 		"sysctl",
 		fmt.Sprintf("net.ipv4.conf.%s.rp_filter=0", tunDeviceName))
 	if err != nil {

psiphon/common/tun/tun_test.go

@@ -407,7 +407,7 @@ func startTestClient(
 
 	// Configure kernel to fix issue described in fixBindToDevice
 
-	err = fixBindToDevice(logger, tunClient.device.Name())
+	err = fixBindToDevice(logger, config.SudoNetworkConfigCommands, tunClient.device.Name())
 	if err != nil {
 		return nil, fmt.Errorf("startTestClient(): fixBindToDevice failed: %s", err)
 	}

psiphon/common/tun/utils.go

@@ -28,14 +28,21 @@ import (
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 )
 
-func runCommand(logger common.Logger, name string, args ...string) error {
+// runNetworkConfigCommand execs a network config command, such as "ifconfig"
+// or "iptables". On platforms that support capabilities, the network config
+// capabilities of the current process is made available to the command
+// subprocess. Alternatively, "sudo" will be used when useSudo is true.
+func runNetworkConfigCommand(
+	logger common.Logger,
+	useSudo bool,
+	commandName string, commandArgs ...string) error {
 
 	// configureSubprocessCapabilities will set inheritable
 	// capabilities on platforms which support that (Linux).
 	// Specifically, CAP_NET_ADMIN will be transferred from
 	// this process to the child command.
 
-	err := configureSubprocessCapabilities()
+	err := configureNetworkConfigSubprocessCapabilities()
 	if err != nil {
 		return common.ContextError(err)
 	}
@@ -43,18 +50,24 @@ func runCommand(logger common.Logger, name string, args ...string) error {
 	// TODO: use CommandContext to interrupt on server shutdown?
 	// (the commands currently being issued shouldn't block...)
 
-	cmd := exec.Command(name, args...)
+	if useSudo {
+		commandArgs = append([]string{commandName}, commandArgs...)
+		commandName = "sudo"
+	}
+
+	cmd := exec.Command(commandName, commandArgs...)
 	output, err := cmd.CombinedOutput()
 
 	logger.WithContextFields(common.LogFields{
-		"command": name,
-		"args":    args,
+		"command": commandName,
+		"args":    commandArgs,
 		"output":  string(output),
 		"error":   err,
 	}).Debug("exec")
 
 	if err != nil {
-		err := fmt.Errorf("command %s %+v failed with %s", name, args, string(output))
+		err := fmt.Errorf(
+			"command %s %+v failed with %s", commandName, commandArgs, string(output))
 		return common.ContextError(err)
 	}
 	return nil