sanitiseMatchedBytes only works with 1 digit parameters

[marcstern]

sanitiseMatchedBytes:5/8 => OK
sanitiseMatchedBytes:10/2 => not accepted => sanitizes the whole string

In msre_action_sanitizeMatchedBytes_init():
if (action->param != NULL && strlen(action->param) == 3)
should be
if (action->param != NULL && strlen(action->param) >= 3)

[zimmerle]

I think @marcstern is trying to highlight something is this piece of code -
https://github.com/SpiderLabs/ModSecurity/blob/12cefbd70f2aab802e1bff53c50786f3b8b89359/apache2/re_actions.c#L445-L454

This piece of code is used by a functionality named sanitaseMatchedBytes; further information available here -
https://github.com/SpiderLabs/ModSecurity/wiki/Reference-Manual-%28v2.x%29#sanitiseMatchedBytes

This function is meant to be used together with operators, for instance: removes the credit card numbers from logging.

SecRule ARGS "@verifyCC \d{13,16}" "phase:2,id:133,nolog,capture,pass,msg:'Potential credit card number in request',sanitiseMatchedBytes"
SecRule RESPONSE_BODY "@verifyCC \d{13,16}" "phase:4,id:134,t:none,log,capture,block,msg:'Potential credit card number is response body',sanitiseMatchedBytes:0/4"

Because it is meant to work with the matched content, as opposed to the entire piece of the request, it uses the size specified with an integer from 0 to 9. @marcstern is seemed to be requested to support for a more "broad" wipe out by supporting infinite number representation.

If my assumptions described here are right, the patch that @marcstern suggests is not correct as it will make it possible for one to specify numbers bigger than an integer.

That numbers are placed in the configuration file (string), later converted from text to number (int) using atoi. atoi output is not being checked as the number is expected to be from 0 to 9. If a number that is bigger than what can be held on an int is placed, it will result in unexpected behavior.

Having said that, why do you think that it is an error? Or why do you think it is important to have such "big" values on that functionality? can you provide us a use case scenario that demands such change?

[zimmerle]

@marcstern ping?

[marcstern]

You're correct about the my request @zimmerle. It would allow to specify "12/36"
You're also correct that it would accept the "xx/y" syntax.
Currently, it checks the syntax and, if incorrect, it uses 0.
You could extend the syntax check to more than 1 character to have the exact same behaviour or you could use strtol() which converts invalid strings to 0 (the version we implemented because it's the easiest one).
The whole function can be simplified:

char* endptr;
actionset->arg_min = (int)strtol(action->param, &endptr, 0);
if (actionset->arg_min < 0 || actionset->arg_min == LONG_MAX || actionset->arg_min == LONG_MIN) actionset->arg_min = 0;
actionset->arg_max = (int)strtol(endptr, NULL, 0);
if (*endptr == '/') {
        actionset->arg_max = (int)strtol(++endptr, NULL, 0);
        if (actionset->arg_max < 0 || actionset->arg_max == LONG_MAX || actionset->arg_max == LONG_MIN) actionset->arg_max = 0;
}
else actionset->arg_max = 0;
return 1;

By using "0" in the strtol radix (instead of 10), we can even use hexa notation (0x12/0X10) - not very useful but it's possible.

[marcstern]

@zimmerle: in case you're waiting for the use case, sorry, I forgot about it.
When you have tokens (like JWT) that are very long, you may want to get something like the first 20 bytes. Another example: a basic auth "Authorization: Basic ..."); with 9 characters, you only get the 3 first characters of the username (and less with Bearer, Digest, etc.).

[marcstern]

Hello.
I don't see any remark.
Could this be accepted?