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

jonasschnelli · 2020-03-02T13:07:43Z

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)

DrahtBot · 2020-03-02T16:41:08Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer #21000 (fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer by practicalswift)
Add I2P support using I2P SAM #20685 (Add I2P support using I2P SAM by vasild)
refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size #20429 (refactor: replace (sizeof(a)/sizeof(a[0])) with C++17 std::size by theStack)
net: fix GetListenPort() to derive the proper port #20196 (net: fix GetListenPort() to derive the proper port by vasild)
refactor: Cleanup thread ctor calls #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 · 2020-03-02T22:54:12Z

Concept ACK.

practicalswift · 2020-03-02T23:22:18Z

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:

bitcoin/src/net.cpp

Lines 802 to 808 in 73db845

    
           uint8_t size_or_shortid; 
        
           try { 
        
               vRecv >> size_or_shortid; 
        
           } catch (const std::exception&) { 
        
               LogPrint(BCLog::NET, "Invalid command name\n"); 
        
           } 
        
           if (size_or_shortid > 0 && size_or_shortid <= 12 && vRecv.size() >= size_or_shortid) {

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

tests: Add "make check-valgrind" to run the unit tests under Valgrind #17639 – tests: Add make check-valgrind to run the unit tests under Valgrind
build: Enable Clang's -Wconditional-uninitialized to warn on potential uninitialized reads #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:

bitcoin/src/net.cpp

Lines 819 to 823 in 73db845

    
           if (!GetCommandFromShortCommandID(size_or_shortid, command_name)) { 
        
               // unknown-short-id 
        
               //  results in a valid but unknown message (will be skipped) 
        
               command_name = "unknown-"+itostr(size_or_shortid); 
        
           }

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:

init: Clarify C and C++ locale assumptions. Add locale sanity checks to verify assumptions at run-time. #18124 – init: Clarify C and C++ locale assumptions. Add locale sanity checks to verify assumptions at run-time
tests: Add fuzzing harness testing the locale independence of the strencodings.h functions #18126 – tests: Add fuzzing harness testing the locale independence of the strencodings.h functions
qt: Kill the locale dependency bug class by not allowing Qt to mess with LC_NUMERIC #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 · 2020-03-02T23:36:42Z

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 · 2020-03-21T11:09:32Z

Add "Waiting for author" tag? :)

jonasschnelli · 2020-03-27T09:04:15Z

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 · 2020-03-27T14:08:11Z

@jonasschnelli

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

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

PastaPastaPasta

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

src/protocol.cpp

src/test/net_tests.cpp

src/net.cpp

src/test/net_tests.cpp

src/test/fuzz/p2p_transport_deserializer.cpp

practicalswift · 2020-04-02T04:32:18Z

@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 · 2020-04-02T19:23:15Z

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

hebasto

129545b 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 786ec9b 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 1b2f52a ?

src/net.cpp

src/net.h

src/protocol.cpp

src/protocol.h

src/net.h

src/test/net_tests.cpp

src/net.h

maflcko · 2020-04-13T21:48:06Z

src/protocol.cpp

+    return 0; //no short command
+}
+
+bool GetCommandFromShortCommandID(uint8_t shortID, std::string& cmd)


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

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

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.

# Conflicts: # src/test/net_tests.cpp

…transport

In order to conform to the TransportDeserializer protocol, we need to reduce the header from the messge size # Conflicts: # src/net.cpp

DrahtBot · 2021-01-26T09:08:01Z

🐙 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".}

fanquake · 2021-03-12T00:38:58Z

src/test/fuzz/p2p_transport_deserializer.cpp

+FUZZ_TARGET_INIT(p2p_transport_deserializer, initialize_p2p_transport_deserializer)
+{
+    // Construct deserializer, with a dummy NodeId
+    std::unique_ptr<TransportDeserializer> v1_deserializer = MakeUnique<V1TransportDeserializer>(Params(), (NodeId)0, SER_NETWORK, INIT_PROTO_VERSION);


Please use std::make_unique in new code.

fanquake · 2021-03-15T02:18:10Z

src/net.cpp

+    return copy_bytes;
+}
+
+Optional<CNetMessage> V2TransportDeserializer::GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size)


Please use std::optional in new code.

