diff --git a/CHANGELOG.md b/CHANGELOG.md index f3908111de..0eb230e1cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,12 @@ All notable changes to this project will be documented in this file. ### Changes +- Geolocation + - geoprobe-target drops LocationOffsets whose signature chain does not verify instead of caching them and writing them to `location_offsets`, which the lake explorer publishes unfiltered. Verification itself now rejects the zero authority pubkey, since `ed25519.Verify` accepts the all-zero (pubkey, signature) pair for roughly one message in four — the zero key decodes to an order-4 point, not to nothing — so an unsigned datagram would otherwise verify. The same guard is applied to the signed TWAMP `ProbePacket.Verify` and `ReplyPacket.Verify`, the other two places a wire-supplied pubkey is used as the verification key. + - The geoprobe agent enforces RFC-16's replay mitigation: an inbound DZD offset is rejected unless its `MeasurementSlot` is within 15 minutes behind or 5 minutes ahead of the current ledger slot, which covers the 5-minute slot caches on both ends. An equal-RTT offset also no longer replaces the cache's best entry, because replacing it reset its expiry clock and let a replay pin the probe's reference point indefinitely. + - The signed TWAMP reflector verifies a probe's signature before touching per-sender pair state. `target_pk` is public onchain, so spoofed probes could previously consume a paying sender's pair budget, repoint its source IP and clear its challenge nonce. Unverified probes still get a reply, now off throwaway state and capped at one per window per pubkey — with a floor so the cap holds even where pair rate limiting is disabled. Ingestion also refuses a cached ledger slot older than two refresh periods, so an RPC outage cannot freeze the replay window around a stale slot. + - A completed target scan that matches nothing propagates instead of being mistaken for a skipped scan, so removing a user's last target or flipping them to Delinquent stops the probing. + - ICMP echo replies are matched on source address as well as ID and sequence. - CLI - `doublezero-solana shreds payments` asks the Solana node for version 1 and reads the JsonParsed instruction list, so a v1 fund no longer fails the listing with error -32015. - CI diff --git a/controlplane/telemetry/cmd/geoprobe-agent/main.go b/controlplane/telemetry/cmd/geoprobe-agent/main.go index 37f82b2ccf..bc3de067ac 100644 --- a/controlplane/telemetry/cmd/geoprobe-agent/main.go +++ b/controlplane/telemetry/cmd/geoprobe-agent/main.go @@ -43,6 +43,28 @@ const ( discoveryInterval = 60 * time.Second defaultDeliveryDNSRefreshInterval = 5 * time.Minute defaultDeliveryDNSTTL = defaultDeliveryDNSRefreshInterval * 5 / 2 + + // dzSlotDuration is the nominal DoubleZero Ledger slot time. + dzSlotDuration = 400 * time.Millisecond + + // Acceptance window for an inbound offset's MeasurementSlot, RFC-16's replay + // mitigation. A DZD stamps offsets with a slot it caches for + // geoprobe.SlotCacheTTL (5m) and this agent compares against its own slot + // cache with the same TTL, so a legitimate offset can sit ~5m either side of + // our view before any RPC or finalization lag. maxOffsetSlotLag allows 15m of + // that skew in the past; maxOffsetSlotLead allows 5m in the future, for when + // our own cached slot is the stale one. + maxOffsetSlotLag = uint64(15 * time.Minute / dzSlotDuration) + maxOffsetSlotLead = uint64(5 * time.Minute / dzSlotDuration) + + // maxSlotReferenceAge bounds how stale the cached ledger slot may be before + // it stops counting as "now" for the replay check. getCurrentSlot falls back + // to its cache indefinitely when RPC fails, so without this bound an outage + // freezes the acceptance window around an old slot: replays near that slot + // stay acceptable for as long as the outage lasts, and genuinely fresh + // offsets eventually fall outside maxOffsetSlotLead. Two refresh periods, so + // a single missed refresh does not stop ingestion. + maxSlotReferenceAge = 2 * geoprobe.SlotCacheTTL ) var ( @@ -148,8 +170,10 @@ func (c *offsetCache) Put(offset *geoprobe.LocationOffset) { return } - if offset.RttNs <= sender.best.offset.RttNs { - // New offset is better than or equal to best: replace best. + if offset.RttNs < sender.best.offset.RttNs { + // Strictly better than best: replace it. An equal-RTT offset must not + // replace best, because replacing also resets best's receivedAt clock — + // a replayed offset would otherwise hold best forever. sender.best = entry } else { // New offset has higher RTT than best, consider it for second-best. @@ -217,6 +241,24 @@ func (c *offsetCache) Evict() int { return evicted } +// slotReference returns slot only while the value cached at cachedAt is recent +// enough to serve as the "now" the replay check measures against. +func slotReference(slot uint64, cachedAt, now time.Time) (uint64, error) { + if age := now.Sub(cachedAt); age > maxSlotReferenceAge { + return 0, fmt.Errorf("cached slot is %s old, exceeds %s", age.Round(time.Second), maxSlotReferenceAge) + } + return slot, nil +} + +// offsetSlotFresh reports whether an offset's MeasurementSlot falls inside the +// acceptance window around the current ledger slot. +func offsetSlotFresh(measurementSlot, currentSlot uint64) bool { + if measurementSlot > currentSlot { + return measurementSlot-currentSlot <= maxOffsetSlotLead + } + return currentSlot-measurementSlot <= maxOffsetSlotLag +} + func marshalBestOffset(cache *offsetCache) [][]byte { best := cache.GetBest() if best == nil { @@ -470,6 +512,21 @@ func main() { return slot, nil } + // Offset ingestion needs a slot it can still treat as "now", which the + // stale-cache fallback above does not guarantee. Composite offsets keep + // using getCurrentSlot: stamping a slightly stale slot is better than the + // probe emitting nothing during an RPC blip. + getSlotReference := func(ctx context.Context) (uint64, error) { + slot, err := getCurrentSlot(ctx) + if err != nil { + return 0, err + } + slotMu.RLock() + cachedAt := slotCachedAt + slotMu.RUnlock() + return slotReference(slot, cachedAt, time.Now()) + } + // Set up UDP sender for composite offsets. senderConn, err := geoprobe.NewUDPConn() if err != nil { @@ -496,7 +553,7 @@ func main() { // Run UDP offset listener. go func() { - runOffsetListener(ctx, log, offsetListener, cache, pState, signedReflector, m) + runOffsetListener(ctx, log, offsetListener, cache, pState, signedReflector, m, getSlotReference) }() // Run eviction goroutine. @@ -658,6 +715,7 @@ func runOffsetListener( parents *parentState, signedReflector signed.Reflector, m *geoprobe.Metrics, + getCurrentSlot func(ctx context.Context) (uint64, error), ) { log.Info("Starting offset listener", "addr", conn.LocalAddr().String()) @@ -718,6 +776,30 @@ func runOffsetListener( log.Debug("signature verification successful", "authority_pubkey", authorityPK) + // RFC-16 replay mitigation. A signature stays valid forever, and the + // only other freshness bound is receipt wall-clock, which a replay + // refreshes by definition — so MeasurementSlot has to be checked against + // the current ledger slot or a captured offset can be replayed + // indefinitely to pin this probe's attested reference point. + currentSlot, err := getCurrentSlot(ctx) + if err != nil { + log.Warn("Rejecting offset, current slot unavailable", + "sender_pubkey", senderPK, "addr", addr, "error", err) + m.OffsetsRejected.WithLabelValues(geoprobe.RejectSlotUnavailable).Inc() + continue + } + if !offsetSlotFresh(offset.MeasurementSlot, currentSlot) { + log.Warn("Rejecting offset with measurement slot outside acceptance window", + "sender_pubkey", senderPK, + "addr", addr, + "measurement_slot", offset.MeasurementSlot, + "current_slot", currentSlot, + "max_lag_slots", maxOffsetSlotLag, + "max_lead_slots", maxOffsetSlotLead) + m.OffsetsRejected.WithLabelValues(geoprobe.RejectSlotOutOfWindow).Inc() + continue + } + cache.Put(offset) signedReflector.SetOffsets(marshalBestOffset(cache)) m.OffsetsReceived.Inc() diff --git a/controlplane/telemetry/cmd/geoprobe-agent/main_test.go b/controlplane/telemetry/cmd/geoprobe-agent/main_test.go index db9fa258f1..9cde465b7a 100644 --- a/controlplane/telemetry/cmd/geoprobe-agent/main_test.go +++ b/controlplane/telemetry/cmd/geoprobe-agent/main_test.go @@ -1,11 +1,17 @@ package main import ( + "context" + "io" + "log/slog" + "net" "sync" "testing" "time" + "github.com/gagliardetto/solana-go" "github.com/malbeclabs/doublezero/controlplane/telemetry/internal/geoprobe" + "github.com/prometheus/client_golang/prometheus" ) func makeTestOffset(senderPubkey [32]byte, rttNs uint64) *geoprobe.LocationOffset { @@ -352,3 +358,173 @@ func TestOffsetCache_ConcurrentAccess(t *testing.T) { } wg.Wait() } + +func TestOffsetSlotFresh(t *testing.T) { + const current = uint64(1_000_000) + + tests := []struct { + name string + measurementSlot uint64 + want bool + }{ + {"same slot", current, true}, + {"lagging within window", current - maxOffsetSlotLag/2, true}, + {"at lag boundary", current - maxOffsetSlotLag, true}, + {"replay past window", current - maxOffsetSlotLag - 1, false}, + {"leading within window", current + maxOffsetSlotLead, true}, + {"leading past window", current + maxOffsetSlotLead + 1, false}, + {"zero slot", 0, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := offsetSlotFresh(tt.measurementSlot, current); got != tt.want { + t.Errorf("offsetSlotFresh(%d, %d) = %v, want %v", tt.measurementSlot, current, got, tt.want) + } + }) + } +} + +// A replayed offset carries the RTT it was captured with. Letting an equal-RTT +// offset replace best also resets best's expiry clock, which is how a replay +// pins the probe's attested reference point forever. +func TestOffsetCache_PutEqualRTTDoesNotRefreshBest(t *testing.T) { + cache := newOffsetCache(1 * time.Hour) + pubkey := [32]byte{1} + + cache.Put(makeTestOffset(pubkey, 1000)) + firstReceivedAt := cache.entries[pubkey].best.receivedAt + + time.Sleep(2 * time.Millisecond) + cache.Put(makeTestOffset(pubkey, 1000)) // byte-identical replay + + if got := cache.entries[pubkey].best.receivedAt; !got.Equal(firstReceivedAt) { + t.Errorf("equal-RTT offset refreshed best's clock (%v -> %v); a replay could hold best forever", + firstReceivedAt, got) + } +} + +// stubSignedReflector satisfies signed.Reflector for listener tests. +type stubSignedReflector struct{} + +func (s *stubSignedReflector) Run(context.Context) error { return nil } +func (s *stubSignedReflector) Close() error { return nil } +func (s *stubSignedReflector) Port() uint16 { return 0 } +func (s *stubSignedReflector) SetAuthorizedKeys([][32]byte) {} +func (s *stubSignedReflector) SetOffsets([][]byte) {} +func (s *stubSignedReflector) SetLogger(logger *slog.Logger) {} + +func TestRunOffsetListener_RejectsOffsetOutsideSlotWindow(t *testing.T) { + const currentSlot = uint64(1_000_000) + + listener, err := geoprobe.NewUDPListener(0) + if err != nil { + t.Fatalf("failed to create listener: %v", err) + } + defer listener.Close() + listenAddr := listener.LocalAddr().(*net.UDPAddr) + + device := solana.NewWallet() + authority := solana.NewWallet() + signer, err := geoprobe.NewOffsetSigner(authority.PrivateKey, device.PublicKey()) + if err != nil { + t.Fatalf("failed to create signer: %v", err) + } + + var devicePK, authorityPK [32]byte + copy(devicePK[:], device.PublicKey().Bytes()) + copy(authorityPK[:], authority.PublicKey().Bytes()) + + cache := newOffsetCache(1 * time.Hour) + parents := &parentState{authorities: map[[32]byte][32]byte{devicePK: authorityPK}} + metrics := geoprobe.NewMetrics("test", device.PublicKey().String(), prometheus.NewRegistry()) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + done := make(chan struct{}) + go func() { + defer close(done) + runOffsetListener(ctx, slog.New(slog.NewTextHandler(io.Discard, nil)), listener, cache, parents, + &stubSignedReflector{}, metrics, func(context.Context) (uint64, error) { return currentSlot, nil }) + }() + defer func() { + cancel() + <-done + }() + + sender, err := net.ListenUDP("udp4", &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1)}) + if err != nil { + t.Fatalf("failed to create sender socket: %v", err) + } + defer sender.Close() + + sendOffset := func(slot uint64) { + t.Helper() + offset := makeTestOffset(devicePK, 1000) + offset.Version = geoprobe.LocationOffsetVersion + offset.MeasurementSlot = slot + if err := signer.SignOffset(offset); err != nil { + t.Fatalf("failed to sign offset: %v", err) + } + if err := geoprobe.SendOffset(sender, &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: listenAddr.Port}, offset); err != nil { + t.Fatalf("failed to send offset: %v", err) + } + } + + // A signed offset stamped with a slot outside the acceptance window is a + // replay: it must never reach the cache. + sendOffset(currentSlot - maxOffsetSlotLag - 1) + time.Sleep(500 * time.Millisecond) + if got := cache.Get(devicePK); got != nil { + t.Fatalf("expected replayed offset to be rejected, got %+v", got) + } + + // A fresh offset over the same path must still be accepted, proving the + // rejection above came from the slot check and not from plumbing. + sendOffset(currentSlot) + deadline := time.Now().Add(2 * time.Second) + for cache.Get(devicePK) == nil { + if time.Now().After(deadline) { + t.Fatal("expected fresh offset to be cached") + } + time.Sleep(10 * time.Millisecond) + } +} + +// getCurrentSlot serves its cache indefinitely when ledger RPC fails. A frozen +// slot would freeze the replay window with it, so ingestion refuses a reference +// that is too old to mean "now". +func TestSlotReference(t *testing.T) { + now := time.Now() + + tests := []struct { + name string + cachedAt time.Time + wantErr bool + }{ + {"just fetched", now, false}, + {"one refresh period old", now.Add(-geoprobe.SlotCacheTTL), false}, + {"at the age bound", now.Add(-maxSlotReferenceAge), false}, + {"past the age bound", now.Add(-maxSlotReferenceAge - time.Second), true}, + {"never fetched", time.Time{}, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + slot, err := slotReference(1_000_000, tt.cachedAt, now) + if tt.wantErr { + if err == nil { + t.Fatalf("expected a stale-reference error, got slot %d", slot) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if slot != 1_000_000 { + t.Errorf("expected slot 1000000, got %d", slot) + } + }) + } +} diff --git a/controlplane/telemetry/cmd/geoprobe-target/main.go b/controlplane/telemetry/cmd/geoprobe-target/main.go index 6698ea44c3..9acee2fb28 100644 --- a/controlplane/telemetry/cmd/geoprobe-target/main.go +++ b/controlplane/telemetry/cmd/geoprobe-target/main.go @@ -335,6 +335,19 @@ func handleOffset(log *slog.Logger, offset *geoprobe.LocationOffset, addr *net.U log.Debug("signature verification complete", "authority_pubkey", solana.PublicKeyFromBytes(offset.AuthorityPubkey[:]).String(), "valid", signatureValid) } + // Until the chain verifies, a LocationOffset is just an attacker-chosen UDP + // datagram. Drop it instead of recording it: every row written to + // location_offsets is aggregated by the public lake explorer without + // filtering on signature_valid, so persisting forgeries would publish them. + if !signatureValid { + log.Warn("dropping offset with invalid signature chain", + "from", addr, + "authority_pubkey", solana.PublicKeyFromBytes(offset.AuthorityPubkey[:]).String(), + "sender_pubkey", solana.PublicKeyFromBytes(offset.SenderPubkey[:]).String(), + "error", verifyError) + return + } + if chWriter != nil { rawBytes, err := offset.Marshal() if err != nil { diff --git a/controlplane/telemetry/cmd/geoprobe-target/main_test.go b/controlplane/telemetry/cmd/geoprobe-target/main_test.go new file mode 100644 index 0000000000..f767179565 --- /dev/null +++ b/controlplane/telemetry/cmd/geoprobe-target/main_test.go @@ -0,0 +1,93 @@ +package main + +import ( + "io" + "log/slog" + "net" + "testing" + "time" + + "github.com/gagliardetto/solana-go" + "github.com/malbeclabs/doublezero/controlplane/telemetry/internal/geoprobe" +) + +func newTestCaches() *geoprobe.MinCacheMap[[32]byte, geoprobe.LocationOffset] { + return geoprobe.NewMinCacheMap[[32]byte, geoprobe.LocationOffset](time.Hour, func(o geoprobe.LocationOffset) uint64 { + return o.RttNs + }) +} + +func newTestOffset() *geoprobe.LocationOffset { + return &geoprobe.LocationOffset{ + Version: geoprobe.LocationOffsetVersion, + MeasurementSlot: 12345, + MeasuredRttNs: 1_000_000, + Lat: 52.3676, + Lng: 4.9041, + RttNs: 1_000_000, + TargetIP: geoprobe.IPToTargetIP("198.51.100.1"), + References: []geoprobe.LocationOffset{}, + } +} + +// Anyone can send a datagram to the geoprobe-target UDP port, so an offset that +// fails signature verification is an attacker-chosen location claim. It must +// reach neither the cache nor ClickHouse, which the lake explorer publishes. +func TestHandleOffset_DropsUnsignedOffset(t *testing.T) { + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + caches := newTestCaches() + writer := geoprobe.NewClickhouseWriter(geoprobe.ClickhouseConfig{Addr: "unused"}, log) + addr := &net.UDPAddr{IP: net.IPv4(203, 0, 113, 7), Port: 41234} + + // An entirely unsigned datagram, and one claiming a real geoprobe's pubkey + // (public onchain) with a junk signature. + unsigned := newTestOffset() + impersonator := solana.NewWallet() + spoofed := newTestOffset() + copy(spoofed.AuthorityPubkey[:], impersonator.PublicKey().Bytes()) + copy(spoofed.SenderPubkey[:], impersonator.PublicKey().Bytes()) + spoofed.Signature[0] = 0xff + + for _, forged := range []*geoprobe.LocationOffset{unsigned, spoofed} { + handleOffset(log, forged, addr, true, writer, caches) + + if got := writer.BufferedRows(); got != 0 { + t.Errorf("expected forged offset to be dropped, got %d buffered clickhouse rows", got) + } + if _, ok := caches.Get(forged.SenderPubkey).Best(); ok { + t.Error("expected forged offset to be dropped, but it entered the cache") + } + } +} + +func TestHandleOffset_AcceptsSignedOffset(t *testing.T) { + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + caches := newTestCaches() + writer := geoprobe.NewClickhouseWriter(geoprobe.ClickhouseConfig{Addr: "unused"}, log) + addr := &net.UDPAddr{IP: net.IPv4(203, 0, 113, 7), Port: 41234} + + probe := solana.NewWallet() + authority := solana.NewWallet() + signer, err := geoprobe.NewOffsetSigner(authority.PrivateKey, probe.PublicKey()) + if err != nil { + t.Fatalf("failed to create signer: %v", err) + } + + offset := newTestOffset() + if err := signer.SignOffset(offset); err != nil { + t.Fatalf("failed to sign offset: %v", err) + } + + handleOffset(log, offset, addr, true, writer, caches) + + if got := writer.BufferedRows(); got != 1 { + t.Errorf("expected 1 buffered clickhouse row for a signed offset, got %d", got) + } + best, ok := caches.Get(offset.SenderPubkey).Best() + if !ok { + t.Fatal("expected signed offset to be cached") + } + if best.RttNs != offset.RttNs { + t.Errorf("expected cached RttNs=%d, got %d", offset.RttNs, best.RttNs) + } +} diff --git a/controlplane/telemetry/internal/geoprobe/clickhouse.go b/controlplane/telemetry/internal/geoprobe/clickhouse.go index 0bcfd6ffc3..d0be346e44 100644 --- a/controlplane/telemetry/internal/geoprobe/clickhouse.go +++ b/controlplane/telemetry/internal/geoprobe/clickhouse.go @@ -152,6 +152,13 @@ func (w *ClickhouseWriter) Record(row OffsetRow) { w.mu.Unlock() } +// BufferedRows returns the number of rows waiting to be flushed. +func (w *ClickhouseWriter) BufferedRows() int { + w.mu.Lock() + defer w.mu.Unlock() + return len(w.buf) +} + func (w *ClickhouseWriter) connect(ctx context.Context) error { if err := RunMigrations(w.cfg, w.log); err != nil { return fmt.Errorf("migrations: %w", err) diff --git a/controlplane/telemetry/internal/geoprobe/icmp_conn.go b/controlplane/telemetry/internal/geoprobe/icmp_conn.go index d8db0e58aa..42e3f0860c 100644 --- a/controlplane/telemetry/internal/geoprobe/icmp_conn.go +++ b/controlplane/telemetry/internal/geoprobe/icmp_conn.go @@ -17,7 +17,10 @@ import ( // Implementations are not safe for concurrent use; callers must serialize access. type icmpSocket interface { sendEcho(dst net.IP, payload []byte) (time.Time, error) - recvEcho(buf []byte) (int, time.Time, error) + // recvEcho returns the ICMP payload length, the sender's IPv4 address and + // the receive timestamp. Callers must match replies on the source address + // as well as ID+seq. + recvEcho(buf []byte) (int, net.IP, time.Time, error) setReadDeadline(t time.Time) error close() error } @@ -82,18 +85,18 @@ func (c *icmpConn) sendEcho(dst net.IP, payload []byte) (time.Time, error) { return txTime, nil } -func (c *icmpConn) recvEcho(buf []byte) (int, time.Time, error) { +func (c *icmpConn) recvEcho(buf []byte) (int, net.IP, time.Time, error) { remaining := int(time.Until(c.deadline).Milliseconds()) if remaining <= 0 { - return 0, time.Time{}, syscall.ETIMEDOUT + return 0, nil, time.Time{}, syscall.ETIMEDOUT } n, err := unix.EpollWait(c.epfd, c.events, remaining) if err != nil && err != syscall.EINTR { - return 0, time.Time{}, fmt.Errorf("epoll_wait: %w", err) + return 0, nil, time.Time{}, fmt.Errorf("epoll_wait: %w", err) } if n == 0 { - return 0, time.Time{}, syscall.ETIMEDOUT + return 0, nil, time.Time{}, syscall.ETIMEDOUT } var oob []byte @@ -101,15 +104,20 @@ func (c *icmpConn) recvEcho(buf []byte) (int, time.Time, error) { oob = c.oob } - msgN, oobn, _, _, err := unix.Recvmsg(c.fd, buf, oob, 0) + msgN, oobn, _, from, err := unix.Recvmsg(c.fd, buf, oob, 0) if err != nil { if err == syscall.EAGAIN || err == syscall.EWOULDBLOCK { - return 0, time.Time{}, syscall.EAGAIN + return 0, nil, time.Time{}, syscall.EAGAIN } - return 0, time.Time{}, err + return 0, nil, time.Time{}, err } fallbackTime := time.Now() + var src net.IP + if sa, ok := from.(*unix.SockaddrInet4); ok { + src = net.IP(append([]byte(nil), sa.Addr[:]...)) + } + // Raw ICMP sockets include the IPv4 header; strip it so callers // see only the ICMP payload (matching icmp.PacketConn.ReadFrom behavior). hdrLen := stripIPv4Header(buf[:msgN]) @@ -117,11 +125,11 @@ func (c *icmpConn) recvEcho(buf []byte) (int, time.Time, error) { copy(buf, buf[hdrLen:msgN]) if !c.hasKernelTS { - return icmpLen, fallbackTime, nil + return icmpLen, src, fallbackTime, nil } rxTime := parseKernelTimestamp(c.oob[:oobn], fallbackTime) - return icmpLen, rxTime, nil + return icmpLen, src, rxTime, nil } func parseKernelTimestamp(oob []byte, fallback time.Time) time.Time { diff --git a/controlplane/telemetry/internal/geoprobe/icmp_conn_test.go b/controlplane/telemetry/internal/geoprobe/icmp_conn_test.go index 7c2eeea003..493276f999 100644 --- a/controlplane/telemetry/internal/geoprobe/icmp_conn_test.go +++ b/controlplane/telemetry/internal/geoprobe/icmp_conn_test.go @@ -35,9 +35,10 @@ func TestICMPConn_RoundTrip(t *testing.T) { require.NoError(t, conn.setReadDeadline(time.Now().Add(2*time.Second))) buf := make([]byte, 1500) - n, rxTime, err := conn.recvEcho(buf) + n, src, rxTime, err := conn.recvEcho(buf) require.NoError(t, err) assert.Greater(t, n, 0) + assert.True(t, src.Equal(net.IPv4(127, 0, 0, 1)), "reply source should be the probed host, got %v", src) rtt := rxTime.Sub(txTime) assert.GreaterOrEqual(t, rtt, time.Duration(0)) @@ -54,7 +55,7 @@ func TestICMPConn_DeadlineExpired(t *testing.T) { require.NoError(t, conn.setReadDeadline(time.Now().Add(-1*time.Second))) buf := make([]byte, 1500) - _, _, err = conn.recvEcho(buf) + _, _, _, err = conn.recvEcho(buf) assert.Error(t, err) } diff --git a/controlplane/telemetry/internal/geoprobe/icmp_pinger.go b/controlplane/telemetry/internal/geoprobe/icmp_pinger.go index 7eca4e6089..f72e278b41 100644 --- a/controlplane/telemetry/internal/geoprobe/icmp_pinger.go +++ b/controlplane/telemetry/internal/geoprobe/icmp_pinger.go @@ -52,6 +52,7 @@ type icmpProbeEntry struct { type pendingProbe struct { addr ProbeAddress + ip net.IP txTime time.Time } @@ -148,7 +149,7 @@ func (p *ICMPPinger) sendEcho(entry *icmpProbeEntry, seq uint16) (time.Time, err func (p *ICMPPinger) readReplies(pending map[uint16]*pendingProbe, results map[ProbeAddress]uint64) { buf := make([]byte, 1500) for len(pending) > 0 { - n, rxTime, err := p.conn.recvEcho(buf) + n, src, rxTime, err := p.conn.recvEcho(buf) if err != nil { if err == syscall.EAGAIN { // Spurious epoll wakeup; re-enter recvEcho with remaining deadline. @@ -175,6 +176,13 @@ func (p *ICMPPinger) readReplies(pending map[uint16]*pendingProbe, results map[P if !exists { continue } + // ID+seq are guessable, so a reply only counts if it came from the host + // we probed; otherwise any host could answer for another target's RTT. + if !src.Equal(pp.ip) { + p.log.Debug("Ignoring ICMP reply from unexpected source", + "expected", pp.ip.String(), "actual", src.String(), "seq", seq) + continue + } rtt := uint64(max(rxTime.Sub(pp.txTime).Nanoseconds(), 0)) results[pp.addr] = rtt @@ -215,7 +223,7 @@ func (p *ICMPPinger) MeasureOne(ctx context.Context, addr ProbeAddress) (uint64, buf := make([]byte, 1500) for { - n, rxTime, err := p.conn.recvEcho(buf) + n, src, rxTime, err := p.conn.recvEcho(buf) if err != nil { if err == syscall.EAGAIN { // Spurious epoll wakeup; re-enter recvEcho with remaining deadline. @@ -236,6 +244,12 @@ func (p *ICMPPinger) MeasureOne(ctx context.Context, addr ProbeAddress) (uint64, if echo.ID != p.id || uint16(echo.Seq) != seq { continue } + // See readReplies: ID+seq alone would let any host answer for this one. + if !src.Equal(entry.ip) { + p.log.Debug("MeasureOne ignoring ICMP reply from unexpected source", + "expected", entry.ip.String(), "actual", src.String(), "seq", seq) + continue + } rtt := uint64(max(rxTime.Sub(txTime).Nanoseconds(), 0)) p.log.Debug("MeasureOne succeeded", "host", addr.Host, "rtt_ns", rtt) @@ -283,7 +297,7 @@ func (p *ICMPPinger) MeasureAll(ctx context.Context) (map[ProbeAddress]uint64, e p.log.Debug("MeasureAll send failed", "host", entry.addr.Host, "error", err) continue } - pending[seq] = &pendingProbe{addr: entry.addr, txTime: txTime} + pending[seq] = &pendingProbe{addr: entry.addr, ip: entry.ip, txTime: txTime} if j < len(batch)-1 { time.Sleep(p.cfg.StaggerDelay) diff --git a/controlplane/telemetry/internal/geoprobe/icmp_pinger_test.go b/controlplane/telemetry/internal/geoprobe/icmp_pinger_test.go index bed84abbb4..c74840f0a8 100644 --- a/controlplane/telemetry/internal/geoprobe/icmp_pinger_test.go +++ b/controlplane/telemetry/internal/geoprobe/icmp_pinger_test.go @@ -33,7 +33,10 @@ type mockSentPacket struct { } type mockReply struct { - data []byte + data []byte + // src is the reply's source address. When nil the mock answers from the + // host the matching echo request was sent to, like a well-behaved target. + src net.IP rxTime time.Time } @@ -45,16 +48,47 @@ func (m *mockICMPSocket) sendEcho(dst net.IP, payload []byte) (time.Time, error) return txTime, nil } -func (m *mockICMPSocket) recvEcho(buf []byte) (int, time.Time, error) { +func (m *mockICMPSocket) recvEcho(buf []byte) (int, net.IP, time.Time, error) { m.mu.Lock() defer m.mu.Unlock() if len(m.replies) == 0 { - return 0, time.Time{}, syscall.ETIMEDOUT + return 0, nil, time.Time{}, syscall.ETIMEDOUT } reply := m.replies[0] m.replies = m.replies[1:] n := copy(buf, reply.data) - return n, reply.rxTime, nil + src := reply.src + if src == nil { + src = m.probedHostLocked(reply.data) + } + return n, src, reply.rxTime, nil +} + +// probedHostLocked returns the destination of the sent echo request whose seq +// matches the given reply, so unattributed mock replies come from the host that +// was actually probed. Caller holds m.mu. +func (m *mockICMPSocket) probedHostLocked(reply []byte) net.IP { + seq, ok := echoSeq(reply) + if ok { + for _, sent := range m.sent { + if sentSeq, ok := echoSeq(sent.payload); ok && sentSeq == seq { + return sent.dst + } + } + } + return net.IPv4(127, 0, 0, 1) +} + +func echoSeq(pkt []byte) (int, bool) { + msg, err := icmp.ParseMessage(icmpProtocol, pkt) + if err != nil { + return 0, false + } + echo, ok := msg.Body.(*icmp.Echo) + if !ok { + return 0, false + } + return echo.Seq, true } func (m *mockICMPSocket) setReadDeadline(t time.Time) error { @@ -382,3 +416,45 @@ func TestICMPPinger_Integration_MeasureAll_Localhost(t *testing.T) { assert.Greater(t, rtt, uint64(0)) assert.Less(t, rtt, uint64(10*time.Millisecond)) } + +// ID+seq are guessable, so a reply from any other host must not be credited as +// the target's RTT. +func TestICMPPinger_MeasureOne_IgnoresMismatchedSource(t *testing.T) { + mock := &mockICMPSocket{} + p := newMockICMPPinger(mock) + defer p.Close() + addr := ProbeAddress{Host: "44.0.0.1", Port: 1} + require.NoError(t, p.AddProbe(addr)) + + mock.mu.Lock() + mock.replies = append(mock.replies, mockReply{ + data: buildEchoReply(0xBEEF, 1), + src: net.IPv4(44, 0, 0, 9), // matching ID+seq, wrong host + rxTime: time.Now(), + }) + mock.mu.Unlock() + + _, ok := p.MeasureOne(context.Background(), addr) + assert.False(t, ok, "reply from an unprobed host should not produce an RTT") +} + +func TestICMPPinger_MeasureAll_IgnoresMismatchedSource(t *testing.T) { + mock := &mockICMPSocket{} + p := newMockICMPPinger(mock) + defer p.Close() + + addr := ProbeAddress{Host: "44.0.0.1", Port: 1} + require.NoError(t, p.AddProbe(addr)) + + mock.mu.Lock() + mock.replies = append(mock.replies, mockReply{ + data: buildEchoReply(0xBEEF, 1), + src: net.IPv4(44, 0, 0, 9), + rxTime: time.Now(), + }) + mock.mu.Unlock() + + results, err := p.MeasureAll(context.Background()) + require.NoError(t, err) + assert.Empty(t, results, "reply from an unprobed host should not produce a result") +} diff --git a/controlplane/telemetry/internal/geoprobe/metrics.go b/controlplane/telemetry/internal/geoprobe/metrics.go index ce5885b0ab..79111cb80b 100644 --- a/controlplane/telemetry/internal/geoprobe/metrics.go +++ b/controlplane/telemetry/internal/geoprobe/metrics.go @@ -43,6 +43,8 @@ const ( RejectUnknownParent = "unknown_parent" RejectWrongAuthority = "wrong_authority" RejectInvalidSignature = "invalid_signature" + RejectSlotOutOfWindow = "slot_out_of_window" + RejectSlotUnavailable = "slot_unavailable" ) // discoveryBuckets covers RPC-heavy discovery operations which commonly diff --git a/controlplane/telemetry/internal/geoprobe/signer.go b/controlplane/telemetry/internal/geoprobe/signer.go index d85b755327..04f686942f 100644 --- a/controlplane/telemetry/internal/geoprobe/signer.go +++ b/controlplane/telemetry/internal/geoprobe/signer.go @@ -50,6 +50,16 @@ func (s *OffsetSigner) SignOffset(offset *LocationOffset) error { func VerifyOffset(offset *LocationOffset) error { pubkey := solana.PublicKeyFromBytes(offset.AuthorityPubkey[:]) + // ed25519.Verify accepts the all-zero (pubkey, signature) pair for a + // fraction of messages: the zero key decodes to a valid point of order 4 + // rather than being rejected, and with S and R also zero the verification + // equation holds whenever the message hash lands on the right residue — + // about one message in four, which an attacker reaches by varying any field. + // No signer has the zero pubkey, so reject it before verifying. + if pubkey.IsZero() { + return fmt.Errorf("authority pubkey is zero") + } + signingBytes, err := offset.GetSigningBytes() if err != nil { return fmt.Errorf("failed to get signing bytes: %w", err) diff --git a/controlplane/telemetry/internal/geoprobe/signer_test.go b/controlplane/telemetry/internal/geoprobe/signer_test.go index 22bdb537ad..ac93f984b9 100644 --- a/controlplane/telemetry/internal/geoprobe/signer_test.go +++ b/controlplane/telemetry/internal/geoprobe/signer_test.go @@ -438,3 +438,28 @@ func TestNewOffsetSigner_ZeroSenderPubkey(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "sender pubkey must not be zero") } + +// An unsigned offset is what an attacker sends when they cannot sign at all, +// and ed25519.Verify accepts the all-zero (pubkey, signature) pair for a +// fraction of messages: the zero pubkey decodes to a point of order 4 rather +// than to nothing, so the equation holds whenever the message hash lands on the +// right residue — roughly one message in four. Slot 2 is one such message with +// these field values, which is why the slot is pinned: at slot 1 the pair is +// rejected by the math and the test would pass without the guard. +func TestVerifyOffset_ZeroPubkeyAndSignature(t *testing.T) { + t.Parallel() + + offset := &LocationOffset{ + Version: LocationOffsetVersion, + MeasurementSlot: 2, + Lat: 1.0, + Lng: 2.0, + MeasuredRttNs: 1000, + RttNs: 1000, + } + + // Assert the specific failure: without it a later change that made some + // other check fire first would leave this passing while the guard rotted. + require.ErrorContains(t, VerifyOffset(offset), "authority pubkey is zero") + require.ErrorContains(t, VerifyOffsetChain(offset), "authority pubkey is zero") +} diff --git a/controlplane/telemetry/internal/geoprobe/target_discovery.go b/controlplane/telemetry/internal/geoprobe/target_discovery.go index 6375d8e1e1..1836819c25 100644 --- a/controlplane/telemetry/internal/geoprobe/target_discovery.go +++ b/controlplane/telemetry/internal/geoprobe/target_discovery.go @@ -91,14 +91,15 @@ func (d *TargetDiscovery) Tick(ctx context.Context, targetCh chan<- TargetUpdate } func (d *TargetDiscovery) discoverAndSend(ctx context.Context, targetCh chan<- TargetUpdate, keyCh chan<- InboundKeyUpdate, icmpTargetCh chan<- ICMPTargetUpdate) { - targets, icmpTargets, inboundKeys, outboundDelivery, icmpDelivery, err := d.discover(ctx) + scanned, targets, icmpTargets, inboundKeys, outboundDelivery, icmpDelivery, err := d.discover(ctx) if err != nil { d.log.Warn("Target discovery tick failed", "error", err) return } - // nil targets means the scan was skipped (target_update_count unchanged). - if targets == nil && inboundKeys == nil && icmpTargets == nil { + // A completed scan that matched nothing must still propagate: it is how a + // deregistered or delinquent user stops being probed. + if !scanned { return } @@ -133,10 +134,14 @@ func (d *TargetDiscovery) discoverAndSend(ctx context.Context, targetCh chan<- T } // discover performs a single discovery cycle: fetch users, filter, extract targets/keys, -// merge with CLI values. Returns nil, nil, nil, nil, nil, nil when the scan is skipped. +// merge with CLI values. The first return value reports whether the scan actually ran, +// and is meaningful only when err is nil: it is false when the scan was skipped +// (target_update_count unchanged), which callers must not confuse with a scan that ran +// and matched nothing. On error every other return value is unset, so callers check err +// first. // The returned delivery maps map measurement target → result destination for targets // whose user has a non-empty ResultDestination, split by target type. -func (d *TargetDiscovery) discover(ctx context.Context) ([]ProbeAddress, []ProbeAddress, [][32]byte, map[ProbeAddress]string, map[ProbeAddress]string, error) { +func (d *TargetDiscovery) discover(ctx context.Context) (bool, []ProbeAddress, []ProbeAddress, [][32]byte, map[ProbeAddress]string, map[ProbeAddress]string, error) { forceFullRefresh := d.tickCount%targetDiscoveryFullRefreshEvery == 0 d.tickCount++ @@ -145,14 +150,14 @@ func (d *TargetDiscovery) discover(ctx context.Context) ([]ProbeAddress, []Probe if current == d.lastSeenTargetUpdateCount && d.tickCount > 1 { d.log.Debug("GeoProbe target_update_count unchanged, skipping target scan", "targetUpdateCount", current) - return nil, nil, nil, nil, nil, nil + return false, nil, nil, nil, nil, nil, nil } d.lastSeenTargetUpdateCount = current } users, err := d.client.GetGeolocationUsers(ctx) if err != nil { - return nil, nil, nil, nil, nil, fmt.Errorf("failed to fetch GeolocationUser accounts: %w", err) + return false, nil, nil, nil, nil, nil, fmt.Errorf("failed to fetch GeolocationUser accounts: %w", err) } var probePKBytes [32]byte @@ -253,7 +258,7 @@ func (d *TargetDiscovery) discover(ctx context.Context) ([]ProbeAddress, []Probe "icmpDeliveryOverrides", len(icmpDelivery), ) - return onchainTargets, onchainIcmpTargets, onchainKeys, outboundDelivery, icmpDelivery, nil + return true, onchainTargets, onchainIcmpTargets, onchainKeys, outboundDelivery, icmpDelivery, nil } // targetToProbeAddress converts a GeolocationTarget to a ProbeAddress. diff --git a/controlplane/telemetry/internal/geoprobe/target_discovery_test.go b/controlplane/telemetry/internal/geoprobe/target_discovery_test.go index 2553c1679b..5fd6ab73f0 100644 --- a/controlplane/telemetry/internal/geoprobe/target_discovery_test.go +++ b/controlplane/telemetry/internal/geoprobe/target_discovery_test.go @@ -111,7 +111,7 @@ func TestTargetDiscovery_HappyPath(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, keys, _, _, err := td.discover(context.Background()) + _, targets, _, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -137,7 +137,7 @@ func TestTargetDiscovery_StatusFilter(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, keys, _, _, err := td.discover(context.Background()) + _, targets, _, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -160,7 +160,7 @@ func TestTargetDiscovery_PaymentFilter(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, keys, _, _, err := td.discover(context.Background()) + _, targets, _, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -186,7 +186,7 @@ func TestTargetDiscovery_CombinedFilter(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, _, _, _, err := td.discover(context.Background()) + _, targets, _, _, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -209,7 +209,7 @@ func TestTargetDiscovery_ProbePKFilter(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, _, _, _, err := td.discover(context.Background()) + _, targets, _, _, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -230,7 +230,7 @@ func TestTargetDiscovery_InboundTargets(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, keys, _, _, err := td.discover(context.Background()) + _, targets, _, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -260,7 +260,7 @@ func TestTargetDiscovery_MixedTargets(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, keys, _, _, err := td.discover(context.Background()) + _, targets, _, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -333,7 +333,7 @@ func TestTargetDiscovery_DeduplicateInboundKeys(t *testing.T) { } td := newTestTargetDiscovery(client) - _, _, keys, _, _, err := td.discover(context.Background()) + _, _, _, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -387,7 +387,7 @@ func TestTargetDiscovery_TargetUpdateCountUnchanged_SkipsScan(t *testing.T) { }) // First call (tick 0): always does full scan (forceFullRefresh). - targets, _, _, _, _, err := td.discover(context.Background()) + _, targets, _, _, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -399,7 +399,7 @@ func TestTargetDiscovery_TargetUpdateCountUnchanged_SkipsScan(t *testing.T) { } // Second call: counter unchanged → should skip. - targets, _, keys, _, _, err := td.discover(context.Background()) + _, targets, _, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -432,11 +432,11 @@ func TestTargetDiscovery_TargetUpdateCountChanged_DoesFullScan(t *testing.T) { }) // First call: full scan. - _, _, _, _, _, _ = td.discover(context.Background()) + _, _, _, _, _, _, _ = td.discover(context.Background()) // Change counter, second call should do full scan. counter.Store(6) - targets, _, _, _, _, err := td.discover(context.Background()) + _, targets, _, _, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -471,12 +471,12 @@ func TestTargetDiscovery_ForcedFullRefresh_IgnoresCounter(t *testing.T) { // Tick through to the next forced refresh (every 5th tick). // Tick 0: forced (0 % 5 == 0), tick 1-4: skipped (counter unchanged), tick 5: forced. for i := 0; i < targetDiscoveryFullRefreshEvery; i++ { - _, _, _, _, _, _ = td.discover(context.Background()) + _, _, _, _, _, _, _ = td.discover(context.Background()) } callsBefore := client.calls // Next tick (tick 5): forced full refresh even though counter unchanged. - targets, _, _, _, _, err := td.discover(context.Background()) + _, targets, _, _, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -502,7 +502,7 @@ func TestTargetDiscovery_NilProbeTargetUpdateCount_AlwaysScans(t *testing.T) { td := newTestTargetDiscovery(client) for i := 0; i < 3; i++ { - _, _, _, _, _, _ = td.discover(context.Background()) + _, _, _, _, _, _, _ = td.discover(context.Background()) } if client.calls != 3 { t.Errorf("expected 3 RPC calls without ProbeTargetUpdateCount, got %d", client.calls) @@ -534,7 +534,7 @@ func TestTargetDiscovery_RejectsNonPublicOutboundTargets(t *testing.T) { }, } td := newTestTargetDiscovery(client) - targets, _, _, _, _, err := td.discover(context.Background()) + _, targets, _, _, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -556,7 +556,7 @@ func TestTargetDiscovery_OutboundIcmpTargets(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, icmpTargets, keys, _, _, err := td.discover(context.Background()) + _, targets, icmpTargets, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -589,7 +589,7 @@ func TestTargetDiscovery_MixedOutboundAndIcmp(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, icmpTargets, keys, _, _, err := td.discover(context.Background()) + _, targets, icmpTargets, keys, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -627,7 +627,7 @@ func TestTargetDiscovery_OutboundIcmpZeroPortDefaulted(t *testing.T) { } td := newTestTargetDiscovery(client) - _, icmpTargets, _, _, _, err := td.discover(context.Background()) + _, _, icmpTargets, _, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -650,7 +650,7 @@ func TestTargetDiscovery_OutboundIcmpPrivateIPRejected(t *testing.T) { } td := newTestTargetDiscovery(client) - _, icmpTargets, _, _, _, err := td.discover(context.Background()) + _, _, icmpTargets, _, _, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -676,7 +676,7 @@ func TestTargetDiscovery_ResultDestination_OutboundOverride(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, _, delivery, _, err := td.discover(context.Background()) + _, targets, _, _, delivery, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -711,7 +711,7 @@ func TestTargetDiscovery_NoResultDestination_NoDeliveryOverride(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, _, delivery, _, err := td.discover(context.Background()) + _, targets, _, _, delivery, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -740,7 +740,7 @@ func TestTargetDiscovery_ResultDestination_ICMPOverride(t *testing.T) { } td := newTestTargetDiscovery(client) - _, icmpTargets, _, _, delivery, err := td.discover(context.Background()) + _, _, icmpTargets, _, _, delivery, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -783,7 +783,7 @@ func TestTargetDiscovery_ResultDestination_MixedUsers(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, _, delivery, _, err := td.discover(context.Background()) + _, targets, _, _, delivery, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -823,7 +823,7 @@ func TestTargetDiscovery_ResultDestination_DomainName(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, _, delivery, _, err := td.discover(context.Background()) + _, targets, _, _, delivery, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -908,7 +908,7 @@ func TestTargetDiscovery_InvalidResultDestination_Ignored(t *testing.T) { } td := newTestTargetDiscovery(client) - targets, _, _, delivery, _, err := td.discover(context.Background()) + _, targets, _, _, delivery, _, err := td.discover(context.Background()) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -919,3 +919,66 @@ func TestTargetDiscovery_InvalidResultDestination_Ignored(t *testing.T) { t.Errorf("expected no delivery overrides for invalid result destination, got %d", len(delivery)) } } + +// Dropping a user's last target, or flipping them to Delinquent, produces a +// completed scan that matches nothing. That result must propagate, or the probe +// keeps measuring and delivering signed offsets for a user who stopped paying. +func TestTargetDiscovery_EmptyScanPropagates(t *testing.T) { + probePK := testProbePubkey() + client := &mockGeolocationUserClient{ + users: []geolocation.KeyedGeolocationUser{ + makeUser(geolocation.GeolocationUserStatusActivated, geolocation.GeolocationPaymentStatusPaid, "user1", []geolocation.GeolocationTarget{ + outboundTarget([4]uint8{44, 0, 0, 1}, 9000, probePK), + inboundTarget(solana.NewWallet().PublicKey(), probePK), + outboundIcmpTarget([4]uint8{44, 0, 0, 2}, 0, probePK), + }), + }, + } + + td := newTestTargetDiscovery(client) + targetCh := make(chan TargetUpdate, 1) + keyCh := make(chan InboundKeyUpdate, 1) + icmpCh := make(chan ICMPTargetUpdate, 1) + + td.Tick(context.Background(), targetCh, keyCh, icmpCh) + + if update := <-targetCh; len(update.Targets) != 1 { + t.Fatalf("expected 1 target on first tick, got %d", len(update.Targets)) + } + if update := <-keyCh; len(update.Keys) != 1 { + t.Fatalf("expected 1 inbound key on first tick, got %d", len(update.Keys)) + } + if update := <-icmpCh; len(update.Targets) != 1 { + t.Fatalf("expected 1 ICMP target on first tick, got %d", len(update.Targets)) + } + + client.users[0].GeolocationUser.PaymentStatus = geolocation.GeolocationPaymentStatusDelinquent + td.Tick(context.Background(), targetCh, keyCh, icmpCh) + + select { + case update := <-targetCh: + if len(update.Targets) != 0 { + t.Errorf("expected empty target update, got %d targets", len(update.Targets)) + } + default: + t.Error("expected an empty target update to be sent for a delinquent user") + } + + select { + case update := <-keyCh: + if len(update.Keys) != 0 { + t.Errorf("expected empty inbound key update, got %d keys", len(update.Keys)) + } + default: + t.Error("expected an empty inbound key update to be sent for a delinquent user") + } + + select { + case update := <-icmpCh: + if len(update.Targets) != 0 { + t.Errorf("expected empty ICMP target update, got %d targets", len(update.Targets)) + } + default: + t.Error("expected an empty ICMP target update to be sent for a delinquent user") + } +} diff --git a/rfcs/rfc16-geolocation-verification.md b/rfcs/rfc16-geolocation-verification.md index 86ac3d0913..4f1ba73a84 100644 --- a/rfcs/rfc16-geolocation-verification.md +++ b/rfcs/rfc16-geolocation-verification.md @@ -159,7 +159,7 @@ _Inbound Measurement Flow_ 3. **Dual Posting:** DZD submits samples to `ProbeLatencySamples` PDA onchain AND sends Offset to Probe via UDP 4. **Probe Caches Offset:** Probe verifies DZD signature, caches Offset, and updates the signed TWAMP reflector's embedded offsets with the best cached offset 5. **Target Sends Probe 0:** Target sends a signed probe packet (containing its Pubkey and Ed25519 signature). Paired probing is needed because TEEs cannot perform time measurements; the probe-side inter-arrival time serves as a proxy RTT. -6. **Probe Sends Reply 0:** Reflector checks that the sender's Pubkey is registered (it does not verify the probe's Ed25519 signature — the pubkey allowlist is sufficient). Reply 0 embeds the original probe, the reflector's signing key, geoprobe identity, location data derived from its best cached DZD offset (`Lat`, `Lng`, `MeasurementSlot`, `RttNs`), the opaque DZD offset blobs, and an Ed25519 signature over the whole packet. Reply 0's `SinceLastRxNs` carries an 8-byte challenge nonce (see "Challenge-Response Inbound Probing" below); legacy senders that ignore the field see no behavior change. +6. **Probe Sends Reply 0:** Reflector checks that the sender's Pubkey is registered, and verifies the probe's Ed25519 signature before touching any per-sender pair state (source IP, pair count, challenge nonce) so spoofed probes cannot disturb a legitimate sender. A probe that fails verification is still replied to — verifying the probe is not required to reply — but off throwaway state and rate-limited. Reply 0 embeds the original probe, the reflector's signing key, geoprobe identity, location data derived from its best cached DZD offset (`Lat`, `Lng`, `MeasurementSlot`, `RttNs`), the opaque DZD offset blobs, and an Ed25519 signature over the whole packet. Reply 0's `SinceLastRxNs` carries an 8-byte challenge nonce (see "Challenge-Response Inbound Probing" below); legacy senders that ignore the field see no behavior change. 7. **Target Sends Probe 1:** Senders not using challenged inbound probing (see "Challenge-Response Inbound Probing" below) pre-sign both probes before any network I/O and fire Probe 1 the moment Reply 0 arrives, with no signing delay. Challenged senders defer signing Probe 1 until they have parsed the Reply 0 nonce; see that section for the trade-off. 8. **Probe Sends Reply 1:** Same structure as reply 0, but `SinceLastRxNs` now contains the time between the reflector's reply 0 Tx and probe 1 Rx — this approximates the network RTT by excluding processing overhead on both sides. `RttNs` = DZD offset's `RttNs` + `SinceLastRxNs`. The reflector allows 2 probes per rate-limit window per sender, then drops until the window resets. 9. **Target Verifies Replies:** Target verifies both replies, then derives two RTT measurements: "Probe-Measured RTT" = `reply1.SinceLastRxNs` (Tx-to-Rx interval at the reflector), and "Target-Measured RTT" = `min(rtt0, rtt1)` (lower of the two sender-side RTTs). Forwards to Client Oracle. diff --git a/tools/twamp/pkg/signed/packet.go b/tools/twamp/pkg/signed/packet.go index 21322b2378..c10f3b926e 100644 --- a/tools/twamp/pkg/signed/packet.go +++ b/tools/twamp/pkg/signed/packet.go @@ -184,6 +184,10 @@ func UnmarshalProbePacket(buf []byte) (*ProbePacket, error) { } func (p *ProbePacket) Verify() bool { + if isZeroPubkey(p.SenderPubkey) { + return false + } + var payload [probePayloadSize]byte binary.BigEndian.PutUint32(payload[0:4], p.Seq) binary.BigEndian.PutUint32(payload[4:8], p.Sec) @@ -193,6 +197,18 @@ func (p *ProbePacket) Verify() bool { return ed25519.Verify(ed25519.PublicKey(p.SenderPubkey[:]), payload[:], p.Signature[:]) } +// isZeroPubkey reports whether pk is the all-zero key. Both ProbePacket and +// ReplyPacket verify with a pubkey taken straight off the wire, and +// ed25519.Verify accepts the all-zero (pubkey, signature) pair for a fraction +// of messages: the zero key decodes to a valid point of order 4 rather than +// being rejected, and with S and R also zero the verification equation holds +// whenever the message hash lands on the right residue — about one message in +// four, which an attacker reaches by varying a sequence number or timestamp. +// No signer has the zero pubkey, so reject it before verifying. +func isZeroPubkey(pk [32]byte) bool { + return pk == [32]byte{} +} + // marshalPayload writes the signed portion of the reply (everything before the // signature) into buf and returns the number of bytes written. func (r *ReplyPacket) marshalPayload(buf []byte) (int, error) { @@ -327,6 +343,10 @@ func NewReplyPacket(probe *ProbePacket, signer Signer, geoprobePubkey [32]byte, } func (r *ReplyPacket) Verify() bool { + if isZeroPubkey(r.AuthorityPubkey) { + return false + } + payloadSize := replyHeaderSize + len(r.Offsets)*LocationOffsetSize payload := make([]byte, payloadSize) if _, err := r.marshalPayload(payload); err != nil { diff --git a/tools/twamp/pkg/signed/packet_test.go b/tools/twamp/pkg/signed/packet_test.go index 3c6c2df685..67a2933ee2 100644 --- a/tools/twamp/pkg/signed/packet_test.go +++ b/tools/twamp/pkg/signed/packet_test.go @@ -669,3 +669,48 @@ func TestParseOffsetInfo(t *testing.T) { assert.False(t, ok) }) } + +// An unsigned datagram is what an attacker sends when they cannot sign at all, +// and ed25519.Verify accepts the all-zero (pubkey, signature) pair for a +// fraction of messages: the zero pubkey decodes to a point of order 4 rather +// than to nothing, so the verification equation holds whenever the message hash +// lands on the right residue — roughly one message in four. Seq 1 is one such +// message with every other probe field zero, which is why the seq is fixed here +// rather than left at 0 (which is one of the messages that happens to fail). +func TestProbePacket_Verify_ZeroPubkeyAndSignature(t *testing.T) { + t.Parallel() + + buf := make([]byte, signed.ProbePacketSize) + binary.BigEndian.PutUint32(buf[0:4], 1) + + probe, err := signed.UnmarshalProbePacket(buf) + require.NoError(t, err) + require.Equal(t, [32]byte{}, probe.SenderPubkey) + require.Equal(t, [64]byte{}, probe.Signature) + + assert.False(t, probe.Verify(), "unsigned probe with a zero sender pubkey must not verify") +} + +func TestReplyPacket_Verify_ZeroPubkeyAndSignature(t *testing.T) { + t.Parallel() + + reply, err := signed.UnmarshalReplyPacket(make([]byte, signed.MinReplyPacketSize)) + require.NoError(t, err) + require.Equal(t, [32]byte{}, reply.AuthorityPubkey) + require.Equal(t, [64]byte{}, reply.Signature) + + assert.False(t, reply.Verify(), "unsigned reply with a zero authority pubkey must not verify") + + // Same check on a real reply whose authority and signature are stripped, so + // the guard is not passing only because of this one payload's hash. + _, senderSigner := newTestSigner(t) + _, reflectorSigner := newTestSigner(t) + probe := signed.NewProbePacket(1, senderSigner) + signedReply, err := signed.NewReplyPacket(probe, reflectorSigner, [32]byte{}, nil, 42, 1.0, 2.0, 3, 4, false) + require.NoError(t, err) + require.True(t, signedReply.Verify()) + + signedReply.AuthorityPubkey = [32]byte{} + signedReply.Signature = [64]byte{} + assert.False(t, signedReply.Verify(), "zeroing the authority pubkey must not make a reply verify") +} diff --git a/tools/twamp/pkg/signed/reflector_linux.go b/tools/twamp/pkg/signed/reflector_linux.go index 22d6e7e0a4..cf712db379 100644 --- a/tools/twamp/pkg/signed/reflector_linux.go +++ b/tools/twamp/pkg/signed/reflector_linux.go @@ -19,6 +19,12 @@ import ( const ( defaultReadTimeout = 1 * time.Second stalePairTimeout = 5 * time.Second + + // minUnverifiedReplyInterval floors the rate limit on replies to probes that + // fail signature verification. verifyInterval of 0 disables pair rate + // limiting, which is fine for probes we authenticated but would leave the + // unverified reply path — an order of magnitude of amplification — uncapped. + minUnverifiedReplyInterval = 1 * time.Second ) // senderState fields are only accessed from the single-goroutine epoll @@ -29,6 +35,10 @@ type senderState struct { pairStart time.Time pairSourceIP [4]byte nonce uint64 // challenge nonce issued in Reply 0 of the current pair; 0 outside a pair + // lastUnverifiedRx rate-limits replies to probes that fail signature + // verification. It is the one field written from unverified input, and + // nothing in the pair flow reads it. + lastUnverifiedRx time.Time } type LinuxReflector struct { @@ -224,6 +234,29 @@ func (r *LinuxReflector) Run(ctx context.Context) error { raw, _ := r.senderStates.LoadOrStore(probe.SenderPubkey, &senderState{}) state := raw.(*senderState) + // RFC-16 leaves per-probe signature verification to the target, so an + // unverified probe still gets a reply — but it must not touch the pair + // state a legitimate sender depends on. target_pk is public onchain, + // so spoofed probes would otherwise consume the sender's pair budget, + // repoint pairSourceIP, and clear its challenge nonce, denying it + // inbound geolocation. Reply off a throwaway state instead: the nonce + // it carries is never stored, so it authenticates nothing. + if !probe.Verify() { + // Cap unverified replies at one per window per pubkey: the reply + // is an order of magnitude larger than the probe, so an unlimited + // reply path is a reflection amplifier. + interval := max(r.verifyInterval, minUnverifiedReplyInterval) + if !state.lastUnverifiedRx.IsZero() && now.Sub(state.lastUnverifiedRx) < interval { + continue + } + state.lastUnverifiedRx = now + state = &senderState{} + if r.logger != nil { + r.logger.Warn("replying to probe with invalid signature without touching pair state", + "sender_pubkey", fmt.Sprintf("%x", probe.SenderPubkey), "from", from) + } + } + // Pair-based rate limiting: allow 2 probes per window, then drop. if interval := r.verifyInterval; interval > 0 { if state.pairCount >= 2 { @@ -245,8 +278,8 @@ func (r *LinuxReflector) Run(ctx context.Context) error { } // Pair integrity: both probes must come from the same source IP. - // The pubkey allowlist (checked above) provides authentication; - // per-probe signature verification is left to the target. + // Only verified probes reach this state, so a spoofed source cannot + // claim or repoint the pair. fromAddr, ok := from.(*unix.SockaddrInet4) if !ok { continue diff --git a/tools/twamp/pkg/signed/reflector_test.go b/tools/twamp/pkg/signed/reflector_test.go index ab37ecb8d6..c5054073e0 100644 --- a/tools/twamp/pkg/signed/reflector_test.go +++ b/tools/twamp/pkg/signed/reflector_test.go @@ -749,6 +749,136 @@ func TestReflector_Linux(t *testing.T) { _, err = conn.Read(replyBuf) assert.Error(t, err, "third probe should be rate-limited") }) + + t.Run("spoofed probe does not disturb a legitimate sender's pair", func(t *testing.T) { + t.Parallel() + + senderPub, senderSigner := newTestSigner(t) + _, reflectorSigner := newTestSigner(t) + + var senderPubKey [32]byte + copy(senderPubKey[:], senderPub) + + // verifyInterval exceeds the test duration: only two probes per window, + // so if spoofed packets consume the pair budget the legitimate pair can + // never complete. + reflector, err := signed.NewLinuxReflector("127.0.0.1:0", 100*time.Millisecond, reflectorSigner, [32]byte{}, [][32]byte{senderPubKey}, 10*time.Second) + require.NoError(t, err) + defer reflector.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + go func() { _ = reflector.Run(ctx) }() + time.Sleep(10 * time.Millisecond) + + conn, err := net.DialUDP("udp", nil, &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: int(reflector.Port())}) + require.NoError(t, err) + defer conn.Close() + + send := func(probe *signed.ProbePacket) { + t.Helper() + var buf [signed.ProbePacketSize]byte + require.NoError(t, probe.Marshal(buf[:])) + _, err := conn.Write(buf[:]) + require.NoError(t, err) + } + readReply := func() *signed.ReplyPacket { + t.Helper() + require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second))) + replyBuf := make([]byte, signed.MaxReplyPacketSize) + n, err := conn.Read(replyBuf) + require.NoError(t, err) + reply, err := signed.UnmarshalReplyPacket(replyBuf[:n]) + require.NoError(t, err) + return reply + } + // A reply to a spoofed probe is allowed; drain whatever arrives. + drain := func() { + t.Helper() + require.NoError(t, conn.SetReadDeadline(time.Now().Add(200*time.Millisecond))) + _, _ = conn.Read(make([]byte, signed.MaxReplyPacketSize)) + } + spoof := func(seq uint32) *signed.ProbePacket { + t.Helper() + p := signed.NewProbePacket(seq, senderSigner) + p.Signature[0] ^= 0xff // target_pk is public onchain; the signature is not forgeable + return p + } + + // Spoofed probe before the pair starts must not consume the budget or + // claim the pair's source IP. + send(spoof(90)) + drain() + + probe0 := signed.NewProbePacket(1, senderSigner) + send(probe0) + reply0 := readReply() + nonce := reply0.SinceLastRxNs + require.NotEqual(t, uint64(0), nonce, "Reply 0 should carry a challenge nonce") + + // Spoofed probe mid-pair must not clear the issued nonce. + send(spoof(91)) + drain() + + probe1 := signed.NewProbePacket(2, senderSigner) + overwriteProbeSecFracAndResign(t, probe1, nonce, senderSigner) + send(probe1) + reply1 := readReply() + + assert.True(t, reply1.Challenged, "legitimate Reply 1 should still be Challenged after spoofed probes") + }) + + t.Run("unverified replies stay capped with rate limiting disabled", func(t *testing.T) { + t.Parallel() + + senderPub, senderSigner := newTestSigner(t) + _, reflectorSigner := newTestSigner(t) + + var senderPubKey [32]byte + copy(senderPubKey[:], senderPub) + + // verifyInterval 0 disables pair rate limiting. Replies to probes we + // could not authenticate must still be capped, or the reflector is a + // UDP amplifier for anyone who knows an authorized pubkey. + reflector, err := signed.NewLinuxReflector("127.0.0.1:0", 100*time.Millisecond, reflectorSigner, [32]byte{}, [][32]byte{senderPubKey}, 0) + require.NoError(t, err) + defer reflector.Close() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + go func() { _ = reflector.Run(ctx) }() + time.Sleep(10 * time.Millisecond) + + conn, err := net.DialUDP("udp", nil, &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: int(reflector.Port())}) + require.NoError(t, err) + defer conn.Close() + + sendSpoofed := func(seq uint32) { + t.Helper() + probe := signed.NewProbePacket(seq, senderSigner) + probe.Signature[0] ^= 0xff + var buf [signed.ProbePacketSize]byte + require.NoError(t, probe.Marshal(buf[:])) + _, err := conn.Write(buf[:]) + require.NoError(t, err) + } + + replyBuf := make([]byte, signed.MaxReplyPacketSize) + + sendSpoofed(1) + require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second))) + _, err = conn.Read(replyBuf) + require.NoError(t, err, "the first unverified probe is answered") + + for seq := uint32(2); seq <= 5; seq++ { + sendSpoofed(seq) + } + require.NoError(t, conn.SetReadDeadline(time.Now().Add(300*time.Millisecond))) + _, err = conn.Read(replyBuf) + assert.Error(t, err, "further unverified probes inside the window must not be answered") + }) } // overwriteProbeSecFracAndResign rewrites the Sec/Frac fields of probe to