JSON requests with no body fail as "premature EOF"

[metalspawn]

I have set up the v3/master version with the nginx connector. Our team is using it with Centos 7.3 SE and nginx 1.10.3. Further details can be provided on request.

While running our API test suite I noticed basic GET requests were failing with a 400. Upon inspection of Modsecurity's audit logs, it was apparent there was an issue parsing the request body - the error returned was: [msg "Failed to parse request body."]. This was followed with: [data "XML parsing error: parse error: premature EOF\x0a"]

The error message was initially confusing as the request contained the header Content-Type: application/json, but we confirmed that was just a minor bug with the error message (PR to follow soon).

Assuming there was some unknown content contained within the body of the request, we spent some time evaluating the exact contents of the payload and concluded that no body was being sent at all (as expected).

So it seems ModSecurity validates the body of the request even if there is none to parse. It works correctly with XML but an empty string is considered invalid JSON. I noted the commented out lines above the parsing execution code at: https://github.com/SpiderLabs/ModSecurity/blob/v3/master/src/transaction.cc#L656-L663

I do wonder on what basis the decision to parse even when there is no content was made. Not wanting to presume too much, our temporary fix was to move that check into the JSON parsing block only (we could have excluded the GET requests and others explicitly in a chained rule, but if a body is present, we believe it should be parsed).

I am happy to provide another PR but wanted to get the community's thoughts on the above first.

cc: @jayrapson

[zimmerle]

Hi @metalspawn,

Regardless of the request body content, there are rules that still needs to be processed during the request body phase. Those may or not may be related to the request body. The key point is this call here:
https://github.com/SpiderLabs/ModSecurity/blob/v3/master/src/transaction.cc#L799

Ideally there is no reason to put a rule in the request body phase if it is not directly related to a request body, but, some users thinks otherwise.

[zimmerle]

Hi @metalspawn,

One thing that I was testing here was the behavior of ModSecurity version 2 dealing with an empty JSON. The behavior is to treat an empty JSON as an error. Check the logs here:
https://gist.github.com/zimmerle/bef703e5f57c42d5d958fe20a59e8971

I was using:

curl -H "Content-Type: application/json" -X POST -d '' http://localhost

Ultimately the ModSecurity version 2 and 3 have to behave in the same fashion. At least for the first version of ModSecurity version 3.

[victorhora]

I'm wondering on this issue because as far as I could see here, on its default recommended configuration ModSecurity is not triggering rule 200002 and looks like is properly validating and parsing JSON data on POST body if the request is well formatted as expected.

curl -H "Content-Type: application/json" -X POST -d '' http://localhost/a

Generates a request which seems to be non-compliant with JSON RFCs as valid JSON requests ("JSON value" or "JSON text") should contain at least one of the following: integer, boolean, string, {} (empty object), [] (empty array) . So being non-compliant (no data) I think the behaviour is correct and it should be the user's choice to either disable/change rule 200002 or, if possible, make sure the POST requests with JSON data are compliant.

Now with GET and JSON as parameter the behaviour is a bit different.

curl -H "Content-Type: application/json" -X GET http://localhost/a?b=%7B%22name%3A%22ABC%22,%22id%22%3A%221%22

This works fine and seems ok from a standard perspective (although I didn't dig much to see if ModSecurity's JSON parser is able to properly parse this as it does with POST requests, but this a different thing).

But if you do:

curl -H "Content-Type: application/json" -X GET -d '' http://localhost/a?b=%7B%22name%3A%22ABC%22,%22id%22%3A%221%22

The request is sent like this:

GET /a?query=%5B%7B%22percentage_alcohol%3E%22%3A+0%2C+%22country%22%3A+null%2C+%22type%22%3A+%22%2Ffood%2Fwine%22%2C+%22name%22%3A+null%2C+%22percentage_alcohol%22%3A+null%7D%5D HTTP/1.1
Host: localhost
User-Agent: curl/7.47.0
Accept: */*
Content-Type: application/json
Content-Length: 0
Connection: close

As far as I know the standard behaviour would be that the receiving party (or ModSecurity parser in this case) would expect some body data to be sent because the Content-Lenght field is present even though common practice suggests that a request containing body data should not be sent as GET. This ends up being handled as a bad request and breaks the parser and triggering rule 200002.

Now, It looks like this request is NOT strictly non-standard according to the RFCs, but it might be interpreted as bad practice and RFC 7231 does mention the following:

A payload within a GET request message has no defined semantics;
sending a payload body on a GET request might cause some existing
implementations to reject the request.

All that said, I'm not sure how we strict this should be. Opinions? :)

[allanbomsft]

Even putting a content-type and content-length on a GET seems strange. But also seems harmless though.

[zimmerle]

That may open a possibility to a bypass. Lets say that ModSecurity ignores the header, and the end application is somehow affected by it.... as suggested in the linked issues, it seems like the better way to deal with this is via SecRule. As it is closer to the end user, better place to understand the semantic of the empty JSON content in the end application.

There is another perspective: an empty content is not a valid JSON, to be valid it demands to have an structure. Example:

{
}

or

[
]

So, the logical assumption is to flag the empty as invalid.

[allanbomsft]

But then why do we allow empty XML or empty form/multipart?

[zimmerle]

Hi @allanbomsft,

That is indeed a good question. Perhaps the best way to handle this, is also considers the XML invalid.

The idea is catch a possible problem (empty content in that case) before it hits the end application, where the behavior can be considered to be unknown.

[marcstern]

The best solution is to accept it in the parser and have a rule to block it, so you can manage exceptions when needed (yes, some applications use this)

[zimmerle]

Closing this issue again as discussed on #1822