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

Improve Windows permissions compare in FIM #9765

Merged
merged 15 commits into from Nov 12, 2021

Conversation

jotacarma90
Copy link
Member

@jotacarma90 jotacarma90 commented Aug 19, 2021

Related issue
#8575

Description

This PR closes the epic issue in which we solve a false positive related to how FIM compares Windows ACL permissions.
The problem occurs when a lookup operation on a SID fails, then FIM reports a change in the file.
Finally, the solution has been to change the format in which we stored the Windows permissions, from a string to a JSON. We have also improved the way they were compared, to avoid false positives, so that changes to the file are only reported when the content of ACL changes (and not when only the username has been changed)

This PR unifies multiple issues and PRs:

Issue PR
Load FIM inventory permissions as JSON -
Adapt syscheck event validator on permission format change wazuh/wazuh-qa#1541
Improve FIM Windows permissions check #9249
Refactor FIM message to hold Windows permissions as JSON #9162
Format Windows permissions in FIM inventory -

Tests

  • Compilation without warnings in every supported platform
    • Linux
    • Windows
  • Source installation
  • Review logs syntax and correct language
  • Memory tests for Linux
    • Scan-build report
    • Coverity
    • Valgrind (memcheck and descriptor leaks check)
    • AddressSanitizer
    • threadSanitizer
  • Memory tests for Windows
    • Scan-build report
    • Coverity

Copy link
Contributor

@JoseAntonioMHerrera JoseAntonioMHerrera left a comment

Choose a reason for hiding this comment

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

LGTM!

@jotacarma90 jotacarma90 marked this pull request as ready for review August 24, 2021 08:18
jotacarma90 and others added 12 commits October 6, 2021 16:22
Refactors functions related to retrieving permissions in Windows in order to simplify usage and remove duplicated code.
Added some more UTs for changes associated with Windows permissions being in JSON format
Fix NULL derreferences in decode_ace_json and perm_json_to_old_format.
Fix a memory leak in fim_registry_get_key_data.
Silenced a false positive NULL derreference on lf->fields reported by scan-build.
Added the required code for wazuh_db to properly save a file/registry permissions when it is formatted as JSON.
Added unit tests for the new code.
Makes comparison of Windows permissions independent of the user name associated with a given ACE.
If AD fails to solve a SID to a user name, prevent FIM from triggering alerts.
* Try to load FIM perm field as JSON

* Update core/syscheck unit tests
@chemamartinez
Copy link
Contributor

chemamartinez commented Nov 10, 2021

Found several memory issues reported by valgrind while running the added unit tests:

test_analysisd_syscheck

==22754== 400 (96 direct, 304 indirect) bytes in 1 blocks are definitely lost in loss record 6 of 10
==22754==    at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==22754==    by 0x3721DB: OSHash_Create (hash_op.c:28)
==22754==    by 0x1E88A4: fim_init (syscheck.c:141)
==22754==    by 0x1AAF86: setup_fim_data_win_perms (test_analysisd_syscheck.c:394)
==22754==    by 0x50E5422: cmocka_run_one_test_or_fixture (in /usr/local/lib/libcmocka.so.0.7.0)
==22754==    by 0x50E6335: _cmocka_run_group_tests (in /usr/local/lib/libcmocka.so.0.7.0)
==22754==    by 0x1C38BD: main (test_analysisd_syscheck.c:3796)

test_syscheck_op

Conditional jumps

==22809== Conditional jump or move depends on uninitialised value(s)
==22809==    at 0x496A4E3: cJSON_CreateNumber (in /var/ossec/lib/libwazuhext.so)
==22809==    by 0x1B9BE5: test_decode_win_acl_json_multiple_aces (test_syscheck_op.c:2449)
==22809==    by 0x50E5322: cmocka_run_one_test_or_fixture (in /usr/local/lib/libcmocka.so.0.7.0)
==22809==    by 0x50E6366: _cmocka_run_group_tests (in /usr/local/lib/libcmocka.so.0.7.0)
==22809==    by 0x1BFE4A: main (test_syscheck_op.c:4486)
==22809==  Uninitialised value was created by a stack allocation
==22809==    at 0x1B9970: test_decode_win_acl_json_multiple_aces (test_syscheck_op.c:2414)

Memory leaks

==22809== 64 bytes in 1 blocks are definitely lost in loss record 1 of 2
==22809==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==22809==    by 0x4968B7D: cJSON_New_Item.isra.2 (in /var/ossec/lib/libwazuhext.so)
==22809==    by 0x496A82F: cJSON_CreateObject (in /var/ossec/lib/libwazuhext.so)
==22809==    by 0x1B96B1: test_decode_win_acl_json_empty_acl (test_syscheck_op.c:2380)
==22809==    by 0x50E5322: cmocka_run_one_test_or_fixture (in /usr/local/lib/libcmocka.so.0.7.0)
==22809==    by 0x50E6366: _cmocka_run_group_tests (in /usr/local/lib/libcmocka.so.0.7.0)
==22809==    by 0x1BFE4A: main (test_syscheck_op.c:4486)
==22809==
==22809== 64 bytes in 1 blocks are definitely lost in loss record 2 of 2
==22809==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==22809==    by 0x4968B7D: cJSON_New_Item.isra.2 (in /var/ossec/lib/libwazuhext.so)
==22809==    by 0x496A82F: cJSON_CreateObject (in /var/ossec/lib/libwazuhext.so)
==22809==    by 0x1B9A10: test_decode_win_acl_json_multiple_aces (test_syscheck_op.c:2430)
==22809==    by 0x50E5322: cmocka_run_one_test_or_fixture (in /usr/local/lib/libcmocka.so.0.7.0)
==22809==    by 0x50E6366: _cmocka_run_group_tests (in /usr/local/lib/libcmocka.so.0.7.0)
==22809==    by 0x1BFE4A: main (test_syscheck_op.c:4486)

Apart from the ones appearing in the master branch which are already solved at #9099. The ones above are newly added so they have to be fixed before merging this pull request.

Update

Fixed at b77bafe

chemamartinez
chemamartinez previously approved these changes Nov 10, 2021
@chemamartinez chemamartinez merged commit 3f505e8 into master Nov 12, 2021
@chemamartinez chemamartinez deleted the 8575-fim-win-perms-improvement branch November 12, 2021 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FIM alerts of owner change when LookupAccountSid fails
5 participants