From ed786bbbcd2a5cc0783e0d2653f639d817f12d15 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Fri, 16 Oct 2026 10:20:11 -0700 Subject: [PATCH 1/2] Fix issues from security review --- CHANGELOG.md | 23 +++ docroot/ssh.mjs | 3 +- go/internal/app/agent.go | 91 +++++++--- go/internal/app/app.go | 21 ++- go/internal/app/ca.go | 2 +- go/internal/app/cert_util.go | 30 +++- go/internal/app/cert_util_test.go | 38 +++- go/internal/app/db.go | 56 +++++- go/internal/app/hosts.go | 8 +- go/internal/app/keys.go | 64 +++++-- go/internal/app/sftp.go | 119 +++++++------ go/internal/app/ssh.go | 35 ++-- go/internal/indexeddb/indexeddb.go | 54 ++++-- go/internal/jsutil/jsutil.go | 33 ++-- go/internal/jsutil/streams.go | 29 +++- go/internal/terminal/terminal.go | 37 +++- go/internal/tests/app_test.go | 60 ++++++- go/internal/tests/jsutil_test.go | 17 ++ go/internal/tests/malformed_test.go | 12 ++ go/internal/tests/paste_test.go | 4 +- go/internal/tests/preset_test.go | 5 +- go/internal/testserver/main_test.go | 11 +- go/internal/webauthnsk/key.go | 220 +----------------------- go/internal/webauthnsk/webauthn.go | 214 +++++++++++++++++++++++ go/internal/webauthnsk/webauthn_test.go | 112 ++++++++++++ go/internal/websocket/websocket.go | 51 ++++-- go/internal/zmodem/filter.go | 5 +- go/internal/zmodem/filter_test.go | 91 ++++++++++ go/internal/zmodem/receive.go | 3 + go/internal/zmodem/session_test.go | 21 +++ xterm/update.sh | 2 +- 31 files changed, 1046 insertions(+), 425 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4a4851c..da165e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ ## next +### :lock: Security fixes + +* Only trust a host certificate when the Certificate Authority is trusted for the certificate's hostname, not the endpoint alias name, and only offer principals in the host-key prompt for which the CA is trusted. +* Reject certificates with `ValidBefore == 0` as expired, while continuing to accept `ssh.CertTimeInfinity` (`0xffffffffffffffff`). +* Expand SFTP wildcard patterns after CLI flag parsing so remote file names starting with `-` cannot inject command flags, and strip directory components from uploaded local file names. +* Require confirmation when `CTRL-ALT-W` triggers `db wipe`, and only process keyboard shortcuts at the top-level `sshterm>` prompt. +* Clear keys from the in-memory SSH agent on `db wipe`, `db restore`, `keys delete`, `keys generate`, `keys discover`, and `keys import`. +* Bind the WebAuthn public key blob as AES-GCM additional authenticated data (AAD) in encrypted WebAuthn key files (existing v1 keys can still be loaded), verify the registration challenge in `WebAuthn.Create`, and enforce a minimum of 1,000 PBKDF2 iterations on encrypted WebAuthn keys and database backups. +* Sanitize certificate critical option names, certificate comments in `keys show`, and error messages in the host-key prompt, and strip 8-bit `CSI` (`\x9b`) characters from pasted text. +* Show a popup notification in addition to the terminal message when a forwarded agent signs a request. + +### :wrench: Bug fixes + +* Fix `agent remove ` for WebAuthn (`ecdsa-sk`) keys and deleted keys, and avoid holding the agent lock while fetching user certificates from an identity provider. +* Fix `caTrustedFor` and host-certificate trust updates when the hostname includes a `:port` suffix (such as jump hosts). +* Fix `db restore` when restoring empty maps, keys with identity providers, or backups with `persist: false`, and respect `config.json`'s `persist` setting. +* Keep `sftp get` working across multiple screens when one screen exits. +* Prevent `autoConnect` from overwriting keys and settings saved by other open tabs. +* Fix a race condition when aborting a ZMODEM transfer with `Ctrl-C`, and reject negative file sizes in ZMODEM `ZFILE` headers. +* Fix WebSocket connection cancellation and release `js.Func` callbacks across `websocket`, `terminal`, `jsutil`, `indexeddb`, `sftp`, and `app`. + +### :wrench: Misc + * Update go 1.27.2 * Update go dependencies: * upgraded github.com/fxamacker/cbor/v2 v2.9.4 => v2.9.6 diff --git a/docroot/ssh.mjs b/docroot/ssh.mjs index c8cbc39..9c6fdd1 100644 --- a/docroot/ssh.mjs +++ b/docroot/ssh.mjs @@ -56,7 +56,7 @@ class TerminalManager { // Remove escape characters from pasted text so that it can't end // bracketed paste mode early, e.g. with "\x1b[201~some command\r". - const paste = t => this.term.paste(t.replace(/\x1b/g, '')); + const paste = t => this.term.paste(t.replace(/[\x1b\x9b]/g, '')); this.contextmenuHandler = event => { event.preventDefault(); event.stopPropagation(); @@ -194,6 +194,7 @@ export class TabManager { this.selectScreen(b.id); const term = terminalManager.getTerm(); + term.showMessage = text => this._showMessage(e, text); const cfg = await fetch('config.json') .then(r => { if (r.ok) return r.json(); diff --git a/go/internal/app/agent.go b/go/internal/app/agent.go index 125fe38..35b5f91 100644 --- a/go/internal/app/agent.go +++ b/go/internal/app/agent.go @@ -116,15 +116,17 @@ func (a *App) agentCommand() *cli.App { return nil } name := ctx.Args().Get(0) - key, exists := a.data.Keys[name] - if !exists { - return fmt.Errorf("key %q not found", name) + err := globalAgent.(*keyRing).RemoveByName(name) + if key, exists := a.data.Keys[name]; exists { + pub, parseErr := key.sshPublicKey() + if parseErr != nil { + return fmt.Errorf("sshPublicKey: %w", parseErr) + } + if rmErr := globalAgent.Remove(pub); rmErr == nil { + err = nil + } } - pub, err := ssh.ParsePublicKey(key.Public) if err != nil { - return fmt.Errorf("ssh.ParsePublicKey: %w", err) - } - if err := globalAgent.Remove(pub); err != nil { return fmt.Errorf("agent.Remove: %w", err) } return nil @@ -221,14 +223,44 @@ type agentKey struct { comment string } +func rawPublicKey(s ssh.Signer) ssh.PublicKey { + if ds, ok := s.(*dynSigner); ok { + return ds.Signer.PublicKey() + } + pub := s.PublicKey() + if cert, ok := pub.(*ssh.Certificate); ok { + return cert.Key + } + return pub +} + +func matchesKey(s ssh.Signer, v []byte) bool { + if ds, ok := s.(*dynSigner); ok { + if bytes.Equal(v, ds.Signer.PublicKey().Marshal()) { + return true + } + if cert := ds.key.parseCert(); cert != nil && bytes.Equal(v, cert.Marshal()) { + return true + } + return false + } + pub := s.PublicKey() + if cert, ok := pub.(*ssh.Certificate); ok && bytes.Equal(v, cert.Key.Marshal()) { + return true + } + return bytes.Equal(v, pub.Marshal()) +} + func (r *keyRing) List() ([]*agent.Key, error) { r.mu.Lock() - defer r.mu.Unlock() if r.locked { + r.mu.Unlock() return nil, nil } - out := make([]*agent.Key, 0, len(r.keys)) - for _, k := range r.keys { + keys := slices.Clone(r.keys) + r.mu.Unlock() + out := make([]*agent.Key, 0, len(keys)) + for _, k := range keys { pub := k.signer.PublicKey() out = append(out, &agent.Key{ Format: pub.Type(), @@ -247,7 +279,7 @@ func (r *keyRing) Sign(key ssh.PublicKey, data []byte) (*ssh.Signature, error) { } v := key.Marshal() i := slices.IndexFunc(r.keys, func(k agentKey) bool { - return bytes.Equal(v, k.signer.PublicKey().Marshal()) + return matchesKey(k.signer, v) }) if i < 0 { r.mu.Unlock() @@ -266,15 +298,9 @@ func (r *keyRing) AddSigner(signer ssh.Signer, comment string) error { if r.locked { return errAgentLocked } - realPub := func(pub ssh.PublicKey) ssh.PublicKey { - if cert, ok := pub.(*ssh.Certificate); ok { - return cert.Key - } - return pub - } - pub := realPub(signer.PublicKey()).Marshal() + pub := rawPublicKey(signer).Marshal() for i, k := range r.keys { - kPub := realPub(k.signer.PublicKey()).Marshal() + kPub := rawPublicKey(k.signer).Marshal() if !bytes.Equal(pub, kPub) { continue } @@ -302,12 +328,27 @@ func (r *keyRing) Remove(key ssh.PublicKey) error { v := key.Marshal() found := false r.keys = slices.DeleteFunc(r.keys, func(k agentKey) bool { - pub := k.signer.PublicKey() - if cert, ok := pub.(*ssh.Certificate); ok && bytes.Equal(v, cert.Key.Marshal()) { + if matchesKey(k.signer, v) { found = true return true } - if bytes.Equal(v, pub.Marshal()) { + return false + }) + if !found { + return errAgentKeyNotFound + } + return nil +} + +func (r *keyRing) RemoveByName(name string) error { + r.mu.Lock() + defer r.mu.Unlock() + if r.locked { + return errAgentLocked + } + found := false + r.keys = slices.DeleteFunc(r.keys, func(k agentKey) bool { + if k.comment == name { found = true return true } @@ -329,6 +370,12 @@ func (r *keyRing) RemoveAll() error { return nil } +func (r *keyRing) ForceRemoveAll() { + r.mu.Lock() + defer r.mu.Unlock() + r.keys = nil +} + func (r *keyRing) Lock(passphrase []byte) error { r.mu.Lock() defer r.mu.Unlock() diff --git a/go/internal/app/app.go b/go/internal/app/app.go index 4d37d86..aea7b84 100644 --- a/go/internal/app/app.go +++ b/go/internal/app/app.go @@ -76,7 +76,8 @@ func New(cfg *Config) (*App, error) { Keys: make(map[string]*key), Params: make(map[string]any), }, - inShell: new(atomic.Bool), + inShell: new(atomic.Bool), + atPrompt: new(atomic.Bool), } app.commands = []*cli.App{ { @@ -134,6 +135,7 @@ type App struct { streamHelper *jsutil.StreamHelper inShell *atomic.Bool + atPrompt *atomic.Bool presetDone bool } @@ -306,8 +308,12 @@ func (a *App) Run() error { if broadcastChannel := js.Global().Get("BroadcastChannel"); !broadcastChannel.IsUndefined() { a.bc = broadcastChannel.New("update:" + a.cfg.DBName) - defer a.bc.Call("close") - a.bc.Set("onmessage", js.FuncOf(a.onBroadcastDBChange)) + bcCb := js.FuncOf(a.onBroadcastDBChange) + defer func() { + a.bc.Call("close") + bcCb.Release() + }() + a.bc.Set("onmessage", bcCb) } if err := a.initDB(); err != nil { @@ -321,12 +327,10 @@ func (a *App) Run() error { } else if theme := a.cfg.Theme; theme != "" { a.setTheme(theme) } - jsutil.UnregisterServiceWorker() defer func() { if a.db != nil { a.db.Close() } - jsutil.UnregisterServiceWorker() }() if a.cfg.AutoConnect != nil { @@ -363,10 +367,10 @@ func (a *App) Run() error { "\x1bp": {"db persist toggle\r", "ALT-P"}, "\x1bb": {"db restore\r", "ALT-B"}, "\x1b\x02": {"db backup\r", "CTRL-ALT-B"}, - "\x1b\x17": {"db wipe\rYES\r", "CTRL-ALT-W"}, + "\x1b\x17": {"db wipe\r", "CTRL-ALT-W"}, } done := t.OnData(func(k string) any { - if a.inShell.Load() { + if a.inShell.Load() || !a.atPrompt.Load() { return nil } if k == "\x12" { @@ -391,7 +395,9 @@ func (a *App) Run() error { } for { + a.atPrompt.Store(true) line, err := t.ReadLine() + a.atPrompt.Store(false) if err != nil { return err } @@ -483,6 +489,7 @@ func (a *App) ctrlC(cancel context.CancelFunc) context.CancelFunc { func (a *App) broadcastDBChange() { globalLastDBChange = time.Now().UTC() + a.lastDBRefresh = globalLastDBChange if a.bc.IsUndefined() { return } diff --git a/go/internal/app/ca.go b/go/internal/app/ca.go index 299cbe2..8431c41 100644 --- a/go/internal/app/ca.go +++ b/go/internal/app/ca.go @@ -51,7 +51,7 @@ func (a *App) addAuthority(name, publicKey string, hostnames []string) error { Name: name, Fingerprint: fp, Public: key.Marshal(), - Hostnames: hostnames, + Hostnames: slices.Clone(hostnames), } return nil } diff --git a/go/internal/app/cert_util.go b/go/internal/app/cert_util.go index 0962bab..88d7fbc 100644 --- a/go/internal/app/cert_util.go +++ b/go/internal/app/cert_util.go @@ -27,10 +27,13 @@ import ( "errors" "fmt" "net" + "path" "slices" "time" "golang.org/x/crypto/ssh" + + "github.com/c2FmZQ/sshterm/internal/termsafe" ) func checkCertificate(cert *ssh.Certificate, certType uint32) error { @@ -42,11 +45,16 @@ func checkCertificate(cert *ssh.Certificate, certType uint32) error { if cert.ValidAfter > now { errs = append(errs, fmt.Errorf("certificate is not yet valid")) } - if cert.ValidBefore > 0 && now > cert.ValidBefore { + if cert.ValidBefore != ssh.CertTimeInfinity && (now > cert.ValidBefore || cert.ValidBefore < cert.ValidAfter) { errs = append(errs, fmt.Errorf("certificate is expired")) } if certType == ssh.HostCert && len(cert.CriticalOptions) > 0 { - errs = append(errs, fmt.Errorf("certificate has critical options: %v", cert.CriticalOptions)) + var opts []string + for k := range cert.CriticalOptions { + opts = append(opts, termsafe.Name(k)) + } + slices.Sort(opts) + errs = append(errs, fmt.Errorf("certificate has critical options: %v", opts)) } c2 := *cert c2.Signature = nil @@ -72,3 +80,21 @@ func checkHostCertPrincipal(cert *ssh.Certificate, hostname string) error { } return fmt.Errorf("host certificate is not valid for hostname %q", hostname) } + +// matchHostnames returns true if any of names (or its host part when formatted +// as host:port) matches any of patterns. +func matchHostnames(patterns []string, names ...string) bool { + for _, pat := range patterns { + for _, n := range names { + if matched, err := path.Match(pat, n); err == nil && matched { + return true + } + if h, _, err := net.SplitHostPort(n); err == nil { + if matched, err := path.Match(pat, h); err == nil && matched { + return true + } + } + } + } + return false +} diff --git a/go/internal/app/cert_util_test.go b/go/internal/app/cert_util_test.go index 5b057f1..b24f249 100644 --- a/go/internal/app/cert_util_test.go +++ b/go/internal/app/cert_util_test.go @@ -26,6 +26,7 @@ package app import ( "crypto/ed25519" "crypto/rand" + "strings" "testing" "time" @@ -56,16 +57,17 @@ func TestCheckCertificate(t *testing.T) { badSig bool expectErr bool }{ - {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert}}, + {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: ssh.CertTimeInfinity}}, {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidAfter: earlier, ValidBefore: later}}, - {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidAfter: earlier}}, + {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidAfter: earlier, ValidBefore: ssh.CertTimeInfinity}}, {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: later}}, - {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidAfter: later}, expectErr: true}, + {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: 0}, expectErr: true}, + {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidAfter: later, ValidBefore: ssh.CertTimeInfinity}, expectErr: true}, {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: earlier}, expectErr: true}, - {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert}, badSig: true, expectErr: true}, - {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, Permissions: ssh.Permissions{CriticalOptions: map[string]string{"foo": ""}}}, expectErr: true}, - {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert}, certType: ssh.UserCert, expectErr: true}, - {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.UserCert}, certType: ssh.HostCert, expectErr: true}, + {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: ssh.CertTimeInfinity}, badSig: true, expectErr: true}, + {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: ssh.CertTimeInfinity, Permissions: ssh.Permissions{CriticalOptions: map[string]string{"foo\x1b[31m": "bar\x1b[32m"}}}, expectErr: true}, + {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidBefore: ssh.CertTimeInfinity}, certType: ssh.UserCert, expectErr: true}, + {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.UserCert, ValidBefore: ssh.CertTimeInfinity}, certType: ssh.HostCert, expectErr: true}, {cert: &ssh.Certificate{Key: sshPub, CertType: ssh.HostCert, ValidAfter: later, ValidBefore: earlier}, badSig: true, expectErr: true}, } { tc.cert.SignCert(rand.Reader, signer) @@ -80,6 +82,9 @@ func TestCheckCertificate(t *testing.T) { t.Errorf("[#%d] checkCertificate(): %v", i, err) continue } + if err != nil && strings.Contains(err.Error(), "\x1b") { + t.Errorf("[#%d] checkCertificate() error contains unsanitized escape sequence: %q", i, err.Error()) + } t.Logf("[#%d] OK checkCertificate(): %v", i, err) } } @@ -105,3 +110,22 @@ func TestCheckHostCertPrincipal(t *testing.T) { } } } + +func TestMatchHostnames(t *testing.T) { + for i, tc := range []struct { + patterns []string + names []string + want bool + }{ + {patterns: []string{"foo.example.com"}, names: []string{"foo.example.com"}, want: true}, + {patterns: []string{"foo.example.com"}, names: []string{"foo.example.com:2222"}, want: true}, + {patterns: []string{"*.example.com"}, names: []string{"bar.example.com:22"}, want: true}, + {patterns: []string{"*.dev.example.com"}, names: []string{"prod.example.com"}, want: false}, + {patterns: []string{"*.dev.example.com"}, names: []string{"prod.example.com:2222"}, want: false}, + {patterns: nil, names: []string{"foo.example.com"}, want: false}, + } { + if got := matchHostnames(tc.patterns, tc.names...); got != tc.want { + t.Errorf("[#%d] matchHostnames(%q, %q) = %v, want %v", i, tc.patterns, tc.names, got, tc.want) + } + } +} diff --git a/go/internal/app/db.go b/go/internal/app/db.go index 3288b03..6fa6c1a 100644 --- a/go/internal/app/db.go +++ b/go/internal/app/db.go @@ -39,6 +39,8 @@ import ( "github.com/urfave/cli/v2" "golang.org/x/crypto/nacl/secretbox" "golang.org/x/crypto/pbkdf2" + + "github.com/c2FmZQ/sshterm/internal/indexeddb" ) func (a *App) dbCommand() *cli.App { @@ -106,7 +108,7 @@ func (a *App) dbCommand() *cli.App { if !a.term.Confirm("You are about to WIPE the database.\nContinue? ", false) { return errors.New("aborted") } - globalAgent = &keyRing{} + globalAgent.(*keyRing).ForceRemoveAll() a.data.Authorities = make(map[string]*authority) a.data.Endpoints = make(map[string]*endpoint) a.data.Hosts = make(map[string]*host) @@ -199,30 +201,66 @@ func (a *App) dbCommand() *cli.App { if len(enc) < 40 || !bytes.Equal(enc[:4], backupMagic) { return fmt.Errorf("invalid backup file") } + iter := binary.BigEndian.Uint32(enc[12:16]) + if iter < 1000 || iter > 1000000 { + return fmt.Errorf("invalid iter value") + } passphrase, err := a.term.ReadPassword("Enter the passphrase for the backup: ") if err != nil { return fmt.Errorf("ReadPassword: %w", err) } - iter := binary.BigEndian.Uint32(enc[12:16]) - if iter > 1000000 { - return fmt.Errorf("invalid iter value") - } dk := pbkdf2.Key([]byte(passphrase), enc[4:12], int(iter), 32, sha256.New) var nonce [24]byte - var key [32]byte + var boxKey [32]byte copy(nonce[:], enc[16:40]) - copy(key[:], dk) - payload, ok := secretbox.Open(nil, enc[40:], &nonce, &key) + copy(boxKey[:], dk) + payload, ok := secretbox.Open(nil, enc[40:], &nonce, &boxKey) if !ok { return fmt.Errorf("unable to decrypt file") } - globalAgent = &keyRing{} var newData appData if err := json.Unmarshal(payload, &newData); err != nil { return fmt.Errorf("json.Unmarshal: %w", err) } + if newData.Authorities == nil { + newData.Authorities = make(map[string]*authority) + } + if newData.Endpoints == nil { + newData.Endpoints = make(map[string]*endpoint) + } + if newData.Hosts == nil { + newData.Hosts = make(map[string]*host) + } + if newData.Keys == nil { + newData.Keys = make(map[string]*key) + } + if newData.Params == nil { + newData.Params = make(map[string]any) + } + for _, k := range newData.Keys { + k.errorf = a.term.Errorf + } + if a.cfg.Persist != nil { + newData.Persist = *a.cfg.Persist + } + globalAgent.(*keyRing).ForceRemoveAll() a.data = newData + if !a.data.Persist { + if a.db != nil { + a.db.Close() + a.db = nil + } + return indexeddb.Delete(a.cfg.DBName) + } + if a.db == nil { + db, err := indexeddb.New(a.cfg.DBName) + if err != nil { + return fmt.Errorf("indexeddb.New: %w", err) + } + a.db = db + a.lastDBRefresh = globalLastDBChange + } return a.saveAll() }, }, diff --git a/go/internal/app/hosts.go b/go/internal/app/hosts.go index 0e8df34..8a0b525 100644 --- a/go/internal/app/hosts.go +++ b/go/internal/app/hosts.go @@ -34,8 +34,12 @@ import ( func (a *App) addHost(name, key string) error { var hk []byte - if key, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key)); err == nil { - hk = key.Marshal() + if key != "" { + pub, _, _, _, err := ssh.ParseAuthorizedKey([]byte(key)) + if err != nil { + return err + } + hk = pub.Marshal() } a.data.Hosts[name] = &host{Name: name, Key: hk} return nil diff --git a/go/internal/app/keys.go b/go/internal/app/keys.go index 63048bb..63a5120 100644 --- a/go/internal/app/keys.go +++ b/go/internal/app/keys.go @@ -27,6 +27,7 @@ package app import ( "bytes" + "context" "crypto" "crypto/ecdsa" "crypto/ed25519" @@ -38,6 +39,7 @@ import ( "errors" "fmt" "io" + "math" "net/http" "sort" "strings" @@ -95,6 +97,7 @@ func (a *App) generateKey(name, passphrase, idp, typ string, bits int, resident Provider: idp, errorf: a.term.Errorf, } + globalAgent.(*keyRing).RemoveByName(name) a.data.Keys[name] = k return k, nil } @@ -274,6 +277,7 @@ func (a *App) keysCommand() *cli.App { a.term.Errorf("certificate update: %v", err) } } + globalAgent.(*keyRing).RemoveByName(name) a.data.Keys[name] = k if err := a.saveKeys(true); err != nil { return err @@ -301,6 +305,12 @@ func (a *App) keysCommand() *cli.App { if !a.term.Confirm(fmt.Sprintf("You are about to delete key %q\nContinue?", name), false) { return errors.New("aborted") } + if key, exists := a.data.Keys[name]; exists { + if pub, err := key.sshPublicKey(); err == nil { + globalAgent.Remove(pub) + } + } + globalAgent.(*keyRing).RemoveByName(name) delete(a.data.Keys, name) return a.saveKeys(true) }, @@ -326,7 +336,7 @@ func (a *App) keysCommand() *cli.App { a.term.Printf("Public key: %s %s\n", strings.TrimSpace(string(ssh.MarshalAuthorizedKey(pub))), name) a.term.Printf("Fingerprint: %s\n", ssh.FingerprintSHA256(pub)) if cert := key.Certificate(); cert != nil { - a.term.Printf("Certificate: %s\n", strings.TrimSpace(string(key.CertBytes))) + a.term.Printf("Certificate: %s\n", strings.TrimSpace(string(ssh.MarshalAuthorizedKey(cert)))) a.term.Printf("Details:\n") a.printCertificate(cert) if err := checkCertificate(cert, ssh.UserCert); err != nil { @@ -461,6 +471,7 @@ func (a *App) keysCommand() *cli.App { } key.Public = sshPub.Marshal() } + globalAgent.(*keyRing).RemoveByName(name) a.data.Keys[name] = key if err := a.saveKeys(true); err != nil { return err @@ -614,10 +625,19 @@ func (a *App) printCertificate(cert *ssh.Certificate) { a.term.Printf(" Public key fp:... %s\n", ssh.FingerprintSHA256(cert.Key)) a.term.Printf(" Type:............ %s\n", cert.Type()) a.term.Printf(" Key ID:.......... %s\n", termsafe.Name(cert.KeyId)) - if cert.ValidBefore != 0 { + if cert.ValidAfter != 0 || cert.ValidBefore != ssh.CertTimeInfinity { + formatTime := func(t uint64) string { + if t == ssh.CertTimeInfinity { + return "forever" + } + if t > math.MaxInt64 { + return fmt.Sprintf("0x%x", t) + } + return time.Unix(int64(t), 0).UTC().Format(time.DateTime) + } a.term.Printf(" Validity:........ %s - %s (UTC)\n", - time.Unix(int64(cert.ValidAfter), 0).UTC().Format(time.DateTime), - time.Unix(int64(cert.ValidBefore), 0).UTC().Format(time.DateTime)) + formatTime(cert.ValidAfter), + formatTime(cert.ValidBefore)) } a.term.Printf(" Authority key:... %s", ssh.MarshalAuthorizedKey(cert.SignatureKey)) a.term.Printf(" Authority key fp: %s\n", ssh.FingerprintSHA256(cert.SignatureKey)) @@ -680,26 +700,32 @@ func (k *key) sshPublicKey() (ssh.PublicKey, error) { return ssh.ParsePublicKey(k.Public) } -func (k *key) Certificate() (cert *ssh.Certificate) { - parseCert := func() *ssh.Certificate { - c, _, _, _, err := ssh.ParseAuthorizedKey(k.CertBytes) - if err != nil { - return nil - } - if cert, ok := c.(*ssh.Certificate); ok { - return cert - } +func (k *key) parseCert() *ssh.Certificate { + if len(k.CertBytes) == 0 { return nil } - cert = parseCert() - if cert != nil && cert.ValidBefore > uint64(time.Now().Add(5*time.Minute).Unix()) { + c, _, _, _, err := ssh.ParseAuthorizedKey(k.CertBytes) + if err != nil { + return nil + } + if cert, ok := c.(*ssh.Certificate); ok { + return cert + } + return nil +} + +func (k *key) Certificate() (cert *ssh.Certificate) { + cert = k.parseCert() + if cert != nil && (cert.ValidBefore == ssh.CertTimeInfinity || cert.ValidBefore > uint64(time.Now().Add(5*time.Minute).Unix())) { return } if err := k.updateCert(); err != nil { - k.errorf("certificate update: %v", err) + if k.errorf != nil { + k.errorf("certificate update: %v", err) + } return } - return parseCert() + return k.parseCert() } func (k *key) updateCert() error { @@ -710,7 +736,9 @@ func (k *key) updateCert() error { if err != nil { return err } - req, err := http.NewRequest("POST", k.Provider, bytes.NewReader(ssh.MarshalAuthorizedKey(pub))) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + req, err := http.NewRequestWithContext(ctx, "POST", k.Provider, bytes.NewReader(ssh.MarshalAuthorizedKey(pub))) if err != nil { return err } diff --git a/go/internal/app/sftp.go b/go/internal/app/sftp.go index 40a8d75..7d8ecfb 100644 --- a/go/internal/app/sftp.go +++ b/go/internal/app/sftp.go @@ -144,6 +144,34 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e return base } + expandArgs := func(in []string) []string { + var out []string + for _, pa := range in { + dirOnly := strings.HasSuffix(pa, "/") + matches, err := client.Glob(joinPath(cwd, pa)) + if err != nil { + out = append(out, shellwords.UnquoteWild(pa)) + continue + } + count := 0 + for _, m := range matches { + if dirOnly { + if st, err := client.Stat(m); err == nil && st.IsDir() { + count++ + out = append(out, strings.TrimPrefix(m+"/", cwd+"/")) + } + continue + } + count++ + out = append(out, strings.TrimPrefix(m, cwd+"/")) + } + if count == 0 { + out = append(out, shellwords.UnquoteWild(pa)) + } + } + return out + } + commands := []*cli.App{ { Name: "cd", @@ -151,17 +179,18 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e UsageText: "cd [dir]\ncd -\ncd", HideHelpCommand: true, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() > 1 { + args := expandArgs(ctx.Args().Slice()) + if len(args) > 1 { cli.ShowSubcommandHelp(ctx) return nil } - if ctx.Args().Len() == 0 { + if len(args) == 0 { lastDir = cwd cwd = homeDir setPrompt() return nil } - dir := ctx.Args().Get(0) + dir := args[0] if dir == "-" { lastDir, cwd = cwd, lastDir setPrompt() @@ -210,12 +239,13 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e }, }, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() == 0 { + args := expandArgs(ctx.Args().Slice()) + if len(args) == 0 { cli.ShowSubcommandHelp(ctx) return nil } var e []error - for _, dir := range ctx.Args().Slice() { + for _, dir := range args { var err error if ctx.Bool("p") { err = client.MkdirAll(joinPath(cwd, dir)) @@ -235,12 +265,13 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e UsageText: "rmdir [ ...]", HideHelpCommand: true, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() == 0 { + args := expandArgs(ctx.Args().Slice()) + if len(args) == 0 { cli.ShowSubcommandHelp(ctx) return nil } var e []error - for _, dir := range ctx.Args().Slice() { + for _, dir := range args { if err := client.RemoveDirectory(joinPath(cwd, dir)); err != nil { e = append(e, fmt.Errorf("%q: %w", dir, err)) } @@ -260,12 +291,13 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e }, }, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() == 0 { + args := expandArgs(ctx.Args().Slice()) + if len(args) == 0 { cli.ShowSubcommandHelp(ctx) return nil } var e []error - for _, dir := range ctx.Args().Slice() { + for _, dir := range args { var err error if ctx.Bool("R") { err = client.RemoveAll(joinPath(cwd, dir)) @@ -285,11 +317,11 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e UsageText: "mv \nmv [...] ", HideHelpCommand: true, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() < 2 { + args := expandArgs(ctx.Args().Slice()) + if len(args) < 2 { cli.ShowSubcommandHelp(ctx) return nil } - args := ctx.Args().Slice() dest := args[len(args)-1] files := args[:len(args)-1] if st, err := client.Stat(joinPath(cwd, dest)); err == nil && st.IsDir() { @@ -325,11 +357,11 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e UsageText: "chmod [...]", HideHelpCommand: true, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() < 2 { + args := expandArgs(ctx.Args().Slice()) + if len(args) < 2 { cli.ShowSubcommandHelp(ctx) return nil } - args := ctx.Args().Slice() mode, err := strconv.ParseInt(args[0], 8, 32) if err != nil { return fmt.Errorf("mode: %w", err) @@ -355,12 +387,13 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e }, }, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() != 2 { + args := expandArgs(ctx.Args().Slice()) + if len(args) != 2 { cli.ShowSubcommandHelp(ctx) return nil } - oldName := ctx.Args().Get(0) - newName := ctx.Args().Get(1) + oldName := args[0] + newName := args[1] if ctx.Bool("s") { return client.Symlink(oldName, joinPath(cwd, newName)) } @@ -374,13 +407,14 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e Description: "The put command initiates the upload of one of more files. Without\narguments, it uploads to the current directory. With the name of an\nexisting directory as argument, it uploads to that directory. With\na non-existent file name, it upload one file to that file name.", HideHelpCommand: true, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() > 1 { + args := expandArgs(ctx.Args().Slice()) + if len(args) > 1 { cli.ShowSubcommandHelp(ctx) return nil } dest := cwd - if ctx.Args().Len() == 1 { - dest = joinPath(cwd, ctx.Args().Get(0)) + if len(args) == 1 { + dest = joinPath(cwd, args[0]) } isDir := false if st, err := client.Stat(dest); err == nil && st.IsDir() { @@ -391,7 +425,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e fmt.Fprintf(t, "%s ", f.Name) var fn string if isDir { - fn = joinPath(cwd, dest, f.Name) + fn = joinPath(cwd, dest, path.Base(f.Name)) } else { fn = joinPath(cwd, dest) } @@ -409,7 +443,8 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e Description: "The get command initiates the download of one of more files.", HideHelpCommand: true, Action: func(ctx *cli.Context) error { - if ctx.Args().Len() == 0 { + args := expandArgs(ctx.Args().Slice()) + if len(args) == 0 { cli.ShowSubcommandHelp(ctx) return nil } @@ -448,7 +483,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e fmt.Fprintln(t) return nil } - for _, f := range ctx.Args().Slice() { + for _, f := range args { if err := get(joinPath(cwd, f)); err != nil { return err } @@ -468,7 +503,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e }, }, Action: func(ctx *cli.Context) error { - args := ctx.Args().Slice() + args := expandArgs(ctx.Args().Slice()) if len(args) == 0 { args = []string{cwd} } @@ -668,6 +703,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e event.Call("preventDefault") return nil }) + defer dragoverHandler.Release() dropHandler := js.FuncOf(func(this js.Value, args []js.Value) any { event := args[0] event.Call("preventDefault") @@ -677,7 +713,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e a.term.Printf("\n") for _, f := range files { a.term.Printf("%s ", f.Name) - if err := a.sftpUploadFile(client, f, joinPath(cwd, f.Name)); err != nil { + if err := a.sftpUploadFile(client, f, joinPath(cwd, path.Base(f.Name))); err != nil { a.term.Errorf("drop: %s", termsafe.Text(err.Error())) return } @@ -685,6 +721,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e }() return nil }) + defer dropHandler.Release() a.cfg.Term.Get("element").Call("addEventListener", "dragover", dragoverHandler) a.cfg.Term.Get("element").Call("addEventListener", "drop", dropHandler) defer a.cfg.Term.Get("element").Call("removeEventListener", "dragover", dragoverHandler) @@ -702,35 +739,8 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e if len(pArgs) == 0 { continue } - var args []string - for i, pa := range pArgs { - if i == 0 { - args = append(args, shellwords.UnquoteWild(pa)) - continue - } - dirOnly := strings.HasSuffix(pa, "/") - matches, err := client.Glob(joinPath(cwd, pa)) - if err != nil { - args = append(args, shellwords.UnquoteWild(pa)) - continue - } - count := 0 - for _, m := range matches { - if dirOnly { - if st, err := client.Stat(m); err == nil && st.IsDir() { - count++ - args = append(args, strings.TrimPrefix(m+"/", cwd+"/")) - } - continue - } - count++ - args = append(args, strings.TrimPrefix(m, cwd+"/")) - } - if count == 0 { - args = append(args, shellwords.UnquoteWild(pa)) - } - } - switch name := args[0]; name { + pArgs[0] = shellwords.UnquoteWild(pArgs[0]) + switch name := pArgs[0]; name { case "help", "?": fmt.Fprintf(t, "Available commands:\n") maxLen := 0 @@ -743,6 +753,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e fmt.Fprintf(t, "Run any command with --help for more details.\n") case "debug": + args := append([]string{pArgs[0]}, expandArgs(pArgs[1:])...) fmt.Fprintf(t, "cmdline parsing: %q -> %q\n", pArgs, args) case "exit", "quit", "bye": @@ -756,7 +767,7 @@ func (a *App) runSFTP(ctx context.Context, target, keyName, jumpHosts string) (e } jsutil.TryCatch( func() { // try - if err := cmd.RunContext(ctx, args); err != nil { + if err := cmd.RunContext(ctx, pArgs); err != nil { fmt.Fprintf(t, "%s\n", termsafe.Text(err.Error())) } }, diff --git a/go/internal/app/ssh.go b/go/internal/app/ssh.go index 79ec548..1fbd5a3 100644 --- a/go/internal/app/ssh.go +++ b/go/internal/app/ssh.go @@ -32,7 +32,6 @@ import ( "fmt" "io" "net" - "path" "strconv" "strings" "time" @@ -123,9 +122,12 @@ func (a *App) runSSH(ctx context.Context, target, keyName, command string, forwa if forwardAgent { _, hostname, _ := parseUserHost(target) fwd := &forwardedAgent{ - agent: globalAgent, - host: hostname, - notify: t.Printf, + agent: globalAgent, + host: hostname, + notify: func(f string, args ...any) { + t.Printf(f, args...) + t.ShowMessage(f, args...) + }, } if err := agent.ForwardToAgent(client, fwd); err != nil { return fmt.Errorf("agent.ForwardToAgent: %w", err) @@ -148,6 +150,8 @@ func (a *App) runSSH(ctx context.Context, target, keyName, command string, forwa session.Stderr = t if command != "" { + a.inShell.Store(true) + defer a.inShell.Store(false) return session.Run(command) } modes := ssh.TerminalModes{ @@ -369,16 +373,9 @@ func (a *App) hostCertificateCallback(hostname string, ep *endpoint, cert *ssh.C if !caExists { return false } - for _, h := range ca.Hostnames { - for _, n := range names { - if matched, err := path.Match(h, n); err == nil && matched { - return true - } - } - } - return false + return matchHostnames(ca.Hostnames, names...) } - caIsTrusted := caTrustedFor(hostname, certHostname) + caIsTrusted := caTrustedFor(certHostname) if !caExists { errs = append(errs, fmt.Errorf("host certificate is signed by an unknown authority")) } else if !caIsTrusted { @@ -395,7 +392,7 @@ func (a *App) hostCertificateCallback(hostname string, ep *endpoint, cert *ssh.C a.printCertificate(cert) a.term.Print("\n") - a.term.Errorf("Host certificate for %s is NOT trusted:\n %v\n", displayName, strings.ReplaceAll(err.Error(), "\n", "\n ")) + a.term.Errorf("Host certificate for %s is NOT trusted:\n %v\n", displayName, strings.ReplaceAll(termsafe.Text(err.Error()), "\n", "\n ")) a.term.Printf("Options:\n") a.term.Printf(" 1- Abort the connection (default)\n") @@ -415,7 +412,7 @@ func (a *App) hostCertificateCallback(hostname string, ep *endpoint, cert *ssh.C if strings.ContainsAny(p, "*?[") || termsafe.Name(p) != p { continue } - if caTrustedFor(hostname, p) { + if caTrustedFor(p) { hostnames = append(hostnames, p) } } @@ -429,8 +426,12 @@ func (a *App) hostCertificateCallback(hostname string, ep *endpoint, cert *ssh.C return nil } if ans == "3" && canTrustCA { + trustHost := certHostname + if h, _, err := net.SplitHostPort(trustHost); err == nil { + trustHost = h + } if caExists { - ca.Hostnames = append(ca.Hostnames, certHostname) + ca.Hostnames = append(ca.Hostnames, trustHost) return a.saveAuthorities(true) } a.data.Authorities[caFP] = &authority{ @@ -438,7 +439,7 @@ func (a *App) hostCertificateCallback(hostname string, ep *endpoint, cert *ssh.C Name: caFP[len(caFP)-8:], Public: cert.SignatureKey.Marshal(), Hostnames: []string{ - certHostname, + trustHost, }, } return a.saveAuthorities(true) diff --git a/go/internal/indexeddb/indexeddb.go b/go/internal/indexeddb/indexeddb.go index e0faa87..00048a7 100644 --- a/go/internal/indexeddb/indexeddb.go +++ b/go/internal/indexeddb/indexeddb.go @@ -42,14 +42,18 @@ var ErrNotFound = errors.New("not found") func Delete(name string) error { req := js.Global().Get("indexedDB").Call("deleteDatabase", js.ValueOf(name)) ch := make(chan error, 1) - req.Set("onerror", js.FuncOf(func(this js.Value, args []js.Value) any { + errFn := js.FuncOf(func(this js.Value, args []js.Value) any { ch <- errors.New("error deleting database") return nil - })) - req.Set("onsuccess", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + defer errFn.Release() + okFn := js.FuncOf(func(this js.Value, args []js.Value) any { ch <- nil return nil - })) + }) + defer okFn.Release() + req.Set("onerror", errFn) + req.Set("onsuccess", okFn) return <-ch } @@ -62,7 +66,7 @@ func New(name string) (*DB, error) { } resCh := make(chan result, 2) - req.Set("onupgradeneeded", js.FuncOf(func(this js.Value, args []js.Value) any { + upgradeFn := js.FuncOf(func(this js.Value, args []js.Value) any { db := req.Get("result") names := db.Get("objectStoreNames") var found bool @@ -77,15 +81,21 @@ func New(name string) (*DB, error) { db.Call("createObjectStore", storeName) } return nil - })) - req.Set("onerror", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + defer upgradeFn.Release() + errFn := js.FuncOf(func(this js.Value, args []js.Value) any { resCh <- result{err: errors.New("error loading database")} return nil - })) - req.Set("onsuccess", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + defer errFn.Release() + okFn := js.FuncOf(func(this js.Value, args []js.Value) any { resCh <- result{v: req.Get("result")} return nil - })) + }) + defer okFn.Release() + req.Set("onupgradeneeded", upgradeFn) + req.Set("onerror", errFn) + req.Set("onsuccess", okFn) r := <-resCh if r.err != nil { return nil, r.err @@ -106,14 +116,18 @@ func (db *DB) Close() { func (db *DB) Get(key string, value any) error { req := db.db.Call("transaction", storeName).Call("objectStore", storeName).Call("get", key) errCh := make(chan error, 2) - req.Set("onerror", js.FuncOf(func(this js.Value, args []js.Value) any { + errFn := js.FuncOf(func(this js.Value, args []js.Value) any { errCh <- errors.New("transaction error") return nil - })) - req.Set("onsuccess", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + defer errFn.Release() + okFn := js.FuncOf(func(this js.Value, args []js.Value) any { errCh <- nil return nil - })) + }) + defer okFn.Release() + req.Set("onerror", errFn) + req.Set("onsuccess", okFn) if err := <-errCh; err != nil { return err } @@ -135,13 +149,17 @@ func (db *DB) Set(key string, value any) error { t := db.db.Call("transaction", storeName, "readwrite") req := t.Call("objectStore", storeName).Call("put", string(b), key) errCh := make(chan error, 2) - req.Set("onerror", js.FuncOf(func(this js.Value, args []js.Value) any { + errFn := js.FuncOf(func(this js.Value, args []js.Value) any { errCh <- errors.New("transaction error") return nil - })) - req.Set("onsuccess", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + defer errFn.Release() + okFn := js.FuncOf(func(this js.Value, args []js.Value) any { errCh <- nil return nil - })) + }) + defer okFn.Release() + req.Set("onerror", errFn) + req.Set("onsuccess", okFn) return <-errCh } diff --git a/go/internal/jsutil/jsutil.go b/go/internal/jsutil/jsutil.go index 22e8755..6283c0d 100644 --- a/go/internal/jsutil/jsutil.go +++ b/go/internal/jsutil/jsutil.go @@ -75,8 +75,10 @@ func NewResolvedPromise(v any) js.Value { } func NewPromise(f func() (any, error)) js.Value { - return Promise.New(js.FuncOf( + var cb js.Func + cb = js.FuncOf( func(this js.Value, args []js.Value) any { + defer cb.Release() resolve := args[0] reject := args[1] go func() { @@ -89,7 +91,8 @@ func NewPromise(f func() (any, error)) js.Value { }() return nil }, - )) + ) + return Promise.New(cb) } func Await(p js.Value) (js.Value, error) { @@ -98,16 +101,18 @@ func Await(p js.Value) (js.Value, error) { } v := make(chan js.Value, 1) e := make(chan error, 1) - resolve := func(this js.Value, args []js.Value) any { + resolveFn := js.FuncOf(func(this js.Value, args []js.Value) any { v <- args[0] return nil - } - reject := func(this js.Value, args []js.Value) any { + }) + defer resolveFn.Release() + rejectFn := js.FuncOf(func(this js.Value, args []js.Value) any { e <- fmt.Errorf("%s", args[0].Call("toString")) return nil - } - p.Call("then", js.FuncOf(resolve)). - Call("catch", js.FuncOf(reject)) + }) + defer rejectFn.Release() + p.Call("then", resolveFn). + Call("catch", rejectFn) select { case value := <-v: return value, nil @@ -171,11 +176,12 @@ func ImportFiles(accept string, multiple bool) []ImportedFile { } ch := make(chan ImportedFile) - input.Call("addEventListener", "cancel", js.FuncOf(func(this js.Value, args []js.Value) any { + cancelFn := js.FuncOf(func(this js.Value, args []js.Value) any { close(ch) return nil - })) - input.Call("addEventListener", "change", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + defer cancelFn.Release() + changeFn := js.FuncOf(func(this js.Value, args []js.Value) any { event := args[0] files := event.Get("target").Get("files") length := files.Length() @@ -190,7 +196,10 @@ func ImportFiles(accept string, multiple bool) []ImportedFile { } close(ch) return nil - })) + }) + defer changeFn.Release() + input.Call("addEventListener", "cancel", cancelFn) + input.Call("addEventListener", "change", changeFn) Body.Call("appendChild", input) input.Call("click") Body.Call("removeChild", input) diff --git a/go/internal/jsutil/streams.go b/go/internal/jsutil/streams.go index f88505b..68eccac 100644 --- a/go/internal/jsutil/streams.go +++ b/go/internal/jsutil/streams.go @@ -78,7 +78,13 @@ func (r *StreamReader) Close() error { func NewReadableStream(r io.Reader, done chan<- error, progress func(int64)) js.Value { cancelCh := make(chan struct{}) + var pullCb, cancelCb js.Func + releaseFuncs := sync.OnceFunc(func() { + pullCb.Release() + cancelCb.Release() + }) allDone := func(err error) { + releaseFuncs() select { case done <- err: default: @@ -86,7 +92,7 @@ func NewReadableStream(r io.Reader, done chan<- error, progress func(int64)) js. } offset := new(atomic.Int64) s := Object.New() - s.Set("pull", js.FuncOf(func(this js.Value, args []js.Value) any { + pullCb = js.FuncOf(func(this js.Value, args []js.Value) any { return NewPromise(func() (any, error) { controller := args[0] size := 16384 @@ -116,16 +122,26 @@ func NewReadableStream(r io.Reader, done chan<- error, progress func(int64)) js. } return nil, nil }) - })) - s.Set("cancel", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + cancelCb = js.FuncOf(func(this js.Value, args []js.Value) any { close(cancelCh) allDone(errors.New("canceled")) return nil - })) + }) + s.Set("pull", pullCb) + s.Set("cancel", cancelCb) return ReadableStream.New(s) } +var ( + streamHelperMu sync.Mutex + defaultStreamHelper *StreamHelper +) + func NewStreamHelper() *StreamHelper { + streamHelperMu.Lock() + defer streamHelperMu.Unlock() + container := js.Global().Get("navigator").Get("serviceWorker") if !container.Truthy() { return nil @@ -144,6 +160,10 @@ func NewStreamHelper() *StreamHelper { return nil } + if defaultStreamHelper != nil { + return defaultStreamHelper + } + h := &StreamHelper{ streams: make(map[string]stream), } @@ -215,6 +235,7 @@ func NewStreamHelper() *StreamHelper { }, )) + defaultStreamHelper = h return h } diff --git a/go/internal/terminal/terminal.go b/go/internal/terminal/terminal.go index 07ca6df..23bc68f 100644 --- a/go/internal/terminal/terminal.go +++ b/go/internal/terminal/terminal.go @@ -58,12 +58,14 @@ func New(ctx context.Context, t js.Value) *Terminal { tt.Escape = tt.vt.Escape tt.setDefaultPrompt() - disp := t.Call("onBell", js.FuncOf(func(this js.Value, args []js.Value) any { + bellCb := js.FuncOf(func(this js.Value, args []js.Value) any { fmt.Fprintf(os.Stderr, "onBell\n") return nil - })) + }) + disp := t.Call("onBell", bellCb) tt.tw.dispose = append(tt.tw.dispose, disp) - disp = t.Call("onData", js.FuncOf(func(this js.Value, args []js.Value) any { + tt.tw.funcs = append(tt.tw.funcs, bellCb) + dataCb := js.FuncOf(func(this js.Value, args []js.Value) any { key := args[0].String() tt.tw.mu.Lock() for _, i := range tt.tw.onDataKeys { @@ -83,9 +85,11 @@ func New(ctx context.Context, t js.Value) *Terminal { return nil } return nil - })) + }) + disp = t.Call("onData", dataCb) tt.tw.dispose = append(tt.tw.dispose, disp) - disp = t.Call("onResize", js.FuncOf(func(this js.Value, args []js.Value) any { + tt.tw.funcs = append(tt.tw.funcs, dataCb) + resizeCb := js.FuncOf(func(this js.Value, args []js.Value) any { event := args[0] select { case <-tt.tw.ctx.Done(): @@ -96,8 +100,10 @@ func New(ctx context.Context, t js.Value) *Terminal { return nil } return nil - })) + }) + disp = t.Call("onResize", resizeCb) tt.tw.dispose = append(tt.tw.dispose, disp) + tt.tw.funcs = append(tt.tw.funcs, resizeCb) tt.vt.SetSize(tt.Cols(), tt.Rows()) go func() { @@ -139,6 +145,7 @@ type termWrapper struct { closeCh chan struct{} resizeCh chan resizeEvent dispose []js.Value + funcs []js.Func r []byte mu sync.Mutex @@ -163,6 +170,10 @@ func (t *termWrapper) Close() error { d.Call("dispose") } t.dispose = nil + for _, f := range t.funcs { + f.Release() + } + t.funcs = nil if !t.isClosed() { close(t.resizeCh) close(t.closeCh) @@ -199,10 +210,13 @@ func (t *termWrapper) Write(b []byte) (int, error) { return 0, ErrClosed } ch := make(chan struct{}) - t.xt.Call("write", jsutil.Uint8ArrayFromBytes(b), js.FuncOf(func(this js.Value, args []js.Value) any { + var cb js.Func + cb = js.FuncOf(func(this js.Value, args []js.Value) any { + cb.Release() close(ch) return nil - })) + }) + t.xt.Call("write", jsutil.Uint8ArrayFromBytes(b), cb) <-ch return len(b), nil } @@ -286,6 +300,13 @@ func (t *Terminal) Greenf(f string, args ...any) { t.Printf("%s%s%s", t.vt.Escape.Green, s, t.vt.Escape.Reset) } +func (t *Terminal) ShowMessage(f string, args ...any) { + if fn := t.tw.xt.Get("showMessage"); fn.Type() == js.TypeFunction { + s := strings.TrimSpace(fmt.Sprintf(f, args...)) + t.tw.xt.Call("showMessage", s) + } +} + func (t *Terminal) Focus() { t.tw.xt.Call("focus") } diff --git a/go/internal/tests/app_test.go b/go/internal/tests/app_test.go index 7f067e5..7bb3eee 100644 --- a/go/internal/tests/app_test.go +++ b/go/internal/tests/app_test.go @@ -166,10 +166,23 @@ func TestKeys(t *testing.T) { {Type: "blah\n", Expect: prompt}, {Type: "keys list\n", Expect: "(?s)ssh-ed25519 .* samekey\r\nssh-ed25519 .* test\r\n"}, {Expect: prompt}, + {Type: "agent add test\n", Expect: "Enter the passphrase for test:"}, + {Type: "blah\n", Expect: prompt}, + {Type: "agent list\n", Expect: "test"}, + {Expect: prompt}, {Type: "keys delete test\n", Expect: `Continue\?`}, {Type: "Y\n", Expect: prompt}, + {Type: "agent list\n", Expect: ""}, + {Expect: prompt}, {Type: "keys list\n", Expect: "ssh-ed25519 .* samekey\r\n"}, {Expect: prompt}, + {Type: "agent add samekey\n", Expect: "Enter the passphrase for samekey:"}, + {Type: "blah\n", Expect: prompt}, + {Type: "agent list\n", Expect: "samekey"}, + {Expect: prompt}, + {Type: "agent remove samekey\n", Expect: prompt}, + {Type: "agent list\n", Expect: ""}, + {Expect: prompt}, {Type: "keys delete samekey\n", Expect: `Continue\?`}, {Type: "Y\n", Expect: prompt}, {Type: "keys list\n", Expect: ""}, @@ -238,6 +251,19 @@ func TestDB(t *testing.T) { {Type: "foobar\n", Expect: prompt}, {Type: "keys list\n", Expect: "ssh-ed25519 .* test"}, {Expect: prompt}, + {Type: "agent add test\n", Expect: "Enter the passphrase for test:"}, + {Type: "foobar\n", Expect: prompt}, + {Type: "agent list\n", Expect: "test"}, + // CTRL-ALT-W triggers db wipe but waits for confirmation instead of auto-confirming. + {Expect: prompt, Do: func([]string) { + terminalIO.Reset() + appConfig.Term.Call("input", "\x1b\x17", true) + }}, + {Expect: `Continue\?`}, + // Shortcuts (like CTRL-L) are ignored while inside a sub-prompt. + {Type: "\x0cn\n", Expect: prompt}, + {Type: "ep list\n", Expect: "test .* websocket"}, + {Expect: prompt}, {Type: "db backup --iter=1000\n", Expect: `(?s)invalid iter value.*sshterm> `}, {Type: "db backup\n", Expect: "Enter a passphrase for the backup:"}, {Type: "foobar\n", Expect: "Enter the same passphrase:"}, @@ -246,6 +272,7 @@ func TestDB(t *testing.T) { {Type: "Y\n", Expect: prompt}, {Type: "ep list\n", Expect: ""}, {Type: "keys list\n", Expect: ""}, + {Type: "agent list\n", Expect: ""}, {Expect: prompt}, }) file := <-downloadCh @@ -263,6 +290,11 @@ func TestDB(t *testing.T) { {Type: "foobar\n", Expect: prompt}, {Type: "ep list\n", Expect: "test .* websocket"}, {Type: "keys list\n", Expect: "ssh-ed25519 .* test"}, + {Type: "agent list\n", Expect: ""}, + {Expect: prompt}, + {Type: "agent add test\n", Expect: "Enter the passphrase for test:"}, + {Type: "foobar\n", Expect: prompt}, + {Type: "agent list\n", Expect: "test"}, {Expect: prompt}, {Type: "exit\n"}, }) @@ -485,10 +517,9 @@ func TestHostCerts(t *testing.T) { {Type: "password\n", Expect: "exec: foo"}, {Wait: time.Second, Type: "\n\n"}, - // Use one of the certificate's principals as the endpoint's hostname. + // Use one of the certificate's principals that the CA is trusted for as the endpoint's hostname. {Type: "ssh testuser@fooserver foo\n", Expect: `(?s)Host certificate for fooserver is NOT trusted.*` + - `3- Continue, and set the hostname of endpoint fooserver to test-server\..*` + - `7- Continue, and set the hostname of endpoint fooserver to baz\..*Choice>`}, + `3- Continue, and set the hostname of endpoint fooserver to test-server\..*Choice>`}, {Type: "3\n", Expect: "Password: "}, {Type: "password\n", Expect: "exec: foo"}, {Wait: time.Second, Type: "\n\n"}, @@ -510,6 +541,13 @@ func TestHostCerts(t *testing.T) { {Type: "password\n", Expect: "exec: foo"}, {Wait: time.Second, Type: "\n\n"}, + // Endpoint with a host:port hostname matches CA trusted for myserver.example.com. + {Type: "ep add --hostname=myserver.example.com:2222 alias-port websocket?cert=true\n", Expect: prompt}, + {Type: "ssh testuser@alias-port foo\n", Expect: `Host certificate for alias-port \(myserver\.example\.com:2222\) is trusted`}, + {Expect: "Password: "}, + {Type: "password\n", Expect: "exec: foo"}, + {Wait: time.Second, Type: "\n\n"}, + // The authority isn't trusted for test-server. {Type: "ca remove-hostname testca test-server\n", Expect: prompt}, {Type: "ssh testuser@test-server foo\n", Expect: `(?s)Host certificate for test-server is NOT trusted.*3- Continue, and trust this authority.*Choice>`}, @@ -615,12 +653,24 @@ func TestSFTP(t *testing.T) { {Type: "ls -l test\n", Expect: `(?s)"test": file does not exist.*sftp> `}, }) - // File names from the server are sanitized. - fileUploader.enqueue("evil\x1b[31m.txt", "text/plain", int64(len(txt)), txt) + // File names from the server are sanitized, and uploaded file names have directory components stripped. + fileUploader.enqueue("../sub/evil\x1b[31m.txt", "text/plain", int64(len(txt)), txt) script(t, []line{ {Type: "put\n", Expect: "100%"}, {Type: "ls\n", Expect: `(?s)evil\?\[31m\.txt.*sftp> `}, {Type: "rm evil*\n", Expect: "sftp> "}, + }) + + // Remote file names starting with '-' expanded by globs must not be parsed as flags. + fileUploader.enqueue("-l", "text/plain", int64(len(txt)), txt) + script(t, []line{ + {Type: "mkdir flagtest\n", Expect: "sftp> "}, + {Type: "cd flagtest\n", Expect: "sftp> "}, + {Type: "put\n", Expect: "100%"}, + {Type: "ls *\n", Expect: `(?s)-l\r\n.*sftp> `}, + {Type: "rm *\n", Expect: "sftp> "}, + {Type: "cd ..\n", Expect: "sftp> "}, + {Type: "rmdir flagtest\n", Expect: "sftp> "}, {Type: "exit\n"}, {Expect: prompt}, diff --git a/go/internal/tests/jsutil_test.go b/go/internal/tests/jsutil_test.go index 906d214..c2cafc6 100644 --- a/go/internal/tests/jsutil_test.go +++ b/go/internal/tests/jsutil_test.go @@ -69,3 +69,20 @@ func TestIsSameOrigin(t *testing.T) { } } } + +func TestServiceWorkerSingleton(t *testing.T) { + sw := js.Global().Get("navigator").Get("serviceWorker") + if sw.IsUndefined() { + t.Skip("Service Worker not available") + } + h1 := jsutil.NewStreamHelper() + cb1 := sw.Get("onmessage") + h2 := jsutil.NewStreamHelper() + cb2 := sw.Get("onmessage") + if h1 != h2 { + t.Errorf("NewStreamHelper() returned different instances: %p != %p", h1, h2) + } + if !cb1.Equal(cb2) { + t.Error("serviceWorker.onmessage was overwritten by second NewStreamHelper() call") + } +} diff --git a/go/internal/tests/malformed_test.go b/go/internal/tests/malformed_test.go index 91cff77..26a9114 100644 --- a/go/internal/tests/malformed_test.go +++ b/go/internal/tests/malformed_test.go @@ -71,6 +71,15 @@ func TestMalformedFiles(t *testing.T) { }) return pem.EncodeToMemory(&pem.Block{Type: "WEBAUTHN ENCRYPTED KEY", Bytes: b.BytesOrPanic()}) } + backupWithIter := func(iter uint32) []byte { + b := make([]byte, 56) + copy(b[:4], []byte{0xe2, 0x9b, 0x94, '0'}) + b[12] = byte(iter >> 24) + b[13] = byte(iter >> 16) + b[14] = byte(iter >> 8) + b[15] = byte(iter) + return b + } script(t, []line{ {Expect: prompt}, {Type: "db wipe\n", Expect: `Continue\?`}, @@ -84,8 +93,11 @@ func TestMalformedFiles(t *testing.T) { }{ {"nopem.key", []byte("-----BEGIN WEBAUTHN KEY-----\nfoo\n"), "keys import bad\n", "invalid PEM data"}, {"badpoint.key", webauthnKey([]byte{4, 1, 2, 3}, 1000), "keys import bad\n", "invalid public key"}, + {"lowiter.key", webauthnKey(elliptic.Marshal(elliptic.P256(), priv.X, priv.Y), 1), "keys import bad\n", "invalid iteration count"}, {"hugeiter.key", webauthnKey(elliptic.Marshal(elliptic.P256(), priv.X, priv.Y), 0xffffffff), "keys import bad\n", "invalid iteration count"}, {"short.backup", []byte{0xe2, 0x9b, 0x94, '0', 1, 2, 3}, "db restore\n", "invalid backup file"}, + {"lowiter.backup", backupWithIter(1), "db restore\n", "invalid iter value"}, + {"hugeiter.backup", backupWithIter(0xffffffff), "db restore\n", "invalid iter value"}, } { fileUploader.enqueue(tc.name, "application/octet-stream", int64(len(tc.content)), tc.content) script(t, []line{ diff --git a/go/internal/tests/paste_test.go b/go/internal/tests/paste_test.go index 4ffa38d..7b71ae2 100644 --- a/go/internal/tests/paste_test.go +++ b/go/internal/tests/paste_test.go @@ -60,8 +60,8 @@ func TestPaste(t *testing.T) { {Type: "db wipe\n", Expect: `Continue\?`}, {Type: "Y\n", Expect: prompt}, }) - // Escape characters are removed from pasted text. - paste("ep add pa\x1bste ./websocket\r") + // Escape characters (including C1 CSI \u009b) are removed from pasted text. + paste("ep add pa\x1bs\u009bte ./websocket\r") script(t, []line{ {Expect: prompt}, {Type: "ep list\n", Expect: `(?s)paste +\./websocket.*sshterm> `}, diff --git a/go/internal/tests/preset_test.go b/go/internal/tests/preset_test.go index d138559..16da375 100644 --- a/go/internal/tests/preset_test.go +++ b/go/internal/tests/preset_test.go @@ -161,7 +161,8 @@ func TestPresetAuthorities(t *testing.T) { script(t, []line{ {Type: "ca list\n", Expect: `testca ` + regexp.QuoteMeta(fp) + ` \*\.example\.com`}, {Type: "ep list\n", Expect: `myserver\.example\.com +\./websocket\?cert=true +foo `}, - {Type: "ssh testuser@myserver.example.com foo\n", Expect: "Password: "}, + {Type: "ssh testuser@myserver.example.com foo\n", Expect: `(?s)Host certificate for myserver\.example\.com \(foo\) is NOT trusted.*not trusted for hostname "foo".*Choice>`}, + {Type: "2\n", Expect: "Password: "}, {Type: "password\n", Expect: "exec: foo"}, {Wait: time.Second, Type: "\n\n"}, {Type: "exit\n"}, @@ -227,7 +228,7 @@ func TestPresetKeys(t *testing.T) { {Type: "ssh testuser@myserver.example.com foo\n", Expect: `(?s)Host certificate for myserver.example.com is trusted.\r\n.*exec: foo`}, {Wait: time.Second, Type: "\n\n"}, - {Type: "keys show foo\n", Expect: `Fingerprint: (SHA256:[^\r\n]+)`, Do: func(m []string) { + {Type: "keys show foo\n", Expect: `(?s)Fingerprint: (SHA256:[^\r\n]+).*Certificate: ssh-ed25519-cert-v01@openssh\.com [A-Za-z0-9+/=]+\r\n.*Validity:\.+ .* \(UTC\)`, Do: func(m []string) { fingerprints = append(fingerprints, m[1]) }}, {Expect: prompt}, diff --git a/go/internal/testserver/main_test.go b/go/internal/testserver/main_test.go index 95b2484..d8e64e1 100644 --- a/go/internal/testserver/main_test.go +++ b/go/internal/testserver/main_test.go @@ -174,7 +174,7 @@ func TestSSHTerm(t *testing.T) { http.Error(w, "internal error", http.StatusInternalServerError) return } - out := ssh.MarshalAuthorizedKey(cert) + out := append(bytes.TrimSpace(ssh.MarshalAuthorizedKey(cert)), []byte(" evil\x1b[31mcomment\n")...) w.Header().Set("content-type", "text/plain") w.Header().Set("content-length", fmt.Sprintf("%d", len(out))) w.Write(out) @@ -401,10 +401,11 @@ func newSSHServer(t *testing.T, dir string, hostCert bool) (*sshServer, error) { signer := authority if hostCert { cert := &ssh.Certificate{ - Key: sshPub, - Serial: 0x12345, - CertType: ssh.HostCert, - KeyId: "test-server", + Key: sshPub, + Serial: 0x12345, + CertType: ssh.HostCert, + KeyId: "test-server", + ValidBefore: ssh.CertTimeInfinity, ValidPrincipals: []string{ "test-server", "myserver.example.com", diff --git a/go/internal/webauthnsk/key.go b/go/internal/webauthnsk/key.go index 9676d0c..748ab66 100644 --- a/go/internal/webauthnsk/key.go +++ b/go/internal/webauthnsk/key.go @@ -27,36 +27,20 @@ package webauthnsk import ( "bytes" - "crypto/aes" - "crypto/cipher" "crypto/ecdsa" - "crypto/elliptic" "crypto/rand" - "crypto/sha256" "encoding/asn1" - "encoding/pem" "errors" "fmt" "io" "math/big" "strings" - "golang.org/x/crypto/cryptobyte" - "golang.org/x/crypto/pbkdf2" "golang.org/x/crypto/ssh" "github.com/c2FmZQ/sshterm/internal/jsutil" ) -const ecdsa256KeyType = "webauthn-sk-ecdsa-sha2-nistp256@openssh.com" - -type Key struct { - typ string - id []byte - pubKey *ecdsa.PublicKey - rpID []byte -} - func Create(name string, resident bool) (*Key, error) { challenge := make([]byte, 32) if _, err := rand.Read(challenge); err != nil { @@ -90,12 +74,8 @@ func Create(name string, resident bool) (*Key, error) { if err != nil { return nil, fmt.Errorf("WebAuthnCreate: %w", err) } - cd, err := parseClientData(resp.ClientDataJSON) - if err != nil { - return nil, fmt.Errorf("ParseClientData: %w", err) - } - if cd.Type != "webauthn.create" { - return nil, fmt.Errorf("unexpected client data type %q", cd.Type) + if err := verifyClientData(resp.ClientDataJSON, "webauthn.create", challenge); err != nil { + return nil, err } att, err := parseAttestationObject(resp.AttestationObject) if err != nil { @@ -194,196 +174,6 @@ func Discover(promptTouch func(string)) (*Key, string, error) { return key, userName, nil } -const ( - // defaultIter is the number of PBKDF2 iterations used to encrypt keys. - defaultIter = 100000 - // maxIter is the maximum number of PBKDF2 iterations accepted when - // decrypting keys. It protects against crafted files that would hang - // the app. - maxIter = 1000000 -) - -func Unmarshal(priv []byte, name string, rp func(string) (string, error)) (*Key, error) { - if !bytes.HasPrefix(priv, []byte("-----BEGIN WEBAUTHN ")) { - return nil, errors.New("unexpected key format") - } - block, _ := pem.Decode(priv) - if block == nil { - return nil, errors.New("invalid PEM data") - } - str := cryptobyte.String(block.Bytes) - var ver uint8 - if !str.ReadUint8(&ver) { - return nil, errTooShort - } - if ver != 1 { - return nil, fmt.Errorf("unexpected version %d", ver) - } - var pubBytes cryptobyte.String - if !str.ReadUint16LengthPrefixed(&pubBytes) { - return nil, errTooShort - } - key, err := UnmarshalPublic(pubBytes) - if err != nil { - return nil, err - } - - var privBytes cryptobyte.String - if !str.ReadUint16LengthPrefixed(&privBytes) { - return nil, errTooShort - } - - switch block.Type { - case "WEBAUTHN KEY": - key.id = privBytes - - case "WEBAUTHN ENCRYPTED KEY": - salt := make([]byte, 16) - if !privBytes.ReadBytes(&salt, 16) { - return nil, errTooShort - } - var numIter uint32 - if !privBytes.ReadUint32(&numIter) { - return nil, errTooShort - } - if numIter > maxIter { - return nil, fmt.Errorf("invalid iteration count %d", numIter) - } - passphrase, err := rp("Enter the passphrase for " + name + ": ") - if err != nil { - return nil, err - } - dk := pbkdf2.Key([]byte(passphrase), salt, int(numIter), 32, sha256.New) - block, err := aes.NewCipher(dk) - if err != nil { - return nil, err - } - gcm, err := cipher.NewGCM(block) - if err != nil { - return nil, err - } - nonce := make([]byte, gcm.NonceSize()) - if !privBytes.ReadBytes(&nonce, len(nonce)) { - return nil, errTooShort - } - keyID, err := gcm.Open(nil, nonce, []byte(privBytes), nil) - if err != nil { - return nil, errors.New("invalid passphrase") - } - key.id = keyID - default: - return nil, errors.New("invalid webauthn key ID") - } - return key, nil -} - -func UnmarshalPublic(pub []byte) (*Key, error) { - var data struct { - Name string - ID string - Key []byte - Application string - } - if err := ssh.Unmarshal(pub, &data); err != nil { - return nil, fmt.Errorf("Unmarshal: %w", err) - } - if data.Name != ecdsa256KeyType || data.ID != "nistp256" { - return nil, fmt.Errorf("unexpected key type %q", data.Name) - } - x, y := elliptic.Unmarshal(elliptic.P256(), data.Key) - if x == nil { - return nil, errors.New("invalid public key") - } - return &Key{ - typ: data.Name, - rpID: []byte(data.Application), - pubKey: &ecdsa.PublicKey{ - Curve: elliptic.P256(), - X: x, - Y: y, - }, - }, nil -} - -func (k *Key) MarshalPrivate(passphrase string) (*pem.Block, error) { - if passphrase == "" { - buf := cryptobyte.NewBuilder([]byte{1}) - buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { - b.AddBytes(k.Marshal()) - }) - buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { - b.AddBytes(k.id) - }) - data, err := buf.Bytes() - if err != nil { - return nil, err - } - return &pem.Block{ - Type: "WEBAUTHN KEY", - Bytes: data, - }, nil - } - salt := make([]byte, 16) - rand.Read(salt) - numIter := defaultIter - dk := pbkdf2.Key([]byte(passphrase), salt, numIter, 32, sha256.New) - block, err := aes.NewCipher(dk) - if err != nil { - return nil, err - } - gcm, err := cipher.NewGCM(block) - if err != nil { - return nil, err - } - nonce := make([]byte, gcm.NonceSize()) - rand.Read(nonce) - encID := gcm.Seal(nonce, nonce, k.id, nil) - buf := cryptobyte.NewBuilder([]byte{1}) - buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { - b.AddBytes(k.Marshal()) - }) - buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { - b.AddBytes(salt) - b.AddUint32(uint32(numIter)) - b.AddBytes(encID) - }) - data, err := buf.Bytes() - if err != nil { - return nil, err - } - return &pem.Block{ - Type: "WEBAUTHN ENCRYPTED KEY", - Bytes: data, - }, nil -} - -func (k *Key) Type() string { - return k.typ -} - -func (k *Key) Marshal() []byte { - w := struct { - Name string - ID string - Key []byte - Application string - }{ - k.Type(), - "nistp256", - elliptic.Marshal(k.pubKey.Curve, k.pubKey.X, k.pubKey.Y), - string(k.rpID), - } - return ssh.Marshal(&w) -} - -func (k *Key) Verify(data []byte, sig *ssh.Signature) error { - return errors.New("verify not implemented") -} - -func (k *Key) PublicKey() ssh.PublicKey { - return k -} - func (k *Key) Sign(_ io.Reader, data []byte) (*ssh.Signature, error) { opts := jsutil.GetOptions{Challenge: data} if len(k.id) > 0 { @@ -397,13 +187,13 @@ func (k *Key) Sign(_ io.Reader, data []byte) (*ssh.Signature, error) { if err := parseAuthenticatorData(resp.AuthenticatorData, &ad); err != nil { return nil, fmt.Errorf("ParseAuthenticatorData: %w", err) } + if err := verifyClientData(resp.ClientDataJSON, "webauthn.get", data); err != nil { + return nil, err + } cd, err := parseClientData(resp.ClientDataJSON) if err != nil { return nil, fmt.Errorf("ParseClientData: %w", err) } - if cd.Type != "webauthn.get" { - return nil, fmt.Errorf("ParseClientData: unexpected client data type %q", cd.Type) - } var sig struct { R, S *big.Int diff --git a/go/internal/webauthnsk/webauthn.go b/go/internal/webauthnsk/webauthn.go index 0aad621..252bef7 100644 --- a/go/internal/webauthnsk/webauthn.go +++ b/go/internal/webauthnsk/webauthn.go @@ -26,18 +26,25 @@ package webauthnsk import ( "bytes" "crypto" + "crypto/aes" + "crypto/cipher" "crypto/ecdsa" "crypto/elliptic" + "crypto/rand" "crypto/sha256" "encoding/asn1" "encoding/base64" "encoding/binary" "encoding/json" + "encoding/pem" "errors" "fmt" "math/big" cbor "github.com/fxamacker/cbor/v2" + "golang.org/x/crypto/cryptobyte" + "golang.org/x/crypto/pbkdf2" + "golang.org/x/crypto/ssh" ) const algES256 = -7 @@ -373,3 +380,210 @@ func RecoverPublicKey(authData1, clientDataJSON1, sig1Bytes, authData2, clientDa } return pk, nil } + +const ecdsa256KeyType = "webauthn-sk-ecdsa-sha2-nistp256@openssh.com" + +type Key struct { + typ string + id []byte + pubKey *ecdsa.PublicKey + rpID []byte +} + +const ( + // minIter is the minimum number of PBKDF2 iterations accepted when + // decrypting keys. + minIter = 1000 + // defaultIter is the number of PBKDF2 iterations used to encrypt keys. + defaultIter = 100000 + // maxIter is the maximum number of PBKDF2 iterations accepted when + // decrypting keys. It protects against crafted files that would hang + // the app. + maxIter = 1000000 +) + +func Unmarshal(priv []byte, name string, rp func(string) (string, error)) (*Key, error) { + if !bytes.HasPrefix(priv, []byte("-----BEGIN WEBAUTHN ")) { + return nil, errors.New("unexpected key format") + } + block, _ := pem.Decode(priv) + if block == nil { + return nil, errors.New("invalid PEM data") + } + str := cryptobyte.String(block.Bytes) + var ver uint8 + if !str.ReadUint8(&ver) { + return nil, errTooShort + } + if ver != 1 && ver != 2 { + return nil, fmt.Errorf("unexpected version %d", ver) + } + var pubBytes cryptobyte.String + if !str.ReadUint16LengthPrefixed(&pubBytes) { + return nil, errTooShort + } + key, err := UnmarshalPublic(pubBytes) + if err != nil { + return nil, err + } + + var privBytes cryptobyte.String + if !str.ReadUint16LengthPrefixed(&privBytes) { + return nil, errTooShort + } + + switch block.Type { + case "WEBAUTHN KEY": + key.id = privBytes + + case "WEBAUTHN ENCRYPTED KEY": + salt := make([]byte, 16) + if !privBytes.ReadBytes(&salt, 16) { + return nil, errTooShort + } + var numIter uint32 + if !privBytes.ReadUint32(&numIter) { + return nil, errTooShort + } + if numIter < minIter || numIter > maxIter { + return nil, fmt.Errorf("invalid iteration count %d", numIter) + } + passphrase, err := rp("Enter the passphrase for " + name + ": ") + if err != nil { + return nil, err + } + dk := pbkdf2.Key([]byte(passphrase), salt, int(numIter), 32, sha256.New) + block, err := aes.NewCipher(dk) + if err != nil { + return nil, err + } + gcm, err := cipher.NewGCM(block) + if err != nil { + return nil, err + } + nonce := make([]byte, gcm.NonceSize()) + if !privBytes.ReadBytes(&nonce, len(nonce)) { + return nil, errTooShort + } + var aad []byte + if ver >= 2 { + aad = []byte(pubBytes) + } + keyID, err := gcm.Open(nil, nonce, []byte(privBytes), aad) + if err != nil { + return nil, errors.New("invalid passphrase") + } + key.id = keyID + default: + return nil, errors.New("invalid webauthn key ID") + } + return key, nil +} + +func UnmarshalPublic(pub []byte) (*Key, error) { + var data struct { + Name string + ID string + Key []byte + Application string + } + if err := ssh.Unmarshal(pub, &data); err != nil { + return nil, fmt.Errorf("Unmarshal: %w", err) + } + if data.Name != ecdsa256KeyType || data.ID != "nistp256" { + return nil, fmt.Errorf("unexpected key type %q", data.Name) + } + x, y := elliptic.Unmarshal(elliptic.P256(), data.Key) + if x == nil { + return nil, errors.New("invalid public key") + } + return &Key{ + typ: data.Name, + rpID: []byte(data.Application), + pubKey: &ecdsa.PublicKey{ + Curve: elliptic.P256(), + X: x, + Y: y, + }, + }, nil +} + +func (k *Key) MarshalPrivate(passphrase string) (*pem.Block, error) { + pubBytes := k.Marshal() + if passphrase == "" { + buf := cryptobyte.NewBuilder([]byte{2}) + buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes(pubBytes) + }) + buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes(k.id) + }) + data, err := buf.Bytes() + if err != nil { + return nil, err + } + return &pem.Block{ + Type: "WEBAUTHN KEY", + Bytes: data, + }, nil + } + salt := make([]byte, 16) + rand.Read(salt) + numIter := defaultIter + dk := pbkdf2.Key([]byte(passphrase), salt, numIter, 32, sha256.New) + block, err := aes.NewCipher(dk) + if err != nil { + return nil, err + } + gcm, err := cipher.NewGCM(block) + if err != nil { + return nil, err + } + nonce := make([]byte, gcm.NonceSize()) + rand.Read(nonce) + encID := gcm.Seal(nonce, nonce, k.id, pubBytes) + buf := cryptobyte.NewBuilder([]byte{2}) + buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes(pubBytes) + }) + buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes(salt) + b.AddUint32(uint32(numIter)) + b.AddBytes(encID) + }) + data, err := buf.Bytes() + if err != nil { + return nil, err + } + return &pem.Block{ + Type: "WEBAUTHN ENCRYPTED KEY", + Bytes: data, + }, nil +} + +func (k *Key) Type() string { + return k.typ +} + +func (k *Key) Marshal() []byte { + w := struct { + Name string + ID string + Key []byte + Application string + }{ + k.Type(), + "nistp256", + elliptic.Marshal(k.pubKey.Curve, k.pubKey.X, k.pubKey.Y), + string(k.rpID), + } + return ssh.Marshal(&w) +} + +func (k *Key) Verify(data []byte, sig *ssh.Signature) error { + return errors.New("verify not implemented") +} + +func (k *Key) PublicKey() ssh.PublicKey { + return k +} diff --git a/go/internal/webauthnsk/webauthn_test.go b/go/internal/webauthnsk/webauthn_test.go index e2aec2f..a53c725 100644 --- a/go/internal/webauthnsk/webauthn_test.go +++ b/go/internal/webauthnsk/webauthn_test.go @@ -24,15 +24,23 @@ package webauthnsk import ( + "bytes" + "crypto/aes" + "crypto/cipher" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" "crypto/sha256" "encoding/asn1" "encoding/base64" + "encoding/binary" + "encoding/pem" "fmt" "math/big" "testing" + + "golang.org/x/crypto/cryptobyte" + "golang.org/x/crypto/pbkdf2" ) func TestRecoverPublicKey(t *testing.T) { @@ -243,3 +251,107 @@ func TestIsPrintableASCII(t *testing.T) { } } } + +func TestKeyMarshalUnmarshal(t *testing.T) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("GenerateKey: %v", err) + } + orig := &Key{ + typ: ecdsa256KeyType, + id: []byte("credential-id-12345"), + rpID: []byte("ssh:default"), + pubKey: &priv.PublicKey, + } + passphrase := "secret-passphrase" + rp := func(string) (string, error) { return passphrase, nil } + + // Round-trip without passphrase + block, err := orig.MarshalPrivate("") + if err != nil { + t.Fatalf("MarshalPrivate(\"\"): %v", err) + } + got, err := Unmarshal(pem.EncodeToMemory(block), "test", rp) + if err != nil { + t.Fatalf("Unmarshal(unencrypted): %v", err) + } + if got.typ != orig.typ || !bytes.Equal(got.rpID, orig.rpID) || !bytes.Equal(got.id, orig.id) { + t.Errorf("Unmarshal(unencrypted) mismatch: got %+v, want %+v", got, orig) + } + + // Round-trip with passphrase + block, err = orig.MarshalPrivate(passphrase) + if err != nil { + t.Fatalf("MarshalPrivate(passphrase): %v", err) + } + got, err = Unmarshal(pem.EncodeToMemory(block), "test", rp) + if err != nil { + t.Fatalf("Unmarshal(passphrase): %v", err) + } + if got.typ != orig.typ || !bytes.Equal(got.rpID, orig.rpID) || !bytes.Equal(got.id, orig.id) { + t.Errorf("Unmarshal(passphrase) mismatch: got %+v, want %+v", got, orig) + } + + // Tampered pubBytes must fail AES-GCM authentication (AAD binding) + pubLen := int(binary.BigEndian.Uint16(block.Bytes[1:3])) + // Flip a bit in the application string at the end of pubBytes so UnmarshalPublic still succeeds + tampered := append([]byte(nil), block.Bytes...) + tampered[1+2+pubLen-1] ^= 0x01 + tamperedPEM := pem.EncodeToMemory(&pem.Block{Type: block.Type, Bytes: tampered}) + if _, err := Unmarshal(tamperedPEM, "test", rp); err == nil { + t.Error("Unmarshal with tampered pubBytes succeeded; want AAD failure") + } + + // Downgrading a v2 key to v1 must also fail AES-GCM authentication + downgraded := append([]byte(nil), tampered...) + downgraded[0] = 1 + downgradedPEM := pem.EncodeToMemory(&pem.Block{Type: block.Type, Bytes: downgraded}) + if _, err := Unmarshal(downgradedPEM, "test", rp); err == nil { + t.Error("Unmarshal with v2->v1 version downgrade succeeded; want AAD failure") + } + + // Backward compatibility: a legacy v1 encrypted key (sealed with nil AAD) still unmarshals + pubBytes := orig.Marshal() + salt := make([]byte, 16) + rand.Read(salt) + dk := pbkdf2.Key([]byte(passphrase), salt, minIter, 32, sha256.New) + cBlock, err := aes.NewCipher(dk) + if err != nil { + t.Fatalf("aes.NewCipher: %v", err) + } + gcm, err := cipher.NewGCM(cBlock) + if err != nil { + t.Fatalf("cipher.NewGCM: %v", err) + } + nonce := make([]byte, gcm.NonceSize()) + rand.Read(nonce) + encIDv1 := gcm.Seal(nonce, nonce, orig.id, nil) + v1Buf := cryptobyte.NewBuilder([]byte{1}) + v1Buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes(pubBytes) + }) + v1Buf.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) { + b.AddBytes(salt) + b.AddUint32(uint32(minIter)) + b.AddBytes(encIDv1) + }) + v1PEM := pem.EncodeToMemory(&pem.Block{Type: "WEBAUTHN ENCRYPTED KEY", Bytes: v1Buf.BytesOrPanic()}) + gotV1, err := Unmarshal(v1PEM, "test", rp) + if err != nil { + t.Fatalf("Unmarshal(legacy v1): %v", err) + } + if gotV1.typ != orig.typ || !bytes.Equal(gotV1.rpID, orig.rpID) || !bytes.Equal(gotV1.id, orig.id) { + t.Errorf("Unmarshal(legacy v1) mismatch: got %+v, want %+v", gotV1, orig) + } + + // Iteration count below minIter or above maxIter must be rejected + iterOffset := 1 + 2 + pubLen + 2 + 16 + for _, badIter := range []uint32{0, 1, 999, maxIter + 1} { + badBytes := append([]byte(nil), block.Bytes...) + binary.BigEndian.PutUint32(badBytes[iterOffset:iterOffset+4], badIter) + badPEM := pem.EncodeToMemory(&pem.Block{Type: block.Type, Bytes: badBytes}) + if _, err := Unmarshal(badPEM, "test", rp); err == nil { + t.Errorf("Unmarshal with numIter=%d succeeded; want error", badIter) + } + } +} diff --git a/go/internal/websocket/websocket.go b/go/internal/websocket/websocket.go index 7c4a505..d84c72e 100644 --- a/go/internal/websocket/websocket.go +++ b/go/internal/websocket/websocket.go @@ -64,30 +64,34 @@ func New(ctx context.Context, url string, log io.Writer) (*WebSocket, error) { ch: make(chan js.Value, 4096), closeCh: make(chan struct{}), } - errCh := make(chan error) - jsWS.Call("addEventListener", "open", js.FuncOf(func(this js.Value, args []js.Value) any { + errCh := make(chan error, 1) + openFn := js.FuncOf(func(this js.Value, args []js.Value) any { select { case errCh <- nil: default: } return nil - })) - jsWS.Call("addEventListener", "error", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + errorFn := js.FuncOf(func(this js.Value, args []js.Value) any { select { case errCh <- fmt.Errorf("websocket error"): default: } ws.Close() return nil - })) - jsWS.Call("addEventListener", "close", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + closeFn := js.FuncOf(func(this js.Value, args []js.Value) any { if !ws.isClosed() { ws.err = ErrClosedByServer } + select { + case errCh <- ErrClosedByServer: + default: + } ws.Close() return nil - })) - jsWS.Call("addEventListener", "message", js.FuncOf(func(this js.Value, args []js.Value) any { + }) + msgFn := js.FuncOf(func(this js.Value, args []js.Value) any { data := args[0].Get("data") if !data.InstanceOf(jsutil.ArrayBuffer) { if !ws.isClosed() { @@ -101,8 +105,31 @@ func New(ctx context.Context, url string, log io.Writer) (*WebSocket, error) { case ws.ch <- data: } return nil - })) - return ws, <-errCh + }) + ws.cleanup = func() { + jsWS.Call("removeEventListener", "open", openFn) + jsWS.Call("removeEventListener", "error", errorFn) + jsWS.Call("removeEventListener", "close", closeFn) + jsWS.Call("removeEventListener", "message", msgFn) + openFn.Release() + errorFn.Release() + closeFn.Release() + msgFn.Release() + } + jsWS.Call("addEventListener", "open", openFn) + jsWS.Call("addEventListener", "error", errorFn) + jsWS.Call("addEventListener", "close", closeFn) + jsWS.Call("addEventListener", "message", msgFn) + select { + case <-ctx.Done(): + ws.Close() + return nil, ctx.Err() + case err := <-errCh: + if err != nil { + return nil, err + } + return ws, nil + } } var _ net.Conn = (*WebSocket)(nil) @@ -115,6 +142,7 @@ type WebSocket struct { r []byte closeCh chan struct{} err error + cleanup func() } func (ws *WebSocket) isClosed() bool { @@ -134,6 +162,9 @@ func (ws *WebSocket) Close() error { } close(ws.closeCh) ws.ws.Call("close") + if ws.cleanup != nil { + ws.cleanup() + } } return nil } diff --git a/go/internal/zmodem/filter.go b/go/internal/zmodem/filter.go index cd6bb3a..0ffde74 100644 --- a/go/internal/zmodem/filter.go +++ b/go/internal/zmodem/filter.go @@ -112,12 +112,11 @@ func New(term TerminalPrinter, download DownloadFunc, upload UploadFunc) *Filter if bytes.Contains(buf[:n], []byte{'\x03'}) { // Ctrl-C f.term.Printf("\x1b[31m[ZMODEM] Transfer aborted by user.\x1b[0m\r\n") f.mu.Lock() - f.active = false - f.window = [6]byte{} pw := f.pipeW f.mu.Unlock() - // Close the write side of the pipe to interrupt the parser + // Close the write side of the pipe to interrupt the parser; + // f.finish(pr) in the session goroutine will clear f.active. if pw != nil { pw.Close() } diff --git a/go/internal/zmodem/filter_test.go b/go/internal/zmodem/filter_test.go index a0fd7fa..cfde690 100644 --- a/go/internal/zmodem/filter_test.go +++ b/go/internal/zmodem/filter_test.go @@ -379,3 +379,94 @@ func TestZmodemFilterDownloadError(t *testing.T) { t.Errorf("terminal output = %q", out) } } + +func TestZmodemFilterCtrlCRace(t *testing.T) { + inR, inW := io.Pipe() + defer inW.Close() + term := &mockTerminal{Reader: inR} + + inDownload := make(chan struct{}) + unblockDownload := make(chan struct{}) + downloadCount := 0 + var countMu sync.Mutex + + download := func(name string, size int64, r io.Reader) error { + countMu.Lock() + downloadCount++ + countMu.Unlock() + close(inDownload) + // Read from r until EOF (triggered by Ctrl-C closing pw), then stay in + // the callback until unblockDownload is closed so the session goroutine + // has not yet called f.finish(pr). + io.ReadAll(r) + <-unblockDownload + return nil + } + filter := New(term, download, nil) + + go func() { + deadline := time.Now().Add(5 * time.Second) + for !strings.Contains(term.String(), "[y/N]") && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + inW.Write([]byte("y")) + }() + + replies := &chanReader{ch: make(chan []byte, 100)} + go func() { + for { + buf := make([]byte, 1024) + n, err := filter.Read(buf) + if n > 0 { + replies.ch <- buf[:n] + } + if err != nil { + close(replies.ch) + return + } + } + }() + + data := []byte("hello") + files := []*File{{Name: "file.txt", Size: int64(len(data)), R: io.NopCloser(bytes.NewReader(data))}} + go func() { + if err := writeHexHeader(filter, header{Type: zRQINIT}); err != nil { + return + } + send(struct { + io.Reader + io.Writer + }{replies, filter}, files, nil) + }() + + select { + case <-inDownload: + case <-time.After(5 * time.Second): + t.Fatalf("timed out waiting for download callback. Terminal: %q", term.String()) + } + + // Send Ctrl-C while the session goroutine is still inside download(). + inW.Write([]byte{'\x03'}) + deadline := time.Now().Add(5 * time.Second) + for !strings.Contains(term.String(), "Transfer aborted by user") { + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for abort message. Terminal: %q", term.String()) + } + time.Sleep(10 * time.Millisecond) + } + + // Write another ZRQINIT signature before the first session goroutine has finished. + // Because f.active is still true until f.finish(pr) runs, this must NOT start a + // concurrent second receive session. + filter.Write([]byte{'*', '*', zDLE, zHEX, '0', '0'}) + + close(unblockDownload) + time.Sleep(50 * time.Millisecond) + + countMu.Lock() + gotCount := downloadCount + countMu.Unlock() + if gotCount != 1 { + t.Errorf("downloadCount = %d, want 1", gotCount) + } +} diff --git a/go/internal/zmodem/receive.go b/go/internal/zmodem/receive.go index f72cd92..6010fcd 100644 --- a/go/internal/zmodem/receive.go +++ b/go/internal/zmodem/receive.go @@ -71,6 +71,9 @@ sessionLoop: var size int64 if len(parts) > 1 { fmt.Sscanf(string(parts[1]), "%d", &size) + if size < 0 { + return fmt.Errorf("invalid file size: %d", size) + } } // We received ZFILE. We send ZRPOS to accept it. diff --git a/go/internal/zmodem/session_test.go b/go/internal/zmodem/session_test.go index fb742dc..69404d1 100644 --- a/go/internal/zmodem/session_test.go +++ b/go/internal/zmodem/session_test.go @@ -166,3 +166,24 @@ func TestSessionSendReceive(t *testing.T) { } } } + +func TestReceiveNegativeFileSize(t *testing.T) { + var in bytes.Buffer + if err := writeBinaryHeader(&in, header{Type: zFILE}); err != nil { + t.Fatalf("writeBinaryHeader: %v", err) + } + if err := writeDataBlock(&in, []byte("evil.bin\x00-42 0 0 0\x00"), zCRCW, false); err != nil { + t.Fatalf("writeDataBlock: %v", err) + } + rw := struct { + io.Reader + io.Writer + }{&in, io.Discard} + err := receive(rw, func(name string, size int64, rc io.Reader) error { + t.Errorf("onFile called with size %d, want error before callback", size) + return nil + }) + if err == nil { + t.Fatal("receive succeeded with negative file size; want error") + } +} diff --git a/xterm/update.sh b/xterm/update.sh index d9cc29e..e37cb71 100755 --- a/xterm/update.sh +++ b/xterm/update.sh @@ -2,7 +2,7 @@ cd $(dirname $0) -npm install +npm ci --ignore-scripts cp node_modules/@xterm/xterm/css/xterm.css ../docroot/xterm.css cp node_modules/@xterm/xterm/LICENSE ../docroot/LICENSE.xterm.txt From 0382ad0941e5dd2039c16436dde08288ddb8b284 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Fri, 16 Oct 2026 10:39:35 -0700 Subject: [PATCH 2/2] Fix issues found in code review - Don't set inShell for non-PTY `ssh host cmd`, so Ctrl-C still cancels the context and closes the connection when a remote command hangs. - Remove deleted or replaced keys from the SSH agent even when the agent is locked (keys delete/generate/import/discover), and reset the agent lock state on db wipe and db restore. - Report an invalid preset host key without aborting the rest of the preset config. - Build the v2->v1 downgrade test case from the untampered key bytes so it actually exercises the version downgrade path. - Return the cached stream helper before touching the service worker, and drop the unused UnregisterServiceWorker. - Return the parsed client data from verifyClientData instead of parsing ClientDataJSON twice in Sign. Co-Authored-By: Claude Fable 5.1 --- CHANGELOG.md | 3 ++- go/internal/app/agent.go | 31 ++++++++++++++++++++++++- go/internal/app/app.go | 4 +++- go/internal/app/db.go | 4 ++-- go/internal/app/keys.go | 10 ++++---- go/internal/app/ssh.go | 2 -- go/internal/jsutil/streams.go | 16 ++++--------- go/internal/webauthnsk/key.go | 13 ++++------- go/internal/webauthnsk/webauthn.go | 14 +++++------ go/internal/webauthnsk/webauthn_test.go | 4 ++-- 10 files changed, 60 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index da165e3..d727f13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,8 @@ * Reject certificates with `ValidBefore == 0` as expired, while continuing to accept `ssh.CertTimeInfinity` (`0xffffffffffffffff`). * Expand SFTP wildcard patterns after CLI flag parsing so remote file names starting with `-` cannot inject command flags, and strip directory components from uploaded local file names. * Require confirmation when `CTRL-ALT-W` triggers `db wipe`, and only process keyboard shortcuts at the top-level `sshterm>` prompt. -* Clear keys from the in-memory SSH agent on `db wipe`, `db restore`, `keys delete`, `keys generate`, `keys discover`, and `keys import`. +* Clear keys from the in-memory SSH agent on `db wipe`, `db restore`, `keys delete`, `keys generate`, `keys discover`, and `keys import`, even when the agent is locked. `db wipe` and `db restore` also reset the agent lock. +* Reject invalid host keys in `config.json`'s `hosts[].key`. The error is reported and the remaining preset config is still applied. * Bind the WebAuthn public key blob as AES-GCM additional authenticated data (AAD) in encrypted WebAuthn key files (existing v1 keys can still be loaded), verify the registration challenge in `WebAuthn.Create`, and enforce a minimum of 1,000 PBKDF2 iterations on encrypted WebAuthn keys and database backups. * Sanitize certificate critical option names, certificate comments in `keys show`, and error messages in the host-key prompt, and strip 8-bit `CSI` (`\x9b`) characters from pasted text. * Show a popup notification in addition to the terminal message when a forwarded agent signs a request. diff --git a/go/internal/app/agent.go b/go/internal/app/agent.go index 35b5f91..a795339 100644 --- a/go/internal/app/agent.go +++ b/go/internal/app/agent.go @@ -325,6 +325,19 @@ func (r *keyRing) Remove(key ssh.PublicKey) error { if r.locked { return errAgentLocked } + return r.remove(key) +} + +// ForceRemove removes a key from the agent even when the agent is locked. It +// is used when the key is deleted or replaced in the database, so that the +// old key cannot be used again after the agent is unlocked. +func (r *keyRing) ForceRemove(key ssh.PublicKey) error { + r.mu.Lock() + defer r.mu.Unlock() + return r.remove(key) +} + +func (r *keyRing) remove(key ssh.PublicKey) error { v := key.Marshal() found := false r.keys = slices.DeleteFunc(r.keys, func(k agentKey) bool { @@ -346,6 +359,18 @@ func (r *keyRing) RemoveByName(name string) error { if r.locked { return errAgentLocked } + return r.removeByName(name) +} + +// ForceRemoveByName removes a key from the agent even when the agent is +// locked. See ForceRemove. +func (r *keyRing) ForceRemoveByName(name string) error { + r.mu.Lock() + defer r.mu.Unlock() + return r.removeByName(name) +} + +func (r *keyRing) removeByName(name string) error { found := false r.keys = slices.DeleteFunc(r.keys, func(k agentKey) bool { if k.comment == name { @@ -370,10 +395,14 @@ func (r *keyRing) RemoveAll() error { return nil } -func (r *keyRing) ForceRemoveAll() { +// Reset removes all keys and clears the lock state. It is used when the +// database is wiped or restored. +func (r *keyRing) Reset() { r.mu.Lock() defer r.mu.Unlock() r.keys = nil + r.locked = false + r.pp = nil } func (r *keyRing) Lock(passphrase []byte) error { diff --git a/go/internal/app/app.go b/go/internal/app/app.go index aea7b84..9c2a953 100644 --- a/go/internal/app/app.go +++ b/go/internal/app/app.go @@ -199,7 +199,9 @@ func (a *App) initPresetConfig() error { } for i, host := range a.cfg.Hosts { if err := a.addHost(host.Name, host.Key); err != nil { - return fmt.Errorf("hosts[%d]: %w", i, err) + // Report the bad entry, but keep processing the rest of + // the preset config. + a.term.Errorf("hosts[%d]: %v", i, err) } } for i, k := range a.cfg.GenerateKeys { diff --git a/go/internal/app/db.go b/go/internal/app/db.go index 6fa6c1a..6ac86a7 100644 --- a/go/internal/app/db.go +++ b/go/internal/app/db.go @@ -108,7 +108,7 @@ func (a *App) dbCommand() *cli.App { if !a.term.Confirm("You are about to WIPE the database.\nContinue? ", false) { return errors.New("aborted") } - globalAgent.(*keyRing).ForceRemoveAll() + globalAgent.(*keyRing).Reset() a.data.Authorities = make(map[string]*authority) a.data.Endpoints = make(map[string]*endpoint) a.data.Hosts = make(map[string]*host) @@ -244,7 +244,7 @@ func (a *App) dbCommand() *cli.App { if a.cfg.Persist != nil { newData.Persist = *a.cfg.Persist } - globalAgent.(*keyRing).ForceRemoveAll() + globalAgent.(*keyRing).Reset() a.data = newData if !a.data.Persist { if a.db != nil { diff --git a/go/internal/app/keys.go b/go/internal/app/keys.go index 63a5120..6b05952 100644 --- a/go/internal/app/keys.go +++ b/go/internal/app/keys.go @@ -97,7 +97,7 @@ func (a *App) generateKey(name, passphrase, idp, typ string, bits int, resident Provider: idp, errorf: a.term.Errorf, } - globalAgent.(*keyRing).RemoveByName(name) + globalAgent.(*keyRing).ForceRemoveByName(name) a.data.Keys[name] = k return k, nil } @@ -277,7 +277,7 @@ func (a *App) keysCommand() *cli.App { a.term.Errorf("certificate update: %v", err) } } - globalAgent.(*keyRing).RemoveByName(name) + globalAgent.(*keyRing).ForceRemoveByName(name) a.data.Keys[name] = k if err := a.saveKeys(true); err != nil { return err @@ -307,10 +307,10 @@ func (a *App) keysCommand() *cli.App { } if key, exists := a.data.Keys[name]; exists { if pub, err := key.sshPublicKey(); err == nil { - globalAgent.Remove(pub) + globalAgent.(*keyRing).ForceRemove(pub) } } - globalAgent.(*keyRing).RemoveByName(name) + globalAgent.(*keyRing).ForceRemoveByName(name) delete(a.data.Keys, name) return a.saveKeys(true) }, @@ -471,7 +471,7 @@ func (a *App) keysCommand() *cli.App { } key.Public = sshPub.Marshal() } - globalAgent.(*keyRing).RemoveByName(name) + globalAgent.(*keyRing).ForceRemoveByName(name) a.data.Keys[name] = key if err := a.saveKeys(true); err != nil { return err diff --git a/go/internal/app/ssh.go b/go/internal/app/ssh.go index 1fbd5a3..7f8d252 100644 --- a/go/internal/app/ssh.go +++ b/go/internal/app/ssh.go @@ -150,8 +150,6 @@ func (a *App) runSSH(ctx context.Context, target, keyName, command string, forwa session.Stderr = t if command != "" { - a.inShell.Store(true) - defer a.inShell.Store(false) return session.Run(command) } modes := ssh.TerminalModes{ diff --git a/go/internal/jsutil/streams.go b/go/internal/jsutil/streams.go index 68eccac..31bdfc8 100644 --- a/go/internal/jsutil/streams.go +++ b/go/internal/jsutil/streams.go @@ -142,6 +142,10 @@ func NewStreamHelper() *StreamHelper { streamHelperMu.Lock() defer streamHelperMu.Unlock() + if defaultStreamHelper != nil { + return defaultStreamHelper + } + container := js.Global().Get("navigator").Get("serviceWorker") if !container.Truthy() { return nil @@ -160,10 +164,6 @@ func NewStreamHelper() *StreamHelper { return nil } - if defaultStreamHelper != nil { - return defaultStreamHelper - } - h := &StreamHelper{ streams: make(map[string]stream), } @@ -269,14 +269,6 @@ func waitForController(container, registration js.Value, timeout time.Duration) } } -func UnregisterServiceWorker() { - if c := js.Global().Get("navigator").Get("serviceWorker"); c.Truthy() { - if r, err := Await(c.Call("getRegistration")); err == nil && r.Truthy() { - r.Call("unregister") - } - } -} - type StreamHelper struct { mu sync.Mutex streams map[string]stream diff --git a/go/internal/webauthnsk/key.go b/go/internal/webauthnsk/key.go index 748ab66..9455d01 100644 --- a/go/internal/webauthnsk/key.go +++ b/go/internal/webauthnsk/key.go @@ -74,7 +74,7 @@ func Create(name string, resident bool) (*Key, error) { if err != nil { return nil, fmt.Errorf("WebAuthnCreate: %w", err) } - if err := verifyClientData(resp.ClientDataJSON, "webauthn.create", challenge); err != nil { + if _, err := verifyClientData(resp.ClientDataJSON, "webauthn.create", challenge); err != nil { return nil, err } att, err := parseAttestationObject(resp.AttestationObject) @@ -119,7 +119,7 @@ func Discover(promptTouch func(string)) (*Key, string, error) { if err != nil { return nil, "", fmt.Errorf("WebAuthnGet: %w", err) } - if err := verifyClientData(resp1.ClientDataJSON, "webauthn.get", challenge1); err != nil { + if _, err := verifyClientData(resp1.ClientDataJSON, "webauthn.get", challenge1); err != nil { return nil, "", fmt.Errorf("assertion 1: %w", err) } @@ -137,7 +137,7 @@ func Discover(promptTouch func(string)) (*Key, string, error) { if err != nil { return nil, "", fmt.Errorf("WebAuthnGet: %w", err) } - if err := verifyClientData(resp2.ClientDataJSON, "webauthn.get", challenge2); err != nil { + if _, err := verifyClientData(resp2.ClientDataJSON, "webauthn.get", challenge2); err != nil { return nil, "", fmt.Errorf("assertion 2: %w", err) } if !bytes.Equal(resp1.ID, resp2.ID) { @@ -187,12 +187,9 @@ func (k *Key) Sign(_ io.Reader, data []byte) (*ssh.Signature, error) { if err := parseAuthenticatorData(resp.AuthenticatorData, &ad); err != nil { return nil, fmt.Errorf("ParseAuthenticatorData: %w", err) } - if err := verifyClientData(resp.ClientDataJSON, "webauthn.get", data); err != nil { - return nil, err - } - cd, err := parseClientData(resp.ClientDataJSON) + cd, err := verifyClientData(resp.ClientDataJSON, "webauthn.get", data) if err != nil { - return nil, fmt.Errorf("ParseClientData: %w", err) + return nil, err } var sig struct { diff --git a/go/internal/webauthnsk/webauthn.go b/go/internal/webauthnsk/webauthn.go index 252bef7..efcbaa7 100644 --- a/go/internal/webauthnsk/webauthn.go +++ b/go/internal/webauthnsk/webauthn.go @@ -201,23 +201,23 @@ func parseClientData(js []byte) (*clientData, error) { // verifyClientData checks that a ClientDataJSON blob belongs to the ceremony // the caller just requested, i.e. that it has the expected type and echoes back -// the challenge that was sent. -func verifyClientData(js []byte, wantType string, wantChallenge []byte) error { +// the challenge that was sent. It returns the parsed client data. +func verifyClientData(js []byte, wantType string, wantChallenge []byte) (*clientData, error) { cd, err := parseClientData(js) if err != nil { - return fmt.Errorf("ParseClientData: %w", err) + return nil, fmt.Errorf("ParseClientData: %w", err) } if cd.Type != wantType { - return fmt.Errorf("unexpected client data type %q", cd.Type) + return nil, fmt.Errorf("unexpected client data type %q", cd.Type) } challenge, err := base64.RawURLEncoding.DecodeString(cd.Challenge) if err != nil { - return fmt.Errorf("invalid client data challenge: %w", err) + return nil, fmt.Errorf("invalid client data challenge: %w", err) } if !bytes.Equal(challenge, wantChallenge) { - return errors.New("client data challenge doesn't match") + return nil, errors.New("client data challenge doesn't match") } - return nil + return cd, nil } // isPrintableASCII reports whether s consists only of printable ASCII diff --git a/go/internal/webauthnsk/webauthn_test.go b/go/internal/webauthnsk/webauthn_test.go index a53c725..2ddec3d 100644 --- a/go/internal/webauthnsk/webauthn_test.go +++ b/go/internal/webauthnsk/webauthn_test.go @@ -222,7 +222,7 @@ func TestVerifyClientData(t *testing.T) { {"not json", `nope`, true}, } { t.Run(tc.name, func(t *testing.T) { - err := verifyClientData([]byte(tc.js), "webauthn.get", challenge) + _, err := verifyClientData([]byte(tc.js), "webauthn.get", challenge) if got := err != nil; got != tc.wantErr { t.Errorf("verifyClientData() error = %v, wantErr %v", err, tc.wantErr) } @@ -303,7 +303,7 @@ func TestKeyMarshalUnmarshal(t *testing.T) { } // Downgrading a v2 key to v1 must also fail AES-GCM authentication - downgraded := append([]byte(nil), tampered...) + downgraded := append([]byte(nil), block.Bytes...) downgraded[0] = 1 downgradedPEM := pem.EncodeToMemory(&pem.Block{Type: block.Type, Bytes: downgraded}) if _, err := Unmarshal(downgradedPEM, "test", rp); err == nil {