net: validate Tor onion service replies and cached keys #36142

pull l0rinc wants to merge 5 commits into bitcoin:master from l0rinc:l0rinc/tor-control-command-framing changing 3 files +141 −24
  1. 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 PrivateKey value for reconnects. 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. 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 is available again. A compromised operator-configured endpoint can return the same malicious value. The reply handler also continues after receiving an invalid service ID, logging it and caching the key before attempting to advertise the invalid address. A later reply without a service ID can reuse an ID left by an earlier reply.

    Fix: This PR validates the returned service ID as a Tor v3 onion address before logging it, caching the key, or advertising the service. Each reply must provide its own service ID, and reply fields stay local until validation succeeds. Key validation 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. It cannot inject another command or argument.

  2. DrahtBot added the label P2P on Sep 1, 2026
  3. DrahtBot commented at 8:09 PM on September 1, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    ACK vasild, winterrdog
    Concept ACK jeanpablojp

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36374 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36374.svg"></sub> (torcontrol: escape backslashes in -torpassword by 0xShadowX)
    • #35292 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35292.svg"></sub> (test: Add coverage for Tor control HASHEDPASSWORD authentication by winterrdog)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

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

    concept ACK

  5. 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.

  6. 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.


    l0rinc commented at 4:47 PM on September 28, 2026:

    Please check the latest version, tell me if you think it resolves your concerns

  7. 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.

  8. 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.

  9. 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.

  10. l0rinc force-pushed on Sep 4, 2026
  11. l0rinc renamed this:
    net: prevent persisted Tor control command injection
    net: validate Tor onion service replies and cached keys
    on Sep 4, 2026
  12. 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.

  13. in src/torcontrol.cpp:556 in 6683358296 outdated
     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

  14. DrahtBot added the label Needs rebase on Sep 16, 2026
  15. l0rinc force-pushed on Sep 25, 2026
  16. l0rinc commented at 3:46 AM on September 25, 2026: contributor

    Rebased, added coverage for service IDs with invalid checksums and returned private keys with unsupported types, invalid Base64, or the wrong length.

  17. DrahtBot removed the label Needs rebase on Sep 25, 2026
  18. l0rinc force-pushed on Sep 25, 2026
  19. DrahtBot added the label CI failed on Sep 25, 2026
  20. DrahtBot removed the label CI failed on Sep 25, 2026
  21. in test/functional/feature_torcontrol.py:20 in 6b3e23fe68 outdated
      12 | @@ -13,16 +13,20 @@
      13 |      p2p_port,
      14 |  )
      15 |  
      16 | +SERVICE_ID = "pg6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd"
      17 | +
      18 |  
      19 |  class MockTorControlServer:
      20 | -    def __init__(self, port, manual_mode=False):
      21 | +    def __init__(self, port, manual_mode=False, private_key=None, service_id=SERVICE_ID):
    


    vasild commented at 3:45 PM on September 25, 2026:

    The name private_key gives the impression that it is... hmm... the private key, but actually it is <key type>:<private_key>.


    l0rinc commented at 7:24 PM on September 25, 2026:

    I tried a few names here and ended up keeping private_key. Tor calls the full KeyType:KeyBlob reply value PrivateKey, and we store it in m_private_key, so the mock follows the same convention.

  22. in test/functional/feature_torcontrol.py:290 in 6b3e23fe68
     285 | +
     286 | +        self.log.info("Test a malformed service ID returned by ADD_ONION")
     287 | +        with self.nodes[0].assert_debug_log(["ADD_ONION returned a malformed service ID"], timeout=10):
     288 | +            self.restart_with_mock(mock_tor)
     289 | +        assert not key_path.exists()
     290 | +        mock_tor.stop()
    


    vasild commented at 3:55 PM on September 25, 2026:

    This text uses "invalid" and "malformed" interchangeably, but they are not quite the same:

    • malformed would be e.g. longer or shorter or that contains not-allowed character, e.g. aa:bb_c"d is malformed service id (contains invalid characters and is not 56 chars long)
    • invalid would be ag6mmjiyjmcrsslvykfwnntlaru7p5svn6y2ymmju6nubxndf4pscryd - it is not malformed, but has a mismatching checksum.

    Even if the above is somewhat subjective, better use just one term in the test, e.g. s/malformed/invalid/.


    l0rinc commented at 7:25 PM on September 25, 2026:

    Adjusted it everywhere, test name, log message, and commit text to use the nomenclature consistently.

  23. in test/functional/feature_torcontrol.py:301 in 6b3e23fe68 outdated
     296 | +        self.log.info("Test that a valid returned private key is cached and reused")
     297 | +        tor_control_port = self.next_port()
     298 | +        mock_tor = MockTorControlServer(tor_control_port, private_key=valid_private_key)
     299 | +        with self.nodes[0].assert_debug_log(["Cached service private key"], timeout=10):
     300 | +            self.restart_with_mock(mock_tor)
     301 | +        assert_equal(key_path.read_bytes(), valid_private_key.encode())
    


    vasild commented at 4:00 PM on September 25, 2026:

    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. Otherwise the assert_equal() may pass due to a stale file being present upfront even if bitcoind does not write to the file.

    Maybe easiest to add key_path.unlink(missing_ok=True) before MockTorControlServer().


    l0rinc commented at 7:31 PM on September 25, 2026:

    The unlink seems to be present in restart_with_mock() already - added assert not key_path.exists() immediately after it to make the precondition explicit.

  24. vasild approved
  25. vasild commented at 4:18 PM on September 25, 2026: contributor

    ACK 6b3e23fe6883569e07f4d8c9413b1dedcc510778

    Some minor things below, feel free to ignore.

    <details> <summary>Show Signature</summary>

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK 6b3e23fe6883569e07f4d8c9413b1dedcc510778
    
    Some minor things below, feel free to ignore.
    -----BEGIN PGP SIGNATURE-----
    
    iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmq2nqQbFIAAAAAABAAO
    bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/EYQf/A+7jw6WENj50dvG476P
    PXYyb95b1oW6w5Ht7JIauA9HwFiBiiKn1k8m5PA+pt7c/IzY6BxWc1WZBaYnuEOz
    jSoNKM1byO21NupAT0sA6uMsawgxhF/XJfgCQeLo7PQjSyDsm2a3WU1Gnjo5cSPU
    MlVi/ps8+2PGy+pxT/nbUS+fATtxYre8vLwY5FvCTaeoNEdPRrPYrLqDI7n9vGmB
    H1qi5n/n7czcuhUfLMK45CmTiAEQYDonHER1iThSHzHLT5kLdi+OCJKNaFuol+ka
    jTIjMe1wQTB/caTkIfhJ0PtHo587u4Z0RafeaKK8Yalls88y0QFIMQyt3Sxq29fU
    c1UOfM8kV2lU7k+NE0CWltZB6/HUlB6AWAIRAOVauPvq3arHeRkRhhx1I/zbJQp+
    IhkSsF0WrYsEcicBG1adOajIG0zUYsDjSzaRYbmuZIriayEY9FE5Kw8WVVTE3JU3
    +eZdOLio8vNNd/s15vVxcsl69MwOVFp4A9FhWxYtA8cSY7TAW44P2hCQgUbYNizN
    pQRwnvZXWTevDLJP2ujBrn0YS4Bq9Jj3k91j+8XArrP9FeMOKfgSzFp/VIXfeGYy
    XXWc7Wa57uC4EWR4aEq+LAQkojMpXNKky1BizBL5ak4xcpbw8jYTlMK7zvLvO/Cn
    OPN1f0WkDw3ztKRgsE1mSl8xc6EmRoLLq95fHfP0T0RaRghbEqL02JASpsafD2oY
    EsY4pRcMrTYcY4f7ojPPRTidduNt6y59+cQvBCupBr9fAQgTmWKCj0trsm/wOMBU
    /U+YnmDAGltGCmFABK7ZjKQ0O1+P8fyYmOTMP+cclPXDmiJzeGHdkoqRWb0yc6eA
    yDZ9BrGGBVo83gi53Vd2z8uJOogUrP4D030spL4ajM0GPg5bqVX/Jx5vt7qhEnQH
    MHQzboFJWeNHWLo6kX/pWtbS2juuHLEjN/qKFkcSaSSnmA7Zh7C6+xezN4wySNhb
    F+nsy1DYaNR/PEvx9Ajc3vTHrkm9ADxVJCXETMtw5t0KfQ7H3ECW/sKP7b244KXt
    pKo/BneW4MQfnrZ67OF2vdG44cgYacfg+NonJOVxIzH9APEkw5y799JdAjqlbAXo
    4mqkR53LzTbY2ho6xRJgDd6MBfsk6YK/mHAhivHVc+rwWo8ThxYBLNQxbng05RR1
    pnNCnqs3m1cbKFYXnUPA1Gtp4gEMbffIaDNWF+2nH97O3HkH5BpTtf47FEmdlYyz
    wf4btfb7ix/6ecMxX8sKnEseXg+xEi4hprcx7Sdepd7Ce1JDPz+2aqF6+fO5Sfdy
    1C+GPNNVl44/m2opycMurdMSSTvnApfMw2Rtmyued6flgWxiZFHjPZrxEUpwYFhv
    NsY=
    =GaWT
    -----END PGP SIGNATURE-----
    

    vasild's public key is on openpgp.org

    </details>

  26. DrahtBot requested review from jeanpablojp on Sep 25, 2026
  27. DrahtBot requested review from winterrdog on Sep 25, 2026
  28. l0rinc force-pushed on Sep 25, 2026
  29. l0rinc commented at 7:32 PM on September 25, 2026: contributor

    Updated the invalid-vs-malformed wording, added extra assertion, simplified the injection cases and commit messages.

  30. winterrdog commented at 11:00 PM on September 25, 2026: contributor

    tACK a3537ed6a344b5fdd4c9d4860985f9dc1521feb1

    successfully built and tested on this toolchain: FreeBSD 15.0/clang++-19/x86_64. the changes LGTM (look good to merge)

  31. DrahtBot requested review from vasild on Sep 25, 2026
  32. in src/torcontrol.cpp:533 in a3537ed6a3
     526 | @@ -511,6 +527,10 @@ void TorController::add_onion_cb(TorControlConnection& _conn, const TorControlRe
     527 |              return;
     528 |          }
     529 |          m_service = LookupNumeric(std::string(m_service_id+".onion"), Params().GetDefaultPort());
     530 | +        if (!m_service.IsValid()) {
     531 | +            LogWarning("tor: ADD_ONION returned an invalid service ID");
     532 | +            return;
     533 | +        }
    


    vasild commented at 8:41 AM on September 26, 2026:

    The following occurred to me in the middle of the night 🛌😴 🌃:

    The method TorController::add_onion_cb() reads the reply from the Tor server and if the reply is fine, then it saves it to disk and calls AddLocal().

    If the reply is invalid, then it should be discarded and shouldn't go outside of this method. However this method modifies two member variables m_service_id and m_service before fully validating everything. Their values will remain after the method returns due to the error. This could have unexpected outcomes depending on where these variables are read elsewhere, outside of TorController::add_onion_cb().

    • m_service_id - 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. At the start of the method we do not want to have a leftover value from a previous call to the method, I guess.
    • m_service - it is assigned from LookupNumeric() and then checked whether is valid. If not, it will remain with its invalid value. Better do:
    -        m_service = LookupNumeric(std::string(m_service_id+".onion"), Params().GetDefaultPort());
    -        if (!m_service.IsValid()) {
    +        const auto service{LookupNumeric(std::string(m_service_id+".onion"), Params().GetDefaultPort())};
    +        if (!service.IsValid()) {
                 LogWarning("tor: ADD_ONION returned an invalid service ID");
                 return;
             }
    +        m_service = service;
             LogInfo("Got tor service ID %s, advertising service %s", m_service_id, m_service.ToStringAddrPort());
    

    l0rinc commented at 3:20 AM on September 28, 2026:

    Thanks, hope this will make you sleep better!

  33. vasild approved
  34. vasild commented at 8:44 AM on September 26, 2026: contributor

    ACK a3537ed6a344b5fdd4c9d4860985f9dc1521feb1

    Not a blocker below. Just something worth mentioning.

    <details> <summary>Show Signature</summary>

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK a3537ed6a344b5fdd4c9d4860985f9dc1521feb1
    
    Not a blocker below. Just something worth mentioning.
    -----BEGIN PGP SIGNATURE-----
    
    iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmq3hfgbFIAAAAAABAAO
    bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/vuMf/3fDG66xgQIyGWz4PVXJ
    2tK6TOEJstD3ngUL7/yV/zz5wyaKwJYarA4AeYL1wCnySTq2p8BP5tZnncS+PkBs
    jcXtnWzMnasx0sGKZ1ib5wEmXVCn+KqOrq3Vw5MgjCelK6DOzk/LZqceyX+DI5/r
    mqElvXOcUDhvKS4lxSds0L+F3ZPLhx7LzFeErH5z/1Cu+ymTiB1S5Nn7WRyVQsDM
    aTMAvnHKFXuLgJ3lsnnRE5BKOOnrO5RlyM4nLftqWyu0aIeshpr1LXawxZ+k/Aau
    JngQC3GS5HgFR7GyIBh9MaFulJxVacCorU5Cswl67rTIc42lPeHEWTLddzpHtZi6
    +sxFnZO0MZ1jSBVrAvIqAJyezTE6hrxDTbxPI6xPjii8xalNJkq/VzByJw2UzuNy
    x+6vgPotN5+7IQni6/vDMNV+1kkkhHkT/eqXKKhtFdRjjuJ+xqU3x03/wsLTffTO
    DOdZ/LRwAg3z8wDuPzXIVyXzy/bw5gbknABtAqiSDF7UKhxfkSU3v4B9iD3u2rgJ
    mGYQ7q9Mec0lXNzguhJp+Y81szXZvdM/IzywcCwIR2BQ7rgjskB2DdJQD4j+1f45
    dEtihVyf8fnjhVOJcSHU3mr/na2XJOR6JtDSSV8tx9j0I+rLb3YHRKEywbzkZT/4
    kqXw6mF2d4zvdnlHXs5xvCRTWx16rfR7cb+TtFqRgQio4AfPAd00wt+li7BvMbLB
    vh5brnPUVKOXjZSPuvOQpzx0cvPikcm9ZhmGImVpCEetAXPfRwTTVwiylQ3v7J9N
    9U3MHGsRIfTJdFf/gRlZwfxWXMH8101g4Y74plm80vgcdUPHhS/mKDCVczd/5O2M
    kp/4Uu7x6GGED3r6yBS6dsoy08bAU96iJ6X6QknrU50ugXqMpF0T+C3WM4B9enzG
    nb8tAXxa7F0mWmCn33jemQy3/M0LCfwskf2pmcF24hM/YEd7nuaTf2YRtCZId3Xx
    T9a9B0VMfrGNib3L9YBrycTN1amvE7GC7W2b/4/VOcoIpy1eeTX6ZU8O1/L86ml0
    tMk6sSw7SouW2FQE8FmaKG0zIEKIfBu794F9Nn6yF+qLC35UUThHKiwbxw0gzir1
    kYRmBvQYwUVU4KKwLvgPKqtQtGocEWo/Y2Fy8a4XjYN3DmBLmT7Ijp13KHV+BkVH
    jRf/HZziEIqWwojYzSngfP7H6jPa5qPZDhtVkt2/r1mvQP4yfx4dPjH+N3pbfsmU
    lRAf+7BIoS7cumd2yEhlcSHroc8AgzHlNeDfvAWpJ87d4nHOF/GndSja7zbwUPQm
    pRK6mXCI9bHBVopBwAVG0MUq7G5FWvqxrqjhhQKwCXz4H1vWVaVGg/cUhJBBPpJt
    P8A=
    =FzqX
    -----END PGP SIGNATURE-----
    

    vasild's public key is on openpgp.org

    </details>

  35. refactor: prepare ADD_ONION validation
    Extract the `NEW:ED25519-V3` request and give the Tor mock a valid default service ID and configurable `PrivateKey` reply.
    Stop the node before clearing or seeding its key cache so each test has known reply and cache state.
    The no-PoW retry uses the mock's normal success reply.
    d63a8576d9
  36. test: characterize ADD_ONION reply handling
    The node logs a checksum-invalid service ID and writes a key-cache file.
    A key returned with that invalid service ID is reused after reconnect.
    A later reply without a service ID reuses the prior ID and adopts its returned key.
    Quoted `PrivateKey` values containing CRLF or spaces are accepted and a key-cache file is written.
    The node also caches returned keys with unsupported types, invalid Base64, or the wrong decoded length.
    
    Reusing a poisoned key from the cache lets CRLF frame another control command or a space add an `ADD_ONION` argument.
    3601a1d108
  37. torcontrol: discard replies missing a service ID
    A later ADD_ONION reply can reuse a service ID left by an earlier reply.
    It can also adopt and cache a returned private key despite lacking its own service ID.
    Keep both reply fields local until the existing nonempty service ID check passes.
    
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    3413d17028
  38. torcontrol: validate returned and cached keys
    A returned private key is cached and later sent as an unquoted ADD_ONION argument.
    A space can add arguments, and CRLF can frame another command on reconnect.
    
    Accept `NEW:ED25519-V3` or an `ED25519-V3` key whose Base64 blob decodes to 64 bytes.
    Reject malformed returned keys before adoption or caching, and refuse malformed cached keys before sending a command.
    Validated keys are nonempty, so an empty local key represents an omitted `PrivateKey` reply field.
    Leave a malformed cache file in place and name it in the warning so the operator can remove it.
    
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    d0b447cf7f
  39. torcontrol: reject invalid service IDs
    An ADD_ONION reply can contain a service ID with a mismatched Tor v3 checksum.
    The callback previously logged that ID, cached its returned key, and attempted to advertise the invalid address.
    
    Require a valid resolved onion address before adopting the returned key or service, logging the ID, caching the key, or advertising the service.
    A rejected reply cannot select the key sent on reconnect.
    
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    0a53d12589
  40. l0rinc force-pushed on Sep 28, 2026
  41. l0rinc closed this on Sep 28, 2026

  42. l0rinc reopened this on Sep 28, 2026

  43. DrahtBot added the label CI failed on Sep 28, 2026
  44. l0rinc commented at 3:19 AM on September 28, 2026: contributor

    Pushed, the onion response is now validated before adoption, and reconnect tests cover missing and invalid service IDs—thanks @vasild!

  45. DrahtBot removed the label CI failed on Sep 28, 2026
  46. vasild approved
  47. vasild commented at 10:15 AM on September 28, 2026: contributor

    ACK 0a53d12589c3116495a50a694006ce5ee220fb2d

    Thanks!

    <details> <summary>Show Signature</summary>

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK 0a53d12589c3116495a50a694006ce5ee220fb2d
    
    Thanks!
    -----BEGIN PGP SIGNATURE-----
    
    iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmq6PiAbFIAAAAAABAAO
    bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/PgEf/Rw/kX9jY7nOlP0gAQrG
    4UYTzMrUzBrez1DUkOge20qwk2OoRPlLzUqMvzmFFOK+MEqukrC7JuSS+WtEQAsh
    jBk9oiTx2a56xj8+beIYebBpfYzvWlOjl1mfWgQocP13+b1sxXc0aVRS0N9T5+/5
    iULjg/0xKG1l9QK0rXpKDhL7QCF1fMr+jbdLaUzmCLlCa/FofooYjfTepl5S+OnC
    029URxW9pgRZlNRHWOM4W+s0u03B4ET0sOvUMDu6lvAQ1jixzUzfqJpyh7aFYwEf
    Tnu8nwbukfpi5JwL8g9Kb2ik2RKW632YaSdNPw4NkcKn1CmSQc6h6k23cXZ1oTEc
    3B8dvbMPDC80IBbtBkUE8+I6OWemmADhhtOxrJH0UysB5Vmg8Y8jSJQNw5+r0M58
    Mc9cSWzNQteDAQwR52sRlgEfR0ZNzFiy4nLYQvbuIYBFeN4qdJ0MugJsNRhD3XSh
    jl7DYHrRVXqoSUUR0wFvv6WOnzF4uJ42w8dvkr52kjkFoeDNJ+VlGP21Y9Ls0iBo
    nhGYTHo9rdIeStvHZ6HLnoUGGhdrOC/EH+VMx04KYSQM4NWqHuE8tmN1jDGgkihm
    e/RqaSr54xzIAFjei/zlBYoQ8hIiC+tHpwsLPtP7xu00Tl+WmlolgOxkuS3Mxyr/
    rFm6yed9RkecQQcrexCMhmWvcBJvchvv1lkilA3a72JZz6SNsRR6h8Jz71PEtVP1
    WWA215yYrG/lp93YprSr6j4COlGWH4y5xTkaZyUvKMfq+KCs82IBeE07k4nSOP9b
    21TaN9kao2JN6NrIIFVicX31gSyQrZ1SN4UQ/if414gKFleHR7XKdoVRjJnAA3oV
    +4mzEBVYLyob0poOMJVQwvDeD+J340pqjYiHAH8XSa5btkEdKdUWHhs8kmjcMvFD
    I2tW719qzy1m/hKyXkGk60f8gtPhfohcE4+PEB8Vs8VSVIl6DNcg/RWnG5P6hcg7
    TZQPsqauQ3yx1S8okkfj+e8H82TZYXH0wh7tndWj7WiGpUPtIbXlzHa3sws6ay2C
    IM0A+8Z4SvPj7F7FWgauglT8dtcqVVhANAFQqUbLY5ObYrxZwyO7aG2fgjVPxN8z
    x1RSseIH1zbMmojH7qcwOZ6k3Kv3o4TpLs9sYso5y7C3vmGbHrcfnYUYSq5kXqZk
    dM0qowKVuBAYfXfcLrq9spipdevd7p5ZiUh+0or7xqHm+QTO3Sr7qY9Uazl/znzk
    GK+ByxZpRonpIoPQ3fHji7rn1q3aziPooWbQ2Ze78LhDTUO3Kw/jZvFUyN4Tqjfq
    XK4lrrjx+VphNKbqEMI9+UvI17jkwqse15EAxsk8Urd6zXZjhjo8G6vkN7oyAm0n
    PqM=
    =AEcH
    -----END PGP SIGNATURE-----
    

    vasild's public key is on openpgp.org

    </details>

  48. DrahtBot requested review from winterrdog on Sep 28, 2026
  49. in src/torcontrol.cpp:539 in 0a53d12589
     540 | +            return;
     541 | +        }
     542 | +        if (!private_key.empty()) m_private_key = std::move(private_key);
     543 | +        m_service = service;
     544 | +        LogInfo("Got tor service ID %s, advertising service %s", service_id, m_service.ToStringAddrPort());
     545 |          if (WriteBinaryFile(GetPrivateKeyFile(), m_private_key)) {
    


    winterrdog commented at 11:18 AM on September 28, 2026:

    small non-blocking nit, feel free to ignore: would it make sense to avoid caching PRIVATE_KEY_NEW, since it is a request token rather than a private key ?

    if TOR ever returns a ServiceID without a PrivateKey, we would currently save NEW:ED25519-V3 to onion_v3_private_key. a properly functional TOR should not do sth like this since we never send DiscardPK, but a broken or malicious endpoint could

    skipping the write should not change the behaviour. auth_cb() already recreates the NEW request when m_private_key is empty. it would just avoid leaving a file that looks like a cached key but isn't one, including a potentially truncated NEW:ED2... file after a crash (pragmatically less likely)

    the normal path, where TOR returns a real key, stays unchanged. we could skip the write while m_private_key is still PRIVATE_KEY_NEW and log a short factual warning that Tor returned no key

    (this is just for open discussion)


    l0rinc commented at 4:45 PM on September 28, 2026:

    I don't have strong opinions, tried renaming (see #36142 (review)), other places already use these names, we can unify in a followup.


    winterrdog commented at 9:31 AM on September 29, 2026:

    other places already use these names, we can unify in a followup.

    all right

  50. winterrdog commented at 11:20 AM on September 28, 2026: contributor

    tACK 0a53d12589c3116495a50a694006ce5ee220fb2d

    again, successfully built and tested with clang++-19 on FreeBSD 15.0, targeting x86_64

  51. DrahtBot requested review from winterrdog on Sep 28, 2026
  52. l0rinc commented at 4:49 PM on September 28, 2026: contributor

    @winterrdog, if you agree with the latest version, please update your ACK hash. If you don't please explain what you think should be changed.

  53. winterrdog commented at 9:31 AM on September 29, 2026: contributor

    if you agree with the latest version, please update your ACK hash.

    i fully agree with everything. i have updated the ACK hash too because i did not intend to use an older hash

    thank you for pointing that out


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-10-05 20:51 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me