From 2b196382be94a6def34481f6456833fbb94a93df Mon Sep 17 00:00:00 2001 From: Stefan van der Merwe <44154511+Sniperlyf3@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:51:41 +0200 Subject: [PATCH 1/8] Harden proxy, forwarding, and build security --- .github/workflows/ci.yml | 8 +- SECURITY_REVIEW.md | 136 +++++++++++++++++++++++++++++++ build.sh | 4 +- cmd/meowshell/forwarding.go | 11 ++- cmd/meowshell/forwarding_test.go | 29 +++++++ cmd/meowshell/transport.go | 10 ++- cmd/meowshell/transport_test.go | 36 ++++++++ go.mod | 2 +- go.sum | 2 + tailcat.ref | 1 + 10 files changed, 230 insertions(+), 9 deletions(-) create mode 100644 SECURITY_REVIEW.md create mode 100644 cmd/meowshell/forwarding_test.go create mode 100644 tailcat.ref diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c7f12a..58c2c9e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,7 +41,7 @@ jobs: # checkout for the real cross-compiled build. - name: Fetch tailcat source (for go.mod's replace directive) env: - SRC_REF: ${{ inputs.tailcat_ref || 'main' }} + SRC_REF: ${{ inputs.tailcat_ref || '91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a' }} run: | git clone --depth=1 https://github.com/tailscale/tailcat.git .tailcat-src git -C .tailcat-src fetch --depth=1 origin "$SRC_REF" @@ -72,7 +72,7 @@ jobs: # tailcat's go.mod asks for a newer Go than setup-go's stable may # be; let the toolchain fetch it rather than pinning a version here. GOTOOLCHAIN: auto - SRC_REF: ${{ inputs.tailcat_ref || 'main' }} + SRC_REF: ${{ inputs.tailcat_ref || '91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a' }} ANDROID_API_LEVEL: ${{ inputs.android_api_level || '21' }} run: ./build.sh @@ -119,7 +119,7 @@ jobs: - name: Build a host tailcat to act as the client env: GOTOOLCHAIN: auto - SRC_REF: ${{ inputs.tailcat_ref || 'main' }} + SRC_REF: ${{ inputs.tailcat_ref || '91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a' }} run: | git clone --depth=1 https://github.com/tailscale/tailcat.git .tailcat-src git -C .tailcat-src fetch --depth=1 origin "$SRC_REF" @@ -376,7 +376,7 @@ jobs: # address -- by having a real client dial in and run commands. env: GOTOOLCHAIN: auto - SRC_REF: ${{ inputs.tailcat_ref || 'main' }} + SRC_REF: ${{ inputs.tailcat_ref || '91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a' }} run: | git clone --depth=1 https://github.com/tailscale/tailcat.git .tailcat-src git -C .tailcat-src fetch --depth=1 origin "$SRC_REF" diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md new file mode 100644 index 0000000..1549dde --- /dev/null +++ b/SECURITY_REVIEW.md @@ -0,0 +1,136 @@ +# Security review + +**Review date:** 2026-09-11 +**Reviewed revision:** working tree based on the `work` branch +**Scope:** the Go CLI/agent and protocol, the .NET process wrapper and agent +client, build/release automation, tests, and the locally fetched tailcat source +at the revision recorded in `tailcat.ref`. Generated native binaries, external +relay infrastructure, GitHub/NuGet account configuration, and Android platform +internals were not independently penetration-tested. + +## Executive summary + +The review found four actionable weaknesses and remediated them in this +revision: + +1. **High — HTTPS proxy transport was not encrypted.** The `https` proxy scheme + followed the raw TCP path used for HTTP. This made proxy credentials and the + CONNECT request visible to an on-path attacker and did not authenticate the + proxy. HTTPS proxy connections now perform a normal, certificate-verified TLS + handshake before sending HTTP. +2. **High — release builds consumed a mutable upstream branch.** Native artifacts + defaulted to the then-current tailcat `main`, so identical Meowshell source + could produce different binaries and an upstream compromise could enter a + release without a repository change. The default and CI are now pinned to the + reviewed full commit in `tailcat.ref`; explicit workflow input remains + available for deliberate upstream testing. +3. **Medium — Unix forwarding could delete a non-socket path.** Starting a Unix + listener unconditionally removed an existing caller-supplied path. It now + uses `lstat` and only removes an actual socket, refusing regular files and + symbolic links. +4. **Medium — the SSH dependency contained reachable denial-of-service flaws.** + `govulncheck` identified GO-2026-6354 and GO-2026-6355 in the SSH handshake + path. `golang.org/x/crypto` is upgraded to v0.56.0, which contains both fixes. + +No hard-coded credentials, shell-based local process launch, unrestricted +local TCP bind by default, unbounded protocol frame allocation, or silent TCP +SSH host-key acceptance was found in the reviewed first-party code. The +repository already uses argument-list process launching, a 64 MiB frame cap, +TOFU `known_hosts` verification for ordinary SSH, loopback-only local forwards +by default, random SOCKS credentials by default, restrictive temporary key and +Unix-socket permissions, and commit-pinned GitHub Actions. + +## Trust boundaries and attack surface + +- **Remote peers and relays:** SSH handshakes, host keys, authentication prompts, + session output, SFTP metadata/data, TCP forwarding, and SOCKS destinations. +- **Local child-process boundary:** the .NET client exchanges framed JSON and + binary data with `meowshell agent` over redirected standard streams. A replaced + packaged binary inherits the application's secrets and authority. +- **Local filesystem:** executable discovery, `HOME`, known-hosts, temporary key + staging, download destinations, and Unix forwarding paths. +- **Caller-controlled configuration:** remote commands, proxy URLs and + credentials, bind addresses, key material, jump hosts, SFTP paths, and + deliberate insecure modes. +- **Build/release:** upstream tailcat source, Go/NuGet dependencies, downloaded + toolchains, CI actions, native artifacts, and NuGet trusted publishing. + +## Findings and disposition + +### SR-01: HTTPS proxy scheme used plaintext TCP — fixed (High) + +`dialHTTPConnectProxy` accepted both HTTP and HTTPS URLs but previously dialed +both with `net.Dialer`. Basic proxy credentials were therefore sent in cleartext +for an HTTPS URL, contrary to the scheme and caller expectations. The HTTPS path +now uses `tls.Dialer`, which validates the proxy certificate and infers SNI from +the proxy host. A regression test verifies that an HTTPS URL starts with a TLS +handshake rather than a plaintext CONNECT request. + +### SR-02: mutable tailcat source in builds — fixed (High) + +`build.sh` and four CI jobs defaulted to `main`. The default is now the complete +reviewed commit ID, stored in `tailcat.ref` for local builds and mirrored in CI. +Maintainers should update this pin through a reviewed change, inspect upstream +diffs, reapply both Android patches, run the full matrix, and run dependency +vulnerability scanning before release. + +### SR-03: Unix listener removed arbitrary existing paths — fixed (Medium) + +The agent removed the requested Unix-socket path before binding. A mistaken path +could destroy an application file; a local attacker able to change a shared +parent directory could substitute a symlink or regular file. Existing paths are +now inspected without following symlinks and removal is limited to socket nodes. +The remaining check/remove race is only exploitable by a principal that can +mutate the socket's parent directory. Callers should place sockets in a private +0700 directory (such as an application-owned runtime directory). + +### SR-04: reachable SSH denial of service in x/crypto — fixed (Medium) + +The reviewed v0.55.0 dependency allowed a peer to deadlock established or +undecided SSH channels (GO-2026-6354 and GO-2026-6355). Because the agent calls +`ssh.NewClientConn`, `govulncheck` found a reachable trace through +`dialSSHClient`. The direct dependency is upgraded to the fixed v0.56.0 release. + +## Accepted design risks and hardening backlog + +1. **Tailcat SSH host-key checking is intentionally disabled.** Tailcat sessions + use `ssh.InsecureIgnoreHostKey`, relying on the cryptographic capability in + the tailcat address/transport rather than OpenSSH-style host identity. This + assumption must be revalidated whenever tailcat's address or handshake design + changes. Ordinary TCP SSH correctly uses TOFU and rejects changed keys. +2. **`InsecureNoAuth` is intentionally dangerous.** It creates a shell whose + bearer address is the credential. Keep the warning prominent, prefer + `AuthorizedKeys`, combine address-only use with client-key restrictions, use + short lifetimes, and never log or persist addresses unnecessarily. +3. **Remote exec is a command string, not an argv-safe execution API.** Command + elements are joined with spaces because SSH exec transmits one command + string. The .NET API documents that it adds no quoting. Applications must not + concatenate untrusted values; use SFTP or a fixed remote helper protocol for + untrusted inputs. +4. **The agent protocol permits 64 MiB frames.** The explicit bound prevents + unlimited allocation but still permits substantial per-frame memory use. The + child is local and trusted in the normal architecture. If the protocol is + exposed to a less-trusted producer, lower the control-frame limit and stream + large data separately. +5. **Forwarding is powerful by design.** Local TCP forwarding is loopback-only by + default and SOCKS auth defaults on for TCP, but callers can opt into nonlocal + binds or unauthenticated sockets. Applications should surface those choices as + security-sensitive and apply destination allowlists when acting on untrusted + requests. +6. **Build inputs remain broader than one repository.** Go modules, NuGet + packages, the Go toolchain, Android NDK, and publishing infrastructure remain + external trust dependencies. Add automated `govulncheck`, NuGet audit/SBOM + generation, artifact provenance/attestation, and release verification against + reproducible hashes. +7. **Secret lifetime is not minimized everywhere.** Passwords, passphrases, and + private keys cross managed strings/arrays or Go byte slices and cannot always + be reliably zeroed. Prefer ssh-agent and Android Keystore sign callbacks over + raw private-key/password configuration. + +## Verification plan + +The security fixes are covered by focused Go tests. The standard repository +checks are `go test ./...`, `go vet ./...`, `./verify-binaries.sh`, and +`dotnet test dotnet/Meowshell.sln --configuration Release`. Release review should +also run `govulncheck ./...` after materializing the pinned tailcat checkout and +a NuGet dependency audit in an environment with the required .NET SDK. diff --git a/build.sh b/build.sh index a9f978e..ad8db98 100755 --- a/build.sh +++ b/build.sh @@ -12,7 +12,9 @@ set -euo pipefail REPO_DIR=$PWD SRC_URL=${SRC_URL:-https://github.com/tailscale/tailcat.git} -SRC_REF=${SRC_REF:-main} +# Keep release inputs reproducible. Override deliberately for upstream testing; +# CI uses this same reviewed revision unless a workflow_dispatch input is given. +SRC_REF=${SRC_REF:-$(tr -d '[:space:]' < "$REPO_DIR/tailcat.ref")} SRC_DIR=${SRC_DIR:-$REPO_DIR/.tailcat-src} OUT_DIR=${OUT_DIR:-$REPO_DIR/dist} API=${ANDROID_API_LEVEL:-21} diff --git a/cmd/meowshell/forwarding.go b/cmd/meowshell/forwarding.go index b8435f6..1c21bde 100644 --- a/cmd/meowshell/forwarding.go +++ b/cmd/meowshell/forwarding.go @@ -52,8 +52,15 @@ func listenUnix(path string) (net.Listener, error) { if path == "" { return nil, fmt.Errorf("a unix listen_network needs a non-empty socket path") } - if err := os.Remove(path); err != nil && !os.IsNotExist(err) { - return nil, fmt.Errorf("removing stale socket %s: %w", path, err) + if info, err := os.Lstat(path); err == nil { + if info.Mode()&os.ModeSocket == 0 { + return nil, fmt.Errorf("refusing to remove non-socket path %s", path) + } + if err := os.Remove(path); err != nil { + return nil, fmt.Errorf("removing stale socket %s: %w", path, err) + } + } else if !os.IsNotExist(err) { + return nil, fmt.Errorf("checking stale socket %s: %w", path, err) } ln, err := net.Listen("unix", path) if err != nil { diff --git a/cmd/meowshell/forwarding_test.go b/cmd/meowshell/forwarding_test.go new file mode 100644 index 0000000..77b40c6 --- /dev/null +++ b/cmd/meowshell/forwarding_test.go @@ -0,0 +1,29 @@ +package main + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +func TestListenUnixRefusesToRemoveNonSocket(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Unix sockets are not supported on Windows") + } + path := filepath.Join(t.TempDir(), "important") + if err := os.WriteFile(path, []byte("keep me"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := listenUnix(path); err == nil || !strings.Contains(err.Error(), "refusing to remove non-socket") { + t.Fatalf("listenUnix over a regular file = %v, want refusal", err) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("regular file was removed: %v", err) + } + if string(got) != "keep me" { + t.Fatalf("regular file changed to %q", got) + } +} diff --git a/cmd/meowshell/transport.go b/cmd/meowshell/transport.go index c01d58c..f1c8e01 100644 --- a/cmd/meowshell/transport.go +++ b/cmd/meowshell/transport.go @@ -3,6 +3,7 @@ package main import ( "bufio" "context" + "crypto/tls" "encoding/base64" "fmt" "net" @@ -104,7 +105,14 @@ func proxyAuthFromURL(u *url.URL) *proxy.Auth { func dialHTTPConnectProxy(ctx context.Context, proxyURL *url.URL, hostPort string) (net.Conn, error) { d := net.Dialer{Timeout: tcpDialTimeout} - conn, err := d.DialContext(ctx, "tcp", proxyURL.Host) + var conn net.Conn + var err error + if proxyURL.Scheme == "https" { + tlsDialer := tls.Dialer{NetDialer: &d} + conn, err = tlsDialer.DialContext(ctx, "tcp", proxyURL.Host) + } else { + conn, err = d.DialContext(ctx, "tcp", proxyURL.Host) + } if err != nil { return nil, err } diff --git a/cmd/meowshell/transport_test.go b/cmd/meowshell/transport_test.go index 92992eb..267bf98 100644 --- a/cmd/meowshell/transport_test.go +++ b/cmd/meowshell/transport_test.go @@ -127,6 +127,42 @@ func TestDialHTTPConnectProxyRejectsNon200(t *testing.T) { } } +func TestDialHTTPSConnectProxyUsesTLS(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + + firstByte := make(chan byte, 1) + go func() { + conn, err := ln.Accept() + if err != nil { + return + } + defer conn.Close() + var b [1]byte + if _, err := conn.Read(b[:]); err == nil { + firstByte <- b[0] + } + }() + + proxyURL := mustParseURL(t, "https://"+ln.Addr().String()) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if _, err := dialHTTPConnectProxy(ctx, proxyURL, "backend.example:22"); err == nil { + t.Fatal("dialHTTPConnectProxy against a non-TLS HTTPS proxy did not error") + } + select { + case got := <-firstByte: + if got != 0x16 { // TLS handshake record. + t.Fatalf("first HTTPS proxy byte = %#x, want TLS handshake %#x", got, byte(0x16)) + } + case <-ctx.Done(): + t.Fatal("HTTPS proxy did not receive a connection") + } +} + func readFull(conn net.Conn, buf []byte) (int, error) { total := 0 for total < len(buf) { diff --git a/go.mod b/go.mod index 3f02192..adc125a 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.27.1 require ( github.com/pkg/sftp v1.13.6 github.com/tailscale/tailcat v0.6.0 - golang.org/x/crypto v0.55.0 + golang.org/x/crypto v0.56.0 golang.org/x/net v0.58.0 golang.org/x/term v0.45.0 tailscale.com v1.103.0-pre.0.20260904030409-31d8badb3bfb diff --git a/go.sum b/go.sum index f4d2ca4..e4e1b0d 100644 --- a/go.sum +++ b/go.sum @@ -199,6 +199,8 @@ golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5y golang.org/x/crypto v0.1.0/go.mod h1:RecgLatLF4+eUMCP1PoPZQb+cVrJcOPbHkTkbkB9sbw= golang.org/x/crypto v0.55.0 h1:+KWHjbgOaAQ66dh/YlkZKHlz9ZUlq61AFirAR9ntP8M= golang.org/x/crypto v0.55.0/go.mod h1:uq0V9dE/fzQuJtbnL+2EhWOE63vo164FY8xqEnV9xis= +golang.org/x/crypto v0.56.0 h1:GUh5Ii4J5jtcseSMiRqr1jXCNHoxjeV9Fmekc2oLy6Y= +golang.org/x/crypto v0.56.0/go.mod h1:OMW5y6CY9l38uPLmxU6l6pwcXp1obtLo3e6gT7gQR2I= golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f h1:W3F4c+6OLc6H2lb//N1q4WpJkhzJCK5J6kUi1NTVXfM= golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f/go.mod h1:J1xhfL/vlindoeF/aINzNzt2Bket5bjo9sdOYzOsU80= golang.org/x/exp/typeparams v0.0.0-20260209203927-2842357ff358 h1:qWFG1Dj7TBjOjOvhEOkmyGPVoquqUKnIU0lEVLp8xyk= diff --git a/tailcat.ref b/tailcat.ref new file mode 100644 index 0000000..cb84349 --- /dev/null +++ b/tailcat.ref @@ -0,0 +1 @@ +91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a From 531bd781e1a15e9867a4d87a74c7eafd50f696f7 Mon Sep 17 00:00:00 2001 From: Stefan van der Merwe <44154511+Sniperlyf3@users.noreply.github.com> Date: Fri, 11 Sep 2026 10:05:22 +0200 Subject: [PATCH 2/8] Document .NET end-to-end test results --- SECURITY_REVIEW.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md index 1549dde..c3d1683 100644 --- a/SECURITY_REVIEW.md +++ b/SECURITY_REVIEW.md @@ -134,3 +134,17 @@ checks are `go test ./...`, `go vet ./...`, `./verify-binaries.sh`, and `dotnet test dotnet/Meowshell.sln --configuration Release`. Release review should also run `govulncheck ./...` after materializing the pinned tailcat checkout and a NuGet dependency audit in an environment with the required .NET SDK. + +## Review verification results + +The review environment was subsequently provisioned with .NET SDK 8.0.425 and +the pinned Linux amd64 binaries. All 86 non-E2E .NET tests passed. The complete +suite executed all 107 tests: 94 passed and 13 relay-dependent E2E cases failed +with `context deadline exceeded` or an SSH EOF after the tailcat connection +could not be established. Local real-binary E2E coverage, including listener +startup/shutdown and key/address operations, did pass. The environment routes +outbound HTTP(S) through a mandatory proxy, and the failures are consistent with +the tailcat transport being unable to reach its relay from this environment. +They are therefore recorded as an environment limitation rather than a passing +result or a demonstrated product regression. The full E2E suite still needs a +run from a network that permits the tailcat transport before release. From b76d0ad42bfcf19e7ea255c38cf8348dcdf98e91 Mon Sep 17 00:00:00 2001 From: Stefan van der Merwe <44154511+Sniperlyf3@users.noreply.github.com> Date: Fri, 11 Sep 2026 10:05:28 +0200 Subject: [PATCH 3/8] Automate dependency audits and fix lifecycle races --- .github/dependabot.yml | 25 ++++ .github/workflows/ci.yml | 31 +++- README.md | 7 + REVIEW_MAP.md | 141 ++++++++++++++++++ SECURITY_REVIEW.md | 65 +++++++- build.sh | 2 +- cmd/meowshell/agent.go | 10 +- cmd/meowshell/agentauth_test.go | 23 +++ cmd/meowshell/transport.go | 30 +++- cmd/meowshell/transport_test.go | 51 +++++++ dotnet/Directory.Build.props | 6 + .../Meowshell.PackageTests.csproj | 8 +- dotnet/Meowshell.Tests/Meowshell.Tests.csproj | 8 +- .../MeowshellAgentConnectionTests.cs | 55 +++++++ .../Meowshell.Tests/MeowshellServerTests.cs | 2 +- dotnet/Meowshell/MeowshellAgentConnection.cs | 33 ++-- dotnet/Meowshell/TailcatListener.cs | 13 +- .../CommentStripper/CommentStripper.csproj | 4 + 18 files changed, 468 insertions(+), 46 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 REVIEW_MAP.md create mode 100644 dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..b73a171 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,25 @@ +version: 2 +updates: + - package-ecosystem: gomod + directory: / + schedule: + interval: weekly + open-pull-requests-limit: 10 + + - package-ecosystem: nuget + directory: /dotnet + schedule: + interval: weekly + open-pull-requests-limit: 10 + + - package-ecosystem: nuget + directory: /scripts/CommentStripper + schedule: + interval: weekly + open-pull-requests-limit: 5 + + - package-ecosystem: github-actions + directory: / + schedule: + interval: weekly + open-pull-requests-limit: 10 diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 58c2c9e..6858618 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,8 +9,8 @@ on: workflow_dispatch: inputs: tailcat_ref: - description: tailcat git ref to build - default: main + description: tailcat git ref override (blank uses tailcat.ref) + default: "" android_api_level: description: minimum Android API level default: "21" @@ -41,8 +41,9 @@ jobs: # checkout for the real cross-compiled build. - name: Fetch tailcat source (for go.mod's replace directive) env: - SRC_REF: ${{ inputs.tailcat_ref || '91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a' }} + SRC_REF: ${{ inputs.tailcat_ref }} run: | + SRC_REF=${SRC_REF:-$(tr -d '[:space:]' < tailcat.ref)} git clone --depth=1 https://github.com/tailscale/tailcat.git .tailcat-src git -C .tailcat-src fetch --depth=1 origin "$SRC_REF" git -C .tailcat-src checkout --detach FETCH_HEAD @@ -56,6 +57,12 @@ jobs: go vet ./... go test ./... + - name: Scan Go call paths for known vulnerabilities + env: + GOTOOLCHAIN: auto + run: | + go run golang.org/x/vuln/cmd/govulncheck@v1.8.0 ./... + - name: Locate the NDK run: | ndk=${ANDROID_NDK_LATEST_HOME:-${ANDROID_NDK_HOME:-${ANDROID_NDK_ROOT:-}}} @@ -72,10 +79,15 @@ jobs: # tailcat's go.mod asks for a newer Go than setup-go's stable may # be; let the toolchain fetch it rather than pinning a version here. GOTOOLCHAIN: auto - SRC_REF: ${{ inputs.tailcat_ref || '91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a' }} + SRC_REF: ${{ inputs.tailcat_ref }} ANDROID_API_LEVEL: ${{ inputs.android_api_level || '21' }} run: ./build.sh + - name: Scan the patched tailcat build input for known vulnerabilities + env: + GOTOOLCHAIN: auto + run: (cd .tailcat-src && go run golang.org/x/vuln/cmd/govulncheck@v1.8.0 ./cmd/tailcat) + - name: Verify each binary targets the platform it claims run: ./verify-binaries.sh @@ -119,8 +131,9 @@ jobs: - name: Build a host tailcat to act as the client env: GOTOOLCHAIN: auto - SRC_REF: ${{ inputs.tailcat_ref || '91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a' }} + SRC_REF: ${{ inputs.tailcat_ref }} run: | + SRC_REF=${SRC_REF:-$(tr -d '[:space:]' < tailcat.ref)} git clone --depth=1 https://github.com/tailscale/tailcat.git .tailcat-src git -C .tailcat-src fetch --depth=1 origin "$SRC_REF" git -C .tailcat-src checkout --detach FETCH_HEAD @@ -234,6 +247,11 @@ jobs: # until the Pack step below has run once. run: mkdir -p nupkg + - name: Audit NuGet dependencies + run: | + dotnet restore dotnet/Meowshell.sln --force --no-cache --nologo + dotnet restore scripts/CommentStripper/CommentStripper.csproj --force --no-cache --nologo + - name: Test # The E2E tests in Meowshell.Tests run against these real binaries # instead of the fake stand-ins the rest of the suite uses; they @@ -376,8 +394,9 @@ jobs: # address -- by having a real client dial in and run commands. env: GOTOOLCHAIN: auto - SRC_REF: ${{ inputs.tailcat_ref || '91dc4979bd4ae88af6ae2c8bb549616de4bcaa5a' }} + SRC_REF: ${{ inputs.tailcat_ref }} run: | + SRC_REF=${SRC_REF:-$(tr -d '[:space:]' < tailcat.ref)} git clone --depth=1 https://github.com/tailscale/tailcat.git .tailcat-src git -C .tailcat-src fetch --depth=1 origin "$SRC_REF" git -C .tailcat-src checkout --detach FETCH_HEAD diff --git a/README.md b/README.md index 734c452..1709742 100644 --- a/README.md +++ b/README.md @@ -78,6 +78,13 @@ writes binaries to `dist/`. Android needs the NDK (`ANDROID_NDK_HOME`, r19+) for cgo-based DNS resolution; Linux and Windows are pure Go. See `./verify-binaries.sh` and `e2e/` for how CI checks the result. +## Security + +See [`SECURITY_REVIEW.md`](SECURITY_REVIEW.md) for the current threat model, +findings, accepted risks, and verification status. The accompanying +[`REVIEW_MAP.md`](REVIEW_MAP.md) groups every first-party file by runtime +boundary and records the order and focus of the repository review. + ## License [MIT](LICENSE) for this repo's own code. The vendored/patched diff --git a/REVIEW_MAP.md b/REVIEW_MAP.md new file mode 100644 index 0000000..3616be1 --- /dev/null +++ b/REVIEW_MAP.md @@ -0,0 +1,141 @@ +# Repository review map + +This is the ordered inventory used for the 2026-09-11 correctness and security +review. Files are grouped by the runtime boundary they implement rather than +alphabetically, so each producer is reviewed next to its consumers and tests. +Generated `bin/`, `obj/`, `dist/`, and `.tailcat-src/` content is excluded. + +## 1. Entrypoints, configuration, and documentation + +- `README.md`, `dotnet/README.md`, `LICENSE` +- `cmd/meowshell/main.go`, `cmd/meowshell/main_test.go` +- `cmd/meowshell/env.go`, `cmd/meowshell/env_test.go` +- `cmd/meowshell/shim_unix.go`, `cmd/meowshell/shim_windows.go` +- `cmd/meowshell/exec_unix.go`, `cmd/meowshell/exec_windows.go` + +Review focus: option defaults, unsafe modes, environment trust, executable +selection, argument boundaries, and platform-specific process replacement. +Result: insecure modes are explicit and local process launches avoid a shell. +Remote exec remains a documented SSH command-string interface, not argv-safe. + +## 2. Agent protocol and lifecycle + +- `cmd/meowshell/protocol.go`, `cmd/meowshell/protocol_test.go` +- `cmd/meowshell/agent.go`, `cmd/meowshell/agent_e2e_test.go` +- `dotnet/Meowshell/MeowshellAgentProtocol.cs` +- `dotnet/Meowshell/MeowshellAgentConnection.cs` +- `dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs` +- `dotnet/Meowshell.Tests/MeowshellAgentConnectionE2ETests.cs` +- `dotnet/Meowshell/MeowshellErrorCode.cs` + +Review focus: framing bounds, message ordering, request/channel ownership, +cancellation, child cleanup, concurrency, backpressure, and error propagation. +Result: frames are bounded and .NET channel data is serialized through a pump. +This review fixed duplicate prompt responses blocking the Go frame loop, a +prompt-close send race, cancellation being misreported as timeout, and agent +process leakage on setup failure/cancellation. + +## 3. SSH authentication, host identity, and transport + +- `cmd/meowshell/connect.go` +- `cmd/meowshell/transport.go`, `cmd/meowshell/transport_test.go` +- `cmd/meowshell/hostkeys.go` +- `cmd/meowshell/agentauth.go`, `cmd/meowshell/agentauth_test.go` +- `cmd/meowshell/sshagent_unix.go`, `cmd/meowshell/sshagent_windows.go` +- `cmd/meowshell/keystage_unix.go`, `cmd/meowshell/keystage_windows.go` +- `cmd/meowshell/sftp.go` +- `cmd/meowshell/agent_auth_e2e_test.go` +- `cmd/meowshell/agent_tcp_e2e_test.go` +- `cmd/meowshell/agent_security_e2e_test.go` +- `dotnet/Meowshell/TailcatSshSession.cs` + +Review focus: HTTPS/SOCKS proxy semantics, host-key verification, secret +handling, ssh-agent forwarding, temporary key permissions, authentication +prompts, and handshake timeouts. Result: HTTPS CONNECT now uses verified TLS; +ordinary TCP SSH uses TOFU and rejects changed keys. Tailcat's capability-based +identity intentionally does not use ordinary SSH host-key verification. + +## 4. Forwarding and SOCKS + +- `cmd/meowshell/forwarding.go`, `cmd/meowshell/forwarding_test.go` +- `cmd/meowshell/tailcatdial.go` +- `cmd/meowshell/agent_forward_e2e_test.go` +- `cmd/meowshell/agent_tailcat_forward_e2e_test.go` +- `dotnet/Meowshell/MeowshellPortForward.cs` +- `dotnet/Meowshell/MeowshellSocksProxy.cs` +- `dotnet/Meowshell.Tests/MeowshellPortForwardTests.cs` +- `dotnet/Meowshell.Tests/MeowshellSocksProxyTests.cs` +- `dotnet/Meowshell.Tests/MeowshellListenersE2ETests.cs` + +Review focus: bind scope, opt-in exposure, SOCKS authentication, constant-time +credential comparison, destination handling, socket permissions, and cleanup. +Result: TCP listeners default to loopback, TCP SOCKS defaults to random auth, +and Unix paths cannot replace non-socket files. + +## 5. SFTP and file copy + +- `cmd/meowshell/agentsftp.go`, `cmd/meowshell/agent_sftp_e2e_test.go` +- `cmd/meowshell/cp.go`, `cmd/meowshell/cp_test.go` +- `dotnet/Meowshell/TailcatFileEntry.cs` +- SFTP and transfer methods/sinks in `dotnet/Meowshell/MeowshellAgentConnection.cs` +- transfer cases in `dotnet/Meowshell.Tests/TailcatClientE2ETests.cs` + +Review focus: local/remote path classification, truncation, upload finalization, +metadata preservation, streaming/backpressure, errors, and cancellation. Result: +operations act with the connected user's authority; callers must enforce any +application-specific path allowlist before passing untrusted paths. + +## 6. .NET process wrappers and public models + +- `dotnet/Meowshell/MeowshellServer.cs` +- `dotnet/Meowshell/TailcatClient.cs` +- `dotnet/Meowshell/TailcatListener.cs` +- `dotnet/Meowshell/MeowshellProcessControl.cs`, `dotnet/Meowshell/JobObject.cs` +- `dotnet/Meowshell/BinaryLocator.cs`, `dotnet/Meowshell/MeowshellBinaries.cs` +- `dotnet/Meowshell/TailcatOptions.cs`, `TailcatAddress.cs`, + `TailcatParsedAddress.cs`, `TailcatPath.cs`, `TailcatPingResult.cs`, + `TailcatEnvironment.cs`, `TailcatDiagnostics.cs`, `TailcatException.cs`, and + `GoDuration.cs` under `dotnet/Meowshell/` +- `dotnet/Meowshell.Tests/MeowshellServerTests.cs` +- `dotnet/Meowshell.Tests/MeowshellServerE2ETests.cs` +- `dotnet/Meowshell.Tests/TailcatClientTests.cs` +- `dotnet/Meowshell.Tests/TailcatClientE2ETests.cs` + +Review focus: argument injection, process-tree cleanup, timeout/cancellation, +binary permissions/discovery, diagnostic secret exposure, parsing, and lifecycle +races. Result: `ArgumentList` and `UseShellExecute=false` are used consistently; +server addresses remain bearer secrets and must not be put in ordinary logs. + +## 7. Packaging, Android integration, and demonstrations + +- `dotnet/Meowshell/Meowshell.csproj` +- `dotnet/Meowshell.Runtime/Meowshell.Runtime.csproj` +- `dotnet/Meowshell/buildTransitive/net10.0-android36.0/Meowshell.targets` +- `dotnet/Meowshell.PackageTests/*` +- `dotnet/Meowshell.AndroidProbe/*`, `dotnet/android-probe-e2e.sh` +- `dotnet/Meowshell.Demo/*` +- `dotnet/Directory.Build.props`, `dotnet/nuget.config`, `dotnet/Meowshell.sln` +- `dotnet/verify-package.sh` + +Review focus: native-asset RID mapping, executable permissions, transitive +runtime dependencies, package consumption, Android extraction, and vulnerable +NuGet dependencies. Test dependencies were refreshed and restore now audits all +direct/transitive dependencies, failing on NU1901–NU1904. + +## 8. Build, release, and external-source controls + +- `build.sh`, `tailcat.ref`, `verify-binaries.sh` +- `.github/workflows/ci.yml`, `.github/dependabot.yml` +- `go.mod`, `go.sum` +- `patches/tailcat/*` +- `e2e/host-e2e.sh`, `e2e/android-e2e.sh`, `e2e/windows-e2e.ps1` +- `scripts/stripcomments/main.go` +- `scripts/CommentStripper/CommentStripper.csproj`, `Program.cs` + +Review focus: immutable third-party inputs, action pinning, least-privilege CI, +artifact validation, dependency advisories, publish credentials, and cross-OS +execution. Result: actions and tailcat are commit-pinned, publishing uses scoped +OIDC, `govulncheck` and transitive NuGet audit run automatically, and Dependabot +covers Go, NuGet, and Actions. Updating `tailcat.ref` still requires manual +upstream diff and patch review because Dependabot does not manage arbitrary Git +source pins. diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md index c3d1683..615bb8e 100644 --- a/SECURITY_REVIEW.md +++ b/SECURITY_REVIEW.md @@ -10,7 +10,7 @@ internals were not independently penetration-tested. ## Executive summary -The review found four actionable weaknesses and remediated them in this +The review found eight actionable weaknesses and remediated them in this revision: 1. **High — HTTPS proxy transport was not encrypted.** The `https` proxy scheme @@ -30,7 +30,24 @@ revision: symbolic links. 4. **Medium — the SSH dependency contained reachable denial-of-service flaws.** `govulncheck` identified GO-2026-6354 and GO-2026-6355 in the SSH handshake - path. `golang.org/x/crypto` is upgraded to v0.56.0, which contains both fixes. + path in both Meowshell and the built tailcat source. Both build inputs now use + `golang.org/x/crypto` v0.56.0, which contains the fixes. +5. **Medium — prompt responses could race shutdown or block the agent.** A + duplicate response filled the one-element prompt channel and blocked the only + frame reader; shutdown could also close the channel between lookup and send. + Delivery is now synchronized with shutdown and duplicate responses are + ignored without blocking. +6. **Medium — cancelled or failed .NET connection setup leaked its child.** A + cancellation was also surfaced as a timeout. Setup now disposes the agent on + every exceptional path and preserves `OperationCanceledException` semantics. +7. **Medium — HTTP proxy setup could hang and expose malformed credentials.** + Cancellation only covered dialing, not the CONNECT response, and URL parse + diagnostics could echo embedded passwords. Cancellation now closes the live + connection, diagnostics redact or omit credentials, and default HTTP(S) + proxy ports are supported. +8. **Low — a fast listener failure could be missed.** The .NET wrapper started + a process before subscribing to its exit event, so `Completed` could hang if + the child exited in that window. Event handlers are now installed first. No hard-coded credentials, shell-based local process launch, unrestricted local TCP bind by default, unbounded protocol frame allocation, or silent TCP @@ -89,7 +106,44 @@ mutate the socket's parent directory. Callers should place sockets in a private The reviewed v0.55.0 dependency allowed a peer to deadlock established or undecided SSH channels (GO-2026-6354 and GO-2026-6355). Because the agent calls `ssh.NewClientConn`, `govulncheck` found a reachable trace through -`dialSSHClient`. The direct dependency is upgraded to the fixed v0.56.0 release. +`dialSSHClient`; scanning the nested tailcat command found server and client +traces as well. The direct dependency and patched tailcat build module are both +upgraded to the fixed v0.56.0 release, and CI scans both modules. + +### SR-05: prompt response concurrency hazards — fixed (Medium) + +Prompt channels are closed during agent shutdown. Response delivery previously +released the prompt-map lock before sending, permitting a send-on-closed-channel +panic. A duplicate response could instead block forever on the full buffered +channel and stop all subsequent frames. Delivery now holds the lifecycle lock +and uses a non-blocking send; a regression test covers duplicate responses. + +### SR-06: .NET agent leak and cancellation misclassification — fixed (Medium) + +Once the agent child was started, exceptions while sending configuration or +waiting for connection escaped without disposal. In addition, cancellation won +the same `WhenAny` branch as timeout and was reported as a `TailcatException`. +Connection setup now has one exception cleanup path and checks caller +cancellation before creating a timeout error. A process-backed test verifies +both cancellation type and child cleanup. + +### SR-07: CONNECT cancellation and credential-safe errors — fixed (Medium) + +After the proxy TCP/TLS connection completed, a server that never returned an +HTTP response could hold connection setup forever despite caller cancellation. +`context.AfterFunc` now closes the connection while setup is in progress. Proxy +URL parse failures no longer include the raw credential-bearing URL, later +errors use `URL.Redacted`, missing hosts are rejected, and omitted HTTP/HTTPS +ports resolve to 80/443. Regression tests cover cancellation and secret-safe +parse errors. + +### SR-08: listener exit-subscription race — fixed (Low) + +`TailcatListener.Start` enabled events and launched the child before registering +its `Exited` callback. A child that failed immediately could exit in between, +leaving `Completed` unresolved and lifecycle callers waiting indefinitely. Exit +and output handlers are now registered before process start; existing crash +tests exercise the fast-failure path. ## Accepted design risks and hardening backlog @@ -119,9 +173,10 @@ undecided SSH channels (GO-2026-6354 and GO-2026-6355). Because the agent calls requests. 6. **Build inputs remain broader than one repository.** Go modules, NuGet packages, the Go toolchain, Android NDK, and publishing infrastructure remain - external trust dependencies. Add automated `govulncheck`, NuGet audit/SBOM + external trust dependencies. Automated `govulncheck`, transitive NuGet audit, + and Dependabot now cover known advisories and routine upgrades. SBOM generation, artifact provenance/attestation, and release verification against - reproducible hashes. + reproducible hashes remain valuable follow-ups. 7. **Secret lifetime is not minimized everywhere.** Passwords, passphrases, and private keys cross managed strings/arrays or Go byte slices and cannot always be reliably zeroed. Prefer ssh-agent and Android Keystore sign callbacks over diff --git a/build.sh b/build.sh index ad8db98..faeec36 100755 --- a/build.sh +++ b/build.sh @@ -52,7 +52,7 @@ git -C "$SRC_DIR" apply "$REPO_DIR/patches/tailcat/pickregion-nil-ifstate.patch" # file's comments for the detail. Added via `go get`, which computes # go.mod/go.sum correctly, rather than hand-patching them. git -C "$SRC_DIR" apply "$REPO_DIR/patches/tailcat/android-netmon-interface-getter.patch" -(cd "$SRC_DIR" && go get github.com/wlynxg/anet@v0.0.5) +(cd "$SRC_DIR" && go get github.com/wlynxg/anet@v0.0.5 golang.org/x/crypto@v0.56.0) # tailcat's SSH server hardcodes /bin/sh and /usr/local/bin:/usr/bin:/bin # for the session shell and PATH, neither of which exist on Android -- diff --git a/cmd/meowshell/agent.go b/cmd/meowshell/agent.go index 9ad16af..f463b01 100644 --- a/cmd/meowshell/agent.go +++ b/cmd/meowshell/agent.go @@ -500,10 +500,16 @@ func (a *agentSession) handleControl(channelID uint32, payload []byte) { func (a *agentSession) deliverPromptResponse(msg controlMessage) { a.promptsMu.Lock() + defer a.promptsMu.Unlock() ch := a.prompts[msg.RequestID] - a.promptsMu.Unlock() if ch != nil { - ch <- msg + // Ignore a duplicate response instead of blocking the only frame reader. + // Holding promptsMu also prevents closeAllPrompts from closing ch between + // the lookup and send. + select { + case ch <- msg: + default: + } } } diff --git a/cmd/meowshell/agentauth_test.go b/cmd/meowshell/agentauth_test.go index 592806e..db5138e 100644 --- a/cmd/meowshell/agentauth_test.go +++ b/cmd/meowshell/agentauth_test.go @@ -7,10 +7,33 @@ import ( "encoding/pem" "io" "testing" + "time" "golang.org/x/crypto/ssh" ) +func TestDeliverPromptResponseDoesNotBlockOnDuplicate(t *testing.T) { + session := newAgentSession(nil, nil) + responses := make(chan controlMessage, 1) + session.prompts["p1"] = responses + + done := make(chan struct{}) + go func() { + session.deliverPromptResponse(controlMessage{RequestID: "p1", Answer: "first"}) + session.deliverPromptResponse(controlMessage{RequestID: "p1", Answer: "duplicate"}) + close(done) + }() + + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("a duplicate prompt response blocked the frame reader") + } + if got := (<-responses).Answer; got != "first" { + t.Fatalf("delivered answer = %q, want first", got) + } +} + func testAuthSession(t *testing.T) (session *agentSession, fromAgent io.Reader, toAgent io.Writer) { t.Helper() agentIn, clientOut := io.Pipe() diff --git a/cmd/meowshell/transport.go b/cmd/meowshell/transport.go index f1c8e01..84c9e12 100644 --- a/cmd/meowshell/transport.go +++ b/cmd/meowshell/transport.go @@ -72,13 +72,18 @@ func splitUserHost(dest, defaultPort string) (user, hostPort string) { func proxyDialer(proxyURL, hostPort string) (dialer, error) { u, err := url.Parse(proxyURL) if err != nil { - return nil, fmt.Errorf("invalid --proxy %q: %w", proxyURL, err) + // url.ParseError includes the original URL, which can contain a proxy + // password. Do not copy it into diagnostics. + return nil, fmt.Errorf("invalid --proxy URL") + } + if u.Hostname() == "" { + return nil, fmt.Errorf("invalid --proxy URL: missing host") } switch u.Scheme { case "socks5", "socks5h": d, err := proxy.SOCKS5("tcp", u.Host, proxyAuthFromURL(u), proxy.Direct) if err != nil { - return nil, fmt.Errorf("configuring SOCKS5 proxy %q: %w", proxyURL, err) + return nil, fmt.Errorf("configuring SOCKS5 proxy %q: %w", u.Redacted(), err) } return func(ctx context.Context) (net.Conn, error) { if cd, ok := d.(proxy.ContextDialer); ok { @@ -105,17 +110,27 @@ func proxyAuthFromURL(u *url.URL) *proxy.Auth { func dialHTTPConnectProxy(ctx context.Context, proxyURL *url.URL, hostPort string) (net.Conn, error) { d := net.Dialer{Timeout: tcpDialTimeout} + proxyAddr := proxyURL.Host + if proxyURL.Port() == "" { + port := "80" + if proxyURL.Scheme == "https" { + port = "443" + } + proxyAddr = net.JoinHostPort(proxyURL.Hostname(), port) + } var conn net.Conn var err error if proxyURL.Scheme == "https" { tlsDialer := tls.Dialer{NetDialer: &d} - conn, err = tlsDialer.DialContext(ctx, "tcp", proxyURL.Host) + conn, err = tlsDialer.DialContext(ctx, "tcp", proxyAddr) } else { - conn, err = d.DialContext(ctx, "tcp", proxyURL.Host) + conn, err = d.DialContext(ctx, "tcp", proxyAddr) } if err != nil { return nil, err } + stopCancellation := context.AfterFunc(ctx, func() { conn.Close() }) + defer stopCancellation() var authHeader string if proxyURL.User != nil { pass, _ := proxyURL.User.Password() @@ -130,12 +145,19 @@ func dialHTTPConnectProxy(ctx context.Context, proxyURL *url.URL, hostPort strin resp, err := http.ReadResponse(br, &http.Request{Method: "CONNECT"}) if err != nil { conn.Close() + if ctxErr := ctx.Err(); ctxErr != nil { + return nil, ctxErr + } return nil, err } if resp.StatusCode != http.StatusOK { conn.Close() return nil, fmt.Errorf("HTTP CONNECT proxy %s refused: %s", proxyURL.Host, resp.Status) } + if err := ctx.Err(); err != nil { + conn.Close() + return nil, err + } return &bufConn{Conn: conn, r: br}, nil } diff --git a/cmd/meowshell/transport_test.go b/cmd/meowshell/transport_test.go index 267bf98..8431d55 100644 --- a/cmd/meowshell/transport_test.go +++ b/cmd/meowshell/transport_test.go @@ -3,10 +3,13 @@ package main import ( "bufio" "context" + "errors" "fmt" + "io" "net" "net/http" "net/url" + "strings" "testing" "time" ) @@ -127,6 +130,54 @@ func TestDialHTTPConnectProxyRejectsNon200(t *testing.T) { } } +func TestDialHTTPConnectProxyHonorsCancellationAfterDial(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + + accepted := make(chan struct{}) + go func() { + conn, err := ln.Accept() + if err != nil { + return + } + defer conn.Close() + close(accepted) + io.Copy(io.Discard, conn) + }() + + ctx, cancel := context.WithCancel(context.Background()) + proxyURL := mustParseURL(t, "http://"+ln.Addr().String()) + done := make(chan error, 1) + go func() { + _, err := dialHTTPConnectProxy(ctx, proxyURL, "backend.example:22") + done <- err + }() + <-accepted + cancel() + select { + case err := <-done: + if !errors.Is(err, context.Canceled) { + t.Fatalf("dialHTTPConnectProxy cancellation = %v, want context.Canceled", err) + } + case <-time.After(time.Second): + t.Fatal("cancellation did not interrupt the proxy response read") + } +} + +func TestProxyDialerDoesNotExposeMalformedURLPassword(t *testing.T) { + const secret = "super-secret-password" + _, err := proxyDialer("http://user:"+secret+"%zz@example.com", "backend.example:22") + if err == nil { + t.Fatal("proxyDialer accepted a malformed URL") + } + if strings.Contains(err.Error(), secret) { + t.Fatalf("proxy parse error exposed the password: %v", err) + } +} + func TestDialHTTPSConnectProxyUsesTLS(t *testing.T) { ln, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { diff --git a/dotnet/Directory.Build.props b/dotnet/Directory.Build.props index 971a5fe..2dcc031 100644 --- a/dotnet/Directory.Build.props +++ b/dotnet/Directory.Build.props @@ -9,6 +9,12 @@ 0 $(MajorMinor).$(BuildNumber) $(MeowshellVersion) + + true + all + low + $(WarningsAsErrors);NU1901;NU1902;NU1903;NU1904 diff --git a/dotnet/Meowshell.PackageTests/Meowshell.PackageTests.csproj b/dotnet/Meowshell.PackageTests/Meowshell.PackageTests.csproj index dda39b7..dd81d7a 100644 --- a/dotnet/Meowshell.PackageTests/Meowshell.PackageTests.csproj +++ b/dotnet/Meowshell.PackageTests/Meowshell.PackageTests.csproj @@ -21,13 +21,13 @@ - - - + + + runtime; build; native; contentfiles; analyzers; buildtransitive all - + runtime; build; native; contentfiles; analyzers; buildtransitive all diff --git a/dotnet/Meowshell.Tests/Meowshell.Tests.csproj b/dotnet/Meowshell.Tests/Meowshell.Tests.csproj index 92826a5..a2f63de 100644 --- a/dotnet/Meowshell.Tests/Meowshell.Tests.csproj +++ b/dotnet/Meowshell.Tests/Meowshell.Tests.csproj @@ -12,13 +12,13 @@ - - - + + + runtime; build; native; contentfiles; analyzers; buildtransitive all - + runtime; build; native; contentfiles; analyzers; buildtransitive all diff --git a/dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs b/dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs new file mode 100644 index 0000000..21f1e6e --- /dev/null +++ b/dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs @@ -0,0 +1,55 @@ +using System.Diagnostics; +using Meowshell; + +namespace Meowshell.Tests; + +public sealed class MeowshellAgentConnectionTests : IDisposable +{ + private readonly string _dir = Directory.CreateTempSubdirectory("meowshell-agent-test-").FullName; + + public void Dispose() => Directory.Delete(_dir, recursive: true); + + [Fact] + public async Task CancellationStopsTheAgentAndRemainsCancellation() + { + if (OperatingSystem.IsWindows()) return; + + var bin = Path.Combine(_dir, "bin"); + Directory.CreateDirectory(bin); + var pidFile = Path.Combine(_dir, "agent.pid"); + var script = $"#!/bin/sh\nprintf '%s' $$ > '{pidFile}'\nexec sleep 30\n"; + var naming = BinaryNaming.ForCurrentPlatform(); + foreach (var name in new[] { naming.FileName("meowshell"), naming.FileName("tailcat") }) + { + var path = Path.Combine(bin, name); + await File.WriteAllTextAsync(path, script); + File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + } + + using var cancellation = new CancellationTokenSource(TimeSpan.FromMilliseconds(250)); + await Assert.ThrowsAnyAsync(() => + MeowshellAgentConnection.ConnectAsync(new TailcatClientOptions + { + BinaryDirectory = bin, + HomeDirectory = Path.Combine(_dir, "home"), + Timeout = TimeSpan.FromSeconds(10), + }, "example.invalid", cancellationToken: cancellation.Token)); + + Assert.True(File.Exists(pidFile), "the agent stand-in never started"); + var pid = int.Parse(await File.ReadAllTextAsync(pidFile), System.Globalization.CultureInfo.InvariantCulture); + Assert.False(IsRunning(pid), "the cancelled connection leaked its agent process"); + } + + private static bool IsRunning(int pid) + { + try + { + using var process = Process.GetProcessById(pid); + return !process.HasExited; + } + catch (ArgumentException) + { + return false; + } + } +} diff --git a/dotnet/Meowshell.Tests/MeowshellServerTests.cs b/dotnet/Meowshell.Tests/MeowshellServerTests.cs index 7fe9d6b..0c21789 100644 --- a/dotnet/Meowshell.Tests/MeowshellServerTests.cs +++ b/dotnet/Meowshell.Tests/MeowshellServerTests.cs @@ -179,7 +179,7 @@ public async Task TheServerIsToldWhereTailcatIs() await using var server = await MeowshellServer.StartAsync(options); var lines = File.ReadAllLines(seen); - Assert.Equal(Path.Combine(options.BinaryDirectory, "libtailcat.so"), lines[0]); + Assert.Equal(Path.Combine(options.BinaryDirectory!, "libtailcat.so"), lines[0]); Assert.Equal(options.HomeDirectory, lines[1]); } diff --git a/dotnet/Meowshell/MeowshellAgentConnection.cs b/dotnet/Meowshell/MeowshellAgentConnection.cs index 12fb8f5..2bf6d65 100644 --- a/dotnet/Meowshell/MeowshellAgentConnection.cs +++ b/dotnet/Meowshell/MeowshellAgentConnection.cs @@ -135,25 +135,32 @@ public static async Task ConnectAsync( var process = new Process { StartInfo = psi, EnableRaisingEvents = true }; MeowshellProcessControl.Start(process); var connection = new MeowshellAgentConnection(process); - - process.ErrorDataReceived += (_, e) => + try { - if (e.Data is null) return; - connection._diagnostics.Add(e.Data); - connection.Log?.Invoke(e.Data); - }; - process.BeginErrorReadLine(); + process.ErrorDataReceived += (_, e) => + { + if (e.Data is null) return; + connection._diagnostics.Add(e.Data); + connection.Log?.Invoke(e.Data); + }; + process.BeginErrorReadLine(); - await connection.SendConfigureAsync(configure ?? new MeowshellAgentConfigureOptions(), proxyUrl, cancellationToken).ConfigureAwait(false); + await connection.SendConfigureAsync(configure ?? new MeowshellAgentConfigureOptions(), proxyUrl, cancellationToken).ConfigureAwait(false); - var settled = await Task.WhenAny(connection._connected.Task, Task.Delay(options.Timeout, cancellationToken)).ConfigureAwait(false); - if (settled != connection._connected.Task) + var settled = await Task.WhenAny(connection._connected.Task, Task.Delay(options.Timeout, cancellationToken)).ConfigureAwait(false); + if (settled != connection._connected.Task) + { + cancellationToken.ThrowIfCancellationRequested(); + throw new TailcatException("meowshell agent did not connect in time", 0, connection._diagnostics.Tail(), MeowshellErrorCode.Timeout); + } + await connection._connected.Task.ConfigureAwait(false); + return connection; + } + catch { await connection.DisposeAsync().ConfigureAwait(false); - throw new TailcatException("meowshell agent did not connect in time", 0, connection._diagnostics.Tail(), MeowshellErrorCode.Timeout); + throw; } - await connection._connected.Task.ConfigureAwait(false); - return connection; } private Task SendConfigureAsync(MeowshellAgentConfigureOptions configure, string? proxyUrl, CancellationToken cancellationToken) => diff --git a/dotnet/Meowshell/TailcatListener.cs b/dotnet/Meowshell/TailcatListener.cs index d1dc0b9..493c13c 100644 --- a/dotnet/Meowshell/TailcatListener.cs +++ b/dotnet/Meowshell/TailcatListener.cs @@ -35,12 +35,8 @@ public static TailcatListener Start(Process process, TimeSpan gracePeriod, Actio { process.EnableRaisingEvents = true; var listener = new TailcatListener(process, gracePeriod); - MeowshellProcessControl.Start(process); - if (OperatingSystem.IsWindows()) - { - listener._job = JobObject.Wrap(process); - } - + // Subscribe before Start: a malformed command can exit quickly enough + // that registering afterwards misses Exited and leaves Completed hung. process.Exited += async (_, _) => { await process.WaitForExitAsync().ConfigureAwait(false); @@ -56,6 +52,11 @@ public static TailcatListener Start(Process process, TimeSpan gracePeriod, Actio listener.Log?.Invoke(e.Data); onLog?.Invoke(e.Data); }; + MeowshellProcessControl.Start(process); + if (OperatingSystem.IsWindows()) + { + listener._job = JobObject.Wrap(process); + } process.BeginOutputReadLine(); process.BeginErrorReadLine(); return listener; diff --git a/scripts/CommentStripper/CommentStripper.csproj b/scripts/CommentStripper/CommentStripper.csproj index 707e6f5..bd7cad1 100644 --- a/scripts/CommentStripper/CommentStripper.csproj +++ b/scripts/CommentStripper/CommentStripper.csproj @@ -5,6 +5,10 @@ net8.0 enable enable + true + all + low + $(WarningsAsErrors);NU1901;NU1902;NU1903;NU1904 From 8eadb86179d21d65074225990ab4ea1e2a2c7636 Mon Sep 17 00:00:00 2001 From: Stefan van der Merwe <44154511+Sniperlyf3@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:42:21 +0200 Subject: [PATCH 4/8] Remove agent cancellation test timing race --- .../MeowshellAgentConnectionTests.cs | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs b/dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs index 21f1e6e..bfb808f 100644 --- a/dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs +++ b/dotnet/Meowshell.Tests/MeowshellAgentConnectionTests.cs @@ -26,20 +26,34 @@ public async Task CancellationStopsTheAgentAndRemainsCancellation() File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); } - using var cancellation = new CancellationTokenSource(TimeSpan.FromMilliseconds(250)); - await Assert.ThrowsAnyAsync(() => - MeowshellAgentConnection.ConnectAsync(new TailcatClientOptions - { - BinaryDirectory = bin, - HomeDirectory = Path.Combine(_dir, "home"), - Timeout = TimeSpan.FromSeconds(10), - }, "example.invalid", cancellationToken: cancellation.Token)); - - Assert.True(File.Exists(pidFile), "the agent stand-in never started"); + using var cancellation = new CancellationTokenSource(); + var connecting = MeowshellAgentConnection.ConnectAsync(new TailcatClientOptions + { + BinaryDirectory = bin, + HomeDirectory = Path.Combine(_dir, "home"), + Timeout = TimeSpan.FromSeconds(10), + }, "example.invalid", cancellationToken: cancellation.Token); + + // Synchronize with the child instead of cancelling after an arbitrary + // delay: process startup can legitimately exceed 250 ms on a busy CI + // runner, which made this regression test test scheduler speed. + await WaitForFileAsync(pidFile, TimeSpan.FromSeconds(5)); + cancellation.Cancel(); + await Assert.ThrowsAnyAsync(() => connecting); + var pid = int.Parse(await File.ReadAllTextAsync(pidFile), System.Globalization.CultureInfo.InvariantCulture); Assert.False(IsRunning(pid), "the cancelled connection leaked its agent process"); } + private static async Task WaitForFileAsync(string path, TimeSpan timeout) + { + using var cancellation = new CancellationTokenSource(timeout); + while (!File.Exists(path)) + { + await Task.Delay(10, cancellation.Token); + } + } + private static bool IsRunning(int pid) { try From 6b53a93922165360041a2704c2bf92451e74cce1 Mon Sep 17 00:00:00 2001 From: Stefan van der Merwe <44154511+Sniperlyf3@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:23:34 +0200 Subject: [PATCH 5/8] Bound protocol resources and handshake lifetimes --- REVIEW_MAP.md | 2 +- SECURITY_REVIEW.md | 133 ++++++++++++++++++- cmd/meowshell/protocol.go | 16 ++- cmd/meowshell/protocol_test.go | 33 +++++ cmd/meowshell/sftp.go | 35 +++-- cmd/meowshell/sftp_test.go | 28 ++++ cmd/meowshell/transport.go | 2 +- dotnet/Meowshell/MeowshellAgentConnection.cs | 45 ++++--- dotnet/Meowshell/MeowshellAgentProtocol.cs | 7 +- 9 files changed, 265 insertions(+), 36 deletions(-) create mode 100644 cmd/meowshell/sftp_test.go diff --git a/REVIEW_MAP.md b/REVIEW_MAP.md index 3616be1..5f7308a 100644 --- a/REVIEW_MAP.md +++ b/REVIEW_MAP.md @@ -43,7 +43,7 @@ process leakage on setup failure/cancellation. - `cmd/meowshell/agentauth.go`, `cmd/meowshell/agentauth_test.go` - `cmd/meowshell/sshagent_unix.go`, `cmd/meowshell/sshagent_windows.go` - `cmd/meowshell/keystage_unix.go`, `cmd/meowshell/keystage_windows.go` -- `cmd/meowshell/sftp.go` +- `cmd/meowshell/sftp.go`, `cmd/meowshell/sftp_test.go` - `cmd/meowshell/agent_auth_e2e_test.go` - `cmd/meowshell/agent_tcp_e2e_test.go` - `cmd/meowshell/agent_security_e2e_test.go` diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md index 615bb8e..368f4ee 100644 --- a/SECURITY_REVIEW.md +++ b/SECURITY_REVIEW.md @@ -10,7 +10,7 @@ internals were not independently penetration-tested. ## Executive summary -The review found eight actionable weaknesses and remediated them in this +The review found eleven actionable weaknesses and remediated them in this revision: 1. **High — HTTPS proxy transport was not encrypted.** The `https` proxy scheme @@ -48,6 +48,17 @@ revision: 8. **Low — a fast listener failure could be missed.** The .NET wrapper started a process before subscribing to its exit event, so `Completed` could hang if the child exited in that window. Event handlers are now installed first. +9. **High — remote output could grow managed memory without bound.** The .NET + multiplexer drained the child continuously into an unbounded per-channel + queue. A hostile SSH peer could exhaust the embedding application's memory. + Queues are now bounded and the protocol reader applies backpressure. +10. **Medium — subprocess-backed SSH handshakes could ignore cancellation and + timeout.** The pipe transport's deadline methods are necessarily no-ops. + Handshake cancellation now closes the connection and `tailcat` is launched + with `CommandContext`, guaranteeing termination at the context deadline. +11. **Low — protocol frame writes assumed one full writer call.** A legal short + write could truncate a frame and desynchronize the protocol. Writes now loop + to completion, and both implementations reject oversized outbound frames. No hard-coded credentials, shell-based local process launch, unrestricted local TCP bind by default, unbounded protocol frame allocation, or silent TCP @@ -72,6 +83,55 @@ Unix-socket permissions, and commit-pinned GitHub Actions. - **Build/release:** upstream tailcat source, Go/NuGet dependencies, downloaded toolchains, CI actions, native artifacts, and NuGet trusted publishing. +## Audit execution plan + +The review is repeatable in six ordered phases; `REVIEW_MAP.md` is the file-level +checklist for phases two through four. + +1. **Baseline and inventory:** freeze the PR revision and tailcat pin; enumerate + source, generated exclusions, dependencies, entrypoints, platforms, secrets, + privileges, and network/filesystem/process boundaries. +2. **Adversarial data flow:** trace every untrusted value from CLI/.NET API or + remote frame through parsing, validation, allocation, logging, persistence, + process arguments, authentication, forwarding, SFTP, and response handling. +3. **Lifecycle and concurrency:** evaluate startup, steady state, cancellation, + timeout, partial I/O, backpressure, duplicate/out-of-order messages, abrupt + EOF, process crashes, repeated disposal, and concurrent channels/prompts. +4. **Platform and supply chain:** compare Unix/Windows/Android behavior; inspect + build patches, mutable inputs, CI permissions/actions, artifacts, package RID + layout, publishing identity, and dependency advisory coverage. +5. **Exploit-oriented validation:** add deterministic regressions for confirmed + defects, exercise protocol limits and races, run unit/E2E tests, race/static + analyzers, Go call-path vulnerability scans, and transitive NuGet audits. +6. **Disposition and release gate:** record fixed, accepted, transferred, and + unverified risks with owners/conditions. A release passes only when required + CI/platform E2E jobs and dependency scans pass against the exact artifacts. + +No finite source audit can prove that every scenario is safe. This plan instead +enumerates the relevant attacker capabilities, failure modes, and trust +boundaries, states exclusions explicitly, and leaves uncertain claims as +release gates or residual risks rather than silently treating them as safe. + +## Security objectives and attacker models + +The audit protects four primary assets: shell execution authority, SSH/private +key material, bearer tailcat addresses and proxy credentials, and availability +of the embedding application/device. It considered: + +- an unauthenticated Internet peer that learns or guesses an address; +- an authenticated but malicious SSH server or shell account; +- a malicious SOCKS/HTTP proxy, DERP relay, or on-path observer; +- a local unprivileged process racing files, sockets, agents, or child startup; +- an application caller supplying malformed, oversized, or adversarial options; +- a compromised package/module source, mutable Git ref, CI action, or artifact; +- abrupt cancellation, process death, short I/O, resource exhaustion, duplicate + messages, reordered lifecycle events, and platform-specific behavior. + +The review does not claim protection from a compromised OS/kernel, a process +with the same effective account that can debug/read this process, a malicious +application intentionally invoking unsafe options, compromise of GitHub/NuGet +administrative accounts, or undiscovered defects in cryptographic primitives. + ## Findings and disposition ### SR-01: HTTPS proxy scheme used plaintext TCP — fixed (High) @@ -145,6 +205,38 @@ leaving `Completed` unresolved and lifecycle callers waiting indefinitely. Exit and output handlers are now registered before process start; existing crash tests exercise the fast-failure path. +### SR-09: unbounded managed channel buffering — fixed (High) + +Every frame from the Go child was appended to an unbounded .NET channel while a +separate pump waited for the application to consume its `Pipe`. Consequently a +remote command that emitted data faster than the application read it could grow +memory until process termination. The queue now holds at most 32 frame/control +items. The main protocol loop awaits queue admission, preserving frame order and +propagating backpressure through the child pipe and SSH channel. The tradeoff is +intentional head-of-line blocking across multiplexed channels under a stalled +consumer, which is preferable to unbounded memory consumption. +Shutdown explicitly faults active sinks before awaiting the reader, so a +consumer that has stopped reading cannot turn that backpressure into a shutdown +deadlock. + +### SR-10: ineffective pipe-transport handshake deadlines — fixed (Medium) + +SSH applies network deadlines through `net.Conn`, but the subprocess pipe +adapter cannot implement kernel socket deadlines and returned success without +enforcement. A stuck or replaced tailcat executable could therefore outlive the +nominal handshake timeout. `dialSSHClient` now derives a bounded handshake +context and closes any transport when it expires; tailcat subprocesses use +`exec.CommandContext`; and `pipeConn.Close` is idempotent so cancellation and +normal cleanup may safely race. + +### SR-11: partial and oversized outbound frames — fixed (Low) + +Go's `io.Writer` contract permits a short write. The protocol writer previously +ignored the byte count, potentially emitting a truncated frame followed by an +unparseable stream. It now writes until all bytes are accepted and fails on no +progress. Both Go and .NET writers enforce the same 64 MiB outbound bound already +used by readers, preventing integer/size mismatches and fail-late allocations. + ## Accepted design risks and hardening backlog 1. **Tailcat SSH host-key checking is intentionally disabled.** Tailcat sessions @@ -182,6 +274,34 @@ tests exercise the fast-failure path. be reliably zeroed. Prefer ssh-agent and Android Keystore sign callbacks over raw private-key/password configuration. +## Scenario coverage matrix + +| Scenario | Control or disposition | +| --- | --- | +| Address disclosure | Prefer authorized keys/client allowlists; address-only mode remains explicit risk. | +| TCP SSH MITM | TOFU known-hosts; changed keys fail closed. | +| Tailcat SSH identity | Capability-address security assumption; tracked as accepted design risk. | +| Malicious HTTPS proxy/on-path peer | Certificate-verified TLS before credentials or CONNECT. | +| Proxy stalls after accepting TCP | Context cancellation closes the live connection. | +| Proxy URL contains secrets | Raw parse errors are suppressed; parsed URLs are redacted. | +| Public local forward/proxy | Refused unless the caller opts into non-loopback bind. | +| Unauthenticated SOCKS use | TCP SOCKS generates credentials by default; constant-time comparison. | +| Unix socket path substitution | Symlinks and non-sockets are refused; private parent directory recommended. | +| Oversized/malformed IPC frame | 64 MiB inbound/outbound cap and structural header checks. | +| Short IPC write | Full-write loop or error; regression coverage included. | +| Duplicate/racing prompt response | Non-blocking delivery under the lifecycle lock. | +| Hostile high-volume command output | Bounded per-channel buffering with transport backpressure. | +| Stalled SSH handshake/subprocess | Bounded context, connection close, and command-context kill. | +| Cancellation during .NET startup | Correct cancellation type and unconditional child cleanup. | +| Child exits immediately | Exit handlers registered before start. | +| SFTP path abuse | Executes with SSH user's authority; embedding app must impose narrower allowlists. | +| Remote command injection | API explicitly transports a shell command string; untrusted concatenation forbidden. | +| Compromised upstream branch | Reviewed full tailcat commit pin; override is explicit. | +| Known vulnerable dependency | CI scans both Go modules and all restorable NuGet graphs; warnings fail builds. | +| Compromised CI action | Actions are full-commit pinned and workflow defaults to read-only contents. | +| Package publication credential theft | NuGet trusted publishing uses short-lived scoped OIDC. | +| Artifact/platform mismatch | ELF/PE architecture and Android-loader verification plus host/device E2E. | + ## Verification plan The security fixes are covered by focused Go tests. The standard repository @@ -193,8 +313,9 @@ a NuGet dependency audit in an environment with the required .NET SDK. ## Review verification results The review environment was subsequently provisioned with .NET SDK 8.0.425 and -the pinned Linux amd64 binaries. All 86 non-E2E .NET tests passed. The complete -suite executed all 107 tests: 94 passed and 13 relay-dependent E2E cases failed +the pinned Linux amd64 binaries. At that revision all 86 non-E2E .NET tests +passed. The then-current complete suite executed all 107 tests: 94 passed and +13 relay-dependent E2E cases failed with `context deadline exceeded` or an SSH EOF after the tailcat connection could not be established. Local real-binary E2E coverage, including listener startup/shutdown and key/address operations, did pass. The environment routes @@ -203,3 +324,9 @@ the tailcat transport being unable to reach its relay from this environment. They are therefore recorded as an environment limitation rather than a passing result or a demonstrated product regression. The full E2E suite still needs a run from a network that permits the tailcat transport before release. + +After the additional PR hardening, all 87 current non-E2E .NET tests pass. Go +unit tests, focused race-detector tests, `go vet`, root and patched-tailcat +`govulncheck`, transitive NuGet audit, workflow lint, and shell syntax checks +also pass. Network/device E2E remains a required CI release gate rather than a +claim derived from the restricted review container. diff --git a/cmd/meowshell/protocol.go b/cmd/meowshell/protocol.go index 2b032c4..a40e4ff 100644 --- a/cmd/meowshell/protocol.go +++ b/cmd/meowshell/protocol.go @@ -27,13 +27,25 @@ type frame struct { } func writeFrame(w io.Writer, f frame) error { + if len(f.Payload) > maxFrameLength-frameHeaderLength { + return fmt.Errorf("frame payload length %d exceeds the %d limit", len(f.Payload), maxFrameLength-frameHeaderLength) + } buf := make([]byte, 4+frameHeaderLength+len(f.Payload)) binary.BigEndian.PutUint32(buf[0:4], uint32(frameHeaderLength+len(f.Payload))) buf[4] = f.Type binary.BigEndian.PutUint32(buf[5:9], f.ChannelID) copy(buf[9:], f.Payload) - _, err := w.Write(buf) - return err + for len(buf) > 0 { + n, err := w.Write(buf) + if err != nil { + return err + } + if n <= 0 { + return io.ErrShortWrite + } + buf = buf[n:] + } + return nil } func readFrame(r io.Reader) (frame, error) { diff --git a/cmd/meowshell/protocol_test.go b/cmd/meowshell/protocol_test.go index a824497..229894d 100644 --- a/cmd/meowshell/protocol_test.go +++ b/cmd/meowshell/protocol_test.go @@ -8,6 +8,18 @@ import ( "testing" ) +type shortWriter struct { + bytes.Buffer + max int +} + +func (w *shortWriter) Write(p []byte) (int, error) { + if len(p) > w.max { + p = p[:w.max] + } + return w.Buffer.Write(p) +} + func TestFrameRoundTrip(t *testing.T) { cases := []frame{ {Type: frameTypeControl, ChannelID: 0, Payload: []byte(`{"msg":"open_channel"}`)}, @@ -47,6 +59,27 @@ func TestReadFrameMultipleInSequence(t *testing.T) { } } +func TestWriteFrameCompletesShortWrites(t *testing.T) { + w := &shortWriter{max: 3} + want := frame{Type: frameTypeData, ChannelID: 42, Payload: []byte("payload")} + if err := writeFrame(w, want); err != nil { + t.Fatalf("writeFrame with short writer: %v", err) + } + got, err := readFrame(&w.Buffer) + if err != nil { + t.Fatalf("readFrame after short writes: %v", err) + } + if got.Type != want.Type || got.ChannelID != want.ChannelID || !bytes.Equal(got.Payload, want.Payload) { + t.Fatalf("frame after short writes = %+v, want %+v", got, want) + } +} + +func TestWriteFrameRejectsOversizedPayload(t *testing.T) { + if err := writeFrame(io.Discard, frame{Payload: make([]byte, maxFrameLength-frameHeaderLength+1)}); err == nil { + t.Fatal("writeFrame accepted an oversized payload") + } +} + func TestReadFrameRejectsOversizedLength(t *testing.T) { r := strings.NewReader(string([]byte{0xff, 0xff, 0xff, 0xff})) if _, err := readFrame(r); err == nil { diff --git a/cmd/meowshell/sftp.go b/cmd/meowshell/sftp.go index 4afa632..eac054b 100644 --- a/cmd/meowshell/sftp.go +++ b/cmd/meowshell/sftp.go @@ -8,6 +8,7 @@ import ( "net" "os/exec" "strings" + "sync" "time" "github.com/pkg/sftp" @@ -32,21 +33,24 @@ type pipeConn struct { cmd *exec.Cmd stdout io.ReadCloser stdin io.WriteCloser + once sync.Once + err error } func (c *pipeConn) Read(p []byte) (int, error) { return c.stdout.Read(p) } func (c *pipeConn) Write(p []byte) (int, error) { return c.stdin.Write(p) } func (c *pipeConn) Close() error { - c.stdin.Close() - c.stdout.Close() - err := c.cmd.Wait() - - var exitErr *exec.ExitError - if errors.As(err, &exitErr) { - return nil - } - return err + c.once.Do(func() { + c.stdin.Close() + c.stdout.Close() + c.err = c.cmd.Wait() + var exitErr *exec.ExitError + if errors.As(c.err, &exitErr) { + c.err = nil + } + }) + return c.err } func (c *pipeConn) LocalAddr() net.Addr { return pipeAddr{} } @@ -63,20 +67,31 @@ func (pipeAddr) Network() string { return "tailcat" } func (pipeAddr) String() string { return "tailcat" } func dialSSHClient(ctx context.Context, dial dialer, remoteAddr, user string, hostKeyCallback ssh.HostKeyCallback, auth []ssh.AuthMethod) (*ssh.Client, error) { - conn, err := dial(ctx) + handshakeCtx, cancel := context.WithTimeout(ctx, sshHandshakeTimeout) + defer cancel() + conn, err := dial(handshakeCtx) if err != nil { return nil, err } + stopCancellation := context.AfterFunc(handshakeCtx, func() { conn.Close() }) sshConn, chans, reqs, err := ssh.NewClientConn(conn, remoteAddr, &ssh.ClientConfig{ User: user, HostKeyCallback: hostKeyCallback, Auth: auth, Timeout: sshHandshakeTimeout, }) + stoppedCancellation := stopCancellation() if err != nil { conn.Close() + if ctxErr := handshakeCtx.Err(); ctxErr != nil { + return nil, fmt.Errorf("SSH handshake: %w", ctxErr) + } return nil, fmt.Errorf("SSH handshake: %w", err) } + if !stoppedCancellation || handshakeCtx.Err() != nil { + conn.Close() + return nil, fmt.Errorf("SSH handshake: %w", handshakeCtx.Err()) + } return ssh.NewClient(sshConn, chans, reqs), nil } diff --git a/cmd/meowshell/sftp_test.go b/cmd/meowshell/sftp_test.go new file mode 100644 index 0000000..50701fe --- /dev/null +++ b/cmd/meowshell/sftp_test.go @@ -0,0 +1,28 @@ +package main + +import ( + "context" + "errors" + "net" + "testing" + "time" + + "golang.org/x/crypto/ssh" +) + +func TestDialSSHClientHonorsContextDuringHandshake(t *testing.T) { + client, server := net.Pipe() + defer server.Close() + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + started := time.Now() + dial := dialer(func(context.Context) (net.Conn, error) { return client, nil }) + _, err := dialSSHClient(ctx, dial, "unresponsive.example:22", "user", ssh.InsecureIgnoreHostKey(), nil) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("dialSSHClient error = %v, want context deadline exceeded", err) + } + if elapsed := time.Since(started); elapsed > time.Second { + t.Fatalf("handshake cancellation took %s, want under 1s", elapsed) + } +} diff --git a/cmd/meowshell/transport.go b/cmd/meowshell/transport.go index 84c9e12..42af4b7 100644 --- a/cmd/meowshell/transport.go +++ b/cmd/meowshell/transport.go @@ -21,7 +21,7 @@ type dialer func(ctx context.Context) (net.Conn, error) func tailcatDialer(tailcatBin string, argv []string) dialer { return func(ctx context.Context) (net.Conn, error) { - cmd := exec.Command(tailcatBin, argv...) + cmd := exec.CommandContext(ctx, tailcatBin, argv...) cmd.Stderr = os.Stderr stdin, err := cmd.StdinPipe() if err != nil { diff --git a/dotnet/Meowshell/MeowshellAgentConnection.cs b/dotnet/Meowshell/MeowshellAgentConnection.cs index 2bf6d65..5bcee93 100644 --- a/dotnet/Meowshell/MeowshellAgentConnection.cs +++ b/dotnet/Meowshell/MeowshellAgentConnection.cs @@ -467,11 +467,11 @@ private async Task RunReadLoopAsync() if (frame is null) break; if (frame.Value.Type == MeowshellAgentProtocol.FrameTypeData) { - HandleData(frame.Value.ChannelId, frame.Value.Payload); + await HandleDataAsync(frame.Value.ChannelId, frame.Value.Payload).ConfigureAwait(false); continue; } var msg = System.Text.Json.JsonSerializer.Deserialize(frame.Value.Payload, MeowshellAgentProtocol.JsonOptions)!; - HandleControl(frame.Value.ChannelId, msg); + await HandleControlAsync(frame.Value.ChannelId, msg).ConfigureAwait(false); } FaultEverything(new TailcatException("meowshell agent exited unexpectedly", 0, _diagnostics.Tail())); } @@ -481,16 +481,16 @@ private async Task RunReadLoopAsync() } } - private void HandleData(uint channelId, byte[] payload) + private async Task HandleDataAsync(uint channelId, byte[] payload) { if (payload.Length == 0) return; var stream = payload[0]; var data = payload.AsMemory(1); if (_channels.TryGetValue(channelId, out var sink)) - _ = sink.OnDataAsync(stream, data); + await sink.OnDataAsync(stream, data).ConfigureAwait(false); } - private void HandleControl(uint channelId, AgentMessage msg) + private async Task HandleControlAsync(uint channelId, AgentMessage msg) { switch (msg.Msg) { @@ -535,7 +535,7 @@ private void HandleControl(uint channelId, AgentMessage msg) } if (_channels.TryGetValue(channelId, out var sink)) - sink.OnControl(msg); + await sink.OnControlAsync(msg).ConfigureAwait(false); } private async Task HandlePromptAsync(AgentMessage msg) @@ -612,6 +612,10 @@ public async Task StopAsync() try { await _process.WaitForExitAsync(grace.Token).ConfigureAwait(false); } catch (OperationCanceledException) { MeowshellProcessControl.TryKill(_process); } } + // Release bounded channel pumps before waiting for the read loop. A + // consumer that stopped reading may have backpressured that loop; faulting + // its sink completes the pipe and lets shutdown make progress. + FaultEverything(new OperationCanceledException("meowshell agent connection stopped")); try { await _readLoop.ConfigureAwait(false); } catch { } } @@ -628,7 +632,7 @@ public async ValueTask DisposeAsync() internal interface IAgentChannelSink { Task OnDataAsync(byte stream, ReadOnlyMemory data); - void OnControl(AgentMessage msg); + Task OnControlAsync(AgentMessage msg); void OnFault(Exception ex); } @@ -638,7 +642,12 @@ internal sealed class AgentChannelDataPump : IAgentChannelSink private readonly IAgentChannelSink _inner; private readonly System.Threading.Channels.Channel _queue = - System.Threading.Channels.Channel.CreateUnbounded(new System.Threading.Channels.UnboundedChannelOptions { SingleReader = true }); + System.Threading.Channels.Channel.CreateBounded(new System.Threading.Channels.BoundedChannelOptions(32) + { + SingleReader = true, + SingleWriter = true, + FullMode = System.Threading.Channels.BoundedChannelFullMode.Wait, + }); private readonly Task _pumpTask; public AgentChannelDataPump(IAgentChannelSink inner) @@ -658,20 +667,19 @@ private async Task RunAsync() } else { - _inner.OnControl(item.Control!); + await _inner.OnControlAsync(item.Control!).ConfigureAwait(false); } } } public Task OnDataAsync(byte stream, ReadOnlyMemory data) { - _queue.Writer.TryWrite(new QueueItem(true, stream, data, null)); - return Task.CompletedTask; + return _queue.Writer.WriteAsync(new QueueItem(true, stream, data, null)).AsTask(); } - public void OnControl(AgentMessage msg) + public async Task OnControlAsync(AgentMessage msg) { - _queue.Writer.TryWrite(new QueueItem(false, 0, ReadOnlyMemory.Empty, msg)); + await _queue.Writer.WriteAsync(new QueueItem(false, 0, ReadOnlyMemory.Empty, msg)).ConfigureAwait(false); if (msg.Msg is "exit_status" or "error") _queue.Writer.TryComplete(); } @@ -686,7 +694,7 @@ public void OnFault(Exception ex) internal sealed class AgentIgnoreSink : IAgentChannelSink { public Task OnDataAsync(byte stream, ReadOnlyMemory data) => Task.CompletedTask; - public void OnControl(AgentMessage msg) { } + public Task OnControlAsync(AgentMessage msg) => Task.CompletedTask; public void OnFault(Exception ex) { } } @@ -694,10 +702,11 @@ internal sealed class AgentRequestResponseSink(TaskCompletionSource complet { public Task OnDataAsync(byte stream, ReadOnlyMemory data) => Task.CompletedTask; - public void OnControl(AgentMessage msg) + public Task OnControlAsync(AgentMessage msg) { if (msg.Msg == "exit_status") completion.TrySetResult(msg.ExitCode); else if (msg.Msg == "error") completion.TrySetException(new TailcatException("upload failed", 0, msg.Message ?? "", MeowshellErrorCodeExtensions.Parse(msg.Code))); + return Task.CompletedTask; } public void OnFault(Exception ex) => completion.TrySetException(ex); @@ -718,7 +727,7 @@ public async Task OnDataAsync(byte stream, ReadOnlyMemory data) if (result.IsCompleted) return; } - public void OnControl(AgentMessage msg) + public Task OnControlAsync(AgentMessage msg) { switch (msg.Msg) { @@ -732,6 +741,7 @@ public void OnControl(AgentMessage msg) _completed.TrySetException(ex); break; } + return Task.CompletedTask; } public void OnFault(Exception ex) @@ -783,7 +793,7 @@ Task IAgentChannelSink.OnDataAsync(byte stream, ReadOnlyMemory data) return pipe.Writer.WriteAsync(data).AsTask(); } - void IAgentChannelSink.OnControl(AgentMessage msg) + Task IAgentChannelSink.OnControlAsync(AgentMessage msg) { switch (msg.Msg) { @@ -799,6 +809,7 @@ void IAgentChannelSink.OnControl(AgentMessage msg) _exitCode.TrySetException(ex); break; } + return Task.CompletedTask; } void IAgentChannelSink.OnFault(Exception ex) diff --git a/dotnet/Meowshell/MeowshellAgentProtocol.cs b/dotnet/Meowshell/MeowshellAgentProtocol.cs index 5c1e473..3e84108 100644 --- a/dotnet/Meowshell/MeowshellAgentProtocol.cs +++ b/dotnet/Meowshell/MeowshellAgentProtocol.cs @@ -25,8 +25,11 @@ internal static class MeowshellAgentProtocol public static async Task WriteFrameAsync(Stream stream, AgentFrame frame, CancellationToken cancellationToken) { - var buf = new byte[4 + FrameHeaderLength + frame.Payload.Length]; - WriteUInt32BigEndian(buf.AsSpan(0, 4), (uint)(FrameHeaderLength + frame.Payload.Length)); + var frameLength = checked(FrameHeaderLength + frame.Payload.Length); + if (frameLength > MaxFrameLength) + throw new TailcatException("meowshell agent protocol error", 0, $"frame length {frameLength} exceeds the {MaxFrameLength} limit"); + var buf = new byte[4 + frameLength]; + WriteUInt32BigEndian(buf.AsSpan(0, 4), (uint)frameLength); buf[4] = frame.Type; WriteUInt32BigEndian(buf.AsSpan(5, 4), frame.ChannelId); frame.Payload.CopyTo(buf.AsSpan(9)); From 9408d3edacaab4d18e1cc9b61d1e6f42a5babe9f Mon Sep 17 00:00:00 2001 From: Stefan van der Merwe <44154511+Sniperlyf3@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:45:10 +0200 Subject: [PATCH 6/8] Stabilize relay-backed CI tests --- .github/workflows/ci.yml | 24 ++++++++++++++++++- .../MeowshellAgentConnectionE2ETests.cs | 1 + .../MeowshellListenersE2ETests.cs | 1 + .../MeowshellServerE2ETests.cs | 1 + dotnet/Meowshell.Tests/RelayE2ECollection.cs | 13 ++++++++++ .../Meowshell.Tests/TailcatClientE2ETests.cs | 1 + 6 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 dotnet/Meowshell.Tests/RelayE2ECollection.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6858618..b091f27 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,6 +18,14 @@ on: permissions: contents: read +# A branch push with an open PR triggers both push and pull_request. Those runs +# used to hammer the same public relay simultaneously with duplicate E2E suites, +# and both could fail even though either run passed alone. Keep only the newest +# run for a branch; tags retain their own ref_name and are not grouped together. +concurrency: + group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }} + cancel-in-progress: true + jobs: build: runs-on: ubuntu-latest @@ -178,6 +186,9 @@ jobs: needs: build strategy: fail-fast: false + # Both architectures use the same public relay service. Running them one + # at a time avoids manufacturing a relay-load race in the test harness. + max-parallel: 1 matrix: include: - runner: ubuntu-latest @@ -260,7 +271,18 @@ jobs: env: DOTNET_E2E_TAILCAT_BIN: ${{ github.workspace }}/dist/tailcat_linux_amd64 DOTNET_E2E_MEOWSHELL_BIN: ${{ github.workspace }}/dist/meowshell_linux_amd64 - run: dotnet test dotnet/Meowshell.sln -c Release --nologo + run: >- + dotnet test dotnet/Meowshell.sln -c Release --nologo + --logger "trx;LogFileName=meowshell-tests.trx" + --results-directory test-results + + - name: Upload .NET test diagnostics + if: failure() + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4 + with: + name: dotnet-test-results + path: test-results/ + if-no-files-found: warn - name: Install the Android workload # Needed to pack Meowshell's android-targeted build below. diff --git a/dotnet/Meowshell.Tests/MeowshellAgentConnectionE2ETests.cs b/dotnet/Meowshell.Tests/MeowshellAgentConnectionE2ETests.cs index ebf7e7d..19ac1cb 100644 --- a/dotnet/Meowshell.Tests/MeowshellAgentConnectionE2ETests.cs +++ b/dotnet/Meowshell.Tests/MeowshellAgentConnectionE2ETests.cs @@ -6,6 +6,7 @@ namespace Meowshell.Tests; +[Collection(RelayE2ECollection.Name)] public sealed class MeowshellAgentConnectionE2ETests : IDisposable { private static readonly Regex AddressPattern = new(@"\btc[A-Za-z0-9_-]{10,}", RegexOptions.Compiled); diff --git a/dotnet/Meowshell.Tests/MeowshellListenersE2ETests.cs b/dotnet/Meowshell.Tests/MeowshellListenersE2ETests.cs index 0e6f2f1..4cc75a4 100644 --- a/dotnet/Meowshell.Tests/MeowshellListenersE2ETests.cs +++ b/dotnet/Meowshell.Tests/MeowshellListenersE2ETests.cs @@ -5,6 +5,7 @@ namespace Meowshell.Tests; +[Collection(RelayE2ECollection.Name)] public sealed class MeowshellListenersE2ETests : IDisposable { private const string TailcatEnvVar = "DOTNET_E2E_TAILCAT_BIN"; diff --git a/dotnet/Meowshell.Tests/MeowshellServerE2ETests.cs b/dotnet/Meowshell.Tests/MeowshellServerE2ETests.cs index 0c0e35e..187b9b7 100644 --- a/dotnet/Meowshell.Tests/MeowshellServerE2ETests.cs +++ b/dotnet/Meowshell.Tests/MeowshellServerE2ETests.cs @@ -4,6 +4,7 @@ namespace Meowshell.Tests; +[Collection(RelayE2ECollection.Name)] public sealed class MeowshellServerE2ETests : IDisposable { private static readonly Regex AddressPattern = new(@"\btc[A-Za-z0-9_-]{10,}", RegexOptions.Compiled); diff --git a/dotnet/Meowshell.Tests/RelayE2ECollection.cs b/dotnet/Meowshell.Tests/RelayE2ECollection.cs new file mode 100644 index 0000000..ffbd7e0 --- /dev/null +++ b/dotnet/Meowshell.Tests/RelayE2ECollection.cs @@ -0,0 +1,13 @@ +using Xunit; + +namespace Meowshell.Tests; + +// These classes all create real clients and ephemeral servers on the same +// public relay. Keeping them in one collection prevents xUnit from producing +// an artificial registration burst while leaving unrelated unit tests free to +// run in parallel. +[CollectionDefinition(Name)] +public sealed class RelayE2ECollection +{ + public const string Name = "Public relay E2E"; +} diff --git a/dotnet/Meowshell.Tests/TailcatClientE2ETests.cs b/dotnet/Meowshell.Tests/TailcatClientE2ETests.cs index 5239c61..2a0df32 100644 --- a/dotnet/Meowshell.Tests/TailcatClientE2ETests.cs +++ b/dotnet/Meowshell.Tests/TailcatClientE2ETests.cs @@ -5,6 +5,7 @@ namespace Meowshell.Tests; +[Collection(RelayE2ECollection.Name)] public sealed class TailcatClientE2ETests : IDisposable { private static readonly Regex AddressPattern = new(@"\btc[A-Za-z0-9_-]{10,}", RegexOptions.Compiled); From 82aaa1ed98f06f781f820cd441ad1d2e25333867 Mon Sep 17 00:00:00 2001 From: Stefan van der Merwe <44154511+Sniperlyf3@users.noreply.github.com> Date: Fri, 11 Sep 2026 14:01:25 +0200 Subject: [PATCH 7/8] Allow push and pull request workflows concurrently --- .github/workflows/ci.yml | 8 -------- 1 file changed, 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b091f27..127594a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,14 +18,6 @@ on: permissions: contents: read -# A branch push with an open PR triggers both push and pull_request. Those runs -# used to hammer the same public relay simultaneously with duplicate E2E suites, -# and both could fail even though either run passed alone. Keep only the newest -# run for a branch; tags retain their own ref_name and are not grouped together. -concurrency: - group: ${{ github.workflow }}-${{ github.head_ref || github.ref_name }} - cancel-in-progress: true - jobs: build: runs-on: ubuntu-latest From ec719a7b667cb9c818ff19165ab40188e5fc319b Mon Sep 17 00:00:00 2001 From: Stefan van der Merwe <44154511+Sniperlyf3@users.noreply.github.com> Date: Fri, 11 Sep 2026 14:34:33 +0200 Subject: [PATCH 8/8] Keep tailcat alive after SSH handshake --- build.sh | 4 +++ cmd/meowshell/transport.go | 9 ++++++- cmd/meowshell/transport_test.go | 44 +++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) diff --git a/build.sh b/build.sh index faeec36..87cac2b 100755 --- a/build.sh +++ b/build.sh @@ -30,6 +30,10 @@ git -C "$SRC_DIR" checkout --detach FETCH_HEAD # CI always starts from a fresh clone) would otherwise still carry the # patch applied below from the previous run, and re-applying it would fail. git -C "$SRC_DIR" reset --hard FETCH_HEAD +# The Android netmon patch adds a source file, so reset alone is insufficient: +# Git deliberately leaves that untracked file behind. SRC_DIR is a disposable +# build checkout; clean it as well so repeated builds start from the same tree. +git -C "$SRC_DIR" clean -fd # netmon.NewStatic() (used by pickregion.go's PickBestRegion, itself called # by ConnInfo.Expand whenever a key's RegionID is -1, the default for diff --git a/cmd/meowshell/transport.go b/cmd/meowshell/transport.go index 42af4b7..26ab255 100644 --- a/cmd/meowshell/transport.go +++ b/cmd/meowshell/transport.go @@ -21,7 +21,14 @@ type dialer func(ctx context.Context) (net.Conn, error) func tailcatDialer(tailcatBin string, argv []string) dialer { return func(ctx context.Context) (net.Conn, error) { - cmd := exec.CommandContext(ctx, tailcatBin, argv...) + if err := ctx.Err(); err != nil { + return nil, err + } + // The context only bounds dialing; it does not own the connection after + // this function returns. In particular, dialSSHClient cancels its + // handshake context after a successful handshake. CommandContext would + // then kill the tailcat process backing the live SSH connection. + cmd := exec.Command(tailcatBin, argv...) cmd.Stderr = os.Stderr stdin, err := cmd.StdinPipe() if err != nil { diff --git a/cmd/meowshell/transport_test.go b/cmd/meowshell/transport_test.go index 8431d55..e6c3925 100644 --- a/cmd/meowshell/transport_test.go +++ b/cmd/meowshell/transport_test.go @@ -9,11 +9,55 @@ import ( "net" "net/http" "net/url" + "runtime" "strings" "testing" "time" ) +func TestTailcatDialerContextDoesNotOwnReturnedConnection(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("cat subprocess fixture is Unix-only") + } + + ctx, cancel := context.WithCancel(context.Background()) + conn, err := tailcatDialer("cat", nil)(ctx) + if err != nil { + t.Fatalf("tailcatDialer: %v", err) + } + defer conn.Close() + + // A dial context governs creation of a connection, not the lifetime of a + // successfully returned connection. dialSSHClient cancels the context it + // passes here as soon as the SSH handshake completes. + cancel() + const message = "still connected\n" + if _, err := io.WriteString(conn, message); err != nil { + t.Fatalf("write after dial context cancellation: %v", err) + } + buf := make([]byte, len(message)) + if _, err := io.ReadFull(conn, buf); err != nil { + t.Fatalf("read after dial context cancellation: %v", err) + } + if got := string(buf); got != message { + t.Fatalf("echo = %q, want %q", got, message) + } +} + +func TestTailcatDialerRejectsCanceledContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + conn, err := tailcatDialer("command-must-not-be-started", nil)(ctx) + if conn != nil { + conn.Close() + t.Fatal("tailcatDialer returned a connection for a canceled context") + } + if !errors.Is(err, context.Canceled) { + t.Fatalf("tailcatDialer error = %v, want context canceled", err) + } +} + func mustParseURL(t *testing.T, raw string) *url.URL { t.Helper() u, err := url.Parse(raw)