-
-
Notifications
You must be signed in to change notification settings - Fork 203
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
SecRequestBodyLimitAction Reject should be invoked only if content-length is greater than SecRequestBodyLimit #1045
Comments
I can fix the same.
I have to check contribution guidelines and other formalities to create PR. |
INBOUND_DATA_ERROR is not working too (when request body is read from io.Reader under ReadRequestBodyFrom()). INBOUND_DATA_ERROR is only set when reader is ByteLenger. |
Hey, thanks for the report, and sorry for the delay. Regarding INBOUND_DATA_ERROR, I just opened a PR that should fix it and extend the test to check if the flag is set when the limit is reached. Any feedback is welcomed. About the limit, It requires a bit more time and reasoning. We are indeed rejecting it as soon as the limit is reached, not when it is passed and it is not aligned with the doc. I recall some offline reasonings with @jcchavezs about this, but I don't recall why we went for that solution right now. If we want to stick with the current behaviour we should update the documentation. Would also be good to check Modsec to eventually be aligned about it. |
Hi @brijeshjvalera thanks for coming by. I think the bug is in coraza/internal/corazawaf/transaction.go Lines 948 to 949 in aa449ce
if tx.RequestBodyLimit == tx.requestBodyBuffer.length are correct. Are you up to come up with a PR and cover it with a test?
|
Now after giving a second thought on this I remember the reason. In our case we preferred simplicity over feature parity. In Go, when you read a request body, the pointer will move to the next byte to be read. In our case if the limit is 10 and the body size is 20 (however unknown on beforehand even if content-length is passed), we will read the first 10 and leave the other 10 in the request body buffer and later we will deliver a combination of coraza buffer + request remaining body (passing the 20 bytes to the upstream), however imagine we read 10 and the body is 11, if we were aiming for |
Looks like this comment should be written in the code somewhere. |
Description
SecRequestBodyLimitAction Reject should reject TX if it has request body content-length greater than configured SecRequestBodyLimit.
Steps to reproduce
Set below configuration:
Send data with content-length 47 (configured)
I have HTTP server wrapped with Coraza running on localhost:9000.
Expected result
It should interrupt only if content-length is more than configured SecRequestBodyLimit. Anything over the limit should be rejected. Ref: https://coraza.io/docs/seclang/directives/#secrequestbodylimit
Actual result
It is getting blocked. Current condition checks if tx.reqBodyBuffer.length == tx.RequestBodyLimit, creates interruption.
It should succeed with 200 OK.
The text was updated successfully, but these errors were encountered: