Repository: scionproto/scion
Pull request #4556 (2024-06-28 13:15:03)

router: strengthen the checks performed by parsePath
This PR strengthens the checks performed by the `parsePath` method to address the issues https://github.com/scionproto/scion/issues/4524 and https://github.com/scionproto/scion/issues/4531. In particular, I added two checks:
- We check that the `CurrHF` field of the parsed path is compatible with the `CurrINF` field
- We check that non-peering segments have at least two hops

This PR is still not complete. I will add a test here after we prove, in VerifiedSCION, that these changes fix our issues. One question that I have regarding the code is whether we should return a `malformedPath` or a `cannotRoute` error. I chose the former, but I would be happy to change that if you think it makes more sense.

jiceatscion: 

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/scionproto/scion/4556)
<!-- Reviewable:end -->

jcp19: Hi @matzf, thank you for the guidance in fixing the test :)

Since the first version of the PR, I strengthened the check performed in `parsePath` -- turns out that the version I had before (which only checked that the current segment had at least two hops) would not reject packets for which the two following conditions hold simultaneously:
1. `IncPath` is called twice during processing (one in `doXover` and the other in `processEgress`)
2. the core segment has size one
With these two properties, we would essentially be performing two Xovers and miss the check on the core segment.

We are now confident that this solution is correct, but we need to finish the proof to be sure -- the proof is available [here](https://github.com/viperproject/VerifiedSCION/pull/364) and it is currently failing on a lemma that we need to refactor.
jcp19: @matzf Should be ready to merge now :)

Patch:
diff --git a/pkg/slayers/path/scion/raw.go b/pkg/slayers/path/scion/raw.go
index 859109377..c8e979629 100644
--- a/pkg/slayers/path/scion/raw.go
+++ b/pkg/slayers/path/scion/raw.go
@@ -172,3 +172,9 @@ func (s *Raw) IsPenultimateHop() bool {
 func (s *Raw) IsLastHop() bool {
 	return int(s.PathMeta.CurrHF) == (s.NumHops - 1)
 }
+
+// CurrINFMatchesCurrHF returns whether the the path's current hopfield
+// is in the path's current segment.
+func (s *Raw) CurrINFMatchesCurrHF() bool {
+	return s.PathMeta.CurrINF == s.infIndexForHF(s.PathMeta.CurrHF)
+}
diff --git a/router/dataplane.go b/router/dataplane.go
index 9117c521a..2fac11ac1 100644
--- a/router/dataplane.go
+++ b/router/dataplane.go
@@ -1359,6 +1359,17 @@ func (p *scionPacketProcessor) parsePath() disposition {
 		// TODO(lukedirtwalker) parameter problem invalid path?
 		return errorDiscard("error", err)
 	}
+	// Segments without the Peering flag must consist of at least two HFs:
+	// https://github.com/scionproto/scion/issues/4524
+	hasSingletonSegment := p.path.PathMeta.SegLen[0] == 1 ||
+		p.path.PathMeta.SegLen[1] == 1 ||
+		p.path.PathMeta.SegLen[2] == 1
+	if !p.infoField.Peer && hasSingletonSegment {
+		return errorDiscard("error", malformedPath)
+	}
+	if !p.path.CurrINFMatchesCurrHF() {
+		return errorDiscard("error", malformedPath)
+	}
 	return pForward
 }
 


Test patch:
diff --git a/router/dataplane_test.go b/router/dataplane_test.go
index 67eef3d48..2ea5e0645 100644
--- a/router/dataplane_test.go
+++ b/router/dataplane_test.go
@@ -1185,8 +1185,9 @@ func TestProcessPkt(t *testing.T) {
 				dpath := &scion.Decoded{
 					Base: scion.Base{
 						PathMeta: scion.MetaHdr{
-							CurrHF: 2,
-							SegLen: [3]uint8{2, 2, 0},
+							CurrINF: 0,
+							CurrHF:  1,
+							SegLen:  [3]uint8{2, 2, 0},
 						},
 						NumINF:  2,
 						NumHops: 4,
@@ -1198,24 +1199,26 @@ func TestProcessPkt(t *testing.T) {
 						{SegID: 0x222, ConsDir: false, Timestamp: util.TimeToSecs(now)},
 					},
 					HopFields: []path.HopField{
-						{ConsIngress: 0, ConsEgress: 1},  // IA 110
 						{ConsIngress: 31, ConsEgress: 0}, // Src
-						{ConsIngress: 0, ConsEgress: 51}, // Dst
+						{ConsIngress: 0, ConsEgress: 51}, // IA 110
 						{ConsIngress: 3, ConsEgress: 0},  // IA 110
+						{ConsIngress: 0, ConsEgress: 1},  // Dst
 					},
 				}
-				dpath.HopFields[2].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[2])
-				dpath.HopFields[3].Mac = computeMAC(t, key, dpath.InfoFields[1], dpath.HopFields[3])
+				dpath.HopFields[1].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[1])
+				dpath.HopFields[2].Mac = computeMAC(t, key, dpath.InfoFields[1], dpath.HopFields[2])
 
 				var dstAddr *net.UDPAddr
 				ingress := uint16(51) // == consEgress, bc non-consdir
 				egress := uint16(0)   // Cross-over. The egress happens in the next segment.
 				if afterProcessing {
-					require.NoError(t, dpath.IncPath())
+					dpath.PathMeta.CurrHF++
+					dpath.PathMeta.CurrINF++
 					dstAddr = &net.UDPAddr{IP: net.ParseIP("10.0.200.200").To4(), Port: 30043}
 				} else {
-					dpath.InfoFields[0].UpdateSegID(dpath.HopFields[2].Mac)
+					dpath.InfoFields[0].UpdateSegID(dpath.HopFields[1].Mac)
 				}
+
 				return router.NewPacket(toBytes(t, spkt, dpath), nil, dstAddr, ingress, egress)
 			},
 			assertFunc: notDiscarded,