Skip to content
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

fix: align TIME_MON variable's behavior #3306

Merged
merged 2 commits into from
Nov 24, 2024

Conversation

airween
Copy link
Member

@airween airween commented Nov 22, 2024

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 airween requested a review from marcstern November 22, 2024 09:46
@airween
Copy link
Member Author

airween commented Nov 22, 2024

@M4tteoP could you review this PR? Thanks!

@airween
Copy link
Member Author

airween commented Nov 22, 2024

There is a very interesting check result:

  • in first attempt the cppcheck step showed an error here
  • I re-run that check and then the error disappeared - see here

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
Copy link
Contributor

M4tteoP commented Nov 22, 2024

@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.

Copy link
Contributor

@fzipi fzipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@airween
Copy link
Member Author

airween commented Nov 23, 2024

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:

  • the first one has 14.7.1, which is a newer
  • the second has 14.7 which probably has an older cppcheck

Thanks @gberkes.

@airween airween requested a review from theseion November 23, 2024 09:37
@airween
Copy link
Member Author

airween commented Nov 24, 2024

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

@airween airween merged commit d9101a4 into owasp-modsecurity:v3/master Nov 24, 2024
49 checks passed
@marcstern marcstern added the 3.x Related to ModSecurity version 3.x label Nov 26, 2024
@dune73
Copy link
Member

dune73 commented Nov 26, 2024

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.

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

Successfully merging this pull request may close these issues.

TIME_MON variable: inconsistent ranges between 2.x and 3.x engines.
6 participants