pillar: fix decimal literals used as file modes

[christoph-zededa]

Description

Several file-mode arguments in pillar are written as decimal literals, so the
permission bits they produce are not the ones they read as:

Literal	Go value	Resulting permission bits
755	0o1363	0o363 — -wxrw--wx
644	0o1204	0o204 — -w----r--

Measured, not inferred:

os.MkdirAll(dir, 755)   -> d-wxrw--wx (0363)
os.MkdirAll(dir, 0o755) -> drwxr-xr-x (0755)

So the vault directories were created world-writable, and the attestation
integrity token file (/run/eve.integrity_token) was written world-readable
while not being readable by its owner. Affected call sites:

• pkg/pillar/vault/key.go — key staging directory
• pkg/pillar/vault/handler_ext4.go (x2) — default vault and vault path
• pkg/pillar/vault/handler_unsupported.go — default vault
• pkg/pillar/cmd/zedagent/attesttask.go — integrity token file

Each site is switched to an octal literal keeping the permissions it already
intended, so there is no change of intent anywhere — only the notation bug.

One extra fix in vault/key.go: correcting the MkdirAll mode there is not
sufficient on its own, because stageKey() mounts a tmpfs onto that directory
immediately afterwards and a tmpfs root defaults to 01777:

before mount:            700  drwx------
after mount (no opts):  1777  drwxrwxrwt     <-- masks whatever mkdir set
after mount (mode=0755): 755  drwxr-xr-x

The staging directory was therefore world-writable for the whole time the
unsealed vault key sits in it. mode=0755 is now passed to the mount as well.

How to test and validate this PR

The mistake itself is easy to check for statically — this reports nothing after
this PR, and the five call sites above before it:

grep -rnE '\.(MkdirAll|Mkdir|WriteFile|OpenFile|Chmod)\(([^()]|\([^()]*\))*,\s*[1-9][0-9]*\s*\)' \
    --include='*.go' . | grep -v /vendor/

Also run:

make -C pkg/pillar fmt-check
make -C pkg/pillar vet
make -C pkg/pillar test

On a device, the permission change can be confirmed directly. Since MkdirAll
only applies its mode when creating the directory, the vault checks need a
device whose vault does not exist yet (fresh install, or an installation
without TPM where the vault is a plain folder):

1. Boot a device with this build and confirm the vault directory is
   drwxr-xr-x rather than d-wxrw--wx:
   stat -c '%a %A %n' /persist/vault
2. While a vault unlock is in progress, confirm the key staging directory is no
   longer world-writable. /TmpVaultDir2 (ext4) or /run/TmpVaultDir2 (zfs)
   should be drwxr-xr-x, not drwxrwxrwt:
   stat -c '%a %A %n' /TmpVaultDir2
   Before this PR the same command reports 1777 drwxrwxrwt.
3. Confirm the vault still unlocks normally and app instances with encrypted
   volumes start — all the affected directories are used by root-owned
   processes only, so no functional change is expected.
4. After a successful attestation, confirm the integrity token file is
   -rw-r--r-- rather than --w----r--:
   stat -c '%a %A %n' /run/eve.integrity_token

Suggested QA regression focus: vault creation and unlock on a fresh install,
both with and without TPM, on ext4 and zfs; plus one attestation cycle.

Changelog notes

Fixed file permissions on the vault directories, the vault key staging
directory and the attestation integrity token file, which were created with
different permission bits than intended.

PR Backports

• 16.0-stable: To be backported.
• 14.5-stable: To be backported.
• 13.4-stable: To be backported.

All five call sites are present unchanged on all three LTS branches, so the fix
applies to each.

Checklist

• I've provided a proper description
• I've added the proper documentation
• I've tested my PR on amd64 device
• I've tested my PR on arm64 device
• I've written the test verification instructions
• I've set the proper labels to this PR
• I've checked the boxes above, or I've provided a good reason why I didn't
  check them

Reasons for the unchecked boxes:

• Documentation: no user-facing or architectural behaviour changes, so there is
  nothing to document. The non-obvious part (a tmpfs root defaulting to
  01777) is explained in the commit message.
• Device testing on amd64/arm64: not yet done, hence the draft status. The
  changes are architecture-independent, but the vault paths are device
  management code, so the on-device steps above still need to be run before
  this leaves draft.
• Labels: done — stable (backport to all three LTS branches) and security.

[rene]

Good catch, but just adding a leading 0 make it work (it's the octal notation for Go). TBH I really don't see 0o being used very often and I don't see any advantages other than make strange to the eyes.... 😆

So 0644, 0755 it works as well...

[christoph-zededa]

Sure, fixed :-)

[christoph-zededa]

/rerun red

[codecov]

Codecov Report

❌ Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 24.48%. Comparing base (cc3374a) to head (bbea20f).
⚠️ Report is 33 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/pillar/vault/handler_ext4.go	0.00%	2 Missing ⚠️
pkg/pillar/cmd/zedagent/attesttask.go	0.00%	1 Missing ⚠️
pkg/pillar/vault/handler_unsupported.go	0.00%	1 Missing ⚠️
pkg/pillar/vault/handler_zfs.go	0.00%	1 Missing ⚠️
pkg/pillar/vault/key.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6264      +/-   ##
==========================================
+ Coverage   24.13%   24.48%   +0.34%     
==========================================
  Files         512      522      +10     
  Lines       93537    95253    +1716     
==========================================
+ Hits        22575    23322     +747     
- Misses      69177    69961     +784     
- Partials     1785     1970     +185     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[eriknordmark]

This seems error prone to begin with and the leading zero doesn't make it less so. Should we make all of them use syscall.S_IRUSR etc to not have any numeric values?

[christoph-zededa]

I would prefer the numeric values (to write and to read).
I added a commit to add a semgrep rule and to actually run semgrep on the rules we already have: bbea20f

[christoph-zededa]

/rerun red