Skip to content

Invalid memory access in parser_conn_limits_operator() #2815

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.

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

Closed
marcstern opened this issue Oct 13, 2022 · 1 comment
Closed

Invalid memory access in parser_conn_limits_operator() #2815

marcstern opened this issue Oct 13, 2022 · 1 comment
Labels
2.x Related to ModSecurity version 2.x

Comments

@marcstern
Copy link

First problem (quite unusual, I admit):

config_orig_path = apr_pstrndup(mp, filename, strlen(filename) - strlen(apr_filepath_name_get(filename)));
apr_filepath_merge(&file, config_orig_path, param, APR_FILEPATH_TRUENAME, mp);

config_orig_path can be NULL, so

config_orig_path = apr_pstrndup(mp, filename, strlen(filename) - strlen(apr_filepath_name_get(filename)));
if (!config_orig_path) {
   return apr_psprintf(mp, "ModSecurity: failed to duplicate filename in parser_conn_limits_operator");
}
apr_filepath_merge(&file, config_orig_path, param, APR_FILEPATH_TRUENAME, mp);

Second problem (I found it in prod, difficult to troubleshoot):

char* param = strchr(p2, ' ');
[...]
param++;

In case we use the SecConnReadStateLimit diective without operator (only a regex), paparm is NULL => memory fault, crash without any message.
Fix:

if (!param && *p2) return apr_psprintf(mp, "ModSecurity: Invalid operator for " \
   "SecConnReadStateLimit: %s, expected operators: @ipMatch, @ipMatchF " \
   "or @ipMatchFromFile with or without !", p2);
param++;
@martinhsv martinhsv added the 2.x Related to ModSecurity version 2.x label Oct 13, 2022
@martinhsv
Copy link
Contributor

Hi Marc,

Thanks for noting these.

Regarding the first of the two, there are more than a few places in v2 where a (freakishly unlikely) failure to allocate memory from a memory pool is not checked. These could perhaps be best handled en masse at some point.

Regarding the second item, just to expand on it a little:

  • the problem input is expressly not supported; although the rx operator is implied in a SecRule directive if an operator is absent, the same is not true for SecConnReadStateLimit. Indeed, the documentation expressly states which three operators are supported and it is only the three (ipMatch, ipMatchF and ipMatchFromFile
  • the failure occurs on startup rather than while running

That doesn't mean this second issue shouldn't be fixed. If an admin creates such an erroneous entry, diagnosing the problem is not at all friendly. I'll tentatively plan to include this in the next 2.9.x release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to ModSecurity version 2.x
Projects
None yet
Development

No branches or pull requests

2 participants