diff --git a/libp2p/kad_dht/utils.py b/libp2p/kad_dht/utils.py index ef91b9373..6a173e59e 100644 --- a/libp2p/kad_dht/utils.py +++ b/libp2p/kad_dht/utils.py @@ -62,6 +62,15 @@ def maybe_consume_signed_record( ) if not (isinstance(peer_id, ID) and record.peer_id == peer_id): return False + # Bind the signer identity to the record's claimed peer id + # (libp2p/py-libp2p#1338): a valid signature from an unrelated + # key must not be able to certify addresses for another peer. + if ID.from_pubkey(envelope.public_key) != record.peer_id: + logger.error( + "Rejected signed peer record: signer identity does not " + "match record peer_id" + ) + return False # Use the default TTL of 2 hours (7200 seconds) if not host.get_peerstore().consume_peer_record(envelope, 7200): logger.error("Failed to update the Certified-Addr-Book") @@ -80,6 +89,15 @@ def maybe_consume_signed_record( ) if not record.peer_id.to_bytes() == msg.id: return False + # Bind the signer identity to the record's claimed peer id + # (libp2p/py-libp2p#1338): a valid signature from an unrelated + # key must not be able to certify addresses for another peer. + if ID.from_pubkey(envelope.public_key) != record.peer_id: + logger.error( + "Rejected signed peer record: signer identity does not " + "match record peer_id" + ) + return False # Use the default TTL of 2 hours (7200 seconds) if not host.get_peerstore().consume_peer_record(envelope, 7200): logger.error("Failed to update the Certified-Addr-Book") diff --git a/newsfragments/1338.bugfix.rst b/newsfragments/1338.bugfix.rst new file mode 100644 index 000000000..f873fa3fe --- /dev/null +++ b/newsfragments/1338.bugfix.rst @@ -0,0 +1 @@ +Fixed a KadDHT vulnerability where a signed ``PeerRecord`` was accepted even when the envelope's signer identity did not match the ``peer_id`` claimed inside the record, allowing an attacker to certify their own addresses for an arbitrary victim peer ID (certified-address-book poisoning). ``maybe_consume_signed_record`` now rejects any record whose signer key does not derive to ``record.peer_id`` on both the ``senderRecord`` and ``signedRecord`` paths. diff --git a/tests/core/kad_dht/test_signed_record_signer_binding.py b/tests/core/kad_dht/test_signed_record_signer_binding.py new file mode 100644 index 000000000..18c6d8bdf --- /dev/null +++ b/tests/core/kad_dht/test_signed_record_signer_binding.py @@ -0,0 +1,84 @@ +""" +Regression tests for libp2p/py-libp2p#1338. + +A signed ``PeerRecord`` must only be accepted when the envelope's *signer* +identity matches the ``peer_id`` claimed inside the record. Otherwise an +attacker can sign a record claiming a victim's peer id (with attacker-controlled +addresses) and poison the certified address book for that victim. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from multiaddr import Multiaddr + +from libp2p.crypto.ed25519 import create_new_key_pair +from libp2p.kad_dht.pb.kademlia_pb2 import Message +from libp2p.kad_dht.utils import maybe_consume_signed_record +from libp2p.peer.envelope import seal_record +from libp2p.peer.id import ID +from libp2p.peer.peer_record import PeerRecord + +_ATTACKER_ADDR = Multiaddr("/ip4/10.0.0.1/tcp/4001") + + +def _host_accepting_records() -> MagicMock: + """A host whose peerstore would accept any record it is handed.""" + host = MagicMock() + host.get_peerstore().consume_peer_record.return_value = True + return host + + +def test_rejects_signed_record_with_mismatched_signer() -> None: + """Message.Peer path: record claims the victim id but is attacker-signed.""" + victim = create_new_key_pair() + attacker = create_new_key_pair() + victim_id = ID.from_pubkey(victim.public_key) + + # Record claims the victim's id, but is signed by the attacker's key. + record = PeerRecord(victim_id, [_ATTACKER_ADDR]) + forged = seal_record(record, attacker.private_key) + + peer = Message.Peer() + peer.id = victim_id.to_bytes() + peer.signedRecord = forged.marshal_envelope() + + # Claimed id == record id, signature is valid for the attacker key, so the + # existing checks pass — the signer-identity binding must catch it. + assert maybe_consume_signed_record(peer, _host_accepting_records()) is False + + +def test_rejects_sender_record_with_mismatched_signer() -> None: + """Message.senderRecord path: same forgery, expected peer id supplied.""" + victim = create_new_key_pair() + attacker = create_new_key_pair() + victim_id = ID.from_pubkey(victim.public_key) + + record = PeerRecord(victim_id, [_ATTACKER_ADDR]) + forged = seal_record(record, attacker.private_key) + + msg = Message() + msg.senderRecord = forged.marshal_envelope() + + # record.peer_id == peer_id (victim) passes the existing check; the binding + # check must still reject it because the signer is the attacker. + assert ( + maybe_consume_signed_record(msg, _host_accepting_records(), peer_id=victim_id) + is False + ) + + +def test_accepts_self_signed_record() -> None: + """A record signed by the same identity it claims must still be accepted.""" + owner = create_new_key_pair() + owner_id = ID.from_pubkey(owner.public_key) + + record = PeerRecord(owner_id, [Multiaddr("/ip4/127.0.0.1/tcp/4001")]) + envelope = seal_record(record, owner.private_key) + + peer = Message.Peer() + peer.id = owner_id.to_bytes() + peer.signedRecord = envelope.marshal_envelope() + + assert maybe_consume_signed_record(peer, _host_accepting_records()) is True