security: harden the PAM trust boundary (Plan 05)#86
Open
tyvsmith wants to merge 2 commits into
Open
Conversation
- Sanitize oneshot child environment: env_clear + allow-list
(SSH_CONNECTION, SSH_TTY, pinned PATH); blocks LD_PRELOAD/LD_*
injection into the root-context child.
- Verify config trust before parsing: /etc/facelock/config.toml and all
parent dirs must be root-owned and not group/world-writable; fstat on
the opened fd avoids TOCTOU. Untrusted config fails closed (PAM_IGNORE).
- Verify daemon peer UID: resolve org.facelock.Daemon owner, require
UID 0, and pin the Authenticate call to the owner's unique bus name.
Removes the single point of failure on the D-Bus policy file.
- Make the -2 error contract live (option 4a): the daemon now encodes
recoverable errors (rate limited, IR required, camera/storage failure)
in-band as AuthResult{model_id:-2,label}; the PAM client classifies
them (rate limited -> PAM_AUTH_ERR) instead of silently escalating to
a fresh root oneshot attempt. CLI ipc_client decodes -2/-3 sentinels.
- Structured timeout classification: match zbus InputOutput(TimedOut),
MethodError(NoReply/Timeout/TimedOut), and fdo variants instead of
substring matching; bound connection establishment with an overall
deadline on a worker thread.
- Container tests: group/world-writable config rejection, LD_PRELOAD
marker + child env capture, non-root fake daemon peer refusal
(test-arch-pam); rate-limit no-escalation assertions
(test-arch-integration).
- Update docs/contracts.md and docs/security.md.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The container config has no [storage] section, so the daemon uses the default /var/lib/facelock/facelock.db; derive the path from the config instead of hardcoding /tmp/facelock-test.db. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the PAM module’s trust boundary by tightening how pam-facelock interacts with configuration, the oneshot fallback process, and the D-Bus daemon—aiming to prevent environment injection, untrusted config influence, and forged daemon replies while preserving “fall through to password” semantics.
Changes:
- Adds root-only peer verification for
org.facelock.Daemonand pinsAuthenticatecalls to the daemon’s unique bus name. - Sanitizes the oneshot child process environment (
env_clear+ allow-list + pinnedPATH, stdin nulled). - Makes the in-band
model_id == -2/-3contract live across daemon, PAM client, and CLI client; adds structured timeout classification and related test coverage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/run-integration-tests.sh | Adds integration assertions for “rate-limited daemon must not escalate to oneshot”. |
| test/run-container-tests.sh | Adds camera-free container tests for config trust, env sanitization, and non-root fake-daemon harness. |
| test/fake-facelock-daemon.py | Adds a fake non-root D-Bus daemon used to validate PAM peer-UID verification. |
| test/Containerfile | Installs Python/GObject/sqlite deps and copies the fake daemon harness into the container. |
| docs/security.md | Documents PAM peer verification, trusted-config validation, and in-band recoverable error encoding. |
| docs/contracts.md | Documents sentinel model_id encoding and updated PAM semantics for daemon errors/timeouts. |
| crates/pam-facelock/src/lib.rs | Implements structured timeout classification, daemon peer verification, trusted config reading, oneshot env allow-list, and error classification routing. |
| crates/facelock-cli/src/ipc_client.rs | Decodes model_id == -2/-3 sentinels into higher-level CLI responses. |
| crates/facelock-cli/src/commands/daemon.rs | Encodes recoverable auth errors in-band (model_id == -2) and adds a unit test for that contract. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+149
to
+150
| run_test "Rate limit: PAM fails without oneshot escalation" \ | ||
| "printf '#!/bin/bash\ntouch /tmp/oneshot-invoked\nexit 2\n' > /usr/local/bin/oneshot-marker && chmod 755 /usr/local/bin/oneshot-marker && sed -i '/^\[daemon\]/a auth_bin = \"/usr/local/bin/oneshot-marker\"' /etc/facelock/config.toml && rm -f /tmp/oneshot-invoked; timeout 30 pamtester facelock-test testuser authenticate < /dev/null; rc=\$?; sed -i '/^auth_bin = /d' /etc/facelock/config.toml; test \$rc -ne 0 && test ! -f /tmp/oneshot-invoked" |
Comment on lines
+139
to
+140
| run_test "Rate limit: seed failed attempts" \ | ||
| "sqlite3 $FACELOCK_DB \"INSERT INTO rate_limit (user, attempt_time) SELECT 'testuser', strftime('%s','now') FROM (VALUES (1),(2),(3),(4),(5),(6));\"" |
Comment on lines
+152
to
+153
| run_test "Rate limit: clear seeded attempts" \ | ||
| "sqlite3 $FACELOCK_DB \"DELETE FROM rate_limit WHERE user = 'testuser';\"" |
Comment on lines
+195
to
+197
| run_test "Peer-UID: non-root daemon owner yields no PAM_SUCCESS" \ | ||
| "! timeout 15 pamtester facelock-test testuser authenticate < /dev/null" \ | ||
| 0 |
Comment on lines
+640
to
+649
| let metadata = file | ||
| .metadata() | ||
| .map_err(|e| format!("failed to stat config {path}: {e}"))?; | ||
| validate_root_owned_nonwritable( | ||
| metadata.uid(), | ||
| metadata.permissions().mode(), | ||
| &format!("config {path}"), | ||
| )?; | ||
|
|
||
| let mut current = Path::new(path).parent(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens the PAM client's trust boundary (Plan 05).
pam-facelockdependencies stay libc/toml/serde/zbus only; fall-through-to-password is preserved on every ambiguous/error state (no fail-open toPAM_SUCCESS).env_clear+ a vetted allow-list (SSH_CONNECTION,SSH_TTY) + pinnedPATH, stdin nulled. BlocksLD_PRELOAD/LD_*injection into the root-context child./etc/facelock/config.tomland all parent dirs must be root-owned and not group/world-writable;fstaton the opened fd avoids TOCTOU. Untrusted config fails closed (PAM_IGNORE).org.facelock.Daemonowner, require UID 0, and pin theAuthenticatecall to the owner's unique bus name — removing the single point of failure on the D-Bus policy file.-2error contract live (chore: self-host model downloads from GitHub assets release #15): the daemon now encodes recoverable errors (rate limited, IR required, camera/storage failure) in-band asAuthResult{model_id:-2,label}; the PAM client classifies them (rate limited →PAM_AUTH_ERR) instead of silently escalating to a fresh root oneshot. CLIipc_clientdecodes the-2/-3sentinels.zbusInputOutput(TimedOut),MethodError(NoReply/Timeout/TimedOut), and fdo variants instead of substring matching; connection establishment is bounded by an overall deadline on a worker thread.Verification
cargo build/cargo test/cargo clippy -D warningsgreen; unit test for the decode branch.just test-arch-pam, camera-free — primary gate): group/world-writable config rejection;LD_PRELOADmarker.so+ child-env capture provingenv_clear; a non-root process owningorg.facelock.Daemonreturningmatched=trueyields noPAM_SUCCESS(peer-UID check). All existingPAM_IGNORE/fail-closed/missing-config/privilege cases stay green.just test-arch-integrationwas run to confirm a rate-limited daemon state does not silently escalate to a fresh root oneshot (rate-limit no-escalation assertions added; integration seeding now derives the realdb_pathfrom config).Contract docs
docs/contracts.mdanddocs/security.mdupdated for the in-band daemon error encoding (-2contract).Closes #71
Closes #72
Closes #73
Closes #74
Closes #75
🤖 Generated with Claude Code