From 5b133c0abecbbe77f2b50cc77587c134e108d17a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Florian=20B=C3=BCtler?=
 <48495685+fbuetler@users.noreply.github.com>
Date: Fri, 19 Apr 2024 09:02:56 +0200
Subject: [PATCH] pathpol: ensure deserialized ACL has default rule (#4505)

An ACL without default rule may panic during evaluation of the filter,
which is why the constructor NewACL checks that such a default rule
exists.

Add the same check also to the deserialization from JSON/YAML.

Fixes #4504
---
 private/path/pathpol/BUILD.bazel    |  1 +
 private/path/pathpol/acl.go         | 40 +++++++++++++++++++++++---
 private/path/pathpol/acl_test.go    | 44 +++++++++++++++++++++++++++++
 private/path/pathpol/policy_test.go | 13 +++++----
 4 files changed, 88 insertions(+), 10 deletions(-)

diff --git a/private/path/pathpol/BUILD.bazel b/private/path/pathpol/BUILD.bazel
index df95add87f..b5c7b1d06d 100644
--- a/private/path/pathpol/BUILD.bazel
+++ b/private/path/pathpol/BUILD.bazel
@@ -45,5 +45,6 @@ go_test(
         "@com_github_golang_mock//gomock:go_default_library",
         "@com_github_stretchr_testify//assert:go_default_library",
         "@com_github_stretchr_testify//require:go_default_library",
+        "@in_gopkg_yaml_v2//:go_default_library",
     ],
 )
diff --git a/private/path/pathpol/acl.go b/private/path/pathpol/acl.go
index 11efbe74ac..bf7d846948 100644
--- a/private/path/pathpol/acl.go
+++ b/private/path/pathpol/acl.go
@@ -27,6 +27,8 @@ import (
 var (
 	// ErrNoDefault indicates that there is no default acl entry.
 	ErrNoDefault = errors.New("ACL does not have a default")
+	// ErrExtraEntries indicates that there extra entries after the default entry.
+	ErrExtraEntries = errors.New("ACL has unused extra entries after a default entry")
 )
 
 type ACL struct {
@@ -35,8 +37,8 @@ type ACL struct {
 
 // NewACL creates a new entry and checks for the presence of a default action
 func NewACL(entries ...*ACLEntry) (*ACL, error) {
-	if len(entries) == 0 || !entries[len(entries)-1].Rule.matchesAll() {
-		return nil, ErrNoDefault
+	if err := validateACL(entries); err != nil {
+		return nil, err
 	}
 	return &ACL{Entries: entries}, nil
 }
@@ -61,7 +63,10 @@ func (a *ACL) MarshalJSON() ([]byte, error) {
 }
 
 func (a *ACL) UnmarshalJSON(b []byte) error {
-	return json.Unmarshal(b, &a.Entries)
+	if err := json.Unmarshal(b, &a.Entries); err != nil {
+		return err
+	}
+	return validateACL(a.Entries)
 }
 
 func (a *ACL) MarshalYAML() (interface{}, error) {
@@ -69,7 +74,10 @@ func (a *ACL) MarshalYAML() (interface{}, error) {
 }
 
 func (a *ACL) UnmarshalYAML(unmarshal func(interface{}) error) error {
-	return unmarshal(&a.Entries)
+	if err := unmarshal(&a.Entries); err != nil {
+		return err
+	}
+	return validateACL(a.Entries)
 }
 
 func (a *ACL) evalPath(pm *snet.PathMetadata) ACLAction {
@@ -90,6 +98,30 @@ func (a *ACL) evalInterface(iface snet.PathInterface, ingress bool) ACLAction {
 	panic("Default ACL action missing")
 }
 
+func validateACL(entries []*ACLEntry) error {
+	if len(entries) == 0 {
+		return ErrNoDefault
+	}
+
+	foundAt := -1
+	for i, e := range entries {
+		if e.Rule.matchesAll() {
+			foundAt = i
+			break
+		}
+	}
+
+	if foundAt < 0 {
+		return ErrNoDefault
+	}
+
+	if foundAt != len(entries)-1 {
+		return ErrExtraEntries
+	}
+
+	return nil
+}
+
 type ACLEntry struct {
 	Action ACLAction
 	Rule   *HopPredicate
diff --git a/private/path/pathpol/acl_test.go b/private/path/pathpol/acl_test.go
index 080c145336..7842afe79e 100644
--- a/private/path/pathpol/acl_test.go
+++ b/private/path/pathpol/acl_test.go
@@ -15,10 +15,12 @@
 package pathpol
 
 import (
+	"encoding/json"
 	"testing"
 
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
+	"gopkg.in/yaml.v2"
 
 	"github.com/scionproto/scion/pkg/addr"
 	"github.com/scionproto/scion/pkg/private/common"
@@ -60,6 +62,48 @@ func TestNewACL(t *testing.T) {
 	}
 }
 
+func TestUnmarshal(t *testing.T) {
+	tests := map[string]struct {
+		Input       string
+		ExpectedErr error
+	}{
+		"No entry": {
+			Input:       `[]`,
+			ExpectedErr: ErrNoDefault,
+		},
+		"No default entry": {
+			Input:       `["+ 42"]`,
+			ExpectedErr: ErrNoDefault,
+		},
+		"Entry without rule": {
+			Input: `["+"]`,
+		},
+		"Entry with hop predicates": {
+			Input: `["+ 42", "-"]`,
+		},
+		"Extra entries (first)": {
+			Input:       `["-", "+ 27"]`,
+			ExpectedErr: ErrExtraEntries,
+		},
+		"Extra entries (in the middle)": {
+			Input:       `["+ 42", "-", "+ 27", "- 30"]`,
+			ExpectedErr: ErrExtraEntries,
+		},
+	}
+	for name, test := range tests {
+		t.Run("json: "+name, func(t *testing.T) {
+			var acl ACL
+			err := json.Unmarshal([]byte(test.Input), &acl)
+			assert.ErrorIs(t, err, test.ExpectedErr)
+		})
+		t.Run("yaml: "+name, func(t *testing.T) {
+			var acl ACL
+			err := yaml.Unmarshal([]byte(test.Input), &acl)
+			assert.ErrorIs(t, err, test.ExpectedErr)
+		})
+	}
+}
+
 func TestACLEntryLoadFromString(t *testing.T) {
 	tests := map[string]struct {
 		String         string
diff --git a/private/path/pathpol/policy_test.go b/private/path/pathpol/policy_test.go
index 9f32d887a1..76d336acd2 100644
--- a/private/path/pathpol/policy_test.go
+++ b/private/path/pathpol/policy_test.go
@@ -586,16 +586,17 @@ func TestFilterOpt(t *testing.T) {
 }
 
 func TestPolicyJsonConversion(t *testing.T) {
+	acl, err := NewACL(
+		&ACLEntry{Action: Allow, Rule: mustHopPredicate(t, "42-0#0")},
+		denyEntry,
+	)
+	require.NoError(t, err)
+
 	policy := NewPolicy("", nil, nil, []Option{
 		{
 			Policy: &ExtPolicy{
 				Policy: &Policy{
-					ACL: &ACL{
-						Entries: []*ACLEntry{
-							{Action: Allow, Rule: mustHopPredicate(t, "0-0#0")},
-							denyEntry,
-						},
-					},
+					ACL: acl,
 					LocalISDAS: &LocalISDAS{
 						AllowedIAs: []addr.IA{
 							xtest.MustParseIA("64-123"),