fix(p2p): frame gossipsub msgId and restrict allowedTopics (A-1256)

yarn-project/p2p/package.json

@@ -101,8 +101,7 @@
     "semver": "^7.6.0",
     "sha3": "^2.1.4",
     "snappy": "^7.2.2",
-    "tslib": "^2.4.0",
-    "xxhash-wasm": "^1.1.0"
+    "tslib": "^2.4.0"
   },
   "devDependencies": {
     "@aztec/archiver": "workspace:^",

yarn-project/p2p/src/services/encoding.test.ts

@@ -1,8 +1,36 @@
-import { MAX_TX_SIZE_KB, TopicType } from '@aztec/stdlib/p2p';
+import { MAX_TX_SIZE_KB, P2PMessage, TopicType } from '@aztec/stdlib/p2p';
 
 import { compressSync, uncompressSync } from 'snappy';
 
-import { SnappyTransform, readSnappyPreamble } from './encoding.js';
+import { SnappyTransform, getMsgIdFn, readSnappyPreamble } from './encoding.js';
+
+describe('getMsgIdFn', () => {
+  it('frames the topic length so a boundary-shifted (topic, data) pair does not collide', async () => {
+    const topic = '/aztec/tx/0.1.0';
+    const data = new P2PMessage(Buffer.from('deadbeefcafe', 'hex')).toMessageData();
+
+    // Aztec gossip data starts with a 4-byte BE length prefix, so data[0] === 0x00.
+    expect(data[0]).toBe(0x00);
+
+    // Shift one byte across the topic/data boundary: T' = T + data[0], D' = data[1:]. Under a raw
+    // topic||data concatenation these hash identically to (T, data) — the suppression attack.
+    const shiftedTopic = topic + String.fromCharCode(data[0]);
+    const shiftedData = data.subarray(1);
+
+    const id = await getMsgIdFn({ topic, data });
+    const shiftedId = await getMsgIdFn({ topic: shiftedTopic, data: shiftedData });
+
+    expect(Buffer.from(shiftedId).equals(Buffer.from(id))).toBe(false);
+  });
+
+  it('is deterministic for the same (topic, data)', async () => {
+    const topic = '/aztec/tx/0.1.0';
+    const data = new P2PMessage(Buffer.from('0102030405', 'hex')).toMessageData();
+    const a = await getMsgIdFn({ topic, data });
+    const b = await getMsgIdFn({ topic, data });
+    expect(Buffer.from(a).equals(Buffer.from(b))).toBe(true);
+  });
+});
 
 describe('readSnappyPreamble', () => {
   describe('basic varint decoding', () => {

yarn-project/p2p/src/services/encoding.ts

@@ -2,12 +2,10 @@
 import { createLogger } from '@aztec/foundation/log';
 import { MAX_TX_SIZE_KB, TopicType, getTopicFromString } from '@aztec/stdlib/p2p';
 
-import type { RPC } from '@chainsafe/libp2p-gossipsub/message';
 import type { DataTransform } from '@chainsafe/libp2p-gossipsub/types';
 import type { Message } from '@libp2p/interface';
 import { webcrypto } from 'node:crypto';
 import { compressSync, uncompressSync } from 'snappy';
-import xxhashFactory from 'xxhash-wasm';
 
 /** Thrown when a Snappy-compressed response exceeds the allowed decompressed size. */
 export class OversizedSnappyResponseError extends Error {
@@ -17,26 +15,9 @@ export class OversizedSnappyResponseError extends Error {
   }
 }
 
-// Load WASM
-const xxhash = await xxhashFactory();
-
-// Use salt to prevent msgId from being mined for collisions
-const h64Seed = BigInt(Math.floor(Math.random() * 1e9));
-
 // Shared buffer to convert msgId to string
 const sharedMsgIdBuf = Buffer.alloc(20);
 
-/**
- * The function used to generate a gossipsub message id
- * We use the first 8 bytes of SHA256(data) for content addressing
- */
-export function fastMsgIdFn(rpcMsg: RPC.Message): string {
-  if (rpcMsg.data) {
-    return xxhash.h64Raw(rpcMsg.data, h64Seed).toString(16);
-  }
-  return '0000000000000000';
-}
-
 export function msgIdToStrFn(msgId: Uint8Array): string {
   // This happens serially, no need to reallocate the buffer
   sharedMsgIdBuf.set(msgId);
@@ -49,11 +30,20 @@ export function msgIdToStrFn(msgId: Uint8Array): string {
  * Follows similarly to:
  * https://github.com/ethereum/consensus-specs/blob/v1.1.0-alpha.7/specs/altair/p2p-interface.md#topics-and-messages
  *
+ * The topic length is framed into the hash input (`uint32be(topicLen) || topic || data`) so the
+ * `(topic, data)` boundary is unambiguous. A raw `topic || data` concatenation is not injective —
+ * shifting bytes across the boundary (e.g. `(topic + data[0], data[1:])`) yields the same bytes and
+ * thus the same id, letting an unsubscribed-topic message pre-occupy a real message's seenCache slot
+ * and suppress it as a duplicate.
+ *
  * @param message - The libp2p message
  * @returns The message identifier
  */
-export async function getMsgIdFn({ topic, data }: Message): Promise<Uint8Array> {
-  const buffer = Buffer.concat([Buffer.from(topic), data]);
+export async function getMsgIdFn({ topic, data }: Pick<Message, 'topic' | 'data'>): Promise<Uint8Array> {
+  const topicBytes = Buffer.from(topic);
+  const framedTopicLength = Buffer.allocUnsafe(4);
+  framedTopicLength.writeUInt32BE(topicBytes.length);
+  const buffer = Buffer.concat([framedTopicLength, topicBytes, data]);
   const hash = await webcrypto.subtle.digest('SHA-256', buffer);
   return Buffer.from(hash.slice(0, 20));
 }

yarn-project/p2p/src/services/libp2p/libp2p_service.ts

@@ -83,7 +83,7 @@ import { type PubSubLibp2p, convertToMultiaddr } from '../../util.js';
 import { getVersions } from '../../versioning.js';
 import { AztecDatastore } from '../data_store.js';
 import { DiscV5Service } from '../discv5/discV5_service.js';
-import { SnappyTransform, fastMsgIdFn, getMsgIdFn, msgIdToStrFn } from '../encoding.js';
+import { SnappyTransform, getMsgIdFn, msgIdToStrFn } from '../encoding.js';
 import { APP_SPECIFIC_WEIGHT, gossipScoreThresholds } from '../gossipsub/scoring.js';
 import { createAllTopicScoreParams } from '../gossipsub/topic_score_params.js';
 import type { PeerManagerInterface } from '../peer-manager/interface.js';
@@ -413,6 +413,13 @@ export class LibP2PService extends WithTracer implements P2PService {
       expectedBlockProposalsPerSlot: config.expectedBlockProposalsPerSlot,
     });
 
+    // Restrict gossipsub to exactly the topics we subscribe to. Without this, an arbitrary-topic
+    // message is transformed, msg-id'd and inserted into the seenCache before the subscription check,
+    // so a crafted topic colliding on msg id can suppress a real message as a duplicate.
+    const allowedTopics = getTopicsForConfig(config.disableTransactions).map(topic =>
+      createTopicString(topic, protocolVersion),
+    );
+
     const node = await createLibp2p({
       start: false,
       peerId,
@@ -497,9 +504,13 @@ export class LibP2PService extends WithTracer implements P2PService {
           mcacheLength: config.gossipsubMcacheLength,
           mcacheGossip: config.gossipsubMcacheGossip,
           seenTTL: config.gossipsubSeenTTL,
+          allowedTopics,
+          // No fastMsgIdFn: the fast-path dedup cache keys on a non-cryptographic 64-bit hash of the
+          // raw data only (no topic), so a collision — accidental or engineered via a weak seed — drops
+          // a different message with no fallback to the full id. Dedup instead rests solely on the
+          // cryptographic, topic-framed msgIdFn below.
           msgIdFn: getMsgIdFn,
           msgIdToStrFn: msgIdToStrFn,
-          fastMsgIdFn: fastMsgIdFn,
           dataTransform: new SnappyTransform(),
           metricsRegister: otelMetricsAdapter,
           metricsTopicStrToLabel: metricsTopicStrToLabels(protocolVersion),

yarn-project/yarn.lock

@@ -1774,7 +1774,6 @@ __metadata:
     typescript: "npm:^5.3.3"
     uint8arrays: "npm:^5.0.3"
     viem: "npm:@aztec/viem@2.38.2"
-    xxhash-wasm: "npm:^1.1.0"
   languageName: unknown
   linkType: soft
 
@@ -22055,13 +22054,6 @@ __metadata:
   languageName: node
   linkType: hard
 
-"xxhash-wasm@npm:^1.1.0":
-  version: 1.1.0
-  resolution: "xxhash-wasm@npm:1.1.0"
-  checksum: 10/cad149dabda4ec4ce7c2edd98815c4cc21eadab72f631c832af110066867850fb86b9430499707358d3e23cafe6d71d0a5c16be8f1864f213e4cd1aa74bd9556
-  languageName: node
-  linkType: hard
-
 "y18n@npm:^5.0.5":
   version: 5.0.8
   resolution: "y18n@npm:5.0.8"