Remediate Command Injection, Secret Leakage, and DoS Vulnerabilities in chainmail - #31
Open
mertcano wants to merge 1 commit into
Open
Remediate Command Injection, Secret Leakage, and DoS Vulnerabilities in chainmail#31mertcano wants to merge 1 commit into
chainmail#31mertcano wants to merge 1 commit into
Conversation
…in `chainmail`
### Description
This pull request introduces critical security remediations across the `chainmail` application, addressing high-severity vulnerabilities identified during the workspace audit[cite: 20]. The updates secure shell executions against command injection, prevent private keys from leaking into application logs, harden the on-chain signature verification logic to fail closed, and implement decompression bounds to protect against memory exhaustion (DoS) attacks[cite: 20].
---
### Key Changes & Remediations
#### 1. Command Injection Mitigation (`chainlink.py`)
* **Safe Subprocess Execution:** Replaced unsafe `os.popen` calls that executed shell commands built with f-string interpolation[cite: 20]. Commands are now assembled as argument vectors (lists) and executed via a centralized `_run(argv)` helper using `subprocess.run(..., check=False)` without a shell[cite: 20, 35]. This prevents shell metacharacter injection from attacker-controlled inputs (like email addresses or PGP fingerprints)[cite: 20, 35].
#### 2. Secret Redaction & Logging (`chainlink.py`)
* **Log Sanitization:** Previously, `chainlink.py` printed invoked commands to standard output verbatim, leaking `--private-key` arguments and Etherscan API keys into CI logs and terminal scrollbacks[cite: 20, 35]. A new `redact()` helper now sanitizes all `stdout`/`stderr` streams and logged command invocations, replacing known secrets with `<redacted>`[cite: 20, 35].
#### 3. Fail-Closed On-Chain Verification (`chainlink.py`)
* **Strict Signature Verification (`verify_email_message`):** Fixed a critical logical flaw where verification returned `True` for *any* output that didn't contain the substring `"false"` (meaning RPC errors or missing binaries incorrectly passed as verified)[cite: 20, 35]. The function now strictly requires a `0` exit status and an explicit affirmative response (`'true'` or ABI-encoded `1`) to confirm registration[cite: 20, 35].
* **Robust Error Handling:** Removed error detection based on the `"error"` substring, which previously missed silent failures[cite: 20]. Command success is now authoritatively determined by the process exit status (`returncode != 0`), and missing `logs` are safely handled via `.get('logs', '[]')` without raising `KeyError`s[cite: 20, 35].
#### 4. Decompression Bomb (DoS) Protection (`chainmail.py`)
* **Bounded Zlib Inflation (`get_content_string`):** The application previously decompressed untrusted base64 URL payloads with `zlib.decompress` without size limits, leaving the web process vulnerable to memory exhaustion from highly compressed malicious payloads[cite: 20, 36].
* **Hard Limits:** Input is now capped at 256 KiB (`MAX_ENCODED_BYTES`), and decompression is bounded to 1 MiB (`MAX_DECOMPRESSED_BYTES + 1`) via `zlib.decompressobj().decompress()`[cite: 20, 36]. Oversized payloads are safely rejected before materializing in memory[cite: 20, 36].
* **Strict Base64 Decoding:** Enabled `validate=True` during `b64decode` to reject non-alphabet characters, and narrowed the previously overly broad `except:` block to catch only specific decoding/decompression errors[cite: 20, 36].
#### 5. Deployment Hardening & Code Cleanliness (`runwebsite.py`, `chainlink.py`)
* **Disabled Unsafe Debuggers:** The Flask/Werkzeug debugger (`app.run(debug=True)`) was previously enabled by default, exposing a remote code execution (RCE) vector[cite: 20, 38]. This is now disabled by default and made opt-in for local development via `os.getenv('CHAINMAIL_DEBUG') == '1'`[cite: 20, 38].
* **Regex Hardening:** Replaced greedy, unanchored regex (`<.+>`) for extracting sender emails with a safe, anchored raw string pattern (`r'<([^<>]+)>\s*$'`)[cite: 20, 38]. This prevents the wrong address from being extracted and verified when a username contains multiple angle brackets[cite: 20, 38].
* **Resource Lifecycle:** File access operations across the application now properly utilize `with open(...)` context managers, ensuring deterministic file handle closures[cite: 20, 35].
---
### Validation
* Syntax and bytecode successfully validated via `python -m py_compile chainlink.py chainmail.py runwebsite.py` (Exit Status 0)[cite: 20].
* Template rendering confirmed secure; Jinja auto-escaping remains enabled and protects verification outputs[cite: 20].
* *Note: Full integration testing requires GnuPG, Foundry binaries (`cast`/`forge`), and active Ethereum RPCs, which were not available in the audit environment.*[cite: 20]
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
This pull request introduces critical security remediations across the
chainmailapplication, addressing high-severity vulnerabilities identified during the workspace audit[cite: 20]. The updates secure shell executions against command injection, prevent private keys from leaking into application logs, harden the on-chain signature verification logic to fail closed, and implement decompression bounds to protect against memory exhaustion (DoS) attacks[cite: 20].Key Changes & Remediations
1. Command Injection Mitigation (
chainlink.py)os.popencalls that executed shell commands built with f-string interpolation[cite: 20]. Commands are now assembled as argument vectors (lists) and executed via a centralized_run(argv)helper usingsubprocess.run(..., check=False)without a shell[cite: 20, 35]. This prevents shell metacharacter injection from attacker-controlled inputs (like email addresses or PGP fingerprints)[cite: 20, 35].2. Secret Redaction & Logging (
chainlink.py)chainlink.pyprinted invoked commands to standard output verbatim, leaking--private-keyarguments and Etherscan API keys into CI logs and terminal scrollbacks[cite: 20, 35]. A newredact()helper now sanitizes allstdout/stderrstreams and logged command invocations, replacing known secrets with<redacted>[cite: 20, 35].3. Fail-Closed On-Chain Verification (
chainlink.py)verify_email_message): Fixed a critical logical flaw where verification returnedTruefor any output that didn't contain the substring"false"(meaning RPC errors or missing binaries incorrectly passed as verified)[cite: 20, 35]. The function now strictly requires a0exit status and an explicit affirmative response ('true'or ABI-encoded1) to confirm registration[cite: 20, 35]."error"substring, which previously missed silent failures[cite: 20]. Command success is now authoritatively determined by the process exit status (returncode != 0), and missinglogsare safely handled via.get('logs', '[]')without raisingKeyErrors[cite: 20, 35].4. Decompression Bomb (DoS) Protection (
chainmail.py)get_content_string): The application previously decompressed untrusted base64 URL payloads withzlib.decompresswithout size limits, leaving the web process vulnerable to memory exhaustion from highly compressed malicious payloads[cite: 20, 36].MAX_ENCODED_BYTES), and decompression is bounded to 1 MiB (MAX_DECOMPRESSED_BYTES + 1) viazlib.decompressobj().decompress()[cite: 20, 36]. Oversized payloads are safely rejected before materializing in memory[cite: 20, 36].validate=Trueduringb64decodeto reject non-alphabet characters, and narrowed the previously overly broadexcept:block to catch only specific decoding/decompression errors[cite: 20, 36].5. Deployment Hardening & Code Cleanliness (
runwebsite.py,chainlink.py)app.run(debug=True)) was previously enabled by default, exposing a remote code execution (RCE) vector[cite: 20, 38]. This is now disabled by default and made opt-in for local development viaos.getenv('CHAINMAIL_DEBUG') == '1'[cite: 20, 38].<.+>) for extracting sender emails with a safe, anchored raw string pattern (r'<([^<>]+)>\s*$')[cite: 20, 38]. This prevents the wrong address from being extracted and verified when a username contains multiple angle brackets[cite: 20, 38].with open(...)context managers, ensuring deterministic file handle closures[cite: 20, 35].Validation
python -m py_compile chainlink.py chainmail.py runwebsite.py(Exit Status 0)[cite: 20].cast/forge), and active Ethereum RPCs, which were not available in the audit environment.[cite: 20]