fix: shell-quote directive targets to prevent OS command injection - #382
Open
bronson-calif wants to merge 1 commit into
Open
fix: shell-quote directive targets to prevent OS command injection#382bronson-calif wants to merge 1 commit into
bronson-calif wants to merge 1 commit into
Conversation
Builtin directive templates for shell-backed platforms (FRR, BIRD, OpenBGPD, TNSR) interpolate the unauthenticated query target into a command string that is executed by a POSIX shell, so a shell metacharacter in the target could break out of its argument and run arbitrary commands on the managed router. - _construct.py: for shell-backed platforms, build the command as a shell-quoted argv line (split the trusted template into words, fill each word, then shlex.quote each) so the target always stays a single literal argument regardless of content. Non-shell NOS platforms keep the plain str.format path. - query.py: reject ASCII control characters in QueryTarget, closing the newline-injection vector on the non-shell NOS CLIs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bronson-calif
marked this pull request as ready for review
July 14, 2026 22:39
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.
Description
Directive command construction now shell-quotes the query target on platforms whose command string is executed by a POSIX shell, so an untrusted target can no longer break out of its intended argument.
For FRR/BIRD/OpenBGPD (
device_type="linux_ssh") and TNSR (dataplane shell sudo vtysh -c),Construct.format()no longer does a barestr.format. It splits the trusted template into words, fills the fields in each word, thenshlex.quotes every word and rejoins:Because the template is tokenized before the target is substituted, the target always lands inside a single argv word and can never introduce a new one — regardless of content. Legitimate regex syntax (
$,|,(),^$) and prefixes are preserved, and the previously-unquoted OpenBGPD template is covered by the same path. Non-shell NOS platforms keep the plainstr.formatpath, since their CLIs do not parse POSIX quoting.Separately,
QueryTargetnow rejects ASCII control characters (\x00–\x1f,\x7f). This closes the newline-injection vector on the non-shell NOS platforms, where netmiko'srstrip-onlynormalize_cmdotherwise lets an embedded newline execute as a second CLI line. No legitimate BGP query target contains a control character.There are no directive-template changes — the fix is entirely in the constructor and the query model, so a template can be edited freely without reintroducing the issue.
Scope note on TNSR: TNSR is included in
SHELL_PLATFORMSbecause it shells out. The quoting produces one correctly-quoted POSIX shell line, which is right for the bash thatdataplane shellreaches; the TNSR CLI parser in front of that bash has quoting semantics I have not verified against real firmware. FRR/BIRD/OpenBGPD go straight to bash and are unaffected. Happy to drop TNSR and track it separately if preferred.Related Issues
Fixes #383.
Motivation and Context
The builtin directive templates for the shell-backed platforms interpolate the unauthenticated query target directly into a command string that a POSIX shell then executes, e.g.
vtysh -c "show bgp ipv4 unicast regexp {target}". Two things let it break out:Construct.format()substituted the target with a barestr.format()— no shell escaping.condition="*"rule compiles tore.compile(".+").match(), which is start-anchored and matches any non-empty string, so every input passes validation.A
"in the target closes the quote and a following;/`/$(...)reaches the shell; OpenBGPD's template is unquoted, so a bare;injects directly. The response still returns the legitimateshow bgpoutput, so the injection is not visible in the query result. The net effect is arbitrary command execution on the managed router from a single unauthenticatedPOST /api/query.Tests
ruff checkandruff format --checkpass on all changed files (matches the CIrye lintstep); longest new line is 93 chars (limit 100).hyperglass/execution/drivers/tests/test_construct.py(a Juniper case) is unaffected.$()/ backtick /;/|/&/ newline bypass variants no longer execute anything on the router, while legitimate anchored (_65000$), empty-path (^$), and community (65000:100) queries still returnlevel=success.format()logic against that payload set to confirm no target can break out of its argument and that legitimate targets are preserved unchanged.