SecRequestBodyLimitAction Reject should be invoked only if content-length is greater than SecRequestBodyLimit

[brijeshjvalera]

Description

SecRequestBodyLimitAction Reject should reject TX if it has request body content-length greater than configured SecRequestBodyLimit.

Steps to reproduce

Set below configuration:

Request-body limit and action configuration

SecRequestBodyLimit 47
SecRequestBodyLimitAction Reject

Send data with content-length 47 (configured)
I have HTTP server wrapped with Coraza running on localhost:9000.

curl -v -X POST http://localhost:9000/ --data 'This is my test-data having content-length: 47.'
Trying [::1]:9000...
Connected to localhost (::1) port 9000
POST / HTTP/1.1
Host: localhost:9000
User-Agent: curl/7.71.1-DEV
Accept: /
Content-Length: 47
Content-Type: application/x-www-form-urlencoded

HTTP/1.1 413 Request Entity Too Large
Date: Mon, 22 Apr 2024 12:18:11 GMT
Content-Length: 0

Connection #0 to host localhost left intact

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.

[brijeshjvalera]

I can fix the same.
TTBOMK, we can skip checking requestBodyBuffer.length with RequestBodyLimit after copying the buffer. Check below:

w, err := io.CopyN(tx.requestBodyBuffer, r, writingBytes)
if err != nil && err != io.EOF {
return nil, int(w), err
}

// We should skip checking this: requestBodyBuffer.length+writingBytes is already checked against tx.RequestBodyLimit.
// if tx.requestBodyBuffer.length == tx.RequestBodyLimit {
// if tx.WAF.RequestBodyLimitAction == types.BodyLimitActionReject {
// return setAndReturnBodyLimitInterruption(tx)
// }

// if tx.WAF.RequestBodyLimitAction == types.BodyLimitActionProcessPartial {
// runProcessRequestBody = true
// }
// }

I have to check contribution guidelines and other formalities to create PR.
Please feel free to check on your end and fix the same.

[brijeshjvalera]

INBOUND_DATA_ERROR is not working too (when request body is read from io.Reader under ReadRequestBodyFrom()).
Ref: https://coraza.io/docs/seclang/variables/#inbound_data_error

INBOUND_DATA_ERROR is only set when reader is ByteLenger.

[M4tteoP]

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.

[jcchavezs]

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.requestBodyBuffer.length == tx.RequestBodyLimit {
	if tx.WAF.RequestBodyLimitAction == types.BodyLimitActionReject {

, the other comparisons if tx.RequestBodyLimit == tx.requestBodyBuffer.length are correct. Are you up to come up with a PR and cover it with a test?

[jcchavezs]

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 >= instead of == we would read at most 11 bytes co corroborate that the limit is passed and the body buffer has to store 11 and the request remaining body becomes empty so if we were about to fix this behaviour we must make the body buffers size to be limit + 1, document everywhere that the buffer's size is limit + 1 (weird by OK) and ack that when reading limit+1 bytes and the body returning an error because it was just in the limit we have to handle that gracefuly in case for example limit is 10 and body size is 10.

[fzipi]

Looks like this comment should be written in the code somewhere.