#36142 net: validate Tor onion service replies and cached keys
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36142 net: validate Tor onion service replies and cached keys | Ready | 3 | Strong | 35 + 106 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Validates Tor onion service IDs and private keys returned by the Tor control port and read from disk before using or caching them. It requires service IDs to resolve to valid Tor v3 addresses, verifies private keys against the Ed25519 v3 format with 64-byte payloads, and ensures replies without service IDs do not adopt keys or reuse state.
Problem: Tor reply parsing unescapes quoted strings, allowing a rogue or compromised Tor control port to return a `PrivateKey` containing CRLF line breaks or spaces. Because the node inserts cached keys unquoted into `ADD_ONION` commands on reconnect, CRLF enables Tor control command injection (such as `SIGNAL SHUTDOWN`) and spaces inject command arguments. In addition, previous logic retained state across replies and attempted to advertise and cache keys for invalid service IDs.
Evidence of importance:
- PR description: 'Tor reply parsing unescapes quoted values, so a control endpoint can return a value containing a line break or space. On reconnect, the node inserts that value unquoted into an ADD_ONION command. CRLF frames the remainder as a separate command, while a space adds further arguments.'
- Functional tests added in test/functional/feature_torcontrol.py explicitly demonstrate injecting 'SIGNAL SHUTDOWN' via CRLF and 'Flags=Detach' via spaces in returned and cached private keys.
Scope: Small diff across torcontrol.cpp and functional tests. Drops the `m_service_id` member variable to isolate per-reply state.
Risk of a mistake: medium (p2p). Mistakes in Tor control parsing or key validation could cause nodes running onion hidden services to fail to bind or advertise their Tor address.
Changed since the previous assessment: Now explicitly isolates reply variables locally so that an `ADD_ONION` reply lacking a service ID cannot adopt keys or reuse the ID from a previous reply, removing the `m_service_id` class member.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- nothing: all reviewers satisfied, awaiting merge 2026-10-21
Agreement
Strong
substantive support, no open objection (vasild, jeanpablojp, winterrdog, winterrdog)
Thread notes: Added jpk68's ACK on commit 0a53d12589c3116495a50a694006ce5ee220fb2d; no new claims or objections raised.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| vasild (member) | suggestion, approach | Validate KeyBlob by strictly allowing only legitimate Base64 characters instead of blacklisting bad characters | fixed · fix pushed · author replied in c:5545894123 and implemented strict key-format validation | answered | yes | 2026-09-03: 'Instead of chasing bad characters from the Tor router reponses, it would be more robust to only allow legit characters and frown upon anything else.' Status from: 2026-09-04: l0rinc: 'addressed all comments, thank you! ... and @vasild for suggesting strict key-format and service-ID validation.' |
| jeanpablojp (contributor) | objection, safety | Space characters in returned keys bypass CRLF checks and allow injecting ADD_ONION arguments such as Port= or Flags=Detach | fixed · fix pushed · author replied in c:5545894123 and checked private-key format to block space injection | changed | yes | 2026-09-03: 'The space still gets through. The key goes into ADD_ONION unquoted, and in the control protocol a space is what separates arguments.' Status from: 2026-09-04: l0rinc: 'Private-key format is now checked when received and before cached reuse, blocking both CRLF and space injection.' |
| jeanpablojp (contributor) | suggestion, safety | Check the cached key in auth_cb before building ADD_ONION and name the file to remove in the warning | fixed · fix pushed · author validated cached keys before reuse and named the file to remove in warnings | answered | yes | 2026-09-03: 'Would it make sense to check the cached key in `auth_cb`, before the `ADD_ONION` is built? The warning could name the file there.' Status from: 2026-09-04: l0rinc: 'Private-key format is now checked when received and before cached reuse... and malformed-cache warnings name the file to' |
| vasild (member) | objection, safety | Malformed ServiceID in Tor daemon reply is passed unchecked to LookupNumeric() and LogInfo() instead of aborting | fixed · fix pushed · author replied in c:5545894123 and validated Service IDs before caching or advertising | changed | yes | 2026-09-04: 'But it must be exactly 56 base32 characters. If it is not, then it is safer to assume malformed reply from Tor, stop the processing right there, and not pass the malformed stuff to other functions.' Status from: 2026-09-04: l0rinc: 'Service IDs are validated before caching the key or advertising the service' |
| winterrdog (contributor) | suggestion, safety | Explicitly enforce owner-only permissions when writing the cached onion private key | accepted · vasild explained umask(0077) makes it safe and winterrdog accepted in rc:3958710162 | answered | by others | 2026-09-04: 'should we explicitly enforce owner-only permissions when writing the cached onion private key ?' Status from: 2026-09-08: winterrdog: 'ah, yes! case closed.' |
| vasild (member) | suggestion, correctness | Ensure the cached key file does not exist before running the test so assert_equal does not pass falsely | fixed · fix pushed · author noted unlink is already in restart_with_mock() and added an explicit assertion | answered | yes | 2026-09-25: 'Would be good here to ensure that the key did not exist or if it existed did not contain exactly the value of `valid_private_key` before this snippet runs.' Status from: 2026-09-25: l0rinc: 'The unlink seems to be present in `restart_with_mock()` already - added `assert not key_path.exists()` immediately after' |
| vasild (member) | suggestion, maintenance | Turn m_service_id into a local variable inside TorController::add_onion_cb() since it is not used elsewhere | accepted · fix pushed · author implemented local scope in push and vasild ACKed | answered | yes | 2026-09-26: 'why is this even a member variable? It is used only inside TorController::add_onion_cb(). I think it should be turned into a local variable inside the method.' Status from: 2026-09-28: vasild: 'ACK 0a53d12589c3116495a50a694006ce5ee220fb2d' |
| vasild (member) | suggestion, safety | Assign m_service to a local variable and only assign to the member variable after validating it | accepted · fix pushed · author implemented validation before adoption in push and vasild ACKed | answered | yes | 2026-09-26: 'it is assigned from LookupNumeric() and then checked whether is valid. If not, it will remain with its invalid value.' Status from: 2026-09-28: vasild: 'ACK 0a53d12589c3116495a50a694006ce5ee220fb2d' |
| winterrdog (contributor) | suggestion, safety | Avoid caching PRIVATE_KEY_NEW if Tor returns a ServiceID without a PrivateKey | accepted · winterrdog accepted the author's resolution and confirmed 'all right' | answered | yes | 2026-09-28: 'would it make sense to avoid caching `PRIVATE_KEY_NEW`, since it is a request token rather than a private key ?' Status from: 2026-09-29: winterrdog: 'all right' |
Support:
- winterrdog (contributor) (Concept ACK, 2026-09-02): (no reason given) [not substantive]
- vasild (member) (Concept ACK, 2026-09-03) [read]: Instead of chasing bad characters from the Tor router reponses, it would be more robust to only allow legit characters and frown upon anything else.
- jeanpablojp (contributor) (Concept ACK, 2026-09-03) [read]: The CRLF side looks well covered.
- vasild (member) (ACK, 2026-09-25) [read]: Some minor things below, feel free to ignore. [not substantive]
- winterrdog (contributor) (Tested ACK, 2026-09-25) [tested]: successfully built and tested on this toolchain: FreeBSD 15.0/clang++-19/x86_64. the changes LGTM (look good to merge)
- vasild (member) (ACK, 2026-09-26) [read]: Not a blocker below. Just something worth mentioning. [not substantive]
- vasild (member) (ACK, 2026-09-28): ACK 0a53d12589c3116495a50a694006ce5ee220fb2d Thanks! [not substantive]
- winterrdog (contributor) (Tested ACK, 2026-09-28) [tested]: again, successfully built and tested with `clang++-19` on FreeBSD `15.0`, targeting `x86_64`
- jpk68 (contributor) (ACK, 2026-10-21): (no reason given) [not substantive]
Participants:
- winterrdog (contributor, support, 8 statements 2026-09-02 to 2026-09-29): Tested ACK on FreeBSD; accepted resolution for PRIVATE_KEY_NEW caching and permissions
- jeanpablojp (contributor, support, 3 statements 2026-09-03 to 2026-09-03): Concept ACK; identified space-injection vector in ADD_ONION arguments and suggested cached-key validation
- vasild (member, support, 10 statements 2026-09-03 to 2026-09-28): ACK; suggested strict whitelist validation, test precondition assertion, and cleanups for m_service_id and m_service
- jpk68 (contributor, support, 1 statements 2026-10-21 to 2026-10-21): ACK on latest commit
Files
122 lines under test/bench/ci.
- test/functional/feature_torcontrol.py +106/-16
- src/torcontrol.cpp +35/-7
- src/torcontrol.h +0/-1
Card
The short text the ranking pass compares across PRs.
Validates Tor onion service IDs and private keys received from Tor control connections and cached on disk. It prevents an unprivileged local process or compromised Tor control endpoint from injecting arbitrary Tor control commands (via CRLF) or command arguments (via spaces) on reconnect. Keys must strictly match NEW:ED25519-V3 or decode to a 64-byte Ed25519 v3 key, and invalid service IDs or replies missing a service ID are rejected before caching or advertising.
Data
thread read through 2026-10-21 (33 statements), head 0a53d12589
labels: P2P
Categories
P2P
P2 · bug fix
Prevents command injection over the Tor control interface where CRLF or spaces in returned keys allowed executing unintended Tor commands such as 'SIGNAL SHUTDOWN' on reconnect. Strictly validating onion service IDs and private keys closes a clear security and stability hazard in Tor integration.
Membership: Matches the P2P area label and modifies Tor control socket communication in src/torcontrol.cpp.
Factors: security/stability 2, bug 2, performance 0, user value 0, leverage 0