fix: integer type limits, 2nd try

[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 int32_t (signed int on AMD64), then the min and max limits will be -2147483648 and +2147483647
  • if you take a look at this minimum value, it can be familiar with the mentioned log message in #3356: Request body excluding files is bigger than the maximum expected. Limit: -2147483648.000000; the reason is (I write that below too) that the current implementation uses atoi() function, which converts a string to signed int, but if the value is bigger than the max value above, then it overflows and casts to int's minimum
• 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 (int32_t), ConfigUnsignedInt (uint32_t) and ConfigUnsignedLong (uint64_t)
• 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

How does it work
The most important part is the new parse() method. This decides what integer type is used in the derived class (signed/unsigned), and uses the necessary function (stoll() / stoull()). Then it checks that the conversion was success with the biggest sized integer. If not, then the errno will be 1 and the value will be the max/min value of used type (int64_t / uin64_t). There is another check: does the parsed value fit into the chosen type (eg. int32_t). If any condition matches, then the method returns with false at startup time.

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. Then I sent a new one (#3420), but there I continued to use int and unsigned long types, but I realized those are not the same on different platforms (amd64 vs i386). In this PR I changed the types with fixed width types.

references

Fixes #3352 and #3356.

[airween]

@jonathansmith9485, @SonNgo2211, @EsadCetiner - here is the new one, please take a review this.

[EsadCetiner]

@airween I've been testing for a day or so and so far I haven't had any issues

[airween]

@airween I've been testing for a day or so and so far I haven't had any issues

Great news, thank you!

[theseion]

Can you elaborate on why this caused #3352 and #3356?

[theseion]

Much better!

[airween]

Can you elaborate on why this caused #3352 and #3356?

I explained in my initial comment:

• if you take a look at this minimum value, it can be familiar with the mentioned log message in #3356: Request body excluding files is bigger than the maximum expected. Limit: -2147483648.000000; the reason is (I write that below too) that the current implementation uses atoi() function, which converts a string to signed int, but if the value is bigger than the max value above, then it overflows and casts to int's minimum

[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