2496 ping pong encoding error (#2512)

* change enr sequence field to be a long scalar instead of open ended byte array * removed utility method for creating packets * made ping/pong parsing backwards compatible with older besu clients Signed-off-by: Justin Florentine <justin.florentine@consensys.net>
3 years ago · 6808445c19
parent ffc22522f8
commit 6808445c19
7 changed files with 106 additions and 31 deletions
--- a/build.gradle
+++ b/build.gradle
@ -600,6 +600,7 @@ tasks.register("dockerDistUntar") {
      from tarTree(distTarFile)
      into(dockerBuildDir)
    }
+    project.delete(files("${dockerBuildDir}/besu"))
    file("${dockerBuildDir}/${distTarFileName}").renameTo("${dockerBuildDir}/besu")
  }
 }
--- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PingPacketData.java
+++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PingPacketData.java
@ -17,6 +17,7 @@ package org.hyperledger.besu.ethereum.p2p.discovery.internal;
 import static com.google.common.base.Preconditions.checkArgument;

 import org.hyperledger.besu.ethereum.p2p.discovery.Endpoint;
+import org.hyperledger.besu.ethereum.rlp.MalformedRLPInputException;
 import org.hyperledger.besu.ethereum.rlp.RLPInput;
 import org.hyperledger.besu.ethereum.rlp.RLPOutput;

@ -77,8 +78,12 @@ public class PingPacketData implements PacketData {
    final long expiration = in.readLongScalar();
    UInt64 enrSeq = null;
    if (!in.isEndOfCurrentList()) {
+      try {
+        enrSeq = UInt64.valueOf(in.readLongScalar());
+      } catch (MalformedRLPInputException malformed) {
        enrSeq = UInt64.fromBytes(in.readBytes());
      }
+    }
    in.leaveListLenient();
    return new PingPacketData(from, to, expiration, enrSeq);
  }
@ -95,13 +100,7 @@ public class PingPacketData implements PacketData {
        .encodeStandalone(out);
    to.encodeStandalone(out);
    out.writeLongScalar(expiration);
-    out.writeBytes(
-        getEnrSeq()
-            .orElseThrow(
-                () ->
-                    new IllegalStateException(
-                        "Attempting to serialize invalid PING packet. Missing 'enrSeq' field"))
-            .toBytes());
+    out.writeLongScalar(enrSeq.toLong());
    out.endList();
  }

--- a/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PongPacketData.java
+++ b/ethereum/p2p/src/main/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PongPacketData.java
@ -15,6 +15,7 @@
 package org.hyperledger.besu.ethereum.p2p.discovery.internal;

 import org.hyperledger.besu.ethereum.p2p.discovery.Endpoint;
+import org.hyperledger.besu.ethereum.rlp.MalformedRLPInputException;
 import org.hyperledger.besu.ethereum.rlp.RLPInput;
 import org.hyperledger.besu.ethereum.rlp.RLPOutput;

@ -57,8 +58,12 @@ public class PongPacketData implements PacketData {
    final long expiration = in.readLongScalar();
    UInt64 enrSeq = null;
    if (!in.isEndOfCurrentList()) {
+      try {
+        enrSeq = UInt64.valueOf(in.readLongScalar());
+      } catch (MalformedRLPInputException malformed) {
        enrSeq = UInt64.fromBytes(in.readBytes());
      }
+    }
    in.leaveListLenient();
    return new PongPacketData(to, hash, expiration, enrSeq);
  }
@ -69,13 +74,7 @@ public class PongPacketData implements PacketData {
    to.encodeStandalone(out);
    out.writeBytes(pingHash);
    out.writeLongScalar(expiration);
-    out.writeBytes(
-        getEnrSeq()
-            .orElseThrow(
-                () ->
-                    new IllegalStateException(
-                        "Attempting to serialize invalid PONG packet. Missing 'enrSeq' field"))
-            .toBytes());
+    out.writeLongScalar(enrSeq.toLong());
    out.endList();
  }

--- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryPacketPcapSedesTest.java
+++ b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/PeerDiscoveryPacketPcapSedesTest.java
@ -37,14 +37,13 @@ import org.junit.Test;

 public class PeerDiscoveryPacketPcapSedesTest {
  private static final String pingHexData =
-      "dcb59003d39fe8cf9bdc72fb83cd5f83493d8e564f9bca0ef496479b928786f7fd56981f3b"
-          + "1b921edc5b28417f964179a731d690ea6a0595292dad3a45f8d1ae535efe5545290a8855086722f3f22ad821146172e865c8bdc0a78d14"
-          + "4d76fef80001e505cb847f00000182765f82765fc9847f00000282765f80845fda127a880000000000000003";
+      "dd49244b390b8ee964c7320742acfbb893ab47f051a323c0d908bd360ae83bd92d1e6dd370af2e5d21c4ce2b054baa1be66fe879d"
+          + "76ca851c12803a26a382d6e56e33071ba98ae22374fd37be02aa8573aac89f955ae21e96f154d82d0f6b2e20101df05cb"
+          + "84b4b57a1982040182765fcb84b4b57a1a82040182765f8460e5f20603";
  private static final String pongHexData =
-      "53cec0d27af44bdc0471d34c4eb631f74b502df7b5513a80a054f0d619f0417d6ba4fd4d6f"
-          + "b83994b95c6d0ae8b175b068a6bffc397e2b408e797069b9370ce47b153dd884b60108e686546a775ed5f85e71059a9c5791e266bd949d"
-          + "0dcfba380102f83bcb84b4b57a1a82040182765fa046896547d3b4259aa1a67bd26e7ec58ab4be650c5552ef0360caf9dae489d53b845b"
-          + "872dc8880000000000000003";
+      "3b3d56e4fcdcf714d6c29b0d521e9f4ec0dd50c73c0bbb9b44758a0a7e416ff30a3ea99de3e78a5cff82ad0a0b82030f78c6eff5a"
+          + "1b2a030276277b6e9b6ad7c35db1a6d5f83586a203b4537d82edc6b2820849a485e55aa06cfc680dc63c22d0002f3cb84"
+          + "b4b57a1a82040182765fa046896547d3b4259aa1a67bd26e7ec58ab4be650c5552ef0360caf9dae489d53b8460e5e7b603";
  private static final String findNeighborsHexData =
      "3b4c3be981427a8e9739dcd4ea3cf29fe1faa104b8381cb7c26053c4b711015b3"
          + "919213819e30284bb82ec90081098ff4af02e8d9aa12692d4a0511fe92a3c137c3b65dddc309a0384ddb60074be46735c798710f04b95a"
--- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PacketTest.java
+++ b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PacketTest.java
@ -30,7 +30,7 @@ import org.junit.Test;
 public class PacketTest {

  private static final String VALID_PONG_PACKET =
-      "53cec0d27af44bdc0471d34c4eb631f74b502df7b5513a80a054f0d619f0417d6ba4fd4d6fb83994b95c6d0ae8b175b068a6bffc397e2b408e797069b9370ce47b153dd884b60108e686546a775ed5f85e71059a9c5791e266bd949d0dcfba380102f83bcb84b4b57a1a82040182765fa046896547d3b4259aa1a67bd26e7ec58ab4be650c5552ef0360caf9dae489d53b845b872dc8880000000000000003";
+      "3b3d56e4fcdcf714d6c29b0d521e9f4ec0dd50c73c0bbb9b44758a0a7e416ff30a3ea99de3e78a5cff82ad0a0b82030f78c6eff5a1b2a030276277b6e9b6ad7c35db1a6d5f83586a203b4537d82edc6b2820849a485e55aa06cfc680dc63c22d0002f3cb84b4b57a1a82040182765fa046896547d3b4259aa1a67bd26e7ec58ab4be650c5552ef0360caf9dae489d53b8460e5e7b603";
  private static final String INVALID_SIGNATURE_PACKET =
      "43f91d11b3338b4dbdf16db4f9fa25d7b4e2db81e6fd63f8f6884dfaea851e106f8f692c77169b387bde7c38832cf2d37a9b97b1553d07587ebe251ee21ee36e0ed54fd9218e3feea3bd13ca6982b25c204d5186e7ec5373ea664c91d42467b30102f3cb842f3ee37b82040e82765fa04139782abaccbc8fd290a7fde1ff138943fa9659f7bd67f97c97b09893d1ee8a84607806e108";

@ -46,17 +46,17 @@ public class PacketTest {
        .isEqualTo(
            Bytes.fromHexString(
                "0x46896547d3b4259aa1a67bd26e7ec58ab4be650c5552ef0360caf9dae489d53b"));
-    assertThat(packetData.getExpiration()).isEqualTo(1535585736);
+    assertThat(packetData.getExpiration()).isEqualTo(1625679798);
    assertThat(packetData.getEnrSeq().isPresent()).isTrue();
    assertThat(packetData.getEnrSeq().get()).isEqualTo(UInt64.valueOf(3L));
    assertThat(packet.getNodeId())
        .isEqualTo(
            Bytes.fromHexString(
-                "0xfbe12329d5d99e3d46cba2d1f9d8d397a4f2955253396f6e0459f3f14bb29c0e4f37d8bac890ff9bfb412879257ba2378a0b48bed6b81647c6972d323212d051"));
+                "0x8cee393bcb969168690845905292da56f5eed661a2f332632c61be3d9171763825f8d520041d6dbf41b4d749a78ff73b6da286a5d7e88c52ac4dae26b9df4602"));
    assertThat(packet.getHash())
        .isEqualTo(
            Bytes.fromHexString(
-                "0x53cec0d27af44bdc0471d34c4eb631f74b502df7b5513a80a054f0d619f0417d"));
+                "0x3b3d56e4fcdcf714d6c29b0d521e9f4ec0dd50c73c0bbb9b44758a0a7e416ff3"));
  }

  @Test
--- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PingPacketDataTest.java
+++ b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PingPacketDataTest.java
@ -55,6 +55,33 @@ public class PingPacketDataTest {
    final long time = System.currentTimeMillis();
    final UInt64 enrSeq = UInt64.ONE;

+    final BytesValueRLPOutput out = new BytesValueRLPOutput();
+    out.startList();
+    out.writeIntScalar(version);
+    from.encodeStandalone(out);
+    to.encodeStandalone(out);
+    out.writeLongScalar(time);
+    out.writeLongScalar(enrSeq.toLong());
+    out.endList();
+
+    final Bytes serialized = out.encoded();
+    final PingPacketData deserialized = PingPacketData.readFrom(RLP.input(serialized));
+
+    assertThat(deserialized.getFrom()).contains(from);
+    assertThat(deserialized.getTo()).isEqualTo(to);
+    assertThat(deserialized.getExpiration()).isEqualTo(time);
+    assertThat(deserialized.getEnrSeq().isPresent()).isTrue();
+    assertThat(deserialized.getEnrSeq().get()).isEqualTo(enrSeq);
+  }
+
+  @Test
+  public void handlesLegacyENREncode() {
+    final int version = 4;
+    final Endpoint from = new Endpoint("127.0.0.1", 30303, Optional.of(30303));
+    final Endpoint to = new Endpoint("127.0.0.2", 30303, Optional.empty());
+    final long time = System.currentTimeMillis();
+    final UInt64 enrSeq = UInt64.ONE;
+
    final BytesValueRLPOutput out = new BytesValueRLPOutput();
    out.startList();
    out.writeIntScalar(version);
@ -88,7 +115,7 @@ public class PingPacketDataTest {
    from.encodeStandalone(out);
    to.encodeStandalone(out);
    out.writeLongScalar(time);
-    out.writeBytes(enrSeq.toBytes());
+    out.writeLongScalar(enrSeq.toLong());
    // Add extra field
    out.writeLongScalar(11);
    out.endList();
@ -117,7 +144,7 @@ public class PingPacketDataTest {
    from.encodeStandalone(out);
    to.encodeStandalone(out);
    out.writeLongScalar(time);
-    out.writeBytes(enrSeq.toBytes());
+    out.writeLongScalar(enrSeq.toLong());
    out.endList();

    final Bytes serialized = out.encoded();
@ -144,7 +171,7 @@ public class PingPacketDataTest {
    from.encodeStandalone(out);
    to.encodeStandalone(out);
    out.writeLongScalar(time);
-    out.writeBytes(enrSeq.toBytes());
+    out.writeLongScalar(enrSeq.toLong());
    out.endList();

    final Bytes serialized = out.encoded();
--- a/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PongPacketDataTest.java
+++ b/ethereum/p2p/src/test/java/org/hyperledger/besu/ethereum/p2p/discovery/internal/PongPacketDataTest.java
@ -55,6 +55,30 @@ public class PongPacketDataTest {
    final Bytes32 hash = Bytes32.fromHexStringLenient("0x1234");
    final UInt64 enrSeq = UInt64.ONE;

+    BytesValueRLPOutput out = new BytesValueRLPOutput();
+    out.startList();
+    to.encodeStandalone(out);
+    out.writeBytes(hash);
+    out.writeLongScalar(time);
+    out.writeLongScalar(enrSeq.toLong());
+    out.endList();
+    final Bytes encoded = out.encoded();
+
+    final PongPacketData deserialized = PongPacketData.readFrom(RLP.input(encoded));
+    assertThat(deserialized.getTo()).isEqualTo(to);
+    assertThat(deserialized.getPingHash()).isEqualTo(hash);
+    assertThat(deserialized.getExpiration()).isEqualTo(time);
+    assertThat(deserialized.getEnrSeq().isPresent()).isTrue();
+    assertThat(deserialized.getEnrSeq().get()).isEqualTo(enrSeq);
+  }
+
+  @Test
+  public void handlesLegacyENREncode() {
+    final long time = System.currentTimeMillis();
+    final Endpoint to = new Endpoint("127.0.0.2", 30303, Optional.empty());
+    final Bytes32 hash = Bytes32.fromHexStringLenient("0x1234");
+    final UInt64 enrSeq = UInt64.ONE;
+
    BytesValueRLPOutput out = new BytesValueRLPOutput();
    out.startList();
    to.encodeStandalone(out);
@ -84,7 +108,33 @@ public class PongPacketDataTest {
    to.encodeStandalone(out);
    out.writeBytes(hash);
    out.writeLongScalar(time);
-    out.writeBytes(enrSeq.toBytes());
+    out.writeLongScalar(enrSeq.toLong());
+    // Add random fields
+    out.writeLong(1234L);
+    out.endList();
+    final Bytes encoded = out.encoded();
+
+    final PongPacketData deserialized = PongPacketData.readFrom(RLP.input(encoded));
+    assertThat(deserialized.getTo()).isEqualTo(to);
+    assertThat(deserialized.getPingHash()).isEqualTo(hash);
+    assertThat(deserialized.getExpiration()).isEqualTo(time);
+    assertThat(deserialized.getEnrSeq().isPresent()).isTrue();
+    assertThat(deserialized.getEnrSeq().get()).isEqualTo(enrSeq);
+  }
+
+  @Test
+  public void readFrom_fixedWidthSeq() {
+    final long time = System.currentTimeMillis();
+    final Endpoint to = new Endpoint("127.0.0.2", 30303, Optional.empty());
+    final Bytes32 hash = Bytes32.fromHexStringLenient("0x1234");
+    final UInt64 enrSeq = UInt64.ONE;
+
+    BytesValueRLPOutput out = new BytesValueRLPOutput();
+    out.startList();
+    to.encodeStandalone(out);
+    out.writeBytes(hash);
+    out.writeLongScalar(time);
+    out.writeLongScalar(enrSeq.toLong());
    // Add random fields
    out.writeLong(1234L);
    out.endList();