Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 7 commits into from

Conversation

jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Mar 2, 2020

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 jonasschnelli force-pushed the 2020/03/net_v2 branch 3 times, most recently from d425adb to 73db845 Compare March 2, 2020 14:56
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 2, 2020

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Member

hebasto commented Mar 2, 2020

Concept ACK.

@practicalswift
Copy link
Contributor

practicalswift commented Mar 2, 2020

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:

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:

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
Copy link
Contributor

practicalswift commented Mar 2, 2020

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
Copy link
Contributor

Add "Waiting for author" tag? :)

@jonasschnelli
Copy link
Contributor Author

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
Copy link
Contributor

@jonasschnelli

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

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

Copy link
Contributor

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/test/net_tests.cpp Outdated Show resolved Hide resolved
src/test/net_tests.cpp Outdated Show resolved Hide resolved
src/test/net_tests.cpp Outdated Show resolved Hide resolved
src/test/net_tests.cpp Show resolved Hide resolved
src/test/net_tests.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/test/net_tests.cpp Outdated Show resolved Hide resolved
src/test/net_tests.cpp Outdated Show resolved Hide resolved
src/test/fuzz/p2p_transport_deserializer.cpp Outdated Show resolved Hide resolved
@practicalswift
Copy link
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
Copy link
Contributor Author

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

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

129545b compiled and unit tests passed locally.

  1. May I suggest to adapt the BIP324 and this PR to the fact that REJECT command has been removed from the Bitcoin Core code (p2p: Remove BIP61 reject messages #15437, validation: Remove REJECT code from CValidationState #17004, test: Remove REJECT message code #18609), i.e., shift command codes 34..38 -> 33..37?

  2. Rekeying code seems duplicated, therefore refactoring could be a good followup.

  3. 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 Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
src/net.h Outdated Show resolved Hide resolved
src/protocol.cpp Outdated Show resolved Hide resolved
src/protocol.cpp Outdated Show resolved Hide resolved
src/protocol.h Outdated Show resolved Hide resolved
src/net.h Outdated Show resolved Hide resolved
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
return 0; //no short command
}

bool GetCommandFromShortCommandID(uint8_t shortID, std::string& cmd)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
In order to conform to the TransportDeserializer protocol, we need to reduce the header from the messge size

# Conflicts:
#	src/net.cpp
@DrahtBot
Copy link
Contributor

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

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return copy_bytes;
}

Optional<CNetMessage> V2TransportDeserializer::GetMessage(std::chrono::microseconds time, uint32_t& out_err_raw_size)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 13, 2021
…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
Copy link
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 fanquake closed this Aug 18, 2021
@jonatack
Copy link
Contributor

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
Copy link
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
Copy link
Contributor

jonatack commented Aug 19, 2021

Thanks for the update, @fanquake.

@dhruv
Copy link
Contributor

dhruv commented Oct 8, 2021

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 dhruv added this to Closed unmerged in P2P encryption Mar 25, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
P2P encryption
Closed unmerged (Superseded)
Status: Closed unmerged (Superseded)
Development

Successfully merging this pull request may close these issues.

None yet