Lock file in world-writable /tmp with unbounded blocking wait #18

New issue

Closed

opened 2026-10-22 17:36:12 -07:00 by eric · 1 comment

eric commented

2026-10-22 17:36:12 -07:00

Owner

Summary

The run lock is /tmp/wancarp.lock, and CARP event runs wait on it with a blocking flock(LOCK_EX) and no timeout. Two problems:

World-writable location. Any local user can create /tmp/wancarp.lock and hold an flock on it. Every CARP event run then blocks forever, so failover never changes WAN state, and every --reconcile quietly backs off (exit(0), nothing logged). Opening with 'ce' doesn't truncate, so a symlink can't clobber anything; the problem is denial of service only. Low risk on a firewall with no untrusted shell users, but cheap to fix.
No timeout. Even without a hostile user, one hung run (e.g. interface_configure() stuck) blocks every later event with nothing in the log. #11 (inherited lock FD held by dhclient) was this failure mode: events blocked indefinitely and reconcile went silent. A bounded wait would have made it visible.

Where

const LOCKFILE = '/tmp/wancarp.lock'; (around line 171)

lock block (around lines 656–667):

$lockfh = fopen(LOCKFILE, 'ce');
$locked = $lockfh !== false && flock($lockfh, $reconcile ? (LOCK_EX | LOCK_NB) : LOCK_EX);

README mentions the path: "Runs are serialized with a lock (/tmp/wancarp.lock)".

Suggested fix

Move the lock to a root-only directory, e.g. /var/run/wancarp.lock.
For event runs, replace the blocking LOCK_EX with a LOCK_EX | LOCK_NB retry loop (e.g. every 0.5 s up to ~60 s, longer than a normal run). On timeout, werr("timed out waiting for lock ...") and end with the existing aborted=lock banner. Note that a timed-out event loses its update, which is acceptable because the next reconcile re-applies the role. Mention this in the comment.
Keep the 'e' (close-on-exec) mode flag and its comment; it is still required.
Update README and header references to the path, and add a CHANGELOG [Unreleased] entry.

Acceptance

Holding the lock from another shell (lockf -k /var/run/wancarp.lock sleep 120) makes an event run log the timeout and aborted=lock within the bound; reconcile still backs off quietly.

## Summary The run lock is `/tmp/wancarp.lock`, and CARP event runs wait on it with a blocking `flock(LOCK_EX)` and no timeout. Two problems: 1. **World-writable location.** Any local user can create `/tmp/wancarp.lock` and hold an `flock` on it. Every CARP event run then blocks forever, so failover never changes WAN state, and every `--reconcile` quietly backs off (`exit(0)`, nothing logged). Opening with `'ce'` doesn't truncate, so a symlink can't clobber anything; the problem is denial of service only. Low risk on a firewall with no untrusted shell users, but cheap to fix. 2. **No timeout.** Even without a hostile user, one hung run (e.g. `interface_configure()` stuck) blocks every later event with nothing in the log. #11 (inherited lock FD held by `dhclient`) was this failure mode: events blocked indefinitely and reconcile went silent. A bounded wait would have made it visible. ## Where - `const LOCKFILE = '/tmp/wancarp.lock';` (around line 171) - lock block (around lines 656–667): ```php $lockfh = fopen(LOCKFILE, 'ce'); $locked = $lockfh !== false && flock($lockfh, $reconcile ? (LOCK_EX | LOCK_NB) : LOCK_EX); ``` - README mentions the path: "Runs are serialized with a lock (`/tmp/wancarp.lock`)". ## Suggested fix - Move the lock to a root-only directory, e.g. `/var/run/wancarp.lock`. - For event runs, replace the blocking `LOCK_EX` with a `LOCK_EX | LOCK_NB` retry loop (e.g. every 0.5 s up to ~60 s, longer than a normal run). On timeout, `werr("timed out waiting for lock ...")` and end with the existing `aborted=lock` banner. Note that a timed-out event loses its update, which is acceptable because the next reconcile re-applies the role. Mention this in the comment. - Keep the `'e'` (close-on-exec) mode flag and its comment; it is still required. - Update README and header references to the path, and add a CHANGELOG `[Unreleased]` entry. ## Acceptance - Holding the lock from another shell (`lockf -k /var/run/wancarp.lock sleep 120`) makes an event run log the timeout and `aborted=lock` within the bound; reconcile still backs off quietly.

eric added the

bug

label

2026-10-22 17:36:12 -07:00

eric referenced this issue from a commit

2026-10-23 15:47:58 -07:00

fix: move the run lock to /var/run and bound the event wait

eric closed this issue

2026-10-23 15:47:58 -07:00

eric referenced this issue

2026-10-23 15:48:54 -07:00

Release test plan: live checks before tagging the next release #21

eric referenced this issue from a commit

2026-10-23 16:06:45 -07:00

fix: follow-up audit of #15, #18 and #20

eric commented

2026-10-23 16:07:31 -07:00

Author

Owner

Follow-up audit: the lock was still takeable by a local user

Fixed in 222180a.

Finding. Moving the lock to /var/run stopped other users from creating it, but fopen(LOCKFILE, 'ce') runs as root with umask 022, so the file came out 0644. flock(LOCK_EX) works on a read-only descriptor (confirmed in PHP), so any local user could still open it read-only and hold it. With the 60 s bound, event runs would end aborted=lock (logged), but --reconcile backs off silently whenever the lock is held, so drift would go uncorrected with nothing in the log.

Fix. The lock is opened under umask(0077), so a new file is 0600, and then chmoded to 0600, which also tightens a file that already exists. Two harness cases check this: a new lock file ends up 0600, and an existing 0644 one is tightened to 0600. Both fail against the previous script.

#21's #18 section now expects -rw------- and adds a check that nobody can't take the lock.

## Follow-up audit: the lock was still takeable by a local user Fixed in 222180a. **Finding.** Moving the lock to `/var/run` stopped other users from creating it, but `fopen(LOCKFILE, 'ce')` runs as root with umask 022, so the file came out `0644`. `flock(LOCK_EX)` works on a read-only descriptor (confirmed in PHP), so any local user could still open it read-only and hold it. With the 60 s bound, event runs would end `aborted=lock` (logged), but `--reconcile` backs off silently whenever the lock is held, so drift would go uncorrected with nothing in the log. **Fix.** The lock is opened under `umask(0077)`, so a new file is `0600`, and then `chmod`ed to `0600`, which also tightens a file that already exists. Two harness cases check this: a new lock file ends up `0600`, and an existing `0644` one is tightened to `0600`. Both fail against the previous script. #21's #18 section now expects `-rw-------` and adds a check that `nobody` can't take the lock.

eric referenced this issue

2026-10-23 16:07:31 -07:00

Release test plan: live checks before tagging the next release #21