Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Handle "filename*" field in MP header #3239

Open
wants to merge 3 commits into
base: v2/master
Choose a base branch
from
Open

fix: Handle "filename*" field in MP header #3239

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cd4905b
Handle "filename*" field in MP header
airween Aug 25, 2024
524a799
Fix string comparison
airween Aug 26, 2024
2b22261
Set MULTIPART_INVALID_QUOTING variable if it is necessary
airween Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 52 additions & 3 deletions apache2/msc_multipart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@
#include "msc_util.h"
#include "msc_parsers.h"

static const char* mime_charset_special = "!#$%&+-^_`{}~";
marcstern marked this conversation as resolved.
Show resolved Hide resolved
static const char* attr_char_special = "!#$&+-.^_`~";

void validate_quotes(modsec_rec *msr, char *data, char quote) {
assert(msr != NULL);
int i, len;
Expand Down Expand Up @@ -85,6 +88,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
assert(msr != NULL);
assert(c_d_value != NULL);
char *p = NULL, *t = NULL;
char *filenameStar = NULL;

/* accept only what we understand */
if (strncmp(c_d_value, "form-data", 9) != 0) {
Expand Down Expand Up @@ -130,7 +134,42 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
*/

char quote = '\0';
if ((*p == '"') || (*p == '\'')) {
if (strcmp(name, "filename*") == 0) {
/* filename*=charset'[optional-language]'filename */
/* Read beyond the charset and the optional language*/
const char* start_of_charset = p;
while ((*p != '\0') && (isalnum(*p) || (strchr(mime_charset_special, *p)))) {
p++;
}
if ((*p != '\'') || (p == start_of_charset)) {
return -16; // Must be at least one legit char before ' for start of language
}
p++;
while ((*p != '\0') && (*p != '\'')) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Languages can only contain alphanum & '-'
while (isalnum(*p) || *p == '-') p++;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and here - as you suggested - we can control this.

The question is: is it allowed to send the request without charset? I mean:

Content-Disposition: form-data; name="post"; filename*=resume.pdf

I can't find any relevant information about that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.rfc-editor.org/rfc/rfc7578#section-4.2 also says:

Some commonly deployed systems use multipart/form-data with file
names directly encoded including octets outside the US-ASCII range.
The encoding used for the file names is typically UTF-8, although
HTML forms will use the charset associated with the form.

Which sounds to me that any charset is valid, regardless of what the other RFC says.

charset is required, AFAICT, only language is optional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, do you think we do not need to check the charset and we should allow anything? Or restrict the charset content only to alnum chars (+ -)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the charset should be restricted to the ABNF from the RFC.

p++;
}
if (*p != '\'') {
msr->mpd->flag_invalid_quoting = 1;
return -17; // Single quote for end-of-language not found
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you use something like this here, as below?

msr->mpd->flag_invalid_quoting = 1;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea - thanks.

@fzipi, @marcstern - what do you think about this guys?

I already added this in 2b22261, but I can remove that.

If we keep this feature, we should add that to v3 too.

}
p++;

/* Now read what should be the actual filename */
const char* start_of_filename = p;
theseion marked this conversation as resolved.
Show resolved Hide resolved
while ((*p != '\0') && (*p != ';')) {
if (*p == '%') {
if ((*(p+1) == '\0') || (!isxdigit(*(p+1))) || (!isxdigit(*(p+2)))) {
return -18;
}
p += 3;
} else if (isalnum(*p) || strchr(attr_char_special, *p)) {
p++;
} else {
return -19;
}
}
value = apr_pmemdup(msr->mp, start_of_filename, p - start_of_filename);
} else if ((*p == '"') || (*p == '\'')) {
/* quoted */
quote = *p; // remember which quote character was used for the value

Expand Down Expand Up @@ -205,8 +244,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
log_escape_nq(msr->mp, value));
}
}
else
if (strcmp(name, "filename") == 0) {
else if (strcmp(name, "filename") == 0) {

validate_quotes(msr, value, quote);

Expand All @@ -223,6 +261,17 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value)
msr_log(msr, 9, "Multipart: Content-Disposition filename: %s",
log_escape_nq(msr->mp, value));
}
} else if (strcmp(name, "filename*") == 0) {
airween marked this conversation as resolved.
Show resolved Hide resolved
if (filenameStar != NULL && strlen(filenameStar) != 0) {
marcstern marked this conversation as resolved.
Show resolved Hide resolved
msr_log(msr, 4,
"Multipart: Warning: Duplicate Content-Disposition filename*: %s.", log_escape_nq(msr->mp, value));
return -20;
}
filenameStar = apr_pstrdup(msr->mp, value);
if (msr->txcfg->debuglog_level >= 9) {
msr_log(msr, 9, "Multipart: Content-Disposition filename*: %s.",
log_escape_nq(msr->mp, value));
}
}
else return -11;

Expand Down
Loading