misc improvements - #378
Open
adrelanos wants to merge 26 commits into
Open
Conversation
Byte-identical cp from developer-meta-files/consumer-templates. pull_request and workflow_dispatch resolve to claude-code-action's agent mode, whose trigger is a non-empty 'prompt' input that this workflow does not pass, so those events could only spin up a runner and exit without calling the model - a green "Claude Code Review" check that reviewed nothing. Co-Authored-By: Claude <noreply@anthropic.com>
Byte-identical to developer-meta-files consumer-templates. The github.ref comment was wrong for one of the two events it described: issue_comment fires with the default branch ref, pull_request_review_comment with the PR merge ref (refs/pull/<number>/merge). No behaviour change -- the concurrency group reaches github.ref only if neither number field is set, and one always is for the events this workflow accepts. Generated with assistance from Claude Code.
Byte-identical propagation from developer-meta-files consumer-templates. Closes scanner-coverage gaps (bandit already present): - consumer-codeql-actions.yml: CodeQL 'actions' language -- workflow-YAML security analysis (script-injection, missing permissions, unpinned actions). - consumer-secrets-audit.yml: manual-dispatch secrets surface audit (presence flags only, values never emitted). Managed files -- do not hand-edit; regenerated on the next propagation pass. ## AI-Assisted Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CI: add codeql-actions + secrets-audit scanner workflows
…only consumer-claude-code: mention-only triggers (propagated from template)
The thirteen disabled-*-by-security-misc scripts are near-identical: they warn that a kernel module was blocked and exit 1. Add the strict preamble and convert echo -> printf (R-034). The '$@' inside the message becomes '$*' deliberately. Under printf, "...$@" expands to one argument per positional parameter, so a multi-word invocation would print one word per line; '$*' joins with spaces, which is what echo did. Sandbox differential vs the pre-change scripts: all thirteen byte-identical in output and still exit 1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
These four scripts are invoked by pam_exec and their EXIT CODE is the
security decision: the PAM stacks use '[success=1 default=ignore]', so the
code selects whether the next module runs. Strict mode must not perturb it.
PAM_SERVICE and PAM_USER come from pam_exec's environment. Read bare under
nounset they abort the script with a NONZERO status, which PAM then reads
as a control-flow decision. Each is now read as '${VAR:-}', which
reproduces today's semantics exactly (an unset value falls through to the
same branch it already took).
pam-abort-on-locked-password was the sharp case. It documents "identifiable
exit codes in case stdout / stderr is not logged in journal"; with PAM_USER
absent it returned 3 ("user does not exist"), and a bare read turned that
into 1. Verified restored.
Exit-code matrix, old vs new, all matching:
PAM_SERVICE=login -> pam_only_if_login 1, pam_only_if_su 0
PAM_SERVICE=su -> pam_only_if_login 0, pam_only_if_su 1
PAM_SERVICE=sshd -> both 0
PAM_SERVICE empty -> both 0
PAM_SERVICE absent -> pam_only_if_login 0, pam_only_if_su 0,
pam_faillock_not_if_x 1
PAM_USER root / nonexistent / absent -> pam-abort 3 in every case
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers the eleven remaining standalones, plus the whole-file debt the gate surfaces on each: echo -> printf (R-034), 'has' instead of 'command -v' (R-090, sourcing has.sh -- security-misc-shared already Depends on helper-scripts), safe-rm (R-120), SC2250 braces. permission-lockdown needed care. Its loop chmods every home directory and records a state file per user, and it tolerated per-user failures: a directory it could not chmod did not stop the ones after it. Under errexit the first failure aborted the whole loop, so every remaining home directory silently stayed un-locked-down -- observed in the sandbox as 1 user processed instead of 11, and the script exiting 1 rather than 0. The two per-user operations are now explicitly best-effort, restoring the original behaviour (including that a failed chmod still records the state file -- arguably wrong, but changing it is not this pass's business). mmap-rnd-bits keeps its 'ls -1 -t /boot/config-* | head -n 1' with a reasoned SC2012 disable: kernel config filenames carry no spaces or newlines, so the parsing hazard is unreachable, and rewriting newest-file detection in a security package is risk without gain. Not touched, deliberately: the dracut modules under usr/lib/dracut/modules.d*. 'module-setup.sh' is SOURCED by dracut at initramfs build time, and remount-secure.sh is installed with 'inst_hook' and SOURCED by dracut-lib at BOOT. A strict-mode block in either runs in dracut's own shell; in the boot case that risks an unbootable system. Sandbox differential vs the pre-change scripts, comparing output AND exit code: 36 cases identical. The only two remaining deltas are 'set -x' traces of the preamble and of echo -> printf, both at unchanged exit codes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The gate passed locally but failed (or would fail) in CI. Cause: CI checks
each repo out ALONE, while locally they sit side by side as submodules
under derivative-maker/packages. A '# shellcheck source=' climbing out of
the repo ('../../../../helper-scripts/...') therefore resolves here and
not there, so shellcheck reports SC1091 and the gate fails on a commit
that looked clean.
The source= paths are kept -- they document where the file actually lives
-- and each is paired with a 'disable=SC1091' so an absent sibling is
tolerated. Same shape already used in security-misc master.
Follow-on findings from the same isolated run, each with a stated reason
rather than a blanket waiver:
- SC2154 on systemd_notify: assigned by the sourced systemd-notify.bsh,
which shellcheck can no longer follow.
- SC2034 on SCRIPTNAME and output_cmd: genuinely unused. suspend-pre's
output_cmd toggle is vestigial -- unlike suspend-post the script prints
directly -- flagged rather than removed, since wiring it up or dropping
it is a maintainer call.
- SC1090 on the uwt.d conf glob: a dynamic source, which R-081 prescribes
SC1090 for.
Verified with a new reproducer, ~/strict-pass-tests/ci-repro.sh, which
clones the repo on its own and runs the real gate there -- the condition
CI actually sees. All six repos: gate exit 0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
check-for-usb-controller: grep --quiet exits on the first match and closes the pipe, sending SIGPIPE to lspci. Under pipefail the pipeline then reports 141, so the guarding if takes the false branch even when a USB controller is present. Let grep read the whole stream and redirect to /dev/null instead. mmap-rnd-bits: head -n 1 has the same early-close hazard against ls; use sed -n '1p', which consumes all input, for consistency. Drop the redundant duplicate set -euo pipefail line already covered by the long-form preamble. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- load_state() split config lines on every space, so any entry whose filename contains a space was mis-fielded: silently skipped (a SUID binary left un-hardened) or mapped to a corrupted policy entry. - Recover the filename by parsing options from the right -- a trailing whitelist keyword, or the mode/owner/group[/capability] tail anchored by the octal mode. Space-free lines unchanged. - Add regression test (drives the real script via print-policy) plus its CI workflow. Canary-verified: fails on the pre-fix parser, passes now. - Bring the file to strict-mode / shellcheck compliance as required to touch it (6-directive block, printf, braces). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- emerg-shutdown.c was recompiled into /run on every boot. Move the compile to a standalone build-emerg-shutdown run from postinst, which installs /usr/bin/emerg-shutdown. Compiling on-target keeps the package Architecture: all while still producing a correct per-arch static binary. - Boot-time emerg-shutdown now just copies /usr/bin/emerg-shutdown into /run (memlockd-resident), with a fallback build if the binary is absent. - dracut module ships /usr/bin/emerg-shutdown (exists right after install, fixing the require_binaries /run chicken-and-egg). - Sandbox-verified: build-emerg-shutdown produces a static ELF in /usr/bin. - Strict/style compliance on the touched scripts (waivers for the debconf maintainer script and the dracut-sourced module). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- The [success=1 default=ignore] pam_exec lines run helper scripts that exit 1 on their expected control-flow path, emitting an alarming 'pam_exec(...): ... failed: exit code 1' journal line. Plain 'quiet' suppresses only the user-facing message, not the log. - Add 'quiet_log' (linux-pam >= 1.5.2, present on bookworm+) to every skip-logic pam_exec line (console-lockdown, unix-faillock, faillock-preauth, wheel), leaving the control flow byte-identical. - Update the scripts' comments to point at the resolution. Note: not verified on a live login journal (needs a real PAM stack); the change is additive and semantics-preserving regardless. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- fm-shim-frontend invoked xdg-mime and gio by bare name, depending on the systemd --user manager's PATH (minimal, profile-derived entries absent) that fm-shim-backend hands it. Use /usr/bin/xdg-mime and /usr/bin/gio, matching the existing /usr/bin/unicode-show call. - Reword the backend TODO: the whole-session PATH fix belongs in desktop-config-dist's start-lxqt-session (out of this repo); the frontend absolute-path change is the in-repo mitigation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Replace the bare 'TODO: research' markers on the permission-hardener whitelist entries (postfix, selinux/utempter, spice, virtualbox, qubes/qfile-unpacker, pam-tmpdir-helper, gst-ptp-helper) with concise documentation: what each helper is, why it is SUID/SGID, what breaks if stripped, and relevant CVE/hardening notes. - Retag the keep-vs-strip decisions TODO-HUMAN-DEVELOPER-ONLY -- a security-trust call for a human maintainer, not AI-resolvable. - 30_default.conf: fix the commented gst-ptp-helper path typo (gstreamer1.0/grstreamer-1.0 -> gstreamer-1.0/gstreamer-1.0); note that filenames with spaces are now parsed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…anup - ci/dfuzzer-build.sh: pin dfuzzer to the exact upstream commit and verify the tag resolves to it (a moved tag now fails loudly). - grub 40_kernel_hardening + remount-secure hidepid: reword standing 'TODO' notes to 'REMINDER' (tracking, not pending work). - remount-secure: replace the echo/true toggle with a printf-array output helper; bring the file to strict-mode / shellcheck compliance (6-directive block, braces) as required to touch it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Many scripts use 'shopt -s inherit_errexit' (bash 4.4+) in their
strict-mode preamble; ${misc:Depends} does not supply the baseline, so
an older /bin/bash would die in the preamble before the script's logic.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- undo_policy_for_file: the strict-mode brace pass quoted "${verbose}",
so 'permission-hardener disable all' ran 'chown "" -- ...' (invalid
empty owner) and 'chmod "" ...'. Make verbose an array so an empty
value expands to zero arguments. Canary-verified.
- emerg-shutdown fallback: when the prebuilt binary is absent, compile
straight into /run (tmpfs, writable even if /usr is read-only) rather
than via build-emerg-shutdown into /usr/bin.
- Document the space-filename parser limitations (consecutive spaces
collapse; an octal-looking path chunk can misanchor -> entry skipped).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Revert the bash (>= 4.4) Depends. Supported Debian (trixie+) always ships bash >= 4.4 and earlier is unsupported, so the inherit_errexit builtin is always present; the dependency adds nothing. Policy bans it (bash-style-guide R-010b). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ames - Space filenames are recovered from the option tail; the mode/owner/group [/capability] tail has a fixed width (3 or 4 fields). - Old left-to-right octal scan misanchored when a space-delimited filename chunk was itself a bare octal (e.g. '/path/a 744 name 0744 root root'): the field-count check then failed and the WHOLE hardener aborted (exit 200), contradicting the comment that claimed such a line was skipped. - Anchor the mode from the right (field_count-3, else field_count-4); prefer the 3-field reading so a filename ending in an octal chunk is not mistaken for a 4-field capability tail. - Regression test: config line with a bare-octal filename chunk must parse via print-policy; fails on the old code (exit 200 / entry missing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ## safe_echo.sh, strings.bsh) at startup; install-deps provides | ||
| ## them under /usr/libexec/helper-scripts/. | ||
| - name: Install genmkfile + helper-scripts + safe-rm | ||
| uses: org-ai-assisted/developer-meta-files/.github/actions/install-deps@master |
…fix credit - 30_default.conf: gst-ptp-helper ships in libgstreamer1.0-0, not gstreamer1.0-plugins-base; fix the disabled entry's path to .../gstreamer1.0/gstreamer-1.0/... (first component was gstreamer-1.0, which does not exist). Verified via dpkg -S. - 25_default_whitelist_qubes.conf: the #8633 EoP fix is qfile-unpacker (qubes-core-agent-linux) refusing a UID change other than the caller's; libqubes-pure path/symlink sanitization (qubes-linux-utils PR Kicksecure#87) is a separate layer. The comment credited only PR Kicksecure#87. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both surfaced by a fresh-claude review of the right-anchor change. - whitelists_disable_all=true was unreachable: the character-class check (which excludes '=') ran before the directive check, so the documented line tripped "invalid characters" and aborted the whole run (exit 200). Recognize the directive first. - Numeric octal-looking owner in a capability entry (e.g. '<f> 0744 0755 root cap'): right-anchoring folded the owner into the mode position, the folded path did not exist, and the entry was silently dropped -- file left un-hardened. Skip recovery when the line is already a well-formed no-space entry (octal mode at field 1, count 4 or 5). - Correct the stale field-order comment: the tail is 'filemode user_owner group_owner [capability]', mode first (per add_to_policy). - Regression tests: numeric-owner case in the whitespace test; new test_whitelists_disable_all.sh. Both fail on the pre-fix code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…stence Fresh-claude review found the earlier field-1 guard reintroduced a symmetric misparse: a spaced filename whose second component is a bare octal (e.g. '/srv/a 744 0644 root root') got dropped because the guard mistook the octal component for the mode. The 5-field reading is genuinely ambiguous lexically -- '/a 744 0644 root root' (spaced name) vs '/foo 0744 1000 root cap' (numeric owner). Resolve by which candidate filename exists on disk, consistent with the tool skipping nonexistent targets. Keep the no-recovery reading only when field 1 is an octal mode and only the un-recovered name exists. - Replace the lexical guard with the existence check inside the right-anchor branch; the whitelist-keyword branch is no longer gated (it was wrongly skipped for '/x 744 y matchwhitelist'). - Regression test: spaced octal-second-component case; the assertion now matches the whole first column (trailing TAB) so '/a 744' cannot substring-match the '/a 744 name' entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fresh-claude follow-up: the field_count==4/5 existence override did not cover a 6-field line (spaced filename + numeric octal-looking owner + capability), which anchored the mode on the owner and silently dropped the entry. No hardening bypass -- numeric owners are rejected later against passwd names -- but a silent drop instead of a loud rejection, and the ==4 disjunct was dead code. Replace the special-case override with one existence-based loop: among the octal candidates at field_count-3 and field_count-4, pick the one whose recovered filename exists; a mode at field 1 means no-space (no recovery); fall back to the rightmost octal candidate when neither name exists. Handles the 3-field, 4-field, spaced, numeric-owner and 6-field cases uniformly. - Regression test: 6-field spaced numeric-owner case; the suite now distinguishes this fix from all three prior parser iterations. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fresh-claude review of the whitespace feature found the write/read sides
disagree: parsing now accepts space-containing filenames into the policy, but
load_state_without_policy split the dpkg-statoverride DB line on whitespace and
required exactly 4 fields, silently dropping any spaced path. A spaced SUID file
then got hardened but could never be un-hardened ('disable' never found it), and
repeated 'enable' failed with exit 203.
- Read the three fixed fields (owner group mode) and take the remainder as the
path, matching the DB's 'owner group mode path' format.
- Regression test test_state_spaced_filename.sh: seed a spaced entry, disable,
assert the mode is restored (fails on the old reader: file stays un-restored).
- Test hygiene: print-policy captures modes into the state DB, so the two
print-policy tests now remove the entries they create instead of leaving stale
root-owned overrides for deleted temp files.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
4 participants
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.
mirror of org-ai-assisted#27