Another round of security fixes

CHANGELOG.md

@@ -2,6 +2,30 @@
 
 ## 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`, 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.
+
+### :wrench: Bug fixes
+
+* Fix `agent remove <name>` 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

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();

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
 		}
@@ -299,15 +325,55 @@ 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 {
-		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
+	}
+	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 {
 			found = true
 			return true
 		}
@@ -329,6 +395,16 @@ func (r *keyRing) RemoveAll() error {
 	return nil
 }
 
+// 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 {
 	r.mu.Lock()
 	defer r.mu.Unlock()

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
 }
 
@@ -197,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 {
@@ -306,8 +310,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 +329,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 +369,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 +397,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 +491,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
 	}

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
 }

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
+}