From 120493611d0070dcd8d1b4b36570b4b696f5a2d8 Mon Sep 17 00:00:00 2001 From: Christoph Ostarek Date: Mon, 3 Aug 2026 14:23:02 +0200 Subject: [PATCH 1/2] pillar: fix decimal literals used as file modes os.MkdirAll(dir, 755) and os.WriteFile(file, data, 644) pass decimal, not octal, values. Go reads 755 as 0o1363, leaving permission bits 0o363 (-wxrw--wx), and 644 as 0o1204, leaving 0o204 (-w----r--). The vault directories were therefore created world-writable, and the attestation integrity token was written world-readable while not being readable by its owner. Use octal literals, keeping the permissions each site already intended. In vault/key.go that is not sufficient on its own: stageKey() mounts a tmpfs onto the key staging directory right after creating it, and a tmpfs root defaults to 01777, which masks the mode underneath for as long as the unsealed vault key is staged there. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Christoph Ostarek --- pkg/pillar/cmd/zedagent/attesttask.go | 2 +- pkg/pillar/vault/handler_ext4.go | 4 ++-- pkg/pillar/vault/handler_unsupported.go | 2 +- pkg/pillar/vault/handler_zfs.go | 2 +- pkg/pillar/vault/key.go | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/pillar/cmd/zedagent/attesttask.go b/pkg/pillar/cmd/zedagent/attesttask.go index 7e5efa966cb..2e331b9ceb7 100644 --- a/pkg/pillar/cmd/zedagent/attesttask.go +++ b/pkg/pillar/cmd/zedagent/attesttask.go @@ -876,7 +876,7 @@ func storeIntegrityToken(token []byte) { if len(token) == 0 { log.Warnf("[ATTEST] Received empty integrity token") } - err := os.WriteFile(types.ITokenFile, token, 644) + err := os.WriteFile(types.ITokenFile, token, 0644) if err != nil { log.Fatalf("Failed to store integrity token, err: %v", err) } diff --git a/pkg/pillar/vault/handler_ext4.go b/pkg/pillar/vault/handler_ext4.go index 5e58ee719e8..c183777bfc1 100644 --- a/pkg/pillar/vault/handler_ext4.go +++ b/pkg/pillar/vault/handler_ext4.go @@ -101,7 +101,7 @@ func (h *Ext4Handler) SetupDefaultVault() error { if os.IsNotExist(err) { // No TPM or TPM lacks required features // Vault is just a plain folder in those cases - return os.MkdirAll(defaultVault, 755) + return os.MkdirAll(defaultVault, 0755) } if err == nil && h.isFscryptEnabled(defaultVault) { // old versions of EVE created vault on TPM platforms @@ -352,7 +352,7 @@ func (h *Ext4Handler) setupVault(vaultPath string, deprecated bool) error { } if err != nil && !deprecated { // Create vault dir - if err := os.MkdirAll(vaultPath, 755); err != nil { + if err := os.MkdirAll(vaultPath, 0755); err != nil { return err } } diff --git a/pkg/pillar/vault/handler_unsupported.go b/pkg/pillar/vault/handler_unsupported.go index e6c1d30f7cb..2e5c9d27447 100644 --- a/pkg/pillar/vault/handler_unsupported.go +++ b/pkg/pillar/vault/handler_unsupported.go @@ -51,7 +51,7 @@ func (h *UnsupportedHandler) SetupDefaultVault() error { if os.IsNotExist(err) { // No TPM or TPM lacks required features // Vault is just a plain folder in those cases - return os.MkdirAll(defaultVault, 755) + return os.MkdirAll(defaultVault, 0755) } return nil } diff --git a/pkg/pillar/vault/handler_zfs.go b/pkg/pillar/vault/handler_zfs.go index 6345b164573..88076e6c08b 100644 --- a/pkg/pillar/vault/handler_zfs.go +++ b/pkg/pillar/vault/handler_zfs.go @@ -389,7 +389,7 @@ func MountVaultZvol(log *base.LogObject, datasetPath string) error { _, err = os.Stat("/" + types.SealedDataset) if err != nil { if os.IsNotExist(err) { - err = os.Mkdir("/"+types.SealedDataset, os.FileMode(755)) + err = os.Mkdir("/"+types.SealedDataset, 0755) if err != nil { return fmt.Errorf("MountVaultZvol path %s creation error: %v", "/"+types.SealedDataset, err) } diff --git a/pkg/pillar/vault/key.go b/pkg/pillar/vault/key.go index 2737a3559bf..003ae96f7c6 100644 --- a/pkg/pillar/vault/key.go +++ b/pkg/pillar/vault/key.go @@ -64,7 +64,7 @@ func deriveVaultKey(log *base.LogObject, cloudKeyOnlyMode, useSealedKey, tpmKeyO // returns function to unstage the key func stageKey(log *base.LogObject, cloudKeyOnlyMode, useSealedKey, tpmKeyOnlyMode bool, keyDirName string, keyFileName string) (func(), error) { // Create a tmpfs file to pass the secret to fscrypt - if err := os.MkdirAll(keyDirName, 755); err != nil { + if err := os.MkdirAll(keyDirName, 0700); err != nil { return nil, fmt.Errorf("error creating keyDir %s %v", keyDirName, err) } From bbea20f1222eec413aac78d34edef7bdec09efa6 Mon Sep 17 00:00:00 2001 From: Christoph Ostarek Date: Wed, 5 Aug 2026 13:13:00 +0200 Subject: [PATCH 2/2] semgrep: run the rules from make and in CI tests/semgrep-rules/ has held rules since May 2025, but nothing ever invoked them: no make target, no workflow, no yetus plugin. They ran only if someone installed semgrep and pointed it at the directory by hand. Add make semgrep, plus a workflow that runs it on pull requests. Only ERROR rules gate. The two big.Int rules are WARNING and match any Bytes() call, so they stay available through make semgrep-all but never fail a build. CI passes --baseline-commit so a pull request is judged on the findings it introduces rather than on pre-existing ones. Add non-octal-file-mode, which flags file modes written as decimal literals: 755 is 0o1363, whose permission bits are 0o363 (-wxrw--wx). Narrow os-openfile-non-perm-mode, which rejected any mode that was not an octal literal and so flagged legitimate expressions such as the os.FileMode(header.Mode) in evetest/utils/tar.go. It now reports only decimal literals and non-permission os.Mode* bits, and still catches the os.ModeAppend it was written for. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Christoph Ostarek --- .github/workflows/semgrep.yml | 33 +++++++++++++++++++ Makefile | 26 +++++++++++++++ tests/semgrep-rules/non-octal-file-mode.yaml | 31 +++++++++++++++++ .../os-openfile-non-perm-mode.yaml | 18 ++++++++-- 4 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/semgrep.yml create mode 100644 tests/semgrep-rules/non-octal-file-mode.yaml diff --git a/.github/workflows/semgrep.yml b/.github/workflows/semgrep.yml new file mode 100644 index 00000000000..a529308133f --- /dev/null +++ b/.github/workflows/semgrep.yml @@ -0,0 +1,33 @@ +# Copyright (c) 2026, Zededa, Inc. +# SPDX-License-Identifier: Apache-2.0 +--- +name: Semgrep + +on: # yamllint disable-line rule:truthy + pull_request: + branches: + - "master" + - "[0-9]+.[0-9]+" + - "[0-9]+.[0-9]+-stable" + - "feature/*" + +permissions: + contents: read + +jobs: + semgrep: + name: Semgrep static analysis + runs-on: ubuntu-latest + steps: + # Full history: the baseline scan below needs the PR's base commit. + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + fetch-depth: 0 + persist-credentials: false + + # Baseline against the PR's base so only findings this PR introduces fail. + - name: Scan for newly introduced findings + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + run: make semgrep SEMGREP_BASELINE="$BASE_SHA" diff --git a/Makefile b/Makefile index 9bfadb01322..c4a6378ccc8 100644 --- a/Makefile +++ b/Makefile @@ -583,6 +583,27 @@ check-pubsub-persistence: @echo "Checking pubsub publications and subscriptions for persistence mismatches" cd pkg/pillar && go run ./tools/pubsubcheck ../../pkg +# Semgrep catches patterns the Go toolchain accepts but that do not mean what +# they read as, e.g. a file mode written as a decimal literal (tests/semgrep-rules). +# Only ERROR rules gate: the WARNING rules are deliberately broad heuristics with +# known false positives, so they are reported by semgrep-all but never fail. +# SEMGREP_BASELINE= reports only findings introduced since that commit, +# which is how CI gates new code without first cleaning up existing findings. +SEMGREP_IMAGE ?= semgrep/semgrep:1.171.0 +SEMGREP_BASELINE ?= +SEMGREP_RUN = docker run --rm -v $(CURDIR):/src:ro -w /src $(SEMGREP_IMAGE) semgrep --metrics=off + +.PHONY: semgrep semgrep-all +semgrep: + @echo Running semgrep + $(SEMGREP_RUN) --error --severity=ERROR \ + $(if $(SEMGREP_BASELINE),--baseline-commit=$(SEMGREP_BASELINE)) \ + --config tests/semgrep-rules/ --exclude=vendor . + +semgrep-all: + @echo Running semgrep, including the WARNING heuristics + $(SEMGREP_RUN) --config tests/semgrep-rules/ --exclude=vendor . + yetus: @echo Running yetus mkdir -p yetus-output @@ -1416,6 +1437,11 @@ help: @echo " Y, the output will be echoed to the console" @echo " check-docker-hashes-consistency check for Dockerfile image inconsistencies" @echo " check-pubsub-persistence check pubsub publications/subscriptions for persistence mismatches" + @echo " semgrep run the blocking (ERROR) semgrep rules over the tree;" + @echo " set SEMGREP_BASELINE= to report only findings" + @echo " introduced since that commit" + @echo " semgrep-all run every semgrep rule, including the WARNING" + @echo " heuristics, which have known false positives" @echo " kernel-tag show current KERNEL_TAG" @echo @echo "Eden testing targets:" diff --git a/tests/semgrep-rules/non-octal-file-mode.yaml b/tests/semgrep-rules/non-octal-file-mode.yaml new file mode 100644 index 00000000000..849a6467898 --- /dev/null +++ b/tests/semgrep-rules/non-octal-file-mode.yaml @@ -0,0 +1,31 @@ +# Copyright (c) 2026 Zededa, Inc. +# SPDX-License-Identifier: Apache-2.0 + +--- +rules: + - id: non-octal-file-mode + message: "File mode ($MODE) is a decimal literal, so it is not the permission + bits it looks like (e.g. 755 is 0o1363, i.e. -wxrw--wx). Use an octal + literal such as 0o755 or 0755." + severity: ERROR + languages: + - go + options: + constant_propagation: true + patterns: + # os.OpenFile is covered by os-openfile-non-perm-mode.yaml instead, which + # additionally checks that O_CREATE makes the mode argument meaningful. + - pattern-either: + - pattern: os.Mkdir($PATH, $MODE) + - pattern: os.MkdirAll($PATH, $MODE) + - pattern: os.WriteFile($PATH, $DATA, $MODE) + - pattern: os.Chmod($PATH, $MODE) + - pattern: $FILE.Chmod($MODE) + - pattern: os.FileMode($MODE) + - pattern: fs.FileMode($MODE) + # Only decimal literals are reported. Octal literals start with a + # leading zero, and named constants + - metavariable-pattern: + metavariable: $MODE + patterns: + - pattern-regex: '^[1-9][0-9]*$' diff --git a/tests/semgrep-rules/os-openfile-non-perm-mode.yaml b/tests/semgrep-rules/os-openfile-non-perm-mode.yaml index 9ccfdf90524..7b58f5e0f67 100644 --- a/tests/semgrep-rules/os-openfile-non-perm-mode.yaml +++ b/tests/semgrep-rules/os-openfile-non-perm-mode.yaml @@ -4,7 +4,8 @@ --- rules: - id: os-openfile-non-perm-mode - message: "os.OpenFile called with os.O_CREATE but invalid permission mode ($MODE). Use an octal permission like 0o644 or 0644." + message: "os.OpenFile called with os.O_CREATE but $MODE is not a permission + mode. Use an octal permission like 0o644 or 0644." severity: ERROR languages: - go @@ -16,7 +17,20 @@ rules: metavariable: $FLAGS patterns: - pattern-regex: '.*O_CREATE.*' + # Report only the two ways the mode is provably wrong. Everything else -- + # variables, os.FileMode() conversions, composite expressions -- can be a + # legitimate mode and is left to review. - metavariable-pattern: metavariable: $MODE patterns: - - pattern-not-regex: '^0[oO]?[0-7]{3,4}$' + - pattern-either: + # A decimal literal is not the permission bits it reads as: + # 644 is 0o1204, whose permission bits are 0o204 (-w----r--). + # A decimal wrapped in a FileMode conversion is caught by + # non-octal-file-mode.yaml, which matches the conversion itself. + - pattern-regex: '^[1-9][0-9]*$' + # File type/attribute bits are not permission bits. ModePerm is + # the exception: it is exactly 0o777. + - patterns: + - pattern-regex: '^os\.Mode[A-Za-z]+$' + - pattern-not-regex: '^os\.ModePerm$'