Skip to content

Malformed JSON body results in 400 Bad Request instead of 500 #162

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: waf_nginx
Choose a base branch
from
Open

Malformed JSON body results in 400 Bad Request instead of 500 #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions apache2/mod_security2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,13 @@ static int hook_request_late(request_rec *r) {
r->connection->keepalive = AP_CONN_CLOSE;
return HTTP_BAD_REQUEST;
break;
case -8 : /* JSON body does not parse */
if (my_error_msg != NULL) {
msr_log(msr, 4, "%s", my_error_msg);
}
r->connection->keepalive = AP_CONN_CLOSE;
return HTTP_BAD_REQUEST;
break;
default :
/* allow through */
break;
Expand Down
2 changes: 1 addition & 1 deletion apache2/msc_reqbody.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,7 +715,7 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
msr->msc_reqbody_error = 1;
msr->msc_reqbody_error_msg = *error_msg;
msr_log(msr, 2, "%s", *error_msg);
return -1;
return -8;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  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.
    '

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ,

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@alxyzc alxyzc Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

}
#else
*error_msg = apr_psprintf(msr->mp, "JSON support was not enabled");
Expand Down