From 92524e1c279b3dc236f1886a05abc7940a837e16 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 9 Jul 2026 17:37:08 +0100 Subject: [PATCH] Harden CodeQL storage and integer boundaries --- .../pulse-security-models/codeql-pack.yml | 7 +++ .../models/go-storage.yml | 14 +++++ internal/config/billing_state.go | 40 +++++++++++--- internal/config/billing_state_test.go | 54 +++++++++++++++++++ internal/hostagent/smartctl.go | 4 +- internal/hostagent/smartctl_test.go | 27 ++++++++++ internal/truenas/client.go | 8 +-- internal/truenas/client_helpers_test.go | 40 ++++++++++++++ pkg/proxmox/client.go | 22 +++----- 9 files changed, 190 insertions(+), 26 deletions(-) create mode 100644 .github/codeql/extensions/pulse-security-models/codeql-pack.yml create mode 100644 .github/codeql/extensions/pulse-security-models/models/go-storage.yml diff --git a/.github/codeql/extensions/pulse-security-models/codeql-pack.yml b/.github/codeql/extensions/pulse-security-models/codeql-pack.yml new file mode 100644 index 000000000..b7328061e --- /dev/null +++ b/.github/codeql/extensions/pulse-security-models/codeql-pack.yml @@ -0,0 +1,7 @@ +name: rcourtman/pulse-security-models +version: 0.0.1 +library: true +extensionTargets: + codeql/go-all: '*' +dataExtensions: + - models/**/*.yml diff --git a/.github/codeql/extensions/pulse-security-models/models/go-storage.yml b/.github/codeql/extensions/pulse-security-models/models/go-storage.yml new file mode 100644 index 000000000..2ab736171 --- /dev/null +++ b/.github/codeql/extensions/pulse-security-models/models/go-storage.yml @@ -0,0 +1,14 @@ +extensions: + - addsTo: + pack: codeql/go-all + extensible: barrierModel + data: + - ["github.com/rcourtman/pulse-go-rewrite/internal/securityutil", "", false, "HashedStorageName", "", "", "ReturnValue", "path-injection", "manual"] + - ["github.com/rcourtman/pulse-go-rewrite/internal/securityutil", "", false, "JoinStorageLeaf", "", "", "ReturnValue[0]", "path-injection", "manual"] + - ["github.com/rcourtman/pulse-go-rewrite/internal/securityutil", "", false, "NormalizeStorageDir", "", "", "ReturnValue[0]", "path-injection", "manual"] + - ["github.com/rcourtman/pulse-go-rewrite/internal/securityutil", "", false, "ValidateOutboundFetchURL", "", "", "ReturnValue[0]", "request-forgery", "manual"] + - addsTo: + pack: codeql/go-all + extensible: barrierGuardModel + data: + - ["github.com/rcourtman/pulse-go-rewrite/internal/config", "", false, "isValidOrgID", "", "", "Argument[0]", "true", "path-injection", "manual"] diff --git a/internal/config/billing_state.go b/internal/config/billing_state.go index 6c5381f8b..5dbe2c132 100644 --- a/internal/config/billing_state.go +++ b/internal/config/billing_state.go @@ -16,6 +16,7 @@ import ( "time" "github.com/rcourtman/pulse-go-rewrite/internal/crypto" + "github.com/rcourtman/pulse-go-rewrite/internal/securityutil" pkglicensing "github.com/rcourtman/pulse-go-rewrite/pkg/licensing" "github.com/rs/zerolog/log" ) @@ -233,7 +234,11 @@ func (s *FileBillingStore) SaveBillingState(orgID string, state *pkglicensing.Bi } func (s *FileBillingStore) billingCryptoManager() (*crypto.CryptoManager, error) { - return crypto.NewCryptoManagerAt(s.resolveDataDir()) + dataDir, err := s.billingBaseDataDir() + if err != nil { + return nil, err + } + return crypto.NewCryptoManagerAt(dataDir) } func (s *FileBillingStore) billingCryptoManagerForSecrets(values ...string) (*crypto.CryptoManager, error) { @@ -250,22 +255,45 @@ func (s *FileBillingStore) billingStatePath(orgID string) (string, error) { if !isValidOrgID(orgID) { return "", fmt.Errorf("invalid organization ID: %s", orgID) } + baseDir, err := s.billingBaseDataDir() + if err != nil { + return "", err + } // Default org stores config at the root data dir for backward compatibility, // so billing state for the default org must live alongside other root configs. if orgID == "default" { - return filepath.Join(s.resolveDataDir(), "billing.json"), nil + return securityutil.JoinStorageLeaf(baseDir, "billing.json") } - return filepath.Join(s.resolveDataDir(), "orgs", orgID, "billing.json"), nil + orgsDir, err := securityutil.JoinStorageLeaf(baseDir, "orgs") + if err != nil { + return "", fmt.Errorf("resolve orgs directory: %w", err) + } + orgDir, err := securityutil.JoinStorageLeaf(orgsDir, orgID) + if err != nil { + return "", fmt.Errorf("resolve organization billing directory: %w", err) + } + return securityutil.JoinStorageLeaf(orgDir, "billing.json") } -func (s *FileBillingStore) resolveDataDir() string { - return ResolveRuntimeDataDir(s.baseDataDir) +func (s *FileBillingStore) billingBaseDataDir() (string, error) { + dataDir, err := securityutil.NormalizeStorageDir(ResolveRuntimeDataDir(s.baseDataDir)) + if err != nil { + return "", fmt.Errorf("resolve billing data directory: %w", err) + } + return dataDir, nil } // loadHMACKey derives a purpose-specific HMAC key from the .encryption.key file. // Returns an error if the key file is missing or invalid (graceful degradation). func (s *FileBillingStore) loadHMACKey() ([]byte, error) { - keyPath := filepath.Join(s.resolveDataDir(), ".encryption.key") + dataDir, err := s.billingBaseDataDir() + if err != nil { + return nil, err + } + keyPath, err := securityutil.JoinStorageLeaf(dataDir, ".encryption.key") + if err != nil { + return nil, err + } raw, err := os.ReadFile(keyPath) if err != nil { return nil, err diff --git a/internal/config/billing_state_test.go b/internal/config/billing_state_test.go index 090b2cbd9..ca100a4ed 100644 --- a/internal/config/billing_state_test.go +++ b/internal/config/billing_state_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "time" @@ -88,6 +89,59 @@ func TestBillingState_RoundTrip(t *testing.T) { assert.Equal(t, endsAt, *loaded.TrialEndsAt) } +func TestBillingState_InvalidOrgIDsRejected(t *testing.T) { + dir := t.TempDir() + store := NewFileBillingStore(dir) + + invalidIDs := []string{ + "", + ".", + "..", + "../bad", + "bad/..", + "bad/../evil", + "bad org", + "bad\torg", + "bad\norg", + "bad\\org", + "bad:org", + strings.Repeat("a", 65), + } + + for _, orgID := range invalidIDs { + err := store.SaveBillingState(orgID, &entitlements.BillingState{}) + require.Error(t, err, "SaveBillingState should reject orgID %q", orgID) + + _, err = store.GetBillingState(orgID) + require.Error(t, err, "GetBillingState should reject orgID %q", orgID) + } + + if _, err := os.Stat(filepath.Join(dir, "orgs")); !os.IsNotExist(err) { + t.Fatalf("unexpected orgs directory state after invalid org IDs: %v", err) + } +} + +func TestBillingState_NonDefaultOrgUsesCanonicalStoragePath(t *testing.T) { + root := t.TempDir() + rawBaseDir := filepath.Join(root, "billing") + string(os.PathSeparator) + ".." + string(os.PathSeparator) + "billing" + expectedBaseDir := filepath.Clean(rawBaseDir) + store := NewFileBillingStore(" " + rawBaseDir + " ") + + state := &entitlements.BillingState{ + Capabilities: []string{"relay"}, + SubscriptionState: entitlements.SubStateTrial, + } + require.NoError(t, store.SaveBillingState("acme.prod-1", state)) + + billingPath := filepath.Join(expectedBaseDir, "orgs", "acme.prod-1", "billing.json") + require.FileExists(t, billingPath) + + loaded, err := store.GetBillingState("acme.prod-1") + require.NoError(t, err) + require.NotNil(t, loaded) + assert.Contains(t, loaded.Capabilities, "relay") +} + func TestBillingState_EncryptsHostedEntitlementSecretsAtRest(t *testing.T) { dir := t.TempDir() t.Setenv("PULSE_LEGACY_KEY_PATH", filepath.Join(t.TempDir(), ".encryption.key")) diff --git a/internal/hostagent/smartctl.go b/internal/hostagent/smartctl.go index 4b2da7330..dcb627688 100644 --- a/internal/hostagent/smartctl.go +++ b/internal/hostagent/smartctl.go @@ -1337,9 +1337,9 @@ func parseSMARTOutput(output []byte, target smartctlTarget) (*DiskSMART, error) } else { for _, attr := range smartData.ATASmartAttributes.Table { if attr.ID == 194 || attr.ID == 190 { - temp := int(parseRawValue(attr.Raw.String, attr.Raw.Value)) + temp := parseRawValue(attr.Raw.String, attr.Raw.Value) if temp > 0 && temp < 150 { - result.Temperature = temp + result.Temperature = int(temp) break } } diff --git a/internal/hostagent/smartctl_test.go b/internal/hostagent/smartctl_test.go index a1b19cd11..9f7bbc234 100644 --- a/internal/hostagent/smartctl_test.go +++ b/internal/hostagent/smartctl_test.go @@ -478,6 +478,33 @@ func TestParseSMARTOutputFallsBackToOriginalTextTemperature(t *testing.T) { } } +func TestParseSMARTOutputRejectsOutOfRangeATARawTemperature(t *testing.T) { + payload := []byte(`{ + "device": {"protocol": "ATA"}, + "model_name": "WDC WD40EFRX", + "serial_number": "WD-999", + "smart_status": {"passed": true}, + "ata_smart_attributes": { + "table": [{ + "id": 194, + "name": "Temperature_Celsius", + "raw": {"value": 9223372036854775807, "string": "9223372036854775807"} + }] + } + }`) + + result, err := parseSMARTOutput(payload, smartctlTarget{Path: "/dev/ada0"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if result == nil { + t.Fatal("expected result") + } + if result.Temperature != 0 { + t.Fatalf("expected out-of-range ATA raw temperature to be ignored, got %#v", result) + } +} + func TestParseSMARTOutputFallsBackToPlainTextOutput(t *testing.T) { output := []byte(` === START OF INFORMATION SECTION === diff --git a/internal/truenas/client.go b/internal/truenas/client.go index d37a2d8d4..a0e2f3c1c 100644 --- a/internal/truenas/client.go +++ b/internal/truenas/client.go @@ -1502,7 +1502,7 @@ func appendDiskTemperature(out map[string]int, diskName string, value any) { } } temperature, ok := parseInt64Any(value) - if !ok || temperature <= 0 { + if !ok || temperature <= 0 || temperature >= 150 { return } out[diskName] = int(temperature) @@ -2887,7 +2887,7 @@ func readIntAny(record map[string]any, keys ...string) int { if !ok || value == nil { continue } - if parsed, ok := parseInt64Any(value); ok { + if parsed, ok := parseInt64Any(value); ok && parsed >= math.MinInt32 && parsed <= math.MaxInt32 { return int(parsed) } } @@ -2907,13 +2907,13 @@ func readIntSliceAny(record map[string]any, keys ...string) []int { case []any: out := make([]int, 0, len(typed)) for _, item := range typed { - if parsed, ok := parseInt64Any(item); ok { + if parsed, ok := parseInt64Any(item); ok && parsed >= math.MinInt32 && parsed <= math.MaxInt32 { out = append(out, int(parsed)) } } return out default: - if parsed, ok := parseInt64Any(value); ok { + if parsed, ok := parseInt64Any(value); ok && parsed >= math.MinInt32 && parsed <= math.MaxInt32 { return []int{int(parsed)} } } diff --git a/internal/truenas/client_helpers_test.go b/internal/truenas/client_helpers_test.go index a1b00ad73..004a2712d 100644 --- a/internal/truenas/client_helpers_test.go +++ b/internal/truenas/client_helpers_test.go @@ -173,6 +173,46 @@ func TestParseInt64FromAnyCoversBranches(t *testing.T) { } } +func TestReadIntHelpersSkipOutOfRangeValues(t *testing.T) { + record := map[string]any{ + "overflow": "2147483648", + "valid": "42", + "values": []any{ + "1", + "2147483648", + "-2147483649", + "2", + }, + } + + if got := readIntAny(record, "overflow", "valid"); got != 42 { + t.Fatalf("readIntAny() = %d, want fallback value 42", got) + } + + got := readIntSliceAny(record, "values") + if len(got) != 2 || got[0] != 1 || got[1] != 2 { + t.Fatalf("readIntSliceAny() = %#v, want []int{1, 2}", got) + } +} + +func TestAppendDiskTemperatureRejectsOutOfRangeValues(t *testing.T) { + temperatures := map[string]int{} + + appendDiskTemperature(temperatures, "sda", int64(149)) + appendDiskTemperature(temperatures, "sdb", int64(150)) + appendDiskTemperature(temperatures, "sdc", "2147483648") + + if got := temperatures["sda"]; got != 149 { + t.Fatalf("expected bounded valid temperature 149, got %d", got) + } + if _, ok := temperatures["sdb"]; ok { + t.Fatal("expected temperature 150 to be rejected") + } + if _, ok := temperatures["sdc"]; ok { + t.Fatal("expected oversized temperature to be rejected") + } +} + func TestParseBoolFromAnyCoversBranches(t *testing.T) { tests := []struct { name string diff --git a/pkg/proxmox/client.go b/pkg/proxmox/client.go index 8eba518c9..01ca63748 100644 --- a/pkg/proxmox/client.go +++ b/pkg/proxmox/client.go @@ -80,31 +80,25 @@ func parseFlexibleIntString(raw string) (int, error) { return floatToIntTrunc(parsed) } - parsed, err := strconv.ParseInt(raw, 10, 64) + parsed, err := strconv.Atoi(raw) if err != nil { return 0, err } - return int64ToInt(parsed) + return parsed, nil } func int64ToInt(v int64) (int, error) { - if strconv.IntSize == 32 { - if v > math.MaxInt32 || v < math.MinInt32 { - return 0, fmt.Errorf("integer %d exceeds int range", v) - } - return int(int32(v)), nil + maxInt := int64(int(^uint(0) >> 1)) + minInt := -maxInt - 1 + if v > maxInt || v < minInt { + return 0, fmt.Errorf("integer %d exceeds int range", v) } return int(v), nil } func uint64ToInt(v uint64) (int, error) { - if strconv.IntSize == 32 { - if v > math.MaxInt32 { - return 0, fmt.Errorf("integer %d exceeds int range", v) - } - return int(int32(v)), nil - } - if v > math.MaxInt64 { + maxInt := uint64(^uint(0) >> 1) + if v > maxInt { return 0, fmt.Errorf("integer %d exceeds int range", v) } return int(v), nil