Merge pull request #2797 from martinhsv/v2/master

Multipart parsing fixes and new MULTIPART_PART_HEADERS collection

Author: martinhsv

CHANGES

@@ -1,6 +1,8 @@
 DD mmm YYYY - 2.9.x (to be released)
 -------------------
 
+ * Multipart parsing fixes and new MULTIPART_PART_HEADERS collection
+   [Issue #2797 - @terjanq, @martinhsv]
  * Limit rsub null termination to where necessary
    [Issue #2794 - @marcstern, @martinhsv]
  * IIS: Update dependencies for next planned release

apache2/msc_multipart.c

@@ -325,7 +325,14 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) {
         }
 
         msr->mpd->mpp_state = 1;
+        msr->mpd->mpp_substate_part_data_read = 0;
         msr->mpd->mpp->last_header_name = NULL;
+
+        /* Record the last part header line in the collection */
+        if (msr->mpd->mpp->last_header_line != NULL) {
+            *(char **)apr_array_push(msr->mpd->mpp->header_lines) = msr->mpd->mpp->last_header_line;
+            msr_log(msr, 9, "Multipart: Added part header line \"%s\"", msr->mpd->mpp->last_header_line);
+        }
     } else {
         /* Header line. */
 
@@ -379,12 +386,28 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) {
                 *error_msg = apr_psprintf(msr->mp, "Multipart: Part header too long.");
                 return -1;
             }
