Skip to content

Commit 7a489bd

Browse files
martinhsvmartinhsv
committed
Multipart parsing fixes and new MULTIPART_PART_HEADERS collection
1 parent e0ff7ed commit 7a489bd

File tree

5 files changed

+230
-41
lines changed

5 files changed

+230
-41
lines changed

‎CHANGES‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
DD mmm YYYY - 2.9.x (to be released)
22
-------------------
33

4+
* Multipart parsing fixes and new MULTIPART_PART_HEADERS collection
5+
[Issue #2797 - @terjanq, @martinhsv]
46
* Limit rsub null termination to where necessary
57
[Issue #2794 - @marcstern, @martinhsv]
68
* IIS: Update dependencies for next planned release

‎apache2/msc_multipart.c‎

Lines changed: 107 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,14 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) {
325325
}
326326

327327
msr->mpd->mpp_state = 1;
328+
msr->mpd->mpp_substate_part_data_read = 0;
328329
msr->mpd->mpp->last_header_name = NULL;
330+
331+
/* Record the last part header line in the collection */
332+
if (msr->mpd->mpp->last_header_line != NULL) {
333+
*(char **)apr_array_push(msr->mpd->mpp->header_lines) = msr->mpd->mpp->last_header_line;
334+
msr_log(msr, 9, "Multipart: Added part header line \"%s\"", msr->mpd->mpp->last_header_line);
335+
}
329336
} else {
330337
/* Header line. */
331338

@@ -379,12 +386,28 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) {
379386
*error_msg = apr_psprintf(msr->mp, "Multipart: Part header too long.");
380387
return -1;
381388
}
389+
if ((msr->mpd->mpp->last_header_line != NULL) && (msr->mpd->mpp->last_header_name != NULL)
390+
&& (new_value != NULL)) {
391+
msr->mpd->mpp->last_header_line = apr_psprintf(msr->mp,
392+
"%s: %s", msr->mpd->mpp->last_header_name, new_value);
393+
}
394+
382395
} else {
383396
char *header_name, *header_value, *data;
384397

385398
/* new header */
386399

400+
/* Record the most recently-seen part header line in the collection */
401+
if (msr->mpd->mpp->last_header_line != NULL) {
402+
*(char **)apr_array_push(msr->mpd->mpp->header_lines) = msr->mpd->mpp->last_header_line;
403+
msr_log(msr, 9, "Multipart: Added part header line \"%s\"", msr->mpd->mpp->last_header_line);
404+
}
405+
387406
data = msr->mpd->buf;
407+
408+
msr->mpd->mpp->last_header_line = apr_pstrdup(msr->mp, data);
409+
remove_lf_crlf_inplace(msr->mpd->mpp->last_header_line);
410+
388411
while((*data != ':') && (*data != '\0')) data++;
389412
if (*data == '\0') {
390413
*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) {
438461
if (error_msg == NULL) return -1;
439462
*error_msg = NULL;
440463

464+
msr->mpd->mpp_substate_part_data_read = 1;
465+
441466
/* Preserve some bytes for later. */
442467
if ( ((MULTIPART_BUF_SIZE - msr->mpd->bufleft) >= 1)
443468
&& (*(p - 1) == '\n') )
@@ -680,10 +705,14 @@ static int multipart_process_boundary(modsec_rec *msr, int last_part, char **err
680705
if (msr->mpd->mpp == NULL) return -1;
681706
msr->mpd->mpp->type = MULTIPART_FORMDATA;
682707
msr->mpd->mpp_state = 0;
708+
msr->mpd->mpp_substate_part_data_read = 0;
683709

684710
msr->mpd->mpp->headers = apr_table_make(msr->mp, 10);
685711
if (msr->mpd->mpp->headers == NULL) return -1;
686712
msr->mpd->mpp->last_header_name = NULL;
713+
msr->mpd->mpp->last_header_line = NULL;
714+
msr->mpd->mpp->header_lines = apr_array_make(msr->mp, 2, sizeof(char *));
715+
if (msr->mpd->mpp->header_lines == NULL) return -1;
687716

688717
msr->mpd->reserve[0] = 0;
689718
msr->mpd->reserve[1] = 0;
@@ -983,6 +1012,19 @@ int multipart_complete(modsec_rec *msr, char **error_msg) {
9831012
&& (*(msr->mpd->buf + 2 + strlen(msr->mpd->boundary)) == '-')
9841013
&& (*(msr->mpd->buf + 2 + strlen(msr->mpd->boundary) + 1) == '-') )
9851014
{
1015+
if ((msr->mpd->crlf_state_buf_end == 2) && (msr->mpd->flag_lf_line != 1)) {
1016+
msr->mpd->flag_lf_line = 1;
1017+
if (msr->mpd->flag_crlf_line) {
1018+
msr_log(msr, 4, "Multipart: Warning: mixed line endings used (CRLF/LF).");
1019+
} else {
1020+
msr_log(msr, 4, "Multipart: Warning: incorrect line endings used (LF).");
1021+
}
1022+
}
1023+
if (msr->mpd->mpp_substate_part_data_read == 0) {
1024+
/* it looks like the final boundary, but it's where part data should begin */
1025+
msr->mpd->flag_invalid_part = 1;
1026+
msr_log(msr, 4, "Multipart: Warning: Invalid part (data contains final boundary)");
1027+
}
9861028
/* Looks like the final boundary - process it. */
9871029
if (multipart_process_boundary(msr, 1 /* final */, error_msg) < 0) {
9881030
msr->mpd->flag_error = 1;
@@ -1075,54 +1117,63 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
10751117
if ( (strlen(msr->mpd->buf) >= strlen(msr->mpd->boundary) + 2)
10761118
&& (strncmp(msr->mpd->buf + 2, msr->mpd->boundary, strlen(msr->mpd->boundary)) == 0) )
10771119
{
1078-
char *boundary_end = msr->mpd->buf + 2 + strlen(msr->mpd->boundary);
1079-
int is_final = 0;
1120+
if (msr->mpd->crlf_state_buf_end == 2) {
1121+
msr->mpd->flag_lf_line = 1;
1122+
}
1123+
if ((msr->mpd->mpp_substate_part_data_read == 0) && (msr->mpd->boundary_count > 0)) {
1124+
/* string matches our boundary, but it's where part data should begin */
1125+
msr->mpd->flag_invalid_part = 1;
1126+
msr_log(msr, 4, "Multipart: Warning: Invalid part (data contains boundary)");
1127+
} else {
1128+
char *boundary_end = msr->mpd->buf + 2 + strlen(msr->mpd->boundary);
1129+
int is_final = 0;
1130+
1131+
/* Is this the final boundary? */
1132+
if ((*boundary_end == '-') && (*(boundary_end + 1)== '-')) {
1133+
is_final = 1;
1134+
boundary_end += 2;
1135+
1136+
if (msr->mpd->is_complete != 0) {
1137+
msr->mpd->flag_error = 1;
1138+
*error_msg = apr_psprintf(msr->mp,
1139+
"Multipart: Invalid boundary (final duplicate).");
1140+
return -1;
1141+
}
1142+
}
10801143

1081-
/* Is this the final boundary? */
1082-
if ((*boundary_end == '-') && (*(boundary_end + 1)== '-')) {
1083-
is_final = 1;
1084-
boundary_end += 2;
1144+
/* Allow for CRLF and LF line endings. */
1145+
if ( ( (*boundary_end == '\r')
1146+
&& (*(boundary_end + 1) == '\n')
1147+
&& (*(boundary_end + 2) == '\0') )
1148+
|| ( (*boundary_end == '\n')
1149+
&& (*(boundary_end + 1) == '\0') ) )
1150+
{
1151+
if (*boundary_end == '\n') {
1152+
msr->mpd->flag_lf_line = 1;
1153+
} else {
1154+
msr->mpd->flag_crlf_line = 1;
1155+
}
10851156

1086-
if (msr->mpd->is_complete != 0) {
1087-
msr->mpd->flag_error = 1;
1088-
*error_msg = apr_psprintf(msr->mp,
1089-
"Multipart: Invalid boundary (final duplicate).");
1090-
return -1;
1091-
}
1092-
}
1157+
if (multipart_process_boundary(msr, (is_final ? 1 : 0), error_msg) < 0) {
1158+
msr->mpd->flag_error = 1;
1159+
return -1;
1160+
}
10931161

1094-
/* Allow for CRLF and LF line endings. */
1095-
if ( ( (*boundary_end == '\r')
1096-
&& (*(boundary_end + 1) == '\n')
1097-
&& (*(boundary_end + 2) == '\0') )
1098-
|| ( (*boundary_end == '\n')
1099-
&& (*(boundary_end + 1) == '\0') ) )
1100-
{
1101-
if (*boundary_end == '\n') {
1102-
msr->mpd->flag_lf_line = 1;
1103-
} else {
1104-
msr->mpd->flag_crlf_line = 1;
1105-
}
1162+
if (is_final) {
1163+
msr->mpd->is_complete = 1;
1164+
}
11061165

1107-
if (multipart_process_boundary(msr, (is_final ? 1 : 0), error_msg) < 0) {
1166+
processed_as_boundary = 1;
1167+
msr->mpd->boundary_count++;
1168+
}
1169+
else {
1170+
/* error */
11081171
msr->mpd->flag_error = 1;
1172+
*error_msg = apr_psprintf(msr->mp,
1173+
"Multipart: Invalid boundary: %s",
1174+
log_escape_nq(msr->mp, msr->mpd->buf));
11091175
return -1;
11101176
}
1111-
1112-
if (is_final) {
1113-
msr->mpd->is_complete = 1;
1114-
}
1115-
1116-
processed_as_boundary = 1;
1117-
msr->mpd->boundary_count++;
1118-
}
1119-
else {
1120-
/* error */
1121-
msr->mpd->flag_error = 1;
1122-
*error_msg = apr_psprintf(msr->mp,
1123-
"Multipart: Invalid boundary: %s",
1124-
log_escape_nq(msr->mp, msr->mpd->buf));
1125-
return -1;
11261177
}
11271178
} else { /* It looks like a boundary but we couldn't match it. */
11281179
char *p = NULL;
@@ -1221,6 +1272,21 @@ int multipart_process_chunk(modsec_rec *msr, const char *buf,
12211272
msr->mpd->bufptr = msr->mpd->buf;
12221273
msr->mpd->bufleft = MULTIPART_BUF_SIZE;
12231274
msr->mpd->buf_contains_line = (c == 0x0a) ? 1 : 0;
1275+
1276+
if (c == 0x0a) {
1277+
if (msr->mpd->crlf_state == 1) {
1278+
msr->mpd->crlf_state = 3;
1279+
} else {
1280+
msr->mpd->crlf_state = 2;
1281+
}
1282+
}
1283+
msr->mpd->crlf_state_buf_end = msr->mpd->crlf_state;
1284+
}
1285+
1286+
if (c == 0x0d) {
1287+
msr->mpd->crlf_state = 1;
1288+
} else if (c != 0x0a) {
1289+
msr->mpd->crlf_state = 0;
12241290
}
12251291

12261292
if ((msr->mpd->is_complete) && (inleft != 0)) {

‎apache2/msc_multipart.h‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ struct multipart_part {
5555

5656
char *last_header_name;
5757
apr_table_t *headers;
58+
char *last_header_line;
59+
apr_array_header_t *header_lines;
5860

5961
unsigned int offset;
6062
unsigned int length;
@@ -81,6 +83,15 @@ struct multipart_data {
8183
char *bufptr;
8284
int bufleft;
8385

86+
/* line ending status seen immediately before current position.
87+
* 0 = neither LF nor CR; 1 = prev char CR; 2 = prev char LF alone;
88+
* 3 = prev two chars were CRLF
89+
*/
90+
int crlf_state;
91+
92+
/* crlf_state at end of previous buffer */
93+
int crlf_state_buf_end;
94+
8495
unsigned int buf_offset;
8596

8697
/* pointer that keeps track of a part while
@@ -94,6 +105,14 @@ struct multipart_data {
94105
*/
95106
int mpp_state;
96107

108+
/* part parsing substate; if mpp_state is 1 (collecting
109+
* data), then for this variable:
110+
* 0 means we have not yet read any data between the
111+
* post-headers blank line and the next boundary
112+
* 1 means we have read at some data after that blank line
113+
*/
114+
int mpp_substate_part_data_read;
115+
97116
/* because of the way this parsing algorithm
98117
* works we hold back the last two bytes of
99118
* each data chunk so that we can discard it

‎apache2/re_variables.c‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1394,6 +1394,52 @@ static int var_files_combined_size_generate(modsec_rec *msr, msre_var *var, msre
13941394
return 1;
13951395
}
13961396

1397+
/* MULTIPART_PART_HEADERS */
1398+
1399+
static int var_multipart_part_headers_generate(modsec_rec *msr, msre_var *var, msre_rule *rule,
1400+
apr_table_t *vartab, apr_pool_t *mptmp)
1401+
{
1402+
multipart_part **parts = NULL;
1403+
int i, j, count = 0;
1404+
1405+
if (msr->mpd == NULL) return 0;
1406+
1407+
parts = (multipart_part **)msr->mpd->parts->elts;
1408+
for(i = 0; i < msr->mpd->parts->nelts; i++) {
1409+
int match = 0;
1410+
1411+
/* Figure out if we want to include this variable. */
1412+
if (var->param == NULL) match = 1;
1413+
else {
1414+
if (var->param_data != NULL) { /* Regex. */
1415+
char *my_error_msg = NULL;
1416+
if (!(msc_regexec((msc_regex_t *)var->param_data, parts[i]->name,
1417+
strlen(parts[i]->name), &my_error_msg) == PCRE_ERROR_NOMATCH)) match = 1;
1418+
} else { /* Simple comparison. */
1419+
if (strcasecmp(parts[i]->name, var->param) == 0) match = 1;
1420+
}
1421+
}
1422+
1423+
/* If we had a match add this argument to the collection. */
1424+
if (match) {
1425+
for (j = 0; j < parts[i]->header_lines->nelts; j++) {
1426+
char *header_line = ((char **)parts[i]->header_lines->elts)[j];
1427+
msre_var *rvar = apr_pmemdup(mptmp, var, sizeof(msre_var));
1428+
1429+
rvar->value = header_line;
1430+
rvar->value_len = strlen(rvar->value);
1431+
rvar->name = apr_psprintf(mptmp, "MULTIPART_PART_HEADERS:%s",
1432+
log_escape_nq(mptmp, parts[i]->name));
1433+
apr_table_addn(vartab, rvar->name, (void *)rvar);
1434+
1435+
count++;
1436+
}
1437+
}
1438+
}
1439+
1440+
return count;
1441+
}
1442+
13971443
/* MODSEC_BUILD */
13981444

13991445
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) {
29663012
PHASE_REQUEST_BODY
29673013
);
29683014

3015+
/* MULTIPART_PART_HEADERS */
3016+
msre_engine_variable_register(engine,
3017+
"MULTIPART_PART_HEADERS",
3018+
VAR_LIST,
3019+
0, 1,
3020+
var_generic_list_validate,
3021+
var_multipart_part_headers_generate,
3022+
VAR_CACHE,
3023+
PHASE_REQUEST_BODY
3024+
);
3025+
29693026
/* GEO */
29703027
msre_engine_variable_register(engine,
29713028
"GEO",

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

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1849,3 +1849,48 @@
18491849
),
18501850
},
18511851
1852+
# part headers
1853+
{
1854+
type => "misc",
1855+
comment => "multipart parser (part headers)",
1856+
conf => qq(
1857+
SecRuleEngine On
1858+
SecDebugLog $ENV{DEBUG_LOG}
1859+
SecDebugLogLevel 9
1860+
SecRequestBodyAccess On
1861+
SecRule MULTIPART_STRICT_ERROR "\@eq 1" "phase:2,deny,status:400,id:500168"
1862+
SecRule REQBODY_PROCESSOR_ERROR "\@eq 1" "phase:2,deny,status:400,id:500169"
1863+
SecRule MULTIPART_PART_HEADERS:image "\@rx content-type:.*jpeg" "phase:2,deny,status:403,id:500170,t:lowercase"
1864+
),
1865+
match_log => {
1866+
debug => [ qr/500170.*against MULTIPART_PART_HEADERS:image.*Rule returned 1./s, 1 ],
1867+
},
1868+
match_response => {
1869+
status => qr/^403$/,
1870+
},
1871+
request => new HTTP::Request(
1872+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt",
1873+
[
1874+
"Content-Type" => q(multipart/form-data; boundary=0000),
1875+
],
1876+
normalize_raw_request_data(
1877+
q(
1878+
--0000
1879+
Content-Disposition: form-data; name="username"
1880+
1881+
Bill
1882+
--0000
1883+
Content-Disposition: form-data; name="email"
1884+
1885+
bill@fakesite.com
1886+
--0000
1887+
Content-Disposition: form-data; name="image"; filename="image.jpg"
1888+
Content-Type: image/jpeg
1889+
1890+
BINARYDATA
1891+
--0000--
1892+
),
1893+
),
1894+
),
1895+
},
1896+

0 commit comments

Comments
 (0)