fix: integer type limits

[airween]

what

This PR fixes the handling of integer type limits, eg. SecRequestBodyNoFilesLimit.

Changes:

• in headers/modsecurity/rules_set_properties.h I implemented a new template with name ConfigValue
• this implements the parse() method which reads safely the integer value from a string, which parsed by engine's SecLang parser (Bison)
• the template also sets the default min and max value to 0
• this method checks the type limits, eg. if the type (for eg. ConfigInt) implements signed integer, then the min and max limits will be -32767 and +32767 (INT_MIN and INT_MAX)
• out-of-bounds values will trigger an error at startup with a message in error.log
• with help of this template I implemented three types: ConfigInt, ConfigUnsignedInt and ConfigUnsignedLong
• it's enough to set the min and max values in all new types (eg. if we want to add new integer types) - see existing new types; I used std::numeric_limits<T>::min() and std::numeric_limits<T>::max()
• I changed the type of variables: m_argumentsLimit, m_requestBodyJsonDepthLimit, m_requestBodyLimit, m_requestBodyNoFilesLimit, m_responseBodyLimit, m_pcreMatchLimit, m_uploadFileLimit and m_uploadFileMode; the current types are here; now there is no more ConfigDouble type variable
• in src/parser/seclang-parser.yy I changed the type in grammar parser; there you can see the error handling (detailed error messages are in template's parse() method)
• I also regenerated src/parser/seclang-parser.cc (it's a generated file by Bison, don't need to check it)

Other changes:

• I fixed some SonarCloud issues in case of ConfigDouble, ConfigString and ConfigSet implementations (also in headers/modsecurity/rules_set_properties.h)
• in top of headers/modsecurity/rules_set_properties.h I had to add undefining macro directives if the target is WIN32, because the CI workflow was unsuccessful on Windows, because - probably - min and max are Windows C++ macros

why

The problem with current solution is that the type of these variables are ConfigDouble, see the variable declarations and their type implementation.

A quick review of current implementation:

• the type of variables were double before this patch - this makes no sense, there is no 1234.6 bytes...
• the type of variables were signed - also makes sense, there is no negative value
• the parser converted the read value with atoi(), which gives an integer with different bitsize

Therefore if a user gave an extra high value, then the atoi() converted it into a negative value.

Previously I sent another PR (#3419), but I realized that solution wasn't complete. In this PR I reworked the whole storage implementation.

references

Fixes #3352 and #3356.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[airween]

Okay, I think I'm done with this.

First: @jonathansmith9485, @SonNgo2211, @EsadCetiner - please take a review this.

Please read the PR's description carefully, eg. if you set an out-of-bound value, then the engine will drop an error and won't start.

If you could make a test, please try to decrease the limit and check the limit works as you expected too.

Please note, that this PR fixes other similar issues too, eg. SecArgumentsLimit - but those values aren't too big typically, like SecRequestBodyNoFilesLimit and SecRequestBodyLimit. So, any help are welcome with wide tests.

[SonNgo2211]

Hi @airween,
Thank you so much for your great work on this fix! I really appreciate your effort. I plan to try this fix in our production environment soon and will provide feedback if needed.
Also, I wanted to ask if it’s possible to increase the hard limits for SecRequestBodyNoFilesLimit and SecRequestBodyLimit?
According to the ModSecurity 3.x documentation I’ve seen, the current hard limits are set at 1GB, and I’m wondering if those can be increased safely or configured for larger values.
Looking forward to your advice. Thanks again!

[airween]

Hi @SonNgo2211,

Also, I wanted to ask if it’s possible to increase the hard limits for SecRequestBodyNoFilesLimit and SecRequestBodyLimit?
According to the ModSecurity 3.x documentation I’ve seen, the current hard limits are set at 1GB, and I’m wondering if those can be increased safely or configured for larger values.

Thanks for bringing this up. Unfortunately that's a false information. I assume the author just copied the v2 documentation and left as is in v3.

In v3 (libmodsecurity3) there isn't any hardcoded limit at all. I have to fix this in the mentioned documentation.

[EsadCetiner]

@airween I just tested the PR and I'm still observing the same behavior, however unlike your previous PR, the directive is now overflowing instead of being set to 0

[175335928862.517382] [/remote.php/dav/files/esadc/] [5] Request body excluding files is bigger than the maximum expected. Limit: -2147483648.000000

Please read the PR's description carefully, eg. if you set an out-of-bound value, then the engine will drop an error and won't start.

What values are considered out of bounds? I have SecRequestBodyNoFilesLimit set to 10737418240 and I didn't get any errors when restarting NGINX

[airween]

I just tested the PR and I'm still observing the same behavior, however unlike your previous PR, the directive is now overflowing instead of being set to 0

[175335928862.517382] [/remote.php/dav/files/esadc/] [5] Request body excluding files is bigger than the maximum expected. Limit: -2147483648.000000

Hmmm... sorry to ask, but are you sure you pulled down the correct branch and tested that code?

I tried with your settings:

$ grep ^SecRequestBodyNoFilesLimit /etc/nginx/modsecurity.conf
SecRequestBodyNoFilesLimit 10737418240

and send your original request:

curl -v -d "test=hello" localhost

Then I see only these lines in debug.log:

[175336092253.135496] [/] [4] Starting phase REQUEST_BODY. (SecRules 2)
[175336092253.135496] [/] [4] Adding request argument (POST): name "test", value "hello"

With the original code I got the same result as you.

What values are considered out of bounds? I have SecRequestBodyNoFilesLimit set to 10737418240 and I didn't get any errors when restarting NGINX

The type of this variable is unsigned long, which means the maximum value on 64 bit systems is 18446744073709551615. The usable interval is between 0 and this value - all other values are out of bounds.

Could you build and run this C++ code on your machine? If yes, what do you get?

// build: g++ ulonglimit.cpp -o ulonglimit
#include <limits>
#include <iostream>

int main() {
    std::cout << std::numeric_limits<unsigned long>::max() << std::endl;
    return 0;
}

My result:

$ ./ulonglimit 
18446744073709551615

It's very-very weird that you get this message: Limit: -2147483648.000000. This means your code still uses the old convert method, and the type of this variable is still a signed type - otherwise it couldn't be a negative value. I thought you use a 32 bit system, and unsigned long is a less bit-size type, but in that case there wouldn't be a negative value with this patch.

[EsadCetiner]

@airween

Hmmm... sorry to ask, but are you sure you pulled down the correct branch and tested that code?

I pulled this branch down git clone airween/ModSecurity --branch v3/secreqbodylimit

I'm on a 64 bit system, so I'm not hitting the limit

./ulonglimit
18446744073709551615

This means your code still uses the old convert method, and the type of this variable is still a signed type

That shouldn't be the case since I removed the old binaries installed via apt, I'll try compiling it in a completely fresh environment, play with it a bit there, then report back.

[airween]

I pulled this branch down git clone airween/ModSecurity --branch v3/secreqbodylimit

cool,

I'm on a 64 bit system, so I'm not hitting the limit

./ulonglimit
18446744073709551615

okay, thanks,

I removed the old binaries installed via apt

Please note that if you use libmodsecurity3 through APT then the installed Nginx module looks the library under /lib/x86_64-linux-gnu/

$ ldd /usr/lib/nginx/modules/ngx_http_modsecurity_module.so | grep modsecurity
	libmodsecurity.so.3 => /lib/x86_64-linux-gnu/libmodsecurity.so.3 (0x00007fe5a473f000)

If you don't set the path before you run ./configure, then the new library will be installed under /usr/local/, and the Nginx module will use the old one. Or remove the current symlink (/lib/x86_64-linux-gnu/libmodsecurity.so.3 is a symbolic link) and create a new one to your new .so file.

Anyway, now I see I have to change the types, because on 32 bit systems the unsigned long is just a 32 bit wide.

[EsadCetiner]

@airween

Please note that if you use libmodsecurity3 through APT then the installed Nginx module looks the library under /lib/x86_64-linux-gnu/

Thanks for that hint, I ran apt remove instead of apt autopurge which removes all traces of the packages. I've been testing it for a few minutes and everything is working as it should, the problem was me😅.

I set a stupidly large limit for SecRequestsBodyNoFilesLimit and I'm getting an error when running nginx -t, so I know for sure I'm using the right binaries.

[airween]

Thanks for that hint, I ran apt remove instead of apt autopurge which removes all traces of the packages. I've been testing it for a few minutes and everything is working as it should, the problem was me😅.

no worries, it's a very good news that it works as well.

I set a stupidly large limit for SecRequestsBodyNoFilesLimit and I'm getting an error when running nginx -t, so I know for sure I'm using the right binaries.

Cool.

Meanwhile I realized that I (and the previous authors) used long and int types, but the problem is that these types width are different on different architectures. Now I'm closing this PR and opened a new one where I use int64_t and uint64_t - which are the same on different architectures.

I'll ask a help again - thanks again.