Merged 2.5.x changes for 2.5.11 into trunk.

CHANGES

@@ -4,6 +4,30 @@
  * Cleanup build files that were from the Apache source.
 
 
+04 Nov 2009 - 2.5.11
+--------------------
+
+ * Added a new multipart flag, MULTIPART_INVALID_QUOTING, which will be
+   set true if any invalid quoting is found during multipart parsing.
+
+ * Fixed parsing quoted strings in multipart Content-Disposition headers.
+   Discovered by Stefan Esser.
+
+ * Cleanup persistence database locking code.
+
+ * Added warning during configure if libcurl is found linked against
+   gnutls for SSL.  The openssl lib is recommended as gnutls has
+   proven to cause issues with mutexes and may crash.
+
+ * Cleanup some mlogc (over)logging.
+
+ * Do not log output filter errors in the error log.
+
+ * Moved output filter to run before other stock filters (mod_deflate,
+   mod_cache, mod_expires, mod_filter) to avoid analyzing modified data
+   in the response.  Patch originally submitted by Ivan Ristic.
+
+
 18 Sep 2009 - 2.5.10
 --------------------
 

apache2/apache2_config.c

@@ -490,7 +490,7 @@ void init_directory_config(directory_config *dcfg) {
     if (dcfg->is_enabled == NOT_SET) dcfg->is_enabled = 0;
 
     if (dcfg->reqbody_access == NOT_SET) dcfg->reqbody_access = 0;
-    if (dcfg->reqbody_buffering == NOT_SET) dcfg->reqbody_buffering = 0;
+    if (dcfg->reqbody_buffering == NOT_SET) dcfg->reqbody_buffering = REQUEST_BODY_FORCEBUF_OFF;
     if (dcfg->reqbody_inmemory_limit == NOT_SET)
         dcfg->reqbody_inmemory_limit = REQUEST_BODY_DEFAULT_INMEMORY_LIMIT;
     if (dcfg->reqbody_limit == NOT_SET) dcfg->reqbody_limit = REQUEST_BODY_DEFAULT_LIMIT;

apache2/apache2_io.c

@@ -407,16 +407,10 @@ static apr_status_t send_of_brigade(modsec_rec *msr, ap_filter_t *f) {
 
     rc = ap_pass_brigade(f->next, msr->of_brigade);
     if (rc != APR_SUCCESS) {
-        int log_level = 1;
+        /* TODO: These need to move to flags in 2.6.  For now log them
-
+         * at level 4 so that they are not confusing users.
-        if (APR_STATUS_IS_ECONNRESET(rc)) {
-            /* Message "Connection reset by peer" is common and not a sign
-             * of something unusual. Hence we don't want to make a big deal
-             * about it, logging at NOTICE level. Everything else we log
-             * at ERROR level.
          */
-             log_level = 3;
+        int log_level = 4;
-        }
 
         if (msr->txcfg->debuglog_level >= log_level) {
             switch(rc) {

apache2/autogen.sh

@@ -1,4 +1,6 @@
 #!/bin/sh
 
+rm -rf autom4te.cache
+
 #automake --add-missing --copy
 autoreconf --install

apache2/build/find_curl.m4

@@ -70,12 +70,25 @@ if test -n "${curl_path}"; then
         AC_MSG_NOTICE([NOTE: curl library may be too old: $CURL_VERSION])
     fi
 
+    dnl # Check/warn if GnuTLS is used
+    AC_MSG_CHECKING([if libcurl is linked with gnutls])
+    curl_uses_gnutls=`echo ${CURL_LIBS} | grep gnutls | wc -l`
+    if test "$curl_uses_gnutls" -ne 0; then
+        AC_MSG_RESULT([yes])
+        AC_MSG_NOTICE([NOTE: curl linked with gnutls may be buggy, openssl recommended])
+        CURL_USES_GNUTLS=yes
+    else
+        AC_MSG_RESULT([no])
+        CURL_USES_GNUTLS=no
+    fi
+
 else
     AC_MSG_RESULT([no])
 fi
 
 AC_SUBST(CURL_LIBS)
 AC_SUBST(CURL_CFLAGS)
+AC_SUBST(CURL_USES_GNUTLS)
 
 if test -z "${CURL_LIBS}"; then
   AC_MSG_NOTICE([*** curl library not found.])

apache2/mlogc-src/mlogc.c

@@ -389,7 +389,7 @@ static void add_entry(const char *data, int start_worker)
 
     error_log(LOG_DEBUG, NULL, "Queue locking thread mutex.");
     if (APR_STATUS_IS_EBUSY(apr_thread_mutex_trylock(mutex))) {
-        error_log(LOG_WARNING, NULL, "Queue waiting on thread mutex.");
+        error_log(LOG_DEBUG, NULL, "Queue waiting on thread mutex.");
         apr_thread_mutex_lock(mutex);
     }
 
@@ -478,7 +478,7 @@ static void transaction_log_init(void)
     /* Put a lock in place to ensure exclusivity. */
     error_log(LOG_DEBUG, NULL, "Transaction initialization locking global mutex.");
     if (APR_STATUS_IS_EBUSY(apr_global_mutex_trylock(gmutex))) {
-        error_log(LOG_WARNING, NULL, "Transaction initialization waiting on global mutex.");
+        error_log(LOG_DEBUG, NULL, "Transaction initialization waiting on global mutex.");
         apr_global_mutex_lock(gmutex);
     }
 
@@ -582,7 +582,7 @@ static void transaction_checkpoint(void)
     /* Put a lock in place to ensure exclusivity. */
     error_log(LOG_DEBUG, NULL, "Checkpoint locking global mutex.");
     if (APR_STATUS_IS_EBUSY(apr_global_mutex_trylock(gmutex))) {
-        error_log(LOG_WARNING, NULL, "Checkpoint waiting on global mutex.");
+        error_log(LOG_DEBUG, NULL, "Checkpoint waiting on global mutex.");
         apr_global_mutex_lock(gmutex);
     }
 
@@ -1532,7 +1532,7 @@ static void * APR_THREAD_FUNC thread_worker(apr_thread_t *thread, void *data)
 
     error_log(LOG_DEBUG, thread, "Worker shutdown locking thread mutex.");
     if (APR_STATUS_IS_EBUSY(apr_thread_mutex_trylock(mutex))) {
-        error_log(LOG_WARNING, thread, "Worker shutdown waiting on thread mutex.");
+        error_log(LOG_DEBUG, thread, "Worker shutdown waiting on thread mutex.");
         apr_thread_mutex_lock(mutex);
     }
 
@@ -1583,7 +1583,7 @@ static void create_new_worker(int lock)
     if (lock) {
         error_log(LOG_DEBUG, NULL, "Worker creation locking thread mutex.");
         if (APR_STATUS_IS_EBUSY(apr_thread_mutex_trylock(mutex))) {
-            error_log(LOG_WARNING, NULL, "Worker creation waiting on thread mutex.");
+            error_log(LOG_DEBUG, NULL, "Worker creation waiting on thread mutex.");
             apr_thread_mutex_lock(mutex);
         }
     }

apache2/mod_security2.c

@@ -1129,8 +1129,18 @@ static void register_hooks(apr_pool_t *mp) {
 
     ap_register_input_filter("MODSECURITY_IN", input_filter,
         NULL, AP_FTYPE_CONTENT_SET);
+
+    /* Ensure that the output filter runs before other modules so that
+     * we get a request that has a better chance of not being modified:
+     *
+     * Currently:
+     *   mod_expires = -2
+     *   mod_cache   = -1
+     *   mod_deflate = -1
+     *   mod_headers =  0
+     */
     ap_register_output_filter("MODSECURITY_OUT", output_filter,
-        NULL, AP_FTYPE_CONTENT_SET);
+        NULL, AP_FTYPE_CONTENT_SET - 3);
 
     ap_register_output_filter("PDFP_OUT", pdfp_output_filter,
         NULL, AP_FTYPE_CONTENT_SET);

apache2/mod_security2_config.h.in

@@ -85,6 +85,9 @@
 /* Define to the one symbol short name of this package. */
 #undef PACKAGE_TARNAME
 
+/* Define to the home page for this package. */
+#undef PACKAGE_URL
+
 /* Define to the version of this package. */
 #undef PACKAGE_VERSION
 
@@ -118,13 +121,14 @@
    nothing if this is not supported.  Do not define if restrict is
    supported directly.  */
 #undef restrict
-/* Work around a bug in Sun C++: it does not support _Restrict, even
+/* Work around a bug in Sun C++: it does not support _Restrict or
-   though the corresponding Sun C compiler does, which causes
+   __restrict__, even though the corresponding Sun C compiler ends up with
-   "#define restrict _Restrict" in the previous line.  Perhaps some future
+   "#define restrict _Restrict" or "#define restrict __restrict__" in the
-   version of Sun C++ will work with _Restrict; if so, it'll probably
+   previous line.  Perhaps some future version of Sun C++ will work with
-   define __RESTRICT, just as Sun C does.  */
+   restrict; if so, hopefully it defines __RESTRICT like Sun C does.  */
 #if defined __SUNPRO_CC && !defined __RESTRICT
 # define _Restrict
+# define __restrict__
 #endif
 
 /* Define to `unsigned int' if <sys/types.h> does not define. */

apache2/modsecurity.c

@@ -277,7 +277,7 @@ apr_status_t modsecurity_tx_init(modsec_rec *msr) {
     }
 
     /* Check if we are forcing buffering, then use memory only. */
-    if (msr->txcfg->reqbody_buffering) {
+    if (msr->txcfg->reqbody_buffering != REQUEST_BODY_FORCEBUF_OFF) {
         msr->msc_reqbody_storage = MSC_REQBODY_MEMORY;
         msr->msc_reqbody_spilltodisk = 0;
     }

apache2/modsecurity.h

@@ -90,6 +90,9 @@ typedef struct msc_string msc_string;
 #define RESPONSE_BODY_LIMIT_ACTION_REJECT       0
 #define RESPONSE_BODY_LIMIT_ACTION_PARTIAL      1
 
+#define REQUEST_BODY_FORCEBUF_OFF               0
+#define REQUEST_BODY_FORCEBUF_ON                1
+
 #define SECACTION_TARGETS                       "REQUEST_URI"
 #define SECACTION_ARGS                          "@unconditionalMatch"
 

apache2/msc_multipart.c

@@ -45,7 +45,7 @@ static char *multipart_construct_filename(modsec_rec *msr) {
      */
     p = filename = apr_pstrdup(msr->mp, q);
     while((c = *p) != 0) {
-        if (!( isalnum(c)||(c == '.') )) *p = '_';
+        if (!( isalnum(c) || (c == '.') )) *p = '_';
         p++;
     }
 
@@ -67,7 +67,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
     /* see if there are any other parts to parse */
 
     p = c_d_value + 9;
-    while((*p == '\t')||(*p == ' ')) p++;
+    while((*p == '\t') || (*p == ' ')) p++;
     if (*p == '\0') return 1; /* this is OK */
 
     if (*p != ';') return -2;
@@ -79,26 +79,35 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
         char *name = NULL, *value = NULL, *start = NULL;
 
         /* go over the whitespace */
-        while((*p == '\t')||(*p == ' ')) p++;
+        while((*p == '\t') || (*p == ' ')) p++;
         if (*p == '\0') return -3;
 
         start = p;
-        while((*p != '\0')&&(*p != '=')&&(*p != '\t')&&(*p != ' ')) p++;
+        while((*p != '\0') && (*p != '=') && (*p != '\t') && (*p != ' ')) p++;
         if (*p == '\0') return -4;
 
         name = apr_pstrmemdup(msr->mp, start, (p - start));
 
-        while((*p == '\t')||(*p == ' ')) p++;
+        while((*p == '\t') || (*p == ' ')) p++;
         if (*p == '\0') return -5;
 
         if (*p != '=') return -13;
         p++;
 
-        while((*p == '\t')||(*p == ' ')) p++;
+        while((*p == '\t') || (*p == ' ')) p++;
         if (*p == '\0') return -6;
 
-        if (*p == '"') {
+        /* Accept both quotes as some backends will accept them, but
+         * technically "'" is invalid and so flag_invalid_quoting is
+         * set so the user can deal with it in the rules if they so wish.
+         */
+        if ((*p == '"') || (*p == '\'')) {
             /* quoted */
+            char quote = *p;
+
+            if (quote == '\'') {
+                msr->mpd->flag_invalid_quoting = 1;
+            }
 
             p++;
             if (*p == '\0') return -7;
@@ -113,8 +122,8 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
                         /* improper escaping */
                         return -8;
                     }
-                    /* only " and \ can be escaped */
+                    /* only quote and \ can be escaped */
-                    if ((*(p + 1) == '"')||(*(p + 1) == '\\')) {
+                    if ((*(p + 1) == quote) || (*(p + 1) == '\\')) {
                         p++;
                     }
                     else {
@@ -128,8 +137,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
                          */
                     }
                 }
-                else
+                else if (*p == quote) {
-                if (*p == '"') {
                     *t = '\0';
                     break;
                 }
@@ -144,14 +152,18 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
             /* not quoted */
 
             start = p;
-            while((*p != '\0')&&(is_token_char(*p))) p++;
+            while((*p != '\0') && (is_token_char(*p))) p++;
             value = apr_pstrmemdup(msr->mp, start, (p - start));
         }
 
         /* evaluate part */
 
         if (strcmp(name, "name") == 0) {
-            if (msr->mpd->mpp->name != NULL) return -14;
+            if (msr->mpd->mpp->name != NULL) {
+                msr_log(msr, 4, "Multipart: Warning: Duplicate Content-Disposition name: %s",
+                    log_escape_nq(msr->mp, value));
+                return -14;
+            }
             msr->mpd->mpp->name = value;
 
             if (msr->txcfg->debuglog_level >= 9) {
@@ -161,7 +173,11 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
         }
         else
         if (strcmp(name, "filename") == 0) {
-            if (msr->mpd->mpp->filename != NULL) return -15;
+            if (msr->mpd->mpp->filename != NULL) {
+                msr_log(msr, 4, "Multipart: Warning: Duplicate Content-Disposition filename: %s",
+                    log_escape_nq(msr->mp, value));
+                return -15;
+            }
             msr->mpd->mpp->filename = value;
 
             if (msr->txcfg->debuglog_level >= 9) {
@@ -172,7 +188,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
         else return -11;
 
         if (*p != '\0') {
-            while((*p == '\t')||(*p == ' ')) p++;
+            while((*p == '\t') || (*p == ' ')) p++;
             /* the next character must be a zero or a semi-colon */
             if (*p == '\0') return 1; /* this is OK */
             if (*p != ';') return -12;
@@ -263,7 +279,7 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) {
     } else {
         /* Header line. */
 
-        if ((msr->mpd->buf[0] == '\t')||(msr->mpd->buf[0] == ' ')) {
+        if ((msr->mpd->buf[0] == '\t') || (msr->mpd->buf[0] == ' ')) {
             char *header_value, *new_value, *data;
 
             /* header folding, add data to the header we are building */
@@ -277,7 +293,7 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) {
 
             /* locate the beginning of data */
             data = msr->mpd->buf;
-            while((*data == '\t')||(*data == ' ')) data++;
+            while((*data == '\t') || (*data == ' ')) data++;
 
             new_value = apr_pstrdup(msr->mp, data);
             remove_lf_crlf_inplace(new_value);
@@ -303,7 +319,7 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) {
             /* new header */
 
             data = msr->mpd->buf;
-            while((*data != ':')&&(*data != '\0')) data++;
+            while((*data != ':') && (*data != '\0')) data++;
             if (*data == '\0') {
                 *error_msg = apr_psprintf(msr->mp, "Multipart: Invalid part header (colon missing): %s.",
                     log_escape_nq(msr->mp, msr->mpd->buf));
@@ -320,7 +336,7 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) {
 
             /* extract the value value */
             data++;
-            while((*data == '\t')||(*data == ' ')) data++;
+            while((*data == '\t') || (*data == ' ')) data++;
             header_value = apr_pstrdup(msr->mp, data);
             remove_lf_crlf_inplace(header_value);
 
@@ -713,7 +729,7 @@ int multipart_init(modsec_rec *msr, char **error_msg) {
         /* Check for extra characters before the boundary. */
         for (p = (char *)(msr->request_content_type + 19); p < msr->mpd->boundary; p++) {
             if (!isspace(*p)) {
-                if ((seen_semicolon == 0)&&(*p == ';')) {
+                if ((seen_semicolon == 0) && (*p == ';')) {
                     seen_semicolon = 1; /* It is OK to have one semicolon. */
                 } else {
                     msr->mpd->flag_error = 1;
@@ -761,7 +777,7 @@ int multipart_init(modsec_rec *msr, char **error_msg) {
         }
 
         /* Is the boundary quoted? */
-        if ((len >= 2)&&(*b == '"')&&(*(b + len - 1) == '"')) {
+        if ((len >= 2) && (*b == '"') && (*(b + len - 1) == '"')) {
             /* Quoted. */
             msr->mpd->boundary = apr_pstrndup(msr->mp, b + 1, len - 2);
             if (msr->mpd->boundary == NULL) return -1;
@@ -771,7 +787,7 @@ int multipart_init(modsec_rec *msr, char **error_msg) {
 
             /* Test for partial quoting. */
             if (   (*b == '"')
-                || ((len >= 2)&&(*(b + len - 1) == '"')) )
+                || ((len >= 2) && (*(b + len - 1) == '"')) )
             {
                 msr->mpd->flag_error = 1;
                 *error_msg = apr_psprintf(msr->mp, "Multipart: Invalid boundary in C-T (quote).");
@@ -865,14 +881,15 @@ int multipart_complete(modsec_rec *msr, char **error_msg) {
         }
     }
 
-    if ((msr->mpd->seen_data != 0)&&(msr->mpd->is_complete == 0)) {
+    if ((msr->mpd->seen_data != 0) && (msr->mpd->is_complete == 0)) {
         if (msr->mpd->boundary_count > 0) {
             /* Check if we have the final boundary (that we haven't
              * processed yet) in the buffer.
              */
             if (msr->mpd->buf_contains_line) {
                 if ( ((unsigned int)(MULTIPART_BUF_SIZE - msr->mpd->bufleft) == (4 + strlen(msr->mpd->boundary)))
-                    && (*(msr->mpd->buf) == '-')&&(*(msr->mpd->buf + 1) == '-')
+                    && (*(msr->mpd->buf) == '-')
+                    && (*(msr->mpd->buf + 1) == '-')
                     && (strncmp(msr->mpd->buf + 2, msr->mpd->boundary, strlen(msr->mpd->boundary)) == 0)
                     && (*(msr->mpd->buf + 2 + strlen(msr->mpd->boundary)) == '-')
                     && (*(msr->mpd->buf + 2 + strlen(msr->mpd->boundary) + 1) == '-') )
@@ -939,7 +956,7 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
         char c = *inptr;
         int process_buffer = 0;
 
-        if ((c == '\r')&&(msr->mpd->bufleft == 1)) {
+        if ((c == '\r') && (msr->mpd->bufleft == 1)) {
             /* we don't want to take \r as the last byte in the buffer */
             process_buffer = 1;
         } else {
@@ -954,25 +971,26 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
         /* until we either reach the end of the line
          * or the end of our internal buffer
          */
-        if ((c == '\n')||(msr->mpd->bufleft == 0)||(process_buffer)) {
+        if ((c == '\n') || (msr->mpd->bufleft == 0) || (process_buffer)) {
             int processed_as_boundary = 0;
 
             *(msr->mpd->bufptr) = 0;
 
             /* Do we have something that looks like a boundary? */
-            if (msr->mpd->buf_contains_line
+            if (   msr->mpd->buf_contains_line
                 && (strlen(msr->mpd->buf) > 3)
-                && (((*(msr->mpd->buf) == '-'))&&(*(msr->mpd->buf + 1) == '-')) )
+                && (*(msr->mpd->buf) == '-')
+                && (*(msr->mpd->buf + 1) == '-') )
             {
                 /* Does it match our boundary? */
-                if ((strlen(msr->mpd->buf) >= strlen(msr->mpd->boundary) + 2)
+                if (   (strlen(msr->mpd->buf) >= strlen(msr->mpd->boundary) + 2)
                     && (strncmp(msr->mpd->buf + 2, msr->mpd->boundary, strlen(msr->mpd->boundary)) == 0) )
                 {
                     char *boundary_end = msr->mpd->buf + 2 + strlen(msr->mpd->boundary);
                     int is_final = 0;
 
                     /* Is this the final boundary? */
-                    if ((*boundary_end == '-')&&(*(boundary_end + 1)== '-')) {
+                    if ((*boundary_end == '-') && (*(boundary_end + 1)== '-')) {
                         is_final = 1;
                         boundary_end += 2;
 
@@ -1057,7 +1075,8 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
                     char *p = msr->mpd->buf;
 
                     for(i = 0; i < len; i++) {
-                        if ((p[i] == '-')&&(i + 1 < len)&&(p[i + 1] == '-')) {
+                        if ((p[i] == '-') && (i + 1 < len) && (p[i + 1] == '-'))
+                        {
                             if (strncmp(p + i + 2, msr->mpd->boundary, strlen(msr->mpd->boundary)) == 0) {
                                 msr->mpd->flag_unmatched_boundary = 1;
                                 break;
@@ -1077,7 +1096,7 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
                     }
                 } else {
                     if (msr->mpd->mpp_state == 0) {
-                        if ((msr->mpd->bufleft == 0)||(process_buffer)) {
+                        if ((msr->mpd->bufleft == 0) || (process_buffer)) {
                             /* part header lines must be shorter than
                              * MULTIPART_BUF_SIZE bytes
                              */
@@ -1115,7 +1134,7 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
             msr->mpd->buf_contains_line = (c == 0x0a) ? 1 : 0;
         }
 
-        if ((msr->mpd->is_complete)&&(inleft != 0)) {
+        if ((msr->mpd->is_complete) && (inleft != 0)) {
             msr->mpd->flag_data_after = 1;
 
             if (msr->txcfg->debuglog_level >= 4) {
@@ -1189,7 +1208,9 @@ apr_status_t multipart_cleanup(modsec_rec *msr) {
 
         parts = (multipart_part **)msr->mpd->parts->elts;
         for(i = 0; i < msr->mpd->parts->nelts; i++) {
-            if ((parts[i]->type == MULTIPART_FILE)&&(parts[i]->tmp_file_size == 0)) {
+            if (    (parts[i]->type == MULTIPART_FILE)
+                 && (parts[i]->tmp_file_size == 0))
+            {
                 /* Delete empty file. */
                 if (parts[i]->tmp_file_name != NULL) {
                     /* make sure it is closed first */
@@ -1300,7 +1321,7 @@ char *multipart_reconstruct_urlencoded_body_sanitize(modsec_rec *msr) {
 
     /* allocate the buffer */
     body = apr_palloc(msr->mp, body_len + 1);
-    if ((body == NULL)||(body_len + 1 == 0)) return NULL;
+    if ((body == NULL) || (body_len + 1 == 0)) return NULL;
     *body = 0;
 
     parts = (multipart_part **)msr->mpd->parts->elts;

apache2/msc_multipart.h

@@ -117,6 +117,7 @@ struct multipart_data {
     int                      flag_unmatched_boundary;
     int                      flag_boundary_whitespace;
     int                      flag_missing_semicolon;
+    int                      flag_invalid_quoting;
 };
 
 

apache2/msc_reqbody.c

@@ -309,7 +309,7 @@ apr_status_t modsecurity_request_body_store(modsec_rec *msr,
                 msr->msc_reqbody_processor);
             return -1;
         }
-    } else if (msr->txcfg->reqbody_buffering) {
+    } else if (msr->txcfg->reqbody_buffering != REQUEST_BODY_FORCEBUF_OFF) {
         /* Increase per-request data length counter if forcing buffering. */
         msr->msc_reqbody_no_files_length += length;
     }
@@ -479,7 +479,7 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
                 return -1;
             }
         }
-    } else if (msr->txcfg->reqbody_buffering) {
+    } else if (msr->txcfg->reqbody_buffering != REQUEST_BODY_FORCEBUF_OFF) {
         /* Convert to a single continous buffer, but don't do anything else. */
         return modsecurity_request_body_end_raw(msr, error_msg);
     }

apache2/persist_dbm.c

@@ -98,7 +98,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
     apr_status_t rc;
     apr_sdbm_datum_t key;
     apr_sdbm_datum_t *value = NULL;
-    apr_sdbm_t *dbm;
+    apr_sdbm_t *dbm = NULL;
     apr_table_t *col = NULL;
     const apr_array_header_t *arr;
     apr_table_entry_t *te;
@@ -109,7 +109,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
         msr_log(msr, 1, "Unable to retrieve collection (name \"%s\", key \"%s\"). Use "
             "SecDataDir to define data directory first.", log_escape(msr->mp, col_name),
             log_escape_ex(msr->mp, col_key, col_key_len));
-        return NULL;
+        goto cleanup;
     }
 
     dbm_filename = apr_pstrcat(msr->mp, msr->txcfg->data_dir, "/", col_name, NULL);
@@ -121,7 +121,8 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
         rc = apr_sdbm_open(&dbm, dbm_filename, APR_READ | APR_SHARELOCK,
             CREATEMODE, msr->mp);
         if (rc != APR_SUCCESS) {
-            return NULL;
+            dbm = NULL;
+            goto cleanup;
         }
     }
     else {
@@ -133,11 +134,11 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "Failed to read from DBM file \"%s\": %s", log_escape(msr->mp,
             dbm_filename), get_apr_error(msr->mp, rc));
-        return NULL;
+        goto cleanup;
     }
 
     if (value->dptr == NULL) { /* Key not found in DBM file. */
-        return NULL;
+        goto cleanup;
     }
 
     /* ENH Need expiration (and perhaps other metadata) accessible in blob
@@ -147,11 +148,14 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
 
     /* Transform raw data into a table. */
     col = collection_unpack(msr, (const unsigned char *)value->dptr, value->dsize, 1);
-    if (col == NULL) return NULL;
+    if (col == NULL) {
+        goto cleanup;
+    }
 
     /* Close after "value" used from fetch or memory may be overwritten. */
     if (existing_dbm == NULL) {
         apr_sdbm_close(dbm);
+        dbm = NULL;
     }
 
     /* Remove expired variables. */
@@ -198,7 +202,8 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
             if (rc != APR_SUCCESS) {
                 msr_log(msr, 1, "Failed to access DBM file \"%s\": %s",
                     log_escape(msr->mp, dbm_filename), get_apr_error(msr->mp, rc));
-                return NULL;
+                dbm = NULL;
+                goto cleanup;
             }
         }
         else {
@@ -210,12 +215,13 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
             msr_log(msr, 1, "Failed deleting collection (name \"%s\", "
                 "key \"%s\"): %s", log_escape(msr->mp, col_name),
                 log_escape_ex(msr->mp, col_key, col_key_len), get_apr_error(msr->mp, rc));
-            return NULL;
+            goto cleanup;
         }
 
 
         if (existing_dbm == NULL) {
             apr_sdbm_close(dbm);
+            dbm = NULL;
         }
 
         if (expired && (msr->txcfg->debuglog_level >= 9)) {
@@ -225,7 +231,7 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
             msr_log(msr, 4, "Deleted collection (name \"%s\", key \"%s\").",
                 log_escape(msr->mp, col_name), log_escape_ex(msr->mp, col_key, col_key_len));
         }
-        return NULL;
+        goto cleanup;
     }
 
     /* Update UPDATE_RATE */
@@ -270,7 +276,23 @@ static apr_table_t *collection_retrieve_ex(apr_sdbm_t *existing_dbm, modsec_rec
                 log_escape(msr->mp, col_name), log_escape_ex(msr->mp, col_key, col_key_len));
     }
 
+    if ((existing_dbm == NULL) && dbm) {
+        /* Should not ever get here */
+        msr_log(msr, 1, "Internal Error: Collection remained open (name \"%s\", key \"%s\").",
+                log_escape(msr->mp, col_name), log_escape_ex(msr->mp, col_key, col_key_len));
+
+        apr_sdbm_close(dbm);
+    }
+
     return col;
+
+cleanup:
+
+    if ((existing_dbm == NULL) && dbm) {
+        apr_sdbm_close(dbm);
+    }
+
+    return NULL;
 }
 
 /**
@@ -293,7 +315,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     apr_status_t rc;
     apr_sdbm_datum_t key;
     apr_sdbm_datum_t value;
-    apr_sdbm_t *dbm;
+    apr_sdbm_t *dbm = NULL;
     const apr_array_header_t *arr;
     apr_table_entry_t *te;
     int i;
@@ -302,21 +324,22 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
 
     var_name = (msc_string *)apr_table_get(col, "__name");
     if (var_name == NULL) {
-        return -1;
+        goto error;
     }
 
     var_key = (msc_string *)apr_table_get(col, "__key");
     if (var_key == NULL) {
-        return -1;
+        goto error;
     }
 
     if (msr->txcfg->data_dir == NULL) {
         msr_log(msr, 1, "Unable to store collection (name \"%s\", key \"%s\"). Use "
             "SecDataDir to define data directory first.",
             log_escape_ex(msr->mp, var_name->value, var_name->value_len), log_escape_ex(msr->mp, var_key->value, var_key->value_len));
-        return -1;
+        goto error;
     }
 
+    // ENH: lowercase the var name in the filename
     dbm_filename = apr_pstrcat(msr->mp, msr->txcfg->data_dir, "/", var_name->value, NULL);
 
     /* Delete IS_NEW on store. */
@@ -377,16 +400,16 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
             get_apr_error(msr->mp, rc));
-        return -1;
+        dbm = NULL;
+        goto error;
     }
 
-        /* Only need to lock to pull in the stored data again. */
+    /* Need to lock to pull in the stored data again and apply deltas. */
     rc = apr_sdbm_lock(dbm, APR_FLOCK_EXCLUSIVE);
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "Failed to exclusivly lock DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
             get_apr_error(msr->mp, rc));
-            apr_sdbm_close(dbm);
+        goto error;
-            return -1;
     }
 
     /* If there is an original value, then create a delta and
@@ -447,7 +470,7 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     /* Now generate the binary object. */
     blob = apr_pcalloc(msr->mp, blob_size);
     if (blob == NULL) {
-        return -1;
+        goto error;
     }
 
     blob[0] = 0x49;
@@ -490,8 +513,6 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     blob[blob_offset + 1] = 0;
 
     /* And, finally, store it. */
-    dbm_filename = apr_pstrcat(msr->mp, msr->txcfg->data_dir, "/", var_name->value, NULL);
-
     key.dptr = var_key->value;
     key.dsize = var_key->value_len + 1;
 
@@ -502,11 +523,9 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "Failed to write to DBM file \"%s\": %s", dbm_filename,
             get_apr_error(msr->mp, rc));
-        return -1;
+        goto error;
     }
 
-    /* ENH: Do we need to unlock()?  Or will just close() suffice? */
-    apr_sdbm_unlock(dbm);
     apr_sdbm_close(dbm);
 
     if (msr->txcfg->debuglog_level >= 4) {
@@ -515,6 +534,14 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
     }
 
     return 0;
+
+error:
+
+    if (dbm) {
+        apr_sdbm_close(dbm);
+    }
+
+    return -1;
 }
 
 /**
@@ -523,19 +550,19 @@ int collection_store(modsec_rec *msr, apr_table_t *col) {
 int collections_remove_stale(modsec_rec *msr, const char *col_name) {
     char *dbm_filename = NULL;
     apr_sdbm_datum_t key, value;
-    apr_sdbm_t *dbm;
+    apr_sdbm_t *dbm = NULL;
     apr_status_t rc;
     apr_array_header_t *keys_arr;
     char **keys;
-    int i;
     apr_time_t now = apr_time_sec(msr->request_time);
+    int i;
 
     if (msr->txcfg->data_dir == NULL) {
         /* The user has been warned about this problem enough times already by now.
          * msr_log(msr, 1, "Unable to access collection file (name \"%s\"). Use SecDataDir to "
          *     "define data directory first.", log_escape(msr->mp, col_name));
          */
-        return -1;
+        goto error;
     }
 
     dbm_filename = apr_pstrcat(msr->mp, msr->txcfg->data_dir, "/", col_name, NULL);
@@ -545,7 +572,8 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "Failed to access DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
             get_apr_error(msr->mp, rc));
-        return -1;
+        dbm = NULL;
+        goto error;
     }
 
     /* First get a list of all keys. */
@@ -554,8 +582,7 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
     if (rc != APR_SUCCESS) {
         msr_log(msr, 1, "Failed to lock DBM file \"%s\": %s", log_escape(msr->mp, dbm_filename),
             get_apr_error(msr->mp, rc));
-        apr_sdbm_close(dbm);
+        goto error;
-        return -1;
     }
 
     /* No one can write to the file while doing this so
@@ -584,8 +611,7 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
         if (rc != APR_SUCCESS) {
             msr_log(msr, 1, "Failed reading DBM file \"%s\": %s",
                 log_escape(msr->mp, dbm_filename), get_apr_error(msr->mp, rc));
-            apr_sdbm_close(dbm);
+            goto error;
-            return -1;
         }
 
         if (value.dptr != NULL) {
@@ -594,8 +620,7 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
 
             col = collection_unpack(msr, (const unsigned char *)value.dptr, value.dsize, 0);
             if (col == NULL) {
-                apr_sdbm_close(dbm);
+                goto error;
-                return -1;
             }
 
             var = (msc_string *)apr_table_get(col, "__expire_KEY");
@@ -618,8 +643,7 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
                         msr_log(msr, 1, "Failed deleting collection (name \"%s\", "
                             "key \"%s\"): %s", log_escape(msr->mp, col_name),
                             log_escape_ex(msr->mp, key.dptr, key.dsize - 1), get_apr_error(msr->mp, rc));
-                        apr_sdbm_close(dbm);
+                        goto error;
-                        return -1;
                     }
                     if (msr->txcfg->debuglog_level >= 4) {
                         msr_log(msr, 4, "Removed stale collection (name \"%s\", "
@@ -636,4 +660,12 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
     apr_sdbm_close(dbm);
 
     return 1;
+
+error:
+
+    if (dbm) {
+        apr_sdbm_close(dbm);
+    }
+
+    return -1;
 }

apache2/re_actions.c

@@ -714,11 +714,10 @@ static char *msre_action_ctl_validate(msre_engine *engine, msre_action *action)
         return NULL;
     } else
     if (strcasecmp(name, "forceRequestBodyVariable") == 0) {
-        if (parse_boolean(value) == -1) {
+        if (strcasecmp(value, "on") == 0) return NULL;
+        if (strcasecmp(value, "off") == 0) return NULL;
         return apr_psprintf(engine->mp, "Invalid setting for ctl name "
             " forceRequestBodyVariable: %s", value);
-        }
-        return NULL;
     } else
     if (strcasecmp(name, "responseBodyAccess") == 0) {
         if (parse_boolean(value) == -1) {
@@ -839,12 +838,17 @@ static apr_status_t msre_action_ctl_execute(modsec_rec *msr, apr_pool_t *mptmp,
         return 1;
     } else
     if (strcasecmp(name, "forceRequestBodyVariable") == 0) {
-        int pv = parse_boolean(value);
+        if (strcasecmp(value, "on") == 0) {
+            msr->txcfg->reqbody_buffering = REQUEST_BODY_FORCEBUF_ON;
+            msr->usercfg->reqbody_buffering = REQUEST_BODY_FORCEBUF_ON;
+        }
+        else
+        if (strcasecmp(value, "off") == 0) {
+            msr->txcfg->reqbody_buffering = REQUEST_BODY_FORCEBUF_OFF;
+            msr->usercfg->reqbody_buffering = REQUEST_BODY_FORCEBUF_OFF;
+        }
 
-        if (pv == -1) return -1;
+        msr_log(msr, 4, "Ctl: Set requestBodyAccess to %d.", msr->txcfg->reqbody_buffering);
-        msr->txcfg->reqbody_buffering = pv;
-        msr->usercfg->reqbody_buffering = pv;
-        msr_log(msr, 4, "Ctl: Set requestBodyAccess to %d.", pv);
 
         return 1;
     } else
@@ -880,7 +884,7 @@ static apr_status_t msre_action_ctl_execute(modsec_rec *msr, apr_pool_t *mptmp,
             msr->usercfg->auditlog_flag = AUDITLOG_RELEVANT;
         }
 
-        msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO
+        msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag);
 
         return 1;
     } else

apache2/re_variables.c

@@ -1366,6 +1366,18 @@ static int var_multipart_missing_semicolon_generate(modsec_rec *msr, msre_var *v
     }
 }
 
+/* MULTIPART_INVALID_QUOTING */
+
+static int var_multipart_invalid_quoting_generate(modsec_rec *msr, msre_var *var, msre_rule *rule,
+    apr_table_t *vartab, apr_pool_t *mptmp)
+{
+    if ((msr->mpd != NULL)&&(msr->mpd->flag_invalid_quoting != 0)) {
+        return var_simple_generate(var, vartab, mptmp, "1");
+    } else {
+        return var_simple_generate(var, vartab, mptmp, "0");
+    }
+}
+
 /* MULTIPART_STRICT_ERROR */
 
 static int var_multipart_strict_error_generate(modsec_rec *msr, msre_var *var, msre_rule *rule,
@@ -1381,6 +1393,7 @@ static int var_multipart_strict_error_generate(modsec_rec *msr, msre_var *var, m
             ||(msr->mpd->flag_header_folding != 0)
             ||(msr->mpd->flag_lf_line != 0)
             ||(msr->mpd->flag_missing_semicolon != 0)
+            ||(msr->mpd->flag_invalid_quoting != 0)
         ) {
             return var_simple_generate(var, vartab, mptmp, "1");
         }
@@ -2454,6 +2467,17 @@ void msre_engine_register_default_variables(msre_engine *engine) {
         PHASE_REQUEST_BODY
     );
 
+    /* MULTIPART_INVALID_QUOTING */
+    msre_engine_variable_register(engine,
+        "MULTIPART_INVALID_QUOTING",
+        VAR_SIMPLE,
+        0, 0,
+        NULL,
+        var_multipart_invalid_quoting_generate,
+        VAR_DONT_CACHE, /* flag */
+        PHASE_REQUEST_BODY
+    );
+
     /* MULTIPART_STRICT_ERROR */
     msre_engine_variable_register(engine,
         "MULTIPART_STRICT_ERROR",

apache2/t/regression/misc/00-multipart-parser.t

@@ -404,3 +404,82 @@
         ),
     ),
 },
+# Data following final boundary should set flag
+{
+    type => "misc",
+    comment => "multipart parser (data after final boundary)",
+    conf => qq(
+        SecRuleEngine On
+        SecDebugLog $ENV{DEBUG_LOG}
+        SecDebugLogLevel 9
+        SecRequestBodyAccess On
+        SecRule MULTIPART_DATA_AFTER "\@eq 1" "phase:2,deny,status:403"
+    ),
+    match_log => {
+        debug => [ qr/name: a.*variable: 1.*Ignoring data after last boundary/s, 1 ],
+        -debug => [ qr/Adding request argument \(BODY\): name "b"/s, 1 ],
+    },
+    match_response => {
+        status => qr/^403$/,
+    },
+    request => new HTTP::Request(
+        POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
+        [
+            "Content-Type" => "multipart/form-data; boundary=---------------------------69343412719991675451336310646",
+        ],
+        normalize_raw_request_data(
+            q(
+                -----------------------------69343412719991675451336310646
+                Content-Disposition: form-data; name="a"
+                
+                1
+                -----------------------------69343412719991675451336310646--
+                -----------------------------69343412719991675451336310646
+                Content-Disposition: form-data; name="b"
+                
+                2
+                -----------------------------69343412719991675451336310646--
+            ),
+        ),
+    ),
+},
+# Single quoted data is invalid
+{
+    type => "misc",
+    comment => "multipart parser (C-D uses single quotes)",
+    conf => qq(
+        SecRuleEngine On
+        SecDebugLog $ENV{DEBUG_LOG}
+        SecDebugLogLevel 9
+        SecRequestBodyAccess On
+        #SecRule MULTIPART_STRICT_ERROR "\@eq 1" "phase:2,deny,status:403"
+        SecRule MULTIPART_INVALID_QUOTING "\@eq 1" "chain,phase:2,deny,status:403"
+        SecRule REQBODY_PROCESSOR_ERROR "\@eq 1"
+    ),
+    match_log => {
+        debug => [ qr/name: a.*variable: 1.*Duplicate Content-Disposition name/s, 1 ],
+        -debug => [ qr/Adding request argument \(BODY\): name "b/s, 1 ],
+    },
+    match_response => {
+        status => qr/^403$/,
+    },
+    request => new HTTP::Request(
+        POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
+        [
+            "Content-Type" => "multipart/form-data; boundary=---------------------------69343412719991675451336310646",
+        ],
+        normalize_raw_request_data(
+            q(
+                -----------------------------69343412719991675451336310646
+                Content-Disposition: form-data; name="a"
+                
+                1
+                -----------------------------69343412719991675451336310646
+                Content-Disposition: form-data; name=';filename="dummy';name=b;"
+                
+                2
+                -----------------------------69343412719991675451336310646--
+            ),
+        ),
+    ),
+},

doc/modsecurity2-apache-reference.xml

@@ -6,7 +6,7 @@
   Manual</title>
 
   <articleinfo>
-    <releaseinfo>Version 2.6.0-trunk (Sep 18, 2009)</releaseinfo>
+    <releaseinfo>Version 2.6.0-trunk (Nov 4, 2009)</releaseinfo>
 
     <copyright>
       <year>2004-2009</year>
@@ -308,6 +308,12 @@
 
         <para><ulink type=""
         url="http://curl.haxx.se/libcurl/">http://curl.haxx.se/libcurl/</ulink></para>
+        <note>
+          <para>Many have had issues with libcurl linked with the GnuTLS
+          library for SSL/TLS support.  It is recommended that the
+          openssl library be used for SSL/TLS support in libcurl.</para>
+        </note>
+
       </listitem>
     </orderedlist>
 
@@ -3111,7 +3117,8 @@ SecRule ARGS "@pm some key words" id:12345,deny,status:500</programlisting>
       <literal>MULTIPART_DATA_AFTER</literal>,
       <literal>MULTIPART_HEADER_FOLDING</literal>,
       <literal>MULTIPART_LF_LINE</literal>,
-      <literal>MULTIPART_SEMICOLON_MISSING</literal>. Each of these variables
+      <literal>MULTIPART_SEMICOLON_MISSING</literal>
+      <literal>MULTIPART_INVALID_QUOTING</literal>. Each of these variables
       covers one unusual (although sometimes legal) aspect of the request body
       in <literal>multipart/form-data format</literal>. Your policies should
       <emphasis>always</emphasis> contain a rule to check either this variable
@@ -3133,7 +3140,8 @@ DB %{MULTIPART_DATA_BEFORE}, \
 DA %{MULTIPART_DATA_AFTER}, \
 HF %{MULTIPART_HEADER_FOLDING}, \
 LF %{MULTIPART_LF_LINE}, \
-SM %{MULTIPART_SEMICOLON_MISSING}'"</programlisting>
+SM %{MULTIPART_SEMICOLON_MISSING}, \
+IQ %{MULTIPART_INVALID_QUOTING}'"</programlisting>
 
       <para>The <literal>multipart/form-data</literal> parser was upgraded in
       ModSecurity v2.1.3 to actively look for signs of evasion. Many variables

modsecurity.conf-minimal

@@ -52,7 +52,8 @@ DB %{MULTIPART_DATA_BEFORE}, \
 DA %{MULTIPART_DATA_AFTER}, \
 HF %{MULTIPART_HEADER_FOLDING}, \
 LF %{MULTIPART_LF_LINE}, \
-SM %{MULTIPART_SEMICOLON_MISSING}'"
+SM %{MULTIPART_SEMICOLON_MISSING}, \
+IQ %{MULTIPART_INVALID_QUOTING}'"
 
 # Did we see anything that might be a boundary?
 SecRule MULTIPART_UNMATCHED_BOUNDARY "!@eq 0" \