Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

jonasschnelli commented at 1:07 PM on March 2, 2020: contributor

BIP324 is here (not submitted to the BIP repository since it's still under work).

This PR adds a message transport de-/serializer for encrypted message after BIP324.

Includes:

correct AEAD handling
short command IDs

Excludes (to keep it reviewable):

The handshake (pubkey exchange [subject to change due to downgrade attack prevention])
ECDH (enabling libsecp256k1 & CKey implementation)
service flag and a way to globally enable it

The code is exclusively used in the tests

This is based on #20962 (please review first)

jonasschnelli added the label P2P on Mar 2, 2020

jonasschnelli force-pushed on Mar 2, 2020

DrahtBot commented at 4:41 PM on March 2, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21000 (fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer by practicalswift)
#20685 (Add I2P support using I2P SAM by vasild)
#20429 (refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size by theStack)
#20196 (net: fix GetListenPort() to derive the proper port by vasild)
#19064 (refactor: Cleanup thread ctor calls by hebasto)

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.

hebasto commented at 10:54 PM on March 2, 2020: member

Concept ACK.

practicalswift commented at 11:22 PM on March 2, 2020: contributor

Concept:

Concept ACK - thanks for working on this!

Implementation:

Some comments after first read-through of the implementation:

1. Uninitialized read in case of invalid command name

In the "Invalid command name" case then a read (and use) of the uninitialized variable size_or_shortid will take place on L808:

https://github.com/bitcoin/bitcoin/blob/73db845771628983c7f288ead73cd3a8793f8c6a/src/net.cpp#L802-L808

Shameless plug: For those interested in killing the uninitialised read bug class, consider reviewing:

#17639 – tests: Add make check-valgrind to run the unit tests under Valgrind
#17895 – build: Enable Clang's -Wconditional-uninitialized to warn on potential uninitialized reads

2. Use of a locale dependent formatting function itostr(…)

itostr is used here:

https://github.com/bitcoin/bitcoin/blob/73db845771628983c7f288ead73cd3a8793f8c6a/src/net.cpp#L819-L823

itostr calls strprintf which calls tfm::format (tinyformat) which uses the standard C++ formatting stringstream interface which is locale dependent.

Shameless plug: For those interested in permanently killing the locale dependency bug class, consider reviewing:

#18124 – init: Clarify C and C++ locale assumptions. Add locale sanity checks to verify assumptions at run-time
#18126 – tests: Add fuzzing harness testing the locale independence of the strencodings.h functions
#18147 – qt: Kill the locale dependency bug class by not allowing Qt to mess with LC_NUMERIC

3. std::exception vs expected std::ios_base::failure

A question: I notice that std::exception is guarded against instead of the expected std::ios_base::failure in case of deserialization errors throughout this PR. Is it intentional? :)

It is somewhat related to another deserialization-exception anomaly I mailed you about back in October last year:

When fuzzing some deserialization code I noticed that `CExtKey::Unserialize(...)`
and `CExtPubKey::(...)` throw `std::runtime_error` instead of the
`std::ios_base::failure` I expected in case of deserialization errors.

I saw that this code was written by you originally: do you remember if there
was a particular reason to go with `std::runtime_error` instead of
`std::ios_base::failure`? If not, do you think it would be safe to change it? :)

The commits in question:
* https://github.com/bitcoin/bitcoin/commit/07685d1bc1b0b815c00a68a5b7b335ffa0d4d90d
* https://github.com/bitcoin/bitcoin/commit/90604f16af63ec066d6561337f476ccd8acec326

These deviations from the expectedstd::ios_base::failure puzzle me - are they intentional, and if so why? I want to learn :)

practicalswift commented at 11:36 PM on March 2, 2020: contributor

Very nice to see that the V2TransportDeserializer is fuzzed already from birth! I hope that fuzz testing will be as natural as unit testing when introducing security critical code in the future. Kudos for taking care of it here!

A small comment regarding the fuzzer:

I think the assertion …

assert(msg.m_raw_message_size == CMessageHeader::HEADER_SIZE + msg.m_message_size);

… in the fuzzing harness is guaranteed to hold for V1TransportDeserializer but not for V2TransportDeserializer.

Could that be the case? :)

practicalswift commented at 11:09 AM on March 21, 2020: contributor

Add "Waiting for author" tag? :)

jonasschnelli force-pushed on Mar 27, 2020

jonasschnelli commented at 9:04 AM on March 27, 2020: contributor

Thanks @practicalswift for the review. I tried to fix the exception handling as well as uninitialised read. I also fixed the invalid fuzzing assertion (for V2).

I'm unsure about the locale dependent formatting. What would you recommend instead of itotsr?

practicalswift commented at 2:08 PM on March 27, 2020: contributor

@jonasschnelli

I'm unsure about the locale dependent formatting. What would you recommend instead of itotsr?

I recommend ToString(…) (util/string.h) :)

jonasschnelli force-pushed on Mar 27, 2020

in src/protocol.cpp:232 in 0e9b029fdc outdated

 198 | @@ -199,3 +199,116 @@ const std::vector<std::string> &getAllNetMessageTypes()
 199 |  {
 200 |      return allNetMessageTypesVec;
 201 |  }
 202 | +
 203 | +

PastaPastaPasta commented at 12:38 AM on April 2, 2020:

Nit remove extra newline

in src/test/net_tests.cpp:325 in 500e268eb1 outdated

 320 | @@ -319,4 +321,73 @@ BOOST_AUTO_TEST_CASE(PoissonNextSend)
 321 |      g_mock_deterministic_tests = false;
 322 |  }
 323 |  
 324 | +void message_serialize_deserialize_test(bool v2, const std::vector<CSerializedNetMsg>& test_msgs) {
 325 | +    // use 32 bytey keys with all zeros

PastaPastaPasta commented at 12:39 AM on April 2, 2020:

    // use 32 byte keys with all zeros

in src/test/net_tests.cpp:337 in 500e268eb1 outdated

 332 | +
 333 | +    if (v2) {
 334 | +        serializer = MakeUnique<V2TransportSerializer>(V2TransportSerializer(k1, k2));
 335 | +        deserializer = MakeUnique<V2TransportDeserializer>(V2TransportDeserializer(Params().MessageStart(), k1, k2));
 336 | +    }
 337 | +    else {

PastaPastaPasta commented at 12:43 AM on April 2, 2020:

I could be wrong but style guide seems to say these should be on the same line https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

    } else {

in src/test/net_tests.cpp:342 in 500e268eb1 outdated

 337 | +    else {
 338 | +        serializer = MakeUnique<V1TransportSerializer>(V1TransportSerializer());
 339 | +        deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION));
 340 | +    }
 341 | +    // run a couple of times through all messages with the same AEAD instance
 342 | +    for (unsigned int i=0; i<100;i++) {

PastaPastaPasta commented at 12:43 AM on April 2, 2020:

Proper social distancing should be followed and pre-inc is preferred.

    for (unsigned int i = 0; i < 100; ++i) {

in src/test/net_tests.cpp:963 in 500e268eb1 outdated

 349 | +
 350 | +            std::vector<unsigned char> serialized_header;
 351 | +            serializer->prepareForTransport(msg, serialized_header);
 352 | +
 353 | +            // read two times
 354 | +            //  first: read header

PastaPastaPasta commented at 12:44 AM on April 2, 2020:

Not sure if this was intended or not, but if it was intended I don't understand why

            // first: read header

in src/test/net_tests.cpp:967 in 500e268eb1 outdated

 352 | +
 353 | +            // read two times
 354 | +            //  first: read header
 355 | +            size_t read_bytes = 0;
 356 | +            if (serialized_header.size() > 0) read_bytes += deserializer->Read((const char *)serialized_header.data(), serialized_header.size());
 357 | +            //  second: read the encrypted payload (if required)

PastaPastaPasta commented at 12:44 AM on April 2, 2020:

            // second: read the encrypted payload (if required)

in src/test/net_tests.cpp:380 in 500e268eb1 outdated

 375 | +    test_msgs.push_back(CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, (int)NODE_NETWORK, 123, CAddress(CService(), NODE_NONE), CAddress(CService(), NODE_NONE), 123, "foobar", 500000, true));
 376 | +    test_msgs.push_back(CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::PING, 123456));
 377 | +    CDataStream stream(ParseHex("020000000001013107ca31e1950a9b44b75ce3e8f30127e4d823ed8add1263a1cc8adcc8e49164000000001716001487835ecf51ea0351ef266d216a7e7a3e74b84b4efeffffff02082268590000000017a9144a94391b99e672b03f56d3f60800ef28bc304c4f8700ca9a3b0000000017a9146d5df9e79f752e3c53fc468db89cafda4f7d00cb87024730440220677de5b11a5617d541ba06a1fa5921ab6b4509f8028b23f18ab8c01c5eb1fcfb02202fe382e6e87653f60ff157aeb3a18fc888736720f27ced546b0b77431edabdb0012102608c772598e9645933a86bcd662a3b939e02fb3e77966c9713db5648d5ba8a0006010000"), SER_NETWORK, PROTOCOL_VERSION);
 378 | +    test_msgs.push_back(CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::TX, CTransaction(deserialize, stream)));
 379 | +    std::vector<CInv> vInv;
 380 | +    for (unsigned int i=0;i<1000;i++) { vInv.push_back(CInv(MSG_BLOCK, Params().GenesisBlock().GetHash())); }

PastaPastaPasta commented at 12:46 AM on April 2, 2020:

    for (unsigned int i = 0;i < 1000; ++i) { 
        vInv.push_back(CInv(MSG_BLOCK, Params().GenesisBlock().GetHash()));
    }

in src/test/net_tests.cpp:385 in 500e268eb1 outdated

 380 | +    for (unsigned int i=0;i<1000;i++) { vInv.push_back(CInv(MSG_BLOCK, Params().GenesisBlock().GetHash())); }
 381 | +    test_msgs.push_back(CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::INV, vInv));
 382 | +
 383 | +    // add a dummy message
 384 | +    std::string dummy;
 385 | +    for (unsigned int i=0;i<100;i++) { dummy += "020000000001013107ca31e1950a9b44b75ce3e8f30127e4d823ed8add1263a1cc8adcc8e49164000000001716001487835ecf51ea0351ef266d216a7e7a3e74b84b4efeffffff02082268590000000017a9144a94391b99e672b03f56d3f60800ef28bc304c4f8700ca9a3b0000000017a9146d5df9e79f752e3c53fc468db89cafda4f7d00cb87024730440220677de5b11a5617d541ba06a1fa5921ab6b4509f8028b23f18ab8c01c5eb1fcfb02202fe382e6e87653f60ff157aeb3a18fc888736720f27ced546b0b77431edabdb0012102608c772598e9645933a86bcd662a3b939e02fb3e77966c9713db5648d5ba8a0006010000"; }

PastaPastaPasta commented at 12:47 AM on April 2, 2020:

    for (unsigned int i = 0; i < 100; ++i) { 
        dummy += "020000000001013107ca31e1950a9b44b75ce3e8f30127e4d823ed8add1263a1cc8adcc8e49164000000001716001487835ecf51ea0351ef266d216a7e7a3e74b84b4efeffffff02082268590000000017a9144a94391b99e672b03f56d3f60800ef28bc304c4f8700ca9a3b0000000017a9146d5df9e79f752e3c53fc468db89cafda4f7d00cb87024730440220677de5b11a5617d541ba06a1fa5921ab6b4509f8028b23f18ab8c01c5eb1fcfb02202fe382e6e87653f60ff157aeb3a18fc888736720f27ced546b0b77431edabdb0012102608c772598e9645933a86bcd662a3b939e02fb3e77966c9713db5648d5ba8a0006010000";
    }

