From 9d892e9494cfc821b4a893e6a4e71ba9f9b26e44 Mon Sep 17 00:00:00 2001 From: Jared Ping <13906915+JaredP94@users.noreply.github.com> Date: Mon, 2 Nov 2026 15:19:31 +0200 Subject: [PATCH] fix(cluster-manager): reject unsafe peer identifiers in the trust store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The trusted-node store joined a peer's UUID straight into a file path. The UUID comes from the network — a pairing exchange or a roster entry another trusted member endorsed — so a crafted identifier such as "../identity" could make a roster merge write a pin outside trusted/, overwriting this node's identity.json. Validate identifiers before deriving pin paths: reject empty identifiers, dot components, either platform's separators, Windows drive/stream syntax, NUL bytes and native reserved names. Load skips such files; Pin and Remove refuse them without touching disk or announcing a change. Ported from NVIDIA/Personal-AI-Router@c299135 (NVIDIA/Personal-AI-Router#159). The tests are adapted to this fork's test layout. Co-authored-by: Kaylee Lubick Signed-off-by: Jared Ping <13906915+JaredP94@users.noreply.github.com> --- services/nvpair-cluster-manager/README.md | 7 + .../nvpair-cluster-manager/connreuse_test.go | 2 +- .../review_closure_test.go | 2 +- services/nvpair-cluster-manager/truststore.go | 42 +++- .../truststore_paths_test.go | 235 ++++++++++++++++++ 5 files changed, 281 insertions(+), 7 deletions(-) create mode 100644 services/nvpair-cluster-manager/truststore_paths_test.go diff --git a/services/nvpair-cluster-manager/README.md b/services/nvpair-cluster-manager/README.md index e833d6a89..ef29dfa0a 100644 --- a/services/nvpair-cluster-manager/README.md +++ b/services/nvpair-cluster-manager/README.md @@ -326,6 +326,13 @@ mechanism (no CRL/OCSP). This keypair + `trusted/` store is the shared trust fabric other inter-node services (`nvpair-workload-manager`, `nvpair-errors` peer-sync) are intended to reuse. Full format in spec §7.4. +A peer's UUID reaches the store from the network (pairing, endorsed roster +entries), so the store treats it as an opaque identifier, never a path. It +refuses to load, pin, or remove an identifier that is not a single local +filename component — empty, `.`/`..`, either platform's separators, Windows +drive or stream syntax, NUL bytes, or a reserved device name — without +cleaning or renaming it, and without touching disk or announcing a change. + ## Inter-node networking - One HTTP listener on TCP **`14321`** serves both the plain-HTTP **pairing diff --git a/services/nvpair-cluster-manager/connreuse_test.go b/services/nvpair-cluster-manager/connreuse_test.go index d86c4cfbd..83254c579 100644 --- a/services/nvpair-cluster-manager/connreuse_test.go +++ b/services/nvpair-cluster-manager/connreuse_test.go @@ -104,7 +104,7 @@ func TestPeerClient_ForgetRevokesWithPinStillOnDisk(t *testing.T) { if _, err := m.peerClient(peerUUID); err != nil { t.Fatalf("pinned peer must yield a client: %v", err) } - pinPath := m.trust.pinPath(peerUUID) + pinPath := mustPinPath(t, m.trust, peerUUID) if _, err := os.Stat(pinPath); err != nil { t.Fatalf("stat pin before Forget: %v", err) } diff --git a/services/nvpair-cluster-manager/review_closure_test.go b/services/nvpair-cluster-manager/review_closure_test.go index d2a2d6ec6..59e510948 100644 --- a/services/nvpair-cluster-manager/review_closure_test.go +++ b/services/nvpair-cluster-manager/review_closure_test.go @@ -60,7 +60,7 @@ func TestFailedTeardownBlocksTrustAndReadmission(t *testing.T) { AdmissionEpoch: 1, State: stateMember, }) - pinPath := m.trust.pinPath(peer.uuid) + pinPath := mustPinPath(t, m.trust, peer.uuid) if err := os.Remove(pinPath); err != nil { t.Fatal(err) } diff --git a/services/nvpair-cluster-manager/truststore.go b/services/nvpair-cluster-manager/truststore.go index e6a48bb75..e3162e9a8 100644 --- a/services/nvpair-cluster-manager/truststore.go +++ b/services/nvpair-cluster-manager/truststore.go @@ -103,8 +103,24 @@ func newTrustStore(clusterDir string) (*TrustStore, error) { return ts, nil } -func (ts *TrustStore) pinPath(uuid string) string { - return filepath.Join(ts.dir, uuid+".json") +// pinFilename treats a peer principal as an opaque identifier, never a path. +// Reject both platforms' separators and Windows drive/stream syntax even when +// running on Unix. Check the bare identifier as well as the suffixed filename +// so an extension cannot mask a Windows reserved principal such as CON. +func pinFilename(uuid string) (string, error) { + name := uuid + ".json" + if uuid == "." || uuid == ".." || strings.ContainsAny(uuid, "/\\:\x00") || !filepath.IsLocal(uuid) || !filepath.IsLocal(name) { + return "", fmt.Errorf("invalid node UUID %q: expected a local filename component", uuid) + } + return name, nil +} + +func (ts *TrustStore) pinPath(uuid string) (string, error) { + name, err := pinFilename(uuid) + if err != nil { + return "", err + } + return filepath.Join(ts.dir, name), nil } // load reads trusted/*.json into memory, skipping *.tmp, unparseable files, and @@ -124,7 +140,12 @@ func (ts *TrustStore) load() { continue } fileUUID := strings.TrimSuffix(name, ".json") - data, err := os.ReadFile(filepath.Join(ts.dir, name)) + path, err := ts.pinPath(fileUUID) + if err != nil { + log.Printf("trusted store: skip unsafe filename %q: %v", name, err) + continue + } + data, err := os.ReadFile(path) if err != nil { log.Printf("trusted store: read %s: %v", name, err) continue @@ -154,6 +175,9 @@ func (ts *TrustStore) load() { // validatePin checks that the pin's inner nodeUuid and embedded certificate // subject/URI all agree with the expected UUID, returning the parsed cert DER. func validatePin(pin *TrustedPin, expectUUID string) ([]byte, error) { + if _, err := pinFilename(expectUUID); err != nil { + return nil, err + } if pin.NodeUUID != expectUUID { return nil, fmt.Errorf("inner nodeUuid %q != filename %q", pin.NodeUUID, expectUUID) } @@ -225,11 +249,15 @@ func (ts *TrustStore) Pin(pin *TrustedPin) error { // writePinLocked persists a pin atomically and updates the in-memory pin map. // Caller holds ts.mu. func (ts *TrustStore) writePinLocked(pin *TrustedPin) error { + path, err := ts.pinPath(pin.NodeUUID) + if err != nil { + return err + } data, err := json.MarshalIndent(pin, "", " ") if err != nil { return err } - if err := atomicWrite(ts.pinPath(pin.NodeUUID), data, 0o600); err != nil { + if err := atomicWrite(path, data, 0o600); err != nil { return err } ts.pins[pin.NodeUUID] = cloneTrustedPin(pin) @@ -370,11 +398,15 @@ func (ts *TrustStore) PubKey(uuid string) (ed25519.PublicKey, bool) { // Remove deletes a peer's pin. A missing file is treated as already-removed // (idempotent success). func (ts *TrustStore) Remove(uuid string) error { + path, err := ts.pinPath(uuid) + if err != nil { + return err + } changed := false defer ts.announce(&changed) ts.mu.Lock() defer ts.mu.Unlock() - if err := os.Remove(ts.pinPath(uuid)); err != nil && !os.IsNotExist(err) { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { return fmt.Errorf("remove pin %s: %w", uuid, err) } _, held := ts.pins[uuid] diff --git a/services/nvpair-cluster-manager/truststore_paths_test.go b/services/nvpair-cluster-manager/truststore_paths_test.go new file mode 100644 index 000000000..cb7ae5226 --- /dev/null +++ b/services/nvpair-cluster-manager/truststore_paths_test.go @@ -0,0 +1,235 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "bytes" + "crypto/ed25519" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/json" + "encoding/pem" + "math/big" + "os" + "path/filepath" + "runtime" + "testing" + "time" +) + +func mustPinPath(t *testing.T, ts *TrustStore, uuid string) string { + t.Helper() + path, err := ts.pinPath(uuid) + if err != nil { + t.Fatalf("pin path: %v", err) + } + return path +} + +// pinWithCommonName supplies a real certificate whose CN carries the principal, +// including crafted identifiers that a trusted peer can sign and endorse. +func pinWithCommonName(t *testing.T, uuid string) *TrustedPin { + t.Helper() + pub, priv, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatalf("generate key: %v", err) + } + cert := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: uuid}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + } + der, err := x509.CreateCertificate(rand.Reader, cert, cert, pub, priv) + if err != nil { + t.Fatalf("create certificate: %v", err) + } + return &TrustedPin{ + NodeUUID: uuid, + ClusterID: "cluster-1", + CertPem: string(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der})), + } +} + +func TestTrustStoreRejectsUnsafePinIdentifiers(t *testing.T) { + test := func(name, uuid string) { + t.Run(name, func(t *testing.T) { + ts, count := newAnnouncingStore(t) + victim := filepath.Join(filepath.Dir(ts.dir), "victim.json") + if err := os.WriteFile(victim, []byte("preserve me"), 0o600); err != nil { + t.Fatalf("write victim: %v", err) + } + if err := ts.Pin(pinWithCommonName(t, uuid)); err == nil { + t.Fatal("unsafe principal was pinned") + } + data, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("read victim: %v", err) + } + if string(data) != "preserve me" { + t.Errorf("victim file changed: %q", data) + } + entries, err := os.ReadDir(ts.dir) + if err != nil { + t.Fatalf("read trusted directory: %v", err) + } + if len(entries) != 0 || len(ts.List()) != 0 || count() != 0 { + t.Errorf("rejected pin mutated store: files=%d pins=%d announcements=%d", len(entries), len(ts.List()), count()) + } + }) + } + test("empty principal", "") + test("current directory", ".") + test("parent directory", "..") + test("Unix traversal", "../victim") + test("Windows traversal", `..\victim`) + test("nested Unix traversal", "sub/../../victim") + test("nested Windows traversal", `sub\..\..\victim`) + test("absolute path", filepath.Join(t.TempDir(), "victim")) + test("drive relative path", "C:victim") + test("UNC path", `\\server\share\victim`) + test("alternate data stream", "peer:stream") + test("NUL byte", "peer\x00victim") + if runtime.GOOS == "windows" { + test("Windows reserved device", "CON") + test("Windows reserved serial port", "COM1") + test("Windows reserved printer port", "LPT1") + } +} + +func TestTrustStoreRejectsUnsafeRemoveIdentifiers(t *testing.T) { + test := func(name, uuid string) { + t.Run(name, func(t *testing.T) { + ts, count := newAnnouncingStore(t) + pin := testPin(t, "safe-peer") + if err := ts.Pin(pin); err != nil { + t.Fatalf("pin safe peer: %v", err) + } + victim := filepath.Join(filepath.Dir(ts.dir), "victim.json") + if err := os.WriteFile(victim, []byte("preserve me"), 0o600); err != nil { + t.Fatalf("write victim: %v", err) + } + if err := ts.Remove(uuid); err == nil { + t.Fatal("unsafe removal succeeded") + } + data, err := os.ReadFile(victim) + if err != nil { + t.Fatalf("read victim: %v", err) + } + if string(data) != "preserve me" { + t.Errorf("victim file changed: %q", data) + } + if _, ok := ts.Get(pin.NodeUUID); !ok || len(ts.List()) != 1 || count() != 1 { + t.Errorf("rejected removal mutated store: safePeer=%t pins=%d announcements=%d", ok, len(ts.List()), count()) + } + }) + } + test("empty principal", "") + test("current directory", ".") + test("parent directory", "..") + test("Unix traversal", "../victim") + test("Windows traversal", `..\victim`) + test("nested Unix traversal", "sub/../../victim") + test("nested Windows traversal", `sub\..\..\victim`) + test("absolute path", filepath.Join(t.TempDir(), "victim")) + test("drive relative path", "C:victim") + test("UNC path", `\\server\share\victim`) + test("alternate data stream", "peer:stream") + test("NUL byte", "peer\x00victim") + if runtime.GOOS == "windows" { + test("Windows reserved device", "CON") + test("Windows reserved serial port", "COM1") + test("Windows reserved printer port", "LPT1") + } +} + +func TestTrustStoreLoadSkipsUnsafePinIdentifiers(t *testing.T) { + test := func(name, uuid string) { + t.Run(name, func(t *testing.T) { + ts, err := newTrustStore(t.TempDir()) + if err != nil { + t.Fatalf("open store: %v", err) + } + data, err := json.Marshal(pinWithCommonName(t, uuid)) + if err != nil { + t.Fatalf("encode pin: %v", err) + } + if err := os.WriteFile(filepath.Join(ts.dir, uuid+".json"), data, 0o600); err != nil { + t.Fatalf("write unsafe pin: %v", err) + } + reloaded, err := newTrustStore(filepath.Dir(ts.dir)) + if err != nil { + t.Fatalf("reload store: %v", err) + } + if len(reloaded.List()) != 0 { + t.Fatal("unsafe pin was loaded") + } + }) + } + test("empty principal", "") + test("current directory principal", ".") + test("parent directory principal", "..") +} + +func TestTrustStoreOpaqueSafeIdentifierSurvivesReload(t *testing.T) { + ts, err := newTrustStore(t.TempDir()) + if err != nil { + t.Fatalf("open store: %v", err) + } + pin := testPin(t, "peer.name-1") + if err := ts.Pin(pin); err != nil { + t.Fatalf("pin safe opaque identifier: %v", err) + } + reloaded, err := newTrustStore(filepath.Dir(ts.dir)) + if err != nil { + t.Fatalf("reload store: %v", err) + } + if got, ok := reloaded.Get(pin.NodeUUID); !ok || got.CertPem != pin.CertPem { + t.Fatal("safe opaque identifier did not survive reload") + } + if err := reloaded.Remove(pin.NodeUUID); err != nil { + t.Fatalf("remove safe pin: %v", err) + } + if err := reloaded.Remove(pin.NodeUUID); err != nil { + t.Fatalf("repeat safe removal: %v", err) + } +} + +func TestMergeRosterRejectsTraversalPrincipal(t *testing.T) { + m := newTestManager(t) + peerUUID, peerCert, peerFP, peerKey := makeNode(t, "trusted-peer") + pinTrusted(t, m, peerUUID, peerCert, peerFP) + identityPath := filepath.Join(filepath.Dir(m.trust.dir), "identity.json") + before, err := os.ReadFile(identityPath) + if err != nil { + t.Fatalf("read identity before merge: %v", err) + } + pin := pinWithCommonName(t, "../identity") + fingerprint, err := certFingerprintFromPEM([]byte(pin.CertPem)) + if err != nil { + t.Fatalf("fingerprint crafted certificate: %v", err) + } + endorsement := signEndorsement(peerKey, peerUUID, pin.NodeUUID, fingerprint, "cluster-1", time.Now().UnixMilli(), 1, 1) + m.mergeRoster(&Roster{ + ClusterID: "cluster-1", + Members: []RosterEntry{{ + NodeUUID: pin.NodeUUID, CertPem: pin.CertPem, CertFingerprint: fingerprint, + AdmissionEpoch: 1, Endorsements: []Endorsement{endorsement}, + }}, + }, peerUUID) + if _, ok := m.trust.Get(pin.NodeUUID); ok { + t.Error("trusted endorsement admitted a traversal principal") + } + if _, ok := m.memberByNodeID(pin.NodeUUID); ok { + t.Error("traversal principal became a member") + } + after, err := os.ReadFile(identityPath) + if err != nil { + t.Fatalf("read identity after merge: %v", err) + } + if !bytes.Equal(before, after) { + t.Error("roster merge overwrote identity.json") + } +}