Security fix 1 - drop unknown getdata messages #312
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PR 18808 - Drop unknown types in getdata PR 19109 - Only allow getdata of recently announced invs
azuchi
requested changes
Sep 10, 2024
| @@ -298,6 +299,7 @@ def on_reject(self, message): pass | |||
| def on_sendcmpct(self, message): pass | |||
| def on_sendheaders(self, message): pass | |||
| def on_tx(self, message): pass | |||
| #def on_notfound(self, message): pass | |||
There was a problem hiding this comment.
There is no need to add this commented out line.
src/net_processing.cpp
Outdated
| @@ -44,6 +45,10 @@ | |||
| static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60; | |||
| /** Minimum time between orphan transactions expire time checks in seconds */ | |||
| static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60; | |||
| /** How long to cache transactions in mapRelay for normal relay */ | |||
| static constexpr std::chrono::seconds RELAY_TX_CACHE_TIME = std::chrono::minutes{15}; | |||
There was a problem hiding this comment.
The constant you added is unreferenced.
There was a problem hiding this comment.
Yes, even in bitcoin it's not used. It was defined by PR 18808. But after the logic was changed, this became unused. I'll remove it.
src/net_processing.cpp
Outdated
| @@ -251,6 +265,9 @@ struct CNodeState { | |||
| */ | |||
| bool fSupportsDesiredCmpctVersion; | |||
|
|
|||
| //! A rolling bloom filter of all announced tx CInvs to this peer. | |||
| CRollingBloomFilter vRecentlyAnnouncedInvs = CRollingBloomFilter{INVENTORY_MAX_RECENT_RELAY, 0.000001}; | |||
There was a problem hiding this comment.
Keeping the name the same makes it easier to track changes in Bitcoin.
Suggested change
| CRollingBloomFilter vRecentlyAnnouncedInvs = CRollingBloomFilter{INVENTORY_MAX_RECENT_RELAY, 0.000001}; | |
| CRollingBloomFilter m_recently_announced_invs = CRollingBloomFilter{INVENTORY_MAX_RECENT_RELAY, 0.000001}; |
src/net_processing.cpp
Outdated
| } | ||
|
|
||
| CTransactionRef tx = FindTxForGetData(pfrom, inv.hash, mempool_req, now); | ||
| int nSendFlags = (inv.type == MSG_TX ? SERIALIZE_TRANSACTION_NO_WITNESS : 0); |
There was a problem hiding this comment.
Like Bitcoin, put it inside the if(tx) branch, otherwise it will not be used.
There was a problem hiding this comment.
I'm sorry, I don't understand the change. do you mean move the variable int nSendFlags inside if?
…edInvs to m_recently_announced_invs
Naviabheeman
force-pushed
the
securityFix1_unknown_getdata
branch
from
6dd7081 to
ca0bf4d
Compare
azuchi
approved these changes
Sep 12, 2024
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Port fixes from bitcoin:
PR 18808 - Drop unknown types in getdata
PR 19109 - Only allow getdata of recently announced invs
Add functional test to verify that unknown get data does not halt the peer and transitions that were not in recently announced invs are not replied