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
This commit is contained in:
Kristian Bremberg
committed by
GitHub
GitHub
parent
0194360f61
commit
6580bf8f6e
@@ -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) {
|
||||
@@ -150,11 +105,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] != "" {
|
||||
if flagRE.MatchString(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
|
||||
}
|
||||
|
||||
|
||||
@@ -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,
|
||||
},
|
||||
}
|
||||
func TestParseFilterTimeout(t *testing.T) {
|
||||
// Test with a pathological regex pattern that should trigger MatchTimeout
|
||||
filter := "/((((.*)*)*)*)*z/"
|
||||
compiled, err := parseFilter(filter)
|
||||
|
||||
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)
|
||||
})
|
||||
// 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")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user