Artifact tag matcher: Curb complexity when parsing regexps from user …

…input (#3836) * Mitigate ReDos in artifact tag matcher This commit adds a limit to the lengrh of regular expressions we take from user input to set a length limit and curve the complexity Minder handles when compiling them. Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]> * Add test for artifact.BuildFilter Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]> --------- Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
mindersec · Jul 10, 2024 · e77e142 · e77e142
1 parent 87416ec
commit e77e142
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 0 deletions.
diff --git a/internal/providers/artifact/versionsfilter.go b/internal/providers/artifact/versionsfilter.go
@@ -47,6 +47,9 @@ func BuildFilter(tags []string, tagRegex string) (*filter, error) {
 
 	// no tags specified, but a regex was, compile it
 	if tagRegex != "" {
+		if len(tagRegex) > 512 {
+			return nil, fmt.Errorf("tag regular expressions are limited to 512 characters")
+		}
 		re, err := regexp.Compile(tagRegex)
 		if err != nil {
 			return nil, fmt.Errorf("error compiling tag regex: %w", err)

diff --git a/internal/providers/artifact/versionsfilter_test.go b/internal/providers/artifact/versionsfilter_test.go
@@ -16,14 +16,89 @@
 package artifact
 
 import (
+	"regexp"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	provifv1 "github.com/stacklok/minder/pkg/providers/v1"
 )
 
+func TestBuildFilter(t *testing.T) {
+	t.Parallel()
+	simpleRegex := `^simpleregex$`
+	compiledSimpleRegex := regexp.MustCompile(simpleRegex)
+	for _, tc := range []struct {
+		name     string
+		tags     []string
+		regex    string
+		expected *filter
+		mustErr  bool
+	}{
+		{
+			name:    "tags",
+			tags:    []string{"hello", "bye"},
+			mustErr: false,
+			expected: &filter{
+				tagMatcher:      &tagListMatcher{tags: []string{"hello", "bye"}},
+				retentionPeriod: time.Time{},
+			},
+		},
+		{
+			name:    "empty-tag",
+			tags:    []string{"hello", ""},
+			mustErr: true,
+		},
+		{
+			name:    "regex",
+			tags:    []string{},
+			regex:   simpleRegex,
+			mustErr: false,
+			expected: &filter{
+				tagMatcher: &tagRegexMatcher{
+					re: compiledSimpleRegex,
+				},
+			},
+		},
+		{
+			name:    "invalidregexp",
+			tags:    []string{},
+			regex:   `$(invalid^`,
+			mustErr: true,
+		},
+		{
+			name:    "valid-long-regexp",
+			tags:    []string{},
+			regex:   `^` + strings.Repeat("A", 1000) + `$`,
+			mustErr: true,
+		},
+		{
+			name:    "no-tags",
+			tags:    []string{},
+			mustErr: false,
+			expected: &filter{
+				tagMatcher: &tagAllMatcher{},
+			},
+		},
+	} {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			f, err := BuildFilter(tc.tags, tc.regex)
+			if tc.mustErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			require.NotNil(t, f.tagMatcher)
+			require.Equal(t, tc.expected.tagMatcher, f.tagMatcher)
+		})
+	}
+}
+
 func Test_filter_IsSkippable(t *testing.T) {
 	t.Parallel()