Replace eval() with json.loads() in run_rabbitmqadmin#1686
Open
robjarawan wants to merge 3 commits into
Open
Conversation
… passwords After commit 72a8d19 removed unquote() calls from exec_rabbitmqadmin, the function relies on UrlParseResult.password to return the decoded value. The __main__ block used urllib.parse.urlparse() (plain ParseResult) which does not decode percent-encoding, so passwords like 'pass%23word' were passed verbatim to rabbitmqadmin instead of 'pass#word', breaking auth for anyone invoking the script directly with an encoded password. Switch __main__ to _urlparse() so url.password returns the decoded value consistently with the non-main code paths. Add three tests: two confirming decoded passwords reach the subprocess, one pinning the stdlib urlparse behaviour to document why _urlparse is needed.
Two bugs in exec_rabbitmqadmin:
1. The legacy (Python < 3.5) path used subprocess.getstatusoutput() which
runs with shell=True. url.hostname, url.username, url.password, and
options were all string-concatenated into the shell command. A password
containing ';', '$(', or backticks executed arbitrary commands.
2. The modern path did command.replace("'", "").split(): this stripped ALL
single quotes from the entire command string (including those inside the
options value) then split on whitespace. A password with a single quote
was silently corrupted; a password or option value with a space was split
into multiple arguments.
Fix: build a list from the start and pass it to subprocess.run() directly.
The password is always a standalone list element, never shell-parsed.
shlex.split() is used only for the options string (operator-controlled
values like "list exchanges name" or "declare user name=X").
Drop the Python < 3.5 code path; Sarracenia requires Python 3.6+.
Add 7 tests covering: list-based dispatch, decoded passwords, passwords
with single quotes, passwords with shell metacharacters, option tokenisation,
AMQPS flag injection, and the stdlib-urlparse regression.
eval() on broker-supplied output is a remote code execution vector: a compromised broker or network MITM can return a Python expression instead of JSON and execute arbitrary code in the sr3 process. exec_rabbitmqadmin already passes --format raw_json, so the output is always a JSON array. json.loads() is the correct parser; it raises JSONDecodeError on non-JSON input rather than executing it. Also: remove the dead duplicate None/empty check above the parse block (already handled by the earlier guard), remove the now-redundant local 'import json' from the __main__ block, and promote json to a module-level import. Bare except: in run_rabbitmqadmin tightened to except Exception:. Add 4 tests: valid JSON round-trip, malicious eval payload safely rejected, non-JSON error text returns [], and failed subprocess exit code returns [].
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
1 participant
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.
run_rabbitmqadmincalledeval()on output returned by rabbitmqadmin. A compromised broker or a network MITM could return a Python expression instead of JSON and execute arbitrary code in the sr3 process.exec_rabbitmqadminalready passes--format raw_json, so the output is always a JSON array.json.loads()is the correct parser and raisesJSONDecodeErroron non-JSON input instead of executing it.What changed
eval(answer)replaced withjson.loads(answer)with aJSONDecodeErrorhandler that logs and returns[]except:tightened toexcept Exception:import jsonpromoted to module level (was duplicated in__main__)Tests
Added in
tests/sarracenia/rabbitmq_admin_test.py:test_valid_json_parsed_to_list— valid JSON array round-trips correctlytest_malicious_eval_payload_not_executed— a list comprehension that would run undereval()is safely rejected byjson.loads()test_non_json_response_returns_empty_list— error text from rabbitmqadmin returns[]without raisingtest_failed_subprocess_returns_empty_list— non-zero exit code returns[]without raisingAll pass. Full credential suite (31 tests): 31 passed, 0 failed.
Fork CI unit tests: passing — https://github.com/robjarawan/sarracenia/actions/runs/24844972369