diff --git a/cmd/devp2p/internal/v5test/discv5tests.go b/cmd/devp2p/internal/v5test/discv5tests.go index 2139cd8ca6..efe9144069 100644 --- a/cmd/devp2p/internal/v5test/discv5tests.go +++ b/cmd/devp2p/internal/v5test/discv5tests.go @@ -52,7 +52,7 @@ func (s *Suite) AllTests() []utesting.Test { {Name: "Ping", Fn: s.TestPing}, {Name: "PingLargeRequestID", Fn: s.TestPingLargeRequestID}, {Name: "PingMultiIP", Fn: s.TestPingMultiIP}, - {Name: "PingHandshakeInterrupted", Fn: s.TestPingHandshakeInterrupted}, + {Name: "HandshakeResend", Fn: s.TestHandshakeResend}, {Name: "TalkRequest", Fn: s.TestTalkRequest}, {Name: "FindnodeZeroDistance", Fn: s.TestFindnodeZeroDistance}, {Name: "FindnodeResults", Fn: s.TestFindnodeResults}, @@ -158,22 +158,20 @@ the attempt from a different IP.`) } } -// TestPingHandshakeInterrupted starts a handshake, but doesn't finish it and sends a second ordinary message -// packet instead of a handshake message packet. The remote node should respond with -// another WHOAREYOU challenge for the second packet. -func (s *Suite) TestPingHandshakeInterrupted(t *utesting.T) { - t.Log(`TestPingHandshakeInterrupted starts a handshake, but doesn't finish it and sends a second ordinary message -packet instead of a handshake message packet. The remote node should respond with -another WHOAREYOU challenge for the second packet.`) - +// TestHandshakeResend starts a handshake, but doesn't finish it and sends a second ordinary message +// packet instead of a handshake message packet. The remote node should repeat the previous WHOAREYOU +// challenge for the first PING. +func (s *Suite) TestHandshakeResend(t *utesting.T) { conn, l1 := s.listen1(t) defer conn.close() // First PING triggers challenge. ping := &v5wire.Ping{ReqID: conn.nextReqID()} conn.write(l1, ping, nil) + var challenge1 *v5wire.Whoareyou switch resp := conn.read(l1).(type) { case *v5wire.Whoareyou: + challenge1 = resp t.Logf("got WHOAREYOU for PING") default: t.Fatal("expected WHOAREYOU, got", resp) @@ -181,9 +179,16 @@ another WHOAREYOU challenge for the second packet.`) // Send second PING. ping2 := &v5wire.Ping{ReqID: conn.nextReqID()} - switch resp := conn.reqresp(l1, ping2).(type) { - case *v5wire.Pong: - checkPong(t, resp, ping2, l1) + conn.write(l1, ping2, nil) + switch resp := conn.read(l1).(type) { + case *v5wire.Whoareyou: + if resp.Nonce != challenge1.Nonce { + t.Fatalf("wrong nonce %x in WHOAREYOU (want %x)", resp.Nonce[:], challenge1.Nonce[:]) + } + if !bytes.Equal(resp.ChallengeData, challenge1.ChallengeData) { + t.Fatalf("wrong ChallengeData in resent WHOAREYOU (want %x)", resp.ChallengeData, challenge1.ChallengeData) + } + resp.Node = conn.remote default: t.Fatal("expected WHOAREYOU, got", resp) } diff --git a/p2p/discover/v5_udp_test.go b/p2p/discover/v5_udp_test.go index 6abe20d7a4..9429fbaf0a 100644 --- a/p2p/discover/v5_udp_test.go +++ b/p2p/discover/v5_udp_test.go @@ -856,10 +856,10 @@ type testCodecFrame struct { } func (c *testCodec) Encode(toID enode.ID, addr string, p v5wire.Packet, _ *v5wire.Whoareyou) ([]byte, v5wire.Nonce, error) { - // To match the behavior of v5wire.Codec, we return the cached encoding of - // WHOAREYOU challenges. - if wp, ok := p.(*v5wire.Whoareyou); ok && len(wp.Encoded) > 0 { - return wp.Encoded, wp.Nonce, nil + if wp, ok := p.(*v5wire.Whoareyou); ok && len(wp.ChallengeData) > 0 { + // To match the behavior of v5wire.Codec, we return the cached encoding of + // WHOAREYOU challenges. + return wp.ChallengeData, wp.Nonce, nil } c.ctr++ @@ -874,7 +874,7 @@ func (c *testCodec) Encode(toID enode.ID, addr string, p v5wire.Packet, _ *v5wir // Store recently sent challenges. if w, ok := p.(*v5wire.Whoareyou); ok { w.Nonce = authTag - w.Encoded = frame + w.ChallengeData = frame if c.sentChallenges == nil { c.sentChallenges = make(map[enode.ID]*v5wire.Whoareyou) } @@ -911,6 +911,7 @@ func (c *testCodec) decodeFrame(input []byte) (frame testCodecFrame, p v5wire.Pa case v5wire.WhoareyouPacket: dec := new(v5wire.Whoareyou) err = rlp.DecodeBytes(frame.Packet, &dec) + dec.ChallengeData = bytes.Clone(input) p = dec default: p, err = v5wire.DecodeMessage(frame.Ptype, frame.Packet) diff --git a/p2p/discover/v5wire/encoding.go b/p2p/discover/v5wire/encoding.go index d6a30a17ca..9a5bbd4612 100644 --- a/p2p/discover/v5wire/encoding.go +++ b/p2p/discover/v5wire/encoding.go @@ -190,10 +190,16 @@ func (c *Codec) Encode(id enode.ID, addr string, packet Packet, challenge *Whoar ) switch { case packet.Kind() == WhoareyouPacket: - // just send the WHOAREYOU packet raw again, rather than the re-encoded challenge data w := packet.(*Whoareyou) - if len(w.Encoded) > 0 { - return w.Encoded, w.Nonce, nil + if len(w.ChallengeData) > 0 { + // This WHOAREYOU packet was encoded before, so it's a resend. + // The unmasked packet content is stored in w.ChallengeData. + // Just apply the masking again to finish encoding. + c.buf.Reset() + c.buf.Write(w.ChallengeData) + copy(head.IV[:], w.ChallengeData) + enc := applyMasking(id, head.IV, c.buf.Bytes()) + return enc, w.Nonce, nil } head, err = c.encodeWhoareyou(id, packet.(*Whoareyou)) case challenge != nil: @@ -228,7 +234,6 @@ func (c *Codec) Encode(id enode.ID, addr string, packet Packet, challenge *Whoar if err != nil { return nil, Nonce{}, err } - challenge.Encoded = bytes.Clone(enc) c.sc.storeSentHandshake(id, addr, challenge) return enc, head.Nonce, err } @@ -246,14 +251,10 @@ func (c *Codec) Encode(id enode.ID, addr string, packet Packet, challenge *Whoar // EncodeRaw encodes a packet with the given header. func (c *Codec) EncodeRaw(id enode.ID, head Header, msgdata []byte) ([]byte, error) { + // header c.writeHeaders(&head) - - // Apply masking. - masked := c.buf.Bytes()[sizeofMaskingIV:] - mask := head.mask(id) - mask.XORKeyStream(masked[:], masked[:]) - - // Write message data. + applyMasking(id, head.IV, c.buf.Bytes()) + // message data c.buf.Write(msgdata) return c.buf.Bytes(), nil } @@ -463,7 +464,7 @@ func (c *Codec) Decode(inputData []byte, addr string) (src enode.ID, n *enode.No // Unmask the static header. var head Header copy(head.IV[:], input[:sizeofMaskingIV]) - mask := head.mask(c.localnode.ID()) + mask := createMask(c.localnode.ID(), head.IV) staticHeader := input[sizeofMaskingIV:sizeofStaticPacketData] mask.XORKeyStream(staticHeader, staticHeader) @@ -679,10 +680,17 @@ func (h *StaticHeader) checkValid(packetLen int, protocolID [6]byte) error { } // mask returns a cipher for 'masking' / 'unmasking' packet headers. -func (h *Header) mask(destID enode.ID) cipher.Stream { +func createMask(destID enode.ID, iv [16]byte) cipher.Stream { block, err := aes.NewCipher(destID[:16]) if err != nil { panic("can't create cipher") } - return cipher.NewCTR(block, h.IV[:]) + return cipher.NewCTR(block, iv[:]) +} + +func applyMasking(destID enode.ID, iv [16]byte, packet []byte) []byte { + masked := packet[sizeofMaskingIV:] + mask := createMask(destID, iv) + mask.XORKeyStream(masked[:], masked[:]) + return packet } diff --git a/p2p/discover/v5wire/encoding_test.go b/p2p/discover/v5wire/encoding_test.go index 5774cb3d8c..dd7ec6d53c 100644 --- a/p2p/discover/v5wire/encoding_test.go +++ b/p2p/discover/v5wire/encoding_test.go @@ -269,6 +269,35 @@ func TestHandshake_BadHandshakeAttack(t *testing.T) { net.nodeB.expectDecodeErr(t, errUnexpectedHandshake, findnode) } +func TestEncodeWhoareyouResend(t *testing.T) { + t.Parallel() + net := newHandshakeTest() + defer net.close() + + // A -> B WHOAREYOU + challenge := &Whoareyou{ + Nonce: Nonce{1, 2, 3, 4}, + IDNonce: testIDnonce, + RecordSeq: 0, + } + enc, _ := net.nodeA.encode(t, net.nodeB, challenge) + net.nodeB.expectDecode(t, WhoareyouPacket, enc) + whoareyou1 := bytes.Clone(enc) + + if len(challenge.ChallengeData) == 0 { + t.Fatal("ChallengeData not assigned by encode") + } + + // A -> B WHOAREYOU + // Send the same challenge again. This should produce exactly + // the same bytes as the first send. + enc, _ = net.nodeA.encode(t, net.nodeB, challenge) + whoareyou2 := bytes.Clone(enc) + if !bytes.Equal(whoareyou2, whoareyou1) { + t.Fatal("re-encoded challenge not equal to first") + } +} + // This test checks some malformed packets. func TestDecodeErrorsV5(t *testing.T) { t.Parallel() diff --git a/p2p/discover/v5wire/msg.go b/p2p/discover/v5wire/msg.go index 089fd4ebdc..e5c29276ca 100644 --- a/p2p/discover/v5wire/msg.go +++ b/p2p/discover/v5wire/msg.go @@ -63,19 +63,20 @@ type ( // WHOAREYOU contains the handshake challenge. Whoareyou struct { - ChallengeData []byte // Encoded challenge - Nonce Nonce // Nonce of request packet - IDNonce [16]byte // Identity proof data - RecordSeq uint64 // ENR sequence number of recipient + Nonce Nonce // Nonce of request packet + IDNonce [16]byte // Identity proof data + RecordSeq uint64 // ENR sequence number of recipient // Node is the locally known node record of recipient. // This must be set by the caller of Encode. - Node *enode.Node + Node *enode.Node `rlp:"-"` + + // ChallengeData stores the unmasked encoding of the whole packet. This is the + // input data for verification. It is assigned by both Encode and Decode + // operations. + ChallengeData []byte `rlp:"-"` sent mclock.AbsTime // for handshake GC. - - // Encoded is packet raw data for sending out, but should not be include in the RLP encoding. - Encoded []byte `rlp:"-"` } // PING is sent during liveness checks.