Bug: Premature EOF in JSON leads to status 500 instead of REQBODY_ERROR

[dune73]

Describe the bug

Instead of failing safely, ModSec triggers a status 500.

$ cat example.json
{ "id" : "123"
$ curl -v http://localhost -H "Content-Type: application/json" -d @example.json
...
< HTTP/1.1 500 Internal Server Error
...

Logs and dumps

[2022-10-05 11:17:58.926606] [-:error] 127.0.0.1:39976 Yz1Lxuf_TBfAeTSTcu5oFwAAAAk [client 127.0.0.1] ModSecurity: JSON parser error: parse error: premature EOF\n [hostname "localhost"] [uri "/"] [unique_id "Yz1Lxuf_TBfAeTSTcu5oFwAAAAk"]
[2022-10-05 11:17:58.926624] [-:error] 127.0.0.1:39976 Yz1Lxuf_TBfAeTSTcu5oFwAAAAk [client 127.0.0.1] ModSecurity: JSON parser error: parse error: premature EOF\n [hostname "localhost"] [uri "/"] [unique_id "Yz1Lxuf_TBfAeTSTcu5oFwAAAAk"]
[2022-10-05 11:17:58.926629] [core:trace3] 127.0.0.1:39976 Yz1Lxuf_TBfAeTSTcu5oFwAAAAk fixups hook gave 500: /

Server (please complete the following information):

• ModSecurity version (and connector): ModSec 2.9.6
• WebServer: Apache 2.5.54

Additional information:

I played around with broken JSON payloads following a customer request. This was the only case where I could make ModSecurity stumble.

[martinhsv]

Hi @dune73 ,

At first glance I would probably agree that it makes more sense for this use case to signal REQBODY_ERROR instead of triggering a status code of 500. Indeed, that is the behaviour in ModSecurity v3.

A slight clarification: with SecRuleEngine On, the outcome is as you describe. But with DetectionOnly REQBODY_ERROR does get set (with appropriate rule 200002 output in the audit log).

Note, however, that this status 500 outcome that you have observed is not unique to failures with JSON parsing. For example, a comparable case with XML produces the same result. Indeed the software uses 500 as a general response to request body parsing failures that are not otherwise categorized. If we were to consider making a change to the JSON-parsing-failure result, we would probably need to map out all other cases where comparable considerations apply, so as to avoid exacerbating inconsistencies within ModSecurity v2.

Also: Just curious if you have some specific danger in mind when you say "Instead of failing safely"? Although the current v2 behaviour is likely not ideal, I don't off hand see anything that I would describe as a safety issue.

[dune73]

Thank you for your swift response Martin. I thought I had seen status 500 with XML before but it was new with JSON for me.

Then playing around I made the following discoveries:

JSON Payload                 Behaviour
{ "i"foo"d" : "123" }        REQBODY_ERROR Set (Error message: JSON parsing error: lexical error: invalid string in json text.\n)
{ id : "123" }               REQBODY_ERROR Set (Error message: JSON parsing error: lexical error: invalid char in json text.\n)
{ "id": [{{ "123" }}] }      REQBODY_ERROR Set (Error message: JSON parsing error: parse error: invalid object key (must ...)
{ "id" : "123" } ...         REQBODY_ERROR Set (Error message: JSON parsing error: parse error: trailing garbage\n)
{ "id" : - }                 REQBODY_ERROR Set (Error message: JSON parsing error: lexical error: malformed number, a digit ...)
{ "id" : "123"               Status 500 (Error message: JSON parser error: parse error: premature EOF\n)

Looking over the various payloads, all with different error messages, lets the last one stand out. So I'd argue it's at least inconsistent and if ModSec3 is doing this better, then that's great.

Failing safely: I think it's always better to make the reaction configurable. With the status 500 you can no longer influence the behaviour while the REQBODY_ERROR allows you do come up with a more nuanced behaviour. Redirect to special error page depending on the context for example. I agree that 500 is not really unsafe here, but it's misleading since the server did identify the client error it just chose to abort instead of triggering the appropriate status 400 with the help of a ModSec rule.

And as you can see above, nothing in the error message gives away the fact, that EOF would be different from the rest of the messages.

[martinhsv]

My guess is that, when this was originally programmed, the distinction was that the first 5 cases are all consistent with at least some successful parsing.

The 6th example, however, has nothing that can be successfully parsed (because even outermost JSON requirement has not been satisfied.

By way of comparison, most multipart parsing errors are signalled through individual variables. However if a supposedly multipart request body contains only the single character "a" (i.e. with no boundaries specified at all), then ModSecurity v2 will return 500.

While that distinction between the use cases makes some sense to me, that doesn't mean that it's the ideal choice. Indeed, partly for the reasons you stated, I do lean towards the conclusion that the ModSecurity v3 behaviour (signalling through variables rather than a response of 500, for the cases under discussion) is preferable.

[dune73]

+1 on the interpretation why this might be the case. That makes sense.

As for the behaviour that is preferable, I agree. Providing a PR is probably outside my capability here. But I would still welcome a fix.

[RedXanadu]

Hello,

I think I've run into this same problem before, but in a different context. I think this may be related to a bug in request body handling that was introduced in ModSecurity 2.9.3. (Apologies if this is not related, but it seems like it may be the same root issue.)

I've found that older versions of ModSecurity respond differently to multipart parsing errors. This also seems to have broken the option ProcessPartial, I believe due to a change in the read_request_body function.

If I deliberately send a request with malformed multipart boundaries, ModSecurity 2.9.1 responds with 400 Bad Request. ModSecurity 2.9.5 instead responds with 500 Internal Server Error. I saw a lot of those internal server errors in testing after updating from 2.9.1 to 2.9.5. I never had this problem on 2.9.1, it always worked as expected.

That could be a problem in a scenario where you've changed this rule:

SecRule REQBODY_ERROR "!@eq 0" \
"id:'200002', phase:2,t:none,log,deny,status:400,…

to have a specific status code, i.e. anything you set (400, 403, etc.) would be ignored and a 500 would be served.

---

ProcessPartial works in ModSecurity 2.9.2 because the read_request_body function would always return 1, unconditionally.

In 2017, the return code was changed so that it would indicate the outcome at the end of reading the request body: https://github.com/SpiderLabs/ModSecurity/pull/1613/files#diff-db36ab5d2f2d3ea8001e77a6f92035890a9729ef947c5dd7ba3ba6dbfff57030R347

The effect of that change seems to be that if reading the request body fails, e.g. a boundary parsing error, then the request body is not processed at all. Phase 2 is not executed and it skips straight to phase 3.

So, ProcessPartial no longer allows partial request body content to be inspected from ModSecurity 2.9.3 onwards.

I've successfully tested restoring/reverting the functionality of ProcessPartial by reverting return rcbe; back to return 1; in 2.9.5. But I don't know what other effects that would have, so I haven't considered opening a PR for that :slightly_smiling_face:

[dune73]

Wow, this is interesting. Maybe worth a separate feature!

Did you or can you try out my curl call from above on 2.9.2?

[marcstern]

Returning 1 instead of rcbe allows more flexibility, as you can decide yourself what to do (ant to log), but put more responsibility on the configurator. I personally prefer this, but I have to admit that returning rcbe is a "default-deny" approach.

A side-remark: In modsecurity_request_body_end(), we return -1 (which leads to a status 500) when there's a parsing problem. Returning -2 would lead to a status 400, which is better for me as it's not an internal error. Same in modsecurity_request_body_end_urlencoded()

[RedXanadu]

Thank you for the insight, Marc. I agree: it is preferable to have choice and control over whether to send a 400 response, a 500, or something else.

@dune73 I tested your six payloads. Specifically looking at the interesting one { "id" : "123" I confirmed that this behaviour has changed in recent versions. I observed the following:

ModSecurity 2.9.2

[2022-10-10 15:59:01.806647] [-:error] 192.168.85.1:49858 Y0QzNYpWt8TqVTt2NxHBlAAAAAY [client 192.168.85.1] ModSecurity: JSON parser error: parse error: premature EOF\n [hostname "localhost"] [uri "/"] [unique_id "Y0QzNYpWt8TqVTt2NxHBlAAAAAY"]
[2022-10-10 15:59:01.806819] [-:error] 192.168.85.1:49858 Y0QzNYpWt8TqVTt2NxHBlAAAAAY [client 192.168.85.1] ModSecurity: Access denied with code 400 (phase 2). Match of "eq 0" against "REQBODY_ERROR" required. [file "/opt/apache-2.4.43/conf/httpd.conf"] [line "134"] [id "200002"] [msg "Failed to parse request body."] [data "JSON parser error: parse error: premature EOF\\x0a"] [severity "CRITICAL"] [tag "Local Lab Service"] [hostname "localhost"] [uri "/"] [unique_id "Y0QzNYpWt8TqVTt2NxHBlAAAAAY"]

ModSecurity: Access denied with code 400

---

ModSecurity 2.9.6 with read_request_body defined to return 1;

[2022-10-10 15:49:59.122327] [-:error] 192.168.85.1:46796 Y0QxF5yUBD7QnT8MUmQH@AAAAAc [client 192.168.85.1] ModSecurity: JSON parser error: parse error: premature EOF\n [hostname "localhost"] [uri "/"] [unique_id "Y0QxF5yUBD7QnT8MUmQH@AAAAAc"]
[2022-10-10 15:49:59.122502] [-:error] 192.168.85.1:46796 Y0QxF5yUBD7QnT8MUmQH@AAAAAc [client 192.168.85.1] ModSecurity: Access denied with code 400 (phase 2). Match of "eq 0" against "REQBODY_ERROR" required. [file "/opt/apache-2.4.43/conf/httpd.conf"] [line "134"] [id "200002"] [msg "Failed to parse request body."] [data "JSON parser error: parse error: premature EOF\\x0a"] [severity "CRITICAL"] [tag "Local Lab Service"] [hostname "localhost"] [uri "/"] [unique_id "Y0QxF5yUBD7QnT8MUmQH@AAAAAc"]

ModSecurity: Access denied with code 400

---

ModSecurity 2.9.6 with read_request_body defined to return rcbe; (the default)

[2022-10-10 15:51:37.580831] [-:error] 192.168.85.1:41250 Y0QxeddH8EhTm2IYahlqwgAAAAY [client 192.168.85.1] ModSecurity: JSON parser error: parse error: premature EOF\n [hostname "localhost"] [uri "/"] [unique_id "Y0QxeddH8EhTm2IYahlqwgAAAAY"]
[2022-10-10 15:51:37.580861] [-:error] 192.168.85.1:41250 Y0QxeddH8EhTm2IYahlqwgAAAAY [client 192.168.85.1] ModSecurity: JSON parser error: parse error: premature EOF\n [hostname "localhost"] [uri "/"] [unique_id "Y0QxeddH8EhTm2IYahlqwgAAAAY"]

From cURL: < HTTP/1.1 500 Internal Server Error

[dune73]

Thank you very much for checking this for me, @RedXanadu.

[dune73]

I have just met this bug again and tried to fight it with

SecRequestBodyLimitAction Reject

But no effect.

Debug Log:

...
Adding JSON argument 'glossary.GlossDiv.GlossList.GlossEntry.GlossSee' with value 'markup'
Input filter: Bucket type EOS contains 0 bytes.
JSON parser error: parse error: premature EOF
Input filter: Completed receiving request body (length 560).
JSON parser error: parse error: premature EOF
Hook insert_error_filter: Adding output filter (r 7f60f0004c30).
Output filter: Receiving output (f 7f60f0006fe8, r 7f60f0004c30).
Starting phase RESPONSE_HEADERS.
...