Fix memory leak of validateDTD's dtd object

owasp-modsecurity · Oct 30, 2023 · beaa452 · beaa452
1 parent a9edee3
commit beaa452
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 22 deletions.
diff --git a/CHANGES b/CHANGES
@@ -1,6 +1,8 @@
 v3.x.y - YYYY-MMM-DD (to be released)
 -------------------------------------
 
+  - Fix memory leak of validateDTD's dtd object
+    [Issue #3008 - @martinhsv, @zimmerle]
   - Fix memory leaks in ValidateSchema
     [Issue #3005 - @martinhsv, @zimmerle]
   - Add support for expirevar action

diff --git a/src/operators/validate_dtd.cc b/src/operators/validate_dtd.cc
@@ -1,6 +1,6 @@
 /*
  * ModSecurity, http://www.modsecurity.org/
- * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. (http://www.trustwave.com/)
+ * Copyright (c) 2015 - 2023 Trustwave Holdings, Inc. (http://www.trustwave.com/)
  *
  * You may not use this file except in compliance with
  * the License.  You may obtain a copy of the License at
@@ -43,25 +43,24 @@ bool ValidateDTD::init(const std::string &file, std::string *error) {
 }
 
 
-bool ValidateDTD::evaluate(Transaction *t, const std::string &str) {
-    xmlValidCtxtPtr cvp;
+bool ValidateDTD::evaluate(Transaction *transaction, const std::string &str) {
 
-    m_dtd = xmlParseDTD(NULL, (const xmlChar *)m_resource.c_str());
-    if (m_dtd == NULL) {
+    XmlDtdPtrManager dtd(xmlParseDTD(NULL, (const xmlChar *)m_resource.c_str()));
+    if (dtd.get() == NULL) {
         std::string err = std::string("XML: Failed to load DTD: ") \
             + m_resource;
-        ms_dbg_a(t, 4, err);
+        ms_dbg_a(transaction, 4, err);
         return true;
     }
 
-    if (t->m_xml->m_data.doc == NULL) {
-        ms_dbg_a(t, 4, "XML document tree could not "\
+    if (transaction->m_xml->m_data.doc == NULL) {
+        ms_dbg_a(transaction, 4, "XML document tree could not "\
             "be found for DTD validation.");
         return true;
     }
 
-    if (t->m_xml->m_data.well_formed != 1) {
-        ms_dbg_a(t, 4, "XML: DTD validation failed because " \
+    if (transaction->m_xml->m_data.well_formed != 1) {
+        ms_dbg_a(transaction, 4, "XML: DTD validation failed because " \
             "content is not well formed.");
         return true;
     }
@@ -76,24 +75,24 @@ bool ValidateDTD::evaluate(Transaction *t, const std::string &str) {
     }
 #endif
 
-    cvp = xmlNewValidCtxt();
+    xmlValidCtxtPtr cvp = xmlNewValidCtxt();
     if (cvp == NULL) {
-        ms_dbg_a(t, 4, "XML: Failed to create a validation context.");
+        ms_dbg_a(transaction, 4, "XML: Failed to create a validation context.");
         return true;
     }
 
     /* Send validator errors/warnings to msr_log */
     cvp->error = (xmlSchemaValidityErrorFunc)error_runtime;
     cvp->warning = (xmlSchemaValidityErrorFunc)warn_runtime;
-    cvp->userData = t;
+    cvp->userData = transaction;
 
-    if (!xmlValidateDtd(cvp, t->m_xml->m_data.doc, m_dtd)) {
-        ms_dbg_a(t, 4, "XML: DTD validation failed.");
+    if (!xmlValidateDtd(cvp, transaction->m_xml->m_data.doc, dtd.get())) {
+        ms_dbg_a(transaction, 4, "XML: DTD validation failed.");
         xmlFreeValidCtxt(cvp);
         return true;
     }
 
-    ms_dbg_a(t, 4, std::string("XML: Successfully validated " \
+    ms_dbg_a(transaction, 4, std::string("XML: Successfully validated " \
         "payload against DTD: ") + m_resource);
 
     xmlFreeValidCtxt(cvp);

diff --git a/src/operators/validate_dtd.h b/src/operators/validate_dtd.h
@@ -1,6 +1,6 @@
 /*
  * ModSecurity, http://www.modsecurity.org/
- * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. (http://www.trustwave.com/)
+ * Copyright (c) 2015 - 2023 Trustwave Holdings, Inc. (http://www.trustwave.com/)
  *
  * You may not use this file except in compliance with
  * the License.  You may obtain a copy of the License at
@@ -33,18 +33,31 @@
 namespace modsecurity {
 namespace operators {
 
-class ValidateDTD : public Operator {
+class XmlDtdPtrManager {
  public:
     /** @ingroup ModSecurity_Operator */
-    explicit ValidateDTD(std::unique_ptr<RunTimeString> param)
-        : Operator("ValidateDTD", std::move(param)) { }
+    explicit XmlDtdPtrManager(xmlDtdPtr dtd)
+        : m_dtd(dtd) { }
+    ~XmlDtdPtrManager() {
 #ifdef WITH_LIBXML2
-    ~ValidateDTD() {
         if (m_dtd != NULL) {
             xmlFreeDtd(m_dtd);
             m_dtd = NULL;
         }
+#endif
     }
+    xmlDtdPtr get() const {return m_dtd;}
+ private:
+    xmlDtdPtr m_dtd; // The resource being managed
+};
+
+class ValidateDTD : public Operator {
+ public:
+    /** @ingroup ModSecurity_Operator */
+    explicit ValidateDTD(std::unique_ptr<RunTimeString> param)
+        : Operator("ValidateDTD", std::move(param)) { }
+#ifdef WITH_LIBXML2
+    ~ValidateDTD() { }
 
     bool evaluate(Transaction *transaction, const std::string  &str) override;
     bool init(const std::string &file, std::string *error) override;
@@ -89,7 +102,6 @@ class ValidateDTD : public Operator {
 
  private:
     std::string m_resource;
-    xmlDtdPtr m_dtd = NULL;
 #endif
 };