net: Sanity check private keys received from SAM proxy

[dergoegge]

Not sanity checking can lead to crashes or worse:

==1715589==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6140000055c2 at pc 0x5622ed66e7ad bp 0x7ffee547a2c0 sp 0x7ffee547a2b8
READ of size 2 at 0x6140000055c2 thread T0 (b-test)
    #0 0x5622ed66e7ac in memcpy include/bits/string_fortified.h:29:10
    #1 0x5622ed66e7ac in i2p::sam::Session::MyDestination() const src/i2p.cpp:362:5
    #2 0x5622ed662e46 in i2p::sam::Session::CreateIfNotCreatedAlready() src/i2p.cpp:414:40
    #3 0x5622ed6619f2 in i2p::sam::Session::Listen(i2p::Connection&) src/i2p.cpp:143:9

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, stickies-v, vasild
Concept ACK	mzumsande, kristapsk

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

[dergoegge]

I would greatly appreciate it if someone could write unit tests for this, @vasild @jonatack maybe?

[mzumsande]

Concept ACK

I think that keys that are loaded from disk (ReadBinaryFile()) also should be sanitized. That's what made the fuzzer crash in #28665.

[dergoegge]

This should probably be in the 26.0 milestone?

[maflcko]

To what extent is the proxy trusted, and to what extent is it required to validate inputs from it? I think it would be good to first document that and then design unit and fuzz tests based on that design assumption?

[maflcko]

typo-nit: received

[fanquake]

To what extent is the proxy trusted, and to what extent is it required to validate inputs from it?

@jonatack @vasild @zzzi2p

[zzzi2p]

The i2pd crash in OP should be reported to that project and/or @orignal

Java I2P probably won't crash on bad input and we do some validation of public and private keys but we can't promise to catch everything.

The format is a little elaborate, I don't know how far down you want to go. Would you validate the individual key fields? Would you check for pubkey/privkey mismatches? Just basic length checks?

From the SAM spec:

