fix(security): harden query target validation against CLI injection#2
Open
z23 wants to merge 3 commits into
Open
fix(security): harden query target validation against CLI injection#2z23 wants to merge 3 commits into
z23 wants to merge 3 commits into
Conversation
The looking-glass interpolates user-supplied targets unquoted into device
commands via `command.format(target=...)`, then sends the result to the
router with Netmiko. Several gaps in the validation pipeline meant that an
anonymous user could submit a target containing CLI metacharacters and have
them reach the device shell:
- Built-in directives for `bgp_aspath` and `bgp_community` ship with
`condition: "*"`, which compiled to `.+` — every non-empty string passed.
- `RuleWithPattern` used `re.match`, which only anchors at the start, so a
custom regex without an explicit `$` allowed trailing payload.
- `QueryTarget` had no `max_length` and only stripped surrounding whitespace,
so embedded `\n`/`\r` survived and could send extra commands over SSH.
- `multiple_separator` was unvalidated, so an operator could misconfigure it
to `;` or `|` and turn multi-target queries into an injection vector.
Three layers of defence are added:
1. `QueryTarget` rejects control characters, `;|&\`<>"\\`, and is capped at
255 chars. Validation runs before any directive rule.
2. `RuleWithPattern` switches to `re.fullmatch`, and the `*` wildcard now
compiles to a restricted character class covering legitimate AS-path
regex and community syntax (digits, letters, `:_-^\$.*+?()[]` and space).
3. `Construct.format` re-checks the post-formatter target and refuses to
build a command if any forbidden character slipped through — a final
gate before the string hits Netmiko.
`Directive.multiple_separator` is now validated against `{" ", ","}`.
Regression tests in `models/tests/test_input_hardening.py` pin the
guarantees so a future loosening can't silently regress them.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Hardens query_target handling to prevent device CLI injection by rejecting metacharacters/control chars early, tightening directive wildcard validation, and adding a final pre-command construction safety check.
Changes:
- Add
QueryTargetmax length + pre-rule forbidden-character validation in the query model. - Tighten
RuleWithPatternmatching by switching tore.fullmatchand restrictingcondition: "*"to a safe character set. - Add a post-formatter forbidden-character gate in
Construct.formatand restrictDirective.multiple_separatorto an allow-list, with regression tests.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
hyperglass/models/api/query.py |
Adds max length + early forbidden-character checks for query targets before directive validation. |
hyperglass/models/directive.py |
Uses fullmatch and introduces a safe wildcard pattern; restricts multiple_separator. |
hyperglass/execution/drivers/_construct.py |
Adds final forbidden-character validation before formatting/sending device commands. |
hyperglass/models/tests/test_input_hardening.py |
Adds regression tests for metachar rejection, fullmatch semantics, and separator allow-list. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Centralize the forbidden-character set as `FORBIDDEN_TARGET_CHARS` in `hyperglass.constants` and import it in both `models.api.query` and `execution.drivers._construct`. The two layers still run independently (defense in depth at different points in the pipeline) but now share a single source of truth so they can't drift. - Fix `RuleWithPattern.validate_target` annotations: return type was `-> str` but the body returns `bool` and can raise; widen `target` to `Union[str, List[str]]` to match the existing list-handling branch. - Drop the unused `InputValidationError` import in `tests/test_input_hardening.py` (Ruff F401). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Existing regression tests only exercised the Layer-2 directive-rule layer (`RuleWithPattern`). The type-level check (`_check_query_target` / `FORBIDDEN_TARGET_CHARS`) and the post-formatter check in `Construct.format` were both untested, so they could regress silently. Add: - Direct `_check_query_target` parametrized tests for every metacharacter class (semicolon, pipe, ampersand, newline, CR, tab, NUL, backtick, double-quote, angle brackets, backslash) and accepted shapes. - A list-targets-checked-element-wise test mirroring the loop in `Query.validate_query_target`. - A test that pins the layered intent: a `condition='.*'` rule would permit a metachar-containing target at Layer 2, but Layer 1 still rejects it. - A parametrized `Construct.format` test that mutates `target` post-init to a forbidden value and asserts `InputInvalid` — proving Layer 3 still rejects even if Layers 1/2 are bypassed. Co-Authored-By: Claude Opus 4.7 (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
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
Anonymous users on a public hyperglass instance can submit a
query_targetthat flows, unquoted, into acommand.format(target=...)template and is then sent verbatim to the router over SSH. Several gaps let CLI metacharacters reach the device:bgp_aspath/bgp_communitydirectives usecondition: "*", which compiled to.+— every non-empty string passed validation.RuleWithPatternusedre.match(anchors only at the start), so a stricter custom regex like[0-9]+accepted12345; reboot.QueryTargethad nomax_lengthand only trimmed surrounding whitespace, so embedded\nsurvived and could send extra commands on the SSH channel.multiple_separatorwas unvalidated, so a misconfigured;or|would chain commands at the device.This PR adds three layers of validation:
QueryTarget(type level,models/api/query.py) — caps length at 255, runs a pre-rule check that rejects control chars and;|&\<>"\`. Targets containing any of those never reach a directive rule.RuleWithPattern(rule level,models/directive.py) — switches tore.fullmatch;condition: "*"now compiles to a restricted character class covering legitimate AS-path regex and community syntax (digits, letters,:_-^$.*+?()[]and space). Regex alternation (|) is intentionally excluded since most built-in templates interpolate the target unquoted, where|would be parsed as a CLI pipe filter rather than alternation.Construct.format(transport level,execution/drivers/_construct.py) — final post-formatter gate that re-checks the target and refuses to build a command if any forbidden character slipped through. Defense in depth in case a custom directive's regex is too loose.Directive.multiple_separatoris also restricted to{" ", ","}.Regression tests in
hyperglass/models/tests/test_input_hardening.pypin the guarantees: metachar rejection, fullmatch semantics, the separator allow-list, and that legitimate AS-path/community targets still pass.Out of scope (filed in the review, not in this PR)
RuleWithoutValidationdefault for directives without acondition(operator-facing behaviour change)known_hostsenforcement; non-root Dockerfile USER; security headersTest plan
pytest hyperglass/— 71 passed, 1 deselected (the deselectedtest_device_builtins_vrf_directive_renders_vrf_in_commandalready fails onmainand is unrelated)test_input_hardening.pycover: wildcard rejecting;,&,<,>, backtick, double-quote, backslash, redirect, embedded\n/\r; wildcard accepting normal AS-path / community values;re.fullmatchblocking trailing payload;multiple_separatorallow-list (rejects;/|, accepts" "/",")🤖 Generated with Claude Code