in src/net.cpp:865 in b2397b06c0 outdated

 860 | +        m_aad_seqnr = 0;
 861 | +        m_aad_pos = 0;
 862 | +        m_bytes_decrypted = 0;
 863 | +        m_time_last_rekey = GetTime();
 864 | +    }
 865 | +    else if (m_bytes_decrypted > REKEY_ABORT_LIMIT_BYTES || GetTime() - m_time_last_rekey > REKEY_ABORT_LIMIT_TIME ||

PastaPastaPasta commented at 12:49 AM on April 2, 2020:

maybe

    } else if (m_bytes_decrypted > REKEY_ABORT_LIMIT_BYTES || GetTime() - m_time_last_rekey > REKEY_ABORT_LIMIT_TIME ||

in src/test/net_tests.cpp:415 in b2397b06c0 outdated

 410 | +    std::unique_ptr<TransportSerializer> serializer = MakeUnique<V2TransportSerializer>(V2TransportSerializer(k1, k2, session_id));;
 411 | +    std::unique_ptr<TransportDeserializer> deserializer = MakeUnique<V2TransportDeserializer>(V2TransportDeserializer(Params().MessageStart(), k1, k2, session_id));
 412 | +
 413 | +    ChaCha20Poly1305AEAD test_decryption_aead(k1.data(), k1.size(), k2.data(), k2.size());
 414 | +
 415 | +    for (unsigned int i=0; i<=76;i++) {

PastaPastaPasta commented at 12:50 AM on April 2, 2020:

    for (unsigned int i = 0; i <= 76; ++i) {

in src/test/net_tests.cpp:436 in b2397b06c0 outdated

 431 | +    // now make sure we are rekeying by checking against a manual aead instance
 432 | +    serializer.reset(new V2TransportSerializer(mutable_k1, mutable_k2, mutable_session_id));
 433 | +    deserializer.reset(new V2TransportDeserializer(Params().MessageStart(), mutable_k1, mutable_k2, mutable_session_id));
 434 | +    uint32_t aad_seqnr = 0;
 435 | +    uint32_t aad_pos = 0;
 436 | +    for (unsigned int i=0; i<=100;i++) {

PastaPastaPasta commented at 12:51 AM on April 2, 2020:

    for (unsigned int i = 0; i <= 100; ++i) {

in src/test/fuzz/p2p_transport_deserializer.cpp:21 in 818b19787a outdated

  17 | @@ -17,24 +18,22 @@ void initialize()
  18 |      SelectParams(CBaseChainParams::REGTEST);
  19 |  }
  20 |  
  21 | -void test_one_input(const std::vector<uint8_t>& buffer)
  22 | -{
  23 | -    V1TransportDeserializer deserializer{Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION};
  24 | +void test_deserializer(std::unique_ptr<TransportDeserializer>& deserializer, const std::vector<uint8_t>& buffer, const int header_size) {

PastaPastaPasta commented at 12:51 AM on April 2, 2020:

void test_deserializer(std::unique_ptr<TransportDeserializer>& deserializer, const std::vector<uint8_t>& buffer, const int header_size) 
{

PastaPastaPasta changes_requested

PastaPastaPasta commented at 12:54 AM on April 2, 2020: contributor

Very glad to see BIP324 still moving forward! Concept ACK, I have some formatting suggestions, otherwise I didn't see any problems

practicalswift commented at 4:32 AM on April 2, 2020: contributor

@PastaPastaPasta Worth mentioning for future reviews: we use clang-format in the project so the 11 specific whitespace review comments could be simplified to a one general review comment "Nit: Please use clang-format-diff.py on this diff" :)

jonasschnelli force-pushed on Apr 2, 2020

jonasschnelli commented at 7:23 PM on April 2, 2020: contributor

Thanks @PastaPastaPasta and @practicalswift. Applied clang-format now.

jonasschnelli force-pushed on Apr 8, 2020

in src/net.cpp:726 in 129545bec6 outdated

 718 | @@ -718,6 +719,163 @@ CNetMessage V1TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
 719 |      return msg;
 720 |  }
 721 |  
 722 | +int V2TransportDeserializer::Read(const char* pch, unsigned int bytes)
 723 | +{
 724 | +    if (!m_in_data) {
 725 | +        // copy data to temporary parsing buffer
 726 | +        unsigned int remaining = CHACHA20_POLY1305_AEAD_AAD_LEN - m_hdr_pos;

hebasto commented at 10:30 PM on April 12, 2020:

nit:

        const unsigned int remaining = CHACHA20_POLY1305_AEAD_AAD_LEN - m_hdr_pos;

in src/net.cpp:727 in 129545bec6 outdated

 718 | @@ -718,6 +719,163 @@ CNetMessage V1TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
 719 |      return msg;
 720 |  }
 721 |  
 722 | +int V2TransportDeserializer::Read(const char* pch, unsigned int bytes)
 723 | +{
 724 | +    if (!m_in_data) {
 725 | +        // copy data to temporary parsing buffer
 726 | +        unsigned int remaining = CHACHA20_POLY1305_AEAD_AAD_LEN - m_hdr_pos;
 727 | +        unsigned int copy_bytes = std::min(remaining, bytes);

hebasto commented at 10:34 PM on April 12, 2020:

nit:

        const unsigned int copy_bytes = std::min(remaining, bytes);

Also #include <algorithm> header for std::min().

in src/net.cpp:746 in 129545bec6 outdated

 741 | +        }
 742 | +
 743 | +        // check and unset rekey bit
 744 | +        // the counterparty can signal a post-this-message rekey by setting the
 745 | +        // most significant bit in the (unencrypted) length
 746 | +        m_rekey_flag = (m_message_size & (1U << 23));

hebasto commented at 10:39 PM on April 12, 2020:

An explicit type conversion could improve readability:

        m_rekey_flag = static_cast<bool>(m_message_size & (1U << 23));

in src/net.cpp:748 in 129545bec6 outdated

 743 | +        // check and unset rekey bit
 744 | +        // the counterparty can signal a post-this-message rekey by setting the
 745 | +        // most significant bit in the (unencrypted) length
 746 | +        m_rekey_flag = (m_message_size & (1U << 23));
 747 | +        if (m_rekey_flag) {
 748 | +            LogPrint(BCLog::NET, "Rekey flag detected %ld\n", m_message_size);

hebasto commented at 10:41 PM on April 12, 2020:

What is the purpose of m_message_size in the log message?

jonasschnelli commented at 9:38 AM on April 14, 2020:

It was initially to debug the most significant bit. I'll remove it.

in src/net.cpp:764 in 129545bec6 outdated

 759 | +
 760 | +        return copy_bytes;
 761 | +    } else {
 762 | +        const unsigned int AAD_LEN = CHACHA20_POLY1305_AEAD_AAD_LEN;
 763 | +        unsigned int remaining = m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN - m_data_pos;
 764 | +        unsigned int copy_bytes = std::min(remaining, bytes);

hebasto commented at 10:44 PM on April 12, 2020:

nit:

        const unsigned int remaining = m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN - m_data_pos;
        const unsigned int copy_bytes = std::min(remaining, bytes);

in src/net.cpp:772 in 129545bec6 outdated

 767 | +        if (vRecv.size() < CHACHA20_POLY1305_AEAD_AAD_LEN + m_data_pos + copy_bytes) {
 768 | +            // Allocate up to 256 KiB ahead, but never more than the total message size (incl. AAD & TAG).
 769 | +            vRecv.resize(std::min(CHACHA20_POLY1305_AEAD_AAD_LEN + m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN, CHACHA20_POLY1305_AEAD_AAD_LEN + m_data_pos + copy_bytes + 256 * 1024 + CHACHA20_POLY1305_AEAD_TAG_LEN));
 770 | +        }
 771 | +
 772 | +        memcpy(&vRecv[AAD_LEN + m_data_pos], pch, copy_bytes);

hebasto commented at 10:46 PM on April 12, 2020:

        memcpy(&vRecv[CHACHA20_POLY1305_AEAD_AAD_LEN + m_data_pos], pch, copy_bytes);

... and drop the line 762:

       const unsigned int AAD_LEN = CHACHA20_POLY1305_AEAD_AAD_LEN;

in src/net.cpp:859 in 129545bec6 outdated

 820 | +            if (!GetCommandFromShortCommandID(size_or_shortid, command_name)) {
 821 | +                // unknown-short-id
 822 | +                //  results in a valid but unknown message (will be skipped)
 823 | +                command_name = "unknown-" + ToString(size_or_shortid);
 824 | +            }
 825 | +        }

hebasto commented at 10:50 PM on April 12, 2020:

What if size_or_shortid is still == 0 ?

jonasschnelli commented at 9:57 AM on April 14, 2020:

Good catch. Add 0 now to the unknown branch.

in src/net.cpp:903 in 129545bec6 outdated

 888 | @@ -731,6 +889,97 @@ void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vec
 889 |      CVectorWriter{SER_NETWORK, INIT_PROTO_VERSION, header, 0, hdr};
 890 |  }
 891 |  
 892 | +void V2TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header)
 893 | +{
 894 | +    size_t serialized_command_size = 1; // short-IDs are 1 byte
 895 | +    uint8_t cmd_short_id = GetShortCommandIDFromCommand(msg.command);

hebasto commented at 10:56 PM on April 12, 2020:

nit:

    const uint8_t cmd_short_id = GetShortCommandIDFromCommand(msg.command);

in src/net.cpp:917 in 129545bec6 outdated

 904 | +    uint32_t packet_length = serialized_command_size + msg.data.size();
 905 | +
 906 | +    // prepare the packet length & message command
 907 | +    std::vector<unsigned char> serialized_header;
 908 | +    // reserve 4 bytes (3bytes AD + 1byte short-ID)
 909 | +    serialized_header.resize(CHACHA20_POLY1305_AEAD_AAD_LEN + 1);

hebasto commented at 11:00 PM on April 12, 2020:

Could be combined:

    std::vector<unsigned char> serialized_header(CHACHA20_POLY1305_AEAD_AAD_LEN + 1);

in src/net.cpp:923 in 129545bec6 outdated

 910 | +    // LE serialize the 24bits length
 911 | +    // we do "manually" encode this since there is no helper for 24bit serialization
 912 | +    packet_length = htole32(packet_length);
 913 | +    memcpy(serialized_header.data(), &packet_length, 3);
 914 | +
 915 | +    // append the short-ID (or eventually) the varstr of the command

hebasto commented at 11:01 PM on April 12, 2020:

    // append the short-ID or (eventually) the varstr of the command

in src/net.cpp:934 in 129545bec6 outdated

 921 | +        // or the ASCII command string
 922 | +        vector_writer << msg.command;
 923 | +    }
 924 | +
 925 | +    // insert header directly into the CSerializedNetMsg data buffer (insert at begin)
 926 | +    // TODO: if we refactor the ChaCha20Poly1350 crypt function to allow sepearte buffers for

hebasto commented at 11:02 PM on April 12, 2020:

typo:

    // TODO: if we refactor the ChaCha20Poly1350 crypt function to allow separate buffers for

in src/net.cpp:946 in 129545bec6 outdated

 933 | +    // length is only allowed up to 2^23 (bit24 is used for indicating rekey)
 934 | +    bool bit24 = msg.data[2] & (1u << 7);
 935 | +    assert(!bit24);
 936 | +
 937 | +    // check if we should rekey after this message
 938 | +    int64_t now = GetTime(); //TODO: check how expansive the GetTime call is and if it is avoidable

hebasto commented at 11:06 PM on April 12, 2020:

nit:

    const int64_t now = GetTime(); //TODO: check how expansive the GetTime call is and if it is avoidable

in src/net.cpp:1021 in 129545bec6 outdated

 934 | +    bool bit24 = msg.data[2] & (1u << 7);
 935 | +    assert(!bit24);
 936 | +
 937 | +    // check if we should rekey after this message
 938 | +    int64_t now = GetTime(); //TODO: check how expansive the GetTime call is and if it is avoidable
 939 | +    bool rekey = false;

hebasto commented at 11:15 PM on April 12, 2020:

Actually, bit24 is equivalent to rekey. Maybe drop bit24 in lines 934-935 and:

    bool rekey = msg.data[2] & (1U << 7);
    assert(!rekey);

jonasschnelli commented at 10:05 AM on April 14, 2020:

Let's keep it for readability.

in src/net.cpp:939 in 129545bec6 outdated

 926 | +    // TODO: if we refactor the ChaCha20Poly1350 crypt function to allow sepearte buffers for
 927 | +    //       the AD, payload and MAC, we could avoid a insert and thus a potential reallocation
 928 | +    msg.data.insert(msg.data.begin(), serialized_header.begin(), serialized_header.end());
 929 | +
 930 | +    // resize the message buffer to make space for the MAC tag
 931 | +    msg.data.resize(msg.data.size() + 16, 0);

hebasto commented at 11:19 PM on April 12, 2020:

Mind using a named constant:

    msg.data.resize(msg.data.size() + CHACHA20_POLY1305_AEAD_TAG_LEN, 0);

?

in src/net.cpp:955 in 129545bec6 outdated

 942 | +        msg.data[2] |= (1u << 7);
 943 | +        rekey = true;
 944 | +    }
 945 | +
 946 | +    // encrypt the payload, ignore return code since it can't fail in this case (controlled buffers, don't check the MAC during encrypting)
 947 | +    m_aead->Crypt(m_payload_seqnr, m_aad_seqnr, m_aad_pos, msg.data.data(), msg.data.size(), msg.data.data(), msg.data.size() - 16, true);

hebasto commented at 11:22 PM on April 12, 2020:

Mind using a named constant:

    m_aead->Crypt(m_payload_seqnr, m_aad_seqnr, m_aad_pos, msg.data.data(), msg.data.size(), msg.data.data(), msg.data.size() - CHACHA20_POLY1305_AEAD_TAG_LEN, true);

?

in src/net.h:737 in 129545bec6 outdated

 732 | +    unsigned int m_hdr_pos;   // read pos in header
 733 | +    unsigned int m_data_pos;  // read pos in data
 734 | +    bool m_rekey_flag;        // rekey in message detected
 735 | +
 736 | +public:
 737 | +    V2TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, const CPrivKey& k1, const CPrivKey& k2, const uint256& session_id) : m_aead(new ChaCha20Poly1305AEAD(k1.data(), k1.size(), k2.data(), k2.size())), m_aead_k1(k1), m_aead_k2(k2), m_bytes_decrypted(0), m_time_last_rekey(GetTime()), m_session_id(session_id), m_payload_seqnr(0), m_aad_seqnr(0), m_aad_pos(0), vRecv(SER_NETWORK, INIT_PROTO_VERSION)

hebasto commented at 11:24 PM on April 12, 2020:

Mind moving the member initializer list to next line to improve readability?

in src/net.h:865 in 129545bec6 outdated

 750 | +        m_rekey_flag = false;
 751 | +    }
 752 | +    bool Complete() const
 753 | +    {
 754 | +        if (!m_in_data)
 755 | +            return false;

hebasto commented at 11:27 PM on April 12, 2020:

style nit: could be placed into one line or in braces.

in src/net.h:916 in 129545bec6 outdated

 793 | +    uint64_t m_payload_seqnr = 0;
 794 | +    uint64_t m_aad_seqnr = 0;
 795 | +    int m_aad_pos = 0;
 796 | +
 797 | +public:
 798 | +    V2TransportSerializer(const CPrivKey& k1, const CPrivKey& k2, const uint256& session_id) : m_aead(new ChaCha20Poly1305AEAD(k1.data(), k1.size(), k2.data(), k2.size())), m_aead_k1(k1), m_aead_k2(k2), m_session_id(session_id)

hebasto commented at 11:28 PM on April 12, 2020:

Mind moving the member initializer list to next line to improve readability?

jonatack commented at 6:23 PM on April 30, 2020:

Agree this would be nice.

in src/protocol.cpp:253 in 129545bec6 outdated

 198 | @@ -199,3 +199,127 @@ const std::vector<std::string> &getAllNetMessageTypes()
 199 |  {
 200 |      return allNetMessageTypesVec;
 201 |  }
 202 | +
 203 | +
 204 | +uint8_t GetShortCommandIDFromCommand(const std::string cmd)
 205 | +{
 206 | +    if (cmd == NetMsgType::ADDR) {

hebasto commented at 11:30 PM on April 12, 2020:

Have you consider a switch statement as an alternative?

in src/protocol.cpp:318 in 129545bec6 outdated

 266 | +    return 0; //no short command
 267 | +}
 268 | +
 269 | +bool GetCommandFromShortCommandID(uint8_t shortID, std::string& cmd)
 270 | +{
 271 | +    if (shortID == NetMsgType::ADDR_SHORT_ID) {

hebasto commented at 11:31 PM on April 12, 2020:

Have you consider a switch statement as an alternative?

jonasschnelli commented at 9:37 AM on April 14, 2020:

Can you elaborate on the differences? Aren't the compiler differences marginal?

ariard commented at 6:42 AM on April 30, 2020:

Alternatively have you considered a translation table, defining a struct

struct {
           std::string  long_cmd;
           std::string  short_cmd;
} short_id_table

You statically declare an array of such structs, with one element for every pair. And GetCommandFromShortCommandID just iter through array until funding a match.

That way in the future we may have a parser to load a new table (bitcoin-cli loadv2mappings) to enable custom peer-agreement mappings without code modifications.

jonatack commented at 3:53 PM on April 30, 2020:

Can you elaborate on the differences? Aren't the compiler differences marginal?

It does seem like a switch statement might be a good alternative to the if...else ifs here, to make explicit the nature of the operation (testing a single value against a set of scoped enumerations or array of values) and possibly generate better code with a jump table instead of repeatedly checking individual values.

A translation table could be an alternative as well.

elichai commented at 9:33 AM on May 3, 2020:

I agree that a switch statement is easier to read(and see what's missing if we'll have more NetMsgType types)

jonasschnelli commented at 3:45 PM on May 6, 2020:

Seems to be a matter of taste. I'd like to keep it as is.

laanwj commented at 2:13 PM on June 18, 2020:

I think the advantage of a (hash)table is that it could be built in one central function, making the mapping in both directions, which leaves only one place to update for new message types (and making sure they never go out of sync).

in src/protocol.h:74 in 129545bec6 outdated

  70 | @@ -71,99 +71,115 @@ namespace NetMsgType {
  71 |   * @see https://bitcoin.org/en/developer-reference#version
  72 |   */
  73 |  extern const char *VERSION;
  74 | +const uint8_t VERSION_SHORT_ID = 38;

hebasto commented at 11:33 PM on April 12, 2020:

nit: hereinafter

constexpr uint8_t VERSION_SHORT_ID = 38;

in src/net.h:834 in 129545bec6 outdated

 710 | +// Re-key after 1GB (RFC4253 / SSH recommendation) or after 1h
 711 | +static constexpr unsigned int REKEY_LIMIT_BYTES = (1024 * 1024 * 1024);
 712 | +static constexpr unsigned int REKEY_LIMIT_TIME = 3600;
 713 | +static constexpr unsigned int REKEY_ABORT_LIMIT_BYTES = REKEY_LIMIT_BYTES * 1.1; // abort after ~10% tolerance buffer
 714 | +static constexpr unsigned int REKEY_ABORT_LIMIT_TIME = REKEY_LIMIT_BYTES * 1.1;  // abort after ~10% tolerance buffer
 715 | +static constexpr unsigned int MIN_REKEY_TIME = 10;                               // minimal rekey time to avoid DOS

hebasto commented at 11:38 PM on April 12, 2020:

MIN_REKEY_TIME is unused now. Is it added intended on purpose in the future?

jonasschnelli commented at 9:36 AM on April 14, 2020:

Thanks. I added the check for not violating the MIN_REKEY_TIME.

hebasto commented at 11:48 PM on April 12, 2020: member

129545bec6288ea4900ffa3284347e1e677bdf6a compiled and unit tests passed locally.

May I suggest to adapt the BIP324 and this PR to the fact that REJECT command has been removed from the Bitcoin Core code (#15437, #17004, #18609), i.e., shift command codes 34..38 -> 33..37?
Rekeying code seems duplicated, therefore refactoring could be a good followup.
In 786ec9bffc09c73d5b94e5056e7396adc0c2eb3d mind not adding the following lines:

    int readHeader(const char* pch, unsigned int nBytes);
    int readData(const char* pch, unsigned int nBytes);

that are removed in 1b2f52af4f85910673ac43be1568e3ad32e878ee ?

in src/test/net_tests.cpp:514 in 1b2f52af4f outdated

 429 | +        read_message(deserializer, serialized_header, test_msg);
 430 | +        CNetMessage msg_deser = deserializer->GetMessage(Params().MessageStart(), GetTimeMicros());
 431 | +
 432 | +        // make sure we detect the failed rekey
 433 | +        // the 76. message (32kb) must have violated the fast rekey limits
 434 | +        BOOST_CHECK(msg_deser.m_valid_header == (i != 76));

dongcarl commented at 8:54 PM on April 13, 2020:

Could you explain a bit what you envision the -netencryptionfastrekey flag to do? Is it a command line flag for testing purposes?

in src/net.h:725 in 786ec9bffc outdated

 720 | +
 721 | +    int readHeader(const char* pch, unsigned int nBytes);
 722 | +    int readData(const char* pch, unsigned int nBytes);
 723 | +
 724 | +public:
 725 | +    V2TransportDeserializer(const CMessageHeader::MessageStartChars& pchMessageStartIn, const CPrivKey& k1, const CPrivKey& k2) : m_aead(new ChaCha20Poly1305AEAD(k1.data(), k1.size(), k2.data(), k2.size())), m_payload_seqnr(0), m_aad_seqnr(0), m_aad_pos(0), vRecv(SER_NETWORK, INIT_PROTO_VERSION)

dongcarl commented at 9:16 PM on April 13, 2020:

Perhaps I'm mistaken, but it seems like this pchMessageStartIn is not needed. I tried compiling with it removed and that seemed to work.

in src/net.h:719 in 786ec9bffc outdated

 714 | +    int m_aad_pos;            // position in the aad keystream
 715 | +    bool m_in_data;           // parsing header (false) or data (true)
 716 | +    uint32_t m_message_size;  // expected message size
 717 | +    CDataStream vRecv;        // received message data
 718 | +    unsigned int m_hdr_pos;   // read pos in header
 719 | +    unsigned int m_data_pos;  // read pos in data

dongcarl commented at 9:23 PM on April 13, 2020:

Perhaps we could use in-class member initializers to de-clutter the constructor signature

in src/protocol.cpp:249 in 129545bec6 outdated

 264 | +        ;
 265 | +    }
 266 | +    return 0; //no short command
 267 | +}
 268 | +
 269 | +bool GetCommandFromShortCommandID(uint8_t shortID, std::string& cmd)

MarcoFalke commented at 9:48 PM on April 13, 2020:

Please replace occurrences of "command" with something like "msg_type" at least in new code. See also #18533

jonasschnelli commented at 8:37 AM on April 14, 2020:

I'd like to keep it to be consistent with BIP324 (and other bips). Changing it might make it more confusing.

laanwj commented at 2:11 PM on June 18, 2020:

I see @MarcoFalke's point. A P2P protocol has no commands, just messages and message ids. But yea it's unfortunate that was not caught in the BIP.

jonasschnelli force-pushed on Apr 14, 2020

jonasschnelli commented at 10:11 AM on April 14, 2020: contributor

Thanks @hebasto, @dongcarl and @MarcoFalke for the review. I tried to address all the points. You'r invited to go again through the PR for further findings.

laanwj added this to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Apr 17, 2020

jonasschnelli force-pushed on Apr 17, 2020

jonasschnelli commented at 3:52 PM on April 17, 2020: contributor

Rebased.

DrahtBot removed the label Needs rebase on Apr 17, 2020

in src/test/fuzz/p2p_transport_deserializer.cpp:56 in f9d3052f98 outdated

  52 | +    const CPrivKey k1(32, 0);
  53 | +    const CPrivKey k2(32, 0);
  54 | +    const uint256 session_id;
  55 | +    std::unique_ptr<TransportDeserializer> v2_deserializer = MakeUnique<V2TransportDeserializer>(V2TransportDeserializer(k1, k2, session_id));
  56 | +    test_deserializer(v1_deserializer, buffer, CMessageHeader::HEADER_SIZE);
  57 | +    test_deserializer(v2_deserializer, buffer, CHACHA20_POLY1305_AEAD_AAD_LEN + CHACHA20_POLY1305_AEAD_TAG_LEN);

MarcoFalke commented at 7:43 PM on April 24, 2020:

Can you explain the fuzzer a bit more? It seems you are reusing the buffer for both deserializers. Assuming that the structure of the buffer for both deserializers should be different, this might cause the fuzz engine to only focus on one of them (the "easier" one) and rarely explore paths in the v2 one, no?

If so, maybe you can split them up in two targets. See ./src/test/fuzz/deserialize.cpp on how to do that.

jonasschnelli commented at 9:55 AM on April 27, 2020:

Thanks! I switched to the same preprocessor bridge then used in deserialize.cpp.

jonasschnelli force-pushed on Apr 27, 2020

in src/protocol.h:257 in caa6c582f4 outdated

 263 |  
 264 |  /* Get a vector of all valid message types (see above) */
 265 |  const std::vector<std::string> &getAllNetMessageTypes();
 266 |  
 267 | +// Short Command IDs are a low bandwidth representations of a message type
 268 | +// The mapping is a peer to peer agreement

ariard commented at 7:05 AM on April 30, 2020:

I think here or BIP should lay out what the advantage and example of p2p agreement mapping. I can foresee people willingly to experiment or deploy their own light-clients protocols inside v2, and therefore favor bandwidth-reduction for what make sense for them.

However, I can't see how peer may signal custom mapping or at least agree they are on the default ones. You can agree out-of-band but can't assert peer identity until you succeed an authentication. And authentication protocol success may rely on mappings like COUNTERSIGN_PUBKEY_SHORT_ID = 47.

You may want a MAPPING/MAPPINGACK exchanged via a regular command-string. Actually BIP only precise "The ID/string mapping is a peer to peer arrangement and MAY be negotiated between the initiating and responding peer." You may update to say "Negotiation mechanism is left open to a future specification effort"

jonasschnelli commented at 3:48 PM on May 6, 2020:

This is probably a discussion that belong to the BIP rather to the implementation. I guess there are multiple ways how peers want to agree on a mapping (protocol version, dedicated command exchange, versionstring, port, etc.).

in src/net.h:837 in 9f81125a9e outdated

 702 | @@ -701,6 +703,53 @@ class V1TransportDeserializer final : public TransportDeserializer
 703 |      CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) override;
 704 |  };
 705 |  
 706 | +class V2TransportDeserializer : public TransportDeserializer

ariard commented at 7:12 AM on April 30, 2020:

You may add a reference saying serializer/deserializer are BIP-324 compliant

jonasschnelli commented at 4:03 PM on May 6, 2020:

Added

in src/net.h:895 in 9f81125a9e outdated

 763 | @@ -715,6 +764,24 @@ class V1TransportSerializer  : public TransportSerializer {
 764 |      void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) override;
 765 |  };
 766 |  
 767 | +class V2TransportSerializer : public TransportSerializer
 768 | +{
 769 | +private:
 770 | +    std::unique_ptr<ChaCha20Poly1305AEAD> m_aead;
 771 | +    CPrivKey m_aead_k1; //keep the keys for a later rekeying

ariard commented at 7:42 AM on April 30, 2020:

You may want to add old_ prefix and precise at first key rotation m_old_aead == m_aead

in src/net.cpp:784 in 9f81125a9e outdated

 729 | +
 730 | +        memcpy(&vRecv[m_hdr_pos], pch, copy_bytes);
 731 | +        m_hdr_pos += copy_bytes;
 732 | +
 733 | +        // if AAD incomplete, exit
 734 | +        if (m_hdr_pos < CHACHA20_POLY1305_AEAD_AAD_LEN) {

ariard commented at 8:04 AM on April 30, 2020:

Why the encrypted length doesn't get its own MAC to guarantee integrity ? It can decrypt to some garbage and therefore open to manipulating ciphertext-MAC seek in message ?

jonasschnelli commented at 3:52 PM on May 6, 2020:

BIP discussion. The length is covered by the MAC.

in src/net.cpp:789 in 9f81125a9e outdated

 734 | +        if (m_hdr_pos < CHACHA20_POLY1305_AEAD_AAD_LEN) {
 735 | +            return copy_bytes;
 736 | +        }
 737 | +
 738 | +        // we got the AAD bytes at this point (3 bytes encrypted packet length)
 739 | +        // we keep the sequence numbers unchanged at this point. Once the message is authenticated and decrypted, we increase the sequence numbers (or the aad_pos)

ariard commented at 8:13 AM on April 30, 2020:

Hmmm isn't the spec making the assumption you rely on some ordered transport protocol (TCP) ? If yes I think that's okay because you should be guarantee against network failure and peers may out-of-sync. But BIP should precise message shouldn't be resend with same sequence number?

Note IIRC Fibre is using UDP.

jonasschnelli commented at 12:05 PM on April 30, 2020:

I think we can make the assumptions that messages are processing in order and that the underlaying transport layer takes care of transmission failures. I don't think we need to add error/out-of-sequence detection in out message transport protocol.

ariard commented at 6:22 AM on May 8, 2020:

I agree it's good for now as an assumption. I think in the future there is valid use-cases where you want to send bitcoin traffic over some non-ordered communication channels like headers-over-radio and still benefit of encryption.

ariard commented at 4:11 PM on September 9, 2020:

I can't find this requirement in the BIP, not in "Packet Handling". Maybe you've already the change just withhold ?

in src/net.cpp:821 in 9f81125a9e outdated

 767 | +    }
 768 | +}
 769 | +
 770 | +CNetMessage V2TransportDeserializer::GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time)
 771 | +{
 772 | +    // In v2, vRecv contains the encrypted payload plus the MAC tag (1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)

ariard commented at 8:22 AM on April 30, 2020:

vRecv doesn't contain also AAD ?

in src/net.cpp:806 in 9f81125a9e outdated

 771 | +{
 772 | +    // In v2, vRecv contains the encrypted payload plus the MAC tag (1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)
 773 | +    assert(Complete());
 774 | +
 775 | +    // defensive decoding (MAC check, decryption, command deserialization)
 776 | +    // we'll always return a CNetMessage (even if encryption fails), we always increase the AEAD sequence numbers

ariard commented at 8:27 AM on April 30, 2020:

Even if decryption fails ? Also comment about increasing AAD sequence number is allusive can you precise a unsuccessful decryption still increase sequence number.

jonasschnelli commented at 3:55 PM on May 6, 2020:

What is the problem of increasing the sequence number if decryption fails? We detect it and can respond (disconnecting) on the layer above the transport.

ariard commented at 6:24 AM on May 8, 2020:

Right, decryption failure is fatal, there is no such ban-per-point in this case.

in src/net.cpp:835 in 9f81125a9e outdated

 781 | +    if (m_aead->Crypt(m_payload_seqnr, m_aad_seqnr, m_aad_pos, (unsigned char*)vRecv.data(), vRecv.size(), (const uint8_t*)vRecv.data(), vRecv.size(), false)) {
 782 | +        // MAC check was successful
 783 | +        valid_checksum = true;
 784 | +
 785 | +        // okay, we could decrypt it, now remove packet length and MAC tag
 786 | +        assert(vRecv.size() > CHACHA20_POLY1305_AEAD_AAD_LEN + CHACHA20_POLY1305_AEAD_TAG_LEN);

ariard commented at 8:33 AM on April 30, 2020:

What if len==0? Or is this case can't be hit due to previous Read logic?

jonasschnelli commented at 3:56 PM on May 6, 2020:

Yes. m_aead->Crypt would fail. This is an additional assertion to make sure the code below survives changes of the crypto system.

in src/net.cpp:865 in 9f81125a9e outdated

 825 | +    CNetMessage msg(std::move(vRecv)); // result in a message with CDataStream with readpos pointing to the message payload
 826 | +    msg.m_command = command_name;
 827 | +
 828 | +    // store state about valid header, netmagic and checksum
 829 | +    msg.m_valid_header = valid_header; // not relevant for v2, always pass
 830 | +    msg.m_valid_checksum = valid_checksum;

ariard commented at 8:48 AM on April 30, 2020:

I think checksum is also not revelant and always true, so comment may be generic for three of them? But generally if this field are always true for v2, a future refactor may just call public methods on messages in ProcessMessage like {Header,Checksum,Netmagic}Valid.

jonasschnelli commented at 3:59 PM on May 6, 2020:

msg.m_valid_checksum is a field we should populate to respect the abstract TransportDeserializer class. In the v2 case, the checksum is the MAC. We set the field whenever the MAC has validated. How to deal with it is a matter of the layer above.

in src/net.cpp:845 in 9f81125a9e outdated

 805 | +        // try for short ID
 806 | +        if (!valid_header && size_or_shortid >= 0) {
 807 | +            valid_header = true;
 808 | +            if (!GetCommandFromShortCommandID(size_or_shortid, command_name)) {
 809 | +                // unknown-short-id
 810 | +                //  results in a valid but unknown message (will be skipped)

ariard commented at 8:50 AM on April 30, 2020:

Maybe it would be better to drop unknown message, or at least as soon as we can to avoid some upstream buffer growing unbounded.

jonasschnelli commented at 12:07 PM on April 30, 2020:

I think we should tolerate unknown short IDs the same way as we tolerate unknown string commands (which we currently do for future backward compatibility).

ariard commented at 6:27 AM on May 8, 2020:

Right can we tolerate them, i,e not disconnecting peer and still drop them? We won't be able to process them anyway.

in src/net.cpp:797 in 9f81125a9e outdated

 792 | +        try {
 793 | +            vRecv >> size_or_shortid;
 794 | +        } catch (const std::ios_base::failure&) {
 795 | +            LogPrint(BCLog::NET, "Invalid command name\n");
 796 | +        }
 797 | +        if (size_or_shortid > 0 && size_or_shortid <= 12 && vRecv.size() >= size_or_shortid) {

ariard commented at 8:52 AM on April 30, 2020:

This line was confusing at first read, precise its size of command we parse or a reference to BIP "The command field MUST start with a byte that defines the length..."

jonasschnelli commented at 4:03 PM on May 6, 2020:

Fair point. Added a comment.

ariard commented at 8:56 AM on April 30, 2020: member

Reviewed until 9f81125, deserializer impl. Great to see progress on this!

See also few questions on spec itself: https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52#gistcomment-3276188

in src/net.cpp:805 in d9081b32a3 outdated

 758 | +        // switch state to reading message data
 759 | +        m_in_data = true;
 760 | +
 761 | +        return copy_bytes;
 762 | +    } else {
 763 | +        const unsigned int remaining = m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN - m_data_pos;

jonatack commented at 6:44 PM on April 30, 2020:

I think a comment for this conditional branch would be helpful.

     } else {
+        // Read the message data
         const unsigned int remaining = m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN - m_data_pos;

jonasschnelli commented at 4:07 PM on May 6, 2020:

Good point. Added.

in src/net.cpp:775 in d9081b32a3 outdated

 719 | @@ -718,6 +720,170 @@ CNetMessage V1TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
 720 |      return msg;
 721 |  }
 722 |  
 723 | +int V2TransportDeserializer::Read(const char* pch, unsigned int bytes)
 724 | +{
 725 | +    if (!m_in_data) {

jonatack commented at 6:46 PM on April 30, 2020:

9f81125 nit: perhaps start with the truthy branch (that reads the message) e.g. if (m_in_data) {

jonasschnelli commented at 4:06 PM on May 6, 2020:

I guess this evolved from legacy code with the focus to keep the diff small (V1 deserialiser has the in_data boolean). I'd say: lets keep it for consistency (better readability if one compared both deserializers).

in src/net.cpp:796 in d9081b32a3 outdated

 750 | +            m_message_size &= ~(1U << 23);
 751 | +        }
 752 | +
 753 | +        // reject messages larger than MAX_SIZE
 754 | +        if (m_message_size > MAX_SIZE) {
 755 | +            return -1;

jonatack commented at 6:58 PM on April 30, 2020:

9f81125 could replace the two -1 fail values returned in V2TransportDeserializer::Read (as well as the two in V1TransportDeserializer::readHeader) with a static constant whose name could make their meaning explicit, e.g. static constexpr int MESSAGE_FAILURE = -1; or similar

jonasschnelli commented at 4:29 PM on May 6, 2020:

I agree that this would be a nice cleanup. But it should also be done for the V1 deserializer. I think we should do that after this PR since it contains lines not changes otherwise.

in src/protocol.h:277 in d9081b32a3 outdated

 271 | +// returns 0 if no short command ID has been found
 272 | +uint8_t GetShortCommandIDFromCommand(const std::string cmd);
 273 | +
 274 | +// returns the command (string) from a short command ID
 275 | +// returns an empty string if short command ID has not been found
 276 | +bool GetCommandFromShortCommandID(uint8_t shortID, std::string& cmd);

jonatack commented at 9:58 PM on April 30, 2020:

caa6c58 Could invert the GetCommandFromShortCommandID parameter order? This would make it like read() and also allow it to have the same first param as GetShortCommandIDFromCommand.

in src/protocol.h:272 in d9081b32a3 outdated

 267 | +// Short Command IDs are a low bandwidth representations of a message type
 268 | +// The mapping is a peer to peer agreement
 269 | +
 270 | +// returns the short command ID for a command
 271 | +// returns 0 if no short command ID has been found
 272 | +uint8_t GetShortCommandIDFromCommand(const std::string cmd);

jonatack commented at 10:06 PM on April 30, 2020:

caa6c58 can reference instead of copy?

-uint8_t GetShortCommandIDFromCommand(const std::string cmd);
+uint8_t GetShortCommandIDFromCommand(const std::string& cmd);

jonasschnelli commented at 4:31 PM on May 6, 2020:

Absolutely. Changed now.

in src/protocol.cpp:209 in d9081b32a3 outdated

 203 | @@ -204,3 +204,127 @@ const std::vector<std::string> &getAllNetMessageTypes()
 204 |  {
 205 |      return allNetMessageTypesVec;
 206 |  }
 207 | +
 208 | +
 209 | +uint8_t GetShortCommandIDFromCommand(const std::string cmd)

jonatack commented at 10:08 PM on April 30, 2020:

caa6c58 can reference instead of copy?

-uint8_t GetShortCommandIDFromCommand(const std::string cmd)
+uint8_t GetShortCommandIDFromCommand(const std::string& cmd)

in src/net.cpp:896 in d9081b32a3 outdated

 902 | +    size_t serialized_command_size = 1; // short-IDs are 1 byte
 903 | +    const uint8_t cmd_short_id = GetShortCommandIDFromCommand(msg.command);
 904 | +    if (cmd_short_id == 0) {
 905 | +        // message command without an assigned short-ID
 906 | +        assert(msg.command.size() <= 12);
 907 | +        // encode as varstr, max 12 chars

jonatack commented at 10:24 PM on April 30, 2020:

9f81125 could replace the int value of 12 here and line 809 above with an explicit CMD_MAX_CHARS_SIZE (or similar) int static constant

jonasschnelli commented at 4:34 PM on May 6, 2020:

Good point. Changed.

jonatack commented at 10:27 PM on April 30, 2020: member

Reviewed up to 9f81125. ACK so far with minor, non-blocking comments; feel free to ignore.

in src/net.cpp:1022 in d9081b32a3 outdated

 941 | +    assert(!bit24);
 942 | +
 943 | +    // check if we should rekey after this message
 944 | +    const int64_t now = GetTime(); //TODO: check how expansive the GetTime call is and if it is avoidable
 945 | +    bool rekey = false;
 946 | +    if (m_bytes_encrypted >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 32 * 1024 : REKEY_LIMIT_BYTES) || now - m_time_last_rekey >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 10 : REKEY_LIMIT_TIME)) {

jonatack commented at 11:57 AM on May 1, 2020:

e04eddb3 this conditional with 2 nested ternary conditionals on one line is pretty hard to read.

<details> <summary>Here are two ideas.</summary>

-    if (m_bytes_encrypted >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 32 * 1024 : REKEY_LIMIT_BYTES) || now - m_time_last_rekey >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 10 : REKEY_LIMIT_TIME)) {
+    bool fast_rekey = gArgs.GetBoolArg("-netencryptionfastrekey", false);
+    if (m_bytes_encrypted >= (fast_rekey ? 32 * 1024 : REKEY_LIMIT_BYTES) || (now - m_time_last_rekey >= (fast_rekey ? 10 : REKEY_LIMIT_TIME))) {

or

-    if (m_bytes_encrypted >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 32 * 1024 : REKEY_LIMIT_BYTES) || now - m_time_last_rekey >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 10 : REKEY_LIMIT_TIME)) {
+    if (gArgs.GetBoolArg("-netencryptionfastrekey", false)) {
+        if (m_bytes_encrypted >= 32 * 1024 || (now - m_time_last_rekey >= 10)) {
+            rekey = true;
+        }
+    } else {
+        if (m_bytes_encrypted >= REKEY_LIMIT_BYTES || (now - m_time_last_rekey >= REKEY_LIMIT_TIME)) {
+            rekey = true;
+        }
+    }
+    if (rekey) {
         LogPrint(BCLog::NET, "Rekey limits reached, performing rekey.\n");
         msg.data[2] |= (1u << 7);
-        rekey = true;
     }

</details>

Also, it would be nice to replace the various magic values with static constexprs that communicate meaning, like you are already doing in this commit for the rekey values in net.h.

in src/net.cpp:945 in d9081b32a3 outdated

 864 | +
 865 | +            // reset sequence numbers
 866 | +            m_payload_seqnr = 0;
 867 | +            m_aad_seqnr = 0;
 868 | +            m_aad_pos = 0;
 869 | +            m_bytes_decrypted = 0;

jonatack commented at 12:12 PM on May 1, 2020:

e04eddb suggestion (like your code at line 983 below). It also clarifies that m_bytes_decrypted is a counter of bytes decrypted for the same key.

             m_aad_pos = 0;
-            m_bytes_decrypted = 0;
+
+            // reset rekey counters
             m_time_last_rekey = now;
+            m_bytes_decrypted = 0;

In general, there is a fair amount of rekey code duplication in this commit but optimising would be premature at this point.

in src/net.h:834 in d9081b32a3 outdated

 708 | +// Re-key after 1GB (RFC4253 / SSH recommendation) or after 1h
 709 | +static constexpr unsigned int REKEY_LIMIT_BYTES = (1024 * 1024 * 1024);
 710 | +static constexpr unsigned int REKEY_LIMIT_TIME = 3600;
 711 | +static constexpr unsigned int REKEY_ABORT_LIMIT_BYTES = REKEY_LIMIT_BYTES * 1.1; // abort after ~10% tolerance buffer
 712 | +static constexpr unsigned int REKEY_ABORT_LIMIT_TIME = REKEY_LIMIT_BYTES * 1.1;  // abort after ~10% tolerance buffer
 713 | +static constexpr unsigned int MIN_REKEY_TIME = 10;                               // minimal rekey time to avoid DOS

jonatack commented at 12:26 PM on May 1, 2020:

e04eddb suggested change

-static constexpr unsigned int MIN_REKEY_TIME = 10;      // minimal rekey time to avoid DOS
+static constexpr unsigned int MIN_REKEY_TIME = 10;      // minimum rekey time in seconds to avoid DOS

in src/net.cpp:1020 in d9081b32a3 outdated

 939 | +    // length is only allowed up to 2^23 (bit24 is used for indicating rekey)
 940 | +    bool bit24 = msg.data[2] & (1u << 7);
 941 | +    assert(!bit24);
 942 | +
 943 | +    // check if we should rekey after this message
 944 | +    const int64_t now = GetTime(); //TODO: check how expansive the GetTime call is and if it is avoidable

jonatack commented at 12:29 PM on May 1, 2020:

It seems that GetTime() is deprecated, see util/time.h

/**
 * DEPRECATED
 * Use either GetSystemTimeInSeconds (not mockable) or GetTime<T> (mockable)
 */
int64_t GetTime();

ariard commented at 9:33 AM on May 2, 2020:

If you want to avoid a syscall in hot path, I think you can fetch time only X messages, its okay to be a bit late, for time responder only check if rekey initiator is too early.

in src/test/net_tests.cpp:779 in d9081b32a3 outdated

 326 | +{
 327 | +    size_t read_bytes = 0;
 328 | +    if (serialized_header.size() > 0) read_bytes += deserializer->Read((const char*)serialized_header.data(), serialized_header.size());
 329 | +    //  second: read the encrypted payload (if required)
 330 | +    if (msg.data.size() > 0) read_bytes += deserializer->Read((const char*)msg.data.data(), msg.data.size());
 331 | +    if (msg.data.size() > read_bytes && msg.data.size() - read_bytes > 0) read_bytes += deserializer->Read((const char*)msg.data.data() + read_bytes, msg.data.size() - read_bytes);

jonatack commented at 12:41 PM on May 1, 2020:

e04eddb what is the difference between msg.data.size() > read_bytes and msg.data.size() - read_bytes > 0? I'm trying to understand why both checks are needed.

jonatack commented at 1:54 PM on May 1, 2020: member

ACK d9081b32a36135c4fe10838 code review, multiple gcc/clang/fuzz builds and test runs on Debian.

The new unit tests pass reliably:

src/test/test_bitcoin -t net_tests -l test_suite

The changed fuzz tests run successfully so long as the existing seeds aren't used for the v2 test:

src/test/fuzz/p2p_v1_transport_deserializer ../qa-assets/fuzz_seed_corpus/

[#340346](/bitcoin-bitcoin/340346/)	REDUCE cov: 1175 ft: 7365 corp: 135/3753Kb exec/s: 68 rss: 806Mb L: 13971/1048576 MS: 2 ChangeBit-EraseBytes-
[#346968](/bitcoin-bitcoin/346968/)	REDUCE cov: 1175 ft: 7365 corp: 135/3732Kb exec/s: 68 rss: 806Mb L: 198741/1048576 MS: 2 PersAutoDict-EraseBytes- DE: "\x00\x00\x00\x00\x00\x00\x00\x05"-

src/test/fuzz/p2p_v2_transport_deserializer

[#264711](/bitcoin-bitcoin/264711/)	REDUCE cov: 2395 ft: 2701 corp: 15/2827b exec/s: 109 rss: 422Mb L: 238/499 MS: 5 ChangeBinInt-EraseBytes-InsertByte-EraseBytes-PersAutoDict- DE: "\xff\xff\xff\xff"-
[#289100](/bitcoin-bitcoin/289100/)	REDUCE cov: 2395 ft: 2701 corp: 15/2571b exec/s: 109 rss: 422Mb L: 243/253 MS: 4 ChangeByte-ChangeByte-EraseBytes-EraseBytes-

With qa-assets/fuzz_seed_corpus, the v2 fuzzer fails immediately with Assertion 'msg.m_raw_message_size == header_size + msg.m_message_size' failed.

<details> <summary>details of 3 runs/crashes</summary>

<code>$ src/test/fuzz/p2p_v2_transport_deserializer ../qa-assets/fuzz_seed_corpus/</code>

run 1

INFO: Seed: 2027223820
INFO: Loaded 1 modules   (492150 inline 8-bit counters): 492150 [0x563254a10348, 0x563254a885be), 
INFO: Loaded 1 PC tables (492150 PCs): 492150 [0x563254a885c0,0x56325520ad20), 
INFO:    39045 files found in ../qa-assets/fuzz_seed_corpus/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
INFO: seed corpus: files: 39045 min: 1b max: 3984182b total: 242031732b rss: 153Mb
[#2048](/bitcoin-bitcoin/2048/)	pulse  cov: 2395 ft: 2641 corp: 6/19b exec/s: 256 rss: 457Mb
[#4096](/bitcoin-bitcoin/4096/)	pulse  cov: 2395 ft: 2641 corp: 6/19b exec/s: 178 rss: 457Mb
[#8192](/bitcoin-bitcoin/8192/)	pulse  cov: 2395 ft: 2656 corp: 7/39b exec/s: 132 rss: 457Mb
p2p_v2_transport_deserializer: test/fuzz/p2p_transport_deserializer.cpp:37: void test_deserializer(std::unique_ptr<TransportDeserializer> &, const std::vector<uint8_t> &, const int &): Assertion `msg.m_raw_message_size == header_size + msg.m_message_size' failed.
==7613== ERROR: libFuzzer: deadly signal
    [#0](/bitcoin-bitcoin/0/) 0x563251cce9a7 in __sanitizer_print_stack_trace /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_stack.cc:38:3
    [#1](/bitcoin-bitcoin/1/) 0x563251c0c826 in fuzzer::Fuzzer::CrashCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:233:5
    [#2](/bitcoin-bitcoin/2/) 0x563251c0c7ef in fuzzer::Fuzzer::StaticCrashSignalCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:206:6
    [#3](/bitcoin-bitcoin/3/) 0x7f46cac8772f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1272f)
    [#4](/bitcoin-bitcoin/4/) 0x7f46ca4e47ba in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x377ba)
    [#5](/bitcoin-bitcoin/5/) 0x7f46ca4cf534 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22534)
    [#6](/bitcoin-bitcoin/6/) 0x7f46ca4cf40e in __tls_get_addr (/lib/x86_64-linux-gnu/libc.so.6+0x2240e)
    [#7](/bitcoin-bitcoin/7/) 0x7f46ca4dd101 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x30101)
    [#8](/bitcoin-bitcoin/8/) 0x563251cf6e7c in test_deserializer(std::unique_ptr<TransportDeserializer, std::default_delete<TransportDeserializer> >&, std::vector<unsigned char, std::allocator<unsigned char> > const&, int const&) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_transport_deserializer.cpp:37:13
    [#9](/bitcoin-bitcoin/9/) 0x563251cf731f in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_transport_deserializer.cpp:59:5
    [#10](/bitcoin-bitcoin/10/) 0x563252f4721f in LLVMFuzzerTestOneInput /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp:38:5
    [#11](/bitcoin-bitcoin/11/) 0x563251c0da7c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:515:13
    [#12](/bitcoin-bitcoin/12/) 0x563251c0d2db in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:440:3
    [#13](/bitcoin-bitcoin/13/) 0x563251c0f1ce in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:723:7
    [#14](/bitcoin-bitcoin/14/) 0x563251c0f445 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:739:3
    [#15](/bitcoin-bitcoin/15/) 0x563251c042d0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:754:6
    [#16](/bitcoin-bitcoin/16/) 0x563251c25ed2 in main /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    [#17](/bitcoin-bitcoin/17/) 0x7f46ca4d109a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    [#18](/bitcoin-bitcoin/18/) 0x563251bfd369 in _start (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_v2_transport_deserializer+0x1d28369)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x77,0xb8,0xe0,0x9f,0x7,0x3f,0xbe,0x55,0x51,0x38,0x0,0x0,0x0,0x0,0x7a,0x97,0x7c,0x73,0x2d,0x8,0xd,0xcb,0x29,0xf,0x5c,0xb7,0x3a,0xe3,0x65,0x32,0x7f,0xf0,0xc8,0x5b,0x6d,0xf7,0x86,0x2,0xb0,0x6c,0xf5,0x2a,0x6a,0xef,0xc6,0x2e,
w\xb8\xe0\x9f\x07?\xbeUQ8\x00\x00\x00\x00z\x97|s-\x08\x0d\xcb)\x0f\\\xb7:\xe3e2\x7f\xf0\xc8[m\xf7\x86\x02\xb0l\xf5*j\xef\xc6.
artifact_prefix='./'; Test unit written to ./crash-a7e7c2c09b926a39e1b1618476b0fc4121e733db
Base64: d7jgnwc/vlVROAAAAAB6l3xzLQgNyykPXLc642Uyf/DIW233hgKwbPUqau/GLg==

run 2

INFO: Seed: 1918826274
INFO: Loaded 1 modules   (492150 inline 8-bit counters): 492150 [0x560dbfdc9348, 0x560dbfe415be), 
INFO: Loaded 1 PC tables (492150 PCs): 492150 [0x560dbfe415c0,0x560dc05c3d20), 
INFO:    39052 files found in ../qa-assets/fuzz_seed_corpus/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
INFO: seed corpus: files: 39052 min: 1b max: 3984182b total: 243085713b rss: 155Mb
[#2048](/bitcoin-bitcoin/2048/)	pulse  cov: 2395 ft: 2641 corp: 6/19b exec/s: 204 rss: 459Mb
[#4096](/bitcoin-bitcoin/4096/)	pulse  cov: 2395 ft: 2641 corp: 6/19b exec/s: 136 rss: 459Mb
[#8192](/bitcoin-bitcoin/8192/)	pulse  cov: 2395 ft: 2656 corp: 7/39b exec/s: 124 rss: 459Mb
p2p_v2_transport_deserializer: test/fuzz/p2p_transport_deserializer.cpp:37: void test_deserializer(std::unique_ptr<TransportDeserializer> &, const std::vector<uint8_t> &, const int): Assertion `msg.m_raw_message_size == header_size + msg.m_message_size' failed.
==7991== ERROR: libFuzzer: deadly signal
    [#0](/bitcoin-bitcoin/0/) 0x560dbd0879a7 in __sanitizer_print_stack_trace /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_stack.cc:38:3
    [#1](/bitcoin-bitcoin/1/) 0x560dbcfc5826 in fuzzer::Fuzzer::CrashCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:233:5
    [#2](/bitcoin-bitcoin/2/) 0x560dbcfc57ef in fuzzer::Fuzzer::StaticCrashSignalCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:206:6
    [#3](/bitcoin-bitcoin/3/) 0x7f3e3b0a172f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1272f)
    [#4](/bitcoin-bitcoin/4/) 0x7f3e3a8fe7ba in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x377ba)
    [#5](/bitcoin-bitcoin/5/) 0x7f3e3a8e9534 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22534)
    [#6](/bitcoin-bitcoin/6/) 0x7f3e3a8e940e in __tls_get_addr (/lib/x86_64-linux-gnu/libc.so.6+0x2240e)
    [#7](/bitcoin-bitcoin/7/) 0x7f3e3a8f7101 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x30101)
    [#8](/bitcoin-bitcoin/8/) 0x560dbd0afe4f in test_deserializer(std::unique_ptr<TransportDeserializer, std::default_delete<TransportDeserializer> >&, std::vector<unsigned char, std::allocator<unsigned char> > const&, int) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_transport_deserializer.cpp:37:13
    [#9](/bitcoin-bitcoin/9/) 0x560dbd0b02ce in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_transport_deserializer.cpp:59:5
    [#10](/bitcoin-bitcoin/10/) 0x560dbe3001af in LLVMFuzzerTestOneInput /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp:38:5
    [#11](/bitcoin-bitcoin/11/) 0x560dbcfc6a7c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:515:13
    [#12](/bitcoin-bitcoin/12/) 0x560dbcfc62db in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:440:3
    [#13](/bitcoin-bitcoin/13/) 0x560dbcfc81ce in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:723:7
    [#14](/bitcoin-bitcoin/14/) 0x560dbcfc8445 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:739:3
    [#15](/bitcoin-bitcoin/15/) 0x560dbcfbd2d0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:754:6
    [#16](/bitcoin-bitcoin/16/) 0x560dbcfdeed2 in main /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    [#17](/bitcoin-bitcoin/17/) 0x7f3e3a8eb09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    [#18](/bitcoin-bitcoin/18/) 0x560dbcfb6369 in _start (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_v2_transport_deserializer+0x1d28369)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x77,0xb8,0xe0,0x9f,0x7,0x3f,0xbe,0x55,0x51,0x38,0x0,0x0,0x0,0x0,0x7a,0x97,0x7c,0x73,0x2d,0x8,0xd,0xcb,0x29,0xf,0x5c,0xb7,0x3a,0xe3,0x65,0x32,0x7f,0xf0,0xc8,0x5b,0x6d,0xf7,0x86,0x2,0xb0,0x6c,0xf5,0x2a,0x6a,0xef,0xc6,0x2e,
w\xb8\xe0\x9f\x07?\xbeUQ8\x00\x00\x00\x00z\x97|s-\x08\x0d\xcb)\x0f\\\xb7:\xe3e2\x7f\xf0\xc8[m\xf7\x86\x02\xb0l\xf5*j\xef\xc6.
artifact_prefix='./'; Test unit written to ./crash-a7e7c2c09b926a39e1b1618476b0fc4121e733db
Base64: d7jgnwc/vlVROAAAAAB6l3xzLQgNyykPXLc642Uyf/DIW233hgKwbPUqau/GLg==

run 3

INFO: Seed: 945305236
INFO: Loaded 1 modules   (492150 inline 8-bit counters): 492150 [0x55df3d84c348, 0x55df3d8c45be), 
INFO: Loaded 1 PC tables (492150 PCs): 492150 [0x55df3d8c45c0,0x55df3e046d20), 
INFO:    39057 files found in ../qa-assets/fuzz_seed_corpus/
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 1048576 bytes
INFO: seed corpus: files: 39057 min: 1b max: 3984182b total: 243949671b rss: 153Mb
[#2048](/bitcoin-bitcoin/2048/)	pulse  cov: 2395 ft: 2641 corp: 6/19b exec/s: 186 rss: 457Mb
[#4096](/bitcoin-bitcoin/4096/)	pulse  cov: 2395 ft: 2641 corp: 6/19b exec/s: 136 rss: 458Mb
[#8192](/bitcoin-bitcoin/8192/)	pulse  cov: 2395 ft: 2656 corp: 7/39b exec/s: 120 rss: 458Mb
p2p_v2_transport_deserializer: test/fuzz/p2p_transport_deserializer.cpp:37: void test_deserializer(std::unique_ptr<TransportDeserializer> &, const std::vector<uint8_t> &, const int): Assertion `msg.m_raw_message_size == header_size + msg.m_message_size' failed.
==8031== ERROR: libFuzzer: deadly signal
    [#0](/bitcoin-bitcoin/0/) 0x55df3ab0a9a7 in __sanitizer_print_stack_trace /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_stack.cc:38:3
    [#1](/bitcoin-bitcoin/1/) 0x55df3aa48826 in fuzzer::Fuzzer::CrashCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:233:5
    [#2](/bitcoin-bitcoin/2/) 0x55df3aa487ef in fuzzer::Fuzzer::StaticCrashSignalCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:206:6
    [#3](/bitcoin-bitcoin/3/) 0x7f322f46b72f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1272f)
    [#4](/bitcoin-bitcoin/4/) 0x7f322ecc87ba in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x377ba)
    [#5](/bitcoin-bitcoin/5/) 0x7f322ecb3534 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22534)
    [#6](/bitcoin-bitcoin/6/) 0x7f322ecb340e in __tls_get_addr (/lib/x86_64-linux-gnu/libc.so.6+0x2240e)
    [#7](/bitcoin-bitcoin/7/) 0x7f322ecc1101 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x30101)
    [#8](/bitcoin-bitcoin/8/) 0x55df3ab32e4f in test_deserializer(std::unique_ptr<TransportDeserializer, std::default_delete<TransportDeserializer> >&, std::vector<unsigned char, std::allocator<unsigned char> > const&, int) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_transport_deserializer.cpp:37:13
    [#9](/bitcoin-bitcoin/9/) 0x55df3ab332ce in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_transport_deserializer.cpp:59:5
    [#10](/bitcoin-bitcoin/10/) 0x55df3bd831af in LLVMFuzzerTestOneInput /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp:38:5
    [#11](/bitcoin-bitcoin/11/) 0x55df3aa49a7c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:515:13
    [#12](/bitcoin-bitcoin/12/) 0x55df3aa492db in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:440:3
    [#13](/bitcoin-bitcoin/13/) 0x55df3aa4b1ce in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:723:7
    [#14](/bitcoin-bitcoin/14/) 0x55df3aa4b445 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:739:3
    [#15](/bitcoin-bitcoin/15/) 0x55df3aa402d0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:754:6
    [#16](/bitcoin-bitcoin/16/) 0x55df3aa61ed2 in main /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    [#17](/bitcoin-bitcoin/17/) 0x7f322ecb509a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    [#18](/bitcoin-bitcoin/18/) 0x55df3aa39369 in _start (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_v2_transport_deserializer+0x1d28369)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x77,0xb8,0xe0,0x9f,0x7,0x3f,0xbe,0x55,0x51,0x38,0x0,0x0,0x0,0x0,0x7a,0x97,0x7c,0x73,0x2d,0x8,0xd,0xcb,0x29,0xf,0x5c,0xb7,0x3a,0xe3,0x65,0x32,0x7f,0xf0,0xc8,0x5b,0x6d,0xf7,0x86,0x2,0xb0,0x6c,0xf5,0x2a,0x6a,0xef,0xc6,0x2e,
w\xb8\xe0\x9f\x07?\xbeUQ8\x00\x00\x00\x00z\x97|s-\x08\x0d\xcb)\x0f\\\xb7:\xe3e2\x7f\xf0\xc8[m\xf7\x86\x02\xb0l\xf5*j\xef\xc6.
artifact_prefix='./'; Test unit written to ./crash-a7e7c2c09b926a39e1b1618476b0fc4121e733db
Base64: d7jgnwc/vlVROAAAAAB6l3xzLQgNyykPXLc642Uyf/DIW233hgKwbPUqau/GLg==

</details>

I did not look if this is because the v2 fuzz test needs to create and consume separate seeds or if there is an issue with the v2 code under test.

Noting also that at some point -netencryptionfastrekey should be added to the help if it's a user-facing option.

A few minor non-blocking comments follow; feel free to pick/choose/ignore, I'm happy to re-review if you retouch.

jonatack commented at 7:55 AM on May 2, 2020: member

Fuzzer updates:

v1. src/test/fuzz/p2p_v1_transport_deserializer ../qa-assets/fuzz_seed_corpus/ still running after 11 million iterations.

[#11209522](/bitcoin-bitcoin/11209522/)	REDUCE cov: 1175 ft: 7368 corp: 138/1841Kb exec/s: 170 rss: 806Mb L: 1510/1048576 MS: 4 CMP-CMP-InsertRepeatedBytes-EraseBytes- DE: "d\x00"-"\x01\x00\x00\x00\x00\x00\x00\x16"-
[#11215383](/bitcoin-bitcoin/11215383/)	REDUCE cov: 1175 ft: 7368 corp: 138/1841Kb exec/s: 170 rss: 806Mb L: 456/1048576 MS: 1 EraseBytes-
[#11399712](/bitcoin-bitcoin/11399712/)	REDUCE cov: 1175 ft: 7368 corp: 138/1841Kb exec/s: 170 rss: 806Mb L: 1509/1048576 MS: 4 EraseBytes-ChangeASCIIInt-CMP-CMP- DE: "\x00s\x00\x00"-"\x00\x00\x00v"-

v2. src/test/fuzz/p2p_v2_transport_deserializer without qa-assets/fuzz_seed_corpus crashed after a few million iterations.

[#1048576](/bitcoin-bitcoin/1048576/)	pulse  cov: 2395 ft: 2701 corp: 15/2571b exec/s: 110 rss: 423Mb
[#2097152](/bitcoin-bitcoin/2097152/)	pulse  cov: 2395 ft: 2701 corp: 15/2571b exec/s: 112 rss: 424Mb
[#4194304](/bitcoin-bitcoin/4194304/)	pulse  cov: 2395 ft: 2701 corp: 15/2571b exec/s: 117 rss: 426Mb

<details> <summary>v2 fuzzer crash output</summary>

<code>$ src/test/fuzz/p2p_v2_transport_deserializer</code>

[#4194304](/bitcoin-bitcoin/4194304/)	pulse  cov: 2395 ft: 2701 corp: 15/2571b exec/s: 117 rss: 426Mb
p2p_v2_transport_deserializer: test/fuzz/p2p_transport_deserializer.cpp:37: void test_deserializer(std::unique_ptr<TransportDeserializer> &, const std::vector<uint8_t> &, const int): Assertion `msg.m_raw_message_size == header_size + msg.m_message_size' failed.
==8131== ERROR: libFuzzer: deadly signal
    [#0](/bitcoin-bitcoin/0/) 0x564d8d9e69a7 in __sanitizer_print_stack_trace /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_stack.cc:38:3
    [#1](/bitcoin-bitcoin/1/) 0x564d8d924826 in fuzzer::Fuzzer::CrashCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:233:5
    [#2](/bitcoin-bitcoin/2/) 0x564d8d9247ef in fuzzer::Fuzzer::StaticCrashSignalCallback() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:206:6
    [#3](/bitcoin-bitcoin/3/) 0x7f53564b372f  (/lib/x86_64-linux-gnu/libpthread.so.0+0x1272f)
    [#4](/bitcoin-bitcoin/4/) 0x7f5355d107ba in gsignal (/lib/x86_64-linux-gnu/libc.so.6+0x377ba)
    [#5](/bitcoin-bitcoin/5/) 0x7f5355cfb534 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x22534)
    [#6](/bitcoin-bitcoin/6/) 0x7f5355cfb40e in __tls_get_addr (/lib/x86_64-linux-gnu/libc.so.6+0x2240e)
    [#7](/bitcoin-bitcoin/7/) 0x7f5355d09101 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x30101)
    [#8](/bitcoin-bitcoin/8/) 0x564d8da0ee4f in test_deserializer(std::unique_ptr<TransportDeserializer, std::default_delete<TransportDeserializer> >&, std::vector<unsigned char, std::allocator<unsigned char> > const&, int) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_transport_deserializer.cpp:37:13
    [#9](/bitcoin-bitcoin/9/) 0x564d8da0f2ce in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_transport_deserializer.cpp:59:5
    [#10](/bitcoin-bitcoin/10/) 0x564d8ec5f1af in LLVMFuzzerTestOneInput /home/jon/projects/bitcoin/bitcoin/src/test/fuzz/fuzz.cpp:38:5
    [#11](/bitcoin-bitcoin/11/) 0x564d8d925a7c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:515:13
    [#12](/bitcoin-bitcoin/12/) 0x564d8d9252db in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:440:3
    [#13](/bitcoin-bitcoin/13/) 0x564d8d926d0d in fuzzer::Fuzzer::MutateAndTestOne() /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:648:19
    [#14](/bitcoin-bitcoin/14/) 0x564d8d9275c5 in fuzzer::Fuzzer::Loop(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, fuzzer::fuzzer_allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:775:5
    [#15](/bitcoin-bitcoin/15/) 0x564d8d91c2d0 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:754:6
    [#16](/bitcoin-bitcoin/16/) 0x564d8d93ded2 in main /tmp/final/llvm.src/projects/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    [#17](/bitcoin-bitcoin/17/) 0x7f5355cfd09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    [#18](/bitcoin-bitcoin/18/) 0x564d8d915369 in _start (/home/jon/projects/bitcoin/bitcoin/src/test/fuzz/p2p_v2_transport_deserializer+0x1d28369)

NOTE: libFuzzer has rudimentary signal handlers.
      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 5 CMP-ChangeBinInt-CopyPart-CopyPart-InsertRepeatedBytes- DE: "\x9bG\x1f\x00"-; base unit: 6931e757f50084d7f7e1142dae37b88f384b7c28
0x65,0xb8,0xe0,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0x60,0xf5,0x65,0xb8,0xe0,0xf5,
e\xb8\xe0```````````````````````````````````````````````````````````````````````````````````\xf5e\xb8\xe0\xf5
artifact_prefix='./'; Test unit written to ./crash-7a494e2caeec52213088a24c43216beb0f331a65
Base64: ZbjgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGD1Zbjg9Q==

</details>

It appears to be the same crash as the ones I was seeing immediately when running the v2 fuzz test with the qa-assets seeds in #18242#pullrequestreview-404120272 above, e.g.

src/test/fuzz/p2p_v2_transport_deserializer ../qa-assets/fuzz_seed_corpus/

in src/net.cpp:905 in d9081b32a3 outdated

 911 | +    // the packet length excludes the 16 byte MAC tag
 912 | +    uint32_t packet_length = serialized_command_size + msg.data.size();
 913 | +
 914 | +    // prepare the packet length & message command and reserve 4 bytes (3bytes AD + 1byte short-ID)
 915 | +    std::vector<unsigned char> serialized_header(CHACHA20_POLY1305_AEAD_AAD_LEN + 1);
 916 | +    // LE serialize the 24bits length

ariard commented at 7:57 AM on May 2, 2020:

Ah yes, I always forgot that Bitcoin isn't network-byte order, maybe it could be part of the spec as a nice reminder.

in src/net.cpp:926 in d9081b32a3 outdated

 932 | +    // TODO: if we refactor the ChaCha20Poly1350 crypt function to allow separate buffers for
 933 | +    //       the AD, payload and MAC, we could avoid a insert and thus a potential reallocation
 934 | +    msg.data.insert(msg.data.begin(), serialized_header.begin(), serialized_header.end());
 935 | +
 936 | +    // resize the message buffer to make space for the MAC tag
 937 | +    msg.data.resize(msg.data.size() + CHACHA20_POLY1305_AEAD_TAG_LEN, 0);

ariard commented at 8:08 AM on May 2, 2020:

I don't know if std::vector::insert would do preventive reallocation, and that's implementation specific, but do resize first with both header_size + tag_len and then insert would be better?

jonasschnelli commented at 5:56 PM on May 6, 2020:

Can you give me a rational why resize before insert would be better?

ariard commented at 6:37 AM on May 8, 2020:

At std::vector::insert, if msg.data capacity isn't enough an allocation is realized. Then at std::vector::resize, if capacity is too short again, a newer allocation is realized and a memcopy processed. You avoid one memory allocator call which may trigger a syscall in worst-case scenario ?

in src/test/net_tests.cpp:933 in e7bed2ed32 outdated

 321 | @@ -320,4 +322,77 @@ BOOST_AUTO_TEST_CASE(PoissonNextSend)
 322 |      g_mock_deterministic_tests = false;
 323 |  }
 324 |  
 325 | +void message_serialize_deserialize_test(bool v2, const std::vector<CSerializedNetMsg>& test_msgs)

ariard commented at 8:41 AM on May 2, 2020:

I think it would be nice to comment what this test aims to verify. AFAICT it checks for round-trip correctness, all others cases like non-encrypted packets, extended packets, shorter packets, zeroed-MAC, garbage MAC are they deferred to fuzzer ?

in src/net.cpp:816 in e04eddb397 outdated

 740 | @@ -741,6 +741,15 @@ int V2TransportDeserializer::Read(const char* pch, unsigned int bytes)
 741 |              return -1;
 742 |          }
 743 |  
 744 | +        // check and unset rekey bit
 745 | +        // the counterparty can signal a post-this-message rekey by setting the

ariard commented at 8:49 AM on May 2, 2020:

What do you mean by "post-this-message" in this context? Is this coming from RFC 4253?

jonasschnelli commented at 5:58 PM on May 6, 2020:

Could be my bad English. What I mean with that comment is that the rekey signals that "after" this message (the next message and all following), a new key will be used.

jonatack commented at 7:09 PM on May 6, 2020:

perhaps "the counterparty can signal a post-message re-key, e.g. after this message a new key will be used, by setting the"

ariard commented at 6:39 AM on May 8, 2020:

Or perhaps "the counterparty can signal a re-key, after this message a new key will be used, by setting the"?

in src/net.cpp:820 in e04eddb397 outdated

 740 | @@ -741,6 +741,15 @@ int V2TransportDeserializer::Read(const char* pch, unsigned int bytes)
 741 |              return -1;
 742 |          }
 743 |  
 744 | +        // check and unset rekey bit
 745 | +        // the counterparty can signal a post-this-message rekey by setting the
 746 | +        // most significant bit in the (unencrypted) length
 747 | +        m_rekey_flag = static_cast<bool>(m_message_size & (1U << 23));
 748 | +        if (m_rekey_flag) {
 749 | +            LogPrint(BCLog::NET, "Rekey flag detected\n");

ariard commented at 8:54 AM on May 2, 2020:

Maybe also log session_id or anything to tie a rekeying to a given session/peer?

in src/net.cpp:797 in e04eddb397 outdated

 790 | @@ -782,6 +791,9 @@ CNetMessage V2TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
 791 |          // MAC check was successful
 792 |          valid_checksum = true;
 793 |  
 794 | +        // count bytes we decrypted including MAC tag + AD

ariard commented at 8:56 AM on May 2, 2020:

You should precise in BIP that we account only after a successful decryption. Also the accounting data scope on MAC+AEAD.

jonasschnelli commented at 6:06 PM on May 6, 2020:

This is a very good point. I think the BIP is correct about the "data sent" (every byte on the wire). But I think the implementation is wrong in the way that it doesn't count messages with invalid headers,... which might be exploitable. I'll work on that.

in src/net.h:833 in e04eddb397 outdated

 707 | +// used to encrypt more than 2^70 bytes under the same {key, nonce}
 708 | +// Re-key after 1GB (RFC4253 / SSH recommendation) or after 1h
 709 | +static constexpr unsigned int REKEY_LIMIT_BYTES = (1024 * 1024 * 1024);
 710 | +static constexpr unsigned int REKEY_LIMIT_TIME = 3600;
 711 | +static constexpr unsigned int REKEY_ABORT_LIMIT_BYTES = REKEY_LIMIT_BYTES * 1.1; // abort after ~10% tolerance buffer
 712 | +static constexpr unsigned int REKEY_ABORT_LIMIT_TIME = REKEY_LIMIT_BYTES * 1.1;  // abort after ~10% tolerance buffer

ariard commented at 9:14 AM on May 2, 2020:

You should precise in BIP that a rekey initiator should be conservative but rekey responder liberal in its window acceptance.

jonasschnelli commented at 6:07 PM on May 6, 2020:

Yes. That could help.

in src/net.h:829 in e04eddb397 outdated

 702 | @@ -703,21 +703,36 @@ class V1TransportDeserializer final : public TransportDeserializer
 703 |      CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) override;
 704 |  };
 705 |  
 706 | +// ChaCha20 must never reuse a {key, nonce} for encryption nor may it be
 707 | +// used to encrypt more than 2^70 bytes under the same {key, nonce}
 708 | +// Re-key after 1GB (RFC4253 / SSH recommendation) or after 1h

ariard commented at 9:42 AM on May 2, 2020:

I don't know about relying on timeclock for rekey. I know it comes from SSH recommendation but it's a client-server architecture, where a connection closing due to clock not being synchronized isn't dramatic.

I'm worried about some clock manipulation to interfere with outbound connections and start some eclipse escalation. Current timedata (bug) logic seems to prevent any exploitation by third-party peers but I wouldn't add that much another assumption on your local clock. NTP attacks have been studied (http://www.cs.bu.edu/~goldbe/papers/NTPattack.pdf), where targeting bitcoin nodes was explicitly quoted...

jonasschnelli commented at 6:11 PM on May 6, 2020:

This is a valid point... would you be interested to analyse attack possibilities? and what would you suggest? Dropping the time based limit?

ariard commented at 6:59 AM on May 8, 2020:

Yes actually time-clock attacks has caught my attention while working on other thing so I'm already investigating this. May I come back to you soon on this and if I don't feel free to ping me ?

As a robust fix we can't rely on receiver-side tracking, like requiring we sent at least X bytes before a re-keying, I think there is protocol messages to triggering querying from us. And relying on block tip increase would be bad due to variance...

ariard commented at 8:02 AM on May 8, 2020:

Thinking further, I would keep the timeclock for triggering a re-key but not checking at re-key acceptance. It would obtain the effect aimed, which it is renew key after 1h. A time-clock manipulation that way wouldn't be able to close connections, just triggering re-key ?

And for a malicious connected peer, it would make the DoS-cpu vector the same that an attacker retrying and abandoning session, and we agreed in BIP discussion that's something we have to bear ?

ariard commented at 9:56 AM on May 2, 2020: member

Have you considered taking in its own PR the rekey part ? Getting right accounting and DoS prevention are maybe worthy of their own attention.

in src/net.cpp:950 in e04eddb397 outdated

 869 | +            m_bytes_decrypted = 0;
 870 | +            m_time_last_rekey = now;
 871 | +        }
 872 | +    } else if (m_bytes_decrypted > REKEY_ABORT_LIMIT_BYTES || now - m_time_last_rekey > REKEY_ABORT_LIMIT_TIME ||
 873 | +               (gArgs.GetBoolArg("-netencryptionfastrekey", false) && m_bytes_decrypted > 64 * 1024)) {
 874 | +        // don't further decrypt and therefore abort connection when counterparty failed to respect rekey limits

ariard commented at 10:02 AM on May 2, 2020:

Where do we abort connection based on invalid checksum and header? It's only checked in ProcessMessages but doesn't trigger a connection AFAICT?

jonasschnelli commented at 6:10 PM on May 6, 2020:

Yes. We don't right now as this is currently identical to V1 (where we don't disconnect if a message has an invalid header or an invalid SHA256 checksum). See #15206 and #15197.

ariard commented at 6:42 AM on May 8, 2020:

Thanks, will review them with context.

in src/net.h:778 in d9081b32a3 outdated

 730 | +    unsigned int m_hdr_pos = 0;     // read pos in header
 731 | +    unsigned int m_data_pos = 0;    // read pos in data
 732 | +    bool m_rekey_flag = false;      // rekey in message detected
 733 | +
 734 | +public:
 735 | +    V2TransportDeserializer(const CPrivKey& k1, const CPrivKey& k2, const uint256& session_id) : m_aead(new ChaCha20Poly1305AEAD(k1.data(), k1.size(), k2.data(), k2.size())), m_aead_k1(k1), m_aead_k2(k2), m_time_last_rekey(GetTime()), m_session_id(session_id), vRecv(SER_NETWORK, INIT_PROTO_VERSION)

jonasschnelli commented at 4:04 PM on May 6, 2020:

AFAIK clang-format "approved" (not changed) this.

jonasschnelli force-pushed on May 6, 2020

jonasschnelli commented at 8:43 AM on May 8, 2020: contributor

I found the issue revealed by the fuzzer crash (reported by @jonatack).

The fuzzer assertion assert(msg.m_raw_message_size == header_size + msg.m_message_size); assumes that the msg.m_message_size never contains the size of the header (in V2, the header is the MAC & the AD [AD == encrypted message length]).

Though, if the decryption failed (due to a valid size message but an invalid MAC), the code did not reduce the header size from msg.m_message_size and thus violating the TransportDeserializer protocol.

The just added commit d1a000d70aa6eccc45a8f7062c4f835073f625ed fixes this.

jonasschnelli commented at 8:46 AM on May 8, 2020: contributor

I added another commit (222d5334681c517636c933c9491200fbbabf3c8d) that fixes the issue reported by @ariard (https://github.com/bitcoin/bitcoin/pull/18242#discussion_r418933381). The bytes counter for detecting a violation of the 1GB rekey limit was not correctly incremented. In case a valid size message had an invalid MAC, the bytes counter was not incremented leading to the problem that a flooding of messages with invalid MACs will not trigger the transmition-limit rekey.

jonatack commented at 1:00 PM on May 9, 2020: member

Ran the new fuzzer for 24 hours with the qa-assets seeds, which previously crashed immediately (https://github.com/bitcoin/bitcoin/pull/18242#pullrequestreview-404120272)... looks good after 5.5M execs.

$ src/test/fuzz/p2p_v2_transport_deserializer ../qa-assets/fuzz_seed_corpus/

[#5332942](/bitcoin-bitcoin/5332942/)	REDUCE cov: 3039 ft: 6681 corp: 55/10942Kb exec/s: 58 rss: 712Mb L: 738789/1040651 MS: 1 EraseBytes-
[#5369365](/bitcoin-bitcoin/5369365/)	REDUCE cov: 3039 ft: 6681 corp: 55/10940Kb exec/s: 58 rss: 712Mb L: 736365/1040651 MS: 3 CMP-PersAutoDict-EraseBytes- DE: "\x0d\x00\x00\x00"-"\x01\x00\x00\x00\x00\x00\x00\x00"-
[#5552616](/bitcoin-bitcoin/5552616/)	REDUCE cov: 3039 ft: 6681 corp: 55/10939Kb exec/s: 58 rss: 712Mb L: 751449/1040651 MS: 1 CrossOver-

in src/net.cpp:840 in 222d533468 outdated

 815 | +
 816 | +            // use direct read since we already read the varlens size uint8_t
 817 | +            command_name.resize(size_or_shortid);
 818 | +            vRecv.read(&command_name[0], size_or_shortid);
 819 | +        }
 820 | +        // try for short ID in case the first byte is a number larger than 12

jonatack commented at 5:31 PM on May 9, 2020:

nit/note for follow-up:

-        // try for short ID in case the first byte is a number larger than 12
+        // try for short ID in case the first byte is a number larger than NET_P2P_V2_CMD_MAX_CHARS_SIZE

rajarshimaitra commented at 3:50 PM on June 16, 2020:

IMO it feels easier to read if variables values are explicitly specified in comments. I tend to check variable values all the time when reading using the editor functionality. That goes away when it's in a comment.

This is in regard to this comment in case git doesn't show it in its intended place.

jonatack commented at 5:56 PM on May 9, 2020: member

ACK 222d5334681c517636c933c9491200fbbabf3c8d code review, built/ran tests, fuzzed for 24 hours as described in #18242 (comment). Could squash the 2 fix commits into their parents, happy to re-ACK in that case. There will be follow-ups as described in the PR description and from the review and BIP feedback, possibly also related to MACing the length; the changes here are only used in the added tests and I think it can be merged.

<details><summary>changes since my previous review took some of the review feedback and fixed 2 bugs found in review and fuzzing</summary>

$ git diff d9081b3 222d533
diff --git a/src/net.cpp b/src/net.cpp
index 80f34dc8db..b8589b29aa 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -81,6 +81,8 @@ const std::string NET_MESSAGE_COMMAND_OTHER = "*other*";
 
 static const uint64_t RANDOMIZER_ID_NETGROUP = 0x6c0edd8036ef4036ULL; // SHA256("netgroup")[0:8]
 static const uint64_t RANDOMIZER_ID_LOCALHOSTNONCE = 0xd93e69e2bbfa5735ULL; // SHA256("localhostnonce")[0:8]
+
+static constexpr uint8_t NET_P2P_V2_CMD_MAX_CHARS_SIZE = 12; //maximal length for V2 (BIP324) string message commands
 //
 // Global state variables
 //
@@ -760,6 +762,7 @@ int V2TransportDeserializer::Read(const char* pch, unsigned int bytes)
 
         return copy_bytes;
     } else {
+        // Read the message data (command, payload & MAC)
         const unsigned int remaining = m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN - m_data_pos;
         const unsigned int copy_bytes = std::min(remaining, bytes);
 
@@ -787,13 +790,13 @@ CNetMessage V2TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
     bool valid_header = false;
     std::string command_name;
 
+    // count bytes we decrypted including MAC tag + AD
+    m_bytes_decrypted += vRecv.size();
+
     if (m_aead->Crypt(m_payload_seqnr, m_aad_seqnr, m_aad_pos, (unsigned char*)vRecv.data(), vRecv.size(), (const uint8_t*)vRecv.data(), vRecv.size(), false)) {
         // MAC check was successful
         valid_checksum = true;
 
-        // count bytes we decrypted including MAC tag + AD
-        m_bytes_decrypted += vRecv.size();
-
         // okay, we could decrypt it, now remove packet length and MAC tag
         assert(vRecv.size() > CHACHA20_POLY1305_AEAD_AAD_LEN + CHACHA20_POLY1305_AEAD_TAG_LEN);
         // CDataStream::erase at the begin will just increase the read pos
@@ -806,15 +809,15 @@ CNetMessage V2TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
         } catch (const std::ios_base::failure&) {
             LogPrint(BCLog::NET, "Invalid command name\n");
         }
-        if (size_or_shortid > 0 && size_or_shortid <= 12 && vRecv.size() >= size_or_shortid) {
-            // string command
+        if (size_or_shortid > 0 && size_or_shortid <= NET_P2P_V2_CMD_MAX_CHARS_SIZE && vRecv.size() >= size_or_shortid) {
+            // first byte is a number between 1 and 12. Must be a string command.
             valid_header = true;
 
             // use direct read since we already read the varlens size uint8_t
             command_name.resize(size_or_shortid);
             vRecv.read(&command_name[0], size_or_shortid);
         }
-        // try for short ID
+        // try for short ID in case the first byte is a number larger than 12
         if (!valid_header && size_or_shortid >= 0) {
             valid_header = true;
             if (!GetCommandFromShortCommandID(size_or_shortid, command_name)) {
@@ -842,8 +845,10 @@ CNetMessage V2TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
     msg.m_valid_checksum = valid_checksum;
     msg.m_valid_netmagic = true; // not relevant for v2, always pass
 
-    // store command string, payload size, wire message size
-    msg.m_message_size = msg.m_recv.size();                                                                    //message payload size (excluding command)
+    // if we could successfully decrypt the message, the message no longer contains the "header" (AAD & MAC)
+    // if failed to decrypt – which we tolerate at this point – we need to reduce the message size by the length of the AD & MAC
+    // to conform to the abstract TransportDeserializer protocol
+    msg.m_message_size = valid_checksum ? msg.m_recv.size() : (msg.m_recv.size() - CHACHA20_POLY1305_AEAD_AAD_LEN - CHACHA20_POLY1305_AEAD_TAG_LEN);
     msg.m_raw_message_size = CHACHA20_POLY1305_AEAD_AAD_LEN + m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN; // raw wire size
 
     const int64_t now = GetTime();
@@ -903,7 +908,7 @@ void V2TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vec
     const uint8_t cmd_short_id = GetShortCommandIDFromCommand(msg.command);
     if (cmd_short_id == 0) {
         // message command without an assigned short-ID
-        assert(msg.command.size() <= 12);
+        assert(msg.command.size() <= NET_P2P_V2_CMD_MAX_CHARS_SIZE);
         // encode as varstr, max 12 chars
         serialized_command_size = ::GetSerializeSize(msg.command, PROTOCOL_VERSION);
     }
diff --git a/src/net.h b/src/net.h
index 0123863800..c233da27e5 100644
--- a/src/net.h
+++ b/src/net.h
@@ -712,6 +712,7 @@ static constexpr unsigned int REKEY_ABORT_LIMIT_BYTES = REKEY_LIMIT_BYTES * 1.1;
 static constexpr unsigned int REKEY_ABORT_LIMIT_TIME = REKEY_LIMIT_BYTES * 1.1;  // abort after ~10% tolerance buffer
 static constexpr unsigned int MIN_REKEY_TIME = 10;                               // minimal rekey time to avoid DOS
 
+// V2TransportDeserializer is a transport deserializer after BIP324
 class V2TransportDeserializer : public TransportDeserializer
 {
 private:
diff --git a/src/protocol.cpp b/src/protocol.cpp
index 9da40d6048..fb6a7e28f9 100644
--- a/src/protocol.cpp
+++ b/src/protocol.cpp
@@ -206,7 +206,7 @@ const std::vector<std::string> &getAllNetMessageTypes()
 }
 
 
-uint8_t GetShortCommandIDFromCommand(const std::string cmd)
+uint8_t GetShortCommandIDFromCommand(const std::string& cmd)
 {
     if (cmd == NetMsgType::ADDR) {
         return NetMsgType::ADDR_SHORT_ID;
diff --git a/src/protocol.h b/src/protocol.h
index c5fa48713f..2c230f347f 100644
--- a/src/protocol.h
+++ b/src/protocol.h
@@ -269,7 +269,7 @@ const std::vector<std::string> &getAllNetMessageTypes();
 
 // returns the short command ID for a command
 // returns 0 if no short command ID has been found
-uint8_t GetShortCommandIDFromCommand(const std::string cmd);
+uint8_t GetShortCommandIDFromCommand(const std::string& cmd);
 
 // returns the command (string) from a short command ID
 // returns an empty string if short command ID has not been found

</details>

DrahtBot added the label Needs rebase on May 12, 2020

jonasschnelli commented at 7:37 AM on May 13, 2020: contributor

Rebased.

jonasschnelli force-pushed on May 13, 2020

DrahtBot removed the label Needs rebase on May 13, 2020

in src/net.cpp:839 in b9468fa767 outdated

 817 | +            // use direct read since we already read the varlens size uint8_t
 818 | +            command_name.resize(size_or_shortid);
 819 | +            vRecv.read(&command_name[0], size_or_shortid);
 820 | +        }
 821 | +        // try for short ID in case the first byte is a number larger than 12
 822 | +        if (!valid_header && size_or_shortid >= 0) {

jonatack commented at 9:47 PM on May 13, 2020:

size_or_shortid is uint8_t. Since it is unsigned, size_or_shortid >= 0 will always be true. Should this be if (!valid_header && size_or_shortid > 0) {?

jonasschnelli commented at 6:44 AM on May 14, 2020:

Not sure. If size_or_shortid is 0 (due to a std::ios_base::failure or if vRecv contain 0), I'd like to enter this if to set command_name to unknown-0. Otherwise command_name would be unset for a size of 0 (which is somehow a undefined short-id).

jonatack commented at 1:45 PM on May 15, 2020:

Ok, if I understand correctly:

-    bool valid_header = false;
     std::string command_name;
@@ -812,15 +811,12 @@ CNetMessage V2TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
         }
         if (size_or_shortid > 0 && size_or_shortid <= NET_P2P_V2_CMD_MAX_CHARS_SIZE && vRecv.size() >= size_or_shortid) {
             // first byte is a number between 1 and 12. Must be a string command.
-            valid_header = true;
 
             // use direct read since we already read the varlens size uint8_t
             command_name.resize(size_or_shortid);
             vRecv.read(&command_name[0], size_or_shortid);
-        }
-        // try for short ID in case the first byte is a number larger than 12
-        if (!valid_header && size_or_shortid >= 0) {
-            valid_header = true;
+        } else if (size_or_shortid == 0 || size_or_shortid > NET_P2P_V2_CMD_MAX_CHARS_SIZE) {
+            // // try for short ID in case the first byte is a number larger than NET_P2P_V2_CMD_MAX_CHARS_SIZE
             if (!GetCommandFromShortCommandID(size_or_shortid, command_name)) {
@@ -842,7 +838,7 @@ CNetMessage V2TransportDeserializer::GetMessage(const CMessageHeader::MessageSta
     msg.m_command = command_name;
 
     // store state about valid header, netmagic and checksum
-    msg.m_valid_header = valid_header; // not relevant for v2, always pass
+    msg.m_valid_header = true ; // not relevant for v2, always pass

jonasschnelli commented at 7:00 AM on May 18, 2020:

What if the buffer contains a 0x09 as first bytes (a valid string based command with the size of 9 chars) but those 9 bytes would actually not follow? Wouldn't it become valid_header=true in your suggestions?

jonatack commented at 3:14 PM on May 18, 2020:

Unless I'm confused, I think the suggestion above is still correct for the case you describe, assuming you want it to fail both of the conditional checks, and net_tests and the p2p_v2_transport_deserializer fuzzer both pass.

In the current code, it's not clear to me what purpose valid_header serves because it always becomes true at the latest by line 823.

Unrelated, but it looks like short command IDs need to be added for the GETCFCHECKPT and CFCHECKPT BIP157 message types that were added in f9e00bb25ac and possibly soon for GETCFHEADERS and GETCFFILTERS as well.

narula commented at 5:51 PM on June 17, 2020:

Agreed this is a bit confusing, and I'm not exactly sure what you're trying to achieve. size_or_shortid >= 0 will always evaluate to true. valid_header will always eventually be set to true. Under what circumstances do you think valid_header should be false?

jonatack commented at 1:07 PM on August 21, 2020:

When rebuilding, the compiler is warning about this now:

net.cpp: In member function ‘virtual CNetMessage V2TransportDeserializer::GetMessage(const unsigned char (&)[4], std::chrono::microseconds)’:
net.cpp:839:46: warning: comparison is always true due to limited range of data type [-Wtype-limits]
  839 |         if (!valid_header && size_or_shortid >= 0) {
      |                              ~~~~~~~~~~~~~~~~^~~~

jonasschnelli commented at 12:05 PM on August 27, 2020:

I just removed the size_or_shortid >= 0 check (your right,.. it's always true). The (!valid_header) check is necessary though,... in case vRecv >> size_or_shortid failed (can't happen?!) or if it contains a value greater than 12 or if below 12, the remaining buffer doesn't have the expected size.

DrahtBot added the label Needs rebase on May 20, 2020

jonasschnelli force-pushed on May 21, 2020

jonasschnelli commented at 10:32 AM on May 21, 2020: contributor

Rebased (only code formatting conflicts in protocol.h).

DrahtBot removed the label Needs rebase on May 21, 2020

DrahtBot added the label Needs rebase on May 29, 2020

dongcarl commented at 4:30 PM on June 2, 2020: member

Rebased this PR for you :relaxed:: https://github.com/bitcoin/bitcoin/compare/master...dongcarl:2020-06-net_v2-rebased

jonasschnelli force-pushed on Jun 2, 2020

jonasschnelli commented at 6:00 PM on June 2, 2020: contributor

Thanks @dongcarl. Was a trivial closing bracket rebase. Rebase pushed.

DrahtBot removed the label Needs rebase on Jun 2, 2020

MarcoFalke referenced this in commit f154071ec8 on Jun 12, 2020

sidhujag referenced this in commit d5007c520a on Jun 13, 2020

in src/crypto/chacha_poly_aead.h:15 in 7aa91cd136 outdated

  10 | @@ -11,6 +11,7 @@
  11 |  
  12 |  static constexpr int CHACHA20_POLY1305_AEAD_KEY_LEN = 32;
  13 |  static constexpr int CHACHA20_POLY1305_AEAD_AAD_LEN = 3; /* 3 bytes length */
  14 | +static constexpr int CHACHA20_POLY1305_AEAD_TAG_LEN = 16; /* 16 bytes poly1305 tag */

rajarshimaitra commented at 7:45 AM on June 16, 2020:

poly1305 tag length of 16 bytes is already defined in https://github.com/bitcoin/bitcoin/blob/1c86ed41483471929840eec09b93d7de3a4aeacf/src/crypto/poly1305.h#L12 Is it necessary to explicitly define it here again?

jonasschnelli commented at 12:25 PM on August 12, 2020:

I prefer to keep the AEAD contents separate.

in src/net.cpp:789 in 7aa91cd136 outdated

 784 | +
 785 | +        return copy_bytes;
 786 | +    }
 787 | +}
 788 | +
 789 | +CNetMessage V2TransportDeserializer::GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time)

rajarshimaitra commented at 2:06 PM on June 16, 2020:

BIP324 removes the Message Magic bytes. Is CMessageHeader::MessageStartChars required here?

jonasschnelli commented at 12:27 PM on August 12, 2020:

The transport deserializer(s) use an abstract class for the runtime flexibility. The network magic is required for V1. I think it makes no sense to try to get rid of it here as long as V1 is supported.

in src/net.cpp:811 in 7aa91cd136 outdated

 795 | +    // we'll always return a CNetMessage (even if encryption fails), we always increase the AEAD sequence numbers
 796 | +    bool valid_checksum = false;
 797 | +    bool valid_header = false;
 798 | +    std::string command_name;
 799 | +
 800 | +    // count bytes we decrypted including MAC tag + AD

rajarshimaitra commented at 2:30 PM on June 16, 2020:

Nit: Should be MAC tag + AAD?

jonasschnelli commented at 12:30 PM on August 12, 2020:

Fixed.

in src/net.cpp:806 in 7aa91cd136 outdated

 790 | +{
 791 | +    // In v2, vRecv contains the encrypted payload plus the MAC tag (1+bytes serialized message command + ? bytes message payload + 16 byte mac tag)
 792 | +    assert(Complete());
 793 | +
 794 | +    // defensive decoding (MAC check, decryption, command deserialization)
 795 | +    // we'll always return a CNetMessage (even if encryption fails), we always increase the AEAD sequence numbers

rajarshimaitra commented at 2:31 PM on June 16, 2020:

Nit: should be (even if decryption fails)?

jonasschnelli commented at 12:30 PM on August 12, 2020:

Fixed.

in src/net.cpp:848 in 7aa91cd136 outdated

 815 | +            vRecv >> size_or_shortid;
 816 | +        } catch (const std::ios_base::failure&) {
 817 | +            LogPrint(BCLog::NET, "Invalid command name\n");
 818 | +        }
 819 | +        if (size_or_shortid > 0 && size_or_shortid <= NET_P2P_V2_CMD_MAX_CHARS_SIZE && vRecv.size() >= size_or_shortid) {
 820 | +            // first byte is a number between 1 and 12. Must be a string command.

rajarshimaitra commented at 3:47 PM on June 16, 2020:

Nit:

         // first byte is a number between 1 and 12. Must be a string command.
        if (size_or_shortid > 0 && size_or_shortid <= NET_P2P_V2_CMD_MAX_CHARS_SIZE && vRecv.size() >= size_or_shortid) {

Putting the comment above the if statement seems more appropriate like it's done for short id.

in src/net.cpp:867 in 7aa91cd136 outdated

 851 | +    msg.m_valid_header = valid_header; // not relevant for v2, always pass
 852 | +    msg.m_valid_checksum = valid_checksum;
 853 | +    msg.m_valid_netmagic = true; // not relevant for v2, always pass
 854 | +
 855 | +    // if we could successfully decrypt the message, the message no longer contains the "header" (AAD & MAC)
 856 | +    // if failed to decrypt – which we tolerate at this point – we need to reduce the message size by the length of the AD & MAC

rajarshimaitra commented at 4:02 PM on June 16, 2020:

Nit: AAD

jonasschnelli commented at 12:33 PM on August 12, 2020:

fixed

in src/net.cpp:899 in 7aa91cd136 outdated

 917 | +        // message command without an assigned short-ID
 918 | +        assert(msg.command.size() <= NET_P2P_V2_CMD_MAX_CHARS_SIZE);
 919 | +        // encode as varstr, max 12 chars
 920 | +        serialized_command_size = ::GetSerializeSize(msg.command, PROTOCOL_VERSION);
 921 | +    }
 922 | +    // prepare the packet length that will later be encrypted and part of the MAC (AD)

rajarshimaitra commented at 9:19 AM on June 17, 2020:

Nit: AAD

in src/net.cpp:937 in 7aa91cd136 outdated

 921 | +    }
 922 | +    // prepare the packet length that will later be encrypted and part of the MAC (AD)
 923 | +    // the packet length excludes the 16 byte MAC tag
 924 | +    uint32_t packet_length = serialized_command_size + msg.data.size();
 925 | +
 926 | +    // prepare the packet length & message command and reserve 4 bytes (3bytes AD + 1byte short-ID)

rajarshimaitra commented at 9:20 AM on June 17, 2020:

Nit: AAD. At this point, I am wondering if it's intentional, if it is then ignore. There are many more, Not commenting the same for the rests.

in src/net.cpp:999 in 7aa91cd136 outdated

 983 | +        // reset the AEAD context
 984 | +        m_aead.reset(new ChaCha20Poly1305AEAD(m_aead_k1.data(), m_aead_k1.size(), m_aead_k2.data(), m_aead_k2.size()));
 985 | +        LogPrint(BCLog::NET, "Rekey: new send keys (%s, %s)\n", HexStr(m_aead_k1), HexStr(m_aead_k2));
 986 | +
 987 | +        // reset the AEAD context
 988 | +        m_aead.reset(new ChaCha20Poly1305AEAD(m_aead_k1.data(), m_aead_k1.size(), m_aead_k2.data(), m_aead_k2.size()));

rajarshimaitra commented at 10:09 AM on June 17, 2020:

Why reset again?

jonasschnelli commented at 12:35 PM on August 12, 2020:

Oops. Rebase issue. Fixed.

in src/net.cpp:889 in 7aa91cd136 outdated

 908 | @@ -738,6 +909,95 @@ void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vec
 909 |      CVectorWriter{SER_NETWORK, INIT_PROTO_VERSION, header, 0, hdr};
 910 |  }
 911 |  
 912 | +void V2TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header)

rajarshimaitra commented at 10:27 AM on June 17, 2020:

it seems the header is redundant here?

jonasschnelli commented at 12:36 PM on August 12, 2020:

Same issue as this. It's required for V1.

in src/net.cpp:941 in 7aa91cd136 outdated

 872 | +
 873 | +            // reset the AEAD context
 874 | +            m_aead.reset(new ChaCha20Poly1305AEAD(m_aead_k1.data(), m_aead_k1.size(), m_aead_k2.data(), m_aead_k2.size()));
 875 | +            LogPrint(BCLog::NET, "Rekey: new recv keys (%s, %s)\n", HexStr(m_aead_k1), HexStr(m_aead_k2));
 876 | +
 877 | +            // reset sequence numbers

rajarshimaitra commented at 11:13 AM on June 17, 2020:

//reset sequence numbers and key counters? Or maybe add //reset key counters before last two? like done in prepareForTransport.

in src/net.cpp:1022 in 7aa91cd136 outdated

 953 | +    assert(!bit24);
 954 | +
 955 | +    // check if we should rekey after this message
 956 | +    const int64_t now = GetTime(); //TODO: check how expansive the GetTime call is and if it is avoidable
 957 | +    bool rekey = false;
 958 | +    if (m_bytes_encrypted >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 32 * 1024 : REKEY_LIMIT_BYTES) || now - m_time_last_rekey >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 10 : REKEY_LIMIT_TIME)) {

rajarshimaitra commented at 11:27 AM on June 17, 2020:

As per the comment on net_tests.cpp line 380

// Setting the netencryptionfastrekey flag results in using a threshold of 64kb / 10 seconds for requiring a rekey

Here m_bytes_encrypted is checked against 32 kb. Is this intentional? Should the BIP draft add netencryptionfastrekey policy for better reference? Also whats the rationale behind fastrekey?

jonasschnelli commented at 12:38 PM on August 12, 2020:

The fast rekey function is not something that needs to be specified in the BIP. The purpose of it is for pure implementation testability and has nothing to do with the specification.

in src/net.h:765 in 7aa91cd136 outdated

 715 | +private:
 716 | +    std::unique_ptr<ChaCha20Poly1305AEAD> m_aead;
 717 | +    CPrivKey m_aead_k1; //keep the keys for later rekeying
 718 | +    CPrivKey m_aead_k2;
 719 | +    uint64_t m_bytes_decrypted = 0; // counter of bytes decrypted under the same key
 720 | +    int64_t m_time_last_rekey = 0;

rajarshimaitra commented at 11:38 AM on June 17, 2020:

Suggested comment (in the spirit of the rest) // recorded time when the last rekey happened

jonasschnelli commented at 12:39 PM on August 12, 2020:

Thanks. Fixed.

in src/net.h:804 in 7aa91cd136 outdated

 754 | +    }
 755 | +    void SetVersion(int nVersionIn)
 756 | +    {
 757 | +        vRecv.SetVersion(nVersionIn);
 758 | +    }
 759 | +    bool OversizedMessageDetected() const

rajarshimaitra commented at 11:40 AM on June 17, 2020:

We are already checking m_message_size against MAX_SIZE in Read. Here the check is happening against MAX_PROTOCOL_MESSAGE_LENGTH.

MAX_SIZE = 33.5MB, MAX_PROTOCOL_MESSAGE_LENGTH = 4MB. What does these two limits signify?
if we are already checking for some size limit in Read why have a separate check here? So far OversizedMessageDetected is not used anywhere. Any special plan for its existence?

jonasschnelli commented at 12:50 PM on August 12, 2020:

Good point. I think this got "rebased-away". Just removed OversizedMessageDetected().

in src/net.h:913 in 7aa91cd136 outdated

 787 | +    uint64_t m_bytes_encrypted = 0; //counter of bytes encrypted with same key
 788 | +    int64_t m_time_last_rekey = 0;
 789 | +    uint256 m_session_id; // the encryption session_id, relevant for rekeying
 790 | +    uint64_t m_payload_seqnr = 0;
 791 | +    uint64_t m_aad_seqnr = 0;
 792 | +    int m_aad_pos = 0;

rajarshimaitra commented at 12:04 PM on June 17, 2020:

Comments from V2TransportDeserializer can be copied here.

in src/net.h:833 in 7aa91cd136 outdated

 783 | +private:
 784 | +    std::unique_ptr<ChaCha20Poly1305AEAD> m_aead;
 785 | +    CPrivKey m_aead_k1; //keep the keys for a later rekeying
 786 | +    CPrivKey m_aead_k2;
 787 | +    uint64_t m_bytes_encrypted = 0; //counter of bytes encrypted with same key
 788 | +    int64_t m_time_last_rekey = 0;

rajarshimaitra commented at 12:04 PM on June 17, 2020:

Same comment here from V2TransportDeserializer can help.

in src/protocol.cpp:250 in 7aa91cd136 outdated

 240 | @@ -241,3 +241,126 @@ std::vector<std::string> serviceFlagsToStr(uint64_t flags)
 241 |  
 242 |      return str_flags;
 243 |  }
 244 | +
 245 | +uint8_t GetShortCommandIDFromCommand(const std::string& cmd)

rajarshimaitra commented at 12:10 PM on June 17, 2020:

A single comment specifying this is used for V2 messaging protocol as per BIP324 might help here.

laanwj commented at 2:08 PM on June 18, 2020:

I would prefer to return an Option<uint8_t> here to make it clearer when there is no short id available for a command (instead of special-casing 0).

laanwj commented at 2:14 PM on June 18, 2020:

A single comment specifying this is used for V2 messaging protocol as per BIP324 might help here.

The comment is in the .h file, where it belongs for public functions.

in src/protocol.h:257 in 7aa91cd136 outdated

 290 | @@ -266,6 +291,17 @@ extern const char* CFCHECKPT;
 291 |  /* Get a vector of all valid message types (see above) */
 292 |  const std::vector<std::string>& getAllNetMessageTypes();
 293 |  
 294 | +// Short Command IDs are a low bandwidth representations of a message type
 295 | +// The mapping is a peer to peer agreement

rajarshimaitra commented at 12:21 PM on June 17, 2020:

Is the mapping a p2p agreement at this point? It seems hardcoded. Which seems all right to me. Trying to have the mapping process here seems like going out of scope for this PR. Maybe a followup PR or even a BIP should be done? In that light maybe removing this comment here can help, or tagging it by a todo or something else? Can be confusing as its not implemented yet.

in src/test/net_tests.cpp:960 in 7aa91cd136 outdated

 358 | +            msg.data = msg_orig.data;
 359 | +            msg.command = msg_orig.command;
 360 | +            size_t raw_msg_size = msg.data.size();
 361 | +
 362 | +            std::vector<unsigned char> serialized_header;
 363 | +            serializer->prepareForTransport(msg, serialized_header);

rajarshimaitra commented at 1:25 PM on June 17, 2020:

It seems it is intended here that serialized header will be returned into the serialized_header? But prepareForTransport doesn't assign the serialized header into the header arguement. Not sure what is happening here.

jonasschnelli commented at 1:00 PM on August 12, 2020:

Keep in mind that prepareForTransport must have the flexibility to work with other transport types including the V1 transport.

in src/test/net_tests.cpp:786 in 7aa91cd136 outdated

 333 | +}
 334 | +
 335 | +// use 32 bytey keys with all zeros
 336 | +static const CPrivKey k1(32, 0);
 337 | +static const CPrivKey k2(32, 0);
 338 | +static const uint256 session_id;

rajarshimaitra commented at 1:36 PM on June 17, 2020:

Session IDs are important for rekey calculation. Shouldn't this be initialized to something for better expression of that intent? I can see how a fixed garbage value can work too. Wondering if that was the motivation to keep it uninitialized.

jonasschnelli commented at 12:59 PM on August 12, 2020:

uint256 has a default constructor the sets the memory to all zeros. Should be enough?

in src/test/net_tests.cpp:773 in 7aa91cd136 outdated

 321 | @@ -320,4 +322,161 @@ BOOST_AUTO_TEST_CASE(PoissonNextSend)
 322 |      g_mock_deterministic_tests = false;
 323 |  }
 324 |  
 325 | +size_t read_message(std::unique_ptr<TransportDeserializer>& deserializer, const std::vector<unsigned char>& serialized_header, const CSerializedNetMsg& msg)

rajarshimaitra commented at 1:41 PM on June 17, 2020:

Why read_message needs a serialized_header? Cant it just compute the header from CSerializedNetMsg like it was done in prepareForTransport?

in src/test/net_tests.cpp:431 in 7aa91cd136 outdated

 430 | +        gArgs.ForceSetArg("-netencryptionfastrekey", "1");
 431 | +        read_message(deserializer, serialized_header, test_msg);
 432 | +        CNetMessage msg_deser = deserializer->GetMessage(Params().MessageStart(), GetTimeMicros());
 433 | +
 434 | +        // make sure we detect the failed rekey
 435 | +        // the 76. message (32kb) must have violated the fast rekey limits

rajarshimaitra commented at 2:07 PM on June 17, 2020:

Nit: 76th?

rajarshimaitra commented at 3:31 PM on June 17, 2020: contributor

Strong concept ACK for anything related to BIP324. Very happy to see how fast BIP324 is moving given its huge privacy benefits. Compiled without issue. Verified all unit tests are passing. Below are a few nits and comments requiring some explanations.

in src/net.h:844 in d55f4725c4 outdated

 775 | +public:
 776 | +    V2TransportSerializer(const CPrivKey& k1, const CPrivKey& k2) : m_aead(new ChaCha20Poly1305AEAD(k1.data(), k1.size(), k2.data(), k2.size())), m_aead_k1(k1), m_aead_k2(k2)
 777 | +    {
 778 | +    }
 779 | +    // prepare for next message
 780 | +    void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header);

narula commented at 5:24 PM on June 17, 2020:

Add override keyword to be consistent with V1TransportSerializer (the same conditions apply here as well, no?)

jonasschnelli commented at 1:07 PM on August 12, 2020:

Right. Thanks. Fixed.

in src/net.cpp:904 in d55f4725c4 outdated

 877 | +    // prepare the packet length that will later be encrypted and part of the MAC (AD)
 878 | +    // the packet length excludes the 16 byte MAC tag
 879 | +    uint32_t packet_length = serialized_command_size + msg.data.size();
 880 | +
 881 | +    // prepare the packet length & message command and reserve 4 bytes (3bytes AD + 1byte short-ID)
 882 | +    std::vector<unsigned char> serialized_header(CHACHA20_POLY1305_AEAD_AAD_LEN + 1);

narula commented at 5:54 PM on June 17, 2020:

I think this should be header.reserve(CHACHA20_POLY1305_AEAD_AAD_LEN + 1); and all instances of serialized_header should be replaced with header.

narula commented at 6:20 PM on June 17, 2020:

Note, after I changed this, the V2 tests fail:

neha@mumford:~/src/bitcoin (HEAD detached at e13accd43a)$ src/test/test_bitcoin -t net_tests -l test_suite
Running 11 test cases...
Entering test module "Bitcoin Core Test Suite"
test/net_tests.cpp(83): Entering test suite "net_tests"
test/net_tests.cpp(85): Entering test case "cnode_listen_port"
test/net_tests.cpp(85): Leaving test case "cnode_listen_port"; testing time: 17203us
test/net_tests.cpp(97): Entering test case "caddrdb_read"
test/net_tests.cpp(97): Leaving test case "caddrdb_read"; testing time: 3942us
test/net_tests.cpp(143): Entering test case "caddrdb_read_corrupted"
test/net_tests.cpp(143): Leaving test case "caddrdb_read_corrupted"; testing time: 3158us
test/net_tests.cpp(173): Entering test case "cnode_simple_test"
test/net_tests.cpp(173): Leaving test case "cnode_simple_test"; testing time: 3249us
test/net_tests.cpp(198): Entering test case "ipv4_peer_with_ipv6_addrMe_test"
test/net_tests.cpp(198): Leaving test case "ipv4_peer_with_ipv6_addrMe_test"; testing time: 2877us
test/net_tests.cpp(236): Entering test case "LimitedAndReachable_Network"
test/net_tests.cpp(236): Leaving test case "LimitedAndReachable_Network"; testing time: 2849us
test/net_tests.cpp(259): Entering test case "LimitedAndReachable_NetworkCaseUnroutableAndInternal"
test/net_tests.cpp(259): Leaving test case "LimitedAndReachable_NetworkCaseUnroutableAndInternal"; testing time: 2807us
test/net_tests.cpp(282): Entering test case "LimitedAndReachable_CNetAddr"
test/net_tests.cpp(282): Leaving test case "LimitedAndReachable_CNetAddr"; testing time: 2843us
test/net_tests.cpp(296): Entering test case "LocalAddress_BasicLifecycle"
test/net_tests.cpp(296): Leaving test case "LocalAddress_BasicLifecycle"; testing time: 2845us
test/net_tests.cpp(310): Entering test case "PoissonNextSend"
test/net_tests.cpp(310): Leaving test case "PoissonNextSend"; testing time: 2813us
test/net_tests.cpp(371): Entering test case "net_v2"
test/net_tests.cpp(361): error: in "net_tests/net_v2": check deserializer->Complete() has failed
test/net_tests.cpp(362): error: in "net_tests/net_v2": check read_bytes == msg.data.size() + serialized_header.size() has failed [23 != 24]
test_bitcoin: net.cpp:783: virtual CNetMessage V2TransportDeserializer::GetMessage(const unsigned char (&)[4], int64_t): Assertion `Complete()' failed.
unknown location(0): fatal error: in "net_tests/net_v2": signal: SIGABRT (application abort requested)
test/net_tests.cpp(362): last checkpoint
test/net_tests.cpp(371): Leaving test case "net_v2"; testing time: 3128us
test/net_tests.cpp(83): Leaving test suite "net_tests"; testing time: 47830us
Leaving test module "Bitcoin Core Test Suite"; testing time: 47866us

*** 3 failures are detected in the test module "Bitcoin Core Test Suite"

</details>

jonasschnelli commented at 1:12 PM on August 12, 2020:

I see your point. The pass-by-reference header field in prepareForTransport() is only used in V1. In V2, there is actually no header (well, you could argue that the MAC-tag and the encrypted length is the header but since the MAC is at the end of the message, it makes little sense to try to use that header field).

Therefor, in V2, the pass-by-reference header field is unused and the whole message (including the AAD & MAC) is part of the message data/payload (which I think is conceptual fine).

in src/protocol.cpp:310 in a2931c174e outdated

 299 | +    } else if (cmd == NetMsgType::TX) {
 300 | +        return NetMsgType::TX_SHORT_ID;
 301 | +    } else if (cmd == NetMsgType::VERACK) {
 302 | +        return NetMsgType::VERACK_SHORT_ID;
 303 | +    } else if (cmd == NetMsgType::VERSION) {
 304 | +        return NetMsgType::VERSION_SHORT_ID;

laanwj commented at 2:09 PM on June 18, 2020:

It'd definitely result in shorter code use a hash table here.

jonasschnelli commented at 1:12 PM on August 12, 2020:

Switched to a std::map.

in src/protocol.cpp:268 in a2931c174e outdated

 257 | +        return NetMsgType::FEEFILTER_SHORT_ID;
 258 | +    } else if (cmd == NetMsgType::FILTERADD) {
 259 | +        return NetMsgType::FILTERADD_SHORT_ID;
 260 | +    } else if (cmd == NetMsgType::FILTERCLEAR) {
 261 | +        return NetMsgType::FILTERCLEAR_SHORT_ID;
 262 | +        ;

laanwj commented at 2:10 PM on June 18, 2020:

Why an empty statement here? (some more below)

jonasschnelli commented at 1:13 PM on August 12, 2020:

Rebase issues. Fixed.

in src/protocol.h:259 in a2931c174e outdated

 290 | @@ -266,6 +291,17 @@ extern const char* CFCHECKPT;
 291 |  /* Get a vector of all valid message types (see above) */
 292 |  const std::vector<std::string>& getAllNetMessageTypes();
 293 |  
 294 | +// Short Command IDs are a low bandwidth representations of a message type
 295 | +// The mapping is a peer to peer agreement
 296 | +
 297 | +// returns the short command ID for a command

laanwj commented at 2:14 PM on June 18, 2020:

Please use doxygen-compatible comments e.g.

/** returns the short command ID for a command…
 */

jonasschnelli commented at 1:15 PM on August 12, 2020:

Fixed.

in src/protocol.h:277 in a2931c174e outdated

 298 | +// returns 0 if no short command ID has been found
 299 | +uint8_t GetShortCommandIDFromCommand(const std::string& cmd);
 300 | +
 301 | +// returns the command (string) from a short command ID
 302 | +// returns an empty string if short command ID has not been found
 303 | +bool GetCommandFromShortCommandID(uint8_t shortID, std::string& cmd);

fjahr commented at 2:41 PM on June 23, 2020:

I do prefer the current order here because I find it more intuitive to have the input first and then the output. But I think in GetCommandFromShortCommandID the input (shortID) should be const and casing should be fixed to short_id. nit: Overall I would prefer a more consistent API between the two functions so that they have the same return type:

// returns the short command ID for a command (string)
// returns 0 if short command ID was not found
bool GetShortCommandIDFromCommand(const std::string& cmd, uint8_t short_id);

// returns the command (string) from a short command ID
// returns an empty string if short command ID has not been found
bool GetCommandFromShortCommandID(const uint8_t short_id, std::string& cmd);

in src/test/net_tests.cpp:992 in e13accd43a outdated

 373 | +    // create some messages where we perform serialization and deserialization
 374 | +    std::vector<CSerializedNetMsg> test_msgs;
 375 | +    test_msgs.push_back(CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK));
 376 | +    test_msgs.push_back(CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, (int)NODE_NETWORK, 123, CAddress(CService(), NODE_NONE), CAddress(CService(), NODE_NONE), 123, "foobar", 500000, true));
 377 | +    test_msgs.push_back(CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::PING, 123456));
 378 | +    CDataStream stream(ParseHex("020000000001013107ca31e1950a9b44b75ce3e8f30127e4d823ed8add1263a1cc8adcc8e49164000000001716001487835ecf51ea0351ef266d216a7e7a3e74b84b4efeffffff02082268590000000017a9144a94391b99e672b03f56d3f60800ef28bc304c4f8700ca9a3b0000000017a9146d5df9e79f752e3c53fc468db89cafda4f7d00cb87024730440220677de5b11a5617d541ba06a1fa5921ab6b4509f8028b23f18ab8c01c5eb1fcfb02202fe382e6e87653f60ff157aeb3a18fc888736720f27ced546b0b77431edabdb0012102608c772598e9645933a86bcd662a3b939e02fb3e77966c9713db5648d5ba8a0006010000"), SER_NETWORK, PROTOCOL_VERSION);

fjahr commented at 2:51 PM on June 23, 2020:

nit: The dummy message seems to be reused several times, could be a constant.

in src/net.h:757 in d55f4725c4 outdated

 699 | @@ -698,6 +700,54 @@ class V1TransportDeserializer final : public TransportDeserializer
 700 |      CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, int64_t time) override;
 701 |  };
 702 |  
 703 | +// V2TransportDeserializer is a transport deserializer after BIP324

fjahr commented at 3:15 PM on June 23, 2020:

nit: Should be doxygen comment I think.

in src/net.h:891 in d55f4725c4 outdated

 761 | @@ -712,6 +762,24 @@ class V1TransportSerializer  : public TransportSerializer {
 762 |      void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) override;
 763 |  };
 764 |  
 765 | +class V2TransportSerializer : public TransportSerializer

fjahr commented at 3:16 PM on June 23, 2020:

nit: Could also add a doxygen comment here.

in src/test/net_tests.cpp:414 in 9ff5cec2cf outdated

 413 | +    CSerializedNetMsg test_msg = CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERSION, PROTOCOL_VERSION, (int)NODE_NETWORK, 123, CAddress(CService(), NODE_NONE), CAddress(CService(), NODE_NONE), 123, "foobar", 500000, true);
 414 | +    CSerializedNetMsg test_msg_short = CNetMsgMaker(INIT_PROTO_VERSION).Make(NetMsgType::VERACK);
 415 | +
 416 | +    // make sure we use the fast rekey rules
 417 | +    std::unique_ptr<TransportSerializer> serializer = MakeUnique<V2TransportSerializer>(V2TransportSerializer(k1, k2, session_id));
 418 | +    ;

fjahr commented at 3:26 PM on June 23, 2020:

remove empty statement

in src/test/net_tests.cpp:327 in e13accd43a outdated

 321 | @@ -320,4 +322,77 @@ BOOST_AUTO_TEST_CASE(PoissonNextSend)
 322 |      g_mock_deterministic_tests = false;
 323 |  }
 324 |  
 325 | +void message_serialize_deserialize_test(bool v2, const std::vector<CSerializedNetMsg>& test_msgs)
 326 | +{
 327 | +    // use 32 bytey keys with all zeros

fjahr commented at 6:12 PM on June 23, 2020:

nit: Would be nice if that typo was fixed.

fjahr commented at 8:07 PM on June 23, 2020: member

Concept ACK

Code looks good but it would be great if we could get the fix-up commits squashed and some of the more substantial feedback addressed before this gets merged.

DrahtBot added the label Needs rebase on Jul 9, 2020

laanwj commented at 3:27 PM on July 30, 2020: member

Needs rebase and comments addressed (there's a lot by now).

PastaPastaPasta commented at 4:11 AM on August 9, 2020: contributor

Hopefully this helps this PR continue:

I went ahead and rebased this PR, there were a bit of conflicts that I had to resolve. The branch can be found here https://github.com/bitcoin/bitcoin/compare/master...PastaPastaPasta:pr_btc_18242 https://github.com/PastaPastaPasta/dash/commits/pr_btc_18242 This branch builds and passes all tests locally

Additionally, I have created a branch that addresses any nits I could find in the review. That branch can be found here https://github.com/bitcoin/bitcoin/compare/master...PastaPastaPasta:pr_btc_18242_code_review

jonasschnelli commented at 9:43 AM on August 11, 2020: contributor

Sorry for the delay. Will pick this up in the next days.

jonasschnelli force-pushed on Aug 12, 2020

DrahtBot removed the label Needs rebase on Aug 12, 2020

jonasschnelli force-pushed on Aug 12, 2020

jonasschnelli commented at 2:16 PM on August 12, 2020: contributor

Improved PR; followed recommendations in various comments. Fixed many nits.

jonatack commented at 8:33 AM on August 13, 2020: member

Nice. Will re-review soon.

decentclock commented at 1:09 AM on August 21, 2020: none

I've compiled, and run the unit tests with no issues at b1ef92a on macOS Catalina.

in src/net.h:804 in b1ef92ae52 outdated

 799 | +    }
 800 | +    void SetVersion(int nVersionIn)
 801 | +    {
 802 | +        vRecv.SetVersion(nVersionIn);
 803 | +    }
 804 | +    int Read(const char* pch, unsigned int nBytes);

jonatack commented at 1:02 PM on August 21, 2020:

In commit 52a2032 "Add BIP324 v2 transport serializer and deserializer"

<details><summary>some suggested changes in <code>src/net.h::V2TransportDeserializer</code></summary>

diff --git a/src/net.h b/src/net.h
index bf909d82d7..ee2f174ad8 100644
--- a/src/net.h
+++ b/src/net.h
@@ -790,19 +790,19 @@ public:
         m_data_pos = 0;
         m_rekey_flag = false;
     }
-    bool Complete() const
+    bool Complete() const override
     {
         if (!m_in_data) {
             return false;
         }
         return (m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN == m_data_pos);
     }
-    void SetVersion(int nVersionIn)
+    void SetVersion(int nVersionIn) override
     {
         vRecv.SetVersion(nVersionIn);
     }
-    int Read(const char* pch, unsigned int nBytes);
-    CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, std::chrono::microseconds time);
+    int Read(const char* pch, unsigned int nBytes) override;
+    CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, std::chrono::microseconds time) override;
 };

 /** The TransportSerializer prepares messages for the network transport

</details>

to fix these

<details><summary>gcc/clang compiler warnings/errors</summary>

In file included from init.cpp:29:
./net.h:793:10: error: ‘virtual bool V2TransportDeserializer::Complete() const’ can be marked override [-Werror=suggest-override]
  793 |     bool Complete() const
      |          ^~~~~~~~
./net.h:800:10: error: ‘virtual void V2TransportDeserializer::SetVersion(int)’ can be marked override [-Werror=suggest-override]
  800 |     void SetVersion(int nVersionIn)
      |          ^~~~~~~~~~
./net.h:804:9: error: ‘virtual int V2TransportDeserializer::Read(const char*, unsigned int)’ can be marked override [-Werror=suggest-override]
  804 |     int Read(const char* pch, unsigned int nBytes);
      |         ^~~~
./net.h:805:17: error: ‘virtual CNetMessage V2TransportDeserializer::GetMessage(const unsigned char (&)[4], std::chrono::microseconds)’ can be marked override [-Werror=suggest-override]
  805 |     CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, std::chrono::microseconds time);
      |                 ^~~~~~~~~~
cc1plus: some warnings being treated as errors
make[2]: *** [Makefile:11597: libbitcoin_server_a-init.o] Error 1
make[2]: *** Waiting for unfinished jobs....
In file included from net.cpp:10:
./net.h:793:10: error: ‘virtual bool V2TransportDeserializer::Complete() const’ can be marked override [-Werror=suggest-override]
  793 |     bool Complete() const
      |          ^~~~~~~~
./net.h:800:10: error: ‘virtual void V2TransportDeserializer::SetVersion(int)’ can be marked override [-Werror=suggest-override]
  800 |     void SetVersion(int nVersionIn)
      |          ^~~~~~~~~~
./net.h:804:9: error: ‘virtual int V2TransportDeserializer::Read(const char*, unsigned int)’ can be marked override [-Werror=suggest-override]
  804 |     int Read(const char* pch, unsigned int nBytes);
      |         ^~~~
./net.h:805:17: error: ‘virtual CNetMessage V2TransportDeserializer::GetMessage(const unsigned char (&)[4], std::chrono::microseconds)’ can be marked override [-Werror=suggest-override]
  805 |     CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, std::chrono::microseconds time);
      |                 ^~~~~~~~~~
net.cpp: In member function ‘virtual CNetMessage V2TransportDeserializer::GetMessage(const unsigned char (&)[4], std::chrono::microseconds)’:
net.cpp:839:46: warning: comparison is always true due to limited range of data type [-Wtype-limits]
  839 |         if (!valid_header && size_or_shortid >= 0) {
      |                              ~~~~~~~~~~~~~~~~^~~~
cc1plus: some warnings being treated as errors
make[2]: *** [Makefile:11653: libbitcoin_server_a-net.o] Error 1
In file included from ./net_processing.h:10,
                 from net_processing.cpp:6:
./net.h:793:10: error: ‘virtual bool V2TransportDeserializer::Complete() const’ can be marked override [-Werror=suggest-override]
  793 |     bool Complete() const
      |          ^~~~~~~~
./net.h:800:10: error: ‘virtual void V2TransportDeserializer::SetVersion(int)’ can be marked override [-Werror=suggest-override]
  800 |     void SetVersion(int nVersionIn)
      |          ^~~~~~~~~~
./net.h:804:9: error: ‘virtual int V2TransportDeserializer::Read(const char*, unsigned int)’ can be marked override [-Werror=suggest-override]
  804 |     int Read(const char* pch, unsigned int nBytes);
      |         ^~~~
./net.h:805:17: error: ‘virtual CNetMessage V2TransportDeserializer::GetMessage(const unsigned char (&)[4], std::chrono::microseconds)’ can be marked override [-Werror=suggest-override]
  805 |     CNetMessage GetMessage(const CMessageHeader::MessageStartChars& message_start, std::chrono::microseconds time);
      |                 ^~~~~~~~~~
cc1plus: some warnings being treated as errors
make[2]: *** [Makefile:11667: libbitcoin_server_a-net_processing.o] Error 1
make[2]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
make[1]: *** [Makefile:18858: all-recursive] Error 1
make[1]: Leaving directory '/home/jon/projects/bitcoin/bitcoin/src'
make: *** [Makefile:790: all-recursive] Error 1

</details>

appeased the compiler and moving forward with review

in src/test/net_tests.cpp:951 in b1ef92ae52 outdated

 345 | +    } else {
 346 | +        serializer = MakeUnique<V1TransportSerializer>(V1TransportSerializer());
 347 | +        deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION));
 348 | +    }
 349 | +    // run a couple of times through all messages with the same AEAD instance
 350 | +    for (unsigned int i = 0; i < 100; i++) {

jonatack commented at 6:43 AM on August 25, 2020:

7fb32f34 nit, here and lines 387, 394, 418, and 499, prefix increment iterator ++i preferred

in src/net.cpp:925 in b1ef92ae52 outdated

 867 | +    // if failed to decrypt – which we tolerate at this point – we need to reduce the message size by the length of the AAD & MAC
 868 | +    // to conform to the abstract TransportDeserializer protocol
 869 | +    msg.m_message_size = valid_checksum ? msg.m_recv.size() : (msg.m_recv.size() - CHACHA20_POLY1305_AEAD_AAD_LEN - CHACHA20_POLY1305_AEAD_TAG_LEN);
 870 | +    msg.m_raw_message_size = CHACHA20_POLY1305_AEAD_AAD_LEN + m_message_size + CHACHA20_POLY1305_AEAD_TAG_LEN; // raw wire size
 871 | +
 872 | +    const int64_t now = GetTime();

jonatack commented at 6:54 AM on August 25, 2020:

2bc6e8b here in GetMessage(), as well as below, line 967 in prepareForTransport, GetTime() is deprecated per util/time.h:

/**
 * DEPRECATED
 * Use either GetSystemTimeInSeconds (not mockable) or GetTime<T> (mockable)
 */
int64_t GetTime();

in src/net.h:846 in b1ef92ae52 outdated

 761 | +    std::unique_ptr<ChaCha20Poly1305AEAD> m_aead;
 762 | +    CPrivKey m_aead_k1; //keep the keys for later rekeying
 763 | +    CPrivKey m_aead_k2;
 764 | +    uint64_t m_bytes_decrypted = 0; // counter of bytes decrypted under the same key
 765 | +    int64_t m_time_last_rekey = 0;  // recorded time when the last rekey happened
 766 | +    uint256 m_session_id;           // the encryption session_id, relevant for rekeying

jonatack commented at 7:02 AM on August 25, 2020:

2bc6e8bc6e9 perhaps set a default value for m_session_id

jonasschnelli commented at 11:35 AM on August 27, 2020:

AFAIK uint256 has always a default of zero.

jonatack commented at 10:27 PM on August 27, 2020:

Indeed, you are right; in uint256.h the default constructor zeroes it out with memset(data, 0, sizeof(data));.

jonatack commented at 11:39 AM on August 25, 2020: member

WIP re-review of this PR along with the BIP draft. In additional to my #18242 (review), ISTM the changes in the last two commits 7f4c8a1b and b1ef92ae are missing test coverage. Built and re-ran the fuzzer successfully to 15 million execs over a couple hours. A few thoughts below.

jonatack commented at 11:45 AM on August 25, 2020: member

Two conversations with outstanding unresolved issues buried in the discussion above:

#18242 (review) - comparison always true
#18242 (review) - time clock attacks

DrahtBot added the label Needs rebase on Sep 1, 2020

jonasschnelli force-pushed on Sep 3, 2020

DrahtBot removed the label Needs rebase on Sep 3, 2020

jonasschnelli commented at 3:53 PM on September 3, 2020: contributor

Rebased.

jonasschnelli commented at 11:41 AM on September 8, 2020: contributor

@jonatack:

I think the "always true comparison" is fixed (#18242 (comment) - comparison always true). Agree?
The time clock attack is something that needs to be discussed first on the BIP draft (which is happening).

jonatack commented at 12:58 PM on September 8, 2020: member

Thanks @jonasschnelli, I've been planning to get back to this, and study Lloyd's last comment in the BIP draft as well.

in src/net.cpp:904 in e549e76f24 outdated

 935 | +    // prepare the packet length that will later be encrypted and part of the MAC (AD)
 936 | +    // the packet length excludes the 16 byte MAC tag
 937 | +    uint32_t packet_length = serialized_command_size + msg.data.size();
 938 | +
 939 | +    // prepare the packet length & message command and reserve 4 bytes (3bytes AAD + 1byte short-ID)
 940 | +    std::vector<unsigned char> serialized_header(CHACHA20_POLY1305_AEAD_AAD_LEN + 1);

ariard commented at 3:30 PM on September 9, 2020:

The 4 bytes reservation is optimistically ? Maybe a ternary above to pick up between 1-byte short-ID and ASCII command string ?

jonasschnelli commented at 9:23 AM on September 10, 2020:

Do you mean reserve additional bytes to avoid further buffer re-allocations? I guess 4 bytes will be sufficient for a couple of messages (like ping, sendheaders, etc.). I would also expect that the internal allocation is larger then 4 bytes anyways. Though I'm not opposed to reserve always a minimum of 64bytes which should find a single inv (most sent message?).

in src/net.cpp:971 in e549e76f24 outdated

 966 | +    assert(!bit24);
 967 | +
 968 | +    // check if we should rekey after this message
 969 | +    const std::chrono::milliseconds now = GetTime<std::chrono::milliseconds>(); //TODO: check how expansive the GetTime call is and if it is avoidable
 970 | +    bool rekey = false;
 971 | +    if (m_bytes_encrypted >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? 32 * 1024 : REKEY_LIMIT_BYTES) || now - m_time_last_rekey >= (gArgs.GetBoolArg("-netencryptionfastrekey", false) ? REKEY_LIMIT_TIME_TEST : REKEY_LIMIT_TIME)) {

ariard commented at 3:36 PM on September 9, 2020:

Maybe add a constant for 32 * 1024 as we have for time ? Also do we have a setting prefix for testing only flag like netencryptionfastrekey to underscore further it's a test-only flag ?

jonasschnelli commented at 9:19 AM on September 10, 2020:

I was trying to avoid additional constants for internal test purposes only. Sadly, we don't have a general test-only flag so far.

in src/net.h:891 in e549e76f24 outdated

 860 | @@ -798,6 +861,27 @@ class V1TransportSerializer  : public TransportSerializer {
 861 |      void prepareForTransport(CSerializedNetMsg& msg, std::vector<unsigned char>& header) override;
 862 |  };
 863 |  
 864 | +class V2TransportSerializer : public TransportSerializer

ariard commented at 3:57 PM on September 9, 2020:

nit: Add reference to BIP324 compliance.

in src/protocol.h:270 in e549e76f24 outdated

 251 | @@ -251,7 +252,17 @@ extern const char* WTXIDRELAY;
 252 |  }; // namespace NetMsgType
 253 |  
 254 |  /* Get a vector of all valid message types (see above) */
 255 | -const std::vector<std::string>& getAllNetMessageTypes();
 256 | +const std::map<uint8_t, std::string>& getAllNetMessageTypes();
 257 | +
 258 | +/** Short Command IDs are a low bandwidth representations of a message type
 259 | + *   The mapping is a peer to peer agreement

ariard commented at 3:59 PM on September 9, 2020:

"with a default pre-shared one as provided by BIP324"?

in src/net.cpp:37 in e549e76f24 outdated

  42 | @@ -42,6 +43,7 @@
  43 |  static_assert(MINIUPNPC_API_VERSION >= 10, "miniUPnPc API version >= 10 assumed");
  44 |  #endif
  45 |  
  46 | +#include <algorithm>

ariard commented at 4:28 PM on September 9, 2020:

I compile without ?

jonatack commented at 9:32 AM on September 10, 2020:

The added code uses functions from https://en.cppreference.com/w/cpp/header/algorithm. Per the developer notes, every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

in src/net.cpp:848 in e549e76f24 outdated

 828 | +            vRecv >> size_or_shortid;
 829 | +        } catch (const std::ios_base::failure&) {
 830 | +            LogPrint(BCLog::NET, "Invalid command name\n");
 831 | +        }
 832 | +        if (size_or_shortid > 0 && size_or_shortid <= NET_P2P_V2_CMD_MAX_CHARS_SIZE && vRecv.size() >= size_or_shortid) {
 833 | +            // first byte is a number between 1 and 12. Must be a string command.

ariard commented at 4:32 PM on September 9, 2020:

Not straightforward to understand IMO. Maybe "first byte encode the char-length of a string command bounded between 1 to 12" ?

ariard commented at 4:42 PM on September 9, 2020: member

Sounds we're getting closer, mostly calls for better comments. Once we solve the BIP issues, I hope to focus next review round on robustness of test coverage.

DrahtBot added the label Needs rebase on Sep 15, 2020

fanquake referenced this in commit 6af9b31bfc on Sep 29, 2020

sidhujag referenced this in commit 7f1c584210 on Sep 29, 2020

jonasschnelli force-pushed on Nov 3, 2020

in src/Makefile.test.include:762 in 2580d64c9f outdated

 758 | @@ -758,13 +759,20 @@ test_fuzz_out_point_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -D
 759 |  test_fuzz_out_point_deserialize_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 760 |  test_fuzz_out_point_deserialize_LDADD = $(FUZZ_SUITE_LD_COMMON)
 761 |  test_fuzz_out_point_deserialize_LDFLAGS = $(FUZZ_SUITE_LDFLAGS_COMMON)
 762 | +test_fuzz_out_point_de=serialize_SOURCES = test/fuzz/deserialize.cpp

jonatack commented at 10:59 AM on November 3, 2020:

88f1e620b maybe a typo

test_fuzz_out_point_deserialize_SOURCES = test/fuzz/deserialize.cpp

jonasschnelli commented at 11:08 AM on November 3, 2020:

Indeed. Rebase mistake. Fixed now.

jonatack commented at 11:56 AM on November 3, 2020:

Thanks. Noting git range-diff 218fe60 e549e76 76ce46f here for my re-reviewing. Would be great to get this in soon.

jonatack commented at 11:03 AM on November 3, 2020: member

Looking through the diff with git range-diff 218fe60 e549e76 2580d6. I plan to re-review in depth after 0.21 branch-off. Maybe remove the "Conflicts" comments in the commit messages of 16ab33a0363c, 88f1e620b7c32 and 713e07df557f9.

jonasschnelli force-pushed on Nov 3, 2020

DrahtBot removed the label Needs rebase on Nov 3, 2020

jonatack commented at 4:34 PM on November 3, 2020: member

IRC discussion today about rekey implementation: http://www.erisian.com.au/bitcoin-core-dev/log-2020-11-03.html#l-504

jonasschnelli removed this from the "Blockers" column in a project

jonasschnelli assigned jonasschnelli on Nov 19, 2020

laanwj added this to the "Blockers" column in a project

jonasschnelli removed this from the "Blockers" column in a project

DrahtBot added the label Needs rebase on Nov 20, 2020

PastaPastaPasta commented at 2:58 AM on December 15, 2020: contributor

@jonasschnelli Do you think you could summarize / talk about what is blocking this PR from moving forward at this point?

jonasschnelli commented at 7:23 AM on December 15, 2020: contributor

@PastaPastaPasta: there is an attempt to overhaul and optimise the AEAD construct. I'd like to work that into the BIP. See https://gist.github.com/jonasschnelli/c530ea8421b8d0e80c51486325587c52 for discussion.

jonasschnelli force-pushed on Jan 19, 2021

Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification 1cfd3fef3e

Expose MAC length in chacha_poly_aead.h 9d008a4b34

Add BIP324 short-IDs to protocol.cpp 88e2c3f84e

jonasschnelli force-pushed on Jan 19, 2021

DrahtBot removed the label Needs rebase on Jan 19, 2021

Add BIP324 v2 transport serializer and deserializer 7ea1831ce0

Add p2p network v2 serializer/deserializer unit tests

# Conflicts:
#	src/test/net_tests.cpp

645d70eb27

Fuzzing: extend the p2p_transport_deserializer fuzz-test to cover v2 transport 4fbf5c9f7e

Fix: deduct message header if MAC-check/decryption failed

In order to conform to the TransportDeserializer protocol, we need to reduce the header from the messge size

# Conflicts:
#	src/net.cpp

9e81bc49b1

jonasschnelli force-pushed on Jan 20, 2021

DrahtBot added the label Needs rebase on Jan 26, 2021

DrahtBot commented at 9:08 AM on January 26, 2021: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

in src/test/fuzz/p2p_transport_deserializer.cpp:43 in 9e81bc49b1

  48 |      }
  49 |  }
  50 | +FUZZ_TARGET_INIT(p2p_transport_deserializer, initialize_p2p_transport_deserializer)
  51 | +{
  52 | +    // Construct deserializer, with a dummy NodeId
  53 | +    std::unique_ptr<TransportDeserializer> v1_deserializer = MakeUnique<V1TransportDeserializer>(Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION);

fanquake commented at 12:38 AM on March 12, 2021:

Please use std::make_unique in new code.

in src/net.cpp:819 in 9e81bc49b1

 814 | +    m_data_pos += copy_bytes;
 815 | +
 816 | +    return copy_bytes;
 817 | +}
 818 | +
 819 | +Optional<CNetMessage> V2TransportDeserializer::GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size)

fanquake commented at 2:18 AM on March 15, 2021:

Please use std::optional in new code.

Fabcien referenced this in commit c91add3229 on May 13, 2021

fanquake commented at 12:24 PM on August 18, 2021: member

My understanding is that someone else is helping with / taking over these changes, and that the BIP is still being overhauled. I think we'll be better off with new PRs, and clean discussion when work on the implementation resumes in this repo. Changes from here are be cherry-picked if / when needed. So I'm going to close this PR for now.

fanquake closed this on Aug 18, 2021

jonatack commented at 1:12 PM on August 18, 2021: member

FWIW, I reviewed this repeatedly, hosted a review club meeting on BIP324 (https://bitcoincore.reviews/16202), and proposed a few times to @jonasschnelli to help move this forward. The reply was that help wasn't needed. So I am curious when that changed and who is helping with it, as little seems to be happening?

fanquake commented at 12:03 AM on August 19, 2021: member

The BIP overhaul has been ongoing for a long time, and recently @dhruv has taken it over in https://gist.github.com/dhruv/5b1275751bc98f3b64bcafce7876b489.

jonatack commented at 10:19 AM on August 19, 2021: member

Thanks for the update, @fanquake.

dhruv commented at 10:51 PM on October 8, 2021: member

This PR is superseded by #23233 which is ready for review. I've tried to go over the comment history here and address anything that's wasn't previously.

dhruv added this to the "Closed unmerged" column in a project

DrahtBot locked this on Oct 30, 2022