…tests Summary: af2a145e575f23c64909e6cf1fb323c603bda7ca Refactor resource exhaustion test (Troy Giorshev) 5c4648d17ba18e4194959963994cc6b37053f127 Fix "invalid message size" test (Troy Giorshev) ff1e7b884447a5ba10553b2d964625f94e255bdc Move size limits to module-global (Troy Giorshev) 57890abf2c7919eddfec36178b1136cd44ffe883 Remove two unneeded tests (Troy Giorshev) Pull request description: This PR touches only the p2p_invalid_messages.py functional test module. There are two main goals accomplished here. First, it fixes the "invalid message size" test, which previously made a message that was invalid for multiple reasons. Second, it refactors the file into a single consistent style. This file appears to have originally had two authors, with different styles and some test duplication. It should now be easier and quicker to understand this module, anticipating the upcoming [BIP324](bitcoin/bitcoin#18242) and [AltNet](bitcoin/bitcoin#18989) changes. This should probably go in ahead of #19107, but the two are not strictly related. --- Depends on D9511 Backport of [[bitcoin/bitcoin#19177 | core#19177]] Test Plan: ninja all && ./test/functional/test_runner.py p2p_invalid_messages Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Differential Revision: https://reviews.bitcoinabc.org/D9519

fanquake · 2021-08-18T12:24:18Z

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.

jonatack · 2021-08-18T13:12:26Z

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 · 2021-08-19T00:03:29Z

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 · 2021-08-19T10:19:33Z

Thanks for the update, @fanquake.

dhruv · 2021-10-08T22:51:02Z

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.

jonasschnelli added the P2P label Mar 2, 2020

jonasschnelli force-pushed the 2020/03/net_v2 branch 3 times, most recently from d425adb to 73db845 Compare March 2, 2020 14:56

This was referenced Mar 2, 2020

Consolidate service flag bit-to-name conversion to a shared serviceFlagToStr function #18165

Merged

Use wtxid for transaction relay #18044

Merged

Serve BIP 157 compact filters #16442

Closed

practicalswift mentioned this pull request Mar 10, 2020

build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

Merged

DrahtBot mentioned this pull request Mar 20, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

DrahtBot mentioned this pull request Mar 27, 2020

test: Add test for wtxid transaction relay #18446

Closed

jonasschnelli force-pushed the 2020/03/net_v2 branch 2 times, most recently from 493c68c to 07821e1 Compare March 27, 2020 09:02

jonasschnelli force-pushed the 2020/03/net_v2 branch from 07821e1 to 818b197 Compare March 27, 2020 15:13

PastaPastaPasta suggested changes Apr 2, 2020

View reviewed changes

jonasschnelli force-pushed the 2020/03/net_v2 branch from 818b197 to 074e28b Compare April 2, 2020 19:21

DrahtBot mentioned this pull request Apr 7, 2020

scripted-diff: Replace strCommand with msg_type #18533

Merged

jonasschnelli force-pushed the 2020/03/net_v2 branch 2 times, most recently from 5e8ab4a to 129545b Compare April 8, 2020 09:01

hebasto reviewed Apr 12, 2020

View reviewed changes

dongcarl reviewed Apr 13, 2020

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

maflcko reviewed Apr 13, 2020

View reviewed changes

jonasschnelli mentioned this pull request Jan 19, 2021

Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification #20962

Closed

jonasschnelli force-pushed the 2020/03/net_v2 branch from 76ce46f to 61f6685 Compare January 19, 2021 10:08

jonasschnelli added 3 commits January 19, 2021 11:24

Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification

1cfd3fe

Expose MAC length in chacha_poly_aead.h

9d008a4

Add BIP324 short-IDs to protocol.cpp

88e2c3f

jonasschnelli force-pushed the 2020/03/net_v2 branch from 61f6685 to 4e3148b Compare January 19, 2021 10:38

DrahtBot removed the Needs rebase label Jan 19, 2021

DrahtBot mentioned this pull request Jan 19, 2021

Add I2P support using I2P SAM #20685

Merged

jonasschnelli added 4 commits January 20, 2021 10:15

Add BIP324 v2 transport serializer and deserializer

7ea1831

Add p2p network v2 serializer/deserializer unit tests

645d70e

# Conflicts: # src/test/net_tests.cpp

Fuzzing: extend the p2p_transport_deserializer fuzz-test to cover v2 …

4fbf5c9

…transport

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

9e81bc4

In order to conform to the TransportDeserializer protocol, we need to reduce the header from the messge size # Conflicts: # src/net.cpp

jonasschnelli force-pushed the 2020/03/net_v2 branch from 4e3148b to 9e81bc4 Compare January 20, 2021 09:24

DrahtBot mentioned this pull request Jan 24, 2021

fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer #21000

Merged

DrahtBot added the Needs rebase label Jan 26, 2021

fanquake reviewed Mar 12, 2021

View reviewed changes

fanquake reviewed Mar 15, 2021

View reviewed changes

fanquake closed this Aug 18, 2021

dhruv mentioned this pull request Oct 8, 2021

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

bitcoin locked and limited conversation to collaborators Oct 30, 2022

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

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

Uh oh!

Conversation

jonasschnelli commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Mar 2, 2020

Uh oh!

practicalswift commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 21, 2020

Uh oh!

jonasschnelli commented Mar 27, 2020

Uh oh!

practicalswift commented Mar 27, 2020

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

practicalswift commented Apr 2, 2020

Uh oh!

jonasschnelli commented Apr 2, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko Apr 13, 2020

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Apr 14, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 26, 2021

Uh oh!

fanquake Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 18, 2021

Uh oh!

jonatack commented Aug 18, 2021

Uh oh!

fanquake commented Aug 19, 2021

jonasschnelli commented Mar 2, 2020 •

edited

Loading

DrahtBot commented Mar 2, 2020 •

edited

Loading

practicalswift commented Mar 2, 2020 •

edited

Loading

practicalswift commented Mar 2, 2020 •

edited

Loading

jonatack commented Aug 19, 2021 •

edited

Loading