Skip to content

Fix: FILES variable does not use multipart part name for key #2379

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.

@airween
Copy link
Member

airween commented Aug 5, 2020

If I'm right, whit this patch the m_variableFiles and m_variableFilesNames transaction variables will have the same value (m->m_name from the body processor). These variables converted to FILES and FILES_NAMES here and here (and in the other function), so the FILES and FILES_NAMES will have the same value.

Or em I wrong?

(Note, there was a similar bug: the m_variableFiles and m_variableFilesNames both had the m_filename value, and fixed in #2016.)

@martinhsv
Copy link
Contributor Author

martinhsv commented Aug 5, 2020

Hi @airween ,

Thanks for the feedback. I'll explain in a little more detail and you can let me know if you still have questions or concerns.

The FILES_NAMES collection (i.e. m_variableFilesNames) has key-value pairs where both the key and the value are the same string, which means that with multipart part content including something ilke this ...

Content-Disposition: form-data; name="abcd"; filename="x00001.txt"

... you could test for the 'name' in either the key position of a rule or the value position of a rule, or both. For example:

SecRule FILES_NAMES:'/abc/' "@contains bcd" "id:1240, phase:2, deny,status:403"

This is similar to how other '_NAMES' collections work. For example, in ARGS_NAMES, the same string is both the key and the value in the key-value pair.

With this pull request, the FILES collection (i.e. m_variableFiles) entries will have a key that is the same as in FILES_NAMES, but the value will be the string from 'filename'. It enables a rule like this:

SecRule FILES:'/abc/' "@contains x000" "id:1240, phase:2, deny,status:403"

... which would not work in v3 prior to this pull request.

This is analogous to ARGS_NAMES vs. ARGS, where they both have the same key but the latter has a value that is different from the key.

Note also, that this change makes the functionality match that of v2.9.3.

Let me know if that clarifies things or if you still have questions or concerns. Thanks.

@airween
Copy link
Member

airween commented Aug 5, 2020

Hi @martinhsv,

many thanks for details, now it's clear. I checked your patch with this test file (which - I think - demonstrates the advantage of the feature), it worked as well (without the patch the last case was failed).

Just my 2 cents: it would be good to add this behavior to the documentation.

@martinhsv martinhsv requested a review from zimmerle August 11, 2020 22:08
@zimmerle
Copy link
Contributor

zimmerle commented Nov 6, 2020

@zimmerle
Copy link
Contributor

zimmerle commented Nov 9, 2020

Merged! Thanks!

@zimmerle zimmerle closed this Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants