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 p2p layer encryption with ECDH/ChaCha20Poly1305 #14032

Open
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@jonasschnelli
Copy link
Member

jonasschnelli commented Aug 23, 2018

This PR adds encryption to the p2p communication after a slightly overhauled version of BIP151 defined here (there is the plan to change BIP151 or to propose this protocol in a new BIP)

The encryption is optional and by default disabled (-netencryption).

If enabled, a peer connecting to another peer signalling NODE_ENCRYPTED (or added via -connect=) will try to do the proposed key handshake and continue with encrypted communication.

If enabled, peers can request (and perform) encrypted communications by sending a handshake request.

Peers not supporting encryption are still accepted (no option to enforce encrypted communication).

There is a plan to make the handshake quantum resistance by adding NewHope to the key handshake (https://newhopecrypto.org). But since this PR is already very large, it's unclear wether this should be an independent patch (probably another ~600 lines of code).

Out of scope:

  • optimized ChaCha20 implementation (for review and security reasons, the implementation is extracted from openssh)
  • benchmarks added to bench (I have done comparison against the v1 protocol with dbl-SHA256 the performance seems very similar)
  • Please no discussion about the used crypto scheme or the proposal itself (better place would be the mailing list)

TODO:

  • add option to -connect= RPC addnode where it is possible to specify the expected service flags (currently -connect= will always try for encrypted coms).

@jonasschnelli jonasschnelli added the P2P label Aug 23, 2018

src/net.cpp Outdated
CNetMessage& msg = vRecvMsg.back();
if (vRecvMsg.empty() || vRecvMsg.back()->complete()) {
if (m_encryption_handler && m_encryption_handler->shouldCryptMsg()) {
vRecvMsg.emplace_back(std::make_shared<NetCryptedMessageEnvelop>(m_encryption_handler, Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION));

This comment has been minimized.

@practicalswift

practicalswift Aug 23, 2018

Member

Nit: Should this be ”envelope” instead of ”envelop”? If so, change applies throughout this PR and also the BIP.

@@ -477,6 +478,7 @@ void SetupServerArgs()
gArgs.AddArg("-maxtipage=<n>", strprintf("Maximum tip age in seconds to consider node in initial block download (default: %u)", DEFAULT_MAX_TIP_AGE), true, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-maxtxfee=<amt>", strprintf("Maximum total fees (in %s) to use in a single wallet transaction or raw transaction; setting this too low may abort large transactions (default: %s)",
CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), false, OptionsCategory::DEBUG_TEST);
gArgs.AddArg("-netencryptionfastrekey", "Rekeys every 10 seconds and after 10kb of data", true, OptionsCategory::DEBUG_TEST);

This comment has been minimized.

@practicalswift

practicalswift Aug 23, 2018

Member

If I’m reading the logic correct then this should be ”every 10 seconds or after 10kb of data”?

src/net.cpp Outdated
msg->nTime = nTimeMicros;
if (msg->m_type == NetMessageType::PLAINTEXT_ENCRYPTION_HANDSHAKE && !msg->verifyHeader()) {
// message contains expected network magic and "version" message command
// threat as version message

This comment has been minimized.

@practicalswift

practicalswift Aug 23, 2018

Member

Nit: “treat”

LOCK(cs);
if (m_bytes_decrypted + vsize > ABORT_LIMIT_BYTES || GetTime() - m_time_last_rekey_send > ABORT_LIMIT_TIME ||
(gArgs.GetBoolArg("-netencryptionfastrekey", false) && m_bytes_decrypted + vsize > 12 * 1024)) {
// don't further decrypt and therefor abort connection when counterparty failed to respect rekey limits

This comment has been minimized.

@practicalswift

practicalswift Aug 23, 2018

Member

Nit: “therefore”?

}

virtual bool complete() const = 0;
virtual uint32_t getMessageSize() const = 0; //returns 0 when message hat not yet been parsed

This comment has been minimized.

@practicalswift

practicalswift Aug 23, 2018

Member

Nit: “has”?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 23, 2018

Reviewers, this pull request conflicts with the following ones:
  • #14336 (net: implement poll by pstratem)
  • #14335 (net: refactor: cleanup ThreadSocketHandler by pstratem)
  • #14050 (Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh by jonasschnelli)
  • #14047 (Add HKDF_HMAC256_L32 and method to negate a private key by jonasschnelli)
  • #14046 (net: Refactor message parsing (CNetMessage), adds flexibility by jonasschnelli)
  • #13766 (Prefer initialization to assignment in constructors. Prefer in-class initializers to member initializers in constructors for constant initializers. by practicalswift)
  • #13123 (net: Add Clang thread safety annotations for guarded variables in the networking code by practicalswift)
  • #12288 ([WIP][NET] Add NATPMP support. by annanay25)

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 23, 2018

Fixed @practicalswift points.

@laanwj laanwj removed this from Blockers in High-priority for review Aug 23, 2018

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Aug 23, 2018

Great work!

Although optimized crypto is certainly out of scope, we do want to be mindful of making any protocol decisions that would preclude using them. :)

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 24, 2018

Note to reviewers: please review...

  • #14046 Refactor message parsing (CNetMessage), adds flexibility
  • #14047 Add HKDF_HMAC256_L32 and method to negate a private key
  • #14049 Enable libsecp256k1 ecdh module, add ECDH function to CKey
  • #14050 Add chacha20/poly1305 and chacha20poly1305_AEAD from openssh
    ... first (extracted commits from this PR)
@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 29, 2018

What is the point of NODE_ENCRYPTED? Service bits shouldn't be used for mere protocol negotiation...

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 29, 2018

What is the point of NODE_ENCRYPTED? Service bits shouldn't be used for mere protocol negotiation...

This has now been discussed on IRC:
https://botbot.me/freenode/bitcoin-core-dev/2018-08-29/?msg=103889728&page=3

// authenticate and decrypt if the message is complete
if (!m_encryption_handler->AuthenticatedAndDecrypt(vRecv)) {
LogPrint(BCLog::NET, "Authentication or decryption failed\n");
return false;

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:06:03 clang-tidy(pr=14032): src/net_encryption.cpp:75:24: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
try {
vRecv >> size_or_shortid;
} catch (const std::exception&) {
return false;

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:06:03 clang-tidy(pr=14032): src/net_encryption.cpp:88:24: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
} catch (const std::exception&) {
return false;
}
if (size_or_shortid == 0) return false; //0 is invalid

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:06:03 clang-tidy(pr=14032): src/net_encryption.cpp:90:46: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
if (size_or_shortid == 0) return false; //0 is invalid
if (size_or_shortid <= 12) {
// string command
if (vRecv.size() < size_or_shortid) return false;

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:06:03 clang-tidy(pr=14032): src/net_encryption.cpp:93:60: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
else {
// must be a short ID
if (!GetCommandFromShortCommandID(size_or_shortid, m_command_name)) {
return false; // short ID not found

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:06:03 clang-tidy(pr=14032): src/net_encryption.cpp:101:28: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
return false;
}
if (memcmp(hdr.pchMessageStart, Params().MessageStart(), CMessageHeader::MESSAGE_START_SIZE) == 0 || hdr.GetCommand() == NetMsgType::VERSION) {
return false;

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:06:03 clang-tidy(pr=14032): src/net_encryption.cpp:139:16: warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]

LOCK(cs);
if (chacha20poly1305_get_length24(&m_recv_aead_ctx, &len_out, m_recv_seq_nr, (const uint8_t*)&data_in.data()[0]) == -1) {
return false;

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:06:03 clang-tidy(pr=14032): src/net_encryption.cpp:222:16: warning: redundant boolean literal in conditional return statement [readability-simplify-boolean-expr]
return ret;
}

BIP151Encryption::BIP151Encryption() : handshake_done(false)

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:06:03 clang-tidy(pr=14032): src/net_encryption.cpp:300:1: warning: constructor does not initialize these fields: m_inbound, m_send_aead_ctx, m_recv_aead_ctx [cppcoreguidelines-pro-type-member-init]
self.nodes[0].generate(101)
self.sync_all()
nodes_session_id = []
for i in range(0,4):

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:09:57 flake8(pr=14032): test/functional/p2p_encryption.py:34:25: E231 missing whitespace after ','
public:
CDataStream vRecv; // received message data
int64_t nTime; // time (in microseconds) of message receipt.
NetMessageType m_type;

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:10:08 pvs-studio-analyzer(pr=14032): src/net_message.h:28 [err] V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_type.
return ret;
}

BIP151Encryption::BIP151Encryption() : handshake_done(false)

This comment has been minimized.

@practicalswift

practicalswift Sep 21, 2018

Member
2018-09-20 04:10:08 pvs-studio-analyzer(pr=14032): src/net_encryption.cpp:300 [err] V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_inbound, m_send_aead_ctx, m_recv_aead_ctx.
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};

chacha20poly1305_init(&aead_ctx, aead_keys, 64);
std::vector<uint8_t> in(bufsize,0);

This comment has been minimized.

@practicalswift

practicalswift Sep 23, 2018

Member
2018-09-22 21:25:19 cpplint(pr=14032): src/bench/chacha20poly1305.cpp:29:  Missing space after ,  [whitespace/comma] [3]

chacha20poly1305_init(&aead_ctx, aead_keys, 64);
std::vector<uint8_t> in(bufsize,0);
std::vector<uint8_t> out(bufsize,0);

This comment has been minimized.

@practicalswift

practicalswift Sep 23, 2018

Member
2018-09-22 21:25:19 cpplint(pr=14032): src/bench/chacha20poly1305.cpp:30:  Missing space after ,  [whitespace/comma] [3]
static void HASH256_(benchmark::State& state, uint64_t bufsize)
{
uint8_t hash[CHash256::OUTPUT_SIZE];
std::vector<uint8_t> in(bufsize,0);

This comment has been minimized.

@practicalswift

practicalswift Sep 23, 2018

Member
2018-09-22 21:25:19 cpplint(pr=14032): src/bench/chacha20poly1305.cpp:51:  Missing space after ,  [whitespace/comma] [3]

public:
BIP151Encryption();
~BIP151Encryption()

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-09-25 21:57:17 clang(pr=14032): net_encryption.h:77:5: warning: '~BIP151Encryption' overrides a destructor but is not marked 'override' [-Winconsistent-missing-destructor-override]
public:
unsigned int m_data_pos;

NetMessageEncryptionHandshake(const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn) : NetMessageBase(nTypeIn, nVersionIn)

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-09-25 21:57:17 clang(pr=14032): net_encryption.h:115:76: warning: unused parameter 'pchMessageStartIn' [-Wunused-parameter]

bool VerifyMessageStart() const override { return true; }
bool VerifyHeader() const override;
bool VerifyChecksum(std::string& error) const override { return true; }

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-09-25 21:57:17 clang(pr=14032): net_encryption.h:145:38: warning: unused parameter 'error' [-Wunused-parameter]

EncryptionHandlerRef m_encryption_handler;

NetCryptedMessage(EncryptionHandlerRef encryption_handler, const CMessageHeader::MessageStartChars& pchMessageStartIn, int nTypeIn, int nVersionIn)

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-09-25 21:57:17 clang(pr=14032): net_encryption.h:161:105: warning: unused parameter 'pchMessageStartIn' [-Wunused-parameter]

bool VerifyMessageStart() const override { return true; }
bool VerifyHeader() const override { return true; }
bool VerifyChecksum(std::string& error) const override { return true; }

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-09-25 21:57:17 clang(pr=14032): net_encryption.h:203:38: warning: unused parameter 'error' [-Wunused-parameter]
CVectorWriter{SER_NETWORK, INIT_PROTO_VERSION, serializedHeader, 0, hdr};
if (should_crypt) {
std::vector<unsigned char> serialized_envelope;
uint32_t envelope_payload_length = serialized_command_size + nMessageSize;

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member
2018-09-25 21:57:17 clang(pr=14032): net.cpp:2815:68: warning: implicit conversion loses integer precision: 'unsigned long' to 'uint32_t' (aka 'unsigned int') [-Wshorten-64-to-32]

This comment has been minimized.

@sipa

sipa Sep 25, 2018

Member

@practicalswift Could you slow down the rate of these nits?

They're useful, but my mailbox is full of basically just these style of comments. Perhaps these are things to point out when a PR is active, and basically ready for merging, not when it's laid dormant for weeks. This way it litters the PR page with loads of information that's distracting for someone who hasn't even gone to give a concept ACK.

This comment has been minimized.

@practicalswift

practicalswift Sep 25, 2018

Member

@sipa Absolutely! I'm still calibrating this reporting to increase the signal to noise, so feedback is appreciated!

I'll try to find the right threshold for things that are worth commenting about, and also some heuristic for determining which PR:s to analyze. Currently I'm analyzing all open PR:s but that is perhaps overkill.

There should probably be a max comment count per PR too. This PR is a good example – it is massive (+1800 lines) which gives a high warning count even if the warning ratio is comparable with other PRs (say one warning per 50 lines of code).

As said - feedback appreciated! :-)

This comment has been minimized.

@Sjors

Sjors Sep 26, 2018

Member

I think waiting for either two concept ACK or one utACK / ACK / tACK would make sense.

connect_nodes(self.nodes[3], 1)
self.sync_all()

def getEncryptionSessions(self, node):

This comment has been minimized.

@practicalswift

practicalswift Sep 30, 2018

Member

Should be get_encryption_sessions? Could also be a function instead of method since self is not used?

m_in_data = true;

return copy_bytes;
} else {

This comment has been minimized.

@practicalswift

practicalswift Sep 30, 2018

Member

Could drop this else statement (and keep the block as-is but one indentation level to the left) to make the code easier to follow?

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

if (Complete()) {

This comment has been minimized.

@practicalswift

practicalswift Sep 30, 2018

Member

Invert logic here to handle the !Complete() case first? That would simplify the code and reduce indentation.

pfrom->m_encryption_handler->EnableEncryption(false);
LogPrint(BCLog::NET, "Enabling encryption as handshake-initiator, sessionID=%s, peer=%d\n", pfrom->m_encryption_handler->GetSessionID().ToString(), pfrom->GetId());

// set the trigger to send the vesrion

This comment has been minimized.

@practicalswift

practicalswift Oct 16, 2018

Member

Should be "version" :-)

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 20, 2018

Needs rebase
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment