Add proper error handling to @rx operator

owasp-modsecurity · Sep 5, 2020 · 95f16ea · 95f16ea
1 parent 5437b44
commit 95f16ea
Show file tree

Hide file tree

Showing 7 changed files with 102 additions and 7 deletions.
diff --git a/src/operators/rx.cc b/src/operators/rx.cc
@@ -29,7 +29,12 @@ namespace operators {
 
 bool Rx::init(const std::string &arg, std::string *error) {
     if (m_string->m_containsMacro == false) {
+        std::string regex_error;
         m_re = new Regex(m_param);
+        if (!m_re->ok(&regex_error)) {
+            *error = "Failed to compile regular expression " + m_re->getPattern() + ": " + regex_error;
+            return false;
+        }
     }
 
     return true;
@@ -47,6 +52,14 @@ bool Rx::evaluate(Transaction *transaction, RuleWithActions *rule,
     if (m_string->m_containsMacro) {
         std::string eparam(m_string->evaluate(transaction));
         re = new Regex(eparam);
+        std::string regex_error;
+        if (!re->ok(&regex_error)) {
+            ms_dbg_a(transaction, 2,
+                    "Failed to compile regular expression with macro "
+                    + re->getPattern() + ": " + regex_error);
+            delete re;
+            return false;
+        }
     } else {
         re = m_re;
     }

diff --git a/src/regex/backend/backend.h b/src/regex/backend/backend.h
@@ -29,7 +29,7 @@ class Backend {
 public:
     virtual ~Backend() {}
 
-    virtual bool ok() const = 0;
+    virtual bool ok(std::string *error = nullptr) const = 0;
 
     virtual std::list<RegexMatch> searchAll(const std::string& s) const = 0;
     virtual bool searchOneMatch(const std::string& s, std::vector<RegexMatchCapture>& captures) const = 0;

diff --git a/src/regex/backend/pcre.cc b/src/regex/backend/pcre.cc
@@ -44,8 +44,18 @@ Pcre::Pcre(const std::string& pattern_)
 
     m_pc = pcre_compile(pattern.c_str(), PCRE_DOTALL|PCRE_MULTILINE,
         &errptr, &erroffset, NULL);
+    if (m_pc == NULL) {
+        m_error = "pcre_compile error at offset " + std::to_string(erroffset) + ": " + std::string(errptr);
+        return;
+    }
 
     m_pce = pcre_study(m_pc, pcre_study_opt, &errptr);
+    if (m_pce == NULL) {
+        m_error = "pcre_study error: " + std::string(errptr);
+        pcre_free(m_pc);
+        m_pc = nullptr;
+        return;
+    }
 }
 
 

diff --git a/src/regex/backend/pcre.h b/src/regex/backend/pcre.h
@@ -52,8 +52,15 @@ class Pcre : public Backend {
     int search(const std::string &s, RegexMatch *m) const override;
     int search(const std::string &s) const override;
 
-    virtual bool ok() const override {
-        return m_pc != NULL;
+    virtual bool ok(std::string *error = nullptr) const override {
+        if (m_pc != NULL) {
+            return true;
+        }
+        if (error != nullptr) {
+            *error= m_error;
+        }
+
+        return false;
     }
 
     virtual const std::string& getPattern() const override {
@@ -64,6 +71,8 @@ class Pcre : public Backend {
 
     pcre *m_pc = NULL;
     pcre_extra *m_pce = NULL;
+
+    std::string m_error;
 };
 
 #endif

diff --git a/src/regex/backend/re2.h b/src/regex/backend/re2.h
@@ -45,8 +45,14 @@ class Re2 : public Backend {
     bool searchOneMatch(const std::string& s, std::vector<RegexMatchCapture>& captures) const override;
     int search(const std::string &s, RegexMatch *m) const override;
     int search(const std::string &s) const override;
-    virtual bool ok() const override {
-        return re.ok();
+    virtual bool ok(std::string *error = nullptr) const override {
+        if (re.ok()) {
+            return true;
+        }
+        if (error != nullptr) {
+            *error = re.error();
+        }
+        return false;
     }
 
     virtual const std::string& getPattern() const override {

diff --git a/src/regex/backend_fallback.h b/src/regex/backend_fallback.h
@@ -45,8 +45,8 @@ class BackendFallback : public backend::Backend {
         : backend(compile_regex_fallback<Args...>(pattern))
     {}
 
-    virtual bool ok() const override {
-        return backend->ok();
+    virtual bool ok(std::string *error = nullptr) const override {
+        return backend->ok(error);
     }
 
     std::list<RegexMatch> searchAll(const std::string& s) const override {

diff --git a/test/test-cases/regression/operator-rx.json b/test/test-cases/regression/operator-rx.json
@@ -85,5 +85,62 @@
       "SecRuleEngine On",
       "SecRule REQUEST_HEADERS:Content-Length \"!^0$\" \"id:1,phase:2,pass,t:trim,block\""
     ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "version_max":0,
+    "title":"Testing Operator :: @rx with invalid regular expression",
+    "expected":{
+      "parser_error":"Rules error.*Failed to compile regular expression \\(\\(value1\\):"
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule ARGS \"@rx ((value1)\" \"id:1,phase:2,pass,t:trim\""
+    ]
+  },
+  {
+    "enabled":1,
+    "version_min":300000,
+    "title":"Testing Operator :: @rx with invalid regular expression after macro expansion",
+    "client":{
+      "ip":"200.249.12.31",
+      "port":123
+    },
+    "server":{
+      "ip":"200.249.12.31",
+      "port":80
+    },
+    "request":{
+      "headers":{
+        "Host":"localhost",
+        "User-Agent":"curl/7.38.0",
+        "Accept":"*/*",
+        "Content-Length": "27",
+        "Content-Type": "application/x-www-form-urlencoded"
+      },
+      "uri":"/",
+      "method":"POST",
+      "body": [
+        "param1=value1&param2=value2"
+      ]
+    },
+    "response":{
+      "headers":{
+        "Date":"Mon, 13 Jul 2015 20:02:41 GMT",
+        "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT",
+        "Content-Type":"text/html"
+      },
+      "body":[
+        "no need."
+      ]
+    },
+    "expected":{
+      "debug_log":"Failed to compile regular expression with macro \\(\\(\\):"
+    },
+    "rules":[
+      "SecRuleEngine On",
+      "SecRule ARGS \"@rx ((%{TX.DOESNT_NEED_TO_EXIST_IT_WILL_BE_AN_EMPTY_STRING})\" \"id:1,phase:2,pass,t:trim\""
+    ]
   }
 ]