From a286368144cb8765d0a43ffcf5dfe6d83e8da2d5 Mon Sep 17 00:00:00 2001 From: jiceatscion <139873336+jiceatscion@users.noreply.github.com> Date: Mon, 18 Mar 2024 13:43:51 +0100 Subject: [PATCH 1/3] cherry-pick 4483 --- pkg/slayers/path/scion/BUILD.bazel | 1 + pkg/slayers/path/scion/base.go | 25 ++++++++++++--- pkg/slayers/path/scion/decoded.go | 7 ----- pkg/slayers/path/scion/raw_test.go | 25 ++++++++++++++- router/dataplane_test.go | 50 ++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 12 deletions(-) diff --git a/pkg/slayers/path/scion/BUILD.bazel b/pkg/slayers/path/scion/BUILD.bazel index a4a57739f..c05261537 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/base.go b/pkg/slayers/path/scion/base.go index f8960fd72..f62c71e1e 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -27,10 +27,17 @@ import ( //@ sl "github.com/scionproto/scion/verification/utils/slices" ) -// 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 +) // @ requires path.PathPackageMem() // @ requires !path.Registered(PathType) @@ -142,6 +149,14 @@ func (s *Base) DecodeFromBytes(data []byte) (r error) { //@ assume int(s.PathMeta.SegLen[i]) >= 0 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 { + //@ fold s.NonInitMem() + return serrors.New("NumHops too large", "NumHops", s.NumHops, "Maximum", MaxHops) + } //@ fold s.Mem() return nil } @@ -212,7 +227,9 @@ func (s *Base) infIndexForHF(hf uint8) (r 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. // @ pure // @ requires acc(s.Mem(), _) // @ ensures r >= MetaLen diff --git a/pkg/slayers/path/scion/decoded.go b/pkg/slayers/path/scion/decoded.go index 6af13f001..3e1963b21 100644 --- a/pkg/slayers/path/scion/decoded.go +++ b/pkg/slayers/path/scion/decoded.go @@ -24,13 +24,6 @@ import ( //@ sl "github.com/scionproto/scion/verification/utils/slices" ) -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. diff --git a/pkg/slayers/path/scion/raw_test.go b/pkg/slayers/path/scion/raw_test.go index ff527d939..3a869b016 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" ) @@ -51,6 +52,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 +77,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 +85,15 @@ 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{} + 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) { for name, tc := range pathReverseCases { name, tc := name, tc diff --git a/router/dataplane_test.go b/router/dataplane_test.go index 4ae857cb9..ef9f8b8ab 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -33,6 +33,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" @@ -600,6 +601,55 @@ func TestProcessPkt(t *testing.T) { srcInterface: 1, 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.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 + + 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: 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) + return assert.Equal(t, expected.Error(), err.Error()) + }, + }, "outbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { return router.NewDP( From 71958e6605443bb1a02e9fcfb266ec0003f95f2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pereira?= Date: Tue, 2 Apr 2024 13:07:28 +0200 Subject: [PATCH 2/3] undo change to test due to the use of (yet) undefined symbols --- router/dataplane_test.go | 50 ---------------------------------------- 1 file changed, 50 deletions(-) diff --git a/router/dataplane_test.go b/router/dataplane_test.go index ef9f8b8ab..4ae857cb9 100644 --- a/router/dataplane_test.go +++ b/router/dataplane_test.go @@ -33,7 +33,6 @@ 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" @@ -601,55 +600,6 @@ func TestProcessPkt(t *testing.T) { srcInterface: 1, 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.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 - - 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: 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) - return assert.Equal(t, expected.Error(), err.Error()) - }, - }, "outbound": { prepareDP: func(ctrl *gomock.Controller) *router.DataPlane { return router.NewDP( From 4e911412302e89dd375a3974214530c615b1ba8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Pereira?= Date: Tue, 2 Apr 2024 13:22:25 +0200 Subject: [PATCH 3/3] fix verification error --- pkg/slayers/path/scion/base.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/slayers/path/scion/base.go b/pkg/slayers/path/scion/base.go index f62c71e1e..a41185eab 100644 --- a/pkg/slayers/path/scion/base.go +++ b/pkg/slayers/path/scion/base.go @@ -154,7 +154,7 @@ func (s *Base) DecodeFromBytes(data []byte) (r error) { // 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 { - //@ fold s.NonInitMem() + //@ defer fold s.NonInitMem() return serrors.New("NumHops too large", "NumHops", s.NumHops, "Maximum", MaxHops) } //@ fold s.Mem()