From d9628802cbf0622496c9a7c5c926d49a7a4c75db Mon Sep 17 00:00:00 2001 From: chimp1984 Date: Wed, 29 Apr 2020 18:37:41 -0500 Subject: [PATCH 1/2] If a new received protectedStorageEntry is expired we do not store it and do not broadcast. It is unclear why we receive expired data (some are very old), but a manipulated node might produce that and as it only removed at each batch process running each minute to clean out expired data it still could propagate. Is an attack vector also to flood the network with outdated offers where the maker is likely not online. Should fix https://github.com/bisq-network/bisq/issues/4026 --- .../main/offer/offerbook/OfferBook.java | 2 +- .../network/p2p/storage/P2PDataStorage.java | 36 +++++++++++-------- 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBook.java b/desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBook.java index 8bc0d091c80..1f60ed26f6b 100644 --- a/desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBook.java +++ b/desktop/src/main/java/bisq/desktop/main/offer/offerbook/OfferBook.java @@ -85,7 +85,7 @@ public void onAdded(Offer offer) { .filter(item -> item.getOffer().getId().equals(offer.getId())) .findAny(); if (candidateWithSameId.isPresent()) { - log.warn("We had an old offer in the list with the same Offer ID. Might be that the state or errorMessage was different. " + + log.warn("We had an old offer in the list with the same Offer ID. We remove the old one. " + "old offerBookListItem={}, new offerBookListItem={}", candidateWithSameId.get(), offerBookListItem); offerBookListItems.remove(candidateWithSameId.get()); } diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java index 9683f4ee01d..96374b23759 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -38,10 +38,9 @@ import bisq.network.p2p.storage.messages.RemoveMailboxDataMessage; import bisq.network.p2p.storage.payload.CapabilityRequiringPayload; import bisq.network.p2p.storage.payload.DateTolerantPayload; -import bisq.network.p2p.storage.payload.ExpirablePayload; -import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.MailboxStoragePayload; import bisq.network.p2p.storage.payload.PersistableNetworkPayload; +import bisq.network.p2p.storage.payload.ProcessOncePersistableNetworkPayload; import bisq.network.p2p.storage.payload.ProtectedMailboxStorageEntry; import bisq.network.p2p.storage.payload.ProtectedStorageEntry; import bisq.network.p2p.storage.payload.ProtectedStoragePayload; @@ -76,7 +75,6 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.Maps; - import java.security.KeyPair; import java.security.PublicKey; @@ -215,7 +213,7 @@ private Set getKnownPayloadHashes() { // PersistedStoragePayload items don't get removed, so we don't have an issue with the case that // an object gets removed in between PreliminaryGetDataRequest and the GetUpdatedDataRequest and we would // miss that event if we do not load the full set or use some delta handling. - Set excludedKeys =this.appendOnlyDataStoreService.getMap().keySet().stream() + Set excludedKeys = this.appendOnlyDataStoreService.getMap().keySet().stream() .map(e -> e.bytes) .collect(Collectors.toSet()); @@ -248,7 +246,7 @@ static private Set filterKnownHashes( .filter(e -> limit.decrementAndGet() >= 0) .map(Map.Entry::getValue) .filter(networkPayload -> shouldTransmitPayloadToPeer(peerCapabilities, - objToPayload.apply(networkPayload))) + objToPayload.apply(networkPayload))) .collect(Collectors.toSet()); if (limit.get() < 0) @@ -351,7 +349,7 @@ public void processGetDataResponse(GetDataResponse getDataResponse, NodeAddress } } else { // We don't broadcast here as we are only connected to the seed node and would be pointless - addPersistableNetworkPayload(e, sender,false, false, false); + addPersistableNetworkPayload(e, sender, false, false, false); } }); log.info("Processing {} persistableNetworkPayloads took {} ms.", @@ -374,7 +372,6 @@ public void shutDown() { @VisibleForTesting void removeExpiredEntries() { - log.trace("removeExpiredEntries"); // The moment when an object becomes expired will not be synchronous in the network and we could // get add network_messages after the object has expired. To avoid repeated additions of already expired // object when we get it sent from new peers, we don’t remove the sequence number from the map. @@ -382,8 +379,8 @@ void removeExpiredEntries() { // is equal and not larger as expected. ArrayList> toRemoveList = map.entrySet().stream() - .filter(entry -> entry.getValue().isExpired(this.clock)) - .collect(Collectors.toCollection(ArrayList::new)); + .filter(entry -> entry.getValue().isExpired(this.clock)) + .collect(Collectors.toCollection(ArrayList::new)); // Batch processing can cause performance issues, so do all of the removes first, then update the listeners // to let them know about the removes. @@ -452,7 +449,7 @@ public void onDisconnect(CloseConnectionReason closeConnectionReason, Connection map.values().stream() .filter(protectedStorageEntry -> protectedStorageEntry.getProtectedStoragePayload() instanceof RequiresOwnerIsOnlinePayload) .filter(protectedStorageEntry -> ((RequiresOwnerIsOnlinePayload) protectedStorageEntry.getProtectedStoragePayload()).getOwnerNodeAddress().equals(peersNodeAddress)) - .forEach(protectedStorageEntry -> { + .forEach(protectedStorageEntry -> { // We only set the data back by half of the TTL and remove the data only if is has // expired after that back dating. // We might get connection drops which are not caused by the node going offline, so @@ -567,9 +564,9 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn } private boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEntry, - @Nullable NodeAddress sender, - @Nullable BroadcastHandler.Listener listener, - boolean allowBroadcast) { + @Nullable NodeAddress sender, + @Nullable BroadcastHandler.Listener listener, + boolean allowBroadcast) { ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); @@ -580,6 +577,15 @@ private boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageE return false; } + // To avoid that expired data get stored and broadcast we check early for expire date. + if (protectedStorageEntry.isExpired(clock)) { + log.warn("We received an expired protectedStorageEntry from ppper {}. getCreationTimeStamp={}, protectedStorageEntry={}", + sender != null ? sender.getFullAddress() : "sender is null", + new Date(protectedStorageEntry.getCreationTimeStamp()), + protectedStorageEntry); + return false; + } + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); // If we have seen a more recent operation for this payload and we have a payload locally, ignore it @@ -652,7 +658,7 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, // If we have seen a more recent operation for this payload, we ignore the current one - if(!hasSequenceNrIncreased(updatedEntry.getSequenceNumber(), hashOfPayload)) + if (!hasSequenceNrIncreased(updatedEntry.getSequenceNumber(), hashOfPayload)) return false; // Verify the updated ProtectedStorageEntry is well formed and valid for update @@ -723,7 +729,7 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, } return true; -} + } /** From c57e73b867692e65d75c6301b7d90e5775a51c0a Mon Sep 17 00:00:00 2001 From: Christoph Atteneder Date: Thu, 30 Apr 2020 10:23:37 +0200 Subject: [PATCH 2/2] Fix typo in log statement --- p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java index 96374b23759..c4ef3071ecf 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -579,7 +579,7 @@ private boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageE // To avoid that expired data get stored and broadcast we check early for expire date. if (protectedStorageEntry.isExpired(clock)) { - log.warn("We received an expired protectedStorageEntry from ppper {}. getCreationTimeStamp={}, protectedStorageEntry={}", + log.warn("We received an expired protectedStorageEntry from peer {}. getCreationTimeStamp={}, protectedStorageEntry={}", sender != null ? sender.getFullAddress() : "sender is null", new Date(protectedStorageEntry.getCreationTimeStamp()), protectedStorageEntry); @@ -1055,4 +1055,3 @@ public static MapValue fromProto(protobuf.MapValue proto) { } } } -