ci: scan changed files for malicious code and disguised content - #4038
Closed
pbeza wants to merge 9 commits into
Closed
ci: scan changed files for malicious code and disguised content#4038pbeza wants to merge 9 commits into
pbeza wants to merge 9 commits into
Conversation
Nothing in CI inspected file content. Scanners that select files by extension never open a payload named .woff2, and dependency scanners read only the dependency graph, so code vendored straight into the tree went unexamined. Adds a required job with two content-based checks. YARA runs DataDog's GuardDog pack, a generic behavioural set covering obfuscation, download-and-execute, silent process spawn, reverse shells and exfiltration. Magika decides content type from content, so a script parked behind an asset extension is visible even when the code itself looks unremarkable; it fires only when the detected type is executable, the extension claims a binary asset, and that extension disagrees with what the type is normally called. All three conditions are needed, or a legitimate .wasm trips it. Only rules in .github/yara/blocking-rules.txt fail the build. That split is measured: a rule qualifies at zero false positives across the 923 tracked files and the 2824 file-versions touched by the last 400 commits on main. 36 of 54 qualified; the excluded ones are mostly capability_* rules, which report that a capability is present rather than misused. New rules therefore default to advisory, so a version bump cannot introduce an unmeasured gate. The pack is fetched from a pinned release and checksum-verified rather than committed, keeping third-party rules out of review while staying reproducible, as ci.yml already does for repro-env. The job runs on all branches, since a push to the branch of a merged PR fires no pull_request event, and skips the pull_request run for same-repo branches to avoid scanning and annotating twice. It reads the changed-file list with git diff -z: without it core.quotePath quotes any path holding a byte above 0x80, that literal fails the -f test, and a homoglyph-named payload is dropped from the scan silently. A missing base commit, a missing scanner or a scanner crash all fail the job, because a scan that examines nothing would otherwise report success.
pipx install magika==1.1.0 fails: 1.1.0 is what the CLI reports as its library version, while the newest package on PyPI is 1.0.3.
This comment was marked as outdated.
This comment was marked as outdated.
Three problems, two of which broke the property the job claims for itself. The base-commit fallback scanned only the tip commit. `github.event.before` is all-zeroes when a push creates a branch, and on a force-push it is the previous tip, now orphaned and so not fetched even at fetch-depth 0. Both fell through to HEAD~1, so a branch created with five commits, or a force-push of two, left everything but the tip unexamined - the force-push case being exactly what this job exists to catch. Measured on this branch: HEAD~1 covers 1 file where the branch delta covers 6. Now falls back to the merge-base with the default branch, and to HEAD~1 only when pushing to the default branch itself. The comment that described this had both halves of the payload semantics backwards, which is how the gap stayed hidden. The scan exited 0 when HEAD_SHA was absent from the clone. Only BASE_SHA was checked, and `mapfile < <(git diff ...)` discards the diff's exit status, so a fork PR whose head is force-pushed between dispatch and checkout produced "No files to scan" and a pass. Both ends are now verified and the diff is staged through a file so its status is checkable. Also: the allowlist is validated against the pack, so a rule renamed upstream fails loudly instead of quietly dropping to advisory; magika's stderr is kept out of the JSONL it parses, so a benign diagnostic cannot fail a clean PR; the allowlist path resolves from the script rather than the working directory; the read loop tolerates a missing final newline instead of demoting the last rule; yara targets are ./-prefixed so a dash-leading filename cannot be read as an option; and a file magika cannot classify is reported rather than dropped. Adds tests over throwaway repos covering the payload under both an ASCII and a homoglyph name, a genuine font, a payload in a non-tip commit, and every refusal path. The multi-commit case asserts both that the branch delta finds the payload and that a tip-only range does not, so it fails if the fallback regresses.
The scan flagged its own fixture: the file embedded a literal require-aliased- through-a-global and an eval sink, which is exactly what the blocking rules look for. Correct behaviour, so the fixture now assembles those strings at runtime instead of containing them.
…naries Switches the engine to YARA-X, VirusTotal's Rust successor to YARA, which is where all upstream work now goes; YARA 4.x gets bug fixes only. Verified behaviour-neutral first: over the whole tracked tree both engines report the same 171 matches across the same 18 rule types with the same per-rule counts, so the measured blocking allowlist carries over unchanged and no rule needed re-measuring. Scanning moves from one invocation per file to a single `yr scan --scan-list`. That deletes the per-file loop along with the workarounds it needed: yara accepts only one target and silently reinterprets extras as rule sources, which is why targets were ./-prefixed and each invocation's status checked separately. Two behaviours of yr needed handling, both now covered by tests. It exits 0 when it cannot read a listed file and reports only on stderr, with no --fail-on-error, so non-empty stderr is fatal. And --scan-list is newline-delimited, so a path containing a newline cannot be expressed - git permits those, so the script refuses rather than mis-listing one as two entries. `yr` deserialises the compiled pack in ~22-31ms and its thread pool is overhead on a short list, so the scan goes single-threaded below 50 files, comfortably above this repo's p90 diff of 17. Measured on 3 files: 27ms single-threaded against 33ms default, while at 930 files those invert to 330ms and 116ms. Installing the scanners replaces apt + pipx with two checksum-verified downloads, cutting a measured 16s from a 26s job: apt-get update 5.4s, apt-get install yara 2.9s, pipx install magika 7.4s. It also fixes the version confusion that broke this job earlier - magika numbers its CLI separately from its Python package, and the CLI number is what --version reports. The dev shell gains both scanners at the same pinned versions, so the scan and its tests are runnable locally. Both come from upstream release binaries rather than nixpkgs: yara-x there is older, and a nixpkgs bump would change the engine underneath the measured allowlist silently, while nixpkgs' magika-cli installs a binary with no LC_RPATH that aborts on every call under Darwin.
Researched the ~27ms fixed cost: Rules::deserialize does a bincode decode, a wasmtime Cranelift JIT of the rules' WASM module, and an Aho-Corasick rebuild, with neither the native code nor the automaton serialised into a .yarc by default. The native-code-serialization cargo feature would cut it to roughly 12ms, but it needs yr built from source, trading an upstream release binary for a self-built one in a security-critical job. Not worth ~15ms in a 14s job, and upstream main already serialises the automaton, so a future release improves it for free.
The Linux release binaries name the host's dynamic linker, which does not exist under nix, so the dev shell failed to build on Linux with 'cannot execute: required file not found' and took every nix-based CI job with it. autoPatchelfHook rewrites the interpreter and rpath. Caught only in CI because 'nix flake check' on Darwin skips the Linux systems, and because these derivations run the binary in installCheckPhase - nix/opengrep.nix installs its Linux binary without ever executing it, so an equivalent problem there would go unnoticed.
autoPatchelfHook went onto both new derivations when the Linux dev shell broke, without checking which one had failed. Only magika had: `file` on the release assets shows magika's Linux binary is dynamically linked while yara-x's is static-pie, so it names no interpreter and has no dynamic dependencies to rewrite. Each comment now says which case it is, so the hook is not re-added to yara-x for symmetry. Also drops the half of the yara-x header comment that restated flake.nix's existing "Pinned to CI version" note, keeping only the reason specific to this tool: nixpkgs has 1.16.0, and taking it from there would let a nixpkgs bump change the engine underneath the measured blocking allowlist.
2 tasks
Contributor
Author
|
Closing in favour of #4056, which takes the same threat in a substantially simpler direction. Two findings while reviewing this one drove the rewrite: The GuardDog rule pack does not cover Rust. All 54 rules carry Native code scanning already provides the machinery this reimplements. Emitting SARIF moves severity, false-positive triage, and merge blocking onto the platform: alert dismissal is per finding and persistent, so #4056 keeps everything from this PR that was load-bearing: the content-vs-extension check, and every cannot-scan-is-failure guard (missing commits, unreadable files, the Also carried over into #4056 as a required admin follow-up: |
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.
Closes #4037