+            if ((msr->mpd->mpp->last_header_line != NULL) && (msr->mpd->mpp->last_header_name != NULL)
+                && (new_value != NULL)) {
+                msr->mpd->mpp->last_header_line = apr_psprintf(msr->mp,
+                    "%s: %s", msr->mpd->mpp->last_header_name, new_value);
+            }
+
         } else {
             char *header_name, *header_value, *data;
 
             /* new header */
 
+            /* Record the most recently-seen part header line in the collection */
+            if (msr->mpd->mpp->last_header_line != NULL) {
+                *(char **)apr_array_push(msr->mpd->mpp->header_lines) = msr->mpd->mpp->last_header_line;
+                msr_log(msr, 9, "Multipart: Added part header line \"%s\"", msr->mpd->mpp->last_header_line);
+	    }
+
             data = msr->mpd->buf;
+
+            msr->mpd->mpp->last_header_line = apr_pstrdup(msr->mp, data);
+            remove_lf_crlf_inplace(msr->mpd->mpp->last_header_line);
+
             while((*data != ':') && (*data != '\0')) data++;
             if (*data == '\0') {
                 *error_msg = apr_psprintf(msr->mp, "Multipart: Invalid part header (colon missing): %s.",
@@ -438,6 +461,8 @@ static int multipart_process_part_data(modsec_rec *msr, char **error_msg) {
     if (error_msg == NULL) return -1;
     *error_msg = NULL;
 
+    msr->mpd->mpp_substate_part_data_read = 1;
+
     /* Preserve some bytes for later. */
     if (   ((MULTIPART_BUF_SIZE - msr->mpd->bufleft) >= 1)
         && (*(p - 1) == '\n') )
@@ -680,10 +705,14 @@ static int multipart_process_boundary(modsec_rec *msr, int last_part, char **err
         if (msr->mpd->mpp == NULL) return -1;
         msr->mpd->mpp->type = MULTIPART_FORMDATA;
         msr->mpd->mpp_state = 0;
+        msr->mpd->mpp_substate_part_data_read = 0;
 
         msr->mpd->mpp->headers = apr_table_make(msr->mp, 10);
         if (msr->mpd->mpp->headers == NULL) return -1;
         msr->mpd->mpp->last_header_name = NULL;
+        msr->mpd->mpp->last_header_line = NULL;
+        msr->mpd->mpp->header_lines = apr_array_make(msr->mp, 2, sizeof(char *));
+        if (msr->mpd->mpp->header_lines == NULL) return -1;
 
         msr->mpd->reserve[0] = 0;
         msr->mpd->reserve[1] = 0;
@@ -983,6 +1012,19 @@ int multipart_complete(modsec_rec *msr, char **error_msg) {
                     && (*(msr->mpd->buf + 2 + strlen(msr->mpd->boundary)) == '-')
                     && (*(msr->mpd->buf + 2 + strlen(msr->mpd->boundary) + 1) == '-') )
                 {
+                    if ((msr->mpd->crlf_state_buf_end == 2) && (msr->mpd->flag_lf_line != 1)) {
+                        msr->mpd->flag_lf_line = 1;
+                        if (msr->mpd->flag_crlf_line) {
+                            msr_log(msr, 4, "Multipart: Warning: mixed line endings used (CRLF/LF).");
+                        } else {
+                            msr_log(msr, 4, "Multipart: Warning: incorrect line endings used (LF).");
+                        }
+                    }
+                    if (msr->mpd->mpp_substate_part_data_read == 0) {
+                        /* it looks like the final boundary, but it's where part data should begin */
+                        msr->mpd->flag_invalid_part = 1;
+                        msr_log(msr, 4, "Multipart: Warning: Invalid part (data contains final boundary)");
+                    }
                     /* Looks like the final boundary - process it. */
                     if (multipart_process_boundary(msr, 1 /* final */, error_msg) < 0) {
                         msr->mpd->flag_error = 1;
@@ -1075,54 +1117,63 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
                 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;
+                    if (msr->mpd->crlf_state_buf_end == 2) {
+                        msr->mpd->flag_lf_line = 1;
+                    }
+                    if ((msr->mpd->mpp_substate_part_data_read == 0) && (msr->mpd->boundary_count > 0)) {
+                        /* string matches our boundary, but it's where part data should begin */
+                        msr->mpd->flag_invalid_part = 1;
+                        msr_log(msr, 4, "Multipart: Warning: Invalid part (data contains boundary)");
+                    } else {
+                        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)== '-')) {
+                            is_final = 1;
+                            boundary_end += 2;
+
+                            if (msr->mpd->is_complete != 0) {
+                                msr->mpd->flag_error = 1;
+                                *error_msg = apr_psprintf(msr->mp,
+                                    "Multipart: Invalid boundary (final duplicate).");
+                                return -1;
+                            }
+                        }
 
-                    /* Is this the final boundary? */
-                    if ((*boundary_end == '-') && (*(boundary_end + 1)== '-')) {
-                        is_final = 1;
-                        boundary_end += 2;
+                        /* Allow for CRLF and LF line endings. */
+                        if (   ( (*boundary_end == '\r')
+                                  && (*(boundary_end + 1) == '\n')
+                                  && (*(boundary_end + 2) == '\0') )
+                            || ( (*boundary_end == '\n')
+                                  && (*(boundary_end + 1) == '\0') ) )
+                        {
+                            if (*boundary_end == '\n') {
+                                msr->mpd->flag_lf_line = 1;
+                            } else {
+                                msr->mpd->flag_crlf_line = 1;
+                            }
 
-                        if (msr->mpd->is_complete != 0) {
-                            msr->mpd->flag_error = 1;
-                            *error_msg = apr_psprintf(msr->mp,
-                                "Multipart: Invalid boundary (final duplicate).");
-                            return -1;
-                        }
-                    }
+                            if (multipart_process_boundary(msr, (is_final ? 1 : 0), error_msg) < 0) {
+                                msr->mpd->flag_error = 1;
+                                return -1;
+                            }
 
-                    /* Allow for CRLF and LF line endings. */
-                    if (   ( (*boundary_end == '\r')
-                              && (*(boundary_end + 1) == '\n')
-                              && (*(boundary_end + 2) == '\0') )
-                        || ( (*boundary_end == '\n')
-                              && (*(boundary_end + 1) == '\0') ) )
-                    {
-                        if (*boundary_end == '\n') {
-                            msr->mpd->flag_lf_line = 1;
-                        } else {
-                            msr->mpd->flag_crlf_line = 1;
-                        }
+                            if (is_final) {
+                                msr->mpd->is_complete = 1;
+                            }
 
-                        if (multipart_process_boundary(msr, (is_final ? 1 : 0), error_msg) < 0) {
+                            processed_as_boundary = 1;
+                            msr->mpd->boundary_count++;
+                        }
+                        else {
+                            /* error */
                             msr->mpd->flag_error = 1;
+                            *error_msg = apr_psprintf(msr->mp,
+                                "Multipart: Invalid boundary: %s",
+                                log_escape_nq(msr->mp, msr->mpd->buf));
                             return -1;
                         }
-
-                        if (is_final) {
-                            msr->mpd->is_complete = 1;
-                        }
-
-                        processed_as_boundary = 1;
-                        msr->mpd->boundary_count++;
-                    }
-                    else {
-                        /* error */
-                        msr->mpd->flag_error = 1;
-                        *error_msg = apr_psprintf(msr->mp,
-                            "Multipart: Invalid boundary: %s",
-                            log_escape_nq(msr->mp, msr->mpd->buf));
-                        return -1;
                     }
                 } else { /* It looks like a boundary but we couldn't match it. */
                     char *p = NULL;
@@ -1221,6 +1272,21 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
             msr->mpd->bufptr = msr->mpd->buf;
             msr->mpd->bufleft = MULTIPART_BUF_SIZE;
             msr->mpd->buf_contains_line = (c == 0x0a) ? 1 : 0;
+
+            if (c == 0x0a) {
+                if (msr->mpd->crlf_state == 1) {
+                    msr->mpd->crlf_state = 3;
+	        } else {
+                    msr->mpd->crlf_state = 2;
+	        }
+	    }
+            msr->mpd->crlf_state_buf_end = msr->mpd->crlf_state;
+        }
+
+        if (c == 0x0d) {
+            msr->mpd->crlf_state = 1;
+        } else if (c != 0x0a) {
+            msr->mpd->crlf_state = 0;
         }
 
         if ((msr->mpd->is_complete) && (inleft != 0)) {

apache2/msc_multipart.h

@@ -55,6 +55,8 @@ struct multipart_part {
 
     char                    *last_header_name;
     apr_table_t             *headers;
+    char                    *last_header_line;
+    apr_array_header_t      *header_lines;
 
     unsigned int             offset;
     unsigned int             length;
@@ -81,6 +83,15 @@ struct multipart_data {
     char                    *bufptr;
     int                      bufleft;
 
+    /* line ending status seen immediately before current position.
+     * 0 = neither LF nor CR; 1 = prev char CR; 2 = prev char LF alone;
+     * 3 = prev two chars were CRLF
+     */
+    int                      crlf_state;
+
+    /* crlf_state at end of previous buffer */
+    int                      crlf_state_buf_end;
+
     unsigned int             buf_offset;
 
     /* pointer that keeps track of a part while
@@ -94,6 +105,14 @@ struct multipart_data {
      */
     int                      mpp_state;
 
+    /* part parsing substate;  if mpp_state is 1 (collecting
+     * data), then for this variable:
+     * 0 means we have not yet read any data between the
+     * post-headers blank line and the next boundary
+     * 1 means we have read at some data after that blank line
+     */
+    int                      mpp_substate_part_data_read;
+
     /* because of the way this parsing algorithm
      * works we hold back the last two bytes of
      * each data chunk so that we can discard it

apache2/re_variables.c

@@ -1394,6 +1394,52 @@ static int var_files_combined_size_generate(modsec_rec *msr, msre_var *var, msre
     return 1;
 }
 
+/* MULTIPART_PART_HEADERS */
+
+static int var_multipart_part_headers_generate(modsec_rec *msr, msre_var *var, msre_rule *rule,
+    apr_table_t *vartab, apr_pool_t *mptmp)
+{
+    multipart_part **parts = NULL;
+    int i, j, count = 0;
+
+    if (msr->mpd == NULL) return 0;
+
+    parts = (multipart_part **)msr->mpd->parts->elts;
+    for(i = 0; i < msr->mpd->parts->nelts; i++) {
+        int match = 0;
+
+        /* Figure out if we want to include this variable. */
+        if (var->param == NULL) match = 1;
+        else {
+            if (var->param_data != NULL) { /* Regex. */
+                char *my_error_msg = NULL;
+                if (!(msc_regexec((msc_regex_t *)var->param_data, parts[i]->name,
+                    strlen(parts[i]->name), &my_error_msg) == PCRE_ERROR_NOMATCH)) match = 1;
+            } else { /* Simple comparison. */
+                if (strcasecmp(parts[i]->name, var->param) == 0) match = 1;
+            }
+        }
+
+        /* If we had a match add this argument to the collection. */
+        if (match) {
+            for (j = 0; j < parts[i]->header_lines->nelts; j++) {
+                char *header_line = ((char **)parts[i]->header_lines->elts)[j];
+                msre_var *rvar = apr_pmemdup(mptmp, var, sizeof(msre_var));
+
+                rvar->value = header_line;
+                rvar->value_len = strlen(rvar->value);
+                rvar->name = apr_psprintf(mptmp, "MULTIPART_PART_HEADERS:%s",
+                    log_escape_nq(mptmp, parts[i]->name));
+                apr_table_addn(vartab, rvar->name, (void *)rvar);
+
+                count++;
+	    }
+        }
+    }
+
+    return count;
+}
+
 /* MODSEC_BUILD */
 
 static int var_modsec_build_generate(modsec_rec *msr, msre_var *var, msre_rule *rule,
@@ -2966,6 +3012,17 @@ void msre_engine_register_default_variables(msre_engine *engine) {
         PHASE_REQUEST_BODY
     );
 
+    /* MULTIPART_PART_HEADERS */
+    msre_engine_variable_register(engine,
+        "MULTIPART_PART_HEADERS",
+        VAR_LIST,
+        0, 1,
+        var_generic_list_validate,
+        var_multipart_part_headers_generate,
+        VAR_CACHE,
+        PHASE_REQUEST_BODY
+    );
+
     /* GEO */
     msre_engine_variable_register(engine,
         "GEO",

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

@@ -1849,3 +1849,48 @@
     ),
 },
 
+# part headers
+{
+    type => "misc",
+    comment => "multipart parser (part headers)",
+    conf => qq(
+        SecRuleEngine On
+        SecDebugLog $ENV{DEBUG_LOG}
+        SecDebugLogLevel 9
+        SecRequestBodyAccess On
+        SecRule MULTIPART_STRICT_ERROR "\@eq 1" "phase:2,deny,status:400,id:500168"
+        SecRule REQBODY_PROCESSOR_ERROR "\@eq 1" "phase:2,deny,status:400,id:500169"
+        SecRule MULTIPART_PART_HEADERS:image "\@rx content-type:.*jpeg" "phase:2,deny,status:403,id:500170,t:lowercase"
+    ),
+    match_log => {
+        debug => [ qr/500170.*against MULTIPART_PART_HEADERS:image.*Rule returned 1./s, 1 ],
+    },
+    match_response => {
+        status => qr/^403$/,
+    },
+    request => new HTTP::Request(
+        POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
+        [
+            "Content-Type" => q(multipart/form-data; boundary=0000),
+        ],
+        normalize_raw_request_data(
+            q(
+                --0000
+                Content-Disposition: form-data; name="username"
+                
+                Bill
+                --0000
+                Content-Disposition: form-data; name="email"
+                
+                bill@fakesite.com
+                --0000
+                Content-Disposition: form-data; name="image"; filename="image.jpg"
+                Content-Type: image/jpeg
+                
+                BINARYDATA
+                --0000--
+            ),
+        ),
+    ),
+},
+