amadan
unixdial: require a byte-exact peer path and refuse a .. component
verifyPeer cleaned both the dialled path and the peer name getpeername(2) reported before comparing them, which accepts a peer whose bound path only normalizes to the same string as an equivalent spelling of it — more than getpeername() actually promised, since the kernel reports the bind string verbatim. Only the dialled side is cleaned now; the peer name must equal that cleaned path byte for byte, and a peer name carrying a .. component is refused outright, checked before any cleaning could resolve it away.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZ1ZvWj7naf73GjTX8MBv5
2 files changed,
+42
−1
internal/unixdial/unixdial.go+25 −1internal/unixdial/unixdial_test.go+17 −0
diff --git a/internal/unixdial/unixdial.go b/internal/unixdial/unixdial.go| index ee89bdc8f..6c0104642 100644 |
| --- a/internal/unixdial/unixdial.go |
| +++ b/internal/unixdial/unixdial.go |
| @@ -32,6 +32,7 @@ import ( |
| "fmt" |
| "net" |
| "path/filepath" |
| + "strings" |
| "github.com/carlosframework/platform/internal/controlsock" |
| ) |
| @@ -70,6 +71,17 @@ func Checked(ctx context.Context, d *net.Dialer, path string) (net.Conn, error) |
| // verifyPeer compares the peer address of a connected unix socket with the |
| // dialled path. net's UnixConn fills RemoteAddr from getpeername(2) at |
| // connect time, which is the listener's bound address. |
| +// |
| +// Only the DIALLED side is cleaned (want, below); the peer name the kernel |
| +// reports must equal want BYTE FOR BYTE. Cleaning got as well would let a |
| +// listener whose bound path merely normalizes to the same string (a |
| +// different-but-equivalent spelling of it) pass as a match, which is more |
| +// than getpeername() actually promised: the kernel reports the bind string |
| +// verbatim, not a normalized one, so an exact match is the only comparison |
| +// that means what this check claims to mean. A peer name containing a ".." |
| +// path component is refused outright for the same reason a relative bind |
| +// path already is: it is never a path this package's callers bind by, so |
| +// its presence means the socket was reached some other way than intended. |
| func verifyPeer(conn net.Conn, dialled string) error { |
| ra := conn.RemoteAddr() |
| ua, ok := ra.(*net.UnixAddr) |
| @@ -78,8 +90,20 @@ func verifyPeer(conn net.Conn, dialled string) error { |
| } |
| want := filepath.Clean(dialled) |
| got := ua.Name |
| - if got == "" || got[0] == '@' || filepath.Clean(got) != want { |
| + if got == "" || got[0] == '@' || hasDotDotComponent(got) || got != want { |
| return fmt.Errorf("%w: dialled %s, listener is bound to %q", ErrPeerMismatch, want, got) |
| } |
| return nil |
| } |
| + |
| +// hasDotDotComponent reports whether name has a ".." path component, |
| +// checked without cleaning it first: cleaning would resolve the very |
| +// traversal this exists to catch before it could ever be seen. |
| +func hasDotDotComponent(name string) bool { |
| + for _, part := range strings.Split(name, string(filepath.Separator)) { |
| + if part == ".." { |
| + return true |
| + } |
| + } |
| + return false |
| +} |
diff --git a/internal/unixdial/unixdial_test.go b/internal/unixdial/unixdial_test.go| index 5100d6bf4..1d3d2eabc 100644 |
| --- a/internal/unixdial/unixdial_test.go |
| +++ b/internal/unixdial/unixdial_test.go |
| @@ -128,6 +128,23 @@ func TestVerifyPeerRefusesAnEmptyNamePeer(t *testing.T) { |
| } |
| } |
| +// TestVerifyPeerRequiresByteEqualBoundPath pins the review fix: the peer |
| +// name the kernel reports must equal the cleaned dialled path byte for |
| +// byte, not merely after being cleaned itself, and a peer name with a ".." |
| +// component is refused outright even when a naive clean-and-compare would |
| +// have accepted it. |
| +func TestVerifyPeerRequiresByteEqualBoundPath(t *testing.T) { |
| + conn := nameOnlyConn{remote: &net.UnixAddr{Net: "unix", Name: "/x/../app.sock"}} |
| + if err := verifyPeer(conn, "/app.sock"); !errors.Is(err, ErrPeerMismatch) { |
| + t.Fatalf("verifyPeer(%q) = %v, want ErrPeerMismatch: a peer name with a .. component must never be accepted", "/x/../app.sock", err) |
| + } |
| + |
| + conn = nameOnlyConn{remote: &net.UnixAddr{Net: "unix", Name: "/app.sock"}} |
| + if err := verifyPeer(conn, "/app.sock"); err != nil { |
| + t.Fatalf("verifyPeer(%q) = %v, want nil: an exact byte-for-byte match must be accepted", "/app.sock", err) |
| + } |
| +} |
| + |
| func TestCheckedPassesConnectErrorsThrough(t *testing.T) { |
| path := short(t, "absent.sock") |
| _, err := Checked(context.Background(), &net.Dialer{}, path) |