~/research/2026-08-25

Validating the cached Apple-display device path before use

merged upstream: basecamp/omarchy #8198 · merged 2026-08-28 · defense-in-depth · shell / caching

tl;dr

Omarchy's brightness wrapper for Apple displays caches the detected HID device path so repeated brightness changes skip re-detection. It trusted that cache on two weak grounds: any existing path was accepted as a device, and with no XDG_RUNTIME_DIR the cache lived at a predictable path in the world-writable /tmp. My patch validates the cached value's shape (a /dev/…hiddev* character device, nothing else) and drops the /tmp fallback entirely. No exploit today — the downstream binary rejects non-Apple devices on its own — so this was filed honestly as hardening, and it's now merged upstream. Review sharpened the tests before the merge; more on that below.

background: what the wrapper does

Omarchy — DHH's opinionated Arch-based Linux distribution, developed on GitHub under the basecamp organization — controls the brightness of Apple Studio/Pro displays through asdcontrol, a third-party tool that talks to the display's USB HID interface. Detecting the right /dev/…hiddev* node is slow, so the wrapper omarchy-brightness-display-apple caches the detected path in a small file and reuses it on the next call.

Anything cached is a value you once computed — and will later trust. That later trust is where the interesting questions live: who else can write this file, and what do you check before using what you read?

gap one: the cache was trusted if the path merely existed

The old check was:

if [[ -n $cached && -e $cached ]]; then
  printf '%s\n' "$cached"   # hand it to asdcontrol
fi

-e asks "does something exist at this path" — not "is this a HID device node". A stale cache (USB interfaces renumber when a display is replugged), or any unexpected value that ends up in that file, is passed straight to a tool that runs with elevated privileges via sudo. The wrapper's own detection logic only ever produces /dev/hiddev* or /dev/usb/hiddev* paths — so the cache accepting anything else means the read path is more permissive than the write path. That asymmetry is the smell.

gap two: a predictable path in a world-writable directory

device_cache="${XDG_RUNTIME_DIR:-/tmp}/omarchy-brightness-display-apple.device"

With XDG_RUNTIME_DIR unset, the cache file lands at a fixed, guessable name in /tmp — a directory every local user can write to. Whoever creates that file first owns its contents; the wrapper would then read another user's value and treat it as its own cached detection. Combined with gap one ("any existing path is fine"), that is a textbook cache-poisoning setup: attacker writes, victim reads, no validation between.

why this is hardening, not a live vulnerability

This mirrors the reasoning of my earlier plugin-add guard (there: a git default; here: a third-party binary's check) and complements upstream's #7336 (still open as of this writing), which re-detects when a cached node stops responding. Liveness and shape are different concerns: #7336 asks "does it still answer?", this patch asks "is it even the kind of thing we'd have written?".

the fix

Two changes, both in the wrapper only:

# Trust a cached value only if it still names a hiddev character device. A
# stale or unexpected cache (a regular file, a non-hiddev node) is ignored and
# we re-detect instead of handing an arbitrary path to asdcontrol. The globs
# are left unquoted on purpose: [[ ]] pattern-matches an unquoted right side,
# and quoting them would turn the match into a literal string comparison.
if [[ ( $cached == /dev/hiddev* || $cached == /dev/usb/hiddev* ) && -c $cached ]]; then
  printf '%s\n' "$cached"
  return 0
fi

The glob mirrors what the wrapper's own detection produces, and the -c test (is it a character device?) is what gives the glob teeth: a crafted path that merely starts with /dev/hiddev is a prefix match, but it is not a character device — only real device nodes under /dev are, and creating those requires root. One bash subtlety worth knowing: the globs are deliberately unquoted, because [[ ]] pattern-matches an unquoted right-hand side — quoting would silently turn the pattern into a literal string comparison.

And the cache location: cache only under the user-private $XDG_RUNTIME_DIR. If there is none, skip caching and detect every run — slightly slower, never poisonable. A predictable /tmp fallback is not a degraded mode; it's a different (worse) trust model.

testing the negative space

The test file is bigger than the fix, which is typical for this kind of guard. The interesting problems were all about testing things that must not happen, on machines you don't control:

• Assert on the negative. A stubbed asdcontrol logs every invocation; poisoned cache values (/dev/null, a regular file, a made-up path — and a glob-matching but non-existent /dev/hiddev999, of which more below) must never appear in that log. A blind-trust wrapper would hand them over and be caught.
• Some cases need hardware. The "valid cache is trusted" arm needs a real /dev/…hiddev* character device, which only exists with an Apple display attached — and can't be faked without root. The test runs that arm when the node exists and skips loudly otherwise, instead of pretending coverage.
• Testing the /tmp fallback without racing it. Proving the old /tmp path is no longer consulted requires planting a decoy at exactly that fixed path — in a shared directory. In the merged version the decoy is a FIFO created with mkfifo: atomic, fails outright if the path is taken, and follows no symlink — so the test avoids the same TOCTOU class it is testing for, and removes the decoy only if it created it. The FIFO also makes the assertion behavioral: a FIFO with no writer blocks whoever opens it, so a wrapper that still consults the path hangs under timeout (exit 124) while one that ignores it exits normally.

what review changed

The upstream review is worth a section of its own, because it practiced what this writeup preaches — and caught me not quite doing it. The reviewer ran a mutation matrix: revert the wrapper, drop each half of the validation in turn, and see whether the suite notices. It didn't, twice.

• The -c check had no coverage. Every poison value in my original list already failed on the pathname prefix, so none of them ever reached the character-device test — deleting && -c $cached left the suite green. The fix is a poison that matches the hiddev glob but is not a device: a non-existent /dev/hiddev999, which is also the realistic stale-cache case (display replugs, interface renumbers, node is gone).
• My /tmp assertion was vacuous too. The original set -C decoy held a value the other half of the validation rejects anyway, so it asserted on the contents when it needed to prove the open. Hence the FIFO above: it separates a wrapper that opens the path from one that never touches it, regardless of what validation happens later.

After those two fixes (pushed to the branch during review), all four mutations fail, each on the assertion that names it. There's a lesson in that beyond this patch: a test suite is itself a cache of assumptions — the only way to know an assertion has teeth is to break the code it guards and watch it bite.

Review also surfaced a sibling change worth watching: #8532 (open as of this writing) constrains the asdcontrol sudoers rule to hiddev paths — the same invariant, enforced one layer down at the privilege boundary. If both land, the wrapper check becomes belt-and-braces; that trade-off is the maintainer's call.

takeaways

• A cache is an input. Validate what you read with the same rigor as what you accept from argv — especially when the file can be older than the world around it, or written by someone else.
• Read paths should be as strict as write paths. If your code only ever writes /dev/…hiddev*, your code should only ever accept /dev/…hiddev* back.
• No safety from directories you share. Predictable filenames in /tmp hand the first-writer advantage to whoever is fastest. Prefer user-private dirs; degrade by doing less, not by trusting more.
• Say what it isn't. Filed as hardening with the non-exploitability stated up front — same honest-severity approach that got the last patch merged. The review even verified the claim: asdcontrol /dev/null exits with "Unsupported device", as stated.
• Mutation-test your guards. A validation check whose deletion keeps the suite green is a check nobody is actually testing. Break the code on purpose; make each assertion earn its place.

Full patch and discussion: basecamp/omarchy #8198 — submitted 2026-08-25, merged 2026-08-28.