net: validate Tor onion service replies and cached keys #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. Tor reply parsing unescapes quoted values, so a control endpoint can return a private key containing a line break or space. When the node reconnects, it inserts that key unquoted into an ADD_ONION command, where CRLF frames the remainder as a separate command and a space adds further arguments. 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 reply handler also continues after receiving an invalid service ID, logging it and caching the key before attempting to advertise the invalid address.

Fix: This PR validates the returned service ID as a Tor v3 onion address before logging it, caching the key, or advertising the service. It accepts NEW:ED25519-V3 or an ED25519-V3 key whose Base64 payload decodes to 64 bytes. Returned keys are validated before adoption or caching, and cached keys are validated before reuse. A malformed cached key leaves the onion service unavailable until the operator removes the file and restarts the node, but cannot inject another command or argument.

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.

vasild commented at 4:49 PM on September 4, 2026: contributor

In general, we can be more strict when receiving replies from the Tor daemon, not just the private key blob. For example, the ServiceID received as a reply to our ADD_ONION command is passed to LookupNumeric() and LogInfo() (see TorController::add_onion_cb()) without much checking. 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.

refactor: prepare ADD_ONION validation

Extract the `NEW:ED25519-V3` key request and let Tor control tests configure the service ID and private key returned by `ADD_ONION`.
The restart helper stops the node before clearing or seeding the key cache, giving each test explicit reply and cache state.

`NoPowServer` only overrides the PoW rejection because the base mock supplies the successful retry response.

3896015744

test: characterize ADD_ONION reply handling

`ADD_ONION` replies provide the onion service ID and may include a generated private key.
Record that a malformed service ID still reaches logging and key caching, while quoted private keys are unescaped and cached verbatim.
Reusing the same bytes from the cache lets an embedded CRLF or space inject another control command or argument.

a04f9ca413

torcontrol: validate ADD_ONION reply fields

A control endpoint can return a malformed service ID or private key in an `ADD_ONION` reply.
A cached key containing CRLF can frame another control command when reused, while a space can add another `ADD_ONION` argument.
A malformed service ID reaches logging and key caching before the node tries to advertise it.

Require the service ID to resolve to a valid onion address through `LookupNumeric()` before continuing.
Before adopting, caching, or reusing a private key, require `NEW:ED25519-V3` or an `ED25519-V3` key with a Base64 blob that decodes to 64 bytes.
A malformed reply stops onion service creation, and a malformed cached key keeps the service unavailable until the operator removes the file.

6683358296

l0rinc force-pushed on Sep 4, 2026

l0rinc renamed this:
~~net: prevent persisted Tor control command injection~~
net: validate Tor onion service replies and cached keys
on Sep 4, 2026

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

Rebased and pushed, addressed all comments, thank you! Private-key format is now checked when received and before cached reuse, blocking both CRLF and space injection. Service IDs are validated before caching the key or advertising the service, and malformed-cache warnings name the file to remove.

Thanks @jeanpablojp for identifying the space-injection gap and suggesting cached-key checks, and @vasild for suggesting strict key-format and service-ID validation.

in src/torcontrol.cpp:556 in 6683358296

 547 | @@ -532,6 +548,10 @@ void TorController::add_onion_cb(TorControlConnection& _conn, const TorControlRe
 548 |              return;
 549 |          }
 550 |          m_service = LookupNumeric(std::string(m_service_id+".onion"), Params().GetDefaultPort());
 551 | +        if (!m_service.IsValid()) {
 552 | +            LogWarning("tor: ADD_ONION returned a malformed service ID");
 553 | +            return;
 554 | +        }
 555 |          LogInfo("Got tor service ID %s, advertising service %s", m_service_id, m_service.ToStringAddrPort());
 556 |          if (WriteBinaryFile(GetPrivateKeyFile(), m_private_key)) {

winterrdog commented at 10:04 PM on September 4, 2026:

just a suggestion we can consider separately: should we explicitly enforce owner-only permissions when writing the cached onion private key ?

this feels kind of similar to how we handle other sensitive files, e.g. GenerateAuthCookie() explicitly (if provided) sets restrictive permissions on the RPC cookie (fs::perms, -rpccookieperms): https://github.com/bitcoin/bitcoin/blob/4519933391dd23dbf1a4eceec6dd53d2e9e71cc3/src/rpc/request.cpp#L100-L146

since this file contains the private key controlling the .onion identity, it seems worth doing the same here. just wanted to flag it while we are touching an area adjacent to private key handling

any thoughts? or the current approach is just fine ?

vasild commented at 12:57 PM on September 8, 2026:

Yes, makes sense. But the correct way is to set the filesystem permissions first and after that write the sensitive data to it, or create it right away with the desired permissions using umask. The above snippet first writes the data and then sets the permissions :( while : ; do cat /path/to/sensitive_file ; done is a trivial way to exploit the first-write-then-set-permissions approach.

There is SetupEnvironment() which calls umask(0077) which should make the above safe, but still it would be better to call fs::permissions() first and then file << COOKIEAUTH_USER << ":" << rand_pwd_hex; in the above snippet. I am not sure why fs::permissions() is needed if this relies on the global umask.

winterrdog commented at 2:02 PM on September 8, 2026:

There is SetupEnvironment() which calls umask(0077) which should make the above safe

ah, yes! case closed.

I am not sure why fs::permissions() is needed if this relies on the global umask.

yes, not necessary

but still it would be better to call fs::permissions() first and then file << COOKIEAUTH_USER << ":" << rand_pwd_hex; in the above snippet.

correct! i will find out why that is so