From d8e61ef627a1ccd81809dcd91d6caf4a607eb438 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 12 Mar 2020 22:37:53 +0100 Subject: [PATCH 1/3] protocols/kad: Do not attempt to store expired record in record store `Kademlia::record_received` calculates the expiration time of a record before inserting it into the record store. Instead of inserting the record into the record store in any case, with this patch the record is only inserted if it is not expired. If the record is expired a `KademliaHandlerIn::Reset` for the given (sub) stream is triggered. This would serve as a tiny defense mechanism against an attacker trying to fill a node's record store with expired records before the record store's clean up procedure removes the records. --- protocols/kad/src/behaviour.rs | 51 ++++++++++++++++++---------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 0c04e5da224..03fa68adc62 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -981,31 +981,35 @@ where // overridden as it avoids having to load the existing record in the // first place. - // The record is cloned because of the weird libp2p protocol requirement - // to send back the value in the response, although this is a waste of - // resources. - match self.store.put(record.clone()) { - Ok(()) => { - debug!("Record stored: {:?}; {} bytes", record.key, record.value.len()); - self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { - peer_id: source, - handler: NotifyHandler::One(connection), - event: KademliaHandlerIn::PutRecordRes { - key: record.key, - value: record.value, - request_id, - }, - }) - } - Err(e) => { - info!("Record not stored: {:?}", e); - self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { - peer_id: source, - handler: NotifyHandler::One(connection), - event: KademliaHandlerIn::Reset(request_id) - }) + if !record.is_expired(Instant::now()) { + // The record is cloned because of the weird libp2p protocol requirement + // to send back the value in the response, although this is a waste of + // resources. + match self.store.put(record.clone()) { + Ok(()) => { + debug!("Record stored: {:?}; {} bytes", record.key, record.value.len()); + self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { + peer_id: source, + handler: NotifyHandler::One(connection), + event: KademliaHandlerIn::PutRecordRes { + key: record.key, + value: record.value, + request_id, + }, + }); + return + } + Err(e) => { + info!("Record not stored: {:?}", e); + } } } + + self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { + peer_id: source, + handler: NotifyHandler::One(connection), + event: KademliaHandlerIn::Reset(request_id) + }) } /// Processes a provider record received from a peer. @@ -1911,4 +1915,3 @@ impl QueryInfo { } } } - From 103cdd886048490a3b4e5c217f77845b1e62279a Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 13 Mar 2020 19:06:52 +0100 Subject: [PATCH 2/3] protocols/kad: Send regular ack when record discarded due to expiration With this commit the remote receives a [`KademliaHandlerIn::PutRecordRes`] even in the case where the record is discarded due to being expired. Given that the remote sent the local node a [`KademliaHandlerEvent::PutRecord`] request, the remote perceives the local node as one node among the k closest nodes to the target. Returning a [`KademliaHandlerIn::Reset`] instead of an [`KademliaHandlerIn::PutRecordRes`] to have the remote try another node would only result in the remote node to contact an even more distant node. In addition returning [`KademliaHandlerIn::PutRecordRes`] does not reveal any internal information to a possibly malicious remote node. --- protocols/kad/src/behaviour.rs | 37 +++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 03fa68adc62..0e6035adccc 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -982,33 +982,42 @@ where // first place. if !record.is_expired(Instant::now()) { - // The record is cloned because of the weird libp2p protocol requirement - // to send back the value in the response, although this is a waste of - // resources. + // The record is cloned because of the weird libp2p protocol + // requirement to send back the value in the response, although this + // is a waste of resources. match self.store.put(record.clone()) { - Ok(()) => { - debug!("Record stored: {:?}; {} bytes", record.key, record.value.len()); + Ok(()) => debug!("Record stored: {:?}; {} bytes", record.key, record.value.len()), + Err(e) => { + info!("Record not stored: {:?}", e); self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { peer_id: source, handler: NotifyHandler::One(connection), - event: KademliaHandlerIn::PutRecordRes { - key: record.key, - value: record.value, - request_id, - }, + event: KademliaHandlerIn::Reset(request_id) }); + return } - Err(e) => { - info!("Record not stored: {:?}", e); - } } } + // The remote receives a [`KademliaHandlerIn::PutRecordRes`] even in the + // case where the record is discarded due to being expired. Given that + // the remote sent the local node a [`KademliaHandlerEvent::PutRecord`] + // request, the remote perceives the local node as one node among the k + // closest nodes to the target. Returning a [`KademliaHandlerIn::Reset`] + // instead of an [`KademliaHandlerIn::PutRecordRes`] to have the remote + // try another node would only result in the remote node to contact an + // even more distant node. In addition returning + // [`KademliaHandlerIn::PutRecordRes`] does not reveal any internal + // information to a possibly malicious remote node. self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler { peer_id: source, handler: NotifyHandler::One(connection), - event: KademliaHandlerIn::Reset(request_id) + event: KademliaHandlerIn::PutRecordRes { + key: record.key, + value: record.value, + request_id, + }, }) } From 960d3a33c3fb689381c144a7eb80e002a72b5aba Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 18 Mar 2020 13:47:55 +0100 Subject: [PATCH 3/3] protocols/kad/src/behaviour: Use `now` and reword expiration comment --- protocols/kad/src/behaviour.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/protocols/kad/src/behaviour.rs b/protocols/kad/src/behaviour.rs index 0e6035adccc..0884e2f5538 100644 --- a/protocols/kad/src/behaviour.rs +++ b/protocols/kad/src/behaviour.rs @@ -981,7 +981,7 @@ where // overridden as it avoids having to load the existing record in the // first place. - if !record.is_expired(Instant::now()) { + if !record.is_expired(now) { // The record is cloned because of the weird libp2p protocol // requirement to send back the value in the response, although this // is a waste of resources. @@ -1004,10 +1004,7 @@ where // case where the record is discarded due to being expired. Given that // the remote sent the local node a [`KademliaHandlerEvent::PutRecord`] // request, the remote perceives the local node as one node among the k - // closest nodes to the target. Returning a [`KademliaHandlerIn::Reset`] - // instead of an [`KademliaHandlerIn::PutRecordRes`] to have the remote - // try another node would only result in the remote node to contact an - // even more distant node. In addition returning + // closest nodes to the target. In addition returning // [`KademliaHandlerIn::PutRecordRes`] does not reveal any internal // information to a possibly malicious remote node. self.queued_events.push_back(NetworkBehaviourAction::NotifyHandler {