libtailscale: mark handed-out fds close-on-exec

fd_linux.go

@@ -0,0 +1,25 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+//go:build linux
+
+package main
+
+import "syscall"
+
+// socketpair returns a new AF_LOCAL SOCK_STREAM socketpair whose ends
+// are both close-on-exec from birth, so spawned processes do not
+// inherit them. Linux creates the pair atomically with SOCK_CLOEXEC.
+func socketpair() ([2]int, error) {
+	return syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM|syscall.SOCK_CLOEXEC, 0)
+}
+
+// recvFlags are the flags TsnetAccept passes to Recvmsg: MSG_WAITALL
+// to keep listener socketpair record framing exact, plus
+// MSG_CMSG_CLOEXEC so the connection fd installed by SCM_RIGHTS is
+// close-on-exec without a race window.
+const recvFlags = syscall.MSG_WAITALL | syscall.MSG_CMSG_CLOEXEC
+
+// closeOnExec marks fd close-on-exec. It is a no-op on Linux, where
+// recvFlags already made received fds close-on-exec.
+func closeOnExec(fd int) {}

fd_other.go

@@ -0,0 +1,42 @@
+// Copyright (c) Tailscale Inc & AUTHORS
+// SPDX-License-Identifier: BSD-3-Clause
+
+//go:build !linux
+
+package main
+
+import "syscall"
+
+// socketpair returns a new AF_LOCAL SOCK_STREAM socketpair whose ends
+// are both close-on-exec, so spawned processes do not inherit them.
+//
+// Platforms without SOCK_CLOEXEC (darwin) create the pair while
+// holding ForkLock's read lock, which delays any concurrent fork
+// (which takes the write lock) until both ends are marked
+// close-on-exec. This is the same dance package net does for
+// accept(2).
+func socketpair() ([2]int, error) {
+	syscall.ForkLock.RLock()
+	defer syscall.ForkLock.RUnlock()
+
+	fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM, 0)
+	if err != nil {
+		return fds, err
+	}
+	syscall.CloseOnExec(fds[0])
+	syscall.CloseOnExec(fds[1])
+	return fds, nil
+}
+
+// recvFlags are the flags TsnetAccept passes to Recvmsg: MSG_WAITALL
+// to keep listener socketpair record framing exact. Platforms without
+// MSG_CMSG_CLOEXEC mark the fd received via SCM_RIGHTS close-on-exec
+// with closeOnExec instead.
+const recvFlags = syscall.MSG_WAITALL
+
+// closeOnExec marks fd close-on-exec. It runs right after Recvmsg
+// installed fd, closing the window in which a concurrent fork could
+// inherit it as far as the kernel allows.
+func closeOnExec(fd int) {
+	syscall.CloseOnExec(fd)
+}

sourcepkg/Makefile

@@ -24,8 +24,8 @@ check: libtailscale.tar.zst
 clean:
 	rm -rf ./libtailscale.tar.zst ../vendor ./libtailscale
 
-../vendor: ../go.mod ../go.sum ../tailscale.go Makefile.src Makefile
+../vendor: ../go.mod ../go.sum ../tailscale.go ../fd_linux.go ../fd_other.go Makefile.src Makefile
 	go mod vendor
 
-libtailscale.tar.zst: Makefile.src configure ../vendor ../LICENSE ../tailscale.go ../go.mod ../go.sum
-	$(TAR) --transform 's#^#libtailscale/#' --transform 's#Makefile.src#Makefile#' -acf $@ Makefile.src configure ../vendor ../LICENSE ../tailscale.go ../go.mod ../go.sum
+libtailscale.tar.zst: Makefile.src configure ../vendor ../LICENSE ../tailscale.go ../fd_linux.go ../fd_other.go ../go.mod ../go.sum
+	$(TAR) --transform 's#^#libtailscale/#' --transform 's#Makefile.src#Makefile#' -acf $@ Makefile.src configure ../vendor ../LICENSE ../tailscale.go ../fd_linux.go ../fd_other.go ../go.mod ../go.sum

tailscale.go

@@ -240,7 +240,9 @@ func TsnetListen(sd C.int, network, addr *C.char, listenerOut *C.int) C.int {
 	// feed an fd for the connection through the listener. This lets C use
 	// epoll on the tailscale_listener to know if it should call
 	// tailscale_accept, which avoids a blocking call on the far side.
-	fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM, 0)
+	// Both ends are close-on-exec, so spawned processes do not inherit
+	// them.
+	fds, err := socketpair()
 	if err != nil {
 		return s.recErr(err)
 	}
@@ -336,7 +338,8 @@ func TsnetAccept(listenerFd C.int, connOut *C.int) C.int {
 
 	// One record per connection: the fd via SCM_RIGHTS plus its remote
 	// IP. The fd parsed below is the number the kernel installed, i.e.
-	// the number C will hold.
+	// the number C will hold, and it is close-on-exec (via recvFlags on
+	// Linux, or the closeOnExec call below elsewhere).
 	//
 	// The socketpair is a stream socket, so records are framed here:
 	// each read asks for the remaining bytes of the head record, with
@@ -352,7 +355,7 @@ func TsnetAccept(listenerFd C.int, connOut *C.int) C.int {
 			// the cmsg rode with the record's first byte; only the tail remains
 			cbuf = nil
 		}
-		nn, on, _, _, err := syscall.Recvmsg(int(listenerFd), buf, cbuf, syscall.MSG_WAITALL)
+		nn, on, _, _, err := syscall.Recvmsg(int(listenerFd), buf, cbuf, recvFlags)
 		n += nn
 		if nn > 0 {
 			oobn = on
@@ -384,6 +387,7 @@ func TsnetAccept(listenerFd C.int, connOut *C.int) C.int {
 		return ln.s.recErr(fmt.Errorf("libtailscale: got %d FDs, want 1", len(fds)))
 	}
 	fd := C.int(fds[0])
+	closeOnExec(int(fd))
 
 	// the entry must exist before C can learn this fd number
 	addrLen := bytes.IndexByte(data, 0)
@@ -400,7 +404,7 @@ func TsnetAccept(listenerFd C.int, connOut *C.int) C.int {
 }
 
 func newConn(s *server, netConn net.Conn, connOut *C.int) error {
-	fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_STREAM, 0)
+	fds, err := socketpair()
 	if err != nil {
 		return err
 	}

tailscale_test.go

@@ -1,10 +1,12 @@
 package main
 
 import (
+	"syscall"
 	"testing"
 	"time"
 
 	"github.com/tailscale/libtailscale/tsnetctest"
+	"golang.org/x/sys/unix"
 )
 
 func TestConn(t *testing.T) {
@@ -66,6 +68,89 @@ func TestGetRemoteAddr(t *testing.T) {
 	tsnetctest.RunTestGetRemoteAddr(t)
 }
 
+// TestSocketpairCloexec checks that socketpair returns both ends
+// close-on-exec, so that spawned processes cannot inherit the
+// listener and connection fds.
+func TestSocketpairCloexec(t *testing.T) {
+	fds, err := socketpair()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer syscall.Close(fds[0])
+	defer syscall.Close(fds[1])
+
+	for _, fd := range fds {
+		flags, err := unix.FcntlInt(uintptr(fd), syscall.F_GETFD, 0)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if flags&syscall.FD_CLOEXEC == 0 {
+			t.Errorf("socketpair end %d is not close-on-exec", fd)
+		}
+	}
+}
+
+// TestRecvFdCloexec exercises the SCM_RIGHTS receive path TsnetAccept
+// uses: an fd passed over the listener socketpair must be
+// close-on-exec by the time it reaches C (via MSG_CMSG_CLOEXEC in
+// recvFlags on Linux, via closeOnExec on platforms without it).
+func TestRecvFdCloexec(t *testing.T) {
+	sp, err := socketpair()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer syscall.Close(sp[0])
+	defer syscall.Close(sp[1])
+
+	// The fd we ship across the socketpair.
+	pair, err := socketpair()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer syscall.Close(pair[0])
+	defer syscall.Close(pair[1])
+
+	var rec [listenAddrLen]byte
+	copy(rec[:], "1.2.3.4")
+	if err := syscall.Sendmsg(sp[0], rec[:], syscall.UnixRights(pair[0]), nil, 0); err != nil {
+		t.Fatal(err)
+	}
+
+	data := make([]byte, listenAddrLen)
+	cbuf := make([]byte, syscall.CmsgSpace(4))
+	n, oobn, _, _, err := syscall.Recvmsg(sp[1], data, cbuf, recvFlags)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if n != listenAddrLen {
+		t.Fatalf("got %d bytes, want %d", n, listenAddrLen)
+	}
+	scms, err := syscall.ParseSocketControlMessage(cbuf[:oobn])
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(scms) != 1 {
+		t.Fatalf("got %d control messages, want 1", len(scms))
+	}
+	fds, err := syscall.ParseUnixRights(&scms[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(fds) != 1 {
+		t.Fatalf("got %d fds, want 1", len(fds))
+	}
+	closeOnExec(fds[0]) // mirrors TsnetAccept
+	defer syscall.Close(fds[0])
+
+	flags, err := unix.FcntlInt(uintptr(fds[0]), syscall.F_GETFD, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if flags&syscall.FD_CLOEXEC == 0 {
+		t.Errorf("fd received via SCM_RIGHTS is not close-on-exec")
+	}
+}
+
 func TestExtractIP(t *testing.T) {
 	ipv4 := "1.23.33.4:12343"
 	ipv6 := "[1::2234::34fc::44]:56576"

tsnetctest/tsnetctest.go

@@ -10,6 +10,7 @@ package tsnetctest
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <fcntl.h>
 #include "../tailscale.h"
 
 char* tmps1;
@@ -27,6 +28,21 @@ char* err = NULL;
 
 tailscale s1, s2;
 
+// check_cloexec fails the test if fd is not close-on-exec. Listener
+// and connection fds must not leak into processes the host forks.
+int check_cloexec(int fd) {
+	int flags = fcntl(fd, F_GETFD);
+	if (flags == -1) {
+		snprintf(err, errlen, "fcntl(%d, F_GETFD): errno %d (%s)", fd, errno, strerror(errno));
+		return 1;
+	}
+	if (!(flags & FD_CLOEXEC)) {
+		snprintf(err, errlen, "fd %d is not close-on-exec", fd);
+		return 1;
+	}
+	return 0;
+}
+
 int set_err(tailscale sd, char tag) {
 	err[0] = tag;
 	err[1] = ':';
@@ -74,16 +90,25 @@ int test_conn() {
 	if ((ret = tailscale_listen(s1, "tcp", ":8081", &ln)) != 0) {
 		return set_err(s1, '8');
 	}
+	if (check_cloexec(ln)) {
+		return 1;
+	}
 
 	tailscale_conn w;
 	if ((ret = tailscale_dial(s2, "tcp", "100.64.0.1:8081", &w)) != 0) {
 		return set_err(s2, '9');
 	}
+	if (check_cloexec(w)) {
+		return 1;
+	}
 
 	tailscale_conn r;
 	if ((ret = tailscale_accept(ln, &r)) != 0) {
 		return set_err(s2, 'a');
 	}
+	if (check_cloexec(r)) {
+		return 1;
+	}
 
 	const char want[] = "hello";
 	ssize_t wret;