Refactor regex pattern validation to use timeout-based approach (#2090)

- Remove isPathologicalRegex function and replace with MatchTimeout
- Simplify parseFilter by relying on runtime timeout protection
- Add comprehensive timeout test for pathological regex patterns
- Set 5-second timeout for all compiled regex operations

Author: KristianGrafana

pkg/zabbix/utils.go

@@ -66,51 +66,6 @@ func splitKeyParams(paramStr string) []string {
 	return params
 }
 
-// isPathologicalRegex detects potentially dangerous regex patterns that could cause ReDoS
-func isPathologicalRegex(pattern string) bool {
-	// Check for consecutive quantifiers
-	consecutiveQuantifiers := []string{`\*\*`, `\+\+`, `\*\+`, `\+\*`}
-	for _, q := range consecutiveQuantifiers {
-		if matched, _ := regexp.MatchString(q, pattern); matched {
-			return true
-		}
-	}
-	
-	// Check for nested quantifiers
-	nestedQuantifiers := []string{
-		`\([^)]*\+[^)]*\)\+`,     // (a+)+
-		`\([^)]*\*[^)]*\)\*`,     // (a*)*
-		`\([^)]*\+[^)]*\)\*`,     // (a+)*
-		`\([^)]*\*[^)]*\)\+`,     // (a*)+
-	}
-	for _, nested := range nestedQuantifiers {
-		if matched, _ := regexp.MatchString(nested, pattern); matched {
-			return true
-		}
-	}
-	
-	// Check for specific catastrophic patterns
-	catastrophicPatterns := []string{
-		`\(\.\*\)\*`,             // (.*)* 
-		`\(\.\+\)\+`,             // (.+)+
-		`\(\.\*\)\+`,             // (.*)+
-		`\(\.\+\)\*`,             // (.+)*
-	}
-	for _, catastrophic := range catastrophicPatterns {
-		if matched, _ := regexp.MatchString(catastrophic, pattern); matched {
-			return true
-		}
-	}
-	
-	// Check for obvious overlapping alternation (manual check for exact duplicates)
-	if strings.Contains(pattern, "(a|a)") || 
-	   strings.Contains(pattern, "(1|1)") || 
-	   strings.Contains(pattern, "(.*|.*)") {
-		return true
-	}
-	
-	return false
-}
 
 // safeRegexpCompile compiles a regex with timeout protection
 func safeRegexpCompile(pattern string) (*regexp2.Regexp, error) {
@@ -149,11 +104,6 @@ func parseFilter(filter string) (*regexp2.Regexp, error) {
 	}
 
 	regexPattern := matches[1]
-	
-	// Security: Check for pathological regex patterns
-	if isPathologicalRegex(regexPattern) {
-		return nil, backend.DownstreamErrorf("error parsing regexp: potentially dangerous regex pattern detected")
-	}
 
 	pattern := ""
 	if matches[2] != "" {
@@ -165,12 +115,15 @@ func parseFilter(filter string) (*regexp2.Regexp, error) {
 	}
 	pattern += regexPattern
 
-	// Security: Test compilation with timeout
+	// Security: Compile regex with timeout protection
 	compiled, err := safeRegexpCompile(pattern)
 	if err != nil {
 		return nil, backend.DownstreamErrorf("error parsing regexp: %v", err)
 	}
 
+	// Set match timeout for runtime DoS protection (5 second timeout)
+	compiled.MatchTimeout = 5 * time.Second
+
 	return compiled, nil
 }
 

pkg/zabbix/utils_test.go

@@ -1,10 +1,9 @@
 package zabbix
 
 import (
-	"reflect"
 	"testing"
+	"time"
 
-	"github.com/dlclark/regexp2"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -63,70 +62,42 @@ func TestParseFilter(t *testing.T) {
 	tests := []struct {
 		name          string
 		filter        string
-		want          *regexp2.Regexp
+		wantPattern   string
 		expectNoError bool
 		expectedError string
 	}{
 		{
 			name:          "Simple regexp",
 			filter:        "/.*/",
-			want:          regexp2.MustCompile(".*", regexp2.RE2),
+			wantPattern:   ".*",
 			expectNoError: true,
 			expectedError: "",
 		},
 		{
 			name:          "Not a regex",
 			filter:        "/var/lib/mysql: Total space",
-			want:          nil,
+			wantPattern:   "",
 			expectNoError: true,
 			expectedError: "",
 		},
 		{
 			name:          "Regexp with modifier",
 			filter:        "/.*/i",
-			want:          regexp2.MustCompile("(?i).*", regexp2.RE2),
+			wantPattern:   "(?i).*",
 			expectNoError: true,
 			expectedError: "",
 		},
 		{
 			name:          "Regexp with unsupported modifier",
 			filter:        "/.*/1",
-			want:          nil,
+			wantPattern:   "",
 			expectNoError: false,
 			expectedError: "",
 		},
-		{
-			name:          "Pathological regex - nested quantifiers (a+)+",
-			filter:        "/^(a+)+$/",
-			want:          nil,
-			expectNoError: false,
-			expectedError: "error parsing regexp: potentially dangerous regex pattern detected",
-		},
-		{
-			name:          "Pathological regex - (.*)* pattern",
-			filter:        "/(.*)*/",
-			want:          nil,
-			expectNoError: false,
-			expectedError: "error parsing regexp: potentially dangerous regex pattern detected",
-		},
-		{
-			name:          "Pathological regex - overlapping alternation",
-			filter:        "/(a|a)*/",
-			want:          nil,
-			expectNoError: false,
-			expectedError: "error parsing regexp: potentially dangerous regex pattern detected",
-		},
-		{
-			name:          "Pathological regex - consecutive quantifiers",
-			filter:        "/a**/",
-			want:          nil,
-			expectNoError: false,
-			expectedError: "error parsing regexp: potentially dangerous regex pattern detected",
-		},
 		{
 			name:          "Safe complex regex",
 			filter:        "/^[a-zA-Z0-9_-]+\\.[a-zA-Z]{2,}$/",
-			want:          regexp2.MustCompile("^[a-zA-Z0-9_-]+\\.[a-zA-Z]{2,}$", regexp2.RE2),
+			wantPattern:   "^[a-zA-Z0-9_-]+\\.[a-zA-Z]{2,}$",
 			expectNoError: true,
 			expectedError: "",
 		},
@@ -142,75 +113,42 @@ func TestParseFilter(t *testing.T) {
 				assert.Error(t, err)
 				assert.EqualError(t, err, tt.expectedError)
 			}
-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("parseFilter() = %v, want %v", got, tt.want)
+			if tt.wantPattern == "" {
+				assert.Nil(t, got)
+			} else {
+				assert.NotNil(t, got)
+				assert.Equal(t, tt.wantPattern, got.String())
+				// Verify that timeout is set for DoS protection
+				assert.Equal(t, 5*time.Second, got.MatchTimeout)
 			}
 		})
 	}
 }
 
-func TestIsPathologicalRegex(t *testing.T) {
-	tests := []struct {
-		name     string
-		pattern  string
-		expected bool
-	}{
-		{
-			name:     "Safe pattern",
-			pattern:  "^[a-zA-Z0-9]+$",
-			expected: false,
-		},
-		{
-			name:     "Nested quantifiers (a+)+",
-			pattern:  "^(a+)+$",
-			expected: true,
-		},
-		{
-			name:     "Nested quantifiers (a*)*",
-			pattern:  "(a*)*",
-			expected: true,
-		},
-		{
-			name:     "Overlapping alternation (a|a)*",
-			pattern:  "(a|a)*",
-			expected: true,
-		},
-		{
-			name:     "Consecutive quantifiers **",
-			pattern:  "a**",
-			expected: true,
-		},
-		{
-			name:     "Consecutive quantifiers ++",
-			pattern:  "a++",
-			expected: true,
-		},
-		{
-			name:     "Catastrophic (.*)* pattern",
-			pattern:  "(.*)*",
-			expected: true,
-		},
-		{
-			name:     "Catastrophic (.+)+ pattern",
-			pattern:  "(.+)+",
-			expected: true,
-		},
-		{
-			name:     "Safe alternation with different patterns",
-			pattern:  "(cat|dog)*",
-			expected: false,
-		},
-		{
-			name:     "Safe quantifier usage",
-			pattern:  "[0-9]+\\.[0-9]*",
-			expected: false,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			result := isPathologicalRegex(tt.pattern)
-			assert.Equal(t, tt.expected, result, "Pattern: %s", tt.pattern)
-		})
+func TestParseFilterTimeout(t *testing.T) {
+	// Test with a pathological regex pattern that should trigger MatchTimeout
+	filter := "/((((.*)*)*)*)*z/"
+	compiled, err := parseFilter(filter)
+	
+	// The regex should compile successfully with timeout protection
+	assert.NoError(t, err)
+	assert.NotNil(t, compiled)
+	assert.Equal(t, 5*time.Second, compiled.MatchTimeout)
+	
+	// Test that the regex times out when matching against a problematic string
+	// This string is crafted to trigger catastrophic backtracking
+	testString := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" // 52 'a's, no 'z'
+	
+	// The match should timeout and return an error
+	match, err := compiled.MatchString(testString)
+	
+	// We expect either a timeout error or the match to complete quickly if RE2 optimizations prevent catastrophic backtracking
+	// In either case, the system should remain responsive
+	assert.False(t, match) // Should not match since there's no 'z'
+	
+	// If there's an error, it should be related to timeout
+	if err != nil {
+		assert.Contains(t, err.Error(), "timeout")
 	}
 }
+