From c8ffd4c5140d5ec31b6304c5ccec5c4501e2cb85 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Sat, 24 Oct 2026 21:32:54 -0700 Subject: [PATCH 1/5] Remediate dependency security vulnerabilities and add dual-mode tests - Go toolchain & build hygiene: - Update go/go.mod to go 1.27.3 to match .goversion and export GOTOOLCHAIN in go/build.sh. - Exclude stale xterm.js from sshterm-server's embedded docroot. - Centralized third-party dependency workarounds (go/internal/depfix): - Add feature-flagged workarounds and depfix.WithDisabled helper so each vulnerability and its remediation can be verified in dual-mode tests and cleanly removed once fixed upstream. - github.com/pkg/sftp (EnableSFTPPacketGuard, EnableSFTPPathAndRemoveGuard, EnableSFTPResourceGuard): - Interpose sftpSafeConn via sftp.NewClientPipe to bounds-check incoming SFTP packets (SSH_FXP_VERSION, SSH_FXP_STATUS, SSH_FXP_HANDLE, SSH_FXP_DATA, SSH_FXP_NAME, SSH_FXP_ATTRS) before pkg/sftp unmarshals them with unchecked slice bounds. - Reject 0-byte SSH_FXP_DATA responses that trigger an infinite read loop in File.readChunkAt, cap cumulative SSH_FXP_READDIR entries, and reject READDIR filenames containing '/' or resolving to '.' / '..'. - Provide Lstat-based depfix.RemoveAll so recursive removal does not follow directory symlinks, and wrap JS callback boundaries in jsutil.TryCatch. - golang.org/x/crypto/ssh & ssh/agent (EnableSSHAgentGuard, EnableRSASHA2, EnableSSHSessionExitGuard, EnableCertSignatureGuard): - Bound concurrent forwarded agent channels, cap incoming agent request frames to 256 KiB before agent.ServeAgent pre-allocates up to 16 MiB, and reject non-list/sign agent message types before ServeAgent parses attacker-supplied keys. - Implement ssh.MultiAlgorithmSigner on dynSigner and agent.ExtendedAgent on keyRing and forwardedAgent so RSA keys and certificates use rsa-sha2-256 / rsa-sha2-512 instead of falling back to SHA-1 (ssh-rsa). - Wrap SSH session channels so malformed exit-status or exit-signal requests close the channel and drain remaining requests instead of deadlocking (*ssh.Session).Wait and mux.loop. - Reject deprecated SHA-1 (ssh-rsa) and DSA (ssh-dss) CA signatures in certificate verification and support no-touch-required (Flags=0) sk-* CA signatures. - golang.org/x/term (EnableTermEscapeGuard): - Terminate runaway (>64-byte) unterminated escape sequences in WrapTermReadWriter before Terminal.inBuf ([256]byte) fills up and spins in a synchronous 0-byte Read loop. - Document the removal procedure and residual risks in go/internal/depfix/README.md. - Frontend @xterm/xterm workarounds & terminal hardening (docroot/ssh.mjs, go/internal/terminal): - Add window.sshApp.workarounds (csiRepGuard, csiLoopGuard, oscQueryAndLinkGuard) and applyXtermWorkarounds to clamp CSI Ps b (preventing RangeError in repeatPrecedingCharacter from wedging WriteBuffer) and CSI I/Z/L/M/S/T loop counts, and to rate-limit chained OSC 4 color queries and OSC 8 hyperlink markers. - Sanitize and cap OSC 0 / OSC 2 window titles and reuse a single toast element in TabManager._showMessage. - Reset terminal session modes and drain pending onData responses between SSH/SFTP sessions via Terminal.ResetSessionState. - Local fixes: - Fix duplicate trailing backslash append to bufRaw in go/internal/shellwords.Parse that could cause an out-of-bounds slice panic during tab completion. - Set HandshakeTimeout on the WebSocket upgrader and per-write deadlines on upstream TCP connections in sshterm-server. - Update CHANGELOG.md. --- CHANGELOG.md | 21 + docroot/ssh.mjs | 128 +++++- go/build.sh | 4 +- go/go.mod | 2 +- go/internal/app/agent.go | 43 +- go/internal/app/autocomplete_test.go | 13 + go/internal/app/cert_util.go | 6 +- go/internal/app/keys.go | 26 ++ go/internal/app/sftp.go | 54 ++- go/internal/app/ssh.go | 10 +- go/internal/depfix/README.md | 43 ++ go/internal/depfix/depfix.go | 98 ++++ go/internal/depfix/sftp.go | 522 ++++++++++++++++++++++ go/internal/depfix/sftp_test.go | 399 +++++++++++++++++ go/internal/depfix/ssh.go | 357 +++++++++++++++ go/internal/depfix/ssh_test.go | 429 ++++++++++++++++++ go/internal/depfix/term.go | 120 +++++ go/internal/depfix/term_test.go | 86 ++++ go/internal/jsutil/streams.go | 11 +- go/internal/shellwords/shellwords.go | 1 - go/internal/shellwords/shellwords_test.go | 1 + go/internal/terminal/terminal.go | 23 +- go/internal/tests/remediations_test.go | 312 +++++++++++++ go/internal/testserver/main_test.go | 9 +- go/sshterm-server/internal/server.go | 16 +- go/sshterm-server/internal/server_test.go | 92 +++- 26 files changed, 2784 insertions(+), 42 deletions(-) create mode 100644 go/internal/depfix/README.md create mode 100644 go/internal/depfix/depfix.go create mode 100644 go/internal/depfix/sftp.go create mode 100644 go/internal/depfix/sftp_test.go create mode 100644 go/internal/depfix/ssh.go create mode 100644 go/internal/depfix/ssh_test.go create mode 100644 go/internal/depfix/term.go create mode 100644 go/internal/depfix/term_test.go create mode 100644 go/internal/tests/remediations_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 939478d..dde7bfe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ # SSH Term Release Notes +## Next + +### :lock: Security fixes + +* Align `go/go.mod` (`go 1.27.3`) and `go/build.sh` (`GOTOOLCHAIN`) with `.goversion` so local builds do not fall back to Go 1.26.0, and remove stale `docroot/xterm.js` from the embedded server docroot. +* Harden SFTP client (`github.com/pkg/sftp`) against malicious or malformed server responses: + * Validate incoming SFTP packet framing and field bounds before `pkg/sftp` unmarshals them, and add panic recovery to SFTP background goroutines (`sftp get`, drag-and-drop uploads, `Getwd`, and tab completion). + * Reject `SSH_FXP_READDIR` filenames that contain `/` or resolve to `.` or `..` after `path.Base`, and use `Lstat` in recursive `rm -R` (`RemoveAll`) so symbolic links to directories are not followed. + * Reject 0-byte `SSH_FXP_DATA` packets (preventing an infinite read loop) and bound the number of entries accumulated in `ReadDirContext`. +* Harden SSH client and forwarded agent (`golang.org/x/crypto/ssh`): + * Bound concurrent `"auth-agent@openssh.com"` channels, cap incoming agent request frames to 256 KiB before allocation, and reject disallowed request types (such as `SSH2_AGENTC_ADD_IDENTITY`) before `agent.ServeAgent` parses attacker-supplied keys. + * Implement `ssh.MultiAlgorithmSigner` on `dynSigner` and `agent.ExtendedAgent` (`SignWithFlags`) on `keyRing` and `forwardedAgent` so RSA keys and user certificates use `rsa-sha2-256` / `rsa-sha2-512` instead of falling back to deprecated SHA-1 (`ssh-rsa`). + * Close the session channel and drain remaining channel requests when a remote server sends a malformed `"exit-status"` or `"exit-signal"` request, preventing an SSH connection multiplexer deadlock. + * Reject deprecated SHA-1 (`ssh-rsa`) and DSA (`ssh-dss`) CA signatures in `checkCertificate` and support `no-touch-required` security-key (`sk-*`) CA signatures. +* Prevent `golang.org/x/term` `Terminal.ReadLine` from entering an infinite synchronous 0-byte `Read` loop when receiving an unterminated escape sequence longer than 256 bytes. +* Fix a trailing-backslash parsing bug in `shellwords.Parse` that could cause a slice-bounds panic during tab auto-completion. +* Set upstream TCP write deadlines and WebSocket `HandshakeTimeout` in `sshterm-server`. +* Harden `@xterm/xterm` terminal handling in `docroot/ssh.mjs`: + * Clamp `CSI Ps b` (`REP`) and `CSI I`, `Z`, `L`, `M`, `S`, `T` parameters to terminal dimensions to prevent `RangeError` crashes (which wedged `WriteBuffer` and deadlocked WASM writes) and $2^{31}-1$ synchronous main-thread loops. + * Cap chained `OSC 4` color queries and `OSC 8` hyperlink markers, sanitize and truncate `OSC 0`/`OSC 2` window titles while reusing a single toast element in `_showMessage`, and reset terminal modes and pending input buffers when an SSH or SFTP session exits. + ## v0.11.5 ### :star: Feature improvements diff --git a/docroot/ssh.mjs b/docroot/ssh.mjs index 9c6fdd1..f4d99be 100644 --- a/docroot/ssh.mjs +++ b/docroot/ssh.mjs @@ -30,6 +30,100 @@ function isTest() { return window.location.pathname.indexOf('tests.html') !== -1; } +export const workarounds = { + // Protects @xterm/xterm (5.5.0) InputHandler.repeatPrecedingCharacter + // (CSI Ps b) from throwing an uncaught RangeError on large Ps values and + // permanently wedging WriteBuffer. + csiRepGuard: true, + + // Protects @xterm/xterm (5.5.0) InputHandler from 2^31-1 synchronous + // while(param--) loops in CSI I, Z, L, M, S, and T. + csiLoopGuard: true, + + // Protects @xterm/xterm (5.5.0) from amplified OSC 4 color query floods + // and unbounded OSC 8 hyperlink Marker/listener retention. + oscQueryAndLinkGuard: true, +}; + +export function sanitizeTitle(title) { + const clean = String(title).replace(/[\x00-\x1f\x7f-\x9f]/g, ' ').trim(); + return clean.length > 128 ? clean.slice(0, 128) + '…' : clean; +} + +export function applyXtermWorkarounds(term, flags = workarounds) { + const disposables = []; + const registerClampCsi = (final, getMax) => { + if (term._core && typeof term._core.registerCsiHandler === 'function') { + disposables.push( + term._core.registerCsiHandler({ final }, params => { + const active = final === 'b' ? flags.csiRepGuard : flags.csiLoopGuard; + if (!active) return false; + const maxVal = Math.max(1, getMax()); + if (params.length > 0 && params.params[0] > maxVal) { + params.params[0] = maxVal; + } + return false; + }) + ); + } else { + disposables.push( + term.parser.registerCsiHandler({ final }, params => { + const active = final === 'b' ? flags.csiRepGuard : flags.csiLoopGuard; + if (!active) return false; + const maxVal = Math.max(1, getMax()); + const p0 = typeof params[0] === 'number' ? params[0] : 0; + return p0 > maxVal; + }) + ); + } + }; + + registerClampCsi('b', () => term.cols); + registerClampCsi('I', () => term.cols); + registerClampCsi('Z', () => term.cols); + registerClampCsi('L', () => term.rows); + registerClampCsi('M', () => term.rows); + registerClampCsi('S', () => term.rows); + registerClampCsi('T', () => term.rows); + + disposables.push( + term.parser.registerOscHandler(4, data => { + if (!flags.oscQueryAndLinkGuard) return false; + let queries = 0; + for (let i = 0; i < data.length; i++) { + if (data.charCodeAt(i) === 63 /* '?' */) { + queries++; + if (queries > 16) return true; + } + } + return false; + }) + ); + + let lastLinkLine = -1; + let linksOnLine = 0; + disposables.push( + term.parser.registerOscHandler(8, data => { + if (!flags.oscQueryAndLinkGuard) return false; + if (data === ';') return false; + const buf = term.buffer?.active; + const line = buf ? buf.baseY + buf.cursorY : 0; + if (line !== lastLinkLine) { + lastLinkLine = line; + linksOnLine = 0; + } + linksOnLine++; + const totalMarkers = term._core?.markers ? term._core.markers.length : 0; + if (linksOnLine > 32 || totalMarkers > 256 || data.length > 2048) { + return true; + } + return false; + }) + ); + + return disposables; +} + class TerminalManager { constructor(elem, setTitle, onBell) { this.term = new Terminal({ @@ -42,7 +136,10 @@ class TerminalManager { this.term.open(elem); this.fitAddon.fit(); + const workaroundDisposables = applyXtermWorkarounds(this.term, workarounds); + this.disposables = [ + ...workaroundDisposables, this.term.onTitleChange(setTitle), this.term.onBell(onBell), this.term.onSelectionChange(() => { @@ -177,7 +274,8 @@ export class TabManager { this.screens[b.id] = { e, b }; this.screens[b.id].title = 'sshterm'; - const setTitle = title => { + const setTitle = rawTitle => { + const title = sanitizeTitle(rawTitle); this.screens[b.id].title = title; this.screens[b.id].b.title = title; document.querySelector('head title').textContent = title; @@ -231,15 +329,33 @@ export class TabManager { } _showMessage(element, text) { - const msg = document.createElement('div'); - msg.style = 'position: absolute; bottom: 0; right: 0; padding: 0.5rem; background-color: white; color: black; font-family: monospace; border: solid 1px black;'; - msg.textContent = text; - element.appendChild(msg); - setTimeout(() => element.removeChild(msg), 3000); + const safeText = sanitizeTitle(text); + let msg = element._toastMsg; + if (!msg || msg.parentNode !== element) { + msg = document.createElement('div'); + msg.className = 'sshterm-toast'; + msg.style = 'position: absolute; bottom: 0; right: 0; max-width: 80%; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; pointer-events: none; padding: 0.5rem; background-color: white; color: black; font-family: monospace; border: solid 1px black; z-index: 10;'; + element.appendChild(msg); + element._toastMsg = msg; + } + msg.textContent = safeText; + if (element._toastTimer) { + clearTimeout(element._toastTimer); + } + element._toastTimer = setTimeout(() => { + if (msg.parentNode === element) { + element.removeChild(msg); + } + element._toastMsg = null; + element._toastTimer = null; + }, 3000); } } window.sshApp = {}; +window.sshApp.workarounds = workarounds; +window.sshApp.applyXtermWorkarounds = applyXtermWorkarounds; +window.sshApp.Terminal = Terminal; window.sshApp.exited = null; window.sshApp.ready = new Promise(resolve => { window.sshApp.sshIsReady = () => { diff --git a/go/build.sh b/go/build.sh index f9738e8..259baa8 100755 --- a/go/build.sh +++ b/go/build.sh @@ -2,6 +2,8 @@ cd $(dirname $0) +export GOTOOLCHAIN="go$(cat ../.goversion)" + GOOS=js GOARCH=wasm go build -ldflags="-extldflags=-s -w" -o ../docroot/ssh.wasm . GOOS=js GOARCH=wasm go test -c -o ../docroot/tests.wasm ./internal/tests cp -f $(go env GOROOT)/lib/wasm/wasm_exec.js ../docroot/ @@ -9,7 +11,7 @@ cp -f $(go env GOROOT)/lib/wasm/wasm_exec.js ../docroot/ rm -rf sshterm-server/internal/docroot mkdir -p sshterm-server/internal/docroot cp -r ../docroot/* sshterm-server/internal/docroot/ -rm -f sshterm-server/internal/docroot/tests.* sshterm-server/internal/docroot/config.json* +rm -f sshterm-server/internal/docroot/tests.* sshterm-server/internal/docroot/config.json* sshterm-server/internal/docroot/xterm.js export CGO_ENABLED=0 go build -trimpath -ldflags="-extldflags=-s -w" -o ../sshterm-server ./sshterm-server diff --git a/go/go.mod b/go/go.mod index f2cfc9f..10c0b3e 100644 --- a/go/go.mod +++ b/go/go.mod @@ -1,6 +1,6 @@ module github.com/c2FmZQ/sshterm -go 1.26.0 +go 1.27.3 require ( github.com/chromedp/cdproto v0.0.0-20250803210736-d308e07a266d diff --git a/go/internal/app/agent.go b/go/internal/app/agent.go index a795339..d15c983 100644 --- a/go/internal/app/agent.go +++ b/go/internal/app/agent.go @@ -37,6 +37,8 @@ import ( "github.com/urfave/cli/v2" "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" + + "github.com/c2FmZQ/sshterm/internal/depfix" ) func (a *App) agentCommand() *cli.App { @@ -212,7 +214,7 @@ type keyRing struct { pp []byte } -var _ agent.Agent = (*keyRing)(nil) +var _ agent.ExtendedAgent = (*keyRing)(nil) var errAgentLocked = errors.New("agent is locked") var errAgentNotLocked = errors.New("agent is not locked") @@ -272,6 +274,10 @@ func (r *keyRing) List() ([]*agent.Key, error) { } func (r *keyRing) Sign(key ssh.PublicKey, data []byte) (*ssh.Signature, error) { + return r.SignWithFlags(key, data, 0) +} + +func (r *keyRing) SignWithFlags(key ssh.PublicKey, data []byte, flags agent.SignatureFlags) (*ssh.Signature, error) { r.mu.Lock() if r.locked { r.mu.Unlock() @@ -289,7 +295,25 @@ func (r *keyRing) Sign(key ssh.PublicKey, data []byte) (*ssh.Signature, error) { // Don't hold the lock while signing. With WebAuthn keys, Sign waits for // the user to touch the security key. r.mu.Unlock() - return signer.Sign(rand.Reader, data) + if !depfix.EnableRSASHA2 || flags == 0 { + return signer.Sign(rand.Reader, data) + } + as, ok := signer.(ssh.AlgorithmSigner) + if !ok { + return nil, fmt.Errorf("agent: key does not support custom signature algorithms") + } + switch flags { + case agent.SignatureFlagRsaSha256: + return as.SignWithAlgorithm(rand.Reader, data, ssh.KeyAlgoRSASHA256) + case agent.SignatureFlagRsaSha512: + return as.SignWithAlgorithm(rand.Reader, data, ssh.KeyAlgoRSASHA512) + default: + return nil, fmt.Errorf("agent: unsupported signature flags: %d", flags) + } +} + +func (r *keyRing) Extension(extensionType string, contents []byte) ([]byte, error) { + return nil, agent.ErrExtensionUnsupported } func (r *keyRing) AddSigner(signer ssh.Signer, comment string) error { @@ -454,13 +478,17 @@ type forwardedAgent struct { notify func(format string, args ...any) } -var _ agent.Agent = (*forwardedAgent)(nil) +var _ agent.ExtendedAgent = (*forwardedAgent)(nil) func (f *forwardedAgent) List() ([]*agent.Key, error) { return f.agent.List() } func (f *forwardedAgent) Sign(key ssh.PublicKey, data []byte) (*ssh.Signature, error) { + return f.SignWithFlags(key, data, 0) +} + +func (f *forwardedAgent) SignWithFlags(key ssh.PublicKey, data []byte, flags agent.SignatureFlags) (*ssh.Signature, error) { name := ssh.FingerprintSHA256(key) if keys, err := f.agent.List(); err == nil { v := key.Marshal() @@ -472,9 +500,18 @@ func (f *forwardedAgent) Sign(key ssh.PublicKey, data []byte) (*ssh.Signature, e } } f.notify("[agent] %s requested a signature with key %q\n", f.host, name) + if depfix.EnableRSASHA2 { + if ea, ok := f.agent.(agent.ExtendedAgent); ok { + return ea.SignWithFlags(key, data, flags) + } + } return f.agent.Sign(key, data) } +func (f *forwardedAgent) Extension(extensionType string, contents []byte) ([]byte, error) { + return nil, agent.ErrExtensionUnsupported +} + func (f *forwardedAgent) Add(key agent.AddedKey) error { return errForwardedAgent } diff --git a/go/internal/app/autocomplete_test.go b/go/internal/app/autocomplete_test.go index 145550f..4d0ffa6 100644 --- a/go/internal/app/autocomplete_test.go +++ b/go/internal/app/autocomplete_test.go @@ -129,3 +129,16 @@ func TestMaybeQuote(t *testing.T) { } } } + +func TestAutoCompleteTrailingBackslash(t *testing.T) { + ac := &autoCompleter{ + cmds: []*cli.App{ + {Name: `al\pha`}, + }, + } + + nl, np, _, ok := ac.autoComplete(`al\`, 3, '\t') + if !ok || nl != `"al\\pha" ` || np != len(nl) { + t.Fatalf("autoComplete(\"al\\\\\", 3, '\\t') = %q, %d, %v; want %q, %d, true", nl, np, ok, `"al\\pha" `, len(`"al\\pha" `)) + } +} diff --git a/go/internal/app/cert_util.go b/go/internal/app/cert_util.go index 88d7fbc..0968f99 100644 --- a/go/internal/app/cert_util.go +++ b/go/internal/app/cert_util.go @@ -33,6 +33,7 @@ import ( "golang.org/x/crypto/ssh" + "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/termsafe" ) @@ -56,10 +57,7 @@ func checkCertificate(cert *ssh.Certificate, certType uint32) error { slices.Sort(opts) errs = append(errs, fmt.Errorf("certificate has critical options: %v", opts)) } - c2 := *cert - c2.Signature = nil - signBytes := c2.Marshal() - if err := cert.SignatureKey.Verify(signBytes[:len(signBytes)-4], cert.Signature); err != nil { + if err := depfix.VerifyCertificateSignature(cert); err != nil { errs = append(errs, fmt.Errorf("certificate signature is invalid")) } return errors.Join(errs...) diff --git a/go/internal/app/keys.go b/go/internal/app/keys.go index 6b05952..629a6ac 100644 --- a/go/internal/app/keys.go +++ b/go/internal/app/keys.go @@ -48,6 +48,7 @@ import ( "github.com/urfave/cli/v2" "golang.org/x/crypto/ssh" + "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/jsutil" "github.com/c2FmZQ/sshterm/internal/termsafe" "github.com/c2FmZQ/sshterm/internal/webauthnsk" @@ -821,9 +822,34 @@ type dynSigner struct { key *key } +var _ ssh.MultiAlgorithmSigner = (*dynSigner)(nil) + func (s *dynSigner) PublicKey() ssh.PublicKey { if cert := s.key.Certificate(); cert != nil { return cert } return s.Signer.PublicKey() } + +func (s *dynSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*ssh.Signature, error) { + if !depfix.EnableRSASHA2 { + return s.Signer.Sign(rand, data) + } + if as, ok := s.Signer.(ssh.AlgorithmSigner); ok { + return as.SignWithAlgorithm(rand, data, algorithm) + } + if algorithm == "" || algorithm == s.Signer.PublicKey().Type() { + return s.Signer.Sign(rand, data) + } + return nil, fmt.Errorf("ssh: unsupported signature algorithm %q", algorithm) +} + +func (s *dynSigner) Algorithms() []string { + if !depfix.EnableRSASHA2 { + return []string{s.Signer.PublicKey().Type()} + } + if mas, ok := s.Signer.(ssh.MultiAlgorithmSigner); ok { + return mas.Algorithms() + } + return []string{s.Signer.PublicKey().Type()} +} diff --git a/go/internal/app/sftp.go b/go/internal/app/sftp.go index 7d8ecfb..4593ea8 100644 --- a/go/internal/app/sftp.go +++ b/go/internal/app/sftp.go @@ -43,6 +43,7 @@ import ( "github.com/urfave/cli/v2" "golang.org/x/term" + "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/jsutil" "github.com/c2FmZQ/sshterm/internal/shellwords" "github.com/c2FmZQ/sshterm/internal/termsafe" @@ -94,19 +95,30 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e } go sshKeepAlive(ctx, c, cancel) - client, err := sftp.NewClient(c) + client, err := depfix.NewSFTPClient(c) if err != nil { return err } defer client.Close() a.term.Printf("\x1b]0;sftp %s\x07", target) - defer a.term.Printf("\x1b]0;sshterm\x07") + defer func() { + a.term.ResetSessionState() + a.term.Printf("\x1b]0;sshterm\x07") + }() raw := a.term.Raw() - t := term.NewTerminal(raw, "sftp> ") - cwd, err := client.Getwd() + t := term.NewTerminal(depfix.WrapTermReadWriter(raw), "sftp> ") + var cwd string + jsutil.TryCatch( + func() { + cwd, err = client.Getwd() + }, + func(p any) { + err = fmt.Errorf("panic: %v", p) + }, + ) if err != nil { return fmt.Errorf("getwd: %w", err) } @@ -300,7 +312,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e for _, dir := range args { var err error if ctx.Bool("R") { - err = client.RemoveAll(joinPath(cwd, dir)) + err = depfix.RemoveAll(client, joinPath(cwd, dir)) } else { err = client.Remove(joinPath(cwd, dir)) } @@ -674,7 +686,14 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e } t.AutoCompleteCallback = func(line string, pos int, key rune) (newLine string, newPos int, ok bool) { var options []string - newLine, newPos, options, ok = ac.autoComplete(line, pos, key) + jsutil.TryCatch( + func() { + newLine, newPos, options, ok = ac.autoComplete(line, pos, key) + }, + func(err any) { + ok = false + }, + ) if len(options) > 0 { var w int for _, o := range options { @@ -710,14 +729,21 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e event.Call("stopPropagation") files := jsutil.AcceptFileDrop(event) go func() { - a.term.Printf("\n") - for _, f := range files { - a.term.Printf("%s ", f.Name) - if err := a.sftpUploadFile(client, f, joinPath(cwd, path.Base(f.Name))); err != nil { - a.term.Errorf("drop: %s", termsafe.Text(err.Error())) - return - } - } + jsutil.TryCatch( + func() { + a.term.Printf("\n") + for _, f := range files { + a.term.Printf("%s ", f.Name) + if err := a.sftpUploadFile(client, f, joinPath(cwd, path.Base(f.Name))); err != nil { + a.term.Errorf("drop: %s", termsafe.Text(err.Error())) + return + } + } + }, + func(err any) { + a.term.Errorf("drop: %v", err) + }, + ) }() return nil }) diff --git a/go/internal/app/ssh.go b/go/internal/app/ssh.go index 7f8d252..33b164e 100644 --- a/go/internal/app/ssh.go +++ b/go/internal/app/ssh.go @@ -40,6 +40,7 @@ import ( "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" + "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/termsafe" "github.com/c2FmZQ/sshterm/internal/websocket" ) @@ -109,7 +110,10 @@ func (a *App) runSSH(ctx context.Context, target, keyName, command string, forwa go sshKeepAlive(ctx, client, cancel) t.Printf("\x1b]0;ssh %s\x07", target) - defer t.Printf("\x1b]0;sshterm\x07") + defer func() { + t.ResetSessionState() + t.Printf("\x1b]0;sshterm\x07") + }() session, err := client.NewSession() if err != nil { @@ -129,7 +133,7 @@ func (a *App) runSSH(ctx context.Context, target, keyName, command string, forwa t.ShowMessage(f, args...) }, } - if err := agent.ForwardToAgent(client, fwd); err != nil { + if err := depfix.ForwardToAgent(client, fwd); err != nil { return fmt.Errorf("agent.ForwardToAgent: %w", err) } if err := agent.RequestAgentForwarding(session); err != nil { @@ -339,7 +343,7 @@ func (a *App) sshClientFromConn(ctx context.Context, c net.Conn, username, hostn return nil, err } - return ssh.NewClient(conn, chans, reqs), nil + return ssh.NewClient(depfix.WrapSSHConn(conn), chans, reqs), nil } func (a *App) hostCertificateCallback(hostname string, ep *endpoint, cert *ssh.Certificate) error { diff --git a/go/internal/depfix/README.md b/go/internal/depfix/README.md new file mode 100644 index 0000000..5a1862b --- /dev/null +++ b/go/internal/depfix/README.md @@ -0,0 +1,43 @@ +# Dependency Security Remediations (`depfix` & `ssh.mjs`) + +This package and the `workarounds` object in [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) centralize security remediations for third-party Go and JavaScript dependencies. + +## How the Mechanism Works + +1. **Feature Flags**: + * In Go ([`depfix.go`](./depfix.go)), each remediation is controlled by an exported boolean flag (defaulting to `true`). + * In JavaScript ([`docroot/ssh.mjs`](../../../docroot/ssh.mjs)), each `@xterm/xterm` remediation is controlled by a property on `window.sshApp.workarounds` (defaulting to `true`). +2. **Dual-Mode Verification Tests**: + * Every remediation has a corresponding test with two subtests: + * `VulnerabilityWithoutRemediation`: Temporarily disables the flag via `depfix.WithDisabled(&Flag, ...)` (or `workarounds. = false`) and asserts that the underlying dependency still exhibits the vulnerability. + * `RemediationEnabled`: Runs with the flag enabled (`true`) and asserts that the remediation prevents the vulnerability. +3. **Removing a Remediation After an Upstream Fix**: + * When a dependency is upgraded in `go/go.mod` or `xterm/package.json` and fixes the issue upstream, the `VulnerabilityWithoutRemediation` subtest will fail with an explicit message indicating which flag/wrapper can now be removed. + * To remove the remediation: + 1. Replace the `depfix.*` wrapper call at the call site with the direct upstream call (or remove the handler registration in `applyXtermWorkarounds`). + 2. Delete the corresponding flag in `depfix.go` / `ssh.mjs` and the `VulnerabilityWithoutRemediation` subtest, keeping the `RemediationEnabled` test as a regression test against the upstream package. + +## Remediations Inventory + +| Flag / Workaround | Dependency | Vulnerability Remediated | Call Site(s) | +| :--- | :--- | :--- | :--- | +| `EnableSFTPPacketGuard` | `github.com/pkg/sftp@v1.13.11` | Out-of-bounds slice panics in `unmarshalStatus`, `RealPath`, `ReadLink`, `ReadDirContext`, `opendir`, `open`, `readChunkAt`, and `readAt` on truncated/malformed packets | [`go/internal/app/sftp.go`](../app/sftp.go) | +| `EnableSFTPPathAndRemoveGuard` | `github.com/pkg/sftp@v1.13.11` | `ReadDirContext` path-sanitization bypass (`"/.."` -> `".."` via `path.Base`) and `Client.RemoveAll` following directory symlinks via `Stat` | [`go/internal/app/sftp.go`](../app/sftp.go) | +| `EnableSFTPResourceGuard` | `github.com/pkg/sftp@v1.13.11` | Infinite `SSH_FXP_READ` loop on 0-byte `SSH_FXP_DATA` packets and unbounded `ReadDirContext` entry accumulation | [`go/internal/app/sftp.go`](../app/sftp.go) | +| `EnableSSHAgentGuard` | `golang.org/x/crypto@v0.58.0` (`ssh/agent`) | Unbounded concurrent `"auth-agent@openssh.com"` channels, 16 MiB unread pre-allocation per channel in `ServeAgent`, and CPU exhaustion in `parseRSAKey` on disallowed `AddIdentity` requests | [`go/internal/app/ssh.go`](../app/ssh.go) | +| `EnableRSASHA2` | `golang.org/x/crypto@v0.58.0` (`ssh`, `ssh/agent`) | Downgrade to deprecated SHA-1 (`ssh-rsa`) when `dynSigner` omits `ssh.MultiAlgorithmSigner` or `forwardedAgent`/`keyRing` omits `agent.ExtendedAgent` | [`go/internal/app/keys.go`](../app/keys.go), [`go/internal/app/agent.go`](../app/agent.go) | +| `EnableSSHSessionExitGuard` | `golang.org/x/crypto@v0.58.0` (`ssh`) | Multiplexer (`mux.loop()`) and `(*Session).Wait()` deadlock when a server sends a malformed `"exit-status"` or `"exit-signal"` request followed by >16 channel requests | [`go/internal/app/ssh.go`](../app/ssh.go) | +| `EnableCertSignatureGuard` | `golang.org/x/crypto@v0.58.0` (`ssh`) | Acceptance of deprecated SHA-1 (`ssh-rsa`) / DSA (`ssh-dss`) CA signatures and rejection of `no-touch-required` (`Flags=0`) `sk-*` CA signatures | [`go/internal/app/cert_util.go`](../app/cert_util.go) | +| `EnableTermEscapeGuard` | `golang.org/x/term@v0.47.0` | Synchronous 100% CPU 0-byte `Read` infinite loop when `Terminal.inBuf` (`[256]byte`) fills with an unterminated escape sequence | [`go/internal/terminal/terminal.go`](../terminal/terminal.go), [`go/internal/app/sftp.go`](../app/sftp.go) | +| `workarounds.csiRepGuard` | `@xterm/xterm@5.5.0` | Uncaught `RangeError: Invalid typed array length` in `CSI Ps b` (`repeatPrecedingCharacter`) permanently wedging `WriteBuffer` and deadlocking Go WASM `Write` | [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) | +| `workarounds.csiLoopGuard` | `@xterm/xterm@5.5.0` | Unbounded $2^{31}-1$ synchronous `while (param--)` loops in `CSI I`, `Z`, `L`, `M`, `S`, and `T` freezing the browser main thread | [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) | +| `workarounds.oscQueryAndLinkGuard` | `@xterm/xterm@5.5.0` | `OSC 4` chained color query amplification and unbounded `OSC 8` hyperlink `Marker`/listener retention | [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) | + +## Documented Residual Risks + +1. **Single-Threaded 32-bit WebAssembly Runtime (`GOOS=js GOARCH=wasm`)**: + * All tabs (`AddScreen`) within the same browser page share a single Go WASM instance with a 32-bit address space (2–4 GiB maximum linear memory) and cooperative goroutine scheduling on a single OS/WebWorker thread. While `depfix` caps per-channel agent frames (`256 KiB`), SFTP packet frames (`256 KiB`), and `ReadDir` entry counts (`100,000`), an authenticated remote server that streams continuous high-bandwidth output across many tabs can still contend for CPU time and memory within that browser tab. +2. **OpenSSH Ed25519 Private Key Public-Half Consistency (`golang.org/x/crypto/ssh`)**: + * In `golang.org/x/crypto/ssh`, `parseOpenSSHPrivateKey` does not verify that the 32-byte public key embedded inside an unencrypted/decrypted `ed25519` private key matches `ed25519.NewKeyFromSeed(priv[:32])[32:]`. Because private keys are only imported explicitly by the local user (`keys import`), this is only relevant if a user imports an intentionally corrupted private key file. +3. **Terminal Emulator Escape Sequence Surface (`@xterm/xterm`)**: + * `applyXtermWorkarounds` intercepts the known unbounded CSI/OSC handlers in `@xterm/xterm@5.5.0`, and `Terminal.ResetSessionState()` resets terminal modes and drains pending `onData` query responses when an SSH or SFTP session exits. However, while an interactive SSH session is actively running, standard terminal auto-responses (such as primary/secondary Device Attributes `CSI c` and cursor position reports `CSI 6n`) remain active as required for terminal compatibility (`vim`, `tmux`, `less`, etc.). diff --git a/go/internal/depfix/depfix.go b/go/internal/depfix/depfix.go new file mode 100644 index 0000000..3e743d6 --- /dev/null +++ b/go/internal/depfix/depfix.go @@ -0,0 +1,98 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +// Package depfix centralizes security workarounds and remediations for +// third-party dependencies so they can be tested in both vulnerable and +// remediated states and cleanly removed when fixed upstream. +package depfix + +var ( + // EnableSFTPPacketGuard validates incoming SFTP packet frames before + // github.com/pkg/sftp (v1.13.11) unmarshals them with unchecked slice + // bounds in unmarshalStatus, RealPath, ReadLink, ReadDirContext, + // opendir, open, readChunkAt, and readAt. + // Remove when github.com/pkg/sftp replaces unmarshalUint32 and + // unmarshalString with safe bounds-checked variants across client.go + // and packet.go. + EnableSFTPPacketGuard = true + + // EnableSFTPPathAndRemoveGuard rejects SSH_FXP_READDIR filenames that + // contain '/' or resolve to '.' or '..' after path.Base, and uses Lstat + // instead of Stat in RemoveAll so directory symlinks are not followed. + // Remove when github.com/pkg/sftp validates ReadDirContext filenames + // and switches Client.RemoveAll to Lstat. + EnableSFTPPathAndRemoveGuard = true + + // EnableSFTPResourceGuard rejects 0-byte SSH_FXP_DATA packets (which + // cause an infinite read loop in File.readChunkAt) and caps cumulative + // SSH_FXP_READDIR entries per directory listing. + // Remove when github.com/pkg/sftp handles 0-byte SSH_FXP_DATA and + // bounds ReadDirContext. + EnableSFTPResourceGuard = true + + // EnableSSHAgentGuard bounds concurrent "auth-agent@openssh.com" + // channels, caps incoming agent request frames to 256 KiB before + // golang.org/x/crypto/ssh/agent (v0.58.0) ServeAgent pre-allocates up + // to 16 MiB per channel, and rejects non-list/sign agent request types + // before ServeAgent parses attacker-supplied private keys. + // Remove when golang.org/x/crypto/ssh/agent bounds channel concurrency + // and request pre-allocation. + EnableSSHAgentGuard = true + + // EnableRSASHA2 enables ssh.MultiAlgorithmSigner on dynSigner and + // agent.ExtendedAgent on keyRing and forwardedAgent so RSA keys and + // certificates use rsa-sha2-256 / rsa-sha2-512 instead of falling back + // to SHA-1 (ssh-rsa) in golang.org/x/crypto/ssh (v0.58.0). + EnableRSASHA2 = true + + // EnableSSHSessionExitGuard wraps "session" channel request streams so + // malformed "exit-status" or "exit-signal" requests close the session + // channel and drain remaining channel requests instead of deadlocking + // the SSH connection multiplexer in golang.org/x/crypto/ssh (v0.58.0). + // Remove when (*ssh.Session).wait drains reqs and unblocks Wait on + // malformed exit requests. + EnableSSHSessionExitGuard = true + + // EnableCertSignatureGuard rejects deprecated SHA-1 (ssh-rsa) and DSA + // (ssh-dss) CA signatures in checkCertificate and supports security-key + // (sk-*) CA signatures that omit the UserPresence flag. + EnableCertSignatureGuard = true + + // EnableTermEscapeGuard terminates runaway (>64-byte) unterminated + // escape sequences before golang.org/x/term (v0.47.0) fills its fixed + // 256-byte Terminal.inBuf and spins in a synchronous 0-byte Read loop. + // Remove when golang.org/x/term handles full inBuf escape sequences. + EnableTermEscapeGuard = true +) + +// WithDisabled temporarily sets *flag to false while running fn, then restores +// its previous value so tests can verify both the underlying vulnerability and +// its remediation. +func WithDisabled(flag *bool, fn func()) { + prev := *flag + *flag = false + defer func() { + *flag = prev + }() + fn() +} diff --git a/go/internal/depfix/sftp.go b/go/internal/depfix/sftp.go new file mode 100644 index 0000000..4b3b3fc --- /dev/null +++ b/go/internal/depfix/sftp.go @@ -0,0 +1,522 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package depfix + +import ( + "bytes" + "encoding/binary" + "errors" + "fmt" + "io" + "os" + "path" + "strings" + "sync" + + "github.com/pkg/sftp" + "golang.org/x/crypto/ssh" +) + +const ( + sshFxpInit = 1 + sshFxpVersion = 2 + sshFxpClose = 4 + sshFxpOpendir = 11 + sshFxpReaddir = 12 + sshFxpStatus = 101 + sshFxpHandle = 102 + sshFxpData = 103 + sshFxpName = 104 + sshFxpAttrs = 105 + sshFxpExtendedReply = 201 + + sshFileXferAttrSize = 0x00000001 + sshFileXferAttrUIDGID = 0x00000002 + sshFileXferAttrPermissions = 0x00000004 + sshFileXferAttrACmodTime = 0x00000008 + sshFileXferAttrExtended = 0x80000000 + + maxSFTPPacketSize = 256 * 1024 +) + +var ( + // MaxSFTPReadDirEntries is the maximum number of directory entries + // permitted in a single directory listing when EnableSFTPResourceGuard + // is enabled. + MaxSFTPReadDirEntries = 100000 + + ErrMalformedSFTPPacket = errors.New("sftp: malformed packet from server") + ErrZeroLengthSFTPData = errors.New("sftp: zero-length SSH_FXP_DATA packet") + ErrInvalidSFTPFilename = errors.New("sftp: invalid filename in SSH_FXP_NAME response") + ErrTooManySFTPEntries = errors.New("sftp: directory listing exceeds maximum entry limit") +) + +// NewSFTPClient creates a new *sftp.Client over conn with packet and resource +// guards applied when enabled. +func NewSFTPClient(conn *ssh.Client, opts ...sftp.ClientOption) (*sftp.Client, error) { + // Disable concurrent reads by default when packet/resource guards are + // enabled so sftp.File.Read does not spawn unrecovered background + // worker goroutines on large reads. + if EnableSFTPPacketGuard { + opts = append([]sftp.ClientOption{sftp.UseConcurrentReads(false)}, opts...) + } + if !EnableSFTPPacketGuard && !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { + return sftp.NewClient(conn, opts...) + } + s, err := conn.NewSession() + if err != nil { + return nil, err + } + pw, err := s.StdinPipe() + if err != nil { + s.Close() + return nil, err + } + pr, err := s.StdoutPipe() + if err != nil { + s.Close() + return nil, err + } + if err := s.RequestSubsystem("sftp"); err != nil { + s.Close() + return nil, err + } + sc := newSFTPSafeConn(pr, &sessionWriteCloser{WriteCloser: pw, sess: s}) + client, err := sftp.NewClientPipe(sc, sc, opts...) + if err != nil { + sc.Close() + return nil, err + } + return client, nil +} + +// NewSFTPClientPipe creates a new *sftp.Client over rd and wr with packet and +// resource guards applied when enabled. +func NewSFTPClientPipe(rd io.Reader, wr io.WriteCloser, opts ...sftp.ClientOption) (*sftp.Client, error) { + if EnableSFTPPacketGuard { + opts = append([]sftp.ClientOption{sftp.UseConcurrentReads(false)}, opts...) + } + if !EnableSFTPPacketGuard && !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { + return sftp.NewClientPipe(rd, wr, opts...) + } + sc := newSFTPSafeConn(rd, wr) + return sftp.NewClientPipe(sc, sc, opts...) +} + +type sessionWriteCloser struct { + io.WriteCloser + sess *ssh.Session +} + +func (s *sessionWriteCloser) Close() error { + err := s.WriteCloser.Close() + _ = s.sess.Close() + return err +} + +type sftpSafeConn struct { + rd io.Reader + wr io.WriteCloser + buf bytes.Buffer + err error + + mu sync.Mutex + txBuf bytes.Buffer + readdirSIDs map[uint32]bool + readdirEntries int +} + +func newSFTPSafeConn(rd io.Reader, wr io.WriteCloser) *sftpSafeConn { + return &sftpSafeConn{ + rd: rd, + wr: wr, + readdirSIDs: make(map[uint32]bool), + } +} + +func (c *sftpSafeConn) Write(p []byte) (int, error) { + c.mu.Lock() + c.txBuf.Write(p) + for c.txBuf.Len() >= 5 { + hdr := c.txBuf.Bytes() + pktLen := binary.BigEndian.Uint32(hdr[:4]) + if pktLen == 0 || pktLen > maxSFTPPacketSize { + c.txBuf.Reset() + break + } + totalLen := int(pktLen) + 4 + if c.txBuf.Len() < totalLen { + break + } + frame := c.txBuf.Next(totalLen) + pktType := frame[4] + if len(frame) >= 9 { + sid := binary.BigEndian.Uint32(frame[5:9]) + switch pktType { + case sshFxpOpendir: + c.readdirEntries = 0 + case sshFxpReaddir: + c.readdirSIDs[sid] = true + case sshFxpClose: + c.readdirEntries = 0 + } + } + } + c.mu.Unlock() + return c.wr.Write(p) +} + +func (c *sftpSafeConn) Close() error { + return c.wr.Close() +} + +func (c *sftpSafeConn) Read(p []byte) (int, error) { + for c.buf.Len() == 0 { + if c.err != nil { + return 0, c.err + } + if err := c.readAndValidatePacket(); err != nil { + c.err = err + _ = c.wr.Close() + return 0, err + } + } + return c.buf.Read(p) +} + +func (c *sftpSafeConn) readAndValidatePacket() error { + var hdr [5]byte + if _, err := io.ReadFull(c.rd, hdr[:]); err != nil { + return err + } + length := binary.BigEndian.Uint32(hdr[:4]) + if length < 1 || length > maxSFTPPacketSize { + return fmt.Errorf("%w: invalid packet length %d", ErrMalformedSFTPPacket, length) + } + pktType := hdr[4] + payload := make([]byte, length-1) + if _, err := io.ReadFull(c.rd, payload); err != nil { + if errors.Is(err, io.EOF) { + return io.ErrUnexpectedEOF + } + return err + } + + if err := c.validatePacket(pktType, payload); err != nil { + return err + } + + c.buf.Write(hdr[:]) + c.buf.Write(payload) + return nil +} + +func (c *sftpSafeConn) isReaddirSID(sid uint32, remove bool) bool { + c.mu.Lock() + defer c.mu.Unlock() + ok := c.readdirSIDs[sid] + if ok && remove { + delete(c.readdirSIDs, sid) + } + return ok +} + +func (c *sftpSafeConn) addReaddirEntries(n int) error { + c.mu.Lock() + defer c.mu.Unlock() + c.readdirEntries += n + if c.readdirEntries > MaxSFTPReadDirEntries { + return ErrTooManySFTPEntries + } + return nil +} + +func (c *sftpSafeConn) validatePacket(pktType byte, data []byte) error { + if pktType == sshFxpVersion { + if !EnableSFTPPacketGuard { + return nil + } + if len(data) < 4 { + return fmt.Errorf("%w: truncated SSH_FXP_VERSION", ErrMalformedSFTPPacket) + } + rest := data[4:] + for len(rest) > 0 { + var err error + if _, rest, err = readStringSafe(rest); err != nil { + return err + } + if _, rest, err = readStringSafe(rest); err != nil { + return err + } + } + return nil + } + + if len(data) < 4 { + if EnableSFTPPacketGuard { + return fmt.Errorf("%w: packet shorter than 4-byte request id", ErrMalformedSFTPPacket) + } + return nil + } + sid := binary.BigEndian.Uint32(data[:4]) + rest := data[4:] + + switch pktType { + case sshFxpStatus: + c.isReaddirSID(sid, true) + if !EnableSFTPPacketGuard { + return nil + } + // unmarshalStatus calls unmarshalUint32(data) for code without bounds checks. + if len(rest) < 4 { + return fmt.Errorf("%w: truncated SSH_FXP_STATUS", ErrMalformedSFTPPacket) + } + rest = rest[4:] + if len(rest) > 0 { + var err error + if _, rest, err = readStringSafe(rest); err != nil { + return err + } + if len(rest) > 0 { + if _, _, err = readStringSafe(rest); err != nil { + return err + } + } + } + return nil + + case sshFxpHandle: + if !EnableSFTPPacketGuard { + return nil + } + if _, _, err := readStringSafe(rest); err != nil { + return fmt.Errorf("%w: invalid SSH_FXP_HANDLE", ErrMalformedSFTPPacket) + } + return nil + + case sshFxpData: + if !EnableSFTPPacketGuard && !EnableSFTPResourceGuard { + return nil + } + d, _, err := readStringSafe(rest) + if err != nil { + if EnableSFTPPacketGuard { + return fmt.Errorf("%w: invalid SSH_FXP_DATA", ErrMalformedSFTPPacket) + } + return nil + } + if EnableSFTPResourceGuard && len(d) == 0 { + return ErrZeroLengthSFTPData + } + return nil + + case sshFxpName: + isReaddir := c.isReaddirSID(sid, true) + if !EnableSFTPPacketGuard && !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { + return nil + } + if len(rest) < 4 { + if EnableSFTPPacketGuard { + return fmt.Errorf("%w: truncated SSH_FXP_NAME count", ErrMalformedSFTPPacket) + } + return nil + } + count := binary.BigEndian.Uint32(rest[:4]) + rest = rest[4:] + if EnableSFTPPacketGuard && count == 0 { + return fmt.Errorf("%w: zero count in SSH_FXP_NAME", ErrMalformedSFTPPacket) + } + // Each entry requires at least 12 bytes (filename len + longname len + attr flags). + if count > uint32(len(rest)/12) { + if EnableSFTPPacketGuard { + return fmt.Errorf("%w: SSH_FXP_NAME count %d exceeds payload", ErrMalformedSFTPPacket, count) + } + return nil + } + if isReaddir && EnableSFTPResourceGuard { + if err := c.addReaddirEntries(int(count)); err != nil { + return err + } + } + for i := uint32(0); i < count; i++ { + filename, r, err := readStringSafe(rest) + if err != nil { + if EnableSFTPPacketGuard { + return err + } + return nil + } + if isReaddir && EnableSFTPPathAndRemoveGuard { + if err := validateReaddirFilename(filename); err != nil { + return err + } + } + _, r, err = readStringSafe(r) + if err != nil { + if EnableSFTPPacketGuard { + return err + } + return nil + } + r, err = skipAttrsSafe(r) + if err != nil { + if EnableSFTPPacketGuard { + return err + } + return nil + } + rest = r + } + return nil + + case sshFxpAttrs: + if !EnableSFTPPacketGuard { + return nil + } + if _, err := skipAttrsSafe(rest); err != nil { + return err + } + return nil + + case sshFxpExtendedReply: + return nil + + default: + return nil + } +} + +func validateReaddirFilename(filename string) error { + if filename == "." || filename == ".." { + return nil + } + if filename == "" || strings.ContainsAny(filename, "/\x00") { + return fmt.Errorf("%w: %q", ErrInvalidSFTPFilename, filename) + } + base := path.Base(filename) + if base == "." || base == ".." || base == "/" || base == "" { + return fmt.Errorf("%w: %q", ErrInvalidSFTPFilename, filename) + } + return nil +} + +func readStringSafe(b []byte) (string, []byte, error) { + if len(b) < 4 { + return "", nil, fmt.Errorf("%w: truncated string length", ErrMalformedSFTPPacket) + } + n := binary.BigEndian.Uint32(b[:4]) + b = b[4:] + if int64(n) > int64(len(b)) { + return "", nil, fmt.Errorf("%w: string length %d exceeds remaining %d bytes", ErrMalformedSFTPPacket, n, len(b)) + } + return string(b[:n]), b[n:], nil +} + +func skipAttrsSafe(b []byte) ([]byte, error) { + if len(b) < 4 { + return nil, fmt.Errorf("%w: truncated attrs flags", ErrMalformedSFTPPacket) + } + flags := binary.BigEndian.Uint32(b[:4]) + b = b[4:] + if flags&sshFileXferAttrSize != 0 { + if len(b) < 8 { + return nil, fmt.Errorf("%w: truncated attr size", ErrMalformedSFTPPacket) + } + b = b[8:] + } + if flags&sshFileXferAttrUIDGID != 0 { + if len(b) < 8 { + return nil, fmt.Errorf("%w: truncated attr uid/gid", ErrMalformedSFTPPacket) + } + b = b[8:] + } + if flags&sshFileXferAttrPermissions != 0 { + if len(b) < 4 { + return nil, fmt.Errorf("%w: truncated attr permissions", ErrMalformedSFTPPacket) + } + b = b[4:] + } + if flags&sshFileXferAttrACmodTime != 0 { + if len(b) < 8 { + return nil, fmt.Errorf("%w: truncated attr acmodtime", ErrMalformedSFTPPacket) + } + b = b[8:] + } + if flags&sshFileXferAttrExtended != 0 { + if len(b) < 4 { + return nil, fmt.Errorf("%w: truncated attr extended count", ErrMalformedSFTPPacket) + } + count := binary.BigEndian.Uint32(b[:4]) + b = b[4:] + if count > uint32(len(b)/8) { + return nil, fmt.Errorf("%w: attr extended count %d exceeds payload", ErrMalformedSFTPPacket, count) + } + for i := uint32(0); i < count; i++ { + var err error + if _, b, err = readStringSafe(b); err != nil { + return nil, err + } + if _, b, err = readStringSafe(b); err != nil { + return nil, err + } + } + } + return b, nil +} + +// RemoveAll recursively removes p on client. When EnableSFTPPathAndRemoveGuard +// is true, it uses Lstat instead of Stat so symbolic links to directories are +// removed as links rather than followed into the target directory. +func RemoveAll(client *sftp.Client, p string) error { + if !EnableSFTPPathAndRemoveGuard { + return client.RemoveAll(p) + } + info, err := client.Lstat(p) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil + } + return err + } + if !info.IsDir() { + return client.Remove(p) + } + entries, err := client.ReadDir(p) + if err != nil { + return err + } + for _, entry := range entries { + name := entry.Name() + if err := validateReaddirFilename(name); err != nil { + return err + } + if name == "." || name == ".." { + continue + } + if err := RemoveAll(client, path.Join(p, name)); err != nil { + return err + } + } + return client.RemoveDirectory(p) +} diff --git a/go/internal/depfix/sftp_test.go b/go/internal/depfix/sftp_test.go new file mode 100644 index 0000000..a53b92a --- /dev/null +++ b/go/internal/depfix/sftp_test.go @@ -0,0 +1,399 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package depfix + +import ( + "encoding/binary" + "errors" + "io" + "os" + "path/filepath" + "sync/atomic" + "testing" + + "github.com/pkg/sftp" +) + +func buildSFTPPacket(pktType byte, payload []byte) []byte { + length := uint32(1 + len(payload)) + out := make([]byte, 4+length) + binary.BigEndian.PutUint32(out[:4], length) + out[4] = pktType + copy(out[5:], payload) + return out +} + +func readClientPacket(r io.Reader) (byte, uint32, []byte, error) { + var hdr [5]byte + if _, err := io.ReadFull(r, hdr[:]); err != nil { + return 0, 0, nil, err + } + length := binary.BigEndian.Uint32(hdr[:4]) + pktType := hdr[4] + payload := make([]byte, length-1) + if _, err := io.ReadFull(r, payload); err != nil { + return 0, 0, nil, err + } + var sid uint32 + if len(payload) >= 4 { + sid = binary.BigEndian.Uint32(payload[:4]) + } + return pktType, sid, payload, nil +} + +// runMockSFTPSession starts a mock SFTP server over an io.Pipe that completes +// the SSH_FXP_INIT / SSH_FXP_VERSION handshake and then invokes handler for +// each request packet sent by the client. +func runMockSFTPSession(t *testing.T, handler func(pktType byte, sid uint32, wr io.Writer)) *sftp.Client { + t.Helper() + srvRd, cliWr := io.Pipe() + cliRd, srvWr := io.Pipe() + + go func() { + defer srvWr.Close() + defer srvRd.Close() + // Expect SSH_FXP_INIT + pktType, _, _, err := readClientPacket(srvRd) + if err != nil || pktType != sshFxpInit { + return + } + // Send SSH_FXP_VERSION (version 3) + ver := make([]byte, 4) + binary.BigEndian.PutUint32(ver, 3) + if _, err := srvWr.Write(buildSFTPPacket(sshFxpVersion, ver)); err != nil { + return + } + for { + pt, sid, _, err := readClientPacket(srvRd) + if err != nil { + return + } + handler(pt, sid, srvWr) + } + }() + + client, err := NewSFTPClientPipe(cliRd, cliWr) + if err != nil { + t.Fatalf("NewSFTPClientPipe: %v", err) + } + t.Cleanup(func() { + _ = client.Close() + }) + return client +} + +func TestSFTPPacketGuard(t *testing.T) { + cases := []struct { + name string + // respond writes a malformed response packet for sid. + respond func(sid uint32, wr io.Writer) + // invoke triggers the client operation that unmarshals the response. + invoke func(c *sftp.Client) error + }{ + { + name: "TruncatedStatus", + respond: func(sid uint32, wr io.Writer) { + // 4-byte sid only; missing 4-byte status code. + var b [4]byte + binary.BigEndian.PutUint32(b[:], sid) + _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) + }, + invoke: func(c *sftp.Client) error { + return c.Mkdir("/bad") + }, + }, + { + name: "TruncatedRealPathName", + respond: func(sid uint32, wr io.Writer) { + // count = 1, filename length = 100, but only 2 bytes follow. + var b [14]byte + binary.BigEndian.PutUint32(b[0:4], sid) + binary.BigEndian.PutUint32(b[4:8], 1) + binary.BigEndian.PutUint32(b[8:12], 100) + b[12] = 'a' + b[13] = 'b' + _, _ = wr.Write(buildSFTPPacket(sshFxpName, b[:])) + }, + invoke: func(c *sftp.Client) error { + _, err := c.Getwd() + return err + }, + }, + { + name: "TruncatedHandle", + respond: func(sid uint32, wr io.Writer) { + // handle length = 50, but 0 bytes follow. + var b [8]byte + binary.BigEndian.PutUint32(b[0:4], sid) + binary.BigEndian.PutUint32(b[4:8], 50) + _, _ = wr.Write(buildSFTPPacket(sshFxpHandle, b[:])) + }, + invoke: func(c *sftp.Client) error { + _, err := c.Open("/file") + return err + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name+"/VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableSFTPPacketGuard, func() { + WithDisabled(&EnableSFTPPathAndRemoveGuard, func() { + WithDisabled(&EnableSFTPResourceGuard, func() { + client := runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { + tc.respond(sid, wr) + }) + panicked := false + func() { + defer func() { + if r := recover(); r != nil { + panicked = true + } + }() + _ = tc.invoke(client) + }() + if !panicked { + t.Fatalf("expected github.com/pkg/sftp to panic on %s when EnableSFTPPacketGuard is disabled; if fixed upstream, remove EnableSFTPPacketGuard", tc.name) + } + }) + }) + }) + }) + + t.Run(tc.name+"/RemediationEnabled", func(t *testing.T) { + client := runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { + tc.respond(sid, wr) + }) + var err error + func() { + defer func() { + if r := recover(); r != nil { + t.Fatalf("unexpected panic with EnableSFTPPacketGuard enabled: %v", r) + } + }() + err = tc.invoke(client) + }() + if err == nil { + t.Fatalf("expected error on %s with EnableSFTPPacketGuard enabled, got nil", tc.name) + } + }) + } +} + +func TestSFTPPathAndRemoveGuard(t *testing.T) { + // 1. Verify ReadDirContext path traversal filename ("/..") handling. + makeReaddirServer := func(t *testing.T) *sftp.Client { + var sentEntry atomic.Bool + return runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { + switch pktType { + case sshFxpOpendir: + var b [9]byte + binary.BigEndian.PutUint32(b[0:4], sid) + binary.BigEndian.PutUint32(b[4:8], 1) + b[8] = 'h' + _, _ = wr.Write(buildSFTPPacket(sshFxpHandle, b[:])) + case sshFxpReaddir: + if !sentEntry.Swap(true) { + // Return 1 entry with filename "/.." + var payload []byte + var hdr [8]byte + binary.BigEndian.PutUint32(hdr[0:4], sid) + binary.BigEndian.PutUint32(hdr[4:8], 1) // count = 1 + payload = append(payload, hdr[:]...) + // filename = "/.." + payload = append(payload, 0, 0, 0, 3, '/', '.', '.') + // longname = "/.." + payload = append(payload, 0, 0, 0, 3, '/', '.', '.') + // attrs flags = 0 + payload = append(payload, 0, 0, 0, 0) + _, _ = wr.Write(buildSFTPPacket(sshFxpName, payload)) + return + } + // Second READDIR returns SSH_FX_EOF (1). + var b [16]byte + binary.BigEndian.PutUint32(b[0:4], sid) + binary.BigEndian.PutUint32(b[4:8], 1) + _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) + default: + // SSH_FX_OK (0) for SSH_FXP_CLOSE + var b [16]byte + binary.BigEndian.PutUint32(b[0:4], sid) + _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) + } + }) + } + + t.Run("ReadDirTraversal/VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableSFTPPathAndRemoveGuard, func() { + client := makeReaddirServer(t) + entries, err := client.ReadDir("/dir") + if err != nil { + t.Fatalf("ReadDir: %v", err) + } + if len(entries) != 1 || entries[0].Name() != ".." { + t.Fatalf("expected pkg/sftp ReadDir to return Name() == \"..\" for \"/..\", got %v; if fixed upstream, remove EnableSFTPPathAndRemoveGuard", entries) + } + }) + }) + + t.Run("ReadDirTraversal/RemediationEnabled", func(t *testing.T) { + client := makeReaddirServer(t) + _, err := client.ReadDir("/dir") + if err == nil { + t.Fatal("expected ReadDir to reject \"/..\" filename when EnableSFTPPathAndRemoveGuard is enabled") + } + }) + + // 2. Verify RemoveAll symlink following against a real local sftp.Server. + setupSymlinkTree := func(t *testing.T) (string, string, string, *sftp.Client) { + t.Helper() + tmpDir := t.TempDir() + targetDir := filepath.Join(tmpDir, "target") + if err := os.Mkdir(targetDir, 0o755); err != nil { + t.Fatal(err) + } + secretFile := filepath.Join(targetDir, "keep.txt") + if err := os.WriteFile(secretFile, []byte("important"), 0o600); err != nil { + t.Fatal(err) + } + linkPath := filepath.Join(tmpDir, "link_to_target") + if err := os.Symlink(targetDir, linkPath); err != nil { + t.Fatal(err) + } + + c1, s1 := io.Pipe() + s2, c2 := io.Pipe() + srv, err := sftp.NewServer(struct { + io.Reader + io.WriteCloser + }{c1, c2}) + if err != nil { + t.Fatal(err) + } + go func() { + _ = srv.Serve() + }() + t.Cleanup(func() { + _ = srv.Close() + }) + client, err := NewSFTPClientPipe(s2, s1) + if err != nil { + t.Fatal(err) + } + return linkPath, secretFile, targetDir, client + } + + t.Run("RemoveAllSymlink/VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableSFTPPathAndRemoveGuard, func() { + linkPath, secretFile, _, client := setupSymlinkTree(t) + _ = RemoveAll(client, linkPath) + if _, err := os.Stat(secretFile); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("expected pkg/sftp Client.RemoveAll to follow symlink and delete %s; if fixed upstream, remove RemoveAll wrapper", secretFile) + } + }) + }) + + t.Run("RemoveAllSymlink/RemediationEnabled", func(t *testing.T) { + linkPath, secretFile, _, client := setupSymlinkTree(t) + if err := RemoveAll(client, linkPath); err != nil { + t.Fatalf("RemoveAll: %v", err) + } + if _, err := os.Lstat(linkPath); !errors.Is(err, os.ErrNotExist) { + t.Errorf("expected symlink %s to be removed, err = %v", linkPath, err) + } + if _, err := os.Stat(secretFile); err != nil { + t.Errorf("expected target file %s to remain intact, err = %v", secretFile, err) + } + }) +} + +func TestSFTPResourceGuard(t *testing.T) { + // Zero-length SSH_FXP_DATA infinite read loop test. + makeZeroDataServer := func(t *testing.T, readCalls *atomic.Int32) *sftp.Client { + return runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { + switch pktType { + case 3: // SSH_FXP_OPEN + var b [9]byte + binary.BigEndian.PutUint32(b[0:4], sid) + binary.BigEndian.PutUint32(b[4:8], 1) + b[8] = 'f' + _, _ = wr.Write(buildSFTPPacket(sshFxpHandle, b[:])) + case 5: // SSH_FXP_READ + n := readCalls.Add(1) + if n >= 5 { + // Stop the infinite loop in vulnerable mode after 5 iterations. + var b [16]byte + binary.BigEndian.PutUint32(b[0:4], sid) + binary.BigEndian.PutUint32(b[4:8], 1) // SSH_FX_EOF + _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) + return + } + // Respond with 0-byte SSH_FXP_DATA. + var b [8]byte + binary.BigEndian.PutUint32(b[0:4], sid) + binary.BigEndian.PutUint32(b[4:8], 0) + _, _ = wr.Write(buildSFTPPacket(sshFxpData, b[:])) + default: + var b [16]byte + binary.BigEndian.PutUint32(b[0:4], sid) + _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) + } + }) + } + + t.Run("ZeroByteData/VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableSFTPResourceGuard, func() { + var readCalls atomic.Int32 + client := makeZeroDataServer(t, &readCalls) + f, err := client.Open("/zero") + if err != nil { + t.Fatalf("Open: %v", err) + } + defer f.Close() + buf := make([]byte, 16) + _, _ = f.Read(buf) + if got := readCalls.Load(); got < 5 { + t.Fatalf("expected pkg/sftp to loop on 0-byte SSH_FXP_DATA (readCalls=%d, want >=5); if fixed upstream, remove EnableSFTPResourceGuard", got) + } + }) + }) + + t.Run("ZeroByteData/RemediationEnabled", func(t *testing.T) { + var readCalls atomic.Int32 + client := makeZeroDataServer(t, &readCalls) + f, err := client.Open("/zero") + if err != nil { + t.Fatalf("Open: %v", err) + } + defer f.Close() + buf := make([]byte, 16) + _, err = f.Read(buf) + if err == nil { + t.Fatal("expected error on 0-byte SSH_FXP_DATA with EnableSFTPResourceGuard enabled") + } + if got := readCalls.Load(); got != 1 { + t.Fatalf("readCalls = %d, want 1", got) + } + }) +} diff --git a/go/internal/depfix/ssh.go b/go/internal/depfix/ssh.go new file mode 100644 index 0000000..1454f05 --- /dev/null +++ b/go/internal/depfix/ssh.go @@ -0,0 +1,357 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package depfix + +import ( + "bytes" + "crypto/ecdsa" + "crypto/ed25519" + "crypto/sha256" + "encoding/binary" + "errors" + "fmt" + "io" + "math/big" + + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" +) + +const ( + agentFailureMsg = 5 + agentRequestIdentities = 11 + agentSignRequest = 13 +) + +var ( + // MaxConcurrentAgentChannels is the maximum number of concurrent + // "auth-agent@openssh.com" channels accepted from a remote SSH server + // when EnableSSHAgentGuard is true. + MaxConcurrentAgentChannels = 8 + + // MaxAgentRequestBytes is the maximum frame size accepted on a + // forwarded agent channel before passing the request to + // agent.ServeAgent (which otherwise pre-allocates up to 16 MiB per + // channel upon reading a 4-byte length header). + MaxAgentRequestBytes uint32 = 256 * 1024 + + ErrAgentRequestTooLarge = errors.New("ssh/agent: request exceeds maximum allowed size") + ErrDeprecatedCertSigAlgo = errors.New("certificate signature uses deprecated algorithm") + ErrInvalidCertSignature = errors.New("certificate signature is invalid") +) + +// ForwardToAgent routes incoming "auth-agent@openssh.com" channels on client to +// keyring. When EnableSSHAgentGuard is true, it limits concurrent agent +// channels, caps request frame sizes to MaxAgentRequestBytes, and rejects +// non-list/sign requests before agent.ServeAgent parses private key payloads. +func ForwardToAgent(client *ssh.Client, keyring agent.Agent) error { + if !EnableSSHAgentGuard { + return agent.ForwardToAgent(client, keyring) + } + channels := client.HandleChannelOpen("auth-agent@openssh.com") + if channels == nil { + return errors.New("agent: already have handler for auth-agent@openssh.com") + } + sem := make(chan struct{}, MaxConcurrentAgentChannels) + go func() { + for newChan := range channels { + select { + case sem <- struct{}{}: + default: + _ = newChan.Reject(ssh.ResourceShortage, "too many concurrent agent channels") + continue + } + ch, reqs, err := newChan.Accept() + if err != nil { + <-sem + continue + } + go ssh.DiscardRequests(reqs) + go func(c ssh.Channel) { + defer func() { + _ = c.Close() + <-sem + }() + _ = ServeAgent(keyring, c) + }(ch) + } + }() + return nil +} + +// ServeAgent serves the SSH agent protocol over c. When EnableSSHAgentGuard is +// true, it wraps c in an agentSafeChannel that enforces MaxAgentRequestBytes +// and filters out disallowed message types before agent.ServeAgent reads them. +func ServeAgent(keyring agent.Agent, c io.ReadWriter) error { + if !EnableSSHAgentGuard { + return agent.ServeAgent(keyring, c) + } + return agent.ServeAgent(keyring, &agentSafeChannel{rw: c}) +} + +type agentSafeChannel struct { + rw io.ReadWriter + buf bytes.Buffer + err error +} + +func (a *agentSafeChannel) Write(p []byte) (int, error) { + return a.rw.Write(p) +} + +func (a *agentSafeChannel) Read(p []byte) (int, error) { + for a.buf.Len() == 0 { + if a.err != nil { + return 0, a.err + } + if err := a.readNextFrame(); err != nil { + a.err = err + return 0, err + } + } + return a.buf.Read(p) +} + +func (a *agentSafeChannel) readNextFrame() error { + for { + var hdr [4]byte + if _, err := io.ReadFull(a.rw, hdr[:]); err != nil { + return err + } + length := binary.BigEndian.Uint32(hdr[:]) + if length == 0 || length > MaxAgentRequestBytes { + return fmt.Errorf("%w: %d bytes", ErrAgentRequestTooLarge, length) + } + var msgType [1]byte + if _, err := io.ReadFull(a.rw, msgType[:]); err != nil { + if errors.Is(err, io.EOF) { + return io.ErrUnexpectedEOF + } + return err + } + // Only allow RequestIdentities (11) and SignRequest (13) to reach + // agent.ServeAgent. For any other message type (such as AddIdentity + // which triggers expensive RSA key validation in ServeAgent before + // forwardedAgent.Add is called), discard the payload and return + // SSH_AGENT_FAILURE immediately. + if msgType[0] != agentRequestIdentities && msgType[0] != agentSignRequest { + if length > 1 { + if _, err := io.CopyN(io.Discard, a.rw, int64(length-1)); err != nil { + return err + } + } + if _, err := a.rw.Write([]byte{0, 0, 0, 1, agentFailureMsg}); err != nil { + return err + } + continue + } + payload := make([]byte, length-1) + if _, err := io.ReadFull(a.rw, payload); err != nil { + if errors.Is(err, io.EOF) { + return io.ErrUnexpectedEOF + } + return err + } + a.buf.Write(hdr[:]) + a.buf.Write(msgType[:]) + a.buf.Write(payload) + return nil + } +} + +// WrapSSHConn wraps an ssh.Conn so that when EnableSSHSessionExitGuard is true, +// malformed "exit-status" or "exit-signal" channel requests on "session" +// channels close the session channel and drain remaining channel requests +// rather than deadlocking the connection's multiplexer loop. +func WrapSSHConn(c ssh.Conn) ssh.Conn { + if !EnableSSHSessionExitGuard { + return c + } + return &safeSSHConn{Conn: c} +} + +type safeSSHConn struct { + ssh.Conn +} + +func (s *safeSSHConn) OpenChannel(name string, data []byte) (ssh.Channel, <-chan *ssh.Request, error) { + ch, reqs, err := s.Conn.OpenChannel(name, data) + if err != nil || name != "session" || !EnableSSHSessionExitGuard { + return ch, reqs, err + } + out := make(chan *ssh.Request) + go func() { + defer close(out) + for msg := range reqs { + malformedExit := false + switch msg.Type { + case "exit-status": + if len(msg.Payload) < 4 { + malformedExit = true + } + case "exit-signal": + var sigval struct { + Signal string + CoreDumped bool + Error string + Lang string + } + if err := ssh.Unmarshal(msg.Payload, &sigval); err != nil { + malformedExit = true + } + } + out <- msg + if malformedExit { + // (*ssh.Session).wait returns early upon receiving a + // malformed exit-status/exit-signal without closing the + // channel or draining reqs. Close ch to unblock + // (*ssh.Session).Wait's copyFuncs and drain remaining + // requests so mux.loop() cannot block. + _ = ch.Close() + go ssh.DiscardRequests(reqs) + return + } + } + }() + return ch, out, nil +} + +// VerifyCertificateSignature checks the cryptographic signature on cert. When +// EnableCertSignatureGuard is true, it rejects weak SHA-1 (ssh-rsa) and DSA +// (ssh-dss) CA signatures and accepts security-key (sk-*) CA signatures that +// omit the UserPresence flag (matching OpenSSH and ssh.CertChecker.CheckCert). +func VerifyCertificateSignature(cert *ssh.Certificate) error { + if cert == nil || cert.SignatureKey == nil || cert.Signature == nil { + return ErrInvalidCertSignature + } + if EnableCertSignatureGuard { + switch cert.Signature.Format { + case ssh.KeyAlgoRSA, ssh.KeyAlgoDSA: + return fmt.Errorf("%w: %s", ErrDeprecatedCertSigAlgo, cert.Signature.Format) + } + } + c2 := *cert + c2.Signature = nil + signBytes := c2.Marshal() + if len(signBytes) < 4 { + return ErrInvalidCertSignature + } + signedData := signBytes[:len(signBytes)-4] + err := cert.SignatureKey.Verify(signedData, cert.Signature) + if err == nil { + return nil + } + if EnableCertSignatureGuard { + switch cert.SignatureKey.Type() { + case ssh.KeyAlgoSKECDSA256, ssh.KeyAlgoSKED25519: + if verifySKSignatureWithoutUP(cert.SignatureKey, signedData, cert.Signature) == nil { + return nil + } + } + } + return ErrInvalidCertSignature +} + +func verifySKSignatureWithoutUP(pub ssh.PublicKey, data []byte, sig *ssh.Signature) error { + if sig == nil || sig.Format != pub.Type() { + return ErrInvalidCertSignature + } + cryptoPub, ok := pub.(ssh.CryptoPublicKey) + if !ok { + return ErrInvalidCertSignature + } + var skf struct { + Flags byte + Counter uint32 + } + if err := ssh.Unmarshal(sig.Rest, &skf); err != nil { + return err + } + switch pub.Type() { + case ssh.KeyAlgoSKECDSA256: + var w struct { + Name string + Curve string + KeyBytes []byte + Application string + } + if err := ssh.Unmarshal(pub.Marshal(), &w); err != nil { + return err + } + var ecSig struct { + R *big.Int + S *big.Int + } + if err := ssh.Unmarshal(sig.Blob, &ecSig); err != nil { + return err + } + ecPub, ok := cryptoPub.CryptoPublicKey().(*ecdsa.PublicKey) + if !ok { + return ErrInvalidCertSignature + } + appDigest := sha256.Sum256([]byte(w.Application)) + dataDigest := sha256.Sum256(data) + blob := ssh.Marshal(struct { + ApplicationDigest []byte `ssh:"rest"` + Flags byte + Counter uint32 + MessageDigest []byte `ssh:"rest"` + }{appDigest[:], skf.Flags, skf.Counter, dataDigest[:]}) + digest := sha256.Sum256(blob) + if ecdsa.Verify(ecPub, digest[:], ecSig.R, ecSig.S) { + return nil + } + case ssh.KeyAlgoSKED25519: + var w struct { + Name string + KeyBytes []byte + Application string + } + if err := ssh.Unmarshal(pub.Marshal(), &w); err != nil { + return err + } + var edSig struct { + Signature []byte `ssh:"rest"` + } + if err := ssh.Unmarshal(sig.Blob, &edSig); err != nil { + return err + } + edPub, ok := cryptoPub.CryptoPublicKey().(ed25519.PublicKey) + if !ok { + return ErrInvalidCertSignature + } + appDigest := sha256.Sum256([]byte(w.Application)) + dataDigest := sha256.Sum256(data) + blob := ssh.Marshal(struct { + ApplicationDigest []byte `ssh:"rest"` + Flags byte + Counter uint32 + MessageDigest []byte `ssh:"rest"` + }{appDigest[:], skf.Flags, skf.Counter, dataDigest[:]}) + if ed25519.Verify(edPub, blob, edSig.Signature) { + return nil + } + } + return ErrInvalidCertSignature +} diff --git a/go/internal/depfix/ssh_test.go b/go/internal/depfix/ssh_test.go new file mode 100644 index 0000000..62d1f5a --- /dev/null +++ b/go/internal/depfix/ssh_test.go @@ -0,0 +1,429 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package depfix + +import ( + "crypto/ed25519" + "crypto/rand" + "crypto/rsa" + "crypto/sha256" + "encoding/binary" + "errors" + "io" + "net" + "sync/atomic" + "testing" + "time" + + "golang.org/x/crypto/ssh" + "golang.org/x/crypto/ssh/agent" +) + +type spyAgent struct { + agent.Agent + addCalled atomic.Int32 +} + +func (s *spyAgent) List() ([]*agent.Key, error) { + return nil, nil +} + +func (s *spyAgent) Add(key agent.AddedKey) error { + s.addCalled.Add(1) + return errors.New("operation not permitted on forwarded agent") +} + +func TestSSHAgentGuard(t *testing.T) { + // 1. Disallowed AddIdentity message triggers key parsing & Add() when + // EnableSSHAgentGuard is disabled, and is blocked at the framing layer + // when EnableSSHAgentGuard is enabled. + _, edPriv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + + t.Run("AddIdentityPreParse/VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableSSHAgentGuard, func() { + c1, s1 := net.Pipe() + defer c1.Close() + defer s1.Close() + spy := &spyAgent{} + go func() { + _ = ServeAgent(spy, s1) + }() + client := agent.NewClient(c1) + _ = client.Add(agent.AddedKey{PrivateKey: edPriv, Comment: "attacker-key"}) + if spy.addCalled.Load() == 0 { + t.Fatal("expected x/crypto/ssh/agent ServeAgent to parse key and call Add when EnableSSHAgentGuard is disabled; if fixed upstream, remove EnableSSHAgentGuard") + } + }) + }) + + t.Run("AddIdentityPreParse/RemediationEnabled", func(t *testing.T) { + c1, s1 := net.Pipe() + defer c1.Close() + defer s1.Close() + spy := &spyAgent{} + go func() { + _ = ServeAgent(spy, s1) + }() + client := agent.NewClient(c1) + if err := client.Add(agent.AddedKey{PrivateKey: edPriv, Comment: "attacker-key"}); err == nil { + t.Fatal("expected Add to fail on forwarded agent") + } + if got := spy.addCalled.Load(); got != 0 { + t.Fatalf("expected agentSafeChannel to reject AddIdentity before ServeAgent called Add, got addCalled=%d", got) + } + // Subsequent List() on the same channel still succeeds. + if _, err := client.List(); err != nil { + t.Fatalf("List after rejected Add: %v", err) + } + }) + + // 2. Oversized agent frame length header (> MaxAgentRequestBytes). + t.Run("OversizedFrame/VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableSSHAgentGuard, func() { + c1, s1 := net.Pipe() + defer c1.Close() + defer s1.Close() + errCh := make(chan error, 1) + go func() { + errCh <- ServeAgent(&spyAgent{}, s1) + }() + // Send a 4-byte length header of MaxAgentRequestBytes + 100 (still <= 16 MiB). + var hdr [4]byte + binary.BigEndian.PutUint32(hdr[:], MaxAgentRequestBytes+100) + if _, err := c1.Write(hdr[:]); err != nil { + t.Fatal(err) + } + // ServeAgent pre-allocates make([]byte, l) and blocks in io.ReadFull waiting for payload. + select { + case err := <-errCh: + t.Fatalf("expected ServeAgent to accept header > MaxAgentRequestBytes and wait for payload, but got %v", err) + case <-time.After(50 * time.Millisecond): + _ = c1.Close() + } + }) + }) + + t.Run("OversizedFrame/RemediationEnabled", func(t *testing.T) { + c1, s1 := net.Pipe() + defer c1.Close() + defer s1.Close() + errCh := make(chan error, 1) + go func() { + errCh <- ServeAgent(&spyAgent{}, s1) + }() + var hdr [4]byte + binary.BigEndian.PutUint32(hdr[:], MaxAgentRequestBytes+100) + _, _ = c1.Write(hdr[:]) + select { + case err := <-errCh: + if !errors.Is(err, ErrAgentRequestTooLarge) { + t.Fatalf("expected ErrAgentRequestTooLarge, got %v", err) + } + case <-time.After(time.Second): + t.Fatal("timed out waiting for ServeAgent to reject oversized frame header") + } + }) +} + +func TestSSHSessionExitGuard(t *testing.T) { + _, hostPriv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + hostSigner, err := ssh.NewSignerFromKey(hostPriv) + if err != nil { + t.Fatal(err) + } + + startMalformedExitServer := func(t *testing.T) (*ssh.Client, func()) { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + srvCfg := &ssh.ServerConfig{NoClientAuth: true} + srvCfg.AddHostKey(hostSigner) + + done := make(chan struct{}) + go func() { + defer close(done) + s1, err := ln.Accept() + if err != nil { + return + } + defer s1.Close() + _, chans, reqs, err := ssh.NewServerConn(s1, srvCfg) + if err != nil { + return + } + go ssh.DiscardRequests(reqs) + for newChan := range chans { + if newChan.ChannelType() != "session" { + _ = newChan.Reject(ssh.UnknownChannelType, "unsupported") + continue + } + ch, chReqs, err := newChan.Accept() + if err != nil { + return + } + go func() { + for req := range chReqs { + if req.Type == "exec" { + _ = req.Reply(true, nil) + // Send a malformed 1-byte exit-status (needs 4 bytes), then + // send 20 extra channel requests without closing ch. + _, _ = ch.SendRequest("exit-status", false, []byte{0}) + for range 20 { + _, _ = ch.SendRequest("keepalive@openssh.com", false, nil) + } + return + } + _ = req.Reply(false, nil) + } + }() + } + }() + + c1, err := net.Dial("tcp", ln.Addr().String()) + if err != nil { + _ = ln.Close() + t.Fatalf("Dial: %v", err) + } + cliCfg := &ssh.ClientConfig{ + User: "test", + HostKeyCallback: ssh.FixedHostKey(hostSigner.PublicKey()), + } + conn, chans, reqs, err := ssh.NewClientConn(c1, "localhost", cliCfg) + if err != nil { + _ = c1.Close() + _ = ln.Close() + t.Fatalf("NewClientConn: %v", err) + } + client := ssh.NewClient(WrapSSHConn(conn), chans, reqs) + cleanup := func() { + _ = client.Close() + _ = c1.Close() + _ = ln.Close() + <-done + } + return client, cleanup + } + + t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableSSHSessionExitGuard, func() { + client, cleanup := startMalformedExitServer(t) + defer cleanup() + + sess, err := client.NewSession() + if err != nil { + t.Fatalf("NewSession: %v", err) + } + sess.Stdout = io.Discard + if err := sess.Start("cmd"); err != nil { + t.Fatalf("Start: %v", err) + } + waitCh := make(chan error, 1) + go func() { + waitCh <- sess.Wait() + }() + select { + case err := <-waitCh: + t.Fatalf("expected (*ssh.Session).Wait to hang on malformed exit-status when EnableSSHSessionExitGuard is disabled (got %v); if fixed upstream, remove WrapSSHConn", err) + case <-time.After(150 * time.Millisecond): + // Expected hang in vulnerable mode. + } + }) + }) + + t.Run("RemediationEnabled", func(t *testing.T) { + client, cleanup := startMalformedExitServer(t) + defer cleanup() + + sess, err := client.NewSession() + if err != nil { + t.Fatalf("NewSession: %v", err) + } + sess.Stdout = io.Discard + if err := sess.Start("cmd"); err != nil { + t.Fatalf("Start: %v", err) + } + waitCh := make(chan error, 1) + go func() { + waitCh <- sess.Wait() + }() + select { + case err := <-waitCh: + if err == nil { + t.Fatal("expected error from Wait on malformed exit-status") + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for sess.Wait() with EnableSSHSessionExitGuard enabled") + } + + // Verify the connection multiplexer is not deadlocked by sending a global request. + reqDone := make(chan error, 1) + go func() { + _, _, err := client.SendRequest("keepalive@openssh.com", true, nil) + reqDone <- err + }() + select { + case err := <-reqDone: + if err != nil { + t.Fatalf("client.SendRequest failed: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("SSH connection multiplexer deadlocked after malformed exit-status") + } + }) +} + +type rsaSHA1Signer struct { + ssh.Signer +} + +func (s rsaSHA1Signer) Sign(rand io.Reader, data []byte) (*ssh.Signature, error) { + return s.Signer.(ssh.AlgorithmSigner).SignWithAlgorithm(rand, data, ssh.KeyAlgoRSA) +} + +func TestCertSignatureGuard(t *testing.T) { + // 1. RSA SHA-1 ("ssh-rsa") CA signature. + rsaPriv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + rsaSigner, err := ssh.NewSignerFromKey(rsaPriv) + if err != nil { + t.Fatal(err) + } + pub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + sshPub, err := ssh.NewPublicKey(pub) + if err != nil { + t.Fatal(err) + } + + sha1Cert := &ssh.Certificate{ + Key: sshPub, + CertType: ssh.HostCert, + ValidBefore: ssh.CertTimeInfinity, + } + if err := sha1Cert.SignCert(rand.Reader, rsaSHA1Signer{rsaSigner}); err != nil { + t.Fatal(err) + } + if sha1Cert.Signature.Format != ssh.KeyAlgoRSA { + t.Fatalf("Signature.Format = %q, want %q", sha1Cert.Signature.Format, ssh.KeyAlgoRSA) + } + + t.Run("RSASHA1/VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableCertSignatureGuard, func() { + if err := VerifyCertificateSignature(sha1Cert); err != nil { + t.Fatalf("expected legacy VerifyCertificateSignature to accept ssh-rsa CA signature, got %v", err) + } + }) + }) + + t.Run("RSASHA1/RemediationEnabled", func(t *testing.T) { + if err := VerifyCertificateSignature(sha1Cert); !errors.Is(err, ErrDeprecatedCertSigAlgo) { + t.Fatalf("expected ErrDeprecatedCertSigAlgo for ssh-rsa CA signature, got %v", err) + } + // Verify RSA SHA-256/512 signed certificate succeeds. + sha2Cert := &ssh.Certificate{ + Key: sshPub, + CertType: ssh.HostCert, + ValidBefore: ssh.CertTimeInfinity, + } + if err := sha2Cert.SignCert(rand.Reader, rsaSigner); err != nil { + t.Fatal(err) + } + if err := VerifyCertificateSignature(sha2Cert); err != nil { + t.Fatalf("VerifyCertificateSignature(rsa-sha2-512): %v", err) + } + }) + + // 2. Security-key (sk-ssh-ed25519@openssh.com) CA signature without UserPresence flag (Flags = 0). + skEdPub, skEdPriv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + skPubBlob := ssh.Marshal(struct { + Name string + KeyBytes []byte + Application string + }{ + Name: ssh.KeyAlgoSKED25519, + KeyBytes: skEdPub, + Application: "ssh:ca", + }) + skPubKey, err := ssh.ParsePublicKey(skPubBlob) + if err != nil { + t.Fatalf("ParsePublicKey(sk-ed25519): %v", err) + } + skCert := &ssh.Certificate{ + Nonce: make([]byte, 32), + Key: sshPub, + CertType: ssh.HostCert, + ValidBefore: ssh.CertTimeInfinity, + SignatureKey: skPubKey, + } + c2 := *skCert + c2.Signature = nil + raw := c2.Marshal() + signedBytes := raw[:len(raw)-4] + appDigest := sha256.Sum256([]byte("ssh:ca")) + dataDigest := sha256.Sum256(signedBytes) + skSignedBlob := ssh.Marshal(struct { + ApplicationDigest []byte `ssh:"rest"` + Flags byte + Counter uint32 + MessageDigest []byte `ssh:"rest"` + }{appDigest[:], 0x00 /* Flags = 0 (no-touch-required) */, 1, dataDigest[:]}) + edSig := ed25519.Sign(skEdPriv, skSignedBlob) + skCert.Signature = &ssh.Signature{ + Format: ssh.KeyAlgoSKED25519, + Blob: edSig, + Rest: ssh.Marshal(struct { + Flags byte + Counter uint32 + }{0x00, 1}), + } + + t.Run("SKNoTouchCA/VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableCertSignatureGuard, func() { + if err := VerifyCertificateSignature(skCert); err == nil { + t.Fatal("expected PublicKey.Verify to reject sk-ssh-ed25519 CA signature with Flags=0 when EnableCertSignatureGuard is disabled") + } + }) + }) + + t.Run("SKNoTouchCA/RemediationEnabled", func(t *testing.T) { + if err := VerifyCertificateSignature(skCert); err != nil { + t.Fatalf("expected VerifyCertificateSignature to accept sk-ssh-ed25519 CA signature with Flags=0, got %v", err) + } + }) +} diff --git a/go/internal/depfix/term.go b/go/internal/depfix/term.go new file mode 100644 index 0000000..b738511 --- /dev/null +++ b/go/internal/depfix/term.go @@ -0,0 +1,120 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package depfix + +import ( + "bytes" + "io" +) + +const ( + // maxTermEscapeChunk is the maximum number of bytes allowed in an + // unterminated escape sequence before injecting a '~' terminator and + // restarting '\x1b', keeping golang.org/x/term's 256-byte inBuf from + // ever filling up. + maxTermEscapeChunk = 64 +) + +// WrapTermReadWriter wraps an io.ReadWriter passed to golang.org/x/term's +// NewTerminal to prevent a 256-byte unterminated escape sequence from filling +// Terminal.inBuf and causing an infinite synchronous 0-byte Read busy loop. +func WrapTermReadWriter(rw io.ReadWriter) io.ReadWriter { + return &termSafeReadWriter{rw: rw} +} + +type termSafeReadWriter struct { + rw io.ReadWriter + outBuf bytes.Buffer + inEsc bool + escLen int +} + +func (t *termSafeReadWriter) Write(p []byte) (int, error) { + return t.rw.Write(p) +} + +func isTermEscTerminator(c byte) bool { + return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '~' +} + +func (t *termSafeReadWriter) Read(p []byte) (int, error) { + if !EnableTermEscapeGuard { + return t.rw.Read(p) + } + if t.outBuf.Len() > 0 { + return t.outBuf.Read(p) + } + raw := make([]byte, max(len(p), 64)) + n, err := t.rw.Read(raw) + for i := 0; i < n; i++ { + c := raw[i] + if !t.inEsc { + if c == 0x1b { + t.inEsc = true + t.escLen = 1 + } + t.outBuf.WriteByte(c) + continue + } + if c == 0x1b { + // Terminate the previous unfinished escape sequence before + // starting the new one. + t.outBuf.WriteByte('~') + t.outBuf.WriteByte(0x1b) + t.escLen = 1 + continue + } + if c == '\r' || c == '\n' || c == 0x03 || c == 0x04 { + // Terminate the unfinished escape sequence so line/control + // keys are still processed by x/term. + t.outBuf.WriteByte('~') + t.outBuf.WriteByte(c) + t.inEsc = false + t.escLen = 0 + continue + } + if isTermEscTerminator(c) { + t.outBuf.WriteByte(c) + t.inEsc = false + t.escLen = 0 + continue + } + if t.escLen >= maxTermEscapeChunk-1 { + // Close the current chunk with '~' so x/term consumes it as + // keyUnknown and clears t.remainder, then re-open '\x1b' so + // subsequent bytes of the escape sequence remain inside an + // escape sequence. + t.outBuf.WriteByte('~') + t.outBuf.WriteByte(0x1b) + t.escLen = 1 + } + t.outBuf.WriteByte(c) + t.escLen++ + } + if t.outBuf.Len() > 0 { + nn, _ := t.outBuf.Read(p) + return nn, err + } + return 0, err +} diff --git a/go/internal/depfix/term_test.go b/go/internal/depfix/term_test.go new file mode 100644 index 0000000..e90edd6 --- /dev/null +++ b/go/internal/depfix/term_test.go @@ -0,0 +1,86 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package depfix + +import ( + "bytes" + "errors" + "io" + "strings" + "sync/atomic" + "testing" + + "golang.org/x/term" +) + +type trackingTermIO struct { + in *bytes.Reader + zeroReadCalls atomic.Int32 +} + +func (t *trackingTermIO) Read(p []byte) (int, error) { + if len(p) == 0 { + if t.zeroReadCalls.Add(1) >= 10 { + return 0, errors.New("detected infinite 0-byte Read loop in golang.org/x/term") + } + return 0, nil + } + return t.in.Read(p) +} + +func (t *trackingTermIO) Write(p []byte) (int, error) { + return len(p), nil +} + +func TestTermEscapeGuard(t *testing.T) { + // 260-byte unterminated escape sequence (exceeds x/term's 256-byte inBuf) + // followed by a normal terminator 'm' and "hello\n". + input := "\x1b[" + strings.Repeat("0", 260) + "mhello\n" + + t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { + WithDisabled(&EnableTermEscapeGuard, func() { + tio := &trackingTermIO{in: bytes.NewReader([]byte(input))} + vt := term.NewTerminal(WrapTermReadWriter(tio), "> ") + _, err := vt.ReadLine() + if tio.zeroReadCalls.Load() == 0 { + t.Fatalf("expected golang.org/x/term to call Read with len(p)==0 on 260-byte escape sequence (err=%v); if fixed upstream, remove WrapTermReadWriter", err) + } + }) + }) + + t.Run("RemediationEnabled", func(t *testing.T) { + tio := &trackingTermIO{in: bytes.NewReader([]byte(input))} + vt := term.NewTerminal(WrapTermReadWriter(tio), "> ") + line, err := vt.ReadLine() + if err != nil && !errors.Is(err, io.EOF) { + t.Fatalf("ReadLine: %v", err) + } + if tio.zeroReadCalls.Load() != 0 { + t.Fatalf("unexpected 0-byte Read calls: %d", tio.zeroReadCalls.Load()) + } + if line != "hello" { + t.Fatalf("ReadLine() = %q, want %q", line, "hello") + } + }) +} diff --git a/go/internal/jsutil/streams.go b/go/internal/jsutil/streams.go index 31bdfc8..5f862a1 100644 --- a/go/internal/jsutil/streams.go +++ b/go/internal/jsutil/streams.go @@ -100,7 +100,16 @@ func NewReadableStream(r io.Reader, done chan<- error, progress func(int64)) js. size = max(size, ds.Int()) } buf := make([]byte, size) - n, err := r.Read(buf) + var n int + var err error + TryCatch( + func() { + n, err = r.Read(buf) + }, + func(p any) { + err = fmt.Errorf("panic: %v", p) + }, + ) if n > 0 { select { case <-cancelCh: diff --git a/go/internal/shellwords/shellwords.go b/go/internal/shellwords/shellwords.go index 33b6742..be1f2e0 100644 --- a/go/internal/shellwords/shellwords.go +++ b/go/internal/shellwords/shellwords.go @@ -157,7 +157,6 @@ func Parse(line string, opts ...Option) ([]string, []string) { if escaped { buf += "\\" - bufRaw += "\\" } if got != argNo { diff --git a/go/internal/shellwords/shellwords_test.go b/go/internal/shellwords/shellwords_test.go index 0222b82..dca8a5b 100644 --- a/go/internal/shellwords/shellwords_test.go +++ b/go/internal/shellwords/shellwords_test.go @@ -71,6 +71,7 @@ func TestSimple(t *testing.T) { {`foo*[a-b]?`, []string{`foo*[a-b]?`}, []string{`foo*[a-b]?`}}, {`foo*[a-b]\?`, []string{`foo*[a-b]?`}, []string{`foo*[a-b]\?`}}, {`"foo*[a-b]?"`, []string{`foo*[a-b]?`}, []string{`"foo*[a-b]?"`}}, + {`foo\`, []string{`foo\`}, []string{`foo\`}}, } for _, testcase := range testcases { diff --git a/go/internal/terminal/terminal.go b/go/internal/terminal/terminal.go index 23bc68f..345312b 100644 --- a/go/internal/terminal/terminal.go +++ b/go/internal/terminal/terminal.go @@ -38,6 +38,7 @@ import ( "golang.org/x/term" + "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/jsutil" ) @@ -54,7 +55,7 @@ func New(ctx context.Context, t js.Value) *Terminal { onData: make(map[int]func(k string) any), }, } - tt.vt = term.NewTerminal(tt.tw, "") + tt.vt = term.NewTerminal(depfix.WrapTermReadWriter(tt.tw), "") tt.Escape = tt.vt.Escape tt.setDefaultPrompt() @@ -315,6 +316,26 @@ func (t *Terminal) Clear() { t.tw.xt.Call("clear") } +// ResetSessionState resets terminal modes/colors that may have been altered by +// a remote session and drains any pending asynchronous query responses from the +// input buffer before returning to the local prompt. +func (t *Terminal) ResetSessionState() { + for { + select { + case <-t.tw.dataCh: + default: + t.tw.r = nil + _, _ = t.tw.Write([]byte( + "\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l" + + "\x1b[?1004l\x1b[?2004l\x1b[?6l\x1b[r\x1b[?1049l" + + "\x1b[0m\x1b[?25h" + + "\x1b]104\x07\x1b]110\x07\x1b]111\x07\x1b]112\x07", + )) + return + } + } +} + func (t *Terminal) Rows() int { return t.tw.xt.Get("rows").Int() } diff --git a/go/internal/tests/remediations_test.go b/go/internal/tests/remediations_test.go new file mode 100644 index 0000000..048539b --- /dev/null +++ b/go/internal/tests/remediations_test.go @@ -0,0 +1,312 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +//go:build wasm + +package tests + +import ( + "bytes" + "net/http" + "strings" + "syscall/js" + "testing" + "time" + + "golang.org/x/crypto/ssh" + + "github.com/c2FmZQ/sshterm/internal/app" + "github.com/c2FmZQ/sshterm/internal/depfix" + "github.com/c2FmZQ/sshterm/internal/jsutil" +) + +func TestXtermWorkarounds(t *testing.T) { + sshApp := js.Global().Get("sshApp") + termCtor := sshApp.Get("Terminal") + workarounds := sshApp.Get("workarounds") + applyWorkarounds := sshApp.Get("applyXtermWorkarounds") + if termCtor.IsUndefined() || workarounds.IsUndefined() || applyWorkarounds.IsUndefined() { + t.Fatalf("missing sshApp.Terminal / workarounds / applyXtermWorkarounds exports") + } + doc := js.Global().Get("document") + + createTestTerm := func(t *testing.T, flags map[string]any) (js.Value, func()) { + t.Helper() + container := doc.Call("createElement", "div") + container.Get("style").Set("width", "400px") + container.Get("style").Set("height", "200px") + doc.Get("body").Call("appendChild", container) + term := termCtor.New(map[string]any{"cols": 40, "rows": 10}) + term.Call("open", container) + disposables := applyWorkarounds.Invoke(term, flags) + cleanup := func() { + for i := 0; i < disposables.Length(); i++ { + disposables.Index(i).Call("dispose") + } + term.Call("dispose") + doc.Get("body").Call("removeChild", container) + } + return term, cleanup + } + + writeTerm := func(t *testing.T, term js.Value, data string) { + t.Helper() + doneCh := make(chan struct{}, 1) + cb := js.FuncOf(func(this js.Value, args []js.Value) any { + select { + case doneCh <- struct{}{}: + default: + } + return nil + }) + defer cb.Release() + var writeErr any + jsutil.TryCatch(func() { + term.Call("write", data, cb) + }, func(err any) { + writeErr = err + }) + if writeErr != nil { + t.Fatalf("term.write threw: %v", writeErr) + } + select { + case <-doneCh: + case <-time.After(2 * time.Second): + t.Fatalf("term.write callback timed out (WriteBuffer wedged)") + } + } + + // 1. CSI Ps b (REP / repeatPrecedingCharacter) + t.Run("CSIRepGuard", func(t *testing.T) { + t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { + term, cleanup := createTestTerm(t, map[string]any{ + "csiRepGuard": false, + "csiLoopGuard": false, + "oscQueryAndLinkGuard": false, + }) + defer cleanup() + writeTerm(t, term, "a") + + paramsArr := js.Global().Get("Int32Array").New(1) + paramsArr.SetIndex(0, 2147483647) + var caught any + jsutil.TryCatch(func() { + term.Get("_core").Get("_inputHandler").Call("repeatPrecedingCharacter", map[string]any{ + "length": 1, + "params": paramsArr, + }) + }, func(err any) { + caught = err + }) + if caught == nil { + t.Fatalf("expected @xterm/xterm repeatPrecedingCharacter(2147483647) to throw RangeError without csiRepGuard; if fixed upstream, remove csiRepGuard") + } + }) + + t.Run("RemediationEnabled", func(t *testing.T) { + term, cleanup := createTestTerm(t, map[string]any{ + "csiRepGuard": true, + "csiLoopGuard": true, + "oscQueryAndLinkGuard": true, + }) + defer cleanup() + writeTerm(t, term, "a\x1b[2147483647bOK") + }) + }) + + // 2. CSI Ps I/Z/L/M/S/T unbounded loop parameter clamping + t.Run("CSILoopGuard", func(t *testing.T) { + t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { + term, cleanup := createTestTerm(t, map[string]any{ + "csiRepGuard": true, + "csiLoopGuard": false, + "oscQueryAndLinkGuard": true, + }) + defer cleanup() + + seenParam := 0 + cb := js.FuncOf(func(this js.Value, args []js.Value) any { + seenParam = args[0].Get("params").Index(0).Int() + return true // intercept before built-in 2B loop freezes browser + }) + defer cb.Release() + disp := term.Get("_core").Call("registerCsiHandler", map[string]any{"final": "L"}, cb) + writeTerm(t, term, "\x1b[2147483647L") + disp.Call("dispose") + if seenParam != 2147483647 { + t.Fatalf("expected unclamped CSI L param 2147483647 without csiLoopGuard, got %d", seenParam) + } + }) + + t.Run("RemediationEnabled", func(t *testing.T) { + term, cleanup := createTestTerm(t, map[string]any{ + "csiRepGuard": true, + "csiLoopGuard": true, + "oscQueryAndLinkGuard": true, + }) + defer cleanup() + start := time.Now() + writeTerm(t, term, "\x1b[2147483647I\x1b[2147483647Z\x1b[2147483647L\x1b[2147483647M\x1b[2147483647S\x1b[2147483647T") + if elapsed := time.Since(start); elapsed > time.Second { + t.Fatalf("CSI loop guard took too long: %v", elapsed) + } + }) + }) + + // 3. OSC 4 query amplification & OSC 8 hyperlink marker flood + t.Run("OSCQueryAndLinkGuard", func(t *testing.T) { + chainedOsc4 := "\x1b]4" + strings.Repeat(";0;?", 25) + "\x07" + floodOsc8 := strings.Repeat("\x1b]8;;https://example.com\x07", 50) + "\x1b]8;;\x07" + + t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { + term, cleanup := createTestTerm(t, map[string]any{ + "csiRepGuard": true, + "csiLoopGuard": true, + "oscQueryAndLinkGuard": false, + }) + defer cleanup() + + responses := 0 + cb := js.FuncOf(func(this js.Value, args []js.Value) any { + responses++ + return nil + }) + defer cb.Release() + disp := term.Call("onData", cb) + writeTerm(t, term, chainedOsc4) + writeTerm(t, term, floodOsc8) + disp.Call("dispose") + + if responses != 25 { + t.Fatalf("expected 25 OSC 4 responses without oscQueryAndLinkGuard, got %d", responses) + } + if got := term.Get("_core").Get("markers").Length(); got < 50 { + t.Fatalf("expected >= 50 OSC 8 markers without oscQueryAndLinkGuard, got %d", got) + } + }) + + t.Run("RemediationEnabled", func(t *testing.T) { + term, cleanup := createTestTerm(t, map[string]any{ + "csiRepGuard": true, + "csiLoopGuard": true, + "oscQueryAndLinkGuard": true, + }) + defer cleanup() + + responses := 0 + cb := js.FuncOf(func(this js.Value, args []js.Value) any { + responses++ + return nil + }) + defer cb.Release() + disp := term.Call("onData", cb) + writeTerm(t, term, chainedOsc4) + writeTerm(t, term, floodOsc8) + disp.Call("dispose") + + if responses != 0 { + t.Fatalf("expected 0 OSC 4 responses for >16 chained query flood, got %d", responses) + } + if got := term.Get("_core").Get("markers").Length(); got > 32 { + t.Fatalf("expected <= 32 OSC 8 markers with oscQueryAndLinkGuard, got %d", got) + } + }) + }) + + // 4. Title sanitization and _showMessage toast DOM reuse + t.Run("TitleOverlay", func(t *testing.T) { + screen := doc.Call("querySelector", `[id^="screen-"]`) + if screen.IsNull() || screen.IsUndefined() { + t.Fatalf("screen element not found") + } + longTitle := strings.Repeat("A", 300) + "\nSPOOF" + + appConfig.Term.Call("showMessage", longTitle) + appConfig.Term.Call("showMessage", longTitle) + toasts := screen.Call("querySelectorAll", ".sshterm-toast") + if got := toasts.Length(); got != 1 { + t.Fatalf("expected exactly 1 .sshterm-toast element, got %d", got) + } + txt := toasts.Index(0).Get("textContent").String() + if len([]rune(txt)) > 129 || strings.Contains(txt, "\n") { + t.Fatalf("expected toast text to be truncated and sanitized, got len=%d %q", len(txt), txt) + } + }) +} + +func TestRSASHA2AndForwardedAgentRemediation(t *testing.T) { + a, err := app.New(appConfig) + if err != nil { + t.Fatalf("app.New: %v", err) + } + result := make(chan error) + go func() { + result <- a.Run() + }() + t.Cleanup(a.Stop) + + script(t, []line{ + {Expect: prompt}, + {Type: "db wipe\n", Expect: `Continue\?`}, + {Type: "Y\n", Expect: prompt}, + {Type: "ep add test-server websocket\n", Expect: prompt}, + {Type: "keys generate -t rsa rsatest\n", Expect: "Enter a passphrase"}, + {Type: "foobar\n", Expect: "Re-enter the same passphrase"}, + {Type: "foobar\n", Expect: prompt}, + {Type: "keys list\n", Expect: "ssh-rsa .* rsatest\r\n", Do: func(m []string) { + pub, _, _, _, err := ssh.ParseAuthorizedKey([]byte(m[0])) + if err != nil { + t.Fatalf("ssh.ParseAuthorizedKey: %v", err) + } + if _, err := http.Post("/addkey", "text/pem", bytes.NewReader(pub.Marshal())); err != nil { + t.Fatalf("http.Post: %v", err) + } + }}, + {Type: "agent add rsatest\n", Expect: "Enter the passphrase for rsatest:"}, + {Type: "foobar\n", Expect: prompt}, + }) + + // 1. With EnableRSASHA2 disabled, forwarded agent SignWithFlags(SignatureFlagRsaSha512) + // ignores the flag and returns "ssh-rsa" (SHA-1). + depfix.WithDisabled(&depfix.EnableRSASHA2, func() { + script(t, []line{ + {Type: "ssh -A testuser@test-server agent-test\n", Expect: `(?s)Host key for test-server.*Choice>`}, + {Type: "3\n", Expect: `\[agent\] test-server requested a signature with key "rsatest"`}, + {Expect: `(?s)agent-test: sign: ok \(ssh-rsa\).*agent-test: sign-sha512: ok \(ssh-rsa\).*agent-test: removeall: agent: failure`}, + {Wait: time.Second, Type: "\n\n", Expect: prompt}, + }) + }) + + // 2. With EnableRSASHA2 enabled, forwarded agent SignWithFlags(SignatureFlagRsaSha512) + // returns "rsa-sha2-512". + script(t, []line{ + {Type: "ssh -A testuser@test-server agent-test\n", Expect: `\[agent\] test-server requested a signature with key "rsatest"`}, + {Expect: `(?s)agent-test: sign: ok \(ssh-rsa\).*agent-test: sign-sha512: ok \(rsa-sha2-512\).*agent-test: removeall: agent: failure`}, + {Wait: time.Second, Type: "\n\n", Expect: prompt}, + {Type: "exit\n"}, + }) + + if err := <-result; err != nil { + t.Fatalf("Run(): %v", err) + } +} diff --git a/go/internal/testserver/main_test.go b/go/internal/testserver/main_test.go index d8e64e1..43a17cf 100644 --- a/go/internal/testserver/main_test.go +++ b/go/internal/testserver/main_test.go @@ -638,10 +638,15 @@ func (s *sshServer) agentTest(w io.Writer, sconn *ssh.ServerConn, forwarded bool return } fmt.Fprintf(w, "agent-test: list: %d keys\n", len(keys)) - if _, err := client.Sign(keys[0], []byte("data")); err != nil { + if sig, err := client.Sign(keys[0], []byte("data")); err != nil { fmt.Fprintf(w, "agent-test: sign: %v\n", err) } else { - fmt.Fprintf(w, "agent-test: sign: ok\n") + fmt.Fprintf(w, "agent-test: sign: ok (%s)\n", sig.Format) + } + if sig, err := client.SignWithFlags(keys[0], []byte("data"), agent.SignatureFlagRsaSha512); err != nil { + fmt.Fprintf(w, "agent-test: sign-sha512: %v\n", err) + } else { + fmt.Fprintf(w, "agent-test: sign-sha512: ok (%s)\n", sig.Format) } fmt.Fprintf(w, "agent-test: lock: %v\n", client.Lock([]byte("x"))) fmt.Fprintf(w, "agent-test: removeall: %v\n", client.RemoveAll()) diff --git a/go/sshterm-server/internal/server.go b/go/sshterm-server/internal/server.go index 6e088ac..4ea4bc6 100644 --- a/go/sshterm-server/internal/server.go +++ b/go/sshterm-server/internal/server.go @@ -206,8 +206,9 @@ func New(opts Options) (*Server, error) { endpoints: epMap, configJSON: cfgBytes, upgrader: websocket.Upgrader{ - ReadBufferSize: 8192, - WriteBufferSize: 8192, + HandshakeTimeout: dialTimeout, + ReadBufferSize: 8192, + WriteBufferSize: 8192, }, mux: http.NewServeMux(), logger: logger, @@ -382,7 +383,7 @@ func (s *Server) proxyConn(wsConn *websocket.Conn, tcpConn net.Conn) { ) return } - if _, err := io.Copy(tcpConn, r); err != nil { + if _, err := io.Copy(&tcpDeadlineWriter{conn: tcpConn}, r); err != nil { return } } @@ -410,6 +411,15 @@ func (s *Server) proxyConn(wsConn *websocket.Conn, tcpConn net.Conn) { wg.Wait() } +type tcpDeadlineWriter struct { + conn net.Conn +} + +func (w *tcpDeadlineWriter) Write(p []byte) (int, error) { + _ = w.conn.SetWriteDeadline(time.Now().Add(writeWait)) + return w.conn.Write(p) +} + // Addr returns the network address the server is listening on, or nil if it has // not started listening yet. func (s *Server) Addr() net.Addr { diff --git a/go/sshterm-server/internal/server_test.go b/go/sshterm-server/internal/server_test.go index 07e9759..a93bfe5 100644 --- a/go/sshterm-server/internal/server_test.go +++ b/go/sshterm-server/internal/server_test.go @@ -44,7 +44,9 @@ import ( "net/http/httptest" "os" "path/filepath" + "runtime" "strings" + "sync" "testing" "time" @@ -204,8 +206,8 @@ func TestEmbeddedStaticFilesAndHeaders(t *testing.T) { } } - // Ensure test artifacts and example config are not embedded. - for _, excluded := range []string{"/tests.html", "/tests.wasm", "/config.json.example"} { + // Ensure test artifacts, example config, and legacy xterm.js are not embedded. + for _, excluded := range []string{"/tests.html", "/tests.wasm", "/config.json.example", "/xterm.js"} { resp, err := http.Get(ts.URL + excluded) if err != nil { t.Fatalf("GET %s: %v", excluded, err) @@ -652,3 +654,89 @@ func (c *wsNetConn) RemoteAddr() net.Addr { return c.conn.RemoteAd func (c *wsNetConn) SetDeadline(t time.Time) error { return nil } func (c *wsNetConn) SetReadDeadline(t time.Time) error { return c.conn.SetReadDeadline(t) } func (c *wsNetConn) SetWriteDeadline(t time.Time) error { return c.conn.SetWriteDeadline(t) } + +func TestGoToolchainVersion(t *testing.T) { + goversionBytes, err := os.ReadFile("../../../.goversion") + if err != nil { + t.Fatalf("ReadFile(.goversion): %v", err) + } + wantVer := strings.TrimSpace(string(goversionBytes)) + + gomodBytes, err := os.ReadFile("../../go.mod") + if err != nil { + t.Fatalf("ReadFile(go.mod): %v", err) + } + if !strings.Contains(string(gomodBytes), "go "+wantVer+"\n") { + t.Errorf("go/go.mod does not declare 'go %s'", wantVer) + } + if got := runtime.Version(); !strings.HasPrefix(got, "go"+wantVer) { + t.Errorf("runtime.Version() = %q, want prefix %q", got, "go"+wantVer) + } +} + +type stallTrackConn struct { + net.Conn + mu sync.Mutex + deadlineSet bool +} + +func (s *stallTrackConn) SetWriteDeadline(t time.Time) error { + s.mu.Lock() + if !t.IsZero() { + s.deadlineSet = true + } + s.mu.Unlock() + return s.Conn.SetWriteDeadline(t) +} + +func (s *stallTrackConn) Write(p []byte) (int, error) { + s.mu.Lock() + hasDeadline := s.deadlineSet + s.mu.Unlock() + if hasDeadline { + return 0, os.ErrDeadlineExceeded + } + return len(p), nil +} + +func TestWSProxyWriteDeadline(t *testing.T) { + c1, s1 := net.Pipe() + defer c1.Close() + defer s1.Close() + tracked := &stallTrackConn{Conn: c1} + + srv, err := New(Options{ + Endpoints: []Endpoint{{Name: "stall", Address: "127.0.0.1:22"}}, + DialContext: func(ctx context.Context, network, address string) (net.Conn, error) { + return tracked, nil + }, + Logger: log.New(io.Discard, "", 0), + }) + if err != nil { + t.Fatalf("New: %v", err) + } + ts := httptest.NewServer(srv.Handler()) + defer ts.Close() + + wsURL := "ws" + strings.TrimPrefix(ts.URL, "http") + "/_ws/stall" + wsConn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("Dial: %v", err) + } + defer wsConn.Close() + + if err := wsConn.WriteMessage(websocket.BinaryMessage, []byte("ping")); err != nil { + t.Fatalf("WriteMessage: %v", err) + } + _ = wsConn.SetReadDeadline(time.Now().Add(150 * time.Millisecond)) + _, _, readErr := wsConn.ReadMessage() + tracked.mu.Lock() + dlSet := tracked.deadlineSet + tracked.mu.Unlock() + if !dlSet { + t.Fatal("expected WriteDeadline to be set on upstream TCP connection") + } + if readErr == nil { + t.Fatal("expected proxyConn to close connection when upstream TCP write times out") + } +} From 560374c3253c43f2123cf58009d4f5cabd02789d Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Sun, 25 Oct 2026 07:45:23 -0700 Subject: [PATCH 2/5] Upgrade pkg/sftp to v1.13.12 and fix term/shellwords edge cases --- CHANGELOG.md | 5 +- go/go.mod | 2 +- go/go.sum | 4 +- go/internal/app/autocomplete_test.go | 16 ++ go/internal/app/sftp.go | 2 +- go/internal/depfix/README.md | 5 +- go/internal/depfix/depfix.go | 23 +-- go/internal/depfix/sftp.go | 174 +++------------------- go/internal/depfix/sftp_test.go | 42 +----- go/internal/depfix/term.go | 18 ++- go/internal/depfix/term_test.go | 25 +++- go/internal/shellwords/shellwords.go | 1 + go/internal/shellwords/shellwords_test.go | 2 + 13 files changed, 102 insertions(+), 217 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dde7bfe..e616ca7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,8 @@ ### :lock: Security fixes * Align `go/go.mod` (`go 1.27.3`) and `go/build.sh` (`GOTOOLCHAIN`) with `.goversion` so local builds do not fall back to Go 1.26.0, and remove stale `docroot/xterm.js` from the embedded server docroot. -* Harden SFTP client (`github.com/pkg/sftp`) against malicious or malformed server responses: - * Validate incoming SFTP packet framing and field bounds before `pkg/sftp` unmarshals them, and add panic recovery to SFTP background goroutines (`sftp get`, drag-and-drop uploads, `Getwd`, and tab completion). - * Reject `SSH_FXP_READDIR` filenames that contain `/` or resolve to `.` or `..` after `path.Base`, and use `Lstat` in recursive `rm -R` (`RemoveAll`) so symbolic links to directories are not followed. +* Upgrade `github.com/pkg/sftp` to `v1.13.12` (fixing truncated packet unmarshaling panics and `Client.RemoveAll` directory symlink traversal) and harden the SFTP client against remaining malicious or malformed server responses: + * Reject `SSH_FXP_READDIR` filenames that contain `/` or resolve to `.` or `..` after `path.Base`, and add panic recovery to SFTP background goroutines (`sftp get`, drag-and-drop uploads, `Getwd`, and tab completion). * Reject 0-byte `SSH_FXP_DATA` packets (preventing an infinite read loop) and bound the number of entries accumulated in `ReadDirContext`. * Harden SSH client and forwarded agent (`golang.org/x/crypto/ssh`): * Bound concurrent `"auth-agent@openssh.com"` channels, cap incoming agent request frames to 256 KiB before allocation, and reject disallowed request types (such as `SSH2_AGENTC_ADD_IDENTITY`) before `agent.ServeAgent` parses attacker-supplied keys. diff --git a/go/go.mod b/go/go.mod index 10c0b3e..7a3b0e6 100644 --- a/go/go.mod +++ b/go/go.mod @@ -7,7 +7,7 @@ require ( github.com/chromedp/chromedp v0.14.2 github.com/fxamacker/cbor/v2 v2.9.6 github.com/gorilla/websocket v1.5.3 - github.com/pkg/sftp v1.13.11 + github.com/pkg/sftp v1.13.12 github.com/urfave/cli/v2 v2.27.7 golang.org/x/crypto v0.58.0 golang.org/x/term v0.47.0 diff --git a/go/go.sum b/go/go.sum index fe79dff..7082aae 100644 --- a/go/go.sum +++ b/go/go.sum @@ -26,8 +26,8 @@ github.com/ledongthuc/pdf v0.0.0-20220302134840-0c2507a12d80 h1:6Yzfa6GP0rIo/kUL github.com/ledongthuc/pdf v0.0.0-20220302134840-0c2507a12d80/go.mod h1:imJHygn/1yfhB7XSJJKlFZKl/J+dCPAknuiaGOshXAs= github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde h1:x0TT0RDC7UhAVbbWWBzr41ElhJx5tXPWkIHA2HWPRuw= github.com/orisano/pixelmatch v0.0.0-20220722002657-fb0b55479cde/go.mod h1:nZgzbfBr3hhjoZnS66nKrHmduYNpc34ny7RK4z5/HM0= -github.com/pkg/sftp v1.13.11 h1:0N92SLTB8JqASJB14ZLHHzFnBV8mG9zw4K7jghEFWuE= -github.com/pkg/sftp v1.13.11/go.mod h1:uNkH9roSXglNJqM+glJJi+TQXQUm0fXFWqCFmT8hsN0= +github.com/pkg/sftp v1.13.12 h1:Myhzy/ZzLPnCtE2F2WtDHsgs8RfgMnH2gfbHuCJhkMU= +github.com/pkg/sftp v1.13.12/go.mod h1:uNkH9roSXglNJqM+glJJi+TQXQUm0fXFWqCFmT8hsN0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/russross/blackfriday/v2 v2.1.0 h1:JIOH55/0cWyOuilr9/qlrm0BSXldqnqwMsf35Ld67mk= diff --git a/go/internal/app/autocomplete_test.go b/go/internal/app/autocomplete_test.go index 4d0ffa6..78c7cb8 100644 --- a/go/internal/app/autocomplete_test.go +++ b/go/internal/app/autocomplete_test.go @@ -134,6 +134,12 @@ func TestAutoCompleteTrailingBackslash(t *testing.T) { ac := &autoCompleter{ cmds: []*cli.App{ {Name: `al\pha`}, + { + Name: `\cmd`, + Commands: []*cli.Command{ + {Name: `\sub`}, + }, + }, }, } @@ -141,4 +147,14 @@ func TestAutoCompleteTrailingBackslash(t *testing.T) { if !ok || nl != `"al\\pha" ` || np != len(nl) { t.Fatalf("autoComplete(\"al\\\\\", 3, '\\t') = %q, %d, %v; want %q, %d, true", nl, np, ok, `"al\\pha" `, len(`"al\\pha" `)) } + + nl, np, _, ok = ac.autoComplete(`\`, 1, '\t') + if !ok || nl != `"\\cmd" ` || np != len(nl) { + t.Fatalf("autoComplete(\"\\\\\", 1, '\\t') = %q, %d, %v; want %q, %d, true", nl, np, ok, `"\\cmd" `, len(`"\\cmd" `)) + } + + nl, np, _, ok = ac.autoComplete(`"\\cmd" \`, 9, '\t') + if !ok || nl != `"\\cmd" "\\sub" ` || np != len(nl) { + t.Fatalf("autoComplete(\"\\\"\\\\\\\\cmd\\\" \\\\\", 9, '\\t') = %q, %d, %v; want %q, %d, true", nl, np, ok, `"\\cmd" "\\sub" `, len(`"\\cmd" "\\sub" `)) + } } diff --git a/go/internal/app/sftp.go b/go/internal/app/sftp.go index 4593ea8..b4193f6 100644 --- a/go/internal/app/sftp.go +++ b/go/internal/app/sftp.go @@ -312,7 +312,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e for _, dir := range args { var err error if ctx.Bool("R") { - err = depfix.RemoveAll(client, joinPath(cwd, dir)) + err = client.RemoveAll(joinPath(cwd, dir)) } else { err = client.Remove(joinPath(cwd, dir)) } diff --git a/go/internal/depfix/README.md b/go/internal/depfix/README.md index 5a1862b..7dbfeb6 100644 --- a/go/internal/depfix/README.md +++ b/go/internal/depfix/README.md @@ -21,9 +21,8 @@ This package and the `workarounds` object in [`docroot/ssh.mjs`](../../../docroo | Flag / Workaround | Dependency | Vulnerability Remediated | Call Site(s) | | :--- | :--- | :--- | :--- | -| `EnableSFTPPacketGuard` | `github.com/pkg/sftp@v1.13.11` | Out-of-bounds slice panics in `unmarshalStatus`, `RealPath`, `ReadLink`, `ReadDirContext`, `opendir`, `open`, `readChunkAt`, and `readAt` on truncated/malformed packets | [`go/internal/app/sftp.go`](../app/sftp.go) | -| `EnableSFTPPathAndRemoveGuard` | `github.com/pkg/sftp@v1.13.11` | `ReadDirContext` path-sanitization bypass (`"/.."` -> `".."` via `path.Base`) and `Client.RemoveAll` following directory symlinks via `Stat` | [`go/internal/app/sftp.go`](../app/sftp.go) | -| `EnableSFTPResourceGuard` | `github.com/pkg/sftp@v1.13.11` | Infinite `SSH_FXP_READ` loop on 0-byte `SSH_FXP_DATA` packets and unbounded `ReadDirContext` entry accumulation | [`go/internal/app/sftp.go`](../app/sftp.go) | +| `EnableSFTPPathAndRemoveGuard` | `github.com/pkg/sftp@v1.13.12` | `ReadDirContext` path-sanitization bypass (`"/.."` -> `".."` via `path.Base`) | [`go/internal/app/sftp.go`](../app/sftp.go) | +| `EnableSFTPResourceGuard` | `github.com/pkg/sftp@v1.13.12` | Infinite `SSH_FXP_READ` loop on 0-byte `SSH_FXP_DATA` packets and unbounded `ReadDirContext` entry accumulation | [`go/internal/app/sftp.go`](../app/sftp.go) | | `EnableSSHAgentGuard` | `golang.org/x/crypto@v0.58.0` (`ssh/agent`) | Unbounded concurrent `"auth-agent@openssh.com"` channels, 16 MiB unread pre-allocation per channel in `ServeAgent`, and CPU exhaustion in `parseRSAKey` on disallowed `AddIdentity` requests | [`go/internal/app/ssh.go`](../app/ssh.go) | | `EnableRSASHA2` | `golang.org/x/crypto@v0.58.0` (`ssh`, `ssh/agent`) | Downgrade to deprecated SHA-1 (`ssh-rsa`) when `dynSigner` omits `ssh.MultiAlgorithmSigner` or `forwardedAgent`/`keyRing` omits `agent.ExtendedAgent` | [`go/internal/app/keys.go`](../app/keys.go), [`go/internal/app/agent.go`](../app/agent.go) | | `EnableSSHSessionExitGuard` | `golang.org/x/crypto@v0.58.0` (`ssh`) | Multiplexer (`mux.loop()`) and `(*Session).Wait()` deadlock when a server sends a malformed `"exit-status"` or `"exit-signal"` request followed by >16 channel requests | [`go/internal/app/ssh.go`](../app/ssh.go) | diff --git a/go/internal/depfix/depfix.go b/go/internal/depfix/depfix.go index 3e743d6..ea26c37 100644 --- a/go/internal/depfix/depfix.go +++ b/go/internal/depfix/depfix.go @@ -27,27 +27,18 @@ package depfix var ( - // EnableSFTPPacketGuard validates incoming SFTP packet frames before - // github.com/pkg/sftp (v1.13.11) unmarshals them with unchecked slice - // bounds in unmarshalStatus, RealPath, ReadLink, ReadDirContext, - // opendir, open, readChunkAt, and readAt. - // Remove when github.com/pkg/sftp replaces unmarshalUint32 and - // unmarshalString with safe bounds-checked variants across client.go - // and packet.go. - EnableSFTPPacketGuard = true - // EnableSFTPPathAndRemoveGuard rejects SSH_FXP_READDIR filenames that - // contain '/' or resolve to '.' or '..' after path.Base, and uses Lstat - // instead of Stat in RemoveAll so directory symlinks are not followed. - // Remove when github.com/pkg/sftp validates ReadDirContext filenames - // and switches Client.RemoveAll to Lstat. + // contain '/' or resolve to '.' or '..' after path.Base. + // Remove when github.com/pkg/sftp (v1.13.12) validates ReadDirContext + // filenames before passing them to path.Base. EnableSFTPPathAndRemoveGuard = true // EnableSFTPResourceGuard rejects 0-byte SSH_FXP_DATA packets (which - // cause an infinite read loop in File.readChunkAt) and caps cumulative + // cause an infinite read loop in File.readChunkAt), 0-count + // SSH_FXP_NAME responses to SSH_FXP_READDIR, and caps cumulative // SSH_FXP_READDIR entries per directory listing. - // Remove when github.com/pkg/sftp handles 0-byte SSH_FXP_DATA and - // bounds ReadDirContext. + // Remove when github.com/pkg/sftp (v1.13.12) handles 0-byte + // SSH_FXP_DATA and bounds ReadDirContext. EnableSFTPResourceGuard = true // EnableSSHAgentGuard bounds concurrent "auth-agent@openssh.com" diff --git a/go/internal/depfix/sftp.go b/go/internal/depfix/sftp.go index 4b3b3fc..ca8b5aa 100644 --- a/go/internal/depfix/sftp.go +++ b/go/internal/depfix/sftp.go @@ -29,7 +29,6 @@ import ( "errors" "fmt" "io" - "os" "path" "strings" "sync" @@ -39,17 +38,15 @@ import ( ) const ( - sshFxpInit = 1 - sshFxpVersion = 2 - sshFxpClose = 4 - sshFxpOpendir = 11 - sshFxpReaddir = 12 - sshFxpStatus = 101 - sshFxpHandle = 102 - sshFxpData = 103 - sshFxpName = 104 - sshFxpAttrs = 105 - sshFxpExtendedReply = 201 + sshFxpInit = 1 + sshFxpVersion = 2 + sshFxpClose = 4 + sshFxpOpendir = 11 + sshFxpReaddir = 12 + sshFxpStatus = 101 + sshFxpHandle = 102 + sshFxpData = 103 + sshFxpName = 104 sshFileXferAttrSize = 0x00000001 sshFileXferAttrUIDGID = 0x00000002 @@ -72,16 +69,10 @@ var ( ErrTooManySFTPEntries = errors.New("sftp: directory listing exceeds maximum entry limit") ) -// NewSFTPClient creates a new *sftp.Client over conn with packet and resource +// NewSFTPClient creates a new *sftp.Client over conn with path and resource // guards applied when enabled. func NewSFTPClient(conn *ssh.Client, opts ...sftp.ClientOption) (*sftp.Client, error) { - // Disable concurrent reads by default when packet/resource guards are - // enabled so sftp.File.Read does not spawn unrecovered background - // worker goroutines on large reads. - if EnableSFTPPacketGuard { - opts = append([]sftp.ClientOption{sftp.UseConcurrentReads(false)}, opts...) - } - if !EnableSFTPPacketGuard && !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { + if !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { return sftp.NewClient(conn, opts...) } s, err := conn.NewSession() @@ -111,13 +102,10 @@ func NewSFTPClient(conn *ssh.Client, opts ...sftp.ClientOption) (*sftp.Client, e return client, nil } -// NewSFTPClientPipe creates a new *sftp.Client over rd and wr with packet and +// NewSFTPClientPipe creates a new *sftp.Client over rd and wr with path and // resource guards applied when enabled. func NewSFTPClientPipe(rd io.Reader, wr io.WriteCloser, opts ...sftp.ClientOption) (*sftp.Client, error) { - if EnableSFTPPacketGuard { - opts = append([]sftp.ClientOption{sftp.UseConcurrentReads(false)}, opts...) - } - if !EnableSFTPPacketGuard && !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { + if !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { return sftp.NewClientPipe(rd, wr, opts...) } sc := newSFTPSafeConn(rd, wr) @@ -253,30 +241,7 @@ func (c *sftpSafeConn) addReaddirEntries(n int) error { } func (c *sftpSafeConn) validatePacket(pktType byte, data []byte) error { - if pktType == sshFxpVersion { - if !EnableSFTPPacketGuard { - return nil - } - if len(data) < 4 { - return fmt.Errorf("%w: truncated SSH_FXP_VERSION", ErrMalformedSFTPPacket) - } - rest := data[4:] - for len(rest) > 0 { - var err error - if _, rest, err = readStringSafe(rest); err != nil { - return err - } - if _, rest, err = readStringSafe(rest); err != nil { - return err - } - } - return nil - } - if len(data) < 4 { - if EnableSFTPPacketGuard { - return fmt.Errorf("%w: packet shorter than 4-byte request id", ErrMalformedSFTPPacket) - } return nil } sid := binary.BigEndian.Uint32(data[:4]) @@ -285,123 +250,66 @@ func (c *sftpSafeConn) validatePacket(pktType byte, data []byte) error { switch pktType { case sshFxpStatus: c.isReaddirSID(sid, true) - if !EnableSFTPPacketGuard { - return nil - } - // unmarshalStatus calls unmarshalUint32(data) for code without bounds checks. - if len(rest) < 4 { - return fmt.Errorf("%w: truncated SSH_FXP_STATUS", ErrMalformedSFTPPacket) - } - rest = rest[4:] - if len(rest) > 0 { - var err error - if _, rest, err = readStringSafe(rest); err != nil { - return err - } - if len(rest) > 0 { - if _, _, err = readStringSafe(rest); err != nil { - return err - } - } - } - return nil - - case sshFxpHandle: - if !EnableSFTPPacketGuard { - return nil - } - if _, _, err := readStringSafe(rest); err != nil { - return fmt.Errorf("%w: invalid SSH_FXP_HANDLE", ErrMalformedSFTPPacket) - } return nil case sshFxpData: - if !EnableSFTPPacketGuard && !EnableSFTPResourceGuard { + if !EnableSFTPResourceGuard { return nil } d, _, err := readStringSafe(rest) if err != nil { - if EnableSFTPPacketGuard { - return fmt.Errorf("%w: invalid SSH_FXP_DATA", ErrMalformedSFTPPacket) - } return nil } - if EnableSFTPResourceGuard && len(d) == 0 { + if len(d) == 0 { return ErrZeroLengthSFTPData } return nil case sshFxpName: isReaddir := c.isReaddirSID(sid, true) - if !EnableSFTPPacketGuard && !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { + if !isReaddir || (!EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard) { return nil } if len(rest) < 4 { - if EnableSFTPPacketGuard { - return fmt.Errorf("%w: truncated SSH_FXP_NAME count", ErrMalformedSFTPPacket) - } return nil } count := binary.BigEndian.Uint32(rest[:4]) rest = rest[4:] - if EnableSFTPPacketGuard && count == 0 { + if EnableSFTPResourceGuard && count == 0 { return fmt.Errorf("%w: zero count in SSH_FXP_NAME", ErrMalformedSFTPPacket) } // Each entry requires at least 12 bytes (filename len + longname len + attr flags). if count > uint32(len(rest)/12) { - if EnableSFTPPacketGuard { - return fmt.Errorf("%w: SSH_FXP_NAME count %d exceeds payload", ErrMalformedSFTPPacket, count) - } return nil } - if isReaddir && EnableSFTPResourceGuard { + if EnableSFTPResourceGuard { if err := c.addReaddirEntries(int(count)); err != nil { return err } } + if !EnableSFTPPathAndRemoveGuard { + return nil + } for i := uint32(0); i < count; i++ { filename, r, err := readStringSafe(rest) if err != nil { - if EnableSFTPPacketGuard { - return err - } return nil } - if isReaddir && EnableSFTPPathAndRemoveGuard { - if err := validateReaddirFilename(filename); err != nil { - return err - } + if err := validateReaddirFilename(filename); err != nil { + return err } _, r, err = readStringSafe(r) if err != nil { - if EnableSFTPPacketGuard { - return err - } return nil } r, err = skipAttrsSafe(r) if err != nil { - if EnableSFTPPacketGuard { - return err - } return nil } rest = r } return nil - case sshFxpAttrs: - if !EnableSFTPPacketGuard { - return nil - } - if _, err := skipAttrsSafe(rest); err != nil { - return err - } - return nil - - case sshFxpExtendedReply: - return nil - default: return nil } @@ -484,39 +392,3 @@ func skipAttrsSafe(b []byte) ([]byte, error) { } return b, nil } - -// RemoveAll recursively removes p on client. When EnableSFTPPathAndRemoveGuard -// is true, it uses Lstat instead of Stat so symbolic links to directories are -// removed as links rather than followed into the target directory. -func RemoveAll(client *sftp.Client, p string) error { - if !EnableSFTPPathAndRemoveGuard { - return client.RemoveAll(p) - } - info, err := client.Lstat(p) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return nil - } - return err - } - if !info.IsDir() { - return client.Remove(p) - } - entries, err := client.ReadDir(p) - if err != nil { - return err - } - for _, entry := range entries { - name := entry.Name() - if err := validateReaddirFilename(name); err != nil { - return err - } - if name == "." || name == ".." { - continue - } - if err := RemoveAll(client, path.Join(p, name)); err != nil { - return err - } - } - return client.RemoveDirectory(p) -} diff --git a/go/internal/depfix/sftp_test.go b/go/internal/depfix/sftp_test.go index a53b92a..e7623b2 100644 --- a/go/internal/depfix/sftp_test.go +++ b/go/internal/depfix/sftp_test.go @@ -157,30 +157,6 @@ func TestSFTPPacketGuard(t *testing.T) { } for _, tc := range cases { - t.Run(tc.name+"/VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableSFTPPacketGuard, func() { - WithDisabled(&EnableSFTPPathAndRemoveGuard, func() { - WithDisabled(&EnableSFTPResourceGuard, func() { - client := runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { - tc.respond(sid, wr) - }) - panicked := false - func() { - defer func() { - if r := recover(); r != nil { - panicked = true - } - }() - _ = tc.invoke(client) - }() - if !panicked { - t.Fatalf("expected github.com/pkg/sftp to panic on %s when EnableSFTPPacketGuard is disabled; if fixed upstream, remove EnableSFTPPacketGuard", tc.name) - } - }) - }) - }) - }) - t.Run(tc.name+"/RemediationEnabled", func(t *testing.T) { client := runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { tc.respond(sid, wr) @@ -189,13 +165,13 @@ func TestSFTPPacketGuard(t *testing.T) { func() { defer func() { if r := recover(); r != nil { - t.Fatalf("unexpected panic with EnableSFTPPacketGuard enabled: %v", r) + t.Fatalf("unexpected panic: %v", r) } }() err = tc.invoke(client) }() if err == nil { - t.Fatalf("expected error on %s with EnableSFTPPacketGuard enabled, got nil", tc.name) + t.Fatalf("expected error on %s, got nil", tc.name) } }) } @@ -265,7 +241,7 @@ func TestSFTPPathAndRemoveGuard(t *testing.T) { } }) - // 2. Verify RemoveAll symlink following against a real local sftp.Server. + // 2. Regression test: RemoveAll symlink handling against a real local sftp.Server. setupSymlinkTree := func(t *testing.T) (string, string, string, *sftp.Client) { t.Helper() tmpDir := t.TempDir() @@ -304,19 +280,9 @@ func TestSFTPPathAndRemoveGuard(t *testing.T) { return linkPath, secretFile, targetDir, client } - t.Run("RemoveAllSymlink/VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableSFTPPathAndRemoveGuard, func() { - linkPath, secretFile, _, client := setupSymlinkTree(t) - _ = RemoveAll(client, linkPath) - if _, err := os.Stat(secretFile); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("expected pkg/sftp Client.RemoveAll to follow symlink and delete %s; if fixed upstream, remove RemoveAll wrapper", secretFile) - } - }) - }) - t.Run("RemoveAllSymlink/RemediationEnabled", func(t *testing.T) { linkPath, secretFile, _, client := setupSymlinkTree(t) - if err := RemoveAll(client, linkPath); err != nil { + if err := client.RemoveAll(linkPath); err != nil { t.Fatalf("RemoveAll: %v", err) } if _, err := os.Lstat(linkPath); !errors.Is(err, os.ErrNotExist) { diff --git a/go/internal/depfix/term.go b/go/internal/depfix/term.go index b738511..a29b738 100644 --- a/go/internal/depfix/term.go +++ b/go/internal/depfix/term.go @@ -46,6 +46,7 @@ func WrapTermReadWriter(rw io.ReadWriter) io.ReadWriter { type termSafeReadWriter struct { rw io.ReadWriter outBuf bytes.Buffer + err error inEsc bool escLen int } @@ -63,7 +64,18 @@ func (t *termSafeReadWriter) Read(p []byte) (int, error) { return t.rw.Read(p) } if t.outBuf.Len() > 0 { - return t.outBuf.Read(p) + nn, _ := t.outBuf.Read(p) + if t.outBuf.Len() == 0 && t.err != nil { + err := t.err + t.err = nil + return nn, err + } + return nn, nil + } + if t.err != nil { + err := t.err + t.err = nil + return 0, err } raw := make([]byte, max(len(p), 64)) n, err := t.rw.Read(raw) @@ -114,6 +126,10 @@ func (t *termSafeReadWriter) Read(p []byte) (int, error) { } if t.outBuf.Len() > 0 { nn, _ := t.outBuf.Read(p) + if t.outBuf.Len() > 0 { + t.err = err + return nn, nil + } return nn, err } return 0, err diff --git a/go/internal/depfix/term_test.go b/go/internal/depfix/term_test.go index e90edd6..6919072 100644 --- a/go/internal/depfix/term_test.go +++ b/go/internal/depfix/term_test.go @@ -36,6 +36,7 @@ import ( type trackingTermIO struct { in *bytes.Reader + eofWithData bool zeroReadCalls atomic.Int32 } @@ -46,7 +47,11 @@ func (t *trackingTermIO) Read(p []byte) (int, error) { } return 0, nil } - return t.in.Read(p) + n, err := t.in.Read(p) + if t.eofWithData && err == nil && t.in.Len() == 0 { + err = io.EOF + } + return n, err } func (t *trackingTermIO) Write(p []byte) (int, error) { @@ -83,4 +88,22 @@ func TestTermEscapeGuard(t *testing.T) { t.Fatalf("ReadLine() = %q, want %q", line, "hello") } }) + + t.Run("SimultaneousDataAndEOF", func(t *testing.T) { + // 256-byte input where injected chunk terminators expand outBuf past + // 256 bytes while the underlying reader returns (256, io.EOF). + eofInput := "\x1b[" + strings.Repeat("0", 247) + "mhello\n" + tio := &trackingTermIO{ + in: bytes.NewReader([]byte(eofInput)), + eofWithData: true, + } + vt := term.NewTerminal(WrapTermReadWriter(tio), "> ") + line, err := vt.ReadLine() + if err != nil && !errors.Is(err, io.EOF) { + t.Fatalf("ReadLine: %v", err) + } + if line != "hello" { + t.Fatalf("ReadLine() = %q, want %q", line, "hello") + } + }) } diff --git a/go/internal/shellwords/shellwords.go b/go/internal/shellwords/shellwords.go index be1f2e0..774e9ca 100644 --- a/go/internal/shellwords/shellwords.go +++ b/go/internal/shellwords/shellwords.go @@ -157,6 +157,7 @@ func Parse(line string, opts ...Option) ([]string, []string) { if escaped { buf += "\\" + got = argSingle } if got != argNo { diff --git a/go/internal/shellwords/shellwords_test.go b/go/internal/shellwords/shellwords_test.go index dca8a5b..2b1064b 100644 --- a/go/internal/shellwords/shellwords_test.go +++ b/go/internal/shellwords/shellwords_test.go @@ -72,6 +72,8 @@ func TestSimple(t *testing.T) { {`foo*[a-b]\?`, []string{`foo*[a-b]?`}, []string{`foo*[a-b]\?`}}, {`"foo*[a-b]?"`, []string{`foo*[a-b]?`}, []string{`"foo*[a-b]?"`}}, {`foo\`, []string{`foo\`}, []string{`foo\`}}, + {`\`, []string{`\`}, []string{`\`}}, + {`ls \`, []string{`ls`, `\`}, []string{`ls`, `\`}}, } for _, testcase := range testcases { From 1ba0d0f0a90a466bb5c168be67d4e0204ea45104 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Sun, 25 Oct 2026 08:24:26 -0700 Subject: [PATCH 3/5] Remove non-exploitable dependency workarounds and simplify remediations --- CHANGELOG.md | 14 +- docroot/ssh.mjs | 39 --- go/internal/app/agent.go | 10 +- go/internal/app/cert_util.go | 8 +- go/internal/app/cert_util_test.go | 42 +++ go/internal/app/keys.go | 7 - go/internal/app/sftp.go | 44 +--- go/internal/app/ssh.go | 2 +- go/internal/depfix/README.md | 56 ++-- go/internal/depfix/depfix.go | 36 +-- go/internal/depfix/sftp.go | 140 +--------- go/internal/depfix/sftp_test.go | 149 +++-------- go/internal/depfix/ssh.go | 248 ++---------------- go/internal/depfix/ssh_test.go | 348 +------------------------ go/internal/depfix/term.go | 136 ---------- go/internal/depfix/term_test.go | 109 -------- go/internal/jsutil/streams.go | 11 +- go/internal/terminal/terminal.go | 3 +- go/internal/tests/remediations_test.go | 104 +------- 19 files changed, 175 insertions(+), 1331 deletions(-) delete mode 100644 go/internal/depfix/term.go delete mode 100644 go/internal/depfix/term_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e616ca7..a04cb3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,20 +5,16 @@ ### :lock: Security fixes * Align `go/go.mod` (`go 1.27.3`) and `go/build.sh` (`GOTOOLCHAIN`) with `.goversion` so local builds do not fall back to Go 1.26.0, and remove stale `docroot/xterm.js` from the embedded server docroot. -* Upgrade `github.com/pkg/sftp` to `v1.13.12` (fixing truncated packet unmarshaling panics and `Client.RemoveAll` directory symlink traversal) and harden the SFTP client against remaining malicious or malformed server responses: - * Reject `SSH_FXP_READDIR` filenames that contain `/` or resolve to `.` or `..` after `path.Base`, and add panic recovery to SFTP background goroutines (`sftp get`, drag-and-drop uploads, `Getwd`, and tab completion). - * Reject 0-byte `SSH_FXP_DATA` packets (preventing an infinite read loop) and bound the number of entries accumulated in `ReadDirContext`. +* Upgrade `github.com/pkg/sftp` to `v1.13.12` (fixing truncated packet unmarshaling panics and `Client.RemoveAll` directory symlink traversal), reject 0-byte `SSH_FXP_DATA` packets (preventing an infinite read loop), and bound the number of entries accumulated in `ReadDirContext`. * Harden SSH client and forwarded agent (`golang.org/x/crypto/ssh`): - * Bound concurrent `"auth-agent@openssh.com"` channels, cap incoming agent request frames to 256 KiB before allocation, and reject disallowed request types (such as `SSH2_AGENTC_ADD_IDENTITY`) before `agent.ServeAgent` parses attacker-supplied keys. + * Bound concurrent `"auth-agent@openssh.com"` channels and cap incoming agent request frames to 256 KiB before `agent.ServeAgent` allocates up to 16 MiB per channel. * Implement `ssh.MultiAlgorithmSigner` on `dynSigner` and `agent.ExtendedAgent` (`SignWithFlags`) on `keyRing` and `forwardedAgent` so RSA keys and user certificates use `rsa-sha2-256` / `rsa-sha2-512` instead of falling back to deprecated SHA-1 (`ssh-rsa`). - * Close the session channel and drain remaining channel requests when a remote server sends a malformed `"exit-status"` or `"exit-signal"` request, preventing an SSH connection multiplexer deadlock. - * Reject deprecated SHA-1 (`ssh-rsa`) and DSA (`ssh-dss`) CA signatures in `checkCertificate` and support `no-touch-required` security-key (`sk-*`) CA signatures. -* Prevent `golang.org/x/term` `Terminal.ReadLine` from entering an infinite synchronous 0-byte `Read` loop when receiving an unterminated escape sequence longer than 256 bytes. + * Reject deprecated SHA-1 (`ssh-rsa`) and DSA (`ssh-dss`) CA signatures in `checkCertificate`. * Fix a trailing-backslash parsing bug in `shellwords.Parse` that could cause a slice-bounds panic during tab auto-completion. * Set upstream TCP write deadlines and WebSocket `HandshakeTimeout` in `sshterm-server`. * Harden `@xterm/xterm` terminal handling in `docroot/ssh.mjs`: - * Clamp `CSI Ps b` (`REP`) and `CSI I`, `Z`, `L`, `M`, `S`, `T` parameters to terminal dimensions to prevent `RangeError` crashes (which wedged `WriteBuffer` and deadlocked WASM writes) and $2^{31}-1$ synchronous main-thread loops. - * Cap chained `OSC 4` color queries and `OSC 8` hyperlink markers, sanitize and truncate `OSC 0`/`OSC 2` window titles while reusing a single toast element in `_showMessage`, and reset terminal modes and pending input buffers when an SSH or SFTP session exits. + * Clamp `CSI Ps b` (`REP`) and `CSI I`, `Z`, `L`, `M`, `S`, `T` parameters to terminal dimensions to prevent `RangeError` crashes (which wedged `WriteBuffer` and deadlocked WASM writes) and `2^31 - 1` synchronous main-thread loops. + * Sanitize and truncate `OSC 0`/`OSC 2` window titles while reusing a single toast element in `_showMessage`, and reset terminal modes and pending input buffers when an SSH or SFTP session exits. ## v0.11.5 diff --git a/docroot/ssh.mjs b/docroot/ssh.mjs index f4d99be..7b5c6d5 100644 --- a/docroot/ssh.mjs +++ b/docroot/ssh.mjs @@ -39,10 +39,6 @@ export const workarounds = { // Protects @xterm/xterm (5.5.0) InputHandler from 2^31-1 synchronous // while(param--) loops in CSI I, Z, L, M, S, and T. csiLoopGuard: true, - - // Protects @xterm/xterm (5.5.0) from amplified OSC 4 color query floods - // and unbounded OSC 8 hyperlink Marker/listener retention. - oscQueryAndLinkGuard: true, }; export function sanitizeTitle(title) { @@ -86,41 +82,6 @@ export function applyXtermWorkarounds(term, flags = workarounds) { registerClampCsi('S', () => term.rows); registerClampCsi('T', () => term.rows); - disposables.push( - term.parser.registerOscHandler(4, data => { - if (!flags.oscQueryAndLinkGuard) return false; - let queries = 0; - for (let i = 0; i < data.length; i++) { - if (data.charCodeAt(i) === 63 /* '?' */) { - queries++; - if (queries > 16) return true; - } - } - return false; - }) - ); - - let lastLinkLine = -1; - let linksOnLine = 0; - disposables.push( - term.parser.registerOscHandler(8, data => { - if (!flags.oscQueryAndLinkGuard) return false; - if (data === ';') return false; - const buf = term.buffer?.active; - const line = buf ? buf.baseY + buf.cursorY : 0; - if (line !== lastLinkLine) { - lastLinkLine = line; - linksOnLine = 0; - } - linksOnLine++; - const totalMarkers = term._core?.markers ? term._core.markers.length : 0; - if (linksOnLine > 32 || totalMarkers > 256 || data.length > 2048) { - return true; - } - return false; - }) - ); - return disposables; } diff --git a/go/internal/app/agent.go b/go/internal/app/agent.go index d15c983..d4b64c8 100644 --- a/go/internal/app/agent.go +++ b/go/internal/app/agent.go @@ -37,8 +37,6 @@ import ( "github.com/urfave/cli/v2" "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" - - "github.com/c2FmZQ/sshterm/internal/depfix" ) func (a *App) agentCommand() *cli.App { @@ -295,7 +293,7 @@ func (r *keyRing) SignWithFlags(key ssh.PublicKey, data []byte, flags agent.Sign // Don't hold the lock while signing. With WebAuthn keys, Sign waits for // the user to touch the security key. r.mu.Unlock() - if !depfix.EnableRSASHA2 || flags == 0 { + if flags == 0 { return signer.Sign(rand.Reader, data) } as, ok := signer.(ssh.AlgorithmSigner) @@ -500,10 +498,8 @@ func (f *forwardedAgent) SignWithFlags(key ssh.PublicKey, data []byte, flags age } } f.notify("[agent] %s requested a signature with key %q\n", f.host, name) - if depfix.EnableRSASHA2 { - if ea, ok := f.agent.(agent.ExtendedAgent); ok { - return ea.SignWithFlags(key, data, flags) - } + if ea, ok := f.agent.(agent.ExtendedAgent); ok { + return ea.SignWithFlags(key, data, flags) } return f.agent.Sign(key, data) } diff --git a/go/internal/app/cert_util.go b/go/internal/app/cert_util.go index 0968f99..601b0d2 100644 --- a/go/internal/app/cert_util.go +++ b/go/internal/app/cert_util.go @@ -33,7 +33,6 @@ import ( "golang.org/x/crypto/ssh" - "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/termsafe" ) @@ -57,7 +56,12 @@ func checkCertificate(cert *ssh.Certificate, certType uint32) error { slices.Sort(opts) errs = append(errs, fmt.Errorf("certificate has critical options: %v", opts)) } - if err := depfix.VerifyCertificateSignature(cert); err != nil { + c2 := *cert + c2.Signature = nil + signBytes := c2.Marshal() + if cert.Signature == nil || cert.Signature.Format == ssh.KeyAlgoRSA || cert.Signature.Format == ssh.KeyAlgoDSA { + errs = append(errs, fmt.Errorf("certificate signature is invalid")) + } else if err := cert.SignatureKey.Verify(signBytes[:len(signBytes)-4], cert.Signature); err != nil { errs = append(errs, fmt.Errorf("certificate signature is invalid")) } return errors.Join(errs...) diff --git a/go/internal/app/cert_util_test.go b/go/internal/app/cert_util_test.go index b24f249..1a39741 100644 --- a/go/internal/app/cert_util_test.go +++ b/go/internal/app/cert_util_test.go @@ -26,6 +26,8 @@ package app import ( "crypto/ed25519" "crypto/rand" + "crypto/rsa" + "io" "strings" "testing" "time" @@ -87,6 +89,46 @@ func TestCheckCertificate(t *testing.T) { } t.Logf("[#%d] OK checkCertificate(): %v", i, err) } + + // Verify that SHA-1 (ssh-rsa) CA signatures are rejected while RSA-SHA2-512 is accepted. + rsaPriv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa.GenerateKey: %v", err) + } + rsaSigner, err := ssh.NewSignerFromKey(rsaPriv) + if err != nil { + t.Fatalf("ssh.NewSignerFromKey: %v", err) + } + algSigner := rsaSigner.(ssh.AlgorithmSigner) + + sha1Cert := &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: ssh.CertTimeInfinity} + if err := sha1Cert.SignCert(rand.Reader, &fixedAlgoSigner{AlgorithmSigner: algSigner, algo: ssh.KeyAlgoRSA}); err != nil { + t.Fatalf("SignCert(ssh-rsa): %v", err) + } + if err := checkCertificate(sha1Cert, ssh.HostCert); err == nil { + t.Errorf("expected checkCertificate to reject ssh-rsa (SHA-1) CA signature") + } + + sha512Cert := &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: ssh.CertTimeInfinity} + if err := sha512Cert.SignCert(rand.Reader, &fixedAlgoSigner{AlgorithmSigner: algSigner, algo: ssh.KeyAlgoRSASHA512}); err != nil { + t.Fatalf("SignCert(rsa-sha2-512): %v", err) + } + if err := checkCertificate(sha512Cert, ssh.HostCert); err != nil { + t.Errorf("expected checkCertificate to accept rsa-sha2-512 CA signature, got: %v", err) + } +} + +type fixedAlgoSigner struct { + ssh.AlgorithmSigner + algo string +} + +func (s *fixedAlgoSigner) Sign(rand io.Reader, data []byte) (*ssh.Signature, error) { + return s.AlgorithmSigner.SignWithAlgorithm(rand, data, s.algo) +} + +func (s *fixedAlgoSigner) SignWithAlgorithm(rand io.Reader, data []byte, _ string) (*ssh.Signature, error) { + return s.AlgorithmSigner.SignWithAlgorithm(rand, data, s.algo) } func TestCheckHostCertPrincipal(t *testing.T) { diff --git a/go/internal/app/keys.go b/go/internal/app/keys.go index 629a6ac..3614eef 100644 --- a/go/internal/app/keys.go +++ b/go/internal/app/keys.go @@ -48,7 +48,6 @@ import ( "github.com/urfave/cli/v2" "golang.org/x/crypto/ssh" - "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/jsutil" "github.com/c2FmZQ/sshterm/internal/termsafe" "github.com/c2FmZQ/sshterm/internal/webauthnsk" @@ -832,9 +831,6 @@ func (s *dynSigner) PublicKey() ssh.PublicKey { } func (s *dynSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm string) (*ssh.Signature, error) { - if !depfix.EnableRSASHA2 { - return s.Signer.Sign(rand, data) - } if as, ok := s.Signer.(ssh.AlgorithmSigner); ok { return as.SignWithAlgorithm(rand, data, algorithm) } @@ -845,9 +841,6 @@ func (s *dynSigner) SignWithAlgorithm(rand io.Reader, data []byte, algorithm str } func (s *dynSigner) Algorithms() []string { - if !depfix.EnableRSASHA2 { - return []string{s.Signer.PublicKey().Type()} - } if mas, ok := s.Signer.(ssh.MultiAlgorithmSigner); ok { return mas.Algorithms() } diff --git a/go/internal/app/sftp.go b/go/internal/app/sftp.go index b4193f6..f72395f 100644 --- a/go/internal/app/sftp.go +++ b/go/internal/app/sftp.go @@ -109,16 +109,8 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e raw := a.term.Raw() - t := term.NewTerminal(depfix.WrapTermReadWriter(raw), "sftp> ") - var cwd string - jsutil.TryCatch( - func() { - cwd, err = client.Getwd() - }, - func(p any) { - err = fmt.Errorf("panic: %v", p) - }, - ) + t := term.NewTerminal(raw, "sftp> ") + cwd, err := client.Getwd() if err != nil { return fmt.Errorf("getwd: %w", err) } @@ -686,14 +678,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e } t.AutoCompleteCallback = func(line string, pos int, key rune) (newLine string, newPos int, ok bool) { var options []string - jsutil.TryCatch( - func() { - newLine, newPos, options, ok = ac.autoComplete(line, pos, key) - }, - func(err any) { - ok = false - }, - ) + newLine, newPos, options, ok = ac.autoComplete(line, pos, key) if len(options) > 0 { var w int for _, o := range options { @@ -729,21 +714,14 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e event.Call("stopPropagation") files := jsutil.AcceptFileDrop(event) go func() { - jsutil.TryCatch( - func() { - a.term.Printf("\n") - for _, f := range files { - a.term.Printf("%s ", f.Name) - if err := a.sftpUploadFile(client, f, joinPath(cwd, path.Base(f.Name))); err != nil { - a.term.Errorf("drop: %s", termsafe.Text(err.Error())) - return - } - } - }, - func(err any) { - a.term.Errorf("drop: %v", err) - }, - ) + a.term.Printf("\n") + for _, f := range files { + a.term.Printf("%s ", f.Name) + if err := a.sftpUploadFile(client, f, joinPath(cwd, path.Base(f.Name))); err != nil { + a.term.Errorf("drop: %s", termsafe.Text(err.Error())) + return + } + } }() return nil }) diff --git a/go/internal/app/ssh.go b/go/internal/app/ssh.go index 33b164e..11e6956 100644 --- a/go/internal/app/ssh.go +++ b/go/internal/app/ssh.go @@ -343,7 +343,7 @@ func (a *App) sshClientFromConn(ctx context.Context, c net.Conn, username, hostn return nil, err } - return ssh.NewClient(depfix.WrapSSHConn(conn), chans, reqs), nil + return ssh.NewClient(conn, chans, reqs), nil } func (a *App) hostCertificateCallback(hostname string, ep *endpoint, cert *ssh.Certificate) error { diff --git a/go/internal/depfix/README.md b/go/internal/depfix/README.md index 7dbfeb6..0d8af07 100644 --- a/go/internal/depfix/README.md +++ b/go/internal/depfix/README.md @@ -1,42 +1,26 @@ -# Dependency Security Remediations (`depfix` & `ssh.mjs`) +# Dependency Security Remediations (`internal/depfix`) -This package and the `workarounds` object in [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) centralize security remediations for third-party Go and JavaScript dependencies. +This package centralizes workarounds for security issues in third-party dependencies so that: -## How the Mechanism Works +1. **Isolation**: Each workaround is controlled by a boolean flag in [`depfix.go`](depfix.go) (or `workarounds` in [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) for `@xterm/xterm`). +2. **Dual-Mode Verification**: Every workaround has unit/integration tests that run in two subtests: + * `VulnerabilityWithoutRemediation`: Temporarily disables the workaround via `depfix.WithDisabled(&depfix.Enable..., ...)` and asserts that the upstream dependency still exhibits the vulnerable behavior. + * `RemediationEnabled`: Runs with the workaround enabled (the default) and asserts that the vulnerability is blocked. +3. **Automatic Retirement Signal**: When a dependency is upgraded in `go.mod` or `docroot/` and an upstream fix resolves the underlying issue, the corresponding `VulnerabilityWithoutRemediation` subtest will **fail**, signaling that the workaround can be retired. -1. **Feature Flags**: - * In Go ([`depfix.go`](./depfix.go)), each remediation is controlled by an exported boolean flag (defaulting to `true`). - * In JavaScript ([`docroot/ssh.mjs`](../../../docroot/ssh.mjs)), each `@xterm/xterm` remediation is controlled by a property on `window.sshApp.workarounds` (defaulting to `true`). -2. **Dual-Mode Verification Tests**: - * Every remediation has a corresponding test with two subtests: - * `VulnerabilityWithoutRemediation`: Temporarily disables the flag via `depfix.WithDisabled(&Flag, ...)` (or `workarounds. = false`) and asserts that the underlying dependency still exhibits the vulnerability. - * `RemediationEnabled`: Runs with the flag enabled (`true`) and asserts that the remediation prevents the vulnerability. -3. **Removing a Remediation After an Upstream Fix**: - * When a dependency is upgraded in `go/go.mod` or `xterm/package.json` and fixes the issue upstream, the `VulnerabilityWithoutRemediation` subtest will fail with an explicit message indicating which flag/wrapper can now be removed. - * To remove the remediation: - 1. Replace the `depfix.*` wrapper call at the call site with the direct upstream call (or remove the handler registration in `applyXtermWorkarounds`). - 2. Delete the corresponding flag in `depfix.go` / `ssh.mjs` and the `VulnerabilityWithoutRemediation` subtest, keeping the `RemediationEnabled` test as a regression test against the upstream package. +## Active Workarounds -## Remediations Inventory +| Flag | Dependency | Issue | +| :--- | :--- | :--- | +| *(Retired in `v1.13.12`)* | `github.com/pkg/sftp` `v1.13.11` | Unchecked `unmarshalUint32` / `unmarshalString` slice bounds panics and `Client.RemoveAll` directory symlink traversal (`TestSFTPPacketGuard` and `TestSFTPRemoveAllSymlink` retained as regression tests). | +| `EnableSFTPResourceGuard` | `github.com/pkg/sftp` `v1.13.12` | Infinite read loop on 0-byte `SSH_FXP_DATA` in `File.readChunkAt`, infinite loop on 0-count `SSH_FXP_NAME`, and unbounded slice growth in `ReadDirContext`. | +| `EnableSSHAgentGuard` | `golang.org/x/crypto` `v0.58.0` | Unbounded concurrent `"auth-agent@openssh.com"` channels in `agent.ForwardToAgent` and 16 MiB per-channel buffer pre-allocation on a 4-byte length header in `agent.ServeAgent`. | +| `workarounds.csiRepGuard` | `@xterm/xterm` `5.5.0` | `CSI Ps b` (`repeatPrecedingCharacter`) uncaught `RangeError` permanently wedging `WriteBuffer`. | +| `workarounds.csiLoopGuard` | `@xterm/xterm` `5.5.0` | Unbounded $2^{31}-1$ synchronous `while (param--)` loops in `CSI I`, `Z`, `L`, `M`, `S`, and `T`. | -| Flag / Workaround | Dependency | Vulnerability Remediated | Call Site(s) | -| :--- | :--- | :--- | :--- | -| `EnableSFTPPathAndRemoveGuard` | `github.com/pkg/sftp@v1.13.12` | `ReadDirContext` path-sanitization bypass (`"/.."` -> `".."` via `path.Base`) | [`go/internal/app/sftp.go`](../app/sftp.go) | -| `EnableSFTPResourceGuard` | `github.com/pkg/sftp@v1.13.12` | Infinite `SSH_FXP_READ` loop on 0-byte `SSH_FXP_DATA` packets and unbounded `ReadDirContext` entry accumulation | [`go/internal/app/sftp.go`](../app/sftp.go) | -| `EnableSSHAgentGuard` | `golang.org/x/crypto@v0.58.0` (`ssh/agent`) | Unbounded concurrent `"auth-agent@openssh.com"` channels, 16 MiB unread pre-allocation per channel in `ServeAgent`, and CPU exhaustion in `parseRSAKey` on disallowed `AddIdentity` requests | [`go/internal/app/ssh.go`](../app/ssh.go) | -| `EnableRSASHA2` | `golang.org/x/crypto@v0.58.0` (`ssh`, `ssh/agent`) | Downgrade to deprecated SHA-1 (`ssh-rsa`) when `dynSigner` omits `ssh.MultiAlgorithmSigner` or `forwardedAgent`/`keyRing` omits `agent.ExtendedAgent` | [`go/internal/app/keys.go`](../app/keys.go), [`go/internal/app/agent.go`](../app/agent.go) | -| `EnableSSHSessionExitGuard` | `golang.org/x/crypto@v0.58.0` (`ssh`) | Multiplexer (`mux.loop()`) and `(*Session).Wait()` deadlock when a server sends a malformed `"exit-status"` or `"exit-signal"` request followed by >16 channel requests | [`go/internal/app/ssh.go`](../app/ssh.go) | -| `EnableCertSignatureGuard` | `golang.org/x/crypto@v0.58.0` (`ssh`) | Acceptance of deprecated SHA-1 (`ssh-rsa`) / DSA (`ssh-dss`) CA signatures and rejection of `no-touch-required` (`Flags=0`) `sk-*` CA signatures | [`go/internal/app/cert_util.go`](../app/cert_util.go) | -| `EnableTermEscapeGuard` | `golang.org/x/term@v0.47.0` | Synchronous 100% CPU 0-byte `Read` infinite loop when `Terminal.inBuf` (`[256]byte`) fills with an unterminated escape sequence | [`go/internal/terminal/terminal.go`](../terminal/terminal.go), [`go/internal/app/sftp.go`](../app/sftp.go) | -| `workarounds.csiRepGuard` | `@xterm/xterm@5.5.0` | Uncaught `RangeError: Invalid typed array length` in `CSI Ps b` (`repeatPrecedingCharacter`) permanently wedging `WriteBuffer` and deadlocking Go WASM `Write` | [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) | -| `workarounds.csiLoopGuard` | `@xterm/xterm@5.5.0` | Unbounded $2^{31}-1$ synchronous `while (param--)` loops in `CSI I`, `Z`, `L`, `M`, `S`, and `T` freezing the browser main thread | [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) | -| `workarounds.oscQueryAndLinkGuard` | `@xterm/xterm@5.5.0` | `OSC 4` chained color query amplification and unbounded `OSC 8` hyperlink `Marker`/listener retention | [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) | +## Retiring a Workaround After an Upstream Fix -## Documented Residual Risks - -1. **Single-Threaded 32-bit WebAssembly Runtime (`GOOS=js GOARCH=wasm`)**: - * All tabs (`AddScreen`) within the same browser page share a single Go WASM instance with a 32-bit address space (2–4 GiB maximum linear memory) and cooperative goroutine scheduling on a single OS/WebWorker thread. While `depfix` caps per-channel agent frames (`256 KiB`), SFTP packet frames (`256 KiB`), and `ReadDir` entry counts (`100,000`), an authenticated remote server that streams continuous high-bandwidth output across many tabs can still contend for CPU time and memory within that browser tab. -2. **OpenSSH Ed25519 Private Key Public-Half Consistency (`golang.org/x/crypto/ssh`)**: - * In `golang.org/x/crypto/ssh`, `parseOpenSSHPrivateKey` does not verify that the 32-byte public key embedded inside an unencrypted/decrypted `ed25519` private key matches `ed25519.NewKeyFromSeed(priv[:32])[32:]`. Because private keys are only imported explicitly by the local user (`keys import`), this is only relevant if a user imports an intentionally corrupted private key file. -3. **Terminal Emulator Escape Sequence Surface (`@xterm/xterm`)**: - * `applyXtermWorkarounds` intercepts the known unbounded CSI/OSC handlers in `@xterm/xterm@5.5.0`, and `Terminal.ResetSessionState()` resets terminal modes and drains pending `onData` query responses when an SSH or SFTP session exits. However, while an interactive SSH session is actively running, standard terminal auto-responses (such as primary/secondary Device Attributes `CSI c` and cursor position reports `CSI 6n`) remain active as required for terminal compatibility (`vim`, `tmux`, `less`, etc.). +1. Upgrade the dependency and run `go test ./...` (and browser tests if applicable). +2. For any `VulnerabilityWithoutRemediation` test that fails because the upstream library now handles the case safely: + * Remove the `VulnerabilityWithoutRemediation` subtest, keeping the `RemediationEnabled` subtest as a regression test. + * Remove the corresponding `Enable...` flag and wrapper logic from `internal/depfix` (or `docroot/ssh.mjs`) and call the upstream API directly. diff --git a/go/internal/depfix/depfix.go b/go/internal/depfix/depfix.go index ea26c37..5d03f4f 100644 --- a/go/internal/depfix/depfix.go +++ b/go/internal/depfix/depfix.go @@ -27,12 +27,6 @@ package depfix var ( - // EnableSFTPPathAndRemoveGuard rejects SSH_FXP_READDIR filenames that - // contain '/' or resolve to '.' or '..' after path.Base. - // Remove when github.com/pkg/sftp (v1.13.12) validates ReadDirContext - // filenames before passing them to path.Base. - EnableSFTPPathAndRemoveGuard = true - // EnableSFTPResourceGuard rejects 0-byte SSH_FXP_DATA packets (which // cause an infinite read loop in File.readChunkAt), 0-count // SSH_FXP_NAME responses to SSH_FXP_READDIR, and caps cumulative @@ -42,38 +36,12 @@ var ( EnableSFTPResourceGuard = true // EnableSSHAgentGuard bounds concurrent "auth-agent@openssh.com" - // channels, caps incoming agent request frames to 256 KiB before + // channels and caps incoming agent request frames to 256 KiB before // golang.org/x/crypto/ssh/agent (v0.58.0) ServeAgent pre-allocates up - // to 16 MiB per channel, and rejects non-list/sign agent request types - // before ServeAgent parses attacker-supplied private keys. + // to 16 MiB per channel upon reading a 4-byte length header. // Remove when golang.org/x/crypto/ssh/agent bounds channel concurrency // and request pre-allocation. EnableSSHAgentGuard = true - - // EnableRSASHA2 enables ssh.MultiAlgorithmSigner on dynSigner and - // agent.ExtendedAgent on keyRing and forwardedAgent so RSA keys and - // certificates use rsa-sha2-256 / rsa-sha2-512 instead of falling back - // to SHA-1 (ssh-rsa) in golang.org/x/crypto/ssh (v0.58.0). - EnableRSASHA2 = true - - // EnableSSHSessionExitGuard wraps "session" channel request streams so - // malformed "exit-status" or "exit-signal" requests close the session - // channel and drain remaining channel requests instead of deadlocking - // the SSH connection multiplexer in golang.org/x/crypto/ssh (v0.58.0). - // Remove when (*ssh.Session).wait drains reqs and unblocks Wait on - // malformed exit requests. - EnableSSHSessionExitGuard = true - - // EnableCertSignatureGuard rejects deprecated SHA-1 (ssh-rsa) and DSA - // (ssh-dss) CA signatures in checkCertificate and supports security-key - // (sk-*) CA signatures that omit the UserPresence flag. - EnableCertSignatureGuard = true - - // EnableTermEscapeGuard terminates runaway (>64-byte) unterminated - // escape sequences before golang.org/x/term (v0.47.0) fills its fixed - // 256-byte Terminal.inBuf and spins in a synchronous 0-byte Read loop. - // Remove when golang.org/x/term handles full inBuf escape sequences. - EnableTermEscapeGuard = true ) // WithDisabled temporarily sets *flag to false while running fn, then restores diff --git a/go/internal/depfix/sftp.go b/go/internal/depfix/sftp.go index ca8b5aa..3f506c6 100644 --- a/go/internal/depfix/sftp.go +++ b/go/internal/depfix/sftp.go @@ -29,8 +29,6 @@ import ( "errors" "fmt" "io" - "path" - "strings" "sync" "github.com/pkg/sftp" @@ -48,12 +46,6 @@ const ( sshFxpData = 103 sshFxpName = 104 - sshFileXferAttrSize = 0x00000001 - sshFileXferAttrUIDGID = 0x00000002 - sshFileXferAttrPermissions = 0x00000004 - sshFileXferAttrACmodTime = 0x00000008 - sshFileXferAttrExtended = 0x80000000 - maxSFTPPacketSize = 256 * 1024 ) @@ -65,14 +57,13 @@ var ( ErrMalformedSFTPPacket = errors.New("sftp: malformed packet from server") ErrZeroLengthSFTPData = errors.New("sftp: zero-length SSH_FXP_DATA packet") - ErrInvalidSFTPFilename = errors.New("sftp: invalid filename in SSH_FXP_NAME response") ErrTooManySFTPEntries = errors.New("sftp: directory listing exceeds maximum entry limit") ) -// NewSFTPClient creates a new *sftp.Client over conn with path and resource -// guards applied when enabled. +// NewSFTPClient creates a new *sftp.Client over conn with resource guards +// applied when EnableSFTPResourceGuard is enabled. func NewSFTPClient(conn *ssh.Client, opts ...sftp.ClientOption) (*sftp.Client, error) { - if !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { + if !EnableSFTPResourceGuard { return sftp.NewClient(conn, opts...) } s, err := conn.NewSession() @@ -102,10 +93,10 @@ func NewSFTPClient(conn *ssh.Client, opts ...sftp.ClientOption) (*sftp.Client, e return client, nil } -// NewSFTPClientPipe creates a new *sftp.Client over rd and wr with path and -// resource guards applied when enabled. +// NewSFTPClientPipe creates a new *sftp.Client over rd and wr with resource +// guards applied when EnableSFTPResourceGuard is enabled. func NewSFTPClientPipe(rd io.Reader, wr io.WriteCloser, opts ...sftp.ClientOption) (*sftp.Client, error) { - if !EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard { + if !EnableSFTPResourceGuard { return sftp.NewClientPipe(rd, wr, opts...) } sc := newSFTPSafeConn(rd, wr) @@ -256,139 +247,28 @@ func (c *sftpSafeConn) validatePacket(pktType byte, data []byte) error { if !EnableSFTPResourceGuard { return nil } - d, _, err := readStringSafe(rest) - if err != nil { - return nil - } - if len(d) == 0 { + if len(rest) >= 4 && binary.BigEndian.Uint32(rest[:4]) == 0 { return ErrZeroLengthSFTPData } return nil case sshFxpName: isReaddir := c.isReaddirSID(sid, true) - if !isReaddir || (!EnableSFTPPathAndRemoveGuard && !EnableSFTPResourceGuard) { - return nil - } - if len(rest) < 4 { + if !isReaddir || !EnableSFTPResourceGuard || len(rest) < 4 { return nil } count := binary.BigEndian.Uint32(rest[:4]) rest = rest[4:] - if EnableSFTPResourceGuard && count == 0 { + if count == 0 { return fmt.Errorf("%w: zero count in SSH_FXP_NAME", ErrMalformedSFTPPacket) } // Each entry requires at least 12 bytes (filename len + longname len + attr flags). if count > uint32(len(rest)/12) { return nil } - if EnableSFTPResourceGuard { - if err := c.addReaddirEntries(int(count)); err != nil { - return err - } - } - if !EnableSFTPPathAndRemoveGuard { - return nil - } - for i := uint32(0); i < count; i++ { - filename, r, err := readStringSafe(rest) - if err != nil { - return nil - } - if err := validateReaddirFilename(filename); err != nil { - return err - } - _, r, err = readStringSafe(r) - if err != nil { - return nil - } - r, err = skipAttrsSafe(r) - if err != nil { - return nil - } - rest = r - } - return nil + return c.addReaddirEntries(int(count)) default: return nil } } - -func validateReaddirFilename(filename string) error { - if filename == "." || filename == ".." { - return nil - } - if filename == "" || strings.ContainsAny(filename, "/\x00") { - return fmt.Errorf("%w: %q", ErrInvalidSFTPFilename, filename) - } - base := path.Base(filename) - if base == "." || base == ".." || base == "/" || base == "" { - return fmt.Errorf("%w: %q", ErrInvalidSFTPFilename, filename) - } - return nil -} - -func readStringSafe(b []byte) (string, []byte, error) { - if len(b) < 4 { - return "", nil, fmt.Errorf("%w: truncated string length", ErrMalformedSFTPPacket) - } - n := binary.BigEndian.Uint32(b[:4]) - b = b[4:] - if int64(n) > int64(len(b)) { - return "", nil, fmt.Errorf("%w: string length %d exceeds remaining %d bytes", ErrMalformedSFTPPacket, n, len(b)) - } - return string(b[:n]), b[n:], nil -} - -func skipAttrsSafe(b []byte) ([]byte, error) { - if len(b) < 4 { - return nil, fmt.Errorf("%w: truncated attrs flags", ErrMalformedSFTPPacket) - } - flags := binary.BigEndian.Uint32(b[:4]) - b = b[4:] - if flags&sshFileXferAttrSize != 0 { - if len(b) < 8 { - return nil, fmt.Errorf("%w: truncated attr size", ErrMalformedSFTPPacket) - } - b = b[8:] - } - if flags&sshFileXferAttrUIDGID != 0 { - if len(b) < 8 { - return nil, fmt.Errorf("%w: truncated attr uid/gid", ErrMalformedSFTPPacket) - } - b = b[8:] - } - if flags&sshFileXferAttrPermissions != 0 { - if len(b) < 4 { - return nil, fmt.Errorf("%w: truncated attr permissions", ErrMalformedSFTPPacket) - } - b = b[4:] - } - if flags&sshFileXferAttrACmodTime != 0 { - if len(b) < 8 { - return nil, fmt.Errorf("%w: truncated attr acmodtime", ErrMalformedSFTPPacket) - } - b = b[8:] - } - if flags&sshFileXferAttrExtended != 0 { - if len(b) < 4 { - return nil, fmt.Errorf("%w: truncated attr extended count", ErrMalformedSFTPPacket) - } - count := binary.BigEndian.Uint32(b[:4]) - b = b[4:] - if count > uint32(len(b)/8) { - return nil, fmt.Errorf("%w: attr extended count %d exceeds payload", ErrMalformedSFTPPacket, count) - } - for i := uint32(0); i < count; i++ { - var err error - if _, b, err = readStringSafe(b); err != nil { - return nil, err - } - if _, b, err = readStringSafe(b); err != nil { - return nil, err - } - } - } - return b, nil -} diff --git a/go/internal/depfix/sftp_test.go b/go/internal/depfix/sftp_test.go index e7623b2..7b08d28 100644 --- a/go/internal/depfix/sftp_test.go +++ b/go/internal/depfix/sftp_test.go @@ -177,121 +177,50 @@ func TestSFTPPacketGuard(t *testing.T) { } } -func TestSFTPPathAndRemoveGuard(t *testing.T) { - // 1. Verify ReadDirContext path traversal filename ("/..") handling. - makeReaddirServer := func(t *testing.T) *sftp.Client { - var sentEntry atomic.Bool - return runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { - switch pktType { - case sshFxpOpendir: - var b [9]byte - binary.BigEndian.PutUint32(b[0:4], sid) - binary.BigEndian.PutUint32(b[4:8], 1) - b[8] = 'h' - _, _ = wr.Write(buildSFTPPacket(sshFxpHandle, b[:])) - case sshFxpReaddir: - if !sentEntry.Swap(true) { - // Return 1 entry with filename "/.." - var payload []byte - var hdr [8]byte - binary.BigEndian.PutUint32(hdr[0:4], sid) - binary.BigEndian.PutUint32(hdr[4:8], 1) // count = 1 - payload = append(payload, hdr[:]...) - // filename = "/.." - payload = append(payload, 0, 0, 0, 3, '/', '.', '.') - // longname = "/.." - payload = append(payload, 0, 0, 0, 3, '/', '.', '.') - // attrs flags = 0 - payload = append(payload, 0, 0, 0, 0) - _, _ = wr.Write(buildSFTPPacket(sshFxpName, payload)) - return - } - // Second READDIR returns SSH_FX_EOF (1). - var b [16]byte - binary.BigEndian.PutUint32(b[0:4], sid) - binary.BigEndian.PutUint32(b[4:8], 1) - _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) - default: - // SSH_FX_OK (0) for SSH_FXP_CLOSE - var b [16]byte - binary.BigEndian.PutUint32(b[0:4], sid) - _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) - } - }) +func TestSFTPRemoveAllSymlink(t *testing.T) { + tmpDir := t.TempDir() + targetDir := filepath.Join(tmpDir, "target") + if err := os.Mkdir(targetDir, 0o755); err != nil { + t.Fatal(err) + } + secretFile := filepath.Join(targetDir, "keep.txt") + if err := os.WriteFile(secretFile, []byte("important"), 0o600); err != nil { + t.Fatal(err) + } + linkPath := filepath.Join(tmpDir, "link_to_target") + if err := os.Symlink(targetDir, linkPath); err != nil { + t.Fatal(err) } - t.Run("ReadDirTraversal/VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableSFTPPathAndRemoveGuard, func() { - client := makeReaddirServer(t) - entries, err := client.ReadDir("/dir") - if err != nil { - t.Fatalf("ReadDir: %v", err) - } - if len(entries) != 1 || entries[0].Name() != ".." { - t.Fatalf("expected pkg/sftp ReadDir to return Name() == \"..\" for \"/..\", got %v; if fixed upstream, remove EnableSFTPPathAndRemoveGuard", entries) - } - }) - }) - - t.Run("ReadDirTraversal/RemediationEnabled", func(t *testing.T) { - client := makeReaddirServer(t) - _, err := client.ReadDir("/dir") - if err == nil { - t.Fatal("expected ReadDir to reject \"/..\" filename when EnableSFTPPathAndRemoveGuard is enabled") - } + c1, s1 := io.Pipe() + s2, c2 := io.Pipe() + srv, err := sftp.NewServer(struct { + io.Reader + io.WriteCloser + }{c1, c2}) + if err != nil { + t.Fatal(err) + } + go func() { + _ = srv.Serve() + }() + t.Cleanup(func() { + _ = srv.Close() }) - - // 2. Regression test: RemoveAll symlink handling against a real local sftp.Server. - setupSymlinkTree := func(t *testing.T) (string, string, string, *sftp.Client) { - t.Helper() - tmpDir := t.TempDir() - targetDir := filepath.Join(tmpDir, "target") - if err := os.Mkdir(targetDir, 0o755); err != nil { - t.Fatal(err) - } - secretFile := filepath.Join(targetDir, "keep.txt") - if err := os.WriteFile(secretFile, []byte("important"), 0o600); err != nil { - t.Fatal(err) - } - linkPath := filepath.Join(tmpDir, "link_to_target") - if err := os.Symlink(targetDir, linkPath); err != nil { - t.Fatal(err) - } - - c1, s1 := io.Pipe() - s2, c2 := io.Pipe() - srv, err := sftp.NewServer(struct { - io.Reader - io.WriteCloser - }{c1, c2}) - if err != nil { - t.Fatal(err) - } - go func() { - _ = srv.Serve() - }() - t.Cleanup(func() { - _ = srv.Close() - }) - client, err := NewSFTPClientPipe(s2, s1) - if err != nil { - t.Fatal(err) - } - return linkPath, secretFile, targetDir, client + client, err := NewSFTPClientPipe(s2, s1) + if err != nil { + t.Fatal(err) } - t.Run("RemoveAllSymlink/RemediationEnabled", func(t *testing.T) { - linkPath, secretFile, _, client := setupSymlinkTree(t) - if err := client.RemoveAll(linkPath); err != nil { - t.Fatalf("RemoveAll: %v", err) - } - if _, err := os.Lstat(linkPath); !errors.Is(err, os.ErrNotExist) { - t.Errorf("expected symlink %s to be removed, err = %v", linkPath, err) - } - if _, err := os.Stat(secretFile); err != nil { - t.Errorf("expected target file %s to remain intact, err = %v", secretFile, err) - } - }) + if err := client.RemoveAll(linkPath); err != nil { + t.Fatalf("RemoveAll: %v", err) + } + if _, err := os.Lstat(linkPath); !errors.Is(err, os.ErrNotExist) { + t.Errorf("expected symlink %s to be removed, err = %v", linkPath, err) + } + if _, err := os.Stat(secretFile); err != nil { + t.Errorf("expected target file %s to remain intact, err = %v", secretFile, err) + } } func TestSFTPResourceGuard(t *testing.T) { diff --git a/go/internal/depfix/ssh.go b/go/internal/depfix/ssh.go index 1454f05..f589b0b 100644 --- a/go/internal/depfix/ssh.go +++ b/go/internal/depfix/ssh.go @@ -25,25 +25,15 @@ package depfix import ( "bytes" - "crypto/ecdsa" - "crypto/ed25519" - "crypto/sha256" "encoding/binary" "errors" "fmt" "io" - "math/big" "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" ) -const ( - agentFailureMsg = 5 - agentRequestIdentities = 11 - agentSignRequest = 13 -) - var ( // MaxConcurrentAgentChannels is the maximum number of concurrent // "auth-agent@openssh.com" channels accepted from a remote SSH server @@ -56,15 +46,12 @@ var ( // channel upon reading a 4-byte length header). MaxAgentRequestBytes uint32 = 256 * 1024 - ErrAgentRequestTooLarge = errors.New("ssh/agent: request exceeds maximum allowed size") - ErrDeprecatedCertSigAlgo = errors.New("certificate signature uses deprecated algorithm") - ErrInvalidCertSignature = errors.New("certificate signature is invalid") + ErrAgentRequestTooLarge = errors.New("ssh/agent: request exceeds maximum allowed size") ) // ForwardToAgent routes incoming "auth-agent@openssh.com" channels on client to // keyring. When EnableSSHAgentGuard is true, it limits concurrent agent -// channels, caps request frame sizes to MaxAgentRequestBytes, and rejects -// non-list/sign requests before agent.ServeAgent parses private key payloads. +// channels and caps request frame sizes to MaxAgentRequestBytes. func ForwardToAgent(client *ssh.Client, keyring agent.Agent) error { if !EnableSSHAgentGuard { return agent.ForwardToAgent(client, keyring) @@ -88,6 +75,7 @@ func ForwardToAgent(client *ssh.Client, keyring agent.Agent) error { continue } go ssh.DiscardRequests(reqs) + go io.Copy(io.Discard, ch.Stderr()) go func(c ssh.Channel) { defer func() { _ = c.Close() @@ -102,7 +90,7 @@ func ForwardToAgent(client *ssh.Client, keyring agent.Agent) error { // ServeAgent serves the SSH agent protocol over c. When EnableSSHAgentGuard is // true, it wraps c in an agentSafeChannel that enforces MaxAgentRequestBytes -// and filters out disallowed message types before agent.ServeAgent reads them. +// before agent.ServeAgent pre-allocates the request buffer. func ServeAgent(keyring agent.Agent, c io.ReadWriter) error { if !EnableSSHAgentGuard { return agent.ServeAgent(keyring, c) @@ -134,224 +122,22 @@ func (a *agentSafeChannel) Read(p []byte) (int, error) { } func (a *agentSafeChannel) readNextFrame() error { - for { - var hdr [4]byte - if _, err := io.ReadFull(a.rw, hdr[:]); err != nil { - return err - } - length := binary.BigEndian.Uint32(hdr[:]) - if length == 0 || length > MaxAgentRequestBytes { - return fmt.Errorf("%w: %d bytes", ErrAgentRequestTooLarge, length) - } - var msgType [1]byte - if _, err := io.ReadFull(a.rw, msgType[:]); err != nil { - if errors.Is(err, io.EOF) { - return io.ErrUnexpectedEOF - } - return err - } - // Only allow RequestIdentities (11) and SignRequest (13) to reach - // agent.ServeAgent. For any other message type (such as AddIdentity - // which triggers expensive RSA key validation in ServeAgent before - // forwardedAgent.Add is called), discard the payload and return - // SSH_AGENT_FAILURE immediately. - if msgType[0] != agentRequestIdentities && msgType[0] != agentSignRequest { - if length > 1 { - if _, err := io.CopyN(io.Discard, a.rw, int64(length-1)); err != nil { - return err - } - } - if _, err := a.rw.Write([]byte{0, 0, 0, 1, agentFailureMsg}); err != nil { - return err - } - continue - } - payload := make([]byte, length-1) - if _, err := io.ReadFull(a.rw, payload); err != nil { - if errors.Is(err, io.EOF) { - return io.ErrUnexpectedEOF - } - return err - } - a.buf.Write(hdr[:]) - a.buf.Write(msgType[:]) - a.buf.Write(payload) - return nil - } -} - -// WrapSSHConn wraps an ssh.Conn so that when EnableSSHSessionExitGuard is true, -// malformed "exit-status" or "exit-signal" channel requests on "session" -// channels close the session channel and drain remaining channel requests -// rather than deadlocking the connection's multiplexer loop. -func WrapSSHConn(c ssh.Conn) ssh.Conn { - if !EnableSSHSessionExitGuard { - return c - } - return &safeSSHConn{Conn: c} -} - -type safeSSHConn struct { - ssh.Conn -} - -func (s *safeSSHConn) OpenChannel(name string, data []byte) (ssh.Channel, <-chan *ssh.Request, error) { - ch, reqs, err := s.Conn.OpenChannel(name, data) - if err != nil || name != "session" || !EnableSSHSessionExitGuard { - return ch, reqs, err - } - out := make(chan *ssh.Request) - go func() { - defer close(out) - for msg := range reqs { - malformedExit := false - switch msg.Type { - case "exit-status": - if len(msg.Payload) < 4 { - malformedExit = true - } - case "exit-signal": - var sigval struct { - Signal string - CoreDumped bool - Error string - Lang string - } - if err := ssh.Unmarshal(msg.Payload, &sigval); err != nil { - malformedExit = true - } - } - out <- msg - if malformedExit { - // (*ssh.Session).wait returns early upon receiving a - // malformed exit-status/exit-signal without closing the - // channel or draining reqs. Close ch to unblock - // (*ssh.Session).Wait's copyFuncs and drain remaining - // requests so mux.loop() cannot block. - _ = ch.Close() - go ssh.DiscardRequests(reqs) - return - } - } - }() - return ch, out, nil -} - -// VerifyCertificateSignature checks the cryptographic signature on cert. When -// EnableCertSignatureGuard is true, it rejects weak SHA-1 (ssh-rsa) and DSA -// (ssh-dss) CA signatures and accepts security-key (sk-*) CA signatures that -// omit the UserPresence flag (matching OpenSSH and ssh.CertChecker.CheckCert). -func VerifyCertificateSignature(cert *ssh.Certificate) error { - if cert == nil || cert.SignatureKey == nil || cert.Signature == nil { - return ErrInvalidCertSignature - } - if EnableCertSignatureGuard { - switch cert.Signature.Format { - case ssh.KeyAlgoRSA, ssh.KeyAlgoDSA: - return fmt.Errorf("%w: %s", ErrDeprecatedCertSigAlgo, cert.Signature.Format) - } - } - c2 := *cert - c2.Signature = nil - signBytes := c2.Marshal() - if len(signBytes) < 4 { - return ErrInvalidCertSignature + var hdr [4]byte + if _, err := io.ReadFull(a.rw, hdr[:]); err != nil { + return err } - signedData := signBytes[:len(signBytes)-4] - err := cert.SignatureKey.Verify(signedData, cert.Signature) - if err == nil { - return nil + length := binary.BigEndian.Uint32(hdr[:]) + if length == 0 || length > MaxAgentRequestBytes { + return fmt.Errorf("%w: %d bytes", ErrAgentRequestTooLarge, length) } - if EnableCertSignatureGuard { - switch cert.SignatureKey.Type() { - case ssh.KeyAlgoSKECDSA256, ssh.KeyAlgoSKED25519: - if verifySKSignatureWithoutUP(cert.SignatureKey, signedData, cert.Signature) == nil { - return nil - } + payload := make([]byte, length) + if _, err := io.ReadFull(a.rw, payload); err != nil { + if errors.Is(err, io.EOF) { + return io.ErrUnexpectedEOF } - } - return ErrInvalidCertSignature -} - -func verifySKSignatureWithoutUP(pub ssh.PublicKey, data []byte, sig *ssh.Signature) error { - if sig == nil || sig.Format != pub.Type() { - return ErrInvalidCertSignature - } - cryptoPub, ok := pub.(ssh.CryptoPublicKey) - if !ok { - return ErrInvalidCertSignature - } - var skf struct { - Flags byte - Counter uint32 - } - if err := ssh.Unmarshal(sig.Rest, &skf); err != nil { return err } - switch pub.Type() { - case ssh.KeyAlgoSKECDSA256: - var w struct { - Name string - Curve string - KeyBytes []byte - Application string - } - if err := ssh.Unmarshal(pub.Marshal(), &w); err != nil { - return err - } - var ecSig struct { - R *big.Int - S *big.Int - } - if err := ssh.Unmarshal(sig.Blob, &ecSig); err != nil { - return err - } - ecPub, ok := cryptoPub.CryptoPublicKey().(*ecdsa.PublicKey) - if !ok { - return ErrInvalidCertSignature - } - appDigest := sha256.Sum256([]byte(w.Application)) - dataDigest := sha256.Sum256(data) - blob := ssh.Marshal(struct { - ApplicationDigest []byte `ssh:"rest"` - Flags byte - Counter uint32 - MessageDigest []byte `ssh:"rest"` - }{appDigest[:], skf.Flags, skf.Counter, dataDigest[:]}) - digest := sha256.Sum256(blob) - if ecdsa.Verify(ecPub, digest[:], ecSig.R, ecSig.S) { - return nil - } - case ssh.KeyAlgoSKED25519: - var w struct { - Name string - KeyBytes []byte - Application string - } - if err := ssh.Unmarshal(pub.Marshal(), &w); err != nil { - return err - } - var edSig struct { - Signature []byte `ssh:"rest"` - } - if err := ssh.Unmarshal(sig.Blob, &edSig); err != nil { - return err - } - edPub, ok := cryptoPub.CryptoPublicKey().(ed25519.PublicKey) - if !ok { - return ErrInvalidCertSignature - } - appDigest := sha256.Sum256([]byte(w.Application)) - dataDigest := sha256.Sum256(data) - blob := ssh.Marshal(struct { - ApplicationDigest []byte `ssh:"rest"` - Flags byte - Counter uint32 - MessageDigest []byte `ssh:"rest"` - }{appDigest[:], skf.Flags, skf.Counter, dataDigest[:]}) - if ed25519.Verify(edPub, blob, edSig.Signature) { - return nil - } - } - return ErrInvalidCertSignature + a.buf.Write(hdr[:]) + a.buf.Write(payload) + return nil } diff --git a/go/internal/depfix/ssh_test.go b/go/internal/depfix/ssh_test.go index 62d1f5a..3799d54 100644 --- a/go/internal/depfix/ssh_test.go +++ b/go/internal/depfix/ssh_test.go @@ -24,84 +24,25 @@ package depfix import ( - "crypto/ed25519" - "crypto/rand" - "crypto/rsa" - "crypto/sha256" "encoding/binary" "errors" - "io" "net" - "sync/atomic" "testing" "time" - "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" ) -type spyAgent struct { +type nopAgent struct { agent.Agent - addCalled atomic.Int32 } -func (s *spyAgent) List() ([]*agent.Key, error) { +func (nopAgent) List() ([]*agent.Key, error) { return nil, nil } -func (s *spyAgent) Add(key agent.AddedKey) error { - s.addCalled.Add(1) - return errors.New("operation not permitted on forwarded agent") -} - func TestSSHAgentGuard(t *testing.T) { - // 1. Disallowed AddIdentity message triggers key parsing & Add() when - // EnableSSHAgentGuard is disabled, and is blocked at the framing layer - // when EnableSSHAgentGuard is enabled. - _, edPriv, err := ed25519.GenerateKey(rand.Reader) - if err != nil { - t.Fatal(err) - } - - t.Run("AddIdentityPreParse/VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableSSHAgentGuard, func() { - c1, s1 := net.Pipe() - defer c1.Close() - defer s1.Close() - spy := &spyAgent{} - go func() { - _ = ServeAgent(spy, s1) - }() - client := agent.NewClient(c1) - _ = client.Add(agent.AddedKey{PrivateKey: edPriv, Comment: "attacker-key"}) - if spy.addCalled.Load() == 0 { - t.Fatal("expected x/crypto/ssh/agent ServeAgent to parse key and call Add when EnableSSHAgentGuard is disabled; if fixed upstream, remove EnableSSHAgentGuard") - } - }) - }) - - t.Run("AddIdentityPreParse/RemediationEnabled", func(t *testing.T) { - c1, s1 := net.Pipe() - defer c1.Close() - defer s1.Close() - spy := &spyAgent{} - go func() { - _ = ServeAgent(spy, s1) - }() - client := agent.NewClient(c1) - if err := client.Add(agent.AddedKey{PrivateKey: edPriv, Comment: "attacker-key"}); err == nil { - t.Fatal("expected Add to fail on forwarded agent") - } - if got := spy.addCalled.Load(); got != 0 { - t.Fatalf("expected agentSafeChannel to reject AddIdentity before ServeAgent called Add, got addCalled=%d", got) - } - // Subsequent List() on the same channel still succeeds. - if _, err := client.List(); err != nil { - t.Fatalf("List after rejected Add: %v", err) - } - }) - - // 2. Oversized agent frame length header (> MaxAgentRequestBytes). + // Oversized agent frame length header (> MaxAgentRequestBytes). t.Run("OversizedFrame/VulnerabilityWithoutRemediation", func(t *testing.T) { WithDisabled(&EnableSSHAgentGuard, func() { c1, s1 := net.Pipe() @@ -109,7 +50,7 @@ func TestSSHAgentGuard(t *testing.T) { defer s1.Close() errCh := make(chan error, 1) go func() { - errCh <- ServeAgent(&spyAgent{}, s1) + errCh <- ServeAgent(nopAgent{}, s1) }() // Send a 4-byte length header of MaxAgentRequestBytes + 100 (still <= 16 MiB). var hdr [4]byte @@ -133,7 +74,7 @@ func TestSSHAgentGuard(t *testing.T) { defer s1.Close() errCh := make(chan error, 1) go func() { - errCh <- ServeAgent(&spyAgent{}, s1) + errCh <- ServeAgent(nopAgent{}, s1) }() var hdr [4]byte binary.BigEndian.PutUint32(hdr[:], MaxAgentRequestBytes+100) @@ -148,282 +89,3 @@ func TestSSHAgentGuard(t *testing.T) { } }) } - -func TestSSHSessionExitGuard(t *testing.T) { - _, hostPriv, err := ed25519.GenerateKey(rand.Reader) - if err != nil { - t.Fatal(err) - } - hostSigner, err := ssh.NewSignerFromKey(hostPriv) - if err != nil { - t.Fatal(err) - } - - startMalformedExitServer := func(t *testing.T) (*ssh.Client, func()) { - t.Helper() - ln, err := net.Listen("tcp", "127.0.0.1:0") - if err != nil { - t.Fatal(err) - } - srvCfg := &ssh.ServerConfig{NoClientAuth: true} - srvCfg.AddHostKey(hostSigner) - - done := make(chan struct{}) - go func() { - defer close(done) - s1, err := ln.Accept() - if err != nil { - return - } - defer s1.Close() - _, chans, reqs, err := ssh.NewServerConn(s1, srvCfg) - if err != nil { - return - } - go ssh.DiscardRequests(reqs) - for newChan := range chans { - if newChan.ChannelType() != "session" { - _ = newChan.Reject(ssh.UnknownChannelType, "unsupported") - continue - } - ch, chReqs, err := newChan.Accept() - if err != nil { - return - } - go func() { - for req := range chReqs { - if req.Type == "exec" { - _ = req.Reply(true, nil) - // Send a malformed 1-byte exit-status (needs 4 bytes), then - // send 20 extra channel requests without closing ch. - _, _ = ch.SendRequest("exit-status", false, []byte{0}) - for range 20 { - _, _ = ch.SendRequest("keepalive@openssh.com", false, nil) - } - return - } - _ = req.Reply(false, nil) - } - }() - } - }() - - c1, err := net.Dial("tcp", ln.Addr().String()) - if err != nil { - _ = ln.Close() - t.Fatalf("Dial: %v", err) - } - cliCfg := &ssh.ClientConfig{ - User: "test", - HostKeyCallback: ssh.FixedHostKey(hostSigner.PublicKey()), - } - conn, chans, reqs, err := ssh.NewClientConn(c1, "localhost", cliCfg) - if err != nil { - _ = c1.Close() - _ = ln.Close() - t.Fatalf("NewClientConn: %v", err) - } - client := ssh.NewClient(WrapSSHConn(conn), chans, reqs) - cleanup := func() { - _ = client.Close() - _ = c1.Close() - _ = ln.Close() - <-done - } - return client, cleanup - } - - t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableSSHSessionExitGuard, func() { - client, cleanup := startMalformedExitServer(t) - defer cleanup() - - sess, err := client.NewSession() - if err != nil { - t.Fatalf("NewSession: %v", err) - } - sess.Stdout = io.Discard - if err := sess.Start("cmd"); err != nil { - t.Fatalf("Start: %v", err) - } - waitCh := make(chan error, 1) - go func() { - waitCh <- sess.Wait() - }() - select { - case err := <-waitCh: - t.Fatalf("expected (*ssh.Session).Wait to hang on malformed exit-status when EnableSSHSessionExitGuard is disabled (got %v); if fixed upstream, remove WrapSSHConn", err) - case <-time.After(150 * time.Millisecond): - // Expected hang in vulnerable mode. - } - }) - }) - - t.Run("RemediationEnabled", func(t *testing.T) { - client, cleanup := startMalformedExitServer(t) - defer cleanup() - - sess, err := client.NewSession() - if err != nil { - t.Fatalf("NewSession: %v", err) - } - sess.Stdout = io.Discard - if err := sess.Start("cmd"); err != nil { - t.Fatalf("Start: %v", err) - } - waitCh := make(chan error, 1) - go func() { - waitCh <- sess.Wait() - }() - select { - case err := <-waitCh: - if err == nil { - t.Fatal("expected error from Wait on malformed exit-status") - } - case <-time.After(2 * time.Second): - t.Fatal("timed out waiting for sess.Wait() with EnableSSHSessionExitGuard enabled") - } - - // Verify the connection multiplexer is not deadlocked by sending a global request. - reqDone := make(chan error, 1) - go func() { - _, _, err := client.SendRequest("keepalive@openssh.com", true, nil) - reqDone <- err - }() - select { - case err := <-reqDone: - if err != nil { - t.Fatalf("client.SendRequest failed: %v", err) - } - case <-time.After(2 * time.Second): - t.Fatal("SSH connection multiplexer deadlocked after malformed exit-status") - } - }) -} - -type rsaSHA1Signer struct { - ssh.Signer -} - -func (s rsaSHA1Signer) Sign(rand io.Reader, data []byte) (*ssh.Signature, error) { - return s.Signer.(ssh.AlgorithmSigner).SignWithAlgorithm(rand, data, ssh.KeyAlgoRSA) -} - -func TestCertSignatureGuard(t *testing.T) { - // 1. RSA SHA-1 ("ssh-rsa") CA signature. - rsaPriv, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatal(err) - } - rsaSigner, err := ssh.NewSignerFromKey(rsaPriv) - if err != nil { - t.Fatal(err) - } - pub, _, err := ed25519.GenerateKey(rand.Reader) - if err != nil { - t.Fatal(err) - } - sshPub, err := ssh.NewPublicKey(pub) - if err != nil { - t.Fatal(err) - } - - sha1Cert := &ssh.Certificate{ - Key: sshPub, - CertType: ssh.HostCert, - ValidBefore: ssh.CertTimeInfinity, - } - if err := sha1Cert.SignCert(rand.Reader, rsaSHA1Signer{rsaSigner}); err != nil { - t.Fatal(err) - } - if sha1Cert.Signature.Format != ssh.KeyAlgoRSA { - t.Fatalf("Signature.Format = %q, want %q", sha1Cert.Signature.Format, ssh.KeyAlgoRSA) - } - - t.Run("RSASHA1/VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableCertSignatureGuard, func() { - if err := VerifyCertificateSignature(sha1Cert); err != nil { - t.Fatalf("expected legacy VerifyCertificateSignature to accept ssh-rsa CA signature, got %v", err) - } - }) - }) - - t.Run("RSASHA1/RemediationEnabled", func(t *testing.T) { - if err := VerifyCertificateSignature(sha1Cert); !errors.Is(err, ErrDeprecatedCertSigAlgo) { - t.Fatalf("expected ErrDeprecatedCertSigAlgo for ssh-rsa CA signature, got %v", err) - } - // Verify RSA SHA-256/512 signed certificate succeeds. - sha2Cert := &ssh.Certificate{ - Key: sshPub, - CertType: ssh.HostCert, - ValidBefore: ssh.CertTimeInfinity, - } - if err := sha2Cert.SignCert(rand.Reader, rsaSigner); err != nil { - t.Fatal(err) - } - if err := VerifyCertificateSignature(sha2Cert); err != nil { - t.Fatalf("VerifyCertificateSignature(rsa-sha2-512): %v", err) - } - }) - - // 2. Security-key (sk-ssh-ed25519@openssh.com) CA signature without UserPresence flag (Flags = 0). - skEdPub, skEdPriv, err := ed25519.GenerateKey(rand.Reader) - if err != nil { - t.Fatal(err) - } - skPubBlob := ssh.Marshal(struct { - Name string - KeyBytes []byte - Application string - }{ - Name: ssh.KeyAlgoSKED25519, - KeyBytes: skEdPub, - Application: "ssh:ca", - }) - skPubKey, err := ssh.ParsePublicKey(skPubBlob) - if err != nil { - t.Fatalf("ParsePublicKey(sk-ed25519): %v", err) - } - skCert := &ssh.Certificate{ - Nonce: make([]byte, 32), - Key: sshPub, - CertType: ssh.HostCert, - ValidBefore: ssh.CertTimeInfinity, - SignatureKey: skPubKey, - } - c2 := *skCert - c2.Signature = nil - raw := c2.Marshal() - signedBytes := raw[:len(raw)-4] - appDigest := sha256.Sum256([]byte("ssh:ca")) - dataDigest := sha256.Sum256(signedBytes) - skSignedBlob := ssh.Marshal(struct { - ApplicationDigest []byte `ssh:"rest"` - Flags byte - Counter uint32 - MessageDigest []byte `ssh:"rest"` - }{appDigest[:], 0x00 /* Flags = 0 (no-touch-required) */, 1, dataDigest[:]}) - edSig := ed25519.Sign(skEdPriv, skSignedBlob) - skCert.Signature = &ssh.Signature{ - Format: ssh.KeyAlgoSKED25519, - Blob: edSig, - Rest: ssh.Marshal(struct { - Flags byte - Counter uint32 - }{0x00, 1}), - } - - t.Run("SKNoTouchCA/VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableCertSignatureGuard, func() { - if err := VerifyCertificateSignature(skCert); err == nil { - t.Fatal("expected PublicKey.Verify to reject sk-ssh-ed25519 CA signature with Flags=0 when EnableCertSignatureGuard is disabled") - } - }) - }) - - t.Run("SKNoTouchCA/RemediationEnabled", func(t *testing.T) { - if err := VerifyCertificateSignature(skCert); err != nil { - t.Fatalf("expected VerifyCertificateSignature to accept sk-ssh-ed25519 CA signature with Flags=0, got %v", err) - } - }) -} diff --git a/go/internal/depfix/term.go b/go/internal/depfix/term.go deleted file mode 100644 index a29b738..0000000 --- a/go/internal/depfix/term.go +++ /dev/null @@ -1,136 +0,0 @@ -// MIT License -// -// Copyright (c) 2026 TTBT Enterprises LLC -// Copyright (c) 2026 Robin Thellend -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in all -// copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -package depfix - -import ( - "bytes" - "io" -) - -const ( - // maxTermEscapeChunk is the maximum number of bytes allowed in an - // unterminated escape sequence before injecting a '~' terminator and - // restarting '\x1b', keeping golang.org/x/term's 256-byte inBuf from - // ever filling up. - maxTermEscapeChunk = 64 -) - -// WrapTermReadWriter wraps an io.ReadWriter passed to golang.org/x/term's -// NewTerminal to prevent a 256-byte unterminated escape sequence from filling -// Terminal.inBuf and causing an infinite synchronous 0-byte Read busy loop. -func WrapTermReadWriter(rw io.ReadWriter) io.ReadWriter { - return &termSafeReadWriter{rw: rw} -} - -type termSafeReadWriter struct { - rw io.ReadWriter - outBuf bytes.Buffer - err error - inEsc bool - escLen int -} - -func (t *termSafeReadWriter) Write(p []byte) (int, error) { - return t.rw.Write(p) -} - -func isTermEscTerminator(c byte) bool { - return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || c == '~' -} - -func (t *termSafeReadWriter) Read(p []byte) (int, error) { - if !EnableTermEscapeGuard { - return t.rw.Read(p) - } - if t.outBuf.Len() > 0 { - nn, _ := t.outBuf.Read(p) - if t.outBuf.Len() == 0 && t.err != nil { - err := t.err - t.err = nil - return nn, err - } - return nn, nil - } - if t.err != nil { - err := t.err - t.err = nil - return 0, err - } - raw := make([]byte, max(len(p), 64)) - n, err := t.rw.Read(raw) - for i := 0; i < n; i++ { - c := raw[i] - if !t.inEsc { - if c == 0x1b { - t.inEsc = true - t.escLen = 1 - } - t.outBuf.WriteByte(c) - continue - } - if c == 0x1b { - // Terminate the previous unfinished escape sequence before - // starting the new one. - t.outBuf.WriteByte('~') - t.outBuf.WriteByte(0x1b) - t.escLen = 1 - continue - } - if c == '\r' || c == '\n' || c == 0x03 || c == 0x04 { - // Terminate the unfinished escape sequence so line/control - // keys are still processed by x/term. - t.outBuf.WriteByte('~') - t.outBuf.WriteByte(c) - t.inEsc = false - t.escLen = 0 - continue - } - if isTermEscTerminator(c) { - t.outBuf.WriteByte(c) - t.inEsc = false - t.escLen = 0 - continue - } - if t.escLen >= maxTermEscapeChunk-1 { - // Close the current chunk with '~' so x/term consumes it as - // keyUnknown and clears t.remainder, then re-open '\x1b' so - // subsequent bytes of the escape sequence remain inside an - // escape sequence. - t.outBuf.WriteByte('~') - t.outBuf.WriteByte(0x1b) - t.escLen = 1 - } - t.outBuf.WriteByte(c) - t.escLen++ - } - if t.outBuf.Len() > 0 { - nn, _ := t.outBuf.Read(p) - if t.outBuf.Len() > 0 { - t.err = err - return nn, nil - } - return nn, err - } - return 0, err -} diff --git a/go/internal/depfix/term_test.go b/go/internal/depfix/term_test.go deleted file mode 100644 index 6919072..0000000 --- a/go/internal/depfix/term_test.go +++ /dev/null @@ -1,109 +0,0 @@ -// MIT License -// -// Copyright (c) 2026 TTBT Enterprises LLC -// Copyright (c) 2026 Robin Thellend -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in all -// copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -package depfix - -import ( - "bytes" - "errors" - "io" - "strings" - "sync/atomic" - "testing" - - "golang.org/x/term" -) - -type trackingTermIO struct { - in *bytes.Reader - eofWithData bool - zeroReadCalls atomic.Int32 -} - -func (t *trackingTermIO) Read(p []byte) (int, error) { - if len(p) == 0 { - if t.zeroReadCalls.Add(1) >= 10 { - return 0, errors.New("detected infinite 0-byte Read loop in golang.org/x/term") - } - return 0, nil - } - n, err := t.in.Read(p) - if t.eofWithData && err == nil && t.in.Len() == 0 { - err = io.EOF - } - return n, err -} - -func (t *trackingTermIO) Write(p []byte) (int, error) { - return len(p), nil -} - -func TestTermEscapeGuard(t *testing.T) { - // 260-byte unterminated escape sequence (exceeds x/term's 256-byte inBuf) - // followed by a normal terminator 'm' and "hello\n". - input := "\x1b[" + strings.Repeat("0", 260) + "mhello\n" - - t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableTermEscapeGuard, func() { - tio := &trackingTermIO{in: bytes.NewReader([]byte(input))} - vt := term.NewTerminal(WrapTermReadWriter(tio), "> ") - _, err := vt.ReadLine() - if tio.zeroReadCalls.Load() == 0 { - t.Fatalf("expected golang.org/x/term to call Read with len(p)==0 on 260-byte escape sequence (err=%v); if fixed upstream, remove WrapTermReadWriter", err) - } - }) - }) - - t.Run("RemediationEnabled", func(t *testing.T) { - tio := &trackingTermIO{in: bytes.NewReader([]byte(input))} - vt := term.NewTerminal(WrapTermReadWriter(tio), "> ") - line, err := vt.ReadLine() - if err != nil && !errors.Is(err, io.EOF) { - t.Fatalf("ReadLine: %v", err) - } - if tio.zeroReadCalls.Load() != 0 { - t.Fatalf("unexpected 0-byte Read calls: %d", tio.zeroReadCalls.Load()) - } - if line != "hello" { - t.Fatalf("ReadLine() = %q, want %q", line, "hello") - } - }) - - t.Run("SimultaneousDataAndEOF", func(t *testing.T) { - // 256-byte input where injected chunk terminators expand outBuf past - // 256 bytes while the underlying reader returns (256, io.EOF). - eofInput := "\x1b[" + strings.Repeat("0", 247) + "mhello\n" - tio := &trackingTermIO{ - in: bytes.NewReader([]byte(eofInput)), - eofWithData: true, - } - vt := term.NewTerminal(WrapTermReadWriter(tio), "> ") - line, err := vt.ReadLine() - if err != nil && !errors.Is(err, io.EOF) { - t.Fatalf("ReadLine: %v", err) - } - if line != "hello" { - t.Fatalf("ReadLine() = %q, want %q", line, "hello") - } - }) -} diff --git a/go/internal/jsutil/streams.go b/go/internal/jsutil/streams.go index 5f862a1..31bdfc8 100644 --- a/go/internal/jsutil/streams.go +++ b/go/internal/jsutil/streams.go @@ -100,16 +100,7 @@ func NewReadableStream(r io.Reader, done chan<- error, progress func(int64)) js. size = max(size, ds.Int()) } buf := make([]byte, size) - var n int - var err error - TryCatch( - func() { - n, err = r.Read(buf) - }, - func(p any) { - err = fmt.Errorf("panic: %v", p) - }, - ) + n, err := r.Read(buf) if n > 0 { select { case <-cancelCh: diff --git a/go/internal/terminal/terminal.go b/go/internal/terminal/terminal.go index 345312b..e7649e8 100644 --- a/go/internal/terminal/terminal.go +++ b/go/internal/terminal/terminal.go @@ -38,7 +38,6 @@ import ( "golang.org/x/term" - "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/jsutil" ) @@ -55,7 +54,7 @@ func New(ctx context.Context, t js.Value) *Terminal { onData: make(map[int]func(k string) any), }, } - tt.vt = term.NewTerminal(depfix.WrapTermReadWriter(tt.tw), "") + tt.vt = term.NewTerminal(tt.tw, "") tt.Escape = tt.vt.Escape tt.setDefaultPrompt() diff --git a/go/internal/tests/remediations_test.go b/go/internal/tests/remediations_test.go index 048539b..b9bfbcd 100644 --- a/go/internal/tests/remediations_test.go +++ b/go/internal/tests/remediations_test.go @@ -36,7 +36,6 @@ import ( "golang.org/x/crypto/ssh" "github.com/c2FmZQ/sshterm/internal/app" - "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/jsutil" ) @@ -100,9 +99,8 @@ func TestXtermWorkarounds(t *testing.T) { t.Run("CSIRepGuard", func(t *testing.T) { t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { term, cleanup := createTestTerm(t, map[string]any{ - "csiRepGuard": false, - "csiLoopGuard": false, - "oscQueryAndLinkGuard": false, + "csiRepGuard": false, + "csiLoopGuard": false, }) defer cleanup() writeTerm(t, term, "a") @@ -125,9 +123,8 @@ func TestXtermWorkarounds(t *testing.T) { t.Run("RemediationEnabled", func(t *testing.T) { term, cleanup := createTestTerm(t, map[string]any{ - "csiRepGuard": true, - "csiLoopGuard": true, - "oscQueryAndLinkGuard": true, + "csiRepGuard": true, + "csiLoopGuard": true, }) defer cleanup() writeTerm(t, term, "a\x1b[2147483647bOK") @@ -138,9 +135,8 @@ func TestXtermWorkarounds(t *testing.T) { t.Run("CSILoopGuard", func(t *testing.T) { t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { term, cleanup := createTestTerm(t, map[string]any{ - "csiRepGuard": true, - "csiLoopGuard": false, - "oscQueryAndLinkGuard": true, + "csiRepGuard": true, + "csiLoopGuard": false, }) defer cleanup() @@ -160,9 +156,8 @@ func TestXtermWorkarounds(t *testing.T) { t.Run("RemediationEnabled", func(t *testing.T) { term, cleanup := createTestTerm(t, map[string]any{ - "csiRepGuard": true, - "csiLoopGuard": true, - "oscQueryAndLinkGuard": true, + "csiRepGuard": true, + "csiLoopGuard": true, }) defer cleanup() start := time.Now() @@ -173,67 +168,7 @@ func TestXtermWorkarounds(t *testing.T) { }) }) - // 3. OSC 4 query amplification & OSC 8 hyperlink marker flood - t.Run("OSCQueryAndLinkGuard", func(t *testing.T) { - chainedOsc4 := "\x1b]4" + strings.Repeat(";0;?", 25) + "\x07" - floodOsc8 := strings.Repeat("\x1b]8;;https://example.com\x07", 50) + "\x1b]8;;\x07" - - t.Run("VulnerabilityWithoutRemediation", func(t *testing.T) { - term, cleanup := createTestTerm(t, map[string]any{ - "csiRepGuard": true, - "csiLoopGuard": true, - "oscQueryAndLinkGuard": false, - }) - defer cleanup() - - responses := 0 - cb := js.FuncOf(func(this js.Value, args []js.Value) any { - responses++ - return nil - }) - defer cb.Release() - disp := term.Call("onData", cb) - writeTerm(t, term, chainedOsc4) - writeTerm(t, term, floodOsc8) - disp.Call("dispose") - - if responses != 25 { - t.Fatalf("expected 25 OSC 4 responses without oscQueryAndLinkGuard, got %d", responses) - } - if got := term.Get("_core").Get("markers").Length(); got < 50 { - t.Fatalf("expected >= 50 OSC 8 markers without oscQueryAndLinkGuard, got %d", got) - } - }) - - t.Run("RemediationEnabled", func(t *testing.T) { - term, cleanup := createTestTerm(t, map[string]any{ - "csiRepGuard": true, - "csiLoopGuard": true, - "oscQueryAndLinkGuard": true, - }) - defer cleanup() - - responses := 0 - cb := js.FuncOf(func(this js.Value, args []js.Value) any { - responses++ - return nil - }) - defer cb.Release() - disp := term.Call("onData", cb) - writeTerm(t, term, chainedOsc4) - writeTerm(t, term, floodOsc8) - disp.Call("dispose") - - if responses != 0 { - t.Fatalf("expected 0 OSC 4 responses for >16 chained query flood, got %d", responses) - } - if got := term.Get("_core").Get("markers").Length(); got > 32 { - t.Fatalf("expected <= 32 OSC 8 markers with oscQueryAndLinkGuard, got %d", got) - } - }) - }) - - // 4. Title sanitization and _showMessage toast DOM reuse + // 3. Title sanitization and _showMessage toast DOM reuse t.Run("TitleOverlay", func(t *testing.T) { screen := doc.Call("querySelector", `[id^="screen-"]`) if screen.IsNull() || screen.IsUndefined() { @@ -254,7 +189,7 @@ func TestXtermWorkarounds(t *testing.T) { }) } -func TestRSASHA2AndForwardedAgentRemediation(t *testing.T) { +func TestRSASHA2ForwardedAgent(t *testing.T) { a, err := app.New(appConfig) if err != nil { t.Fatalf("app.New: %v", err) @@ -284,23 +219,8 @@ func TestRSASHA2AndForwardedAgentRemediation(t *testing.T) { }}, {Type: "agent add rsatest\n", Expect: "Enter the passphrase for rsatest:"}, {Type: "foobar\n", Expect: prompt}, - }) - - // 1. With EnableRSASHA2 disabled, forwarded agent SignWithFlags(SignatureFlagRsaSha512) - // ignores the flag and returns "ssh-rsa" (SHA-1). - depfix.WithDisabled(&depfix.EnableRSASHA2, func() { - script(t, []line{ - {Type: "ssh -A testuser@test-server agent-test\n", Expect: `(?s)Host key for test-server.*Choice>`}, - {Type: "3\n", Expect: `\[agent\] test-server requested a signature with key "rsatest"`}, - {Expect: `(?s)agent-test: sign: ok \(ssh-rsa\).*agent-test: sign-sha512: ok \(ssh-rsa\).*agent-test: removeall: agent: failure`}, - {Wait: time.Second, Type: "\n\n", Expect: prompt}, - }) - }) - - // 2. With EnableRSASHA2 enabled, forwarded agent SignWithFlags(SignatureFlagRsaSha512) - // returns "rsa-sha2-512". - script(t, []line{ - {Type: "ssh -A testuser@test-server agent-test\n", Expect: `\[agent\] test-server requested a signature with key "rsatest"`}, + {Type: "ssh -A testuser@test-server agent-test\n", Expect: `(?s)Host key for test-server.*Choice>`}, + {Type: "3\n", Expect: `\[agent\] test-server requested a signature with key "rsatest"`}, {Expect: `(?s)agent-test: sign: ok \(ssh-rsa\).*agent-test: sign-sha512: ok \(rsa-sha2-512\).*agent-test: removeall: agent: failure`}, {Wait: time.Second, Type: "\n\n", Expect: prompt}, {Type: "exit\n"}, From f6564d8018e1f22d8ef8c4965c97917a1a8c1169 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Sun, 25 Oct 2026 08:33:30 -0700 Subject: [PATCH 4/5] Remove depfix package and use upstream sftp and ssh/agent directly --- CHANGELOG.md | 3 +- go/internal/app/sftp.go | 3 +- go/internal/app/ssh.go | 3 +- go/internal/depfix/README.md | 26 --- go/internal/depfix/depfix.go | 57 ------- go/internal/depfix/sftp.go | 274 ----------------------------- go/internal/depfix/sftp_test.go | 294 -------------------------------- go/internal/depfix/ssh.go | 143 ---------------- go/internal/depfix/ssh_test.go | 91 ---------- 9 files changed, 3 insertions(+), 891 deletions(-) delete mode 100644 go/internal/depfix/README.md delete mode 100644 go/internal/depfix/depfix.go delete mode 100644 go/internal/depfix/sftp.go delete mode 100644 go/internal/depfix/sftp_test.go delete mode 100644 go/internal/depfix/ssh.go delete mode 100644 go/internal/depfix/ssh_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a04cb3b..caf2316 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,9 +5,8 @@ ### :lock: Security fixes * Align `go/go.mod` (`go 1.27.3`) and `go/build.sh` (`GOTOOLCHAIN`) with `.goversion` so local builds do not fall back to Go 1.26.0, and remove stale `docroot/xterm.js` from the embedded server docroot. -* Upgrade `github.com/pkg/sftp` to `v1.13.12` (fixing truncated packet unmarshaling panics and `Client.RemoveAll` directory symlink traversal), reject 0-byte `SSH_FXP_DATA` packets (preventing an infinite read loop), and bound the number of entries accumulated in `ReadDirContext`. +* Upgrade `github.com/pkg/sftp` to `v1.13.12` (fixing truncated packet unmarshaling panics and `Client.RemoveAll` directory symlink traversal). * Harden SSH client and forwarded agent (`golang.org/x/crypto/ssh`): - * Bound concurrent `"auth-agent@openssh.com"` channels and cap incoming agent request frames to 256 KiB before `agent.ServeAgent` allocates up to 16 MiB per channel. * Implement `ssh.MultiAlgorithmSigner` on `dynSigner` and `agent.ExtendedAgent` (`SignWithFlags`) on `keyRing` and `forwardedAgent` so RSA keys and user certificates use `rsa-sha2-256` / `rsa-sha2-512` instead of falling back to deprecated SHA-1 (`ssh-rsa`). * Reject deprecated SHA-1 (`ssh-rsa`) and DSA (`ssh-dss`) CA signatures in `checkCertificate`. * Fix a trailing-backslash parsing bug in `shellwords.Parse` that could cause a slice-bounds panic during tab auto-completion. diff --git a/go/internal/app/sftp.go b/go/internal/app/sftp.go index f72395f..823d731 100644 --- a/go/internal/app/sftp.go +++ b/go/internal/app/sftp.go @@ -43,7 +43,6 @@ import ( "github.com/urfave/cli/v2" "golang.org/x/term" - "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/jsutil" "github.com/c2FmZQ/sshterm/internal/shellwords" "github.com/c2FmZQ/sshterm/internal/termsafe" @@ -95,7 +94,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e } go sshKeepAlive(ctx, c, cancel) - client, err := depfix.NewSFTPClient(c) + client, err := sftp.NewClient(c) if err != nil { return err } diff --git a/go/internal/app/ssh.go b/go/internal/app/ssh.go index 11e6956..35acda3 100644 --- a/go/internal/app/ssh.go +++ b/go/internal/app/ssh.go @@ -40,7 +40,6 @@ import ( "golang.org/x/crypto/ssh" "golang.org/x/crypto/ssh/agent" - "github.com/c2FmZQ/sshterm/internal/depfix" "github.com/c2FmZQ/sshterm/internal/termsafe" "github.com/c2FmZQ/sshterm/internal/websocket" ) @@ -133,7 +132,7 @@ func (a *App) runSSH(ctx context.Context, target, keyName, command string, forwa t.ShowMessage(f, args...) }, } - if err := depfix.ForwardToAgent(client, fwd); err != nil { + if err := agent.ForwardToAgent(client, fwd); err != nil { return fmt.Errorf("agent.ForwardToAgent: %w", err) } if err := agent.RequestAgentForwarding(session); err != nil { diff --git a/go/internal/depfix/README.md b/go/internal/depfix/README.md deleted file mode 100644 index 0d8af07..0000000 --- a/go/internal/depfix/README.md +++ /dev/null @@ -1,26 +0,0 @@ -# Dependency Security Remediations (`internal/depfix`) - -This package centralizes workarounds for security issues in third-party dependencies so that: - -1. **Isolation**: Each workaround is controlled by a boolean flag in [`depfix.go`](depfix.go) (or `workarounds` in [`docroot/ssh.mjs`](../../../docroot/ssh.mjs) for `@xterm/xterm`). -2. **Dual-Mode Verification**: Every workaround has unit/integration tests that run in two subtests: - * `VulnerabilityWithoutRemediation`: Temporarily disables the workaround via `depfix.WithDisabled(&depfix.Enable..., ...)` and asserts that the upstream dependency still exhibits the vulnerable behavior. - * `RemediationEnabled`: Runs with the workaround enabled (the default) and asserts that the vulnerability is blocked. -3. **Automatic Retirement Signal**: When a dependency is upgraded in `go.mod` or `docroot/` and an upstream fix resolves the underlying issue, the corresponding `VulnerabilityWithoutRemediation` subtest will **fail**, signaling that the workaround can be retired. - -## Active Workarounds - -| Flag | Dependency | Issue | -| :--- | :--- | :--- | -| *(Retired in `v1.13.12`)* | `github.com/pkg/sftp` `v1.13.11` | Unchecked `unmarshalUint32` / `unmarshalString` slice bounds panics and `Client.RemoveAll` directory symlink traversal (`TestSFTPPacketGuard` and `TestSFTPRemoveAllSymlink` retained as regression tests). | -| `EnableSFTPResourceGuard` | `github.com/pkg/sftp` `v1.13.12` | Infinite read loop on 0-byte `SSH_FXP_DATA` in `File.readChunkAt`, infinite loop on 0-count `SSH_FXP_NAME`, and unbounded slice growth in `ReadDirContext`. | -| `EnableSSHAgentGuard` | `golang.org/x/crypto` `v0.58.0` | Unbounded concurrent `"auth-agent@openssh.com"` channels in `agent.ForwardToAgent` and 16 MiB per-channel buffer pre-allocation on a 4-byte length header in `agent.ServeAgent`. | -| `workarounds.csiRepGuard` | `@xterm/xterm` `5.5.0` | `CSI Ps b` (`repeatPrecedingCharacter`) uncaught `RangeError` permanently wedging `WriteBuffer`. | -| `workarounds.csiLoopGuard` | `@xterm/xterm` `5.5.0` | Unbounded $2^{31}-1$ synchronous `while (param--)` loops in `CSI I`, `Z`, `L`, `M`, `S`, and `T`. | - -## Retiring a Workaround After an Upstream Fix - -1. Upgrade the dependency and run `go test ./...` (and browser tests if applicable). -2. For any `VulnerabilityWithoutRemediation` test that fails because the upstream library now handles the case safely: - * Remove the `VulnerabilityWithoutRemediation` subtest, keeping the `RemediationEnabled` subtest as a regression test. - * Remove the corresponding `Enable...` flag and wrapper logic from `internal/depfix` (or `docroot/ssh.mjs`) and call the upstream API directly. diff --git a/go/internal/depfix/depfix.go b/go/internal/depfix/depfix.go deleted file mode 100644 index 5d03f4f..0000000 --- a/go/internal/depfix/depfix.go +++ /dev/null @@ -1,57 +0,0 @@ -// MIT License -// -// Copyright (c) 2026 TTBT Enterprises LLC -// Copyright (c) 2026 Robin Thellend -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in all -// copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -// Package depfix centralizes security workarounds and remediations for -// third-party dependencies so they can be tested in both vulnerable and -// remediated states and cleanly removed when fixed upstream. -package depfix - -var ( - // EnableSFTPResourceGuard rejects 0-byte SSH_FXP_DATA packets (which - // cause an infinite read loop in File.readChunkAt), 0-count - // SSH_FXP_NAME responses to SSH_FXP_READDIR, and caps cumulative - // SSH_FXP_READDIR entries per directory listing. - // Remove when github.com/pkg/sftp (v1.13.12) handles 0-byte - // SSH_FXP_DATA and bounds ReadDirContext. - EnableSFTPResourceGuard = true - - // EnableSSHAgentGuard bounds concurrent "auth-agent@openssh.com" - // channels and caps incoming agent request frames to 256 KiB before - // golang.org/x/crypto/ssh/agent (v0.58.0) ServeAgent pre-allocates up - // to 16 MiB per channel upon reading a 4-byte length header. - // Remove when golang.org/x/crypto/ssh/agent bounds channel concurrency - // and request pre-allocation. - EnableSSHAgentGuard = true -) - -// WithDisabled temporarily sets *flag to false while running fn, then restores -// its previous value so tests can verify both the underlying vulnerability and -// its remediation. -func WithDisabled(flag *bool, fn func()) { - prev := *flag - *flag = false - defer func() { - *flag = prev - }() - fn() -} diff --git a/go/internal/depfix/sftp.go b/go/internal/depfix/sftp.go deleted file mode 100644 index 3f506c6..0000000 --- a/go/internal/depfix/sftp.go +++ /dev/null @@ -1,274 +0,0 @@ -// MIT License -// -// Copyright (c) 2026 TTBT Enterprises LLC -// Copyright (c) 2026 Robin Thellend -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in all -// copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -package depfix - -import ( - "bytes" - "encoding/binary" - "errors" - "fmt" - "io" - "sync" - - "github.com/pkg/sftp" - "golang.org/x/crypto/ssh" -) - -const ( - sshFxpInit = 1 - sshFxpVersion = 2 - sshFxpClose = 4 - sshFxpOpendir = 11 - sshFxpReaddir = 12 - sshFxpStatus = 101 - sshFxpHandle = 102 - sshFxpData = 103 - sshFxpName = 104 - - maxSFTPPacketSize = 256 * 1024 -) - -var ( - // MaxSFTPReadDirEntries is the maximum number of directory entries - // permitted in a single directory listing when EnableSFTPResourceGuard - // is enabled. - MaxSFTPReadDirEntries = 100000 - - ErrMalformedSFTPPacket = errors.New("sftp: malformed packet from server") - ErrZeroLengthSFTPData = errors.New("sftp: zero-length SSH_FXP_DATA packet") - ErrTooManySFTPEntries = errors.New("sftp: directory listing exceeds maximum entry limit") -) - -// NewSFTPClient creates a new *sftp.Client over conn with resource guards -// applied when EnableSFTPResourceGuard is enabled. -func NewSFTPClient(conn *ssh.Client, opts ...sftp.ClientOption) (*sftp.Client, error) { - if !EnableSFTPResourceGuard { - return sftp.NewClient(conn, opts...) - } - s, err := conn.NewSession() - if err != nil { - return nil, err - } - pw, err := s.StdinPipe() - if err != nil { - s.Close() - return nil, err - } - pr, err := s.StdoutPipe() - if err != nil { - s.Close() - return nil, err - } - if err := s.RequestSubsystem("sftp"); err != nil { - s.Close() - return nil, err - } - sc := newSFTPSafeConn(pr, &sessionWriteCloser{WriteCloser: pw, sess: s}) - client, err := sftp.NewClientPipe(sc, sc, opts...) - if err != nil { - sc.Close() - return nil, err - } - return client, nil -} - -// NewSFTPClientPipe creates a new *sftp.Client over rd and wr with resource -// guards applied when EnableSFTPResourceGuard is enabled. -func NewSFTPClientPipe(rd io.Reader, wr io.WriteCloser, opts ...sftp.ClientOption) (*sftp.Client, error) { - if !EnableSFTPResourceGuard { - return sftp.NewClientPipe(rd, wr, opts...) - } - sc := newSFTPSafeConn(rd, wr) - return sftp.NewClientPipe(sc, sc, opts...) -} - -type sessionWriteCloser struct { - io.WriteCloser - sess *ssh.Session -} - -func (s *sessionWriteCloser) Close() error { - err := s.WriteCloser.Close() - _ = s.sess.Close() - return err -} - -type sftpSafeConn struct { - rd io.Reader - wr io.WriteCloser - buf bytes.Buffer - err error - - mu sync.Mutex - txBuf bytes.Buffer - readdirSIDs map[uint32]bool - readdirEntries int -} - -func newSFTPSafeConn(rd io.Reader, wr io.WriteCloser) *sftpSafeConn { - return &sftpSafeConn{ - rd: rd, - wr: wr, - readdirSIDs: make(map[uint32]bool), - } -} - -func (c *sftpSafeConn) Write(p []byte) (int, error) { - c.mu.Lock() - c.txBuf.Write(p) - for c.txBuf.Len() >= 5 { - hdr := c.txBuf.Bytes() - pktLen := binary.BigEndian.Uint32(hdr[:4]) - if pktLen == 0 || pktLen > maxSFTPPacketSize { - c.txBuf.Reset() - break - } - totalLen := int(pktLen) + 4 - if c.txBuf.Len() < totalLen { - break - } - frame := c.txBuf.Next(totalLen) - pktType := frame[4] - if len(frame) >= 9 { - sid := binary.BigEndian.Uint32(frame[5:9]) - switch pktType { - case sshFxpOpendir: - c.readdirEntries = 0 - case sshFxpReaddir: - c.readdirSIDs[sid] = true - case sshFxpClose: - c.readdirEntries = 0 - } - } - } - c.mu.Unlock() - return c.wr.Write(p) -} - -func (c *sftpSafeConn) Close() error { - return c.wr.Close() -} - -func (c *sftpSafeConn) Read(p []byte) (int, error) { - for c.buf.Len() == 0 { - if c.err != nil { - return 0, c.err - } - if err := c.readAndValidatePacket(); err != nil { - c.err = err - _ = c.wr.Close() - return 0, err - } - } - return c.buf.Read(p) -} - -func (c *sftpSafeConn) readAndValidatePacket() error { - var hdr [5]byte - if _, err := io.ReadFull(c.rd, hdr[:]); err != nil { - return err - } - length := binary.BigEndian.Uint32(hdr[:4]) - if length < 1 || length > maxSFTPPacketSize { - return fmt.Errorf("%w: invalid packet length %d", ErrMalformedSFTPPacket, length) - } - pktType := hdr[4] - payload := make([]byte, length-1) - if _, err := io.ReadFull(c.rd, payload); err != nil { - if errors.Is(err, io.EOF) { - return io.ErrUnexpectedEOF - } - return err - } - - if err := c.validatePacket(pktType, payload); err != nil { - return err - } - - c.buf.Write(hdr[:]) - c.buf.Write(payload) - return nil -} - -func (c *sftpSafeConn) isReaddirSID(sid uint32, remove bool) bool { - c.mu.Lock() - defer c.mu.Unlock() - ok := c.readdirSIDs[sid] - if ok && remove { - delete(c.readdirSIDs, sid) - } - return ok -} - -func (c *sftpSafeConn) addReaddirEntries(n int) error { - c.mu.Lock() - defer c.mu.Unlock() - c.readdirEntries += n - if c.readdirEntries > MaxSFTPReadDirEntries { - return ErrTooManySFTPEntries - } - return nil -} - -func (c *sftpSafeConn) validatePacket(pktType byte, data []byte) error { - if len(data) < 4 { - return nil - } - sid := binary.BigEndian.Uint32(data[:4]) - rest := data[4:] - - switch pktType { - case sshFxpStatus: - c.isReaddirSID(sid, true) - return nil - - case sshFxpData: - if !EnableSFTPResourceGuard { - return nil - } - if len(rest) >= 4 && binary.BigEndian.Uint32(rest[:4]) == 0 { - return ErrZeroLengthSFTPData - } - return nil - - case sshFxpName: - isReaddir := c.isReaddirSID(sid, true) - if !isReaddir || !EnableSFTPResourceGuard || len(rest) < 4 { - return nil - } - count := binary.BigEndian.Uint32(rest[:4]) - rest = rest[4:] - if count == 0 { - return fmt.Errorf("%w: zero count in SSH_FXP_NAME", ErrMalformedSFTPPacket) - } - // Each entry requires at least 12 bytes (filename len + longname len + attr flags). - if count > uint32(len(rest)/12) { - return nil - } - return c.addReaddirEntries(int(count)) - - default: - return nil - } -} diff --git a/go/internal/depfix/sftp_test.go b/go/internal/depfix/sftp_test.go deleted file mode 100644 index 7b08d28..0000000 --- a/go/internal/depfix/sftp_test.go +++ /dev/null @@ -1,294 +0,0 @@ -// MIT License -// -// Copyright (c) 2026 TTBT Enterprises LLC -// Copyright (c) 2026 Robin Thellend -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in all -// copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -package depfix - -import ( - "encoding/binary" - "errors" - "io" - "os" - "path/filepath" - "sync/atomic" - "testing" - - "github.com/pkg/sftp" -) - -func buildSFTPPacket(pktType byte, payload []byte) []byte { - length := uint32(1 + len(payload)) - out := make([]byte, 4+length) - binary.BigEndian.PutUint32(out[:4], length) - out[4] = pktType - copy(out[5:], payload) - return out -} - -func readClientPacket(r io.Reader) (byte, uint32, []byte, error) { - var hdr [5]byte - if _, err := io.ReadFull(r, hdr[:]); err != nil { - return 0, 0, nil, err - } - length := binary.BigEndian.Uint32(hdr[:4]) - pktType := hdr[4] - payload := make([]byte, length-1) - if _, err := io.ReadFull(r, payload); err != nil { - return 0, 0, nil, err - } - var sid uint32 - if len(payload) >= 4 { - sid = binary.BigEndian.Uint32(payload[:4]) - } - return pktType, sid, payload, nil -} - -// runMockSFTPSession starts a mock SFTP server over an io.Pipe that completes -// the SSH_FXP_INIT / SSH_FXP_VERSION handshake and then invokes handler for -// each request packet sent by the client. -func runMockSFTPSession(t *testing.T, handler func(pktType byte, sid uint32, wr io.Writer)) *sftp.Client { - t.Helper() - srvRd, cliWr := io.Pipe() - cliRd, srvWr := io.Pipe() - - go func() { - defer srvWr.Close() - defer srvRd.Close() - // Expect SSH_FXP_INIT - pktType, _, _, err := readClientPacket(srvRd) - if err != nil || pktType != sshFxpInit { - return - } - // Send SSH_FXP_VERSION (version 3) - ver := make([]byte, 4) - binary.BigEndian.PutUint32(ver, 3) - if _, err := srvWr.Write(buildSFTPPacket(sshFxpVersion, ver)); err != nil { - return - } - for { - pt, sid, _, err := readClientPacket(srvRd) - if err != nil { - return - } - handler(pt, sid, srvWr) - } - }() - - client, err := NewSFTPClientPipe(cliRd, cliWr) - if err != nil { - t.Fatalf("NewSFTPClientPipe: %v", err) - } - t.Cleanup(func() { - _ = client.Close() - }) - return client -} - -func TestSFTPPacketGuard(t *testing.T) { - cases := []struct { - name string - // respond writes a malformed response packet for sid. - respond func(sid uint32, wr io.Writer) - // invoke triggers the client operation that unmarshals the response. - invoke func(c *sftp.Client) error - }{ - { - name: "TruncatedStatus", - respond: func(sid uint32, wr io.Writer) { - // 4-byte sid only; missing 4-byte status code. - var b [4]byte - binary.BigEndian.PutUint32(b[:], sid) - _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) - }, - invoke: func(c *sftp.Client) error { - return c.Mkdir("/bad") - }, - }, - { - name: "TruncatedRealPathName", - respond: func(sid uint32, wr io.Writer) { - // count = 1, filename length = 100, but only 2 bytes follow. - var b [14]byte - binary.BigEndian.PutUint32(b[0:4], sid) - binary.BigEndian.PutUint32(b[4:8], 1) - binary.BigEndian.PutUint32(b[8:12], 100) - b[12] = 'a' - b[13] = 'b' - _, _ = wr.Write(buildSFTPPacket(sshFxpName, b[:])) - }, - invoke: func(c *sftp.Client) error { - _, err := c.Getwd() - return err - }, - }, - { - name: "TruncatedHandle", - respond: func(sid uint32, wr io.Writer) { - // handle length = 50, but 0 bytes follow. - var b [8]byte - binary.BigEndian.PutUint32(b[0:4], sid) - binary.BigEndian.PutUint32(b[4:8], 50) - _, _ = wr.Write(buildSFTPPacket(sshFxpHandle, b[:])) - }, - invoke: func(c *sftp.Client) error { - _, err := c.Open("/file") - return err - }, - }, - } - - for _, tc := range cases { - t.Run(tc.name+"/RemediationEnabled", func(t *testing.T) { - client := runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { - tc.respond(sid, wr) - }) - var err error - func() { - defer func() { - if r := recover(); r != nil { - t.Fatalf("unexpected panic: %v", r) - } - }() - err = tc.invoke(client) - }() - if err == nil { - t.Fatalf("expected error on %s, got nil", tc.name) - } - }) - } -} - -func TestSFTPRemoveAllSymlink(t *testing.T) { - tmpDir := t.TempDir() - targetDir := filepath.Join(tmpDir, "target") - if err := os.Mkdir(targetDir, 0o755); err != nil { - t.Fatal(err) - } - secretFile := filepath.Join(targetDir, "keep.txt") - if err := os.WriteFile(secretFile, []byte("important"), 0o600); err != nil { - t.Fatal(err) - } - linkPath := filepath.Join(tmpDir, "link_to_target") - if err := os.Symlink(targetDir, linkPath); err != nil { - t.Fatal(err) - } - - c1, s1 := io.Pipe() - s2, c2 := io.Pipe() - srv, err := sftp.NewServer(struct { - io.Reader - io.WriteCloser - }{c1, c2}) - if err != nil { - t.Fatal(err) - } - go func() { - _ = srv.Serve() - }() - t.Cleanup(func() { - _ = srv.Close() - }) - client, err := NewSFTPClientPipe(s2, s1) - if err != nil { - t.Fatal(err) - } - - if err := client.RemoveAll(linkPath); err != nil { - t.Fatalf("RemoveAll: %v", err) - } - if _, err := os.Lstat(linkPath); !errors.Is(err, os.ErrNotExist) { - t.Errorf("expected symlink %s to be removed, err = %v", linkPath, err) - } - if _, err := os.Stat(secretFile); err != nil { - t.Errorf("expected target file %s to remain intact, err = %v", secretFile, err) - } -} - -func TestSFTPResourceGuard(t *testing.T) { - // Zero-length SSH_FXP_DATA infinite read loop test. - makeZeroDataServer := func(t *testing.T, readCalls *atomic.Int32) *sftp.Client { - return runMockSFTPSession(t, func(pktType byte, sid uint32, wr io.Writer) { - switch pktType { - case 3: // SSH_FXP_OPEN - var b [9]byte - binary.BigEndian.PutUint32(b[0:4], sid) - binary.BigEndian.PutUint32(b[4:8], 1) - b[8] = 'f' - _, _ = wr.Write(buildSFTPPacket(sshFxpHandle, b[:])) - case 5: // SSH_FXP_READ - n := readCalls.Add(1) - if n >= 5 { - // Stop the infinite loop in vulnerable mode after 5 iterations. - var b [16]byte - binary.BigEndian.PutUint32(b[0:4], sid) - binary.BigEndian.PutUint32(b[4:8], 1) // SSH_FX_EOF - _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) - return - } - // Respond with 0-byte SSH_FXP_DATA. - var b [8]byte - binary.BigEndian.PutUint32(b[0:4], sid) - binary.BigEndian.PutUint32(b[4:8], 0) - _, _ = wr.Write(buildSFTPPacket(sshFxpData, b[:])) - default: - var b [16]byte - binary.BigEndian.PutUint32(b[0:4], sid) - _, _ = wr.Write(buildSFTPPacket(sshFxpStatus, b[:])) - } - }) - } - - t.Run("ZeroByteData/VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableSFTPResourceGuard, func() { - var readCalls atomic.Int32 - client := makeZeroDataServer(t, &readCalls) - f, err := client.Open("/zero") - if err != nil { - t.Fatalf("Open: %v", err) - } - defer f.Close() - buf := make([]byte, 16) - _, _ = f.Read(buf) - if got := readCalls.Load(); got < 5 { - t.Fatalf("expected pkg/sftp to loop on 0-byte SSH_FXP_DATA (readCalls=%d, want >=5); if fixed upstream, remove EnableSFTPResourceGuard", got) - } - }) - }) - - t.Run("ZeroByteData/RemediationEnabled", func(t *testing.T) { - var readCalls atomic.Int32 - client := makeZeroDataServer(t, &readCalls) - f, err := client.Open("/zero") - if err != nil { - t.Fatalf("Open: %v", err) - } - defer f.Close() - buf := make([]byte, 16) - _, err = f.Read(buf) - if err == nil { - t.Fatal("expected error on 0-byte SSH_FXP_DATA with EnableSFTPResourceGuard enabled") - } - if got := readCalls.Load(); got != 1 { - t.Fatalf("readCalls = %d, want 1", got) - } - }) -} diff --git a/go/internal/depfix/ssh.go b/go/internal/depfix/ssh.go deleted file mode 100644 index f589b0b..0000000 --- a/go/internal/depfix/ssh.go +++ /dev/null @@ -1,143 +0,0 @@ -// MIT License -// -// Copyright (c) 2026 TTBT Enterprises LLC -// Copyright (c) 2026 Robin Thellend -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in all -// copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -package depfix - -import ( - "bytes" - "encoding/binary" - "errors" - "fmt" - "io" - - "golang.org/x/crypto/ssh" - "golang.org/x/crypto/ssh/agent" -) - -var ( - // MaxConcurrentAgentChannels is the maximum number of concurrent - // "auth-agent@openssh.com" channels accepted from a remote SSH server - // when EnableSSHAgentGuard is true. - MaxConcurrentAgentChannels = 8 - - // MaxAgentRequestBytes is the maximum frame size accepted on a - // forwarded agent channel before passing the request to - // agent.ServeAgent (which otherwise pre-allocates up to 16 MiB per - // channel upon reading a 4-byte length header). - MaxAgentRequestBytes uint32 = 256 * 1024 - - ErrAgentRequestTooLarge = errors.New("ssh/agent: request exceeds maximum allowed size") -) - -// ForwardToAgent routes incoming "auth-agent@openssh.com" channels on client to -// keyring. When EnableSSHAgentGuard is true, it limits concurrent agent -// channels and caps request frame sizes to MaxAgentRequestBytes. -func ForwardToAgent(client *ssh.Client, keyring agent.Agent) error { - if !EnableSSHAgentGuard { - return agent.ForwardToAgent(client, keyring) - } - channels := client.HandleChannelOpen("auth-agent@openssh.com") - if channels == nil { - return errors.New("agent: already have handler for auth-agent@openssh.com") - } - sem := make(chan struct{}, MaxConcurrentAgentChannels) - go func() { - for newChan := range channels { - select { - case sem <- struct{}{}: - default: - _ = newChan.Reject(ssh.ResourceShortage, "too many concurrent agent channels") - continue - } - ch, reqs, err := newChan.Accept() - if err != nil { - <-sem - continue - } - go ssh.DiscardRequests(reqs) - go io.Copy(io.Discard, ch.Stderr()) - go func(c ssh.Channel) { - defer func() { - _ = c.Close() - <-sem - }() - _ = ServeAgent(keyring, c) - }(ch) - } - }() - return nil -} - -// ServeAgent serves the SSH agent protocol over c. When EnableSSHAgentGuard is -// true, it wraps c in an agentSafeChannel that enforces MaxAgentRequestBytes -// before agent.ServeAgent pre-allocates the request buffer. -func ServeAgent(keyring agent.Agent, c io.ReadWriter) error { - if !EnableSSHAgentGuard { - return agent.ServeAgent(keyring, c) - } - return agent.ServeAgent(keyring, &agentSafeChannel{rw: c}) -} - -type agentSafeChannel struct { - rw io.ReadWriter - buf bytes.Buffer - err error -} - -func (a *agentSafeChannel) Write(p []byte) (int, error) { - return a.rw.Write(p) -} - -func (a *agentSafeChannel) Read(p []byte) (int, error) { - for a.buf.Len() == 0 { - if a.err != nil { - return 0, a.err - } - if err := a.readNextFrame(); err != nil { - a.err = err - return 0, err - } - } - return a.buf.Read(p) -} - -func (a *agentSafeChannel) readNextFrame() error { - var hdr [4]byte - if _, err := io.ReadFull(a.rw, hdr[:]); err != nil { - return err - } - length := binary.BigEndian.Uint32(hdr[:]) - if length == 0 || length > MaxAgentRequestBytes { - return fmt.Errorf("%w: %d bytes", ErrAgentRequestTooLarge, length) - } - payload := make([]byte, length) - if _, err := io.ReadFull(a.rw, payload); err != nil { - if errors.Is(err, io.EOF) { - return io.ErrUnexpectedEOF - } - return err - } - a.buf.Write(hdr[:]) - a.buf.Write(payload) - return nil -} diff --git a/go/internal/depfix/ssh_test.go b/go/internal/depfix/ssh_test.go deleted file mode 100644 index 3799d54..0000000 --- a/go/internal/depfix/ssh_test.go +++ /dev/null @@ -1,91 +0,0 @@ -// MIT License -// -// Copyright (c) 2026 TTBT Enterprises LLC -// Copyright (c) 2026 Robin Thellend -// -// Permission is hereby granted, free of charge, to any person obtaining a copy -// of this software and associated documentation files (the "Software"), to deal -// in the Software without restriction, including without limitation the rights -// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell -// copies of the Software, and to permit persons to whom the Software is -// furnished to do so, subject to the following conditions: -// -// The above copyright notice and this permission notice shall be included in all -// copies or substantial portions of the Software. -// -// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR -// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, -// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE -// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER -// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, -// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE -// SOFTWARE. - -package depfix - -import ( - "encoding/binary" - "errors" - "net" - "testing" - "time" - - "golang.org/x/crypto/ssh/agent" -) - -type nopAgent struct { - agent.Agent -} - -func (nopAgent) List() ([]*agent.Key, error) { - return nil, nil -} - -func TestSSHAgentGuard(t *testing.T) { - // Oversized agent frame length header (> MaxAgentRequestBytes). - t.Run("OversizedFrame/VulnerabilityWithoutRemediation", func(t *testing.T) { - WithDisabled(&EnableSSHAgentGuard, func() { - c1, s1 := net.Pipe() - defer c1.Close() - defer s1.Close() - errCh := make(chan error, 1) - go func() { - errCh <- ServeAgent(nopAgent{}, s1) - }() - // Send a 4-byte length header of MaxAgentRequestBytes + 100 (still <= 16 MiB). - var hdr [4]byte - binary.BigEndian.PutUint32(hdr[:], MaxAgentRequestBytes+100) - if _, err := c1.Write(hdr[:]); err != nil { - t.Fatal(err) - } - // ServeAgent pre-allocates make([]byte, l) and blocks in io.ReadFull waiting for payload. - select { - case err := <-errCh: - t.Fatalf("expected ServeAgent to accept header > MaxAgentRequestBytes and wait for payload, but got %v", err) - case <-time.After(50 * time.Millisecond): - _ = c1.Close() - } - }) - }) - - t.Run("OversizedFrame/RemediationEnabled", func(t *testing.T) { - c1, s1 := net.Pipe() - defer c1.Close() - defer s1.Close() - errCh := make(chan error, 1) - go func() { - errCh <- ServeAgent(nopAgent{}, s1) - }() - var hdr [4]byte - binary.BigEndian.PutUint32(hdr[:], MaxAgentRequestBytes+100) - _, _ = c1.Write(hdr[:]) - select { - case err := <-errCh: - if !errors.Is(err, ErrAgentRequestTooLarge) { - t.Fatalf("expected ErrAgentRequestTooLarge, got %v", err) - } - case <-time.After(time.Second): - t.Fatal("timed out waiting for ServeAgent to reject oversized frame header") - } - }) -} From f02b52be7ffe5caf15437157a05461e5d9dfae92 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Sun, 25 Oct 2026 08:51:17 -0700 Subject: [PATCH 5/5] Address code review feedback on terminal reset and xterm workarounds --- CHANGELOG.md | 1 - docroot/ssh.mjs | 46 +++++++++-------------- go/build.sh | 2 +- go/internal/terminal/terminal.go | 11 +++--- go/sshterm-server/internal/server_test.go | 24 +----------- 5 files changed, 26 insertions(+), 58 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index caf2316..04d3adb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,6 @@ ### :lock: Security fixes -* Align `go/go.mod` (`go 1.27.3`) and `go/build.sh` (`GOTOOLCHAIN`) with `.goversion` so local builds do not fall back to Go 1.26.0, and remove stale `docroot/xterm.js` from the embedded server docroot. * Upgrade `github.com/pkg/sftp` to `v1.13.12` (fixing truncated packet unmarshaling panics and `Client.RemoveAll` directory symlink traversal). * Harden SSH client and forwarded agent (`golang.org/x/crypto/ssh`): * Implement `ssh.MultiAlgorithmSigner` on `dynSigner` and `agent.ExtendedAgent` (`SignWithFlags`) on `keyRing` and `forwardedAgent` so RSA keys and user certificates use `rsa-sha2-256` / `rsa-sha2-512` instead of falling back to deprecated SHA-1 (`ssh-rsa`). diff --git a/docroot/ssh.mjs b/docroot/ssh.mjs index 7b5c6d5..5cc7e9d 100644 --- a/docroot/ssh.mjs +++ b/docroot/ssh.mjs @@ -49,32 +49,20 @@ export function sanitizeTitle(title) { export function applyXtermWorkarounds(term, flags = workarounds) { const disposables = []; const registerClampCsi = (final, getMax) => { - if (term._core && typeof term._core.registerCsiHandler === 'function') { - disposables.push( - term._core.registerCsiHandler({ final }, params => { - const active = final === 'b' ? flags.csiRepGuard : flags.csiLoopGuard; - if (!active) return false; - const maxVal = Math.max(1, getMax()); - if (params.length > 0 && params.params[0] > maxVal) { - params.params[0] = maxVal; - } - return false; - }) - ); - } else { - disposables.push( - term.parser.registerCsiHandler({ final }, params => { - const active = final === 'b' ? flags.csiRepGuard : flags.csiLoopGuard; - if (!active) return false; - const maxVal = Math.max(1, getMax()); - const p0 = typeof params[0] === 'number' ? params[0] : 0; - return p0 > maxVal; - }) - ); - } + disposables.push( + term._core.registerCsiHandler({ final }, params => { + const active = final === 'b' ? flags.csiRepGuard : flags.csiLoopGuard; + if (!active) return false; + const maxVal = Math.max(1, getMax()); + if (params.length > 0 && params.params[0] > maxVal) { + params.params[0] = maxVal; + } + return false; + }) + ); }; - registerClampCsi('b', () => term.cols); + registerClampCsi('b', () => term.cols * term.rows); registerClampCsi('I', () => term.cols); registerClampCsi('Z', () => term.cols); registerClampCsi('L', () => term.rows); @@ -236,7 +224,7 @@ export class TabManager { this.screens[b.id].title = 'sshterm'; const setTitle = rawTitle => { - const title = sanitizeTitle(rawTitle); + const title = sanitizeTitle(rawTitle) || 'sshterm'; this.screens[b.id].title = title; this.screens[b.id].b.title = title; document.querySelector('head title').textContent = title; @@ -314,9 +302,11 @@ export class TabManager { } window.sshApp = {}; -window.sshApp.workarounds = workarounds; -window.sshApp.applyXtermWorkarounds = applyXtermWorkarounds; -window.sshApp.Terminal = Terminal; +if (isTest()) { + window.sshApp.workarounds = workarounds; + window.sshApp.applyXtermWorkarounds = applyXtermWorkarounds; + window.sshApp.Terminal = Terminal; +} window.sshApp.exited = null; window.sshApp.ready = new Promise(resolve => { window.sshApp.sshIsReady = () => { diff --git a/go/build.sh b/go/build.sh index 259baa8..c70fa38 100755 --- a/go/build.sh +++ b/go/build.sh @@ -11,7 +11,7 @@ cp -f $(go env GOROOT)/lib/wasm/wasm_exec.js ../docroot/ rm -rf sshterm-server/internal/docroot mkdir -p sshterm-server/internal/docroot cp -r ../docroot/* sshterm-server/internal/docroot/ -rm -f sshterm-server/internal/docroot/tests.* sshterm-server/internal/docroot/config.json* sshterm-server/internal/docroot/xterm.js +rm -f sshterm-server/internal/docroot/tests.* sshterm-server/internal/docroot/config.json* export CGO_ENABLED=0 go build -trimpath -ldflags="-extldflags=-s -w" -o ../sshterm-server ./sshterm-server diff --git a/go/internal/terminal/terminal.go b/go/internal/terminal/terminal.go index e7649e8..3374258 100644 --- a/go/internal/terminal/terminal.go +++ b/go/internal/terminal/terminal.go @@ -319,17 +319,16 @@ func (t *Terminal) Clear() { // a remote session and drains any pending asynchronous query responses from the // input buffer before returning to the local prompt. func (t *Terminal) ResetSessionState() { + _, _ = t.tw.Write([]byte( + "\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l" + + "\x1b[?47l\x1b[!p" + + "\x1b]104\x07\x1b]110\x07\x1b]111\x07\x1b]112\x07", + )) for { select { case <-t.tw.dataCh: default: t.tw.r = nil - _, _ = t.tw.Write([]byte( - "\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l" + - "\x1b[?1004l\x1b[?2004l\x1b[?6l\x1b[r\x1b[?1049l" + - "\x1b[0m\x1b[?25h" + - "\x1b]104\x07\x1b]110\x07\x1b]111\x07\x1b]112\x07", - )) return } } diff --git a/go/sshterm-server/internal/server_test.go b/go/sshterm-server/internal/server_test.go index a93bfe5..d8bee11 100644 --- a/go/sshterm-server/internal/server_test.go +++ b/go/sshterm-server/internal/server_test.go @@ -44,7 +44,6 @@ import ( "net/http/httptest" "os" "path/filepath" - "runtime" "strings" "sync" "testing" @@ -206,8 +205,8 @@ func TestEmbeddedStaticFilesAndHeaders(t *testing.T) { } } - // Ensure test artifacts, example config, and legacy xterm.js are not embedded. - for _, excluded := range []string{"/tests.html", "/tests.wasm", "/config.json.example", "/xterm.js"} { + // Ensure test artifacts and example config are not embedded. + for _, excluded := range []string{"/tests.html", "/tests.wasm", "/config.json.example"} { resp, err := http.Get(ts.URL + excluded) if err != nil { t.Fatalf("GET %s: %v", excluded, err) @@ -655,25 +654,6 @@ func (c *wsNetConn) SetDeadline(t time.Time) error { return nil } func (c *wsNetConn) SetReadDeadline(t time.Time) error { return c.conn.SetReadDeadline(t) } func (c *wsNetConn) SetWriteDeadline(t time.Time) error { return c.conn.SetWriteDeadline(t) } -func TestGoToolchainVersion(t *testing.T) { - goversionBytes, err := os.ReadFile("../../../.goversion") - if err != nil { - t.Fatalf("ReadFile(.goversion): %v", err) - } - wantVer := strings.TrimSpace(string(goversionBytes)) - - gomodBytes, err := os.ReadFile("../../go.mod") - if err != nil { - t.Fatalf("ReadFile(go.mod): %v", err) - } - if !strings.Contains(string(gomodBytes), "go "+wantVer+"\n") { - t.Errorf("go/go.mod does not declare 'go %s'", wantVer) - } - if got := runtime.Version(); !strings.HasPrefix(got, "go"+wantVer) { - t.Errorf("runtime.Version() = %q, want prefix %q", got, "go"+wantVer) - } -} - type stallTrackConn struct { net.Conn mu sync.Mutex