From 9615b5f2b0e461809399a7ea101a4e75c082e202 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 6 Aug 2026 18:08:14 +0100 Subject: [PATCH] fix(security): close the four open CodeQL findings Resolves every open code scanning alert on the repository. Dependabot and secret scanning were already clear. SMART temperature truncation (alerts 312, 313). parseRawValue returns a 64-bit raw attribute value, but DiskSMART.Temperature is an int, which is 32 bits wide on the 386 and arm release builds Pulse ships. The range check ran after the narrowing conversion, so a raw value of 4294967316 truncated to 20 and was published as a plausible 20 degree reading. validSMARTTemperature64 now gates the conversion. Provider MSP restore archive names (alert 314). cleanProviderMSPArchiveName rejected a leading "../" but not a bare "..", which path.Clean produces from entries such as ".." and "a/../..". pathIsInside caught the escape downstream, so this was not exploitable, but the sanitizer now rejects it outright instead of depending on a second gate. TrueNAS device paths (alert 315). vdev.Device is supplied by the appliance, concatenated into a path and published verbatim on ZFSDevice.Path, so values like "//evil.example.com/share" and "/\evil.example.com" passed straight through. devicePath now drops traversal segments and backslashes and collapses a leading double slash. The alert's open-redirect framing does not apply here, there is no redirect sink on this path, but the value is untrusted input rendered as a path and is worth normalising. Patrol readiness cache key (alert 311). The key is persisted to ai_patrol_model_readiness.json and embedded an unkeyed SHA-256 of the Ollama Basic Auth username and password. That password is chosen by a human, so anyone holding the evidence file could recover it offline at two SHA-256 operations per guess. The fingerprint is now HMAC-SHA256 keyed with a 32-byte per-install salt stored beside the evidence at mode 600. Credential rotation still invalidates the cache and the key still survives a restart. Each fix carries a regression test confirmed to fail against the previous implementation. monitoring.md carries the one warranted contract refinement. It already required SMART temperature selection to accept only plausible readings, and that rule now states the width at which plausibility is decided. Contract-Neutral: CodeQL security fixes with no public-contract delta and no payload change. monitoring.md carries the one warranted refinement (SMART plausibility decided at 64-bit width). Residual demands are inapplicable: ai-runtime readiness prose documents interruption semantics, not cache-key derivation, and the credential-invalidation contract is unchanged; cloud-paid and deployment-installability contracts never name archive-entry sanitisation; agent-lifecycle owns smartctl.go but its SMART temperature prose lives in the staged monitoring.md. --- .../v6/internal/subsystems/monitoring.md | 7 +- ...sue1624_issue1614_patrol_readiness_test.go | 2 +- internal/ai/issue1640_readiness_gate_test.go | 2 +- internal/ai/patrol_model_readiness.go | 73 ++++++++++- .../patrol_model_readiness_cachekey_test.go | 120 ++++++++++++++++++ internal/ai/patrol_model_readiness_test.go | 8 +- internal/cloudcp/provider_msp_backup.go | 5 +- .../provider_msp_backup_archive_name_test.go | 56 ++++++++ internal/hostagent/smartctl.go | 13 +- .../smartctl_temperature_width_test.go | 43 +++++++ internal/truenas/provider.go | 19 ++- internal/truenas/provider_device_path_test.go | 36 ++++++ 12 files changed, 370 insertions(+), 14 deletions(-) create mode 100644 internal/ai/patrol_model_readiness_cachekey_test.go create mode 100644 internal/cloudcp/provider_msp_backup_archive_name_test.go create mode 100644 internal/hostagent/smartctl_temperature_width_test.go create mode 100644 internal/truenas/provider_device_path_test.go diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 4cd2a96b1..c0f58384c 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1940,7 +1940,12 @@ retry may enrich an earlier smartctl attempt but must not erase earlier model, serial, failure, or counter evidence. SMART temperature selection accepts only plausible readings, prefers ATA attribute 194 over 190 when higher-level temperature fields are invalid, and preserves reported zero counters as known -values while leaving omitted counters unknown. +values while leaving omitted counters unknown. Plausibility is decided at the +full 64-bit width of the raw attribute, before any narrowing to the reported +`int` temperature. `int` is 32 bits wide on the 386 and arm agent builds, so a +raw value whose low 32 bits happen to land in the plausible band, such as +4294967316 truncating to 20, must be rejected as the out-of-range value it is +rather than published as a real reading. Disk identity, temperature, I/O, controller association, and pool membership also carry typed collection state from `pkg/diskinventory`: `available`, diff --git a/internal/ai/issue1624_issue1614_patrol_readiness_test.go b/internal/ai/issue1624_issue1614_patrol_readiness_test.go index 8a5e43bac..f4da61e8a 100644 --- a/internal/ai/issue1624_issue1614_patrol_readiness_test.go +++ b/internal/ai/issue1624_issue1614_patrol_readiness_test.go @@ -166,7 +166,7 @@ func TestIssue1624ReadinessDetailsSurviveCacheCloneAndPersistence(t *testing.T) result.Provider = config.AIProviderOllama result.Model = "test-model" result.Details = []string{`Scenario "typed-tool" tool protocol: nonce did not match`} - result.CacheKey = patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) + result.CacheKey = service.patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) service.recordPatrolModelReadiness(result, time.Now()) reloaded := NewService(persistence, nil) diff --git a/internal/ai/issue1640_readiness_gate_test.go b/internal/ai/issue1640_readiness_gate_test.go index 79f432f72..5d07d8e9b 100644 --- a/internal/ai/issue1640_readiness_gate_test.go +++ b/internal/ai/issue1640_readiness_gate_test.go @@ -65,7 +65,7 @@ func TestIssue1640InterruptedRunKeepsToolPassWithoutAVerdict(t *testing.T) { // interrupted check may not claim readiness, and may not block the run. service := NewService(config.NewConfigPersistence(t.TempDir()), nil) service.cfg = cfg - result.CacheKey = patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) + result.CacheKey = service.patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) service.recordPatrolModelReadiness(result, time.Now()) readiness := service.PatrolRuntimeReadiness() diff --git a/internal/ai/patrol_model_readiness.go b/internal/ai/patrol_model_readiness.go index ee636ad4a..cbe2c54f1 100644 --- a/internal/ai/patrol_model_readiness.go +++ b/internal/ai/patrol_model_readiness.go @@ -2,6 +2,7 @@ package ai import ( "context" + "crypto/hmac" "crypto/rand" "crypto/sha256" "encoding/hex" @@ -36,6 +37,8 @@ const ( PatrolModeNotAssessed = "not_assessed" patrolModelReadinessCacheFilename = "ai_patrol_model_readiness.json" + patrolModelReadinessSaltFilename = "ai_patrol_model_readiness.salt" + patrolModelReadinessSaltBytes = 32 patrolReadinessObservationTool = "readiness_record_observation" patrolReadinessInventoryTool = "readiness_list_inventory" patrolReadinessChangeTool = "readiness_apply_change" @@ -138,6 +141,13 @@ type patrolModelReadinessCache struct { mu sync.RWMutex result *PatrolModelReadinessResult recordedAt time.Time + + // salt keys the credential fingerprint inside the cache key. It is + // resolved lazily from disk so the key stays reproducible across + // restarts without the persisted evidence committing to the + // credentials themselves. + saltMu sync.Mutex + salt []byte } type persistedPatrolModelReadiness struct { @@ -217,7 +227,7 @@ func (s *Service) CachedPatrolModelReadiness() (*PatrolModelReadinessResult, tim } selectedProvider, selectedModel := config.ParseModelString(cfg.GetPatrolModel()) if !strings.EqualFold(selectedProvider, result.Provider) || selectedModel != result.Model || - result.CacheKey != patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) { + result.CacheKey != s.patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) { return nil, time.Time{} } return result, recordedAt @@ -314,10 +324,59 @@ func (s *Service) loadPatrolModelReadiness() { s.patrolModelReadinessCache.recordedAt = persisted.RecordedAt } -func patrolModelReadinessCacheKey(cfg *config.AIConfig, providerName, model string) string { +// patrolModelReadinessSalt returns the per-install secret that keys the +// credential fingerprint in the cache key. It is generated once and stored +// beside the evidence file so the key survives a restart. When there is no +// persistence to store it in, a process-local salt is used instead: the +// fingerprint stays unguessable and the cache simply does not outlive the +// process, which is the safe way to degrade. +func (s *Service) patrolModelReadinessSalt() []byte { + if s == nil { + return nil + } + s.patrolModelReadinessCache.saltMu.Lock() + defer s.patrolModelReadinessCache.saltMu.Unlock() + if len(s.patrolModelReadinessCache.salt) > 0 { + return s.patrolModelReadinessCache.salt + } + + saltPath := "" + if s.persistence != nil { + saltPath = filepath.Join(s.persistence.DataDir(), patrolModelReadinessSaltFilename) + } + if saltPath != "" { + if existing, err := os.ReadFile(saltPath); err == nil && len(existing) == patrolModelReadinessSaltBytes { + s.patrolModelReadinessCache.salt = existing + return existing + } + } + + salt := make([]byte, patrolModelReadinessSaltBytes) + if _, err := rand.Read(salt); err != nil { + // Without entropy there is no safe fingerprint to publish, so + // return nil and let the caller fall back to an empty cache key, + // which forces a fresh readiness probe rather than trusting + // stale evidence. + log.Warn().Err(err).Msg("failed to generate Patrol model readiness salt") + return nil + } + if saltPath != "" { + if err := os.WriteFile(saltPath, salt, 0o600); err != nil { + log.Warn().Err(err).Msg("failed to persist Patrol model readiness salt; readiness evidence will not survive restart") + } + } + s.patrolModelReadinessCache.salt = salt + return salt +} + +func (s *Service) patrolModelReadinessCacheKey(cfg *config.AIConfig, providerName, model string) string { if cfg == nil { return "" } + salt := s.patrolModelReadinessSalt() + if len(salt) == 0 { + return "" + } providerName = strings.ToLower(strings.TrimSpace(providerName)) endpoint := "" switch providerName { @@ -332,7 +391,13 @@ func patrolModelReadinessCacheKey(cfg *config.AIConfig, providerName, model stri if providerName == config.AIProviderOllama { credentialMaterial = cfg.OllamaUsername + "\n" + cfg.OllamaPassword } - credentialFingerprint := sha256.Sum256([]byte(credentialMaterial)) + // Keyed with the per-install salt rather than hashed bare. The cache key + // is written to disk, and an Ollama Basic Auth password is low enough + // entropy that an unkeyed SHA-256 of it would be recoverable offline by + // anyone who obtained the evidence file. + credentialMAC := hmac.New(sha256.New, salt) + credentialMAC.Write([]byte(credentialMaterial)) + credentialFingerprint := credentialMAC.Sum(nil) material := fmt.Sprintf("%s\n%s\n%s\n%x\n%d\n%d\n%d", providerName, strings.TrimSpace(model), @@ -359,7 +424,7 @@ func (s *Service) RunPatrolModelReadiness(ctx context.Context, providerName, mod finish := func() PatrolModelReadinessResult { result.DurationMs = time.Since(started).Milliseconds() if cfg != nil && result.Provider != "" && result.Model != "" { - result.CacheKey = patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) + result.CacheKey = s.patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) } // Cancellation is an operator action or request-budget boundary, not new // evidence about the model. Preserve the last completed evaluation. diff --git a/internal/ai/patrol_model_readiness_cachekey_test.go b/internal/ai/patrol_model_readiness_cachekey_test.go new file mode 100644 index 000000000..5c1df127a --- /dev/null +++ b/internal/ai/patrol_model_readiness_cachekey_test.go @@ -0,0 +1,120 @@ +package ai + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "os" + "path/filepath" + "testing" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" +) + +// The readiness cache key is written to disk in ai_patrol_model_readiness.json. +// It must still change when credentials change, but it must not be a digest an +// attacker holding that file can brute-force back to the Ollama Basic Auth +// password, which is chosen by a human and therefore low entropy. + +func readinessCredentialTestConfig(password string) *config.AIConfig { + cfg := readinessTestConfig() + cfg.OllamaUsername = "pulse" + cfg.OllamaPassword = password + return cfg +} + +func TestPatrolModelReadinessCacheKeyStillTracksCredentialChanges(t *testing.T) { + service := NewService(config.NewConfigPersistence(t.TempDir()), nil) + + first := service.patrolModelReadinessCacheKey(readinessCredentialTestConfig("hunter2"), config.AIProviderOllama, "test-model") + second := service.patrolModelReadinessCacheKey(readinessCredentialTestConfig("hunter3"), config.AIProviderOllama, "test-model") + + if first == "" || second == "" { + t.Fatalf("cache key must be derivable, got %q and %q", first, second) + } + if first == second { + t.Fatal("changing the Ollama password must change the cache key, or stale readiness evidence survives a credential rotation") + } +} + +func TestPatrolModelReadinessCacheKeyIsNotAnUnkeyedPasswordDigest(t *testing.T) { + const password = "hunter2" + cfg := readinessCredentialTestConfig(password) + service := NewService(config.NewConfigPersistence(t.TempDir()), nil) + + key := service.patrolModelReadinessCacheKey(cfg, config.AIProviderOllama, "test-model") + if key == "" { + t.Fatal("expected a cache key") + } + + // The pre-fix construction embedded hex(sha256(username \n password)) + // directly in the hashed material, so anyone holding the evidence file + // could confirm a guessed password with two SHA-256 operations. Rebuild + // that derivation and require the key to have moved off it. + unkeyed := sha256.Sum256([]byte(cfg.OllamaUsername + "\n" + password)) + legacyMaterial := fmt.Sprintf("%s\n%s\n%s\n%x\n%d\n%d\n%d", + config.AIProviderOllama, + "test-model", + cfg.OllamaBaseURL, + unkeyed, + cfg.GetRequestTimeout().Milliseconds(), + cfg.GetPatrolInvestigationBudget(), + cfg.GetPatrolInvestigationTimeout().Milliseconds(), + ) + legacySum := sha256.Sum256([]byte(legacyMaterial)) + if key == hex.EncodeToString(legacySum[:]) { + t.Fatal("cache key is still the unkeyed SHA-256 derivation of the Ollama credentials") + } + + // Two installs with the same credentials must not agree, which is what + // proves the fingerprint is salted rather than a global constant. + other := NewService(config.NewConfigPersistence(t.TempDir()), nil) + if otherKey := other.patrolModelReadinessCacheKey(cfg, config.AIProviderOllama, "test-model"); otherKey == key { + t.Fatal("identical credentials produced identical cache keys across installs; the fingerprint is not salted") + } +} + +func TestPatrolModelReadinessSaltPersistsAtRestAndIsPrivate(t *testing.T) { + dir := t.TempDir() + cfg := readinessCredentialTestConfig("hunter2") + + first := NewService(config.NewConfigPersistence(dir), nil) + key := first.patrolModelReadinessCacheKey(cfg, config.AIProviderOllama, "test-model") + + // A restart must reproduce the key or persisted evidence is worthless. + reloaded := NewService(config.NewConfigPersistence(dir), nil) + if got := reloaded.patrolModelReadinessCacheKey(cfg, config.AIProviderOllama, "test-model"); got != key { + t.Fatalf("cache key changed across restart: %q -> %q", key, got) + } + + saltPath := filepath.Join(dir, patrolModelReadinessSaltFilename) + info, err := os.Stat(saltPath) + if err != nil { + t.Fatalf("stat readiness salt: %v", err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Fatalf("readiness salt permissions = %o, want 600", perm) + } + salt, err := os.ReadFile(saltPath) + if err != nil { + t.Fatalf("read readiness salt: %v", err) + } + if len(salt) != patrolModelReadinessSaltBytes { + t.Fatalf("readiness salt length = %d, want %d", len(salt), patrolModelReadinessSaltBytes) + } +} + +func TestPatrolModelReadinessCacheKeyWorksWithoutPersistence(t *testing.T) { + // A Service with no persistence falls back to a process-local salt. The + // key must still be stable within the process so in-memory caching works. + service := &Service{} + cfg := readinessCredentialTestConfig("hunter2") + + key := service.patrolModelReadinessCacheKey(cfg, config.AIProviderOllama, "test-model") + if key == "" { + t.Fatal("expected a cache key without persistence") + } + if again := service.patrolModelReadinessCacheKey(cfg, config.AIProviderOllama, "test-model"); again != key { + t.Fatalf("cache key unstable within a process: %q -> %q", key, again) + } +} diff --git a/internal/ai/patrol_model_readiness_test.go b/internal/ai/patrol_model_readiness_test.go index 23985aee3..da6e6e8d8 100644 --- a/internal/ai/patrol_model_readiness_test.go +++ b/internal/ai/patrol_model_readiness_test.go @@ -297,7 +297,7 @@ func TestPatrolModelReadinessCachePersistsAndInvalidates(t *testing.T) { result.Model = "test-model" result.Success = true result.Status = PatrolModelReadinessPass - result.CacheKey = patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) + result.CacheKey = first.patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) first.recordPatrolModelReadiness(result, time.Now()) reloaded := NewService(persistence, nil) @@ -336,7 +336,7 @@ func TestRunPatrolModelReadinessCancellationPreservesCompletedEvidence(t *testin completed.Success = true completed.Status = PatrolModelReadinessPass completed.Summary = "completed evidence" - completed.CacheKey = patrolModelReadinessCacheKey(cfg, completed.Provider, completed.Model) + completed.CacheKey = service.patrolModelReadinessCacheKey(cfg, completed.Provider, completed.Model) service.recordPatrolModelReadiness(completed, time.Now()) ctx, cancel := context.WithCancel(context.Background()) @@ -382,7 +382,7 @@ func TestPatrolRuntimeReadinessUsesAdvisorForSelectedAutonomyMode(t *testing.T) result.Provider = config.AIProviderOllama result.Model = "test-model" result.Cause = PatrolFailureCauseModelToolSupportUnverified - result.CacheKey = patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) + result.CacheKey = service.patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) result.Modes.Monitor = PatrolModeSuitability{Status: PatrolModeVerified, Summary: "Watch only verified."} result.Modes.Approval = PatrolModeSuitability{Status: PatrolModeNotSuitable, Summary: "Continuation failed."} service.recordPatrolModelReadiness(result, time.Now()) @@ -393,7 +393,7 @@ func TestPatrolRuntimeReadinessUsesAdvisorForSelectedAutonomyMode(t *testing.T) } cfg.PatrolAutonomyLevel = config.PatrolAutonomyAssisted - result.CacheKey = patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) + result.CacheKey = service.patrolModelReadinessCacheKey(cfg, result.Provider, result.Model) service.recordPatrolModelReadiness(result, time.Now().Add(time.Millisecond)) readiness = service.PatrolRuntimeReadiness() if !readiness.Ready || readiness.Status != PatrolReadinessWarning { diff --git a/internal/cloudcp/provider_msp_backup.go b/internal/cloudcp/provider_msp_backup.go index 07c8dfd0f..2a95ee8b3 100644 --- a/internal/cloudcp/provider_msp_backup.go +++ b/internal/cloudcp/provider_msp_backup.go @@ -1013,7 +1013,10 @@ func cleanProviderMSPArchiveName(raw string) (string, error) { } name = strings.TrimPrefix(name, "./") cleaned := path.Clean(name) - if cleaned == "." || strings.HasPrefix(cleaned, "../") || path.IsAbs(cleaned) { + // ".." must be rejected on its own, not just as a "../" prefix: an entry + // named ".." or "a/../.." cleans to exactly ".." and would otherwise pass + // this gate and resolve to the parent of the restore target. + if cleaned == "." || cleaned == ".." || strings.HasPrefix(cleaned, "../") || path.IsAbs(cleaned) { return "", fmt.Errorf("backup archive contains unsafe entry name %q", raw) } return cleaned, nil diff --git a/internal/cloudcp/provider_msp_backup_archive_name_test.go b/internal/cloudcp/provider_msp_backup_archive_name_test.go new file mode 100644 index 000000000..a7f6e2084 --- /dev/null +++ b/internal/cloudcp/provider_msp_backup_archive_name_test.go @@ -0,0 +1,56 @@ +package cloudcp + +import "testing" + +// cleanProviderMSPArchiveName is the first gate on attacker-influenced tar +// entry names during a provider MSP backup restore. pathIsInside catches an +// escape downstream, but this gate must reject traversal on its own so a +// future caller that skips the join check does not inherit a hole. +func TestCleanProviderMSPArchiveNameRejectsTraversal(t *testing.T) { + unsafe := []string{ + "..", + "../", + "../etc/passwd", + "./..", + "a/../..", + "control-plane/../../..", + `..\..\windows`, + "/etc/passwd", + "/", + "", + " ", + } + + for _, raw := range unsafe { + t.Run(raw, func(t *testing.T) { + cleaned, err := cleanProviderMSPArchiveName(raw) + if err == nil { + t.Fatalf("cleanProviderMSPArchiveName(%q) = %q, want an error", raw, cleaned) + } + }) + } +} + +func TestCleanProviderMSPArchiveNameAcceptsLegitimateEntries(t *testing.T) { + cases := map[string]string{ + "control-plane/state.json": "control-plane/state.json", + "./control-plane/state.json": "control-plane/state.json", + "tenants/acme/pulse.db": "tenants/acme/pulse.db", + `tenants\acme\pulse.db`: "tenants/acme/pulse.db", + "tenants/acme/./pulse.db": "tenants/acme/pulse.db", + "tenants/acme/nested/../pulse.db": "tenants/acme/pulse.db", + "manifest.json": "manifest.json", + } + + for raw, want := range cases { + t.Run(raw, func(t *testing.T) { + got, err := cleanProviderMSPArchiveName(raw) + if err != nil { + t.Fatalf("cleanProviderMSPArchiveName(%q) error = %v", raw, err) + } + if got != want { + t.Fatalf("cleanProviderMSPArchiveName(%q) = %q, want %q", raw, got, want) + } + }) + } +} diff --git a/internal/hostagent/smartctl.go b/internal/hostagent/smartctl.go index 63d8e5b49..30f2a813b 100644 --- a/internal/hostagent/smartctl.go +++ b/internal/hostagent/smartctl.go @@ -1893,7 +1893,11 @@ func parseSMARTOutput(output []byte, target smartctlTarget) (*DiskSMART, error) continue } temp := parseRawValue(attr.Raw.String, attr.Raw.Value) - if validSMARTTemperature(int(temp)) { + // Range-check in int64 before narrowing: parseRawValue + // returns a full 64-bit value and int is 32 bits on the + // 386/arm builds, so converting first would let a raw + // value such as 1<<32+20 truncate into the valid band. + if validSMARTTemperature64(temp) { result.Temperature = int(temp) break } @@ -1939,6 +1943,13 @@ func parseSMARTOutput(output []byte, target smartctlTarget) (*DiskSMART, error) } func validSMARTTemperature(value int) bool { + return validSMARTTemperature64(int64(value)) +} + +// validSMARTTemperature64 is the authoritative range check. Callers holding a +// 64-bit raw SMART value must use it before narrowing to int, which is only +// 32 bits wide on the 386 and arm release builds. +func validSMARTTemperature64(value int64) bool { return value > 0 && value < 150 } diff --git a/internal/hostagent/smartctl_temperature_width_test.go b/internal/hostagent/smartctl_temperature_width_test.go new file mode 100644 index 000000000..b2a6ac3ee --- /dev/null +++ b/internal/hostagent/smartctl_temperature_width_test.go @@ -0,0 +1,43 @@ +package hostagent + +import "testing" + +// SMART raw attribute values are 64-bit, but DiskSMART.Temperature is an int, +// which is 32 bits wide on the 386 and arm release builds. Range-checking after +// the narrowing conversion let a raw value whose low 32 bits landed in the +// plausible band pass as a real temperature. validSMARTTemperature64 is the +// gate that must run first, so it is pinned here rather than through the parse +// path: on a 64-bit test host the conversion is lossless and the end-to-end +// case cannot distinguish the fixed code from the broken code. +func TestValidSMARTTemperature64RejectsValuesThatTruncateIntoRange(t *testing.T) { + cases := []struct { + name string + value int64 + want bool + }{ + {name: "plausible reading", value: 38, want: true}, + {name: "lower bound excluded", value: 0, want: false}, + {name: "upper bound excluded", value: 150, want: false}, + {name: "negative", value: -5, want: false}, + + // Each of these has low 32 bits equal to a plausible temperature. + {name: "2^32 plus 20", value: 1<<32 + 20, want: false}, + {name: "2^32 plus 45", value: 1<<32 + 45, want: false}, + {name: "2^40 plus 38", value: 1<<40 + 38, want: false}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := validSMARTTemperature64(tc.value); got != tc.want { + t.Fatalf("validSMARTTemperature64(%d) = %v, want %v", tc.value, got, tc.want) + } + }) + } + + // The int wrapper must stay consistent with the 64-bit gate. + for _, value := range []int{0, 1, 38, 149, 150, -5} { + if validSMARTTemperature(value) != validSMARTTemperature64(int64(value)) { + t.Fatalf("validSMARTTemperature(%d) disagrees with the 64-bit gate", value) + } + } +} diff --git a/internal/truenas/provider.go b/internal/truenas/provider.go index c763b63f1..9d04ea2d1 100644 --- a/internal/truenas/provider.go +++ b/internal/truenas/provider.go @@ -1643,17 +1643,34 @@ func zfsPoolFromPool(pool Pool) models.ZFSPool { } } +// devicePath renders a vdev's device node as an absolute path. The input comes +// from the appliance's own API and is published verbatim on ZFSDevice.Path, so +// it is normalised here rather than trusted: a traversal segment or a backslash +// has no place in a ZFS device node, and a leading "//" would let the value +// present as a protocol-relative reference to whatever consumes it. func devicePath(device string) string { device = strings.TrimSpace(device) if device == "" { return "" } + if strings.Contains(device, `\`) || hasTraversalSegment(device) { + return "" + } if strings.HasPrefix(device, "/") { - return device + return "/" + strings.TrimLeft(device, "/") } return "/dev/" + device } +func hasTraversalSegment(value string) bool { + for _, segment := range strings.Split(value, "/") { + if segment == ".." { + return true + } + } + return false +} + func poolScanSummary(pool Pool) string { if pool.Scan == nil { return "" diff --git a/internal/truenas/provider_device_path_test.go b/internal/truenas/provider_device_path_test.go new file mode 100644 index 000000000..7bccacdd7 --- /dev/null +++ b/internal/truenas/provider_device_path_test.go @@ -0,0 +1,36 @@ +package truenas + +import "testing" + +// vdev.Device arrives from the appliance API and is published verbatim on +// ZFSDevice.Path, so devicePath normalises it instead of trusting it. +func TestDevicePathNormalisesUntrustedApplianceValues(t *testing.T) { + cases := []struct { + name string + device string + want string + }{ + {name: "bare device node", device: "sda", want: "/dev/sda"}, + {name: "partition", device: "nvme0n1p2", want: "/dev/nvme0n1p2"}, + {name: "absolute by-id path preserved", device: "/dev/disk/by-id/ata-SAMSUNG", want: "/dev/disk/by-id/ata-SAMSUNG"}, + {name: "freebsd gptid", device: "gptid/abcd-1234", want: "/dev/gptid/abcd-1234"}, + {name: "whitespace trimmed", device: " sdb ", want: "/dev/sdb"}, + {name: "empty stays empty", device: "", want: ""}, + + {name: "leading double slash collapsed", device: "//evil.example.com/share", want: "/evil.example.com/share"}, + {name: "leading triple slash collapsed", device: "///evil", want: "/evil"}, + {name: "backslash rejected", device: `/\evil.example.com`, want: ""}, + {name: "windows style path rejected", device: `C:\Windows`, want: ""}, + {name: "relative traversal rejected", device: "../../etc/passwd", want: ""}, + {name: "absolute traversal rejected", device: "/dev/../etc/passwd", want: ""}, + {name: "embedded traversal rejected", device: "disk/../../root", want: ""}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := devicePath(tc.device); got != tc.want { + t.Fatalf("devicePath(%q) = %q, want %q", tc.device, got, tc.want) + } + }) + } +}