Skip to content

Multipart Content-Disposition should allow filename* field #2214

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.

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

Closed
wants to merge 1 commit into from
Closed

Conversation

martinhsv
Copy link
Contributor

No description provided.

@zimmerle zimmerle self-requested a review February 11, 2020 13:24
@zimmerle zimmerle self-assigned this Feb 11, 2020
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label Feb 11, 2020
@zimmerle
Copy link
Contributor

Merged! Thanks!

@zimmerle zimmerle closed this Feb 11, 2020
@zehric
Copy link

zehric commented Oct 7, 2020

@martinhsv Why does filenameStar not get consumed anywhere? As far as I can tell it is only local to the function it is declared in. Should we not populate m_mpp->m_filename with the content of filenameStar at the end? And why is it required that a filename directive be present if the filename* is present?

@martinhsv
Copy link
Contributor Author

The primary goal of this change was to address the most immediate problem. Formerly, the parsing would error-out in a way that was problematic to work around if filename*= were present at all.

The level of support for filename*= can be extended, but even if that is deemed desirable now or in future, the initial implementation was to at least provide meaningful improvement and progress.

Part of the challenge with a more extensive implementation is deciding what to do with a Content-Disposition header that does NOT also have filename= (the RFC that introduced filename*= encourages -- but does not require -- also including filename= in that part's Content-Disposition), e.g.

Content-Disposition: form-data; name="myname"; filename*=utf-8''03CB16644685216C1E818D1369BB5D4B.png"

In this case, should ModSecurity ideally handle this with the comparable code path as would occur with filename= ?

But it seemed that not all web servers support filename*= (for example nginx would ignore it but not produce an error).

One risk is that:

  • ModSecurity uses comparable functionality as if it had been filename=
  • we do not populate collections like ARGS and ARGS_POST with myname
  • the web server will process the part the same as if neither filename= nor filename*= were present (i.e. as a regular $_POST variable)
  • an attacker has a way to bypass a rule that is inspecting ARGS or ARGS_POST (because he can add a spurious filename*= knowing that it will be ignored by the web server)

And since both nginx and Apache HTTP Server are the two most common deployments for ModSecurity, and both ignore filename*=, we should be able expect legitimate clients sending to those two web servers to not send filename*= without also sending filename=.

Of course, there are ways to extend the functionality and handle any outstanding concerns. For example, how to handle the specific use case I outlined above could be supported by a new configuration directive.

However, all things considered, it seemed sensible to proceed with the initial implementation contained in this pull request and then assess future feedback/demand from the community.

I hope that helps.

@zehric
Copy link

zehric commented Oct 14, 2020

Thanks @martinhsv, that does make a lot of sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants