path: Add length check to TLV payload decoding

.github/workflows/go.yml

@@ -6,22 +6,17 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        go: [1.16, 1.17]
+        go: [1.21, 1.22]
     steps:
+      - name: Check out source
+        uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 #v4.1.1
       - name: Set up Go
-        uses: actions/setup-go@v2
+        uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 #v5.0.0
         with:
-          go-version: ${{ matrix.go }}
-      - name: Check out source
-        uses: actions/checkout@v2
+          go-version: ${{ matrix.go  }}
       - name: Install Linters
-        run: "curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(go env GOPATH)/bin v1.42.0"
+        run: "go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.56.0"
       - name: Build
-        env:
-          GO111MODULE: "on"
         run: go build ./...
       - name: Test
-        env:
-          GO111MODULE: "on"
-        run: |
-          sh ./goclean.sh
+        run: sh ./goclean.sh

bench_test.go

@@ -68,8 +68,8 @@ func BenchmarkProcessPacket(b *testing.B) {
 		b.Fatalf("unable to create test route: %v", err)
 	}
 	b.ReportAllocs()
-	path[0].log.Start()
-	defer path[0].log.Stop()
+	requireNoErr(b, path[0].log.Start())
+	b.Cleanup(func() { requireNoErr(b, path[0].log.Stop()) })
 	b.StartTimer()
 
 	var (
@@ -83,14 +83,14 @@ func BenchmarkProcessPacket(b *testing.B) {
 
 		b.StopTimer()
 		router := path[0]
-		router.log.Stop()
+		requireNoErr(b, router.log.Stop())
 		path[0] = &Router{
 			nodeID:   router.nodeID,
 			nodeAddr: router.nodeAddr,
 			onionKey: router.onionKey,
 			log:      NewMemoryReplayLog(),
 		}
-		path[0].log.Start()
+		requireNoErr(b, path[0].log.Start())
 		b.StartTimer()
 	}
 

cmd/main.go

@@ -142,8 +142,15 @@ func main() {
 			privKeyECDH, chaincfg.TestNet3Params(), replayLog,
 		)
 
-		replayLog.Start()
-		defer replayLog.Stop()
+		if err := replayLog.Start(); err != nil {
+			log.Fatalf("Error starting replay log: %v", err)
+		}
+		defer func() {
+			err := replayLog.Stop()
+			if err != nil {
+				log.Fatalf("Error stopping replay log: %v", err)
+			}
+		}()
 
 		var packet sphinx.OnionPacket
 		err = packet.Decode(bytes.NewBuffer(binMsg))

crypto.go

@@ -60,8 +60,8 @@ func (p *PrivKeyECDH) PubKey() *secp256k1.PublicKey {
 // k is our private key, and P is the public key, we perform the following
 // operation:
 //
-//  sx := k*P
-//  s := sha256(sx.SerializeCompressed())
+//	sx := k*P
+//	s := sha256(sx.SerializeCompressed())
 //
 // NOTE: This is part of the SingleKeyECDH interface.
 func (p *PrivKeyECDH) ECDH(pub *secp256k1.PublicKey) ([32]byte, error) {
@@ -207,16 +207,6 @@ func blindBaseElement(blindingFactor []byte) *secp256k1.PublicKey {
 	return secp256k1.NewPublicKey(&result.X, &result.Y)
 }
 
-// sharedSecretGenerator is an interface that abstracts away exactly *how* the
-// shared secret for each hop is generated.
-//
-// TODO(roasbef): rename?
-type sharedSecretGenerator interface {
-	// generateSharedSecret given a public key, generates a shared secret
-	// using private data of the underlying sharedSecretGenerator.
-	generateSharedSecret(dhKey *secp256k1.PublicKey) (Hash256, error)
-}
-
 // generateSharedSecret generates the shared secret by given ephemeral key.
 func (r *Router) generateSharedSecret(dhKey *secp256k1.PublicKey) (Hash256, error) {
 	var sharedSecret Hash256

log.go

@@ -2,15 +2,7 @@ package sphinx
 
 import "github.com/decred/slog"
 
-// sphxLog  is a logger that is initialized with no output filters.  This
-// means the package will not perform any logging by default until the caller
-// requests it.
-// The default amount of logging is none.
-var sphxLog = slog.Disabled
-
 // UseLogger uses a specified Logger to output package logging info.
 // This should be used in preference to SetLogWriter if the caller is also
 // using slog.
-func UseLogger(logger slog.Logger) {
-	sphxLog = logger
-}
+func UseLogger(logger slog.Logger) {}

path.go

@@ -6,6 +6,7 @@ import (
 	"encoding/binary"
 	"fmt"
 	"io"
+	"math"
 
 	"github.com/decred/dcrd/dcrec/secp256k1/v4"
 	"github.com/decred/dcrd/wire"
@@ -261,6 +262,10 @@ func (hp *HopPayload) Decode(r io.Reader) error {
 		}
 
 		payloadSize = uint32(varInt)
+		if payloadSize > math.MaxUint16 {
+			return fmt.Errorf("payload size %d larger than max allowed %d",
+				payloadSize, math.MaxUint16)
+		}
 		hp.Type = PayloadTLV
 	}
 

replaylog_test.go

@@ -9,8 +9,8 @@ import (
 // MemoryReplayLog work as expected.
 func TestMemoryReplayLogStorageAndRetrieval(t *testing.T) {
 	rl := NewMemoryReplayLog()
-	rl.Start()
-	defer rl.Stop()
+	requireNoErr(t, rl.Start())
+	t.Cleanup(func() { requireNoErr(t, rl.Stop()) })
 
 	var hashPrefix HashPrefix
 	hashPrefix[0] = 1
@@ -86,8 +86,8 @@ func TestMemoryReplayLogStorageAndRetrieval(t *testing.T) {
 // works as expected.
 func TestMemoryReplayLogPutBatch(t *testing.T) {
 	rl := NewMemoryReplayLog()
-	rl.Start()
-	defer rl.Stop()
+	requireNoErr(t, rl.Start())
+	t.Cleanup(func() { requireNoErr(t, rl.Stop()) })
 
 	var hashPrefix1, hashPrefix2 HashPrefix
 	hashPrefix1[0] = 1

sphinx.go

@@ -522,7 +522,7 @@ func (r *Router) Start() error {
 // Stop stops / closes the ReplayLog's channeldb and its accompanying
 // garbage collector goroutine.
 func (r *Router) Stop() {
-	r.log.Stop()
+	_ = r.log.Stop()
 }
 
 // ProcessOnionPacket processes an incoming onion packet which has been forward

sphinx_test.go

@@ -176,12 +176,12 @@ func TestSphinxCorrectness(t *testing.T) {
 	// Now simulate the message propagating through the mix net eventually
 	// reaching the final destination.
 	for i := 0; i < len(nodes); i++ {
-		// Start each node's ReplayLog and defer shutdown
-		nodes[i].log.Start()
-		defer nodes[i].log.Stop()
-
 		hop := nodes[i]
 
+		// Start each node's ReplayLog and defer shutdown
+		requireNoErr(t, hop.log.Start())
+		t.Cleanup(func() { requireNoErr(t, hop.log.Stop()) })
+
 		t.Logf("Processing at hop: %v \n", i)
 		onionPacket, err := hop.ProcessOnionPacket(fwdMsg, nil, uint32(i)+1)
 		if err != nil {
@@ -240,8 +240,8 @@ func TestSphinxSingleHop(t *testing.T) {
 	}
 
 	// Start the ReplayLog and defer shutdown
-	nodes[0].log.Start()
-	defer nodes[0].log.Stop()
+	requireNoErr(t, nodes[0].log.Start())
+	t.Cleanup(func() { requireNoErr(t, nodes[0].log.Stop()) })
 
 	// Simulating a direct single-hop payment, send the sphinx packet to
 	// the destination node, making it process the packet fully.
@@ -267,8 +267,8 @@ func TestSphinxNodeRelpay(t *testing.T) {
 	}
 
 	// Start the ReplayLog and defer shutdown
-	nodes[0].log.Start()
-	defer nodes[0].log.Stop()
+	requireNoErr(t, nodes[0].log.Start())
+	t.Cleanup(func() { requireNoErr(t, nodes[0].log.Stop()) })
 
 	// Allow the node to process the initial packet, this should proceed
 	// without any failures.
@@ -292,8 +292,8 @@ func TestSphinxNodeRelpaySameBatch(t *testing.T) {
 	}
 
 	// Start the ReplayLog and defer shutdown
-	nodes[0].log.Start()
-	defer nodes[0].log.Stop()
+	requireNoErr(t, nodes[0].log.Start())
+	t.Cleanup(func() { requireNoErr(t, nodes[0].log.Stop()) })
 
 	tx := nodes[0].BeginTxn([]byte("0"), 2)
 
@@ -338,8 +338,8 @@ func TestSphinxNodeRelpayLaterBatch(t *testing.T) {
 	}
 
 	// Start the ReplayLog and defer shutdown
-	nodes[0].log.Start()
-	defer nodes[0].log.Stop()
+	requireNoErr(t, nodes[0].log.Start())
+	t.Cleanup(func() { requireNoErr(t, nodes[0].log.Stop()) })
 
 	tx := nodes[0].BeginTxn([]byte("0"), 1)
 
@@ -383,8 +383,8 @@ func TestSphinxNodeReplayBatchIdempotency(t *testing.T) {
 	}
 
 	// Start the ReplayLog and defer shutdown
-	nodes[0].log.Start()
-	defer nodes[0].log.Stop()
+	requireNoErr(t, nodes[0].log.Start())
+	t.Cleanup(func() { requireNoErr(t, nodes[0].log.Stop()) })
 
 	tx := nodes[0].BeginTxn([]byte("0"), 1)
 
@@ -434,8 +434,8 @@ func TestSphinxAssocData(t *testing.T) {
 	}
 
 	// Start the ReplayLog and defer shutdown
-	nodes[0].log.Start()
-	defer nodes[0].log.Stop()
+	requireNoErr(t, nodes[0].log.Start())
+	t.Cleanup(func() { requireNoErr(t, nodes[0].log.Stop()) })
 
 	_, err = nodes[0].ProcessOnionPacket(fwdMsg, []byte("somethingelse"), 1)
 	if err == nil {
@@ -676,12 +676,12 @@ func TestSphinxHopVariableSizedPayloads(t *testing.T) {
 		// of hops here as virtual EOB hops may have been inserted into
 		// the route.
 		for i := 0; i < len(routers); i++ {
-			// Start each node's ReplayLog and defer shutdown
-			routers[i].log.Start()
-			defer routers[i].log.Stop()
-
 			currentHop := routers[i]
 
+			// Start each node's ReplayLog and defer shutdown
+			requireNoErr(t, currentHop.log.Start())
+			t.Cleanup(func() { requireNoErr(t, currentHop.log.Stop()) })
+
 			// Ensure that this hop is able to properly process
 			// this onion packet. If additional EOB hops were
 			// added, then it should be able to properly decrypt

util_test.go

@@ -0,0 +1,10 @@
+package sphinx
+
+import "testing"
+
+func requireNoErr(t testing.TB, err error) {
+	t.Helper()
+	if err != nil {
+		t.Fatal(err)
+	}
+}