Malformed JSON body results in 400 Bad Request instead of 500

[alxyzc]

• Introducing new return value (-8) in modsecurity_request_body_end when failing to parse JSON bodies
• This -8 return value in turn results in a 400 Bad Request response status code in NGINX

[azhao155]

1. why only update this branch? how about the else condition?
2. could we check the RFC whether this fits into 400, from my understanding, if it's invalid request, when nginx parse the request, it will return 400 instead of modsec parsing and return. Also this is more like module parsing request error, which mean it's server side parsing error, which should be 500.
   '

[alxyzc]

1 - I thought of the same -- if we agree that this is an elegant implementation of the fix, I will update the change to all scenarios where the body parsing fails.

2 - Quoting RFC 7231 for reference:

6.5.1. 400 Bad Request
The 400 (Bad Request) status code indicates that the server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing).

6.6.1. 500 Internal Server Error
The 500 (Internal Server Error) status code indicates that the server encountered an unexpected condition that prevented it from fulfilling the request.

My understanding is that 500 usually happens when the server throws an unhandled exception, which could be caused by a variety of reasons, including a missing implementation of a specific scenario. In this case, we can see the return -1 as throwing an exception which was not handled gracefully higher in the call hierarchy, thus results in a 500 Internal Server Error, which is the main complaint of the IcM ticket.

However, this code change effectively patching that missing code path, so that the body parsing error is now handled gracefully by resulting in a 400, which according to the RFC is a valid response code when we "cannot or will not process" the "malformed request syntax" due to "a client error".

[azhao155]

this is kind of tricky, in this case, if return 400, we treat this as client error, but this incomplete json is only unexpected condition for modsec to fulfilling the request.
Also we have been return 500 for parsing error for quite long time for customers, if we change to 400, not sure what's the impact in the client side. Could we pull in Avinash for more inputs?

[alonzop]

to add to Yanshu comments - if I recall , part of the problem was that there was no logging on the modsec
I don't see that being addressed, although I see that msr_log is called ,

[azhao155]

also we might need to understand what's json_complete doing? And what error condition to trigger it. which help us to get more understanding for whether it's client error or just something specific for modsec

[alonzop]

but the fix appears to be low risk, i m ok with it

[alxyzc]

@azhao155 in this particular case in the IcM it is the client sending non-JSON text when the Content-Type is application/json. I think your point is that whether we can rely on json_complete to always successfully parse a correctly formatted JSON, so that when it doesn't it always implies that the JSON is malformed? In json_complete it delegates to the YAJL library for parsing the JSON, which like any library makes promises but a question mark could always be placed on whether we can trust those promises.

@alonzop There is no firewall log entries for malformed requests because these requests will trigger no rules. There are entries, on the other hand, in the nginx/error.log, as it is treated by ModSecurity as an exception. However the error logs complains about not being able to "print json logs" instead of not being able to parse the JSON body. I was stepping through the ModSecurity code, and the logging gets really strange when the request does not parse -- a warning string is passed into the logger where it's expecting a structured message with very specifically defined fields (source code) 🤔 I'm okay with not changing the behavior if it has a potential of breaking customers, but the logging will probably require a deeper dive.

[alonzop]

@alxyzc not sure about breaking as i am not aware of use cases, but we should get input from pm/ @asridharan
as for the logging, i understand , and ok with that, no need to add noise to the waf logs, maybe we should fix the msg in the error.log to reflect the correct problem

[azhao155]

@alxyzc @asridharan Since now we start to change behavior return 400 instead of 500 for json parsing error, should we do the same thing for other format as well?

[asridharan]

@alxyzc I would prefer not changing the response status code explicitly for JSON errors. Can we have a discussion on this before proceeding on this?