owasp-modsecurity / ModSecurity

ModSecurity is an open source, cross platform web application firewall (WAF) engine for Apache, IIS and Nginx. It has a robust event-based programming language which provides protection from a range of attacks against web applications and allows for HTTP traffic monitoring, logging and real-time analysis.
https://www.modsecurity.org
Apache License 2.0
8.31k stars 1.61k forks source link

fix: align TIME_MON variable's behavior #3306

Closed airween closed 5 days ago

airween commented 1 week ago

what

This PR changes TIME_MON variable's behavior and fixes #3305.

Also fixes the regex in changed variable's test file (the old one allows any number, eg. 0 or 1111 which makes no sense; the new one allows between 1 and 12.).

why

As the issue describes in libmodsecurity3 the TIME_MON variable can be between 0 and 11. mod_security2 uses values between 1 and 12 - which is much more natural.

references

See issue #3305.

airween commented 1 week ago

@M4tteoP could you review this PR? Thanks!

airween commented 1 week ago

There is a very interesting check result:

I tried this on my local environment (with a newer cppcheck (2.16.0)), and that also reports this issue:

$ cppcheck -U YYSTYPE -U MBEDTLS_MD5_ALT -U MBEDTLS_SHA1_ALT -D MS_CPPCHECK_DISABLED_FOR_PARSER -U YY_USER_INIT --suppressions-list=./test/cppcheck_suppressions.txt --inline-suppr --enable=warning,style,performance,portability,unusedFunction,missingInclude --inconclusive --template="warning: {file},{line},{severity},{id},{message}" -I headers -I . -I others -I src -I others/mbedtls/include --error-exitcode=1 -i "src/parser/seclang-parser.cc" -i "src/parser/seclang-scanner.cc" -i others --std=c++17 --force --verbose src/utils/system.cc 
Checking src/utils/system.cc ...
Defines:MS_CPPCHECK_DISABLED_FOR_PARSER=1
Undefines: MBEDTLS_MD5_ALT; MBEDTLS_SHA1_ALT; YYSTYPE; YY_USER_INIT
Includes: -Iheaders/ -I./ -Iothers/ -Isrc/ -Iothers/mbedtls/include/
Platform:native
Checking src/utils/system.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1...
Checking src/utils/system.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;NO_LOGS...
Checking src/utils/system.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;S_IFMT;S_IFREG;S_ISREG...
warning: src/utils/system.cc,213,error,syntaxError,syntax error
Checking src/utils/system.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;WIN32...
Checking src/utils/system.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;_MSC_VER...
Checking src/utils/system.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;__GNUC__...
Checking src/utils/system.cc: MS_CPPCHECK_DISABLED_FOR_PARSER=1;__OpenBSD__...

@eduar-hte do you have any idea?

M4tteoP commented 1 week ago

@M4tteoP could you review this PR? Thanks!

It looks good to me! I just don't know how this change will be handled regarding breaking changes. Rules relying on TIME_MON might require changes after this fix.

airween commented 6 days ago

There is a very interesting check result:

* in first attempt the `cppcheck` step showed an error [here](https://github.com/owasp-modsecurity/ModSecurity/actions/runs/11970218824/job/33372492045#step:5:4039)

* I re-run that check and then the error disappeared - see [here](https://github.com/owasp-modsecurity/ModSecurity/actions/runs/11970218824/job/33374020597#step:5:4038)

Seems like @gberkes solved the myth: cppcheck 2.16 detects this as FP - see #3307.

The reason is why those checks have different results above that they have two different image:

Thanks @gberkes.

airween commented 5 days ago

@theseion could you take a look at it again? I changed the regex, I think now it fills the expectations.

sonarcloud[bot] commented 5 days ago

Quality Gate Passed 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

dune73 commented 3 days ago

Thanks for this fix (and @M4tteoP's report in the first case).

It's a rare, but apparently a breaking change. Please report with care. And glad you settled on the ModSec 2 behavior.

I see the attractiveness of Jan being "0", but it's really counterintuitive since we write dates differently.