From 6e82be5c4a6019d242928dd6ee25aa2b27e81ad7 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Tue, 12 Mar 2024 14:22:02 +0100 Subject: [PATCH 1/6] paths: Add check for hopfield count < 64 when deserializing a scion path header. Added unit test for deserialization and unit test for router ingress. --- pkg/slayers/path/scion/base.go | 11 ++++++++- pkg/slayers/path/scion/raw_test.go | 20 ++++++++++++++++ router/dataplane_test.go | 37 ++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/pkg/slayers/path/scion/base.go b/pkg/slayers/path/scion/base.go index b2a7812239..926da0cb03 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -66,6 +66,13 @@ func (s *Base) DecodeFromBytes(data []byte) error { } s.NumHops += int(s.PathMeta.SegLen[i]) } + // We must check the validity of NumHops. It is possible to fit more than 64 hops in + // the length of a scion header. Yet a path of more than 64 hops cannot be followed to + // the end because CurrHF is only 6 bits long. + if s.NumHops > 64 { + return serrors.New( + fmt.Sprintf("NumHops > 64")) + } return nil } @@ -113,7 +120,9 @@ func (s *Base) infIndexForHF(hf uint8) uint8 { } } -// Len returns the length of the path in bytes. +// Len returns the length of the path in bytes. That is, the number of byte required to +// store it, based on the metadata. The actual number of bytes available to contain it +// can be inferred from the common header field HdrLen. It may or may not be consistent. func (s *Base) Len() int { return MetaLen + s.NumINF*path.InfoLen + s.NumHops*path.HopLen } diff --git a/pkg/slayers/path/scion/raw_test.go b/pkg/slayers/path/scion/raw_test.go index ff527d9398..d2e4560d7c 100644 --- a/pkg/slayers/path/scion/raw_test.go +++ b/pkg/slayers/path/scion/raw_test.go @@ -51,6 +51,19 @@ var emptyRawTestPath = &scion.Raw{ Raw: make([]byte, scion.MetaLen), } +var overlongPath = &scion.Raw{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrINF: 0, + CurrHF: 0, + SegLen: [3]uint8{24, 24, 17}, + }, + NumINF: 3, + NumHops: 65, + }, + Raw: rawPath, +} + func TestRawSerialize(t *testing.T) { b := make([]byte, rawTestPath.Len()) assert.NoError(t, rawTestPath.SerializeTo(b)) @@ -71,6 +84,13 @@ func TestRawSerliazeDecode(t *testing.T) { assert.Equal(t, rawTestPath, s) } +func TestOverlongSerliazeDecode(t *testing.T) { + b := make([]byte, overlongPath.Len()) + assert.NoError(t, overlongPath.SerializeTo(b)) // permitted, if only to enable this test. + s := &scion.Raw{} + assert.Error(t, s.DecodeFromBytes(b)) // invalid raw packet. +} + func TestRawReverse(t *testing.T) { for name, tc := range pathReverseCases { name, tc := name, tc diff --git a/router/dataplane_test.go b/router/dataplane_test.go index baf163fb53..bf26ce99fd 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -620,6 +620,43 @@ func TestProcessPkt(t *testing.T) { egressInterface: 0, assertFunc: assert.NoError, }, + "inbound_longpath": { + prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { + return router.NewDP(fakeExternalInterfaces, + nil, mock_router.NewMockBatchConn(ctrl), + fakeInternalNextHops, nil, + xtest.MustParseIA("1-ff00:0:110"), nil, key) + }, + mockMsg: func(afterProcessing bool) *ipv4.Message { + spkt, dpath := prepBaseMsg(now) + spkt.DstIA = xtest.MustParseIA("1-ff00:0:110") + dst := addr.MustParseHost("10.0.100.100") + _ = spkt.SetDstAddr(dst) + dpath.HopFields = []path.HopField{ + {ConsIngress: 41, ConsEgress: 40}, + {ConsIngress: 31, ConsEgress: 30}, + {ConsIngress: 1, ConsEgress: 0}, + } + + // Everything is the same a in the inbound test, except that we tossed in + // 64 extra hops and two extra segments. + dpath.Base.PathMeta.CurrHF = 2 + dpath.Base.PathMeta.SegLen = [3]uint8{24, 24, 17} + dpath.Base.NumINF = 3 + dpath.Base.NumHops = 65 + + dpath.HopFields[2].Mac = computeMAC(t, key, dpath.InfoFields[0], dpath.HopFields[2]) + ret := toMsg(t, spkt, dpath) + if afterProcessing { + ret.Addr = &net.UDPAddr{IP: dst.IP().AsSlice(), Port: topology.EndhostPort} + ret.Flags, ret.NN, ret.N, ret.OOB = 0, 0, 0, nil + } + return ret + }, + srcInterface: 1, + egressInterface: 0, + assertFunc: assert.Error, + }, "outbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { return router.NewDP( From 45c757ea2f4c965c9d499bad59c9842cc6ab056b Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Tue, 12 Mar 2024 14:58:54 +0100 Subject: [PATCH 2/6] paths: Moved check for NumHops higher up the decoding stack. No need to check before we know the stuff is going to be used. --- pkg/slayers/path/scion/base.go | 7 ------- pkg/slayers/path/scion/decoded.go | 7 +++++++ pkg/slayers/path/scion/decoded_test.go | 19 +++++++++++++++++++ pkg/slayers/path/scion/raw_test.go | 20 -------------------- 4 files changed, 26 insertions(+), 27 deletions(-) diff --git a/pkg/slayers/path/scion/base.go b/pkg/slayers/path/scion/base.go index 926da0cb03..f531ef18c0 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -66,13 +66,6 @@ func (s *Base) DecodeFromBytes(data []byte) error { } s.NumHops += int(s.PathMeta.SegLen[i]) } - // We must check the validity of NumHops. It is possible to fit more than 64 hops in - // the length of a scion header. Yet a path of more than 64 hops cannot be followed to - // the end because CurrHF is only 6 bits long. - if s.NumHops > 64 { - return serrors.New( - fmt.Sprintf("NumHops > 64")) - } return nil } diff --git a/pkg/slayers/path/scion/decoded.go b/pkg/slayers/path/scion/decoded.go index da183f2903..b88db84199 100644 --- a/pkg/slayers/path/scion/decoded.go +++ b/pkg/slayers/path/scion/decoded.go @@ -46,6 +46,13 @@ func (s *Decoded) DecodeFromBytes(data []byte) error { return serrors.New("DecodedPath raw too short", "expected", minLen, "actual", len(data)) } + // We must check the validity of NumHops. It is possible to fit more than 64 hops in + // the length of a scion header. Yet a path of more than 64 hops cannot be followed to + // the end because CurrHF is only 6 bits long. + if s.NumHops > 64 { + return serrors.New("NumHops > 64") + } + offset := MetaLen s.InfoFields = make([]path.InfoField, s.NumINF) for i := 0; i < s.NumINF; i++ { diff --git a/pkg/slayers/path/scion/decoded_test.go b/pkg/slayers/path/scion/decoded_test.go index 096f26972e..af6b01dc3f 100644 --- a/pkg/slayers/path/scion/decoded_test.go +++ b/pkg/slayers/path/scion/decoded_test.go @@ -87,6 +87,18 @@ var emptyDecodedTestPath = &scion.Decoded{ HopFields: []path.HopField{}, } +var overlongPath = &scion.Decoded{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrINF: 0, + CurrHF: 0, + SegLen: [3]uint8{24, 24, 17}, + }, + NumINF: 3, + NumHops: 65, + }, +} + var rawPath = []byte("\x00\x00\x20\x80\x00\x00\x01\x11\x00\x00\x01\x00\x01\x00\x02\x22\x00\x00" + "\x01\x00\x00\x3f\x00\x01\x00\x00\x01\x02\x03\x04\x05\x06\x00\x3f\x00\x03\x00\x02\x01\x02\x03" + "\x04\x05\x06\x00\x3f\x00\x00\x00\x02\x01\x02\x03\x04\x05\x06\x00\x3f\x00\x01\x00\x00\x01\x02" + @@ -167,6 +179,13 @@ func TestDecodedSerializeDecode(t *testing.T) { assert.Equal(t, decodedTestPath, s) } +func TestOverlongSerliazeDecode(t *testing.T) { + b := make([]byte, overlongPath.Len()) + assert.NoError(t, overlongPath.SerializeTo(b)) // permitted, if only to enable this test. + s := &scion.Decoded{} + assert.Error(t, s.DecodeFromBytes(b)) // invalid raw packet. +} + func TestDecodedReverse(t *testing.T) { for name, tc := range pathReverseCases { name, tc := name, tc diff --git a/pkg/slayers/path/scion/raw_test.go b/pkg/slayers/path/scion/raw_test.go index d2e4560d7c..ff527d9398 100644 --- a/pkg/slayers/path/scion/raw_test.go +++ b/pkg/slayers/path/scion/raw_test.go @@ -51,19 +51,6 @@ var emptyRawTestPath = &scion.Raw{ Raw: make([]byte, scion.MetaLen), } -var overlongPath = &scion.Raw{ - Base: scion.Base{ - PathMeta: scion.MetaHdr{ - CurrINF: 0, - CurrHF: 0, - SegLen: [3]uint8{24, 24, 17}, - }, - NumINF: 3, - NumHops: 65, - }, - Raw: rawPath, -} - func TestRawSerialize(t *testing.T) { b := make([]byte, rawTestPath.Len()) assert.NoError(t, rawTestPath.SerializeTo(b)) @@ -84,13 +71,6 @@ func TestRawSerliazeDecode(t *testing.T) { assert.Equal(t, rawTestPath, s) } -func TestOverlongSerliazeDecode(t *testing.T) { - b := make([]byte, overlongPath.Len()) - assert.NoError(t, overlongPath.SerializeTo(b)) // permitted, if only to enable this test. - s := &scion.Raw{} - assert.Error(t, s.DecodeFromBytes(b)) // invalid raw packet. -} - func TestRawReverse(t *testing.T) { for name, tc := range pathReverseCases { name, tc := name, tc From 861721c3367c61bd39e992c946e2fd8d448e43b4 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Tue, 12 Mar 2024 15:19:35 +0100 Subject: [PATCH 3/6] paths: use the official constant for the numhops check, rather than a litteral 64. --- pkg/slayers/path/scion/decoded.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/slayers/path/scion/decoded.go b/pkg/slayers/path/scion/decoded.go index b88db84199..f3e1d5eafb 100644 --- a/pkg/slayers/path/scion/decoded.go +++ b/pkg/slayers/path/scion/decoded.go @@ -49,8 +49,8 @@ func (s *Decoded) DecodeFromBytes(data []byte) error { // We must check the validity of NumHops. It is possible to fit more than 64 hops in // the length of a scion header. Yet a path of more than 64 hops cannot be followed to // the end because CurrHF is only 6 bits long. - if s.NumHops > 64 { - return serrors.New("NumHops > 64") + if s.NumHops > MaxHops { + return serrors.New("NumHops too large", "NumHops", s.NumHops, "Maximum", MaxHops) } offset := MetaLen From 3eb8273edb9fa56df1546173be7d2cc71695b6cc Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Thu, 14 Mar 2024 16:52:19 +0100 Subject: [PATCH 4/6] moved the HF count check back into base. Improved router test. --- pkg/slayers/path/scion/base.go | 21 ++++++++++++++++++--- pkg/slayers/path/scion/decoded.go | 14 -------------- pkg/slayers/path/scion/decoded_test.go | 19 ------------------- pkg/slayers/path/scion/raw_test.go | 22 +++++++++++++++++++++- router/dataplane_test.go | 18 +++++++++++++++++- 5 files changed, 56 insertions(+), 38 deletions(-) diff --git a/pkg/slayers/path/scion/base.go b/pkg/slayers/path/scion/base.go index f531ef18c0..6853fd49c6 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -22,10 +22,17 @@ import ( "github.com/scionproto/scion/pkg/slayers/path" ) -// MetaLen is the length of the PathMetaHeader. -const MetaLen = 4 +const ( + // MaxINFs is the maximum number of info fields in a SCION path. + MaxINFs = 3 + // MaxHops is the maximum number of hop fields in a SCION path. + MaxHops = 64 -const PathType path.Type = 1 + // MetaLen is the length of the PathMetaHeader. + MetaLen = 4 + + PathType path.Type = 1 +) func RegisterPath() { path.RegisterPath(path.Metadata{ @@ -66,6 +73,14 @@ func (s *Base) DecodeFromBytes(data []byte) error { } s.NumHops += int(s.PathMeta.SegLen[i]) } + + // We must check the validity of NumHops. It is possible to fit more than 64 hops in + // the length of a scion header. Yet a path of more than 64 hops cannot be followed to + // the end because CurrHF is only 6 bits long. + if s.NumHops > MaxHops { + return serrors.New("NumHops too large", "NumHops", s.NumHops, "Maximum", MaxHops) + } + return nil } diff --git a/pkg/slayers/path/scion/decoded.go b/pkg/slayers/path/scion/decoded.go index f3e1d5eafb..c3ce572aa7 100644 --- a/pkg/slayers/path/scion/decoded.go +++ b/pkg/slayers/path/scion/decoded.go @@ -19,13 +19,6 @@ import ( "github.com/scionproto/scion/pkg/slayers/path" ) -const ( - // MaxINFs is the maximum number of info fields in a SCION path. - MaxINFs = 3 - // MaxHops is the maximum number of hop fields in a SCION path. - MaxHops = 64 -) - // Decoded implements the SCION (data-plane) path type. Decoded is intended to be used in // non-performance critical code paths, where the convenience of having a fully parsed path trumps // the loss of performance. @@ -46,13 +39,6 @@ func (s *Decoded) DecodeFromBytes(data []byte) error { return serrors.New("DecodedPath raw too short", "expected", minLen, "actual", len(data)) } - // We must check the validity of NumHops. It is possible to fit more than 64 hops in - // the length of a scion header. Yet a path of more than 64 hops cannot be followed to - // the end because CurrHF is only 6 bits long. - if s.NumHops > MaxHops { - return serrors.New("NumHops too large", "NumHops", s.NumHops, "Maximum", MaxHops) - } - offset := MetaLen s.InfoFields = make([]path.InfoField, s.NumINF) for i := 0; i < s.NumINF; i++ { diff --git a/pkg/slayers/path/scion/decoded_test.go b/pkg/slayers/path/scion/decoded_test.go index af6b01dc3f..096f26972e 100644 --- a/pkg/slayers/path/scion/decoded_test.go +++ b/pkg/slayers/path/scion/decoded_test.go @@ -87,18 +87,6 @@ var emptyDecodedTestPath = &scion.Decoded{ HopFields: []path.HopField{}, } -var overlongPath = &scion.Decoded{ - Base: scion.Base{ - PathMeta: scion.MetaHdr{ - CurrINF: 0, - CurrHF: 0, - SegLen: [3]uint8{24, 24, 17}, - }, - NumINF: 3, - NumHops: 65, - }, -} - var rawPath = []byte("\x00\x00\x20\x80\x00\x00\x01\x11\x00\x00\x01\x00\x01\x00\x02\x22\x00\x00" + "\x01\x00\x00\x3f\x00\x01\x00\x00\x01\x02\x03\x04\x05\x06\x00\x3f\x00\x03\x00\x02\x01\x02\x03" + "\x04\x05\x06\x00\x3f\x00\x00\x00\x02\x01\x02\x03\x04\x05\x06\x00\x3f\x00\x01\x00\x00\x01\x02" + @@ -179,13 +167,6 @@ func TestDecodedSerializeDecode(t *testing.T) { assert.Equal(t, decodedTestPath, s) } -func TestOverlongSerliazeDecode(t *testing.T) { - b := make([]byte, overlongPath.Len()) - assert.NoError(t, overlongPath.SerializeTo(b)) // permitted, if only to enable this test. - s := &scion.Decoded{} - assert.Error(t, s.DecodeFromBytes(b)) // invalid raw packet. -} - func TestDecodedReverse(t *testing.T) { for name, tc := range pathReverseCases { name, tc := name, tc diff --git a/pkg/slayers/path/scion/raw_test.go b/pkg/slayers/path/scion/raw_test.go index ff527d9398..0ad4995972 100644 --- a/pkg/slayers/path/scion/raw_test.go +++ b/pkg/slayers/path/scion/raw_test.go @@ -51,6 +51,19 @@ var emptyRawTestPath = &scion.Raw{ Raw: make([]byte, scion.MetaLen), } +var overlongPath = &scion.Raw{ + Base: scion.Base{ + PathMeta: scion.MetaHdr{ + CurrINF: 0, + CurrHF: 0, + SegLen: [3]uint8{24, 24, 17}, + }, + NumINF: 3, + NumHops: 65, + }, + Raw: rawPath, +} + func TestRawSerialize(t *testing.T) { b := make([]byte, rawTestPath.Len()) assert.NoError(t, rawTestPath.SerializeTo(b)) @@ -63,7 +76,7 @@ func TestRawDecodeFromBytes(t *testing.T) { assert.Equal(t, rawTestPath, s) } -func TestRawSerliazeDecode(t *testing.T) { +func TestRawSerializeDecode(t *testing.T) { b := make([]byte, rawTestPath.Len()) assert.NoError(t, rawTestPath.SerializeTo(b)) s := &scion.Raw{} @@ -71,6 +84,13 @@ func TestRawSerliazeDecode(t *testing.T) { assert.Equal(t, rawTestPath, s) } +func TestOverlongSerializeDecode(t *testing.T) { + b := make([]byte, overlongPath.Len()) + assert.NoError(t, overlongPath.SerializeTo(b)) // permitted, if only to enable this test. + s := &scion.Raw{} + assert.Error(t, s.DecodeFromBytes(b)) // invalid raw packet. +} + func TestRawReverse(t *testing.T) { for name, tc := range pathReverseCases { name, tc := name, tc diff --git a/router/dataplane_test.go b/router/dataplane_test.go index bf26ce99fd..0623cb56e5 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -34,6 +34,7 @@ import ( "github.com/scionproto/scion/pkg/addr" libepic "github.com/scionproto/scion/pkg/experimental/epic" + "github.com/scionproto/scion/pkg/private/serrors" "github.com/scionproto/scion/pkg/private/util" "github.com/scionproto/scion/pkg/private/xtest" "github.com/scionproto/scion/pkg/scrypto" @@ -642,6 +643,11 @@ func TestProcessPkt(t *testing.T) { // 64 extra hops and two extra segments. dpath.Base.PathMeta.CurrHF = 2 dpath.Base.PathMeta.SegLen = [3]uint8{24, 24, 17} + dpath.InfoFields = append( + dpath.InfoFields, + path.InfoField{SegID: 0x112, ConsDir: true, Timestamp: util.TimeToSecs(now)}, + path.InfoField{SegID: 0x113, ConsDir: true, Timestamp: util.TimeToSecs(now)}, + ) dpath.Base.NumINF = 3 dpath.Base.NumHops = 65 @@ -655,7 +661,17 @@ func TestProcessPkt(t *testing.T) { }, srcInterface: 1, egressInterface: 0, - assertFunc: assert.Error, + assertFunc: func(t assert.TestingT, err error, _ ...interface{}) bool { + if !assert.Error(t, err) { + return false + } + expected := serrors.New("NumHops too large", + "NumHops", 65, "Maximum", scion.MaxHops).Error() + if !assert.Equal(t, expected, err.Error()) { + return false + } + return true + }, }, "outbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { From 2c63d759221269b15c871bfa0b71a9c87994434f Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Fri, 15 Mar 2024 17:20:52 +0100 Subject: [PATCH 5/6] lintified --- router/dataplane_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/router/dataplane_test.go b/router/dataplane_test.go index 0623cb56e5..970c471764 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -667,10 +667,7 @@ func TestProcessPkt(t *testing.T) { } expected := serrors.New("NumHops too large", "NumHops", 65, "Maximum", scion.MaxHops).Error() - if !assert.Equal(t, expected, err.Error()) { - return false - } - return true + return assert.Equal(t, expected, err.Error()) }, }, "outbound": { From b2cfdbaf8b7dbf88a96ed5d657d5e211c74211d8 Mon Sep 17 00:00:00 2001 From: Jean-Christophe Hugly Date: Mon, 18 Mar 2024 12:44:43 +0100 Subject: [PATCH 6/6] Better check of the error in raw_test.go Also make the similar check in dataplane_test.go in the same style. --- pkg/slayers/path/scion/BUILD.bazel | 1 + pkg/slayers/path/scion/raw_test.go | 5 ++++- router/dataplane_test.go | 4 ++-- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/pkg/slayers/path/scion/BUILD.bazel b/pkg/slayers/path/scion/BUILD.bazel index a4a57739fa..c05261537c 100644 --- a/pkg/slayers/path/scion/BUILD.bazel +++ b/pkg/slayers/path/scion/BUILD.bazel @@ -24,6 +24,7 @@ go_test( ], deps = [ ":go_default_library", + "//pkg/private/serrors:go_default_library", "//pkg/slayers/path:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@com_github_stretchr_testify//require:go_default_library", diff --git a/pkg/slayers/path/scion/raw_test.go b/pkg/slayers/path/scion/raw_test.go index 0ad4995972..3a869b0164 100644 --- a/pkg/slayers/path/scion/raw_test.go +++ b/pkg/slayers/path/scion/raw_test.go @@ -21,6 +21,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/scionproto/scion/pkg/private/serrors" "github.com/scionproto/scion/pkg/slayers/path" "github.com/scionproto/scion/pkg/slayers/path/scion" ) @@ -88,7 +89,9 @@ func TestOverlongSerializeDecode(t *testing.T) { b := make([]byte, overlongPath.Len()) assert.NoError(t, overlongPath.SerializeTo(b)) // permitted, if only to enable this test. s := &scion.Raw{} - assert.Error(t, s.DecodeFromBytes(b)) // invalid raw packet. + expected := serrors.New("NumHops too large", "NumHops", 65, "Maximum", scion.MaxHops) + err := s.DecodeFromBytes(b) + assert.Equal(t, expected.Error(), err.Error()) } func TestRawReverse(t *testing.T) { diff --git a/router/dataplane_test.go b/router/dataplane_test.go index 970c471764..d0879885ba 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -666,8 +666,8 @@ func TestProcessPkt(t *testing.T) { return false } expected := serrors.New("NumHops too large", - "NumHops", 65, "Maximum", scion.MaxHops).Error() - return assert.Equal(t, expected, err.Error()) + "NumHops", 65, "Maximum", scion.MaxHops) + return assert.Equal(t, expected.Error(), err.Error()) }, }, "outbound": {