Do not show diff

[calve]

Sensitive data may leak through the diff attached to alerts when some file changes. This pull request add a nodiff option, which allows to explicitely set files for which we never want to output a diff.

---

This change is

[jrossi]

This is great and a very useful feature. While we review could add this to the docs: https://github.com/ossec/ossec-docs by creating a pull request?

[jrossi]

Think this could be more descriptive with something like Diff truncated due to nodiff

[jrossi]

Also is printf always present on all supported platforms. I really hate shelling out for this use case it's pointless outside of integrating into the reset of the function.

[calve]

Are you suggesting to directly fwrite() to the file ?

[jrossi]

Nope, sorry for not being clear. Given the code this is sane solution.

[jrossi]

Also it's not clear that function is_nodiff checks the osman chest if that was used.

[jrossi]

One thing that I don't see is how OSmatch is ever used. The is_nodiff function only processes the list of files but does not process the OSmatches that are created via the Conf/xml

[jrossi]

Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

src/config/syscheck-config.c, line 714 [r1] (raw file):
This can fail:

If the function fails, the return value is zero. To get extended error information, call GetLastError.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms724265(v=vs.85).aspx

What happens if it fails?

---

src/syscheckd/seechanges.c, line 52 [r1] (raw file):
Yeah more closer review shows no regex/osmatch testing is done. Could we get this added or remove the feature from the config section.

---

src/syscheckd/syscheck.c, line 144 [r1] (raw file):
We try to use {} even if not required as it leads to problems later (sometimes not always).

---

src/syscheckd/syscheck.c, line 326 [r1] (raw file):
We try to use {} even if not required as it leads to problems later (sometimes not always).

---

Comments from the review on Reviewable.io

[calve]

Thanks for the review. I updated the code according to your suggestions. I added the regex match feature.

As for ExpandEnvironmentStrings() failures, I have absolutely no idea what to do. The rest of the code base does not seems to check the return code either. I would suggest to drop that Windows-only block, as diff are not runned on these systems (per the documentation). On the other side, maybe someone will port the feature some days.

[jrossi]

For ExpandEnvironmentStrings good point I did not check on that. Thank you.

---

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from the review on Reviewable.io

[jrossi]

@calve Could you add docs github.com/ossec/ossec-docs ? Also add yourself as contributor?

Thank you again great stuff and something I think others will use.

[calve]

Hi guys,

isn't it worth assigning a CVE under Information Leakage (https://cwe.mitre.org/data/definitions/200.html) to spread the fact that sensible informations might have leaked over email ? cc @ddpbsd