From 6580bf8f6e6843d59b664742aa51fd3a9f7de851 Mon Sep 17 00:00:00 2001 From: Kristian Bremberg <114284895+KristianGrafana@users.noreply.github.com> Date: Wed, 24 Sep 2025 14:27:16 +0200 Subject: [PATCH] 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 --- pkg/zabbix/utils.go | 55 ++------------- pkg/zabbix/utils_test.go | 140 +++++++++++---------------------------- 2 files changed, 43 insertions(+), 152 deletions(-) diff --git a/pkg/zabbix/utils.go b/pkg/zabbix/utils.go index e97657b..80c2767 100644 --- a/pkg/zabbix/utils.go +++ b/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 } diff --git a/pkg/zabbix/utils_test.go b/pkg/zabbix/utils_test.go index 697a39f..0f84c3d 100644 --- a/pkg/zabbix/utils_test.go +++ b/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") } } +