service: fix sameuser check (#2642)

Change the socket search to check both the remote and local fields of the
socket match the socket we want to find.

Sockets are identified by the 4-uple

	local_addr, local_port, remote_addr, remote_port

Two socket can differ by a single one of this four elements.
It is possible for the same local_port to be used by two different sockets,
as long as they are connecting to different remote addresses (or remote
ports).

An example of this bug in action can be seen at:

https://github.com/golang/vscode-go/runs/3141270564?check_suite_focus=true

There the server starts listening on 127.0.0.1:46011 and rejects a valid
client connection by finding the following socket:

60: 0100007F:DD82 0100007F:962D 06 00000000:00000000 03:00000133 00000000     0        0 0 3 0000000000000000

the local address of this socket is 0100007F:DD82 (127.0.0.1:56706), and the
remote address is 0100007F:962D (127.0.0.1:38445).

The reported error is:

	closing connection from different user (127.0.0.1:56706): connections to localhost are only accepted from the same UNIX user for security reasons

note how the local port does match the socket line (56706) but the remote
port is wrong (38445 instead of 46011).

Note also that the state of this socket is 06, or TIME_WAIT, which would be
impossible if this was the right socket, since the right socket would still
be open.

Fixes https://github.com/golang/vscode-go/issues/1555

service/dap/server.go

@@ -342,7 +342,7 @@ func (s *Server) Run() {
 			return
 		}
 		if s.config.CheckLocalConnUser {
-			if !sameuser.CanAccept(s.listener.Addr(), conn.RemoteAddr()) {
+			if !sameuser.CanAccept(s.listener.Addr(), conn.LocalAddr(), conn.RemoteAddr()) {
 				s.log.Error("Error accepting client connection: Only connections from the same user that started this instance of Delve are allowed to connect. See --only-same-user.")
 				s.triggerServerStop()
 				return

service/internal/sameuser/sameuser.go

@@ -4,6 +4,6 @@ package sameuser
 
 import "net"
 
-func CanAccept(_, _ net.Addr) bool {
+func CanAccept(_, _, _ net.Addr) bool {
 	return true
 }

service/internal/sameuser/sameuser_linux.go

@@ -30,7 +30,7 @@ func (e *errConnectionNotFound) Error() string {
 	return fmt.Sprintf("connection not found in %s", e.filename)
 }
 
-func sameUserForHexLocalAddr(filename, hexaddr string) (bool, error) {
+func sameUserForHexLocalAddr(filename, localAddr, remoteAddr string) (bool, error) {
 	b, err := readFile(filename)
 	if err != nil {
 		return false, err
@@ -39,22 +39,25 @@ func sameUserForHexLocalAddr(filename, hexaddr string) (bool, error) {
 		// The format contains whitespace padding (%4d, %5u), so we use
 		// fmt.Sscanf instead of splitting on whitespace.
 		var (
-			sl                    int
-			localAddr, remoteAddr string
-			state                 int
-			queue, timer          string
-			retransmit            int
-			remoteUID             uint
+			sl                            int
+			readLocalAddr, readRemoteAddr string
+			state                         int
+			queue, timer                  string
+			retransmit                    int
+			remoteUID                     uint
 		)
 		// Note that we must use %d where the kernel format uses %5u:
 		// %u is not understood by the fmt package (%U is something else),
 		// %5d cuts off longer uids (e.g. 149098 on gLinux).
 		n, err := fmt.Sscanf(line, "%4d: %s %s %02X %s %s %08X %d",
-			&sl, &localAddr, &remoteAddr, &state, &queue, &timer, &retransmit, &remoteUID)
+			&sl, &readLocalAddr, &readRemoteAddr, &state, &queue, &timer, &retransmit, &remoteUID)
 		if n != 8 || err != nil {
 			continue // invalid line (e.g. header line)
 		}
-		if localAddr != hexaddr {
+		if readLocalAddr != remoteAddr || readRemoteAddr != localAddr {
+			// this check is deliberately crossed, the (readLocalAddr,
+			// readRemoteAddr) pair is from the point of view of the client, the
+			// (localAddr, remoteAddr) is from the point of view of the server.
 			continue
 		}
 		same := uid == int(remoteUID)
@@ -66,15 +69,29 @@ func sameUserForHexLocalAddr(filename, hexaddr string) (bool, error) {
 	return false, &errConnectionNotFound{filename}
 }
 
-func sameUserForRemoteAddr4(remoteAddr *net.TCPAddr) (bool, error) {
+func addrToHex4(addr *net.TCPAddr) string {
 	// For details about the format, see the kernel side implementation:
 	// https://elixir.bootlin.com/linux/v5.2.2/source/net/ipv4/tcp_ipv4.c#L2375
-	b := remoteAddr.IP.To4()
-	hexaddr := fmt.Sprintf("%02X%02X%02X%02X:%04X", b[3], b[2], b[1], b[0], remoteAddr.Port)
-	r, err := sameUserForHexLocalAddr("/proc/net/tcp", hexaddr)
+	b := addr.IP.To4()
+	return fmt.Sprintf("%02X%02X%02X%02X:%04X", b[3], b[2], b[1], b[0], addr.Port)
+}
+
+func addrToHex6(addr *net.TCPAddr) string {
+	a16 := addr.IP.To16()
+	// For details about the format, see the kernel side implementation:
+	// https://elixir.bootlin.com/linux/v5.2.2/source/net/ipv6/tcp_ipv6.c#L1792
+	words := make([]uint32, 4)
+	if err := binary.Read(bytes.NewReader(a16), binary.LittleEndian, words); err != nil {
+		panic(err)
+	}
+	return fmt.Sprintf("%08X%08X%08X%08X:%04X", words[0], words[1], words[2], words[3], addr.Port)
+}
+
+func sameUserForRemoteAddr4(localAddr, remoteAddr *net.TCPAddr) (bool, error) {
+	r, err := sameUserForHexLocalAddr("/proc/net/tcp", addrToHex4(localAddr), addrToHex4(remoteAddr))
 	if _, isNotFound := err.(*errConnectionNotFound); isNotFound {
 		// See Issue #1835
-		r, err2 := sameUserForHexLocalAddr("/proc/net/tcp6", "0000000000000000FFFF0000"+hexaddr)
+		r, err2 := sameUserForHexLocalAddr("/proc/net/tcp6", "0000000000000000FFFF0000"+addrToHex4(localAddr), "0000000000000000FFFF0000"+addrToHex4(remoteAddr))
 		if err2 == nil {
 			return r, nil
 		}
@@ -82,43 +99,34 @@ func sameUserForRemoteAddr4(remoteAddr *net.TCPAddr) (bool, error) {
 	return r, err
 }
 
-func sameUserForRemoteAddr6(remoteAddr *net.TCPAddr) (bool, error) {
-	a16 := remoteAddr.IP.To16()
-	// For details about the format, see the kernel side implementation:
-	// https://elixir.bootlin.com/linux/v5.2.2/source/net/ipv6/tcp_ipv6.c#L1792
-	words := make([]uint32, 4)
-	if err := binary.Read(bytes.NewReader(a16), binary.LittleEndian, words); err != nil {
-		return false, err
-	}
-	hexaddr := fmt.Sprintf("%08X%08X%08X%08X:%04X", words[0], words[1], words[2], words[3], remoteAddr.Port)
-	return sameUserForHexLocalAddr("/proc/net/tcp6", hexaddr)
+func sameUserForRemoteAddr6(localAddr, remoteAddr *net.TCPAddr) (bool, error) {
+	return sameUserForHexLocalAddr("/proc/net/tcp6", addrToHex6(localAddr), addrToHex6(remoteAddr))
 }
 
-func sameUserForRemoteAddr(remoteAddr *net.TCPAddr) (bool, error) {
+func sameUserForRemoteAddr(localAddr, remoteAddr *net.TCPAddr) (bool, error) {
 	if remoteAddr.IP.To4() == nil {
-		return sameUserForRemoteAddr6(remoteAddr)
+		return sameUserForRemoteAddr6(localAddr, remoteAddr)
 	}
-	return sameUserForRemoteAddr4(remoteAddr)
+	return sameUserForRemoteAddr4(localAddr, remoteAddr)
 }
 
-func CanAccept(listenAddr, remoteAddr net.Addr) bool {
+func CanAccept(listenAddr, localAddr, remoteAddr net.Addr) bool {
 	laddr, ok := listenAddr.(*net.TCPAddr)
 	if !ok || !laddr.IP.IsLoopback() {
 		return true
 	}
-	addr, ok := remoteAddr.(*net.TCPAddr)
-	if !ok {
-		panic(fmt.Sprintf("BUG: conn.RemoteAddr is %T, want *net.TCPAddr", remoteAddr))
-	}
-	same, err := sameUserForRemoteAddr(addr)
+	remoteaAddrTCP := remoteAddr.(*net.TCPAddr)
+	localAddrTCP := localAddr.(*net.TCPAddr)
+
+	same, err := sameUserForRemoteAddr(localAddrTCP, remoteaAddrTCP)
 	if err != nil {
 		log.Printf("cannot check remote address: %v", err)
 	}
 	if !same {
 		if logflags.Any() {
-			log.Printf("closing connection from different user (%v): connections to localhost are only accepted from the same UNIX user for security reasons", addr)
+			log.Printf("closing connection from different user (%v): connections to localhost are only accepted from the same UNIX user for security reasons", remoteaAddrTCP)
 		} else {
-			fmt.Fprintf(os.Stderr, "closing connection from different user (%v): connections to localhost are only accepted from the same UNIX user for security reasons", addr)
+			fmt.Fprintf(os.Stderr, "closing connection from different user (%v): connections to localhost are only accepted from the same UNIX user for security reasons\n", remoteaAddrTCP)
 		}
 		return false
 	}

service/internal/sameuser/sameuser_linux_test.go

@@ -14,33 +14,36 @@ func TestSameUserForRemoteAddr(t *testing.T) {
 		return []byte(proc), nil
 	}
 	for _, tt := range []struct {
-		name string
-		proc string
-		addr *net.TCPAddr
-		want bool
+		name                  string
+		proc                  string
+		localAddr, remoteAddr *net.TCPAddr
+		want                  bool
 	}{
 		{
 			name: "ipv4-same",
 			proc: `  sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode
   21: 0100007F:E682 0100007F:0FC8 01 00000000:00000000 00:00000000 00000000 149098        0 8420541 2 0000000000000000 20 0 0 10 -1                  `,
-			addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 59010},
-			want: true,
+			localAddr:  &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 4040},
+			remoteAddr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 59010},
+			want:       true,
 		},
 
 		{
 			name: "ipv4-not-found",
 			proc: `  sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode
   21: 0100007F:E682 0100007F:0FC8 01 00000000:00000000 00:00000000 00000000 149098        0 8420541 2 0000000000000000 20 0 0 10 -1                  `,
-			addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 2342},
-			want: false,
+			localAddr:  &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 4040},
+			remoteAddr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 2342},
+			want:       false,
 		},
 
 		{
 			name: "ipv4-different-uid",
 			proc: `  sl  local_address rem_address   st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode
   21: 0100007F:E682 0100007F:0FC8 01 00000000:00000000 00:00000000 00000000 149097        0 8420541 2 0000000000000000 20 0 0 10 -1                  `,
-			addr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 59010},
-			want: false,
+			localAddr:  &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 4040},
+			remoteAddr: &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 59010},
+			want:       false,
 		},
 
 		{
@@ -48,16 +51,17 @@ func TestSameUserForRemoteAddr(t *testing.T) {
 			proc: `  sl  local_address                         remote_address                        st tx_queue rx_queue tr tm->when retrnsmt   uid  timeout inode
    5: 00000000000000000000000001000000:D3E4 00000000000000000000000001000000:0FC8 01 00000000:00000000 00:00000000 00000000 149098        0 8425526 2 0000000000000000 20 0 0 10 -1
    6: 00000000000000000000000001000000:0FC8 00000000000000000000000001000000:D3E4 01 00000000:00000000 00:00000000 00000000 149098        0 8424744 1 0000000000000000 20 0 0 10 -1`,
-			addr: &net.TCPAddr{IP: net.ParseIP("::1"), Port: 54244},
-			want: true,
+			localAddr:  &net.TCPAddr{IP: net.ParseIP("::1"), Port: 4040},
+			remoteAddr: &net.TCPAddr{IP: net.ParseIP("::1"), Port: 54244},
+			want:       true,
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			proc = tt.proc
 			// The returned error is for reporting only.
-			same, _ := sameUserForRemoteAddr(tt.addr)
+			same, _ := sameUserForRemoteAddr(tt.localAddr, tt.remoteAddr)
 			if got, want := same, tt.want; got != want {
-				t.Errorf("sameUserForRemoteAddr(%v) = %v, want %v", tt.addr, got, want)
+				t.Errorf("sameUserForRemoteAddr(%v, %v) = %v, want %v", tt.localAddr, tt.remoteAddr, got, want)
 			}
 		})
 	}

service/rpccommon/server.go

@@ -146,7 +146,7 @@ func (s *ServerImpl) Run() error {
 			}
 
 			if s.config.CheckLocalConnUser {
-				if !sameuser.CanAccept(s.listener.Addr(), c.RemoteAddr()) {
+				if !sameuser.CanAccept(s.listener.Addr(), c.LocalAddr(), c.RemoteAddr()) {
 					c.Close()
 					continue
 				}