-
Notifications
You must be signed in to change notification settings - Fork 1.7k
v2: Allow partial processing of XML, JSON, and multipart request body #3483
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
Open
hnakamur
wants to merge
23
commits into
owasp-modsecurity:v2/master
Choose a base branch
from
hnakamur:v2/stop_processing_after_reqbody_limit_for_process_partial
base: v2/master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+2,159
−79
Open
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
a682f13
Allow partial processing of multipart, JSON, and XML request body
hnakamur 55270e5
Adjust test since ProcessPartial no more causes "no final boundary mi…
hnakamur 586e1ef
Fix json test match_log
hnakamur e9dca3c
Fix expected error message in regression test
hnakamur 0e4728f
Accept partial epilogue in body larger than limit for ProcessPartial
hnakamur 24508d6
Fix indent in apache2/msc_multipart.c
hnakamur bea943b
Fix indent in apache2/msc_json.c
hnakamur 9b2a5fe
Add tests for url-encoded, JSON, and XML with ProcessPartial
hnakamur 0bfb828
Refine tests for multipart with ProcessPartial
hnakamur 43f95fd
Modify url-encoded reqbody tests for ProcesPartial
hnakamur 3d73c02
Support partial processing of url-encoded reqbody
hnakamur d914f23
Add tests for MULTIPART_PART_HEADERS with ProcessPartial
hnakamur 55a1828
Reject invalid final boundary for multipart with ProcessPartial
hnakamur 0b599ad
Fix indent in apache2/msc_multipart.c
hnakamur cb95a24
Modify tests for multipart with ProcessPartial
hnakamur 61d4e45
Modify multipart_complete for incomplete final boundary
hnakamur be5feee
Process an incomplete boundary as a non-final boundary
hnakamur 50de8bb
Update tests/regression/rule/15-json.t
hnakamur a64b544
Update tests/regression/rule/15-json.t
hnakamur a83c26c
Fix spelling of reqbody_partial_processing_enabled
hnakamur da8b174
Fix indentations in test files
hnakamur f679e11
Add rules to check multipart error in tests
hnakamur 9277589
Fix indentations in tests/regression/rule/10-xml.t
hnakamur File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling logic has been modified to only return error codes when if_limit_action is REQUEST_BODY_LIMIT_ACTION_REJECT. However, this means that when if_limit_action is REQUEST_BODY_LIMIT_ACTION_PARTIAL, errors from modsecurity_request_body_end (rcbe) are silently ignored and APR_SUCCESS is returned. This could mask legitimate parsing errors that occur during partial processing. Consider returning errors for non -5 error codes even in ProcessPartial mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change causes a parser error returns a response with status code 500, which is not wanted since this case is not an internal server error.
In case of a parse error, REQBODY_ERROR is set to 1, which can be used by a rule to return a 400 Bad Request response, for example:
Side note: However, I found that no rule in OWASP CRS 4.22.0 checks
REQBODY_ERROR.This is wanted behavior for both of
REQUEST_BODY_LIMIT_ACTION_REJECTandREQUEST_BODY_LIMIT_ACTION_PARTIAL.Therefore I think the right modification here is:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, then we can make this as resolved. Thanks.
Good advice - would you mind to open a PR or just an issue under coreruleset?
Do you want to add/change this one too? I'm waiting for your response, until that I don't resolve this comment.