The $privkey is the base 64 of the concatenation of the [Destination](https://geti2p.net/en/docs/spec/common-structures#type_Destination) 
followed by the [Private Key](https://geti2p.net/en/docs/spec/common-structures#type_PrivateKey) 
followed by the [Signing Private Key](https://geti2p.net/en/docs/spec/common-structures#type_SigningPrivateKey),
 optionally followed by the [Offline Signature](https://geti2p.net/en/docs/spec/common-structures#struct_OfflineSignature), 
which is 663 or more bytes in binary and 884 or more bytes in base 64, depending on signature type. 
The binary format is specified in [Private Key File](https://github.com/i2p/i2p.i2p/blob/master/core/java/src/net/i2p/data/PrivateKeyFile.java). 
See additional notes about the [Private Key](https://geti2p.net/en/docs/spec/common-structures#type_PrivateKey)
 in the Destination Key Generation section below.

If the signing private key is all zeros, 
the [Offline Signature](https://geti2p.net/en/docs/spec/common-structures#struct_OfflineSignature) 
section follows....

And there's more near the bottom in the 'Destination Key Generation' section of https://geti2p.net/en/docs/api/samv3

You're not using offline signatures so you can skip that part.

I can't really answer your question about "trust" in SAM. It probably won't send you bad input, and in most cases you can treat the private key as an opaque string without "validation". So as long as you don't corrupt it when you store and retrieve it, you should be fine. But it's up to you.

[zzzi2p]

Ah, correction, OP is a bitcoin crash not a i2pd crash, apologies to @orignal

[mzumsande]

To what extent is the proxy trusted, and to what extent is it required to validate inputs from it? I think it would be good to first document that and then design unit and fuzz tests based on that design assumption?

Wouldn't it be best to write error handling code / tests on the assumption that no external input source should be able to cause memory corruption within bitcoind - including trusted ones, like the datadir?

[stickies-v]

Concept ACK

I think I would prefer the constructor of Binary to do size sanity checks so we don't need to call a helper function any time we use a Binary, but it seems this doesn't pair very elegantly with the lazy construction approach of sam::Session. Below a diff of one such example implementation that passes unit tests by making m_private_key a std::optional<Binary> (using not-super-safe dereferencing on m_private_key for now just to PoC it) edit: I think vasild's approach is better

git diff on 9ff405e

diff --git a/src/i2p.cpp b/src/i2p.cpp
index 1b8345059e..fc5d23e8fb 100644
--- a/src/i2p.cpp
+++ b/src/i2p.cpp
@@ -77,19 +77,6 @@ static Binary DecodeI2PBase64(const std::string& i2p_b64)
     return std::move(*decoded);
 }
 
-static Binary SanitizePrivKey(Binary&& binary)
-{
-    if (binary.size() < 387) {
-        throw std::runtime_error("Private key too small (size < 387 bytes)");
-    }
-
-    if (binary.size() > (387 + std::numeric_limits<uint16_t>::max())) {
-        throw std::runtime_error("Private key too large");
-    }
-
-    return binary;
-}
-
 /**
  * Derive the .b32.i2p address of an I2P destination (binary).
  * @param[in] dest I2P destination.
@@ -340,7 +327,7 @@ void Session::DestGenerate(const Sock& sock)
     // If SIGNATURE_TYPE is not specified, then the default one is DSA_SHA1.
     const Reply& reply = SendRequestAndGetReply(sock, "DEST GENERATE SIGNATURE_TYPE=7", false);
 
-    m_private_key = SanitizePrivKey(DecodeI2PBase64(reply.Get("PRIV")));
+    m_private_key = DecodeI2PBase64(reply.Get("PRIV"));
 }
 
 void Session::GenerateAndSavePrivateKey(const Sock& sock)
@@ -349,7 +336,7 @@ void Session::GenerateAndSavePrivateKey(const Sock& sock)
 
     // umask is set to 0077 in common/system.cpp, which is ok.
     if (!WriteBinaryFile(m_private_key_file,
-                         std::string(m_private_key.begin(), m_private_key.end()))) {
+                         std::string(m_private_key->begin(), m_private_key->end()))) {
         throw std::runtime_error(
             strprintf("Cannot save I2P private key to %s", fs::quoted(fs::PathToString(m_private_key_file))));
     }
@@ -364,16 +351,16 @@ Binary Session::MyDestination() const
     static constexpr size_t CERT_LEN_POS = 385;
 
     uint16_t cert_len;
-    memcpy(&cert_len, &m_private_key.at(CERT_LEN_POS), sizeof(cert_len));
+    memcpy(&cert_len, &m_private_key->at(CERT_LEN_POS), sizeof(cert_len));
     cert_len = be16toh(cert_len);
 
     const size_t dest_len = DEST_LEN_BASE + cert_len;
 
-    if (dest_len > m_private_key.size()) {
+    if (dest_len > m_private_key->size()) {
         throw std::runtime_error("Certificate length did not match the actual private key length");
     }
 
-    return Binary{m_private_key.begin(), m_private_key.begin() + dest_len};
+    return Binary{m_private_key->begin(), m_private_key->begin() + dest_len};
 }
 
 void Session::CreateIfNotCreatedAlready()
@@ -399,18 +386,18 @@ void Session::CreateIfNotCreatedAlready()
                       "inbound.quantity=1 outbound.quantity=1",
                       session_id));
 
-        m_private_key = SanitizePrivKey(DecodeI2PBase64(reply.Get("DESTINATION")));
+        m_private_key = DecodeI2PBase64(reply.Get("DESTINATION"));
     } else {
         // Read our persistent destination (private key) from disk or generate
         // one and save it to disk. Then use it when creating the session.
         const auto& [read_ok, data] = ReadBinaryFile(m_private_key_file);
         if (read_ok) {
-            m_private_key = SanitizePrivKey(Binary{data.begin(), data.end()});
+            m_private_key = Binary{data.begin(), data.end()};
         } else {
             GenerateAndSavePrivateKey(*sock);
         }
 
-        const std::string& private_key_b64 = SwapBase64(EncodeBase64(m_private_key));
+        const std::string& private_key_b64 = SwapBase64(EncodeBase64(m_private_key.value()));
 
         SendRequestAndGetReply(*sock,
                                strprintf("SESSION CREATE STYLE=STREAM ID=%s DESTINATION=%s "
diff --git a/src/i2p.h b/src/i2p.h
index cb9da64816..e5bcef53e5 100644
--- a/src/i2p.h
+++ b/src/i2p.h
@@ -23,7 +23,31 @@ namespace i2p {
 /**
  * Binary data.
  */
-using Binary = std::vector<uint8_t>;
+class Binary {
+private:
+    std::vector<uint8_t> m_data;
+    void Validate() const {
+        if (m_data.size() < 387) {
+            throw std::runtime_error("Private key too small (size < 387 bytes)");
+        } else if (m_data.size() > (387 + std::numeric_limits<uint16_t>::max())) {
+            throw std::runtime_error("Private key too large");
+        }
+    }
+
+public:
+    template<typename... Args>
+    Binary(Args&&... args) : m_data(std::forward<Args>(args)...) { Validate(); }
+
+    operator Span<const uint8_t>() const { return m_data; }
+
+    // forward to std::vector methods
+    template<typename... Args>
+    auto& at(Args&&... args) const { return m_data.at(std::forward<Args>(args)...); }
+    auto begin() const { return m_data.begin(); }
+    auto data() const { return m_data.data(); }
+    auto end() const { return m_data.end(); }
+    auto size() const { return m_data.size(); }
+};
 
 /**
  * An established connection with another peer.
@@ -253,7 +277,7 @@ private:
      * The private key of this peer.
      * @see The reply to the "DEST GENERATE" command in https://geti2p.net/en/docs/api/samv3
      */
-    Binary m_private_key GUARDED_BY(m_mutex);
+    std::optional<Binary> m_private_key GUARDED_BY(m_mutex);
 
     /**
      * SAM control socket.

[maflcko]

In any case, I don't think this is a blocker for rc1. It is not a regression and this diff can be added to rc2 trivially.

[vasild]

Concept ACK

[stickies-v]

ACK adb5d6b

[vasild]

ACK adb5d6b

[kristapsk]

Concept ACK

[maflcko]

code lgtm ACK 5cf4d26

[stickies-v]

re-ACK 5cf4d26

[vasild]

ACK 5cf4d26

[fanquake]

Backported to 26.x in #28754.