Merge pull request #14 from netfoundry/security_review_fixes

Security review fixes

Author: emoscardini

client/sftp.go

@@ -16,6 +16,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 	"time"
 
 	"github.com/pkg/sftp"
@@ -48,6 +49,11 @@ func RunSFTP(
 	cfg := &ssh.ClientConfig{
 		User:            user,
 		Auth:            []ssh.AuthMethod{ssh.PublicKeys(signer)},
+		// Host key verification is intentionally skipped. The connection arrives
+		// over a Ziti overlay that enforces mutual TLS using the controller's
+		// PKI — the host's Ziti identity is cryptographically proven before any
+		// SSH bytes are exchanged. A traditional known-hosts check would be
+		// redundant and weaker than the guarantee Ziti already provides.
 		HostKeyCallback: ssh.InsecureIgnoreHostKey(), //nolint:gosec
 	}
 
@@ -272,6 +278,9 @@ func downloadDir(client *sftp.Client, remoteDir, localDir string, preserve, quie
 	}
 
 	for _, entry := range entries {
+		if err := safeName(entry.Name()); err != nil {
+			return fmt.Errorf("remote dir %q: %w", remoteDir, err)
+		}
 		rPath := path.Join(remoteDir, entry.Name())
 		lPath := filepath.Join(localDir, entry.Name())
 		if entry.IsDir() {
@@ -342,6 +351,19 @@ func downloadFile(client *sftp.Client, remotePath, localPath string, preserve, q
 	return nil
 }
 
+// safeName rejects remote-supplied entry names that could escape the local
+// download target via path traversal (e.g. "..", "../etc/passwd", or names
+// embedding a path separator).
+func safeName(name string) error {
+	if name == "" || name == "." || name == ".." {
+		return fmt.Errorf("rejected unsafe remote entry name %q", name)
+	}
+	if strings.ContainsAny(name, `/\`) {
+		return fmt.Errorf("rejected unsafe remote entry name %q", name)
+	}
+	return nil
+}
+
 // ---------------------------------------------------------------------------
 // Progress reporting
 // ---------------------------------------------------------------------------

cmd/ziti-scp/main.go

@@ -277,7 +277,7 @@ func runSign(p signParams) error {
 	}
 
 	certPath := deriveCertPath(privKeyPath)
-	if err := os.WriteFile(certPath, []byte(certLine+"\n"), 0644); err != nil {
+	if err := config.AtomicWriteFile(certPath, []byte(certLine+"\n"), 0644); err != nil {
 		return fmt.Errorf("write certificate to %q: %w", certPath, err)
 	}
 
@@ -322,7 +322,7 @@ func runEnroll(jwtPath, outPath string) error {
 	if err != nil {
 		return fmt.Errorf("marshal identity config: %w", err)
 	}
-	if err := os.WriteFile(outPath, cfgJSON, 0600); err != nil {
+	if err := config.AtomicWriteFile(outPath, cfgJSON, 0600); err != nil {
 		return fmt.Errorf("write identity file %q: %w", outPath, err)
 	}
 

cmd/ziti-ssh-ca/main.go

@@ -12,6 +12,7 @@ import (
 	"bufio"
 	"encoding/json"
 	"fmt"
+	"io"
 	"log/slog"
 	"net"
 	"net/url"
@@ -205,7 +206,7 @@ func runEnroll(jwtPath, outPath string) error {
 	if err != nil {
 		return fmt.Errorf("marshal identity config: %w", err)
 	}
-	if err := os.WriteFile(outPath, cfgJSON, 0600); err != nil {
+	if err := config.AtomicWriteFile(outPath, cfgJSON, 0600); err != nil {
 		return fmt.Errorf("write identity file %q: %w", outPath, err)
 	}
 
@@ -379,7 +380,15 @@ func handleConn(conn net.Conn, signer ssh.Signer, caPubBytes []byte, principal,
 		log = log.With("derived_principal", effectivePrincipal)
 	}
 
-	reader := bufio.NewReader(conn)
+	// Bound the connection lifetime: 30 s is generous for a single-line request.
+	// The size cap (4 KiB) is well above any real SSH public key but prevents a
+	// slow-sender from holding the connection indefinitely.
+	if err := conn.SetDeadline(time.Now().Add(30 * time.Second)); err != nil {
+		log.Error("set deadline", "err", err)
+		return
+	}
+	const maxRequestBytes = 4096
+	reader := bufio.NewReader(io.LimitReader(conn, maxRequestBytes))
 	line, err := reader.ReadString('\n')
 	if err != nil {
 		log.Error("read request", "err", err)

cmd/ziti-ssh-host/main.go

@@ -13,6 +13,7 @@
 package main
 
 import (
+	"bytes"
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/json"
@@ -253,7 +254,7 @@ func runEnroll(jwtPath, identityFile string) error {
 	if err != nil {
 		return fmt.Errorf("marshal identity config: %w", err)
 	}
-	if err := os.WriteFile(identityFile, cfgJSON, 0600); err != nil {
+	if err := config.AtomicWriteFile(identityFile, cfgJSON, 0600); err != nil {
 		return fmt.Errorf("write identity file %q: %w", identityFile, err)
 	}
 	slog.Info("identity enrolled", "path", identityFile)
@@ -456,8 +457,22 @@ func intCAFromController(controllerURL string, rootPool *x509.CertPool) (*x509.C
 			"selfSigned", cert.Subject.String() == cert.Issuer.String())
 	}
 
+	if len(state.PeerCertificates) == 0 {
+		return nil, fmt.Errorf("no certificates in TLS chain from %q", host)
+	}
+	leaf := state.PeerCertificates[0]
+	// Walk the chain and pick the cert whose Subject matches the leaf's Issuer
+	// field. Raw-bytes comparison avoids encoding differences that string
+	// formatting can obscure, and correctly identifies the signing intermediate
+	// even when the chain contains cross-signed or multiple intermediate certs.
+	for _, cert := range state.PeerCertificates[1:] {
+		if cert.IsCA && bytes.Equal(cert.RawSubject, leaf.RawIssuer) {
+			return cert, nil
+		}
+	}
+	// Fallback to the original heuristic for unusual chain orderings.
 	for _, cert := range state.PeerCertificates {
-		if cert.IsCA && cert.Subject.String() != cert.Issuer.String() {
+		if cert.IsCA && !bytes.Equal(cert.RawSubject, cert.RawIssuer) {
 			return cert, nil
 		}
 	}
@@ -674,11 +689,40 @@ func loadPermissionsConfig(zitiCtx ziti.Context, serviceName string) (*host.Perm
 		Permissions: make(map[string]host.IdentityPermissions, len(wire.Permissions)),
 	}
 	for identity, entry := range wire.Permissions {
+		if entry.SudoersRule != "" {
+			if err := host.ValidateSudoersRule(entry.SudoersRule); err != nil {
+				return nil, fmt.Errorf("service %q config: identity %q: %w", serviceName, identity, err)
+			}
+		}
 		pc.Permissions[identity] = host.IdentityPermissions{
 			Groups:      entry.Groups,
 			SudoersRule: entry.SudoersRule,
 		}
 	}
+	// Validate that no two explicit (non-glob) config keys derive to the same
+	// Linux username. A collision allows one Ziti identity to connect as another
+	// identity's Linux account, potentially inheriting elevated permissions.
+	// Glob patterns are skipped — they match many identities and have no single
+	// derived username. Runtime collision detection in UserManager.EnsureUser
+	// handles the glob case.
+	derived := make(map[string]string, len(pc.Permissions))
+	var collisions []string
+	for identity := range pc.Permissions {
+		if strings.ContainsAny(identity, "*?") {
+			continue
+		}
+		uname := ca.DeriveUsername(identity)
+		if prev, exists := derived[uname]; exists {
+			collisions = append(collisions, fmt.Sprintf("%q and %q both derive to %q", prev, identity, uname))
+		} else {
+			derived[uname] = identity
+		}
+	}
+	if len(collisions) > 0 {
+		return nil, fmt.Errorf("service %q config has username collisions (fix or remove one of each pair): %s",
+			serviceName, strings.Join(collisions, "; "))
+	}
+
 	slog.Info("loaded ziti-ssh-host.v1 config", "service", serviceName, "identities", len(pc.Permissions))
 	return pc, nil
 }
@@ -922,7 +966,7 @@ func runProxy(identityFile string, sshServices []string, mode string, zitiTimeou
 						"username", username,
 						"groups", perms.Groups,
 						"has_sudoers", perms.SudoersRule != "")
-					return mgr.EnsureUser(username, perms)
+					return mgr.EnsureUser(zitiIdentity, username, perms)
 				},
 				OnDisconnect: func(zitiIdentity string) {
 					username := ca.DeriveUsername(zitiIdentity)
@@ -958,6 +1002,10 @@ func runProxy(identityFile string, sshServices []string, mode string, zitiTimeou
 	go func() {
 		sig := <-sigCh
 		slog.Info("received signal, stopping proxy listeners", "signal", sig)
+		// Restore default signal handling so a second SIGINT/SIGTERM exits
+		// immediately if the drain hangs (instead of being silently swallowed
+		// by the now-drained channel).
+		signal.Reset(syscall.SIGTERM, syscall.SIGINT)
 		if _, err := daemon.SdNotify(false, "STOPPING=1"); err != nil {
 			slog.Debug("sd_notify STOPPING failed", "err", err)
 		}

cmd/ziti-ssh/main.go

@@ -357,7 +357,7 @@ func runSign(p signParams) error {
 	}
 
 	certPath := deriveCertPath(privKeyPath)
-	if err := os.WriteFile(certPath, []byte(certLine+"\n"), 0644); err != nil {
+	if err := config.AtomicWriteFile(certPath, []byte(certLine+"\n"), 0644); err != nil {
 		return fmt.Errorf("write certificate to %q: %w", certPath, err)
 	}
 
@@ -882,7 +882,7 @@ func runEnroll(jwtPath, outPath string) error {
 	if err != nil {
 		return fmt.Errorf("marshal identity config: %w", err)
 	}
-	if err := os.WriteFile(outPath, cfgJSON, 0600); err != nil {
+	if err := config.AtomicWriteFile(outPath, cfgJSON, 0600); err != nil {
 		return fmt.Errorf("write identity file %q: %w", outPath, err)
 	}
 

config/atomic.go

@@ -0,0 +1,53 @@
+package config
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+)
+
+// AtomicWriteFile writes data to dest atomically: it creates a sibling temp
+// file, writes and fsyncs the data, sets the file mode, renames it over dest,
+// and fsyncs the parent directory. This ensures that a concurrent reader never
+// sees a partial write and that the new content survives a crash immediately
+// after rename.
+func AtomicWriteFile(dest string, data []byte, mode os.FileMode) error {
+	dir := filepath.Dir(dest)
+	tmp, err := os.CreateTemp(dir, ".tmp-*")
+	if err != nil {
+		return fmt.Errorf("create temp file in %q: %w", dir, err)
+	}
+	tmpName := tmp.Name()
+	committed := false
+	defer func() {
+		if !committed {
+			_ = tmp.Close()
+			_ = os.Remove(tmpName)
+		}
+	}()
+
+	if _, err := tmp.Write(data); err != nil {
+		return fmt.Errorf("write temp file: %w", err)
+	}
+	if err := tmp.Sync(); err != nil {
+		return fmt.Errorf("sync temp file: %w", err)
+	}
+	if err := tmp.Chmod(mode); err != nil {
+		return fmt.Errorf("chmod temp file: %w", err)
+	}
+	if err := tmp.Close(); err != nil {
+		return fmt.Errorf("close temp file: %w", err)
+	}
+
+	if err := os.Rename(tmpName, dest); err != nil {
+		return fmt.Errorf("rename temp file to %q: %w", dest, err)
+	}
+	committed = true
+
+	// Sync the directory so the rename is durable.
+	if d, err := os.Open(dir); err == nil {
+		_ = d.Sync()
+		_ = d.Close()
+	}
+	return nil
+}

config/ztapis.go

@@ -46,13 +46,5 @@ func PersistZtAPIs(identityFile string, urls []string) error {
 		mode = fi.Mode()
 	}
 
-	tmp := identityFile + ".tmp"
-	if err := os.WriteFile(tmp, out, mode); err != nil {
-		return fmt.Errorf("write temp identity file: %w", err)
-	}
-	if err := os.Rename(tmp, identityFile); err != nil {
-		os.Remove(tmp)
-		return fmt.Errorf("rename temp identity file: %w", err)
-	}
-	return nil
+	return AtomicWriteFile(identityFile, out, mode)
 }