Repository: scionproto/scion
Pull request #4506 (2024-04-15 14:58:57)

router: return SCMP on invalid dst address
This PR fixes issue #4126.

Currently, it is missing a test in `braccept`.

matzf: 

<!-- 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/4506)
<!-- Reviewable:end -->

jcp19: @matzf Yes, that's the plan. However, I haven't add enough time yet to look into how to test for this particular behaviour.
jiceatscion: In router/dataplane_internal_test, there's a series of test cases for SCMP responses: TestSlowPathProcessing().
You might be able to add one test case in-there with not too much fuss.

jcp19: I finally took a look at this and added a test now. should be ready for review

Patch:
diff --git a/router/dataplane.go b/router/dataplane.go
index 76a92e434..02324114d 100644
--- a/router/dataplane.go
+++ b/router/dataplane.go
@@ -122,6 +122,7 @@ var (
 	invalidSrcIA                  = errors.New("invalid source ISD-AS")
 	invalidDstIA                  = errors.New("invalid destination ISD-AS")
 	invalidSrcAddrForTransit      = errors.New("invalid source address for transit pkt")
+	invalidDstAddr                = errors.New("invalid destination address")
 	cannotRoute                   = errors.New("cannot route, dropping pkt")
 	emptyValue                    = errors.New("empty value")
 	malformedPath                 = errors.New("malformed path content")
@@ -1588,6 +1589,14 @@ func (p *scionPacketProcessor) resolveInbound() (*net.UDPAddr, processResult, er
 			cause:    err,
 		}
 		return nil, processResult{SlowPathRequest: slowPathRequest}, slowPathRequired
+	case errors.Is(err, invalidDstAddr):
+		log.Debug("SCMP: invalid destination address")
+		slowPathRequest := slowPathRequest{
+			scmpType: slayers.SCMPTypeParameterProblem,
+			code:     slayers.SCMPCodeInvalidDestinationAddress,
+			cause:    err,
+		}
+		return nil, processResult{SlowPathRequest: slowPathRequest}, slowPathRequired
 	default:
 		return a, processResult{}, nil
 	}
@@ -1988,8 +1997,7 @@ func (d *DataPlane) resolveLocalDst(
 
 	dst, err := s.DstAddr()
 	if err != nil {
-		// TODO parameter problem.
-		return nil, err
+		return nil, invalidDstAddr
 	}
 	switch dst.Type() {
 	case addr.HostTypeSVC:


Test patch:
diff --git a/router/dataplane_internal_test.go b/router/dataplane_internal_test.go
index 7b9914e7f..393e289ab 100644
--- a/router/dataplane_internal_test.go
+++ b/router/dataplane_internal_test.go
@@ -512,6 +512,30 @@ func TestSlowPathProcessing(t *testing.T) {
 			},
 			expectedLayerType: slayers.LayerTypeSCMPParameterProblem,
 		},
+		"invalid dest addr": {
+			prepareDP: func(ctrl *gomock.Controller) *DataPlane {
+				return NewDP(fakeExternalInterfaces,
+					nil, mock_router.NewMockBatchConn(ctrl),
+					fakeInternalNextHops,
+					map[addr.SVC][]*net.UDPAddr{},
+					xtest.MustParseIA("1-ff00:0:110"), nil, testKey)
+			},
+			mockMsg: func() []byte {
+				spkt := prepBaseMsg(t, payload, 0)
+				spkt.RawDstAddr = []byte("invalid")
+				spkt.DstAddrType = 1 // invalid address type
+				ret := toMsg(t, spkt)
+				return ret
+			},
+			srcInterface: 1,
+			expectedSlowPathRequest: slowPathRequest{
+				typ:      slowPathSCMP,
+				scmpType: slayers.SCMPTypeParameterProblem,
+				code:     slayers.SCMPCodeInvalidDestinationAddress,
+				cause:    invalidDstAddr,
+			},
+			expectedLayerType: slayers.LayerTypeSCMPParameterProblem,
+		},
 	}
 	for name, tc := range testCases {
 		name, tc := name, tc