fix: Lstat in Mkdir rejects symlink-to-dir

heiko commented

2026-05-22 00:27:45 +02:00

Owner

Fixes #36.

Summary

os.Stat in the EEXIST fallback follows symlinks: a symlink planted at a cert directory path between os.Mkdir failing and the stat check was silently accepted, redirecting subsequent cert/key writes to the symlink target.
Switch to os.Lstat; symlinks fail IsDir() and the outer EEXIST error is returned instead.

Test plan

go test ./internal/shared/ -run TestMkdir — TestMkdir_SymlinkToDir fails on master, passes here

Fixes #36. ## Summary - `os.Stat` in the EEXIST fallback follows symlinks: a symlink planted at a cert directory path between `os.Mkdir` failing and the stat check was silently accepted, redirecting subsequent cert/key writes to the symlink target. - Switch to `os.Lstat`; symlinks fail `IsDir()` and the outer EEXIST error is returned instead. ## Test plan - [x] `go test ./internal/shared/ -run TestMkdir` — `TestMkdir_SymlinkToDir` fails on master, passes here

heiko added 3 commits

2026-05-22 00:27:46 +02:00

packaging: fix lintian override filenames and per-package coverage

nagonag (Push) / nagonag (push) Successful in 1m35s

Details

e73fea9fce

Rename debian/lintian-override to the Debian convention
debian/<package>.lintian-overrides, one file per binary package.
Add cert-proxy-server.lintian-overrides with the matching package prefix
so the server .deb also suppresses the Apache copyright tag.
Update .gogogo.conf src: references accordingly.

test: add regression for #36 ai:claude-sonnet-4-6 3979f3d354

fix: Lstat in Mkdir rejects symlink-to-dir ai:claude-sonnet-4-6 ffe56ce3b4

Fixes #36.

heiko commented

2026-05-22 00:31:18 +02:00

Author

Owner

Code review

What it does

Fixes a symlink-substitution vulnerability in internal/shared/Mkdir. The EEXIST fallback used os.Stat (symlink-following), so a symlink planted at a cert directory path between the failed os.Mkdir and the stat check would be accepted as a real directory, redirecting all subsequent cert/key writes to the attacker's chosen target. Swapping to os.Lstat closes that path: stat.IsDir() returns false for a symlink and the outer EEXIST error is surfaced instead.

Correctness

Fix is minimal and correct. Lstat on a symlink returns ModeSymlink; IsDir() is false; control falls through to return err which carries the original EEXIST. All four Mkdir tests pass.
Variable shadowing on line 13 is intentional and load-bearing — the inner err from Lstat is local; the outer err (EEXIST) is what gets returned. Do not flatten this.
Residual TOCTOU window is unchanged: there is a narrow race between os.Mkdir and os.Lstat. However, the worst-case outcome post-fix is an error (not a redirect), so it is not a regression. The issue asks only for symlink defence, not full TOCTOU elimination.

Style & conventions

One-line change, no padding or cleanup added — good scope discipline.
No new comments in production code, consistent with project style.

Test coverage

TestMkdir_SymlinkToDir covers exactly the vulnerability: real directory, symlink pointing at it, Mkdir on the symlink path must error.
TDD: test committed red before the fix commit.
Existing three Mkdir tests still pass — no regressions.

Minor observations (not blocking, all pre-existing)

os.IsExist is deprecated since Go 1.16; errors.Is(err, fs.ErrExist) is preferred. Out of scope.
TestMkdir_Idempotent silently discards the os.Mkdir error (_ = os.Mkdir(...)), inconsistent with file style. Out of scope.
The 0777 on os.Mkdir is tracked as #28 — correctly left untouched.

Verdict

Ready to merge.

— 🤖 Generated with Claude Code (claude-sonnet-4-6)

## Code review ### What it does Fixes a symlink-substitution vulnerability in `internal/shared/Mkdir`. The EEXIST fallback used `os.Stat` (symlink-following), so a symlink planted at a cert directory path between the failed `os.Mkdir` and the stat check would be accepted as a real directory, redirecting all subsequent cert/key writes to the attacker's chosen target. Swapping to `os.Lstat` closes that path: `stat.IsDir()` returns false for a symlink and the outer EEXIST error is surfaced instead. ### Correctness - Fix is minimal and correct. `Lstat` on a symlink returns `ModeSymlink`; `IsDir()` is false; control falls through to `return err` which carries the original EEXIST. All four `Mkdir` tests pass. - Variable shadowing on line 13 is intentional and load-bearing — the inner `err` from `Lstat` is local; the outer `err` (EEXIST) is what gets returned. Do not flatten this. - Residual TOCTOU window is unchanged: there is a narrow race between `os.Mkdir` and `os.Lstat`. However, the worst-case outcome post-fix is an error (not a redirect), so it is not a regression. The issue asks only for symlink defence, not full TOCTOU elimination. ### Style & conventions - One-line change, no padding or cleanup added — good scope discipline. - No new comments in production code, consistent with project style. ### Test coverage - `TestMkdir_SymlinkToDir` covers exactly the vulnerability: real directory, symlink pointing at it, `Mkdir` on the symlink path must error. - TDD: test committed red before the fix commit. - Existing three `Mkdir` tests still pass — no regressions. ### Minor observations (not blocking, all pre-existing) - `os.IsExist` is deprecated since Go 1.16; `errors.Is(err, fs.ErrExist)` is preferred. Out of scope. - `TestMkdir_Idempotent` silently discards the `os.Mkdir` error (`_ = os.Mkdir(...)`), inconsistent with file style. Out of scope. - The `0777` on `os.Mkdir` is tracked as #28 — correctly left untouched. ### Verdict Ready to merge. — 🤖 Generated with Claude Code (claude-sonnet-4-6)

heiko commented

2026-05-22 00:44:59 +02:00

Author

Owner

Opus security review

Fix correctness

Correct for the stated vulnerability. Lstat returns metadata for the symlink itself; IsDir() is false on a symlink even when the target is a directory. The fall-through to return err on line 23 returns the outer EEXIST — the variable shadowing is load-bearing, not a bug. The benign idempotent case (real directory pre-exists) still returns nil.

Remaining attack surface

writeFile has the same vulnerability class — more dangerous now that the directory door is closed.

cmd/cert-proxy-client/cert/cert.go:319–344: os.OpenFile without O_NOFOLLOW follows symlinks on Linux. The temp filename is <artifact>-<unix_ts>.pem where the timestamp is time.Now().Unix() — predictable to the second. An attacker with write access to <certbase>/<domain> (same precondition as #36) can pre-plant a symlink at the predicted filename. The O_WRONLY|O_TRUNC write then redirects private key material to an attacker-chosen path. The subsequent file.Chmod(0600) operates on the open fd, giving the attacker a chmod primitive on any path the client can reach.

Fix: add syscall.O_NOFOLLOW to the OpenFile flags. Warrants a separate issue at the same severity as #36.

Mode 0777 on os.Mkdir (line 10)

Directories holding private keys should use 0700 at source. Umask may save you in practice, but the intent is wrong. Already tracked as #28.

Test adequacy

Sufficient — pins the exact regression. Dangling-symlink and symlink-to-file cases are nice-to-haves, not blockers.

Verdict

Merge this fix. File a P1 followup for writeFile/O_NOFOLLOW — same threat model, arguably worse because a predictable second-resolution timestamp gives the attacker a concrete per-file redirect path now that the directory redirect is closed.

— 🤖 Opus review via Claude Code (claude-opus-4-7)

## Opus security review ### Fix correctness Correct for the stated vulnerability. `Lstat` returns metadata for the symlink itself; `IsDir()` is false on a symlink even when the target is a directory. The fall-through to `return err` on line 23 returns the outer EEXIST — the variable shadowing is load-bearing, not a bug. The benign idempotent case (real directory pre-exists) still returns nil. ### Remaining attack surface **`writeFile` has the same vulnerability class — more dangerous now that the directory door is closed.** `cmd/cert-proxy-client/cert/cert.go:319–344`: `os.OpenFile` without `O_NOFOLLOW` follows symlinks on Linux. The temp filename is `<artifact>-<unix_ts>.pem` where the timestamp is `time.Now().Unix()` — predictable to the second. An attacker with write access to `<certbase>/<domain>` (same precondition as #36) can pre-plant a symlink at the predicted filename. The `O_WRONLY|O_TRUNC` write then redirects **private key material** to an attacker-chosen path. The subsequent `file.Chmod(0600)` operates on the open fd, giving the attacker a chmod primitive on any path the client can reach. Fix: add `syscall.O_NOFOLLOW` to the `OpenFile` flags. Warrants a separate issue at the same severity as #36. **Mode `0777` on `os.Mkdir` (line 10)** Directories holding private keys should use `0700` at source. Umask may save you in practice, but the intent is wrong. Already tracked as #28. ### Test adequacy Sufficient — pins the exact regression. Dangling-symlink and symlink-to-file cases are nice-to-haves, not blockers. ### Verdict Merge this fix. File a P1 followup for `writeFile`/`O_NOFOLLOW` — same threat model, arguably worse because a predictable second-resolution timestamp gives the attacker a concrete per-file redirect path now that the directory redirect is closed. — 🤖 Opus review via Claude Code (claude-opus-4-7)

heiko commented

2026-05-22 06:17:13 +02:00

Author

Owner

PASS: go test ./... passed.

heiko commented

2026-05-22 08:30:21 +02:00

Author

Owner

Review findings

Blocking: the branch is stale against current master. Forgejo reports the PR as not mergeable, and the current master..pr-40 diff removes the statically-linked-binary override lines from both debian/cert-proxy-client.lintian-overrides and debian/cert-proxy-server.lintian-overrides. Those lines were added by the current master packaging commit (1146618), so merging this PR as-is would partly undo that packaging fix. Rebase or merge master and keep the lintian override files unchanged unless the static-linking override removal is intentional and explained.

No new security or bug issue opened from this pass. The symlink/temp-file write issue noted in the earlier review is already tracked as #35, and this PR is the fix for #36. The directory mode concern remains #28.

Verification: env GOCACHE=/tmp/cert-proxy-go-build CCACHE_DIR=/tmp/cert-proxy-ccache go test ./internal/shared/ passed.

(co)authored by ai:gpt-5-codex

## Review findings - Blocking: the branch is stale against current `master`. Forgejo reports the PR as not mergeable, and the current `master..pr-40` diff removes the `statically-linked-binary` override lines from both `debian/cert-proxy-client.lintian-overrides` and `debian/cert-proxy-server.lintian-overrides`. Those lines were added by the current `master` packaging commit (`1146618`), so merging this PR as-is would partly undo that packaging fix. Rebase or merge `master` and keep the lintian override files unchanged unless the static-linking override removal is intentional and explained. No new security or bug issue opened from this pass. The symlink/temp-file write issue noted in the earlier review is already tracked as #35, and this PR is the fix for #36. The directory mode concern remains #28. Verification: `env GOCACHE=/tmp/cert-proxy-go-build CCACHE_DIR=/tmp/cert-proxy-ccache go test ./internal/shared/` passed. (co)authored by ai:gpt-5-codex

heiko commented

2026-05-23 06:24:40 +02:00

Author

Owner

PASS: go test ./... passed.