Fix/rm 050 review blockers - #484
Open
toderian wants to merge 11 commits into
Open
Conversation
What changed: - harden excerpt redaction for multi-token and URL-safe credential shapes - bound DNS and streamed HTTP capture with existing timeout constants - prove per-node evidence isolation through final comparison aggregation Why: - close privacy, availability, and contract blockers found by independent review Checks: - focused RM-050 backend suite: 300 passed, 15 subtests passed - full RedMesh suite: 2045 passed, 4 unrelated baseline failures, 3 skipped
What changed: - cover compound credential key spellings in excerpt redaction - follow redirects manually without reading intermediate bodies - enforce a wall deadline around streamed reads and preserve exact-cap hashes Why: - close residual privacy, availability, and evidence-loss findings Checks: - response fingerprint and aggregation suites: 55 passed, 9 subtests passed
What changed: - Pin redirect hops to the authorized endpoint. The manual hop loop followed any Location, so a target answering `302 -> http://169.254.169.254/` made this node fetch cloud metadata and archive it as scan evidence. Each hop is now validated against the scanned host and port; an out-of-scope hop is refused and the last in-scope response is kept as the evidence. - Sanitize final_url and title. Both were archived raw while only the excerpt was scrubbed, so userinfo credentials and token query values persisted. Both now pass through sanitize_excerpt, and a new redaction pattern strips URL userinfo (which no key/value pattern recognised). - Emit body_complete in the http dict. When a body is truncated, body_length falls back to the peer's Content-Length and title comes from a partial body; consumers comparing vantages need to distinguish measured from declared. Why: - The redirect-scope escape is a live SSRF on the shipped path, not only a review blocker. Pinning to the authorized host+port is the whole control: classifying the hop IP would add nothing and would break scanning private targets, which is a normal authorized case. Checks: - red_mesh response-fingerprint suite: 41 passed, 7 subtests. - Full red_mesh suite: 2052 passed; the 4 rulebook-purge failures are pre-existing on this branch without these changes (verified by stash). - git diff --check clean.
What changed: - Add two payloads to the off-scope redirect cases: `http://host@evil.test/` (userinfo trick, where the real host is evil.test) and `//evil.test/x` (protocol-relative escape). Why: - These are the two classic ways a host pin gets bypassed. Both are refused today; encoding them keeps a later refactor of the scope check from silently reintroducing them. Checks: - response-fingerprint suite: 41 passed, 9 subtests. git diff --check clean.
What changed: - redirect_stays_on_target now requires urlparse AND urllib3's parse_url to agree on host and port. Validating with urlparse alone was bypassable: `http://169.254.169.254\@authorized-host/` reads as host authorized-host to urlparse but as 169.254.169.254 to urllib3, the parser requests actually dials with. The guard passed the hop and the node fetched cloud metadata - the SSRF this branch claims to close. - Scrub captured response headers. `location` is archived and routinely carries a token in its query string, especially on a hop refused as off-target whose raw Location was stored verbatim. - Test helper models headers with CaseInsensitiveDict as real responses do; a plain dict let a lookup pass in test while missing in production. Why: - Found by an independent security review of the pushed branch. The previous guard was necessary but not sufficient: it trusted a different parser than the one performing the connection. Requiring agreement fails closed against the whole differential class, not one spelling of it. Checks: - Full red_mesh suite: 2057 passed, 76 subtests; the 4 rulebook-purge failures are pre-existing and unrelated (verified earlier by stash). - Adversarial sweep of 14 payloads: 0 unsafe allows; every URL the guard permits resolves to the authorized host under requests' own preparation. - git diff --check clean.
What changed: - Extract the reader-shutdown sequence in read_bounded_response_body into a single _stop() used by all exit paths, and drain the queue before closing the response rather than after. Why: - Only the deadline path drained the one-slot queue; the byte-cap and queue-empty paths just set the flag and returned. Checking `stopped` before a put is not sufficient on its own: a put that was already blocked completes as soon as the consumer takes an item, refilling the queue, and the reader can then block on a further put before observing the flag. Queue.put has no timeout, so that thread waited forever, pinning a chunk and the response. - Draining before the close matters because response.close() can itself block on the lock the reader holds inside iter_content. Checks: - response-fingerprint suite: 44 passed, 14 subtests. Note: this does NOT fix the separate slow-drip hang, where response.close() blocks past the deadline against a real socket. That is tracked separately and still blocks merge.
What changed: - P0.1 Release the connection with socket shutdown instead of Response.close(). close() acquires the same buffer lock the reader thread holds inside iter_content, so against a peer that keeps dripping bytes it blocked far past the deadline - a 2s budget had not returned after 60s - stalling the whole scan job inline in the phase plan. shutdown() makes the blocked read fail at once and the reader unwinds on its own. - P0.2 Distinguish a token from prose by an unbroken alphanumeric run rather than by alphabet. Admitting `-` and `_` without that condition redacted ordinary hyphenated titles, CSS class names and paths that develop leaves intact, collapsing two genuinely different vantages to one token and manufacturing a false "no divergence". - P1 Match credential keys with (?<![A-Za-z0-9]) rather than \b. An underscore is a word character, so \b could never match after one and db_password=, jwt_secret= and similar keys archived their values verbatim. - P1 Guard the body read/parse block. It is no longer safe-by-construction now the body is streamed, and an uncaught error there reached execute_job's catch-all and skipped every remaining scan phase. - P0.3 Add real-socket tests: a slow-drip peer must not outlast the deadline (fails without P0.1 and fails fast rather than hanging), plus complete and oversized capture end-to-end through requests. Why: - Every existing test mocked the socket, so a close that blocks and a deadline that never fires were both invisible behind a green suite. Blockers found by three independent reviews of the pushed branch. Checks: - response-fingerprint suite: 49 passed, 21 subtests, 1.36s (was a 15s+ hang). - Full red_mesh suite: 2062 passed; the 4 rulebook-purge failures are pre-existing and unrelated. - Redaction matrix: 3 prose cases pass through, urlsafe/base64/stripe tokens still redact; regex timing 30-38ms at the 4096-char scan cap. - git diff --check clean.
What changed: - The byte-cap exit path still called response.close() directly instead of _stop(), so the blocking-close hang was only half fixed: a peer that bursts past the 1MB cap and then drips keeps the socket alive, close() blocks on the reader's buffer lock, and neither the deadline nor _fingerprint_http's finally is ever reached. Verified: reverting this one line hangs the new test for 20s; with it the read returns in under a second. - Drop the response.close() fallback from release_response_connection. It was the blocking call the helper exists to replace, on the one path where the reader is blocked, and the caller's finally already closes the response. - Remove `=` from the base64 rule's trailing exclusion. Including it made a long run followed by padding-plus-more unmatchable, narrowing the rule against what develop caught (~3% of previously matched tokens). - Guard the finally in _fingerprint_http: a raising close would escape both handlers, leak the session, and skip every remaining scan phase. - Consolidate the two near-identical test servers into one _Peer driven by a chunk script, with serving()/dripping() constructors, cleanup on both, and a clear failure when the local connect fails. Removes ~25 duplicated lines and makes the burst-then-drip case expressible - which is what the byte-cap regression test needed. Why: - Second independent review round. The half-fixed teardown is the one that mattered: the docstring stated "every exit path must call this" and one did not. Checks: - response-fingerprint suite: 50 passed, 21 subtests, 1.39s. - Full red_mesh suite: 2063 passed; 4 pre-existing rulebook-purge failures. - Byte-cap regression test verified to fail with the fix reverted. - git diff --check clean.
What changed: - Match session-cookie and SAML parameter names explicitly: jsessionid, phpsessid and samlresponse carry no `session`/`token` substring, so the key alternation never saw them and their values were archived verbatim. - Make the userinfo password half optional. `https://TOKEN@host/` is a credential, and without this the e-mail rule matched it first and replaced the host along with it - removing the field that makes two vantages comparable in the first place. Why: - Second security review round. Both reach the immutable archive through the captured location header and the body excerpt. - `code`, `state`, `sig` and `ticket` were reported alongside these and are deliberately NOT added: they are ordinary English words, and redacting them would destroy comparison signal exactly as the base64 rule did before it was narrowed. Recorded as a known residual instead. Checks: - response-fingerprint suite: 53 passed, 27 subtests. - Full red_mesh suite: 2066 passed; 4 pre-existing rulebook-purge failures. - Verified both directions: ordinary URLs, prose and CSS class names pass through untouched; userinfo, session ids and keyed credentials are redacted. - git diff --check clean.
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.
No description provided.