Add opt-in Windows FIM **check_acl** / CHECK_ACL so agents can detect NTFS DACL/ACE changes and raise integrity alerts with human-readable permission detail. - #2285
Merged
Conversation
Track GetFileAttributes in an optional sum field so attribute flips alert (ossec#1352), with realtime attribute notify and human-readable decoder text.
Track NTFS discretionary ACL changes with SID-stable digests, inheritance flags, and ACE-level diffs, without enabling ACL via check_all.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends OSSEC’s Windows File Integrity Monitoring (syscheck) to optionally track NTFS ACL (DACL/ACE) changes and Windows file attribute changes, integrating them into the existing checksum/sum-field format and alerting pipeline (including human-readable “what changed” details).
Changes:
- Adds Windows ACL canonicalization/digesting + diff formatting helpers and wires them into baseline creation and realtime/scheduled change alerts.
- Extends sum parsing/comparison utilities to support optional
attrsandaclfields while ignoring local-cache snapshot data after\n. - Updates the manager-side syscheck decoder to surface Windows attribute change details, and adds unit tests for the new helper code.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/syscheckd/run_realtime.c | Uses newline-agnostic sum comparisons and optionally appends ACL diff text to realtime alerts; expands Windows notify mask to include attributes. |
| src/syscheckd/run_check.c | Generates extended sums including optional attrs and ACL digest + snapshot for local cache; fixes LocalFree(pSD) ownership SD handling. |
| src/syscheckd/create_db.c | Builds baseline entries with optional attrs/ACL fields and stores ACL snapshots in local cache; upgrades legacy cache formats when enabling attrs/ACL. |
| src/shared/win_acl_op.c | New Windows ACL read/canonical snapshot/digest/diff implementation. |
| src/shared/tests/win_acl_test.c | New unit tests for ACL digest/snapshot/diff logic. |
| src/shared/tests/Makefile | Adds win_acl_test to the auto test binaries. |
| src/shared/tests/fim_sum_test.c | Extends tests for new sum formats, equality, and attrs formatting. |
| src/shared/fim_sum_op.c | Adds attrs/ACL-aware sum parsing/equality/field extraction and Windows attribute name formatting. |
| src/headers/win_acl_op.h | New public header for ACL helper APIs and data structures. |
| src/headers/fim_sum_op.h | Updates header comments and adds declarations for new sum helpers + attrs formatting. |
| src/config/syscheck-config.h | Adds CHECK_ATTRS and CHECK_ACL option bits. |
| src/config/syscheck-config.c | Parses check_attrs/check_acl directory attributes and renders them via syscheck_opts2str. |
| src/analysisd/decoders/syscheck.c | Parses optional attrs/ACL fields and includes Windows attribute-change details in the alert message. |
| CHANGELOG.md | Documents new opt-in Windows FIM attrs and ACL capabilities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+668
to
+674
| if (!ConvertSidToStringSidA(sid, &sid_str) || !sid_str) { | ||
| continue; | ||
| } | ||
| snprintf(ace.sid, sizeof(ace.sid), "%s", sid_str); | ||
| LocalFree(sid_str); | ||
| ace.ord = (unsigned int)n; | ||
|
|
Comment on lines
+339
to
+363
| int fim_win_acl_digest(const fim_acl_t *acl, char *md5_hex33) | ||
| { | ||
| char snap[8192]; | ||
| os_md5 md5; | ||
|
|
||
| if (!acl || !md5_hex33) { | ||
| return (-1); | ||
| } | ||
| if (acl->special == FIM_ACL_NODACL) { | ||
| OS_MD5_Str("NODACL", md5); | ||
| memcpy(md5_hex33, md5, 33); | ||
| return (0); | ||
| } | ||
| if (acl->special == FIM_ACL_NULLDACL) { | ||
| OS_MD5_Str("NULLDACL", md5); | ||
| memcpy(md5_hex33, md5, 33); | ||
| return (0); | ||
| } | ||
| if (fim_win_acl_snapshot(acl, snap, sizeof(snap)) != 0) { | ||
| return (-1); | ||
| } | ||
| OS_MD5_Str(snap, md5); | ||
| memcpy(md5_hex33, md5, 33); | ||
| return (0); | ||
| } |
Comment on lines
378
to
383
| rc = ReadDirectoryChangesW(rtlocald->h, | ||
| rtlocald->buffer, | ||
| sizeof(rtlocald->buffer) / sizeof(TCHAR), | ||
| TRUE, | ||
| FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_DIR_NAME | FILE_NOTIFY_CHANGE_SIZE | FILE_NOTIFY_CHANGE_LAST_WRITE | FILE_NOTIFY_CHANGE_SECURITY, | ||
| FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_DIR_NAME | FILE_NOTIFY_CHANGE_SIZE | FILE_NOTIFY_CHANGE_LAST_WRITE | FILE_NOTIFY_CHANGE_SECURITY | FILE_NOTIFY_CHANGE_ATTRIBUTES, | ||
| 0, |
Resolve CHANGELOG.md by keeping Windows FIM check_attrs/check_acl entries alongside ModSecurity and syscheck_control entries from main.
Split attrs/acl sum fields independently, use original DACL indexes for ACE order, digest from a non-truncated ACL snapshot, and gate attribute realtime notifies on check_attrs.
Keep files monitored when ACL/attrs reads fail, honor current directory opts over sticky cache flags, digest opaque ACE types, store full ACL snapshots for diffs, and surface ACL digest changes in alerts.
Distinguish configured dirs with zero opts from no match, and only fall back to sticky cache flags when unmatched. Gate attrs-flag rebuild on sum_off when ACL forces the ninth field.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/syscheckd/run_realtime.c:138
memcpy(updated, buf, 7)corrupts legacy 6-flag syscheck cache entries by copying the first digit of the size field into the flag prefix. This can break parsing (fim_sum_data_offset) and produce invalid cached sums after an update. The copy should respectsum_offand explicitly initialize the missing sha256 flag when upgrading legacy entries.
os_calloc((size_t)nflags + strlen(c_sum) + 1, sizeof(char), updated);
memcpy(updated, buf, 7);
if (nflags >= 8) {
updated[7] = (fields >= 8) ? '+' : '-';
/* When only ACL forced the attrs slot, keep the slot
src/syscheckd/create_db.c:606
memcpy(updated, buf, 7)can corrupt legacy 6-flag syscheck cache entries by copying the first digit of the size field into the flag prefix. This breaks the on-disk cache format and can make subsequentfim_sum_data_offset()/ comparisons misbehave. The prefix copy should respectsum_offand explicitly initialize the sha256 flag when upgrading legacy entries.
os_calloc((size_t)nflags + strlen(c_sum) + 1, sizeof(char), updated);
memcpy(updated, buf, 7);
if (nflags >= 8) {
updated[7] = (fields >= 8) ? '+' : '-';
/* When only ACL forced the attrs slot, keep the slot
src/syscheckd/run_realtime.c:79
- The
sum_off >= 9guard prevents ACL appendix/diff text from being emitted when ACL tracking is newly enabled (or when the cache entry is still in 7/8-flag format).fim_win_acl_change_text()already checks whether the new entry contains an ACL digest/snapshot, so this guard can incorrectly suppress the first full-matrix report and later diffs.
This issue also appears on line 134 of the same file.
if (sum_off >= 9 &&
fim_win_acl_change_text(buf + sum_off, c_sum,
acl_txt, (size_t)OS_MAXSTR + 1) > 0) {
snprintf(alert_msg, OS_MAXSTR, "%s %s\n%s",
sum_only, file_name, acl_txt);
src/syscheckd/run_check.c:383
syscheck_opts_for_path()does a strict prefix match againstsyscheck.dir[i], but it doesn't normalize configured directory strings. If a monitored directory is configured with a trailing '/' or '\' (common in configs),next = path[len]will be a normal character and the function will incorrectly treat it as a non-match, causingcheck_attrs/check_aclresolution to fail for those paths.
for (i = 0; syscheck.dir[i]; i++) {
size_t len = strlen(syscheck.dir[i]);
char next;
if (len == 0 || len < best_len) {
src/syscheckd/create_db.c:548
- The
sum_off >= 9guard can suppress ACL appendix/diff text when the local cache entry is still in 7/8-flag format (e.g., first run after enablingcheck_acl).fim_win_acl_change_text()already returns 0 unless the new sum contains an ACL digest/snapshot and the digest changed, so the extra guard can prevent expected ACL detail from being attached to alerts.
This issue also appears on line 602 of the same file.
if (sum_off >= 9 &&
fim_win_acl_change_text(buf + sum_off, c_sum,
acl_txt, (size_t)OS_MAXSTR + 1) > 0) {
snprintf(alert_msg, OS_MAXSTR, "%s %s\n%s",
sum_only, file_name, acl_txt);
Respect sum_off when healing flag prefixes, drop the sum_off ACL appendix gate, and normalize trailing separators in directory opts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
check_acl="yes|no"(parsed on all platforms; effective on Windows). Not included incheck_all, so upgrades do not force a mass re-baseline.check_aclis on, the attrs sum slot is always present (0ifcheck_attrsis off) so field order stays…:sha256:attrs:acl.\nin the payload, truncated if needed.NODACL/NULLDACL).\nfor diffs; manager DB stores digest only.FILE_NOTIFY_CHANGE_SECURITY— no mask change required.LocalFreeof the owner security descriptor on the Windows FIM path.ossec_config.syschecksyntax (separate docs PR).check_attrs(How to monitor Windows hidden files? #1352 /fix/1352-windows-fim-attrs). Prefer that as the PR base until attrs lands onmain.