fix(modules): validate modules at load instead of at match time - #383
Open
TBX3D wants to merge 7 commits into
Open
fix(modules): validate modules at load instead of at match time#383TBX3D wants to merge 7 commits into
TBX3D wants to merge 7 commits into
Conversation
a word matcher with no words (or a regex matcher with no patterns) evaluates to true under the default AND condition, so it silently matches every response instead of never matching. this is easy to hit via a yaml typo: word: instead of words:, a forgotten words: list, or bad indentation nulling the list. a words: [""] list hits the same bug through strings.Contains(x, ""), which is always true. reject both cases in validateMatchers, the shared load-time check http, dns, and tcp module configs all route through, so the guard applies to every transport that accepts word/regex matchers.
validateMatchers only special-cased favicon/range/encoding and let any other type string through, so a typo like "words" for "word" loaded fine and just never matched (executor default case returns false). dns and tcp already reject unknown matcher types in their own validators; give the shared http/dns/tcp path the same allowlist. also add a permanent test that parses every shipped module under modules/, so a future load-time guard gets checked against real modules instead of just fixtures.
regex patterns in matchers and extractors compiled lazily at match time across http, dns, and tcp, swallowing the compile error and just skipping the pattern. an invalid regex (a typo, an unbalanced paren) loaded fine and then silently never matched or extracted, for the life of the module. compile every matcher and extractor regex pattern during ParseYAMLModuleBytes instead, covering http (including request-chain steps), dns, and tcp, so a bad pattern fails load with the compile error attached.
ParseYAMLModuleBytes only validated http/dns/tcp/fingerprint config when its pointer was non-nil, so "type: http" with no http: section (or a typo'd section name like htttp:) parsed and registered clean; the mismatch only surfaced at Execute, which a passive scan never reaches, so the module quietly counted as loaded and did nothing. require the block matching the declared type. also switch parsing to KnownFields(true) so a typo'd field inside a present section (e.g. methdo: for method:) is caught too; verified against all 189 shipped modules with zero strict-decode failures before enabling it.
LoadAll treated builtins as all-or-nothing: if anything loaded from the on-disk modules/ dir, the embedded set was skipped entirely. running sif from a directory that happens to contain a modules/ folder (an unrelated project, a planted one) replaced every builtin with whatever was on disk, dropping the rest silently. load the embedded set as the baseline and let the on-disk builtin dir layer over it: Register already replaces by id, so a disk module overrides only its own id and every other embedded module survives. also stop trusting the bare cwd-relative "modules" fallback outside a sif checkout. it is offered when go.mod in the working directory names this module (so editing modules/ and re-running the binary still works) or when there is no embedded fs to fall back on at all; anywhere else the trusted paths are the directory colocated with the binary, the embedded fs shipped in it, and the packaged data dirs, so an unrelated cwd's modules/ folder is not silently treated as a builtin source.
loadDir's filepath.Walk callback returned any per-entry error (an unreadable file, a directory whose contents can't be listed) straight through, which aborts the whole walk: every module sorted after the bad entry never got a chance to load, silently. log and skip the bad entry instead so the rest of the walk proceeds.
severity was a free-form string copied verbatim into Finding.Severity, so a typo like "CRTICAL" loaded fine and just never ranked against a real severity downstream. reject a severity that names something outside info/low/medium/high/ critical (case-insensitive), matching the levels documented in docs/modules.md. an empty severity is left alone: plenty of modules and test fixtures omit it deliberately, which is a separate concern from catching a typo in one that's actually set.
github-actions
Bot
added
modules
pr summary7 files changed (+447 -16)
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #383 +/- ##
=======================================
Coverage ? 65.26%
=======================================
Files ? 88
Lines ? 7920
Branches ? 0
=======================================
Hits ? 5169
Misses ? 2355
Partials ? 396 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 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.
stacked on #382, so only the commits above it are this pr's.
six more guards over the same theme, a module that loads clean and then quietly does nothing. unknown matcher types were let through where dns and tcp already reject them, regex compiled lazily at match time and swallowed the error, a type: http module with no http: block only failed at Execute, which a passive scan never reaches, and a typo'd severity ranked against nothing downstream.
the loader itself had two. loadDir aborted the whole walk on one unreadable entry, and LoadAll skipped the embedded set wholesale whenever anything loaded from an on-disk modules/ dir, so running sif from a directory that happens to contain one replaced every builtin. embedded is now the baseline that disk layers over by id.