net: prevent persisted Tor control command injection #36142

l0rinc commented at 8:09 PM on September 1, 2026: contributor

Problem: A node creates its onion service through Tor's control protocol and caches the returned private key for reconnects. Quoted reply values are unescaped, so a control endpoint can return an escaped line break that becomes part of the cached key. When the node reconnects, it authenticates to Tor and inserts that key into an ADD_ONION command, where the embedded CRLF frames the remainder as a separate control command. An unprivileged local process can exploit this by impersonating the default loopback endpoint while Tor is unavailable, seeding the cache, and releasing the port before Tor returns. A compromised operator-configured endpoint can return the same malicious key. The functional test demonstrates SIGNAL SHUTDOWN. No P2P or RPC input reaches this path.

Fix: This PR rejects CR and LF in every outbound Tor control command and rejects a returned private key before adopting or caching it. The outbound-command check also protects nodes with an existing malformed cache and covers line breaks in -torpassword. Valid Tor keys are unaffected. A malformed cached key leaves the onion service unavailable until the operator removes the file and restarts the node, but can no longer frame another command.

test: characterize Tor control command injection

Tor control reply decoding unescapes quoted values. Record that a private key returned with an escaped CRLF is cached
verbatim and lets its remainder reach Tor as a second command through a later `ADD_ONION` request.

3b817b6dda

torcontrol: reject line breaks in commands

Tor control commands end with CRLF, so a line break inside a command sends its remainder as a separate command. Reply
decoding can introduce one into an `ADD_ONION` private key, which is cached and resent on reconnect. A malicious control
endpoint can therefore persist a command that the node later sends after authenticating to Tor. A line break in
`-torpassword` can frame a second command through `AUTHENTICATE` in the same way.

Reject line breaks before sending a Tor control command, and reject returned keys before adopting or caching them. Valid
Tor keys are unaffected. An existing malformed cache leaves the onion service unavailable until the operator removes it,
but cannot frame a second command.

b6ed2bcc77

DrahtBot added the label P2P on Sep 1, 2026

DrahtBot commented at 8:09 PM on September 1, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36142.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	winterrdog, jeanpablojp, vasild

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35292 (test: Add coverage for Tor control HASHEDPASSWORD authentication by winterrdog)
#34486 (net: Reduce local network activity when networkactive=0 by willcl-ark)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

winterrdog commented at 9:35 AM on September 2, 2026: contributor

concept ACK

jeanpablojp commented at 2:06 AM on September 3, 2026: contributor

Concept ACK

The CRLF side looks well covered. The space still gets through. The key goes into ADD_ONION unquoted, and in the control protocol a space is what separates arguments. A returned key carrying one still passes the check, gets cached, and goes back out on the next startup, this time to the real Tor.

If the injected token is a Port= on the same virtual port the node publishes, Tor ends up with two targets for it and connections go to either one. Pointed at a real Tor instead of the mock, fifteen of thirty connections to the node's onion address landed on the injected target, with no warning about it in the log. Swap the Port= for Flags=Detach and the service stays up after bitcoind exits.

in src/torcontrol.cpp:550 in b6ed2bcc77

 541 | @@ -531,6 +542,14 @@ void TorController::add_onion_cb(TorControlConnection& _conn, const TorControlRe
 542 |              }
 543 |              return;
 544 |          }
 545 | +        if (private_key) {
 546 | +            // Command() would refuse to send it back, so never adopt or cache such a key.
 547 | +            if (ContainsLineBreak(*private_key)) {
 548 | +                LogWarning("tor: ADD_ONION returned a private key containing a line break");
 549 | +                return;
 550 | +            }

jeanpablojp commented at 2:06 AM on September 3, 2026:

Would it make sense to reject the space here alongside CR and LF? It is the only other byte that works as a separator there, and none of the twenty keys I asked Tor for had any of the three.

            // A line break frames a second command and a space smuggles further
            // ADD_ONION arguments, so never adopt or cache such a key.
            if (private_key->find_first_of(" \r\n") != std::string::npos) {
                LogWarning("tor: ADD_ONION returned a private key containing a line break or a space");
                return;
            }

I applied this locally and the torcontrol unit and functional tests still pass, with no assertion changed.

in src/torcontrol.cpp:225 in b6ed2bcc77

 219 | @@ -215,6 +220,11 @@ bool TorControlConnection::ProcessBuffer()
 220 |  
 221 |  bool TorControlConnection::Command(const std::string &cmd, const ReplyHandlerCB& reply_handler)
 222 |  {
 223 | +    if (ContainsLineBreak(cmd)) {
 224 | +        // Log only the keyword because the arguments may carry the private key or password
 225 | +        LogWarning("tor: Refusing to send %s: command contains a line break", cmd.substr(0, cmd.find_first_of(" \r\n")));

jeanpablojp commented at 2:06 AM on September 3, 2026:

This guard covers a cache that already has CR or LF, but it can't be extended to the space, since control commands legitimately contain spaces. 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. That is what the operator has to delete, and it doesn't appear in the default log today.

vasild commented at 1:06 PM on September 3, 2026: contributor

Concept ACK

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. For example, for the ADD_ONION command:

https://spec.torproject.org/control-spec/commands.html#add_onion

The server reply format is:

    "250-ServiceID=" ServiceID CRLF
    ["250-PrivateKey=" KeyType ":" KeyBlob CRLF]
    *("250-ClientAuth=" ClientName ":" ClientBlob CRLF)
    "250 OK" CRLF

The KeyBlob format is left intentionally opaque, however ... For a “ED25519-V3” key is the Base64 encoding of ...

That is - better to treat anything that is not a valid base64 character as a malformed reply for KeyBlob.