Skip to content

sha256 support for wazuh-ossec#410

Merged
TJOSERAFAEL merged 2 commits intowazuh:sha256-fixfrom
arshad01:3.1_sha256
Mar 6, 2018
Merged

sha256 support for wazuh-ossec#410
TJOSERAFAEL merged 2 commits intowazuh:sha256-fixfrom
arshad01:3.1_sha256

Conversation

@arshad01
Copy link
Contributor

@arshad01 arshad01 commented Feb 21, 2018

Hello
Please review my pull request based on 3.1 branch for sha256 signature support. I have followed the current code structure to add the new signature. Hope this is useful.

Added a new attribute "check_sha256sum" with values yes/no in the syscheck's directories option.

Thanks
Arshad

@vikman90
Copy link
Member

Hi @arshad01,

This is an impressive work! SHA256 checking for FIM will add a lot of value to Wazuh.

We don't plan to release 3.1.x versions so I suggest you to change the base branch to master. I have merged manually your branch into master and there are no conflicts.

I see that there are 3 extra commits in your PR, this is because we moved them into the master branch, they won't apply in the merging.

Please let me review your changes carefully before accepting the PR. So far, I've taken a look at your code and I've seen two details, I'll comment you in the code.

Thank you very much for your work. Looking forward to see it working!

Best regards.

@vikman90 vikman90 self-assigned this Feb 22, 2018
Copy link
Member

@vikman90 vikman90 left a comment

Choose a reason for hiding this comment

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

Please consider changing these two issues before merging the PR.

Thanks again.

src/LOCATION Outdated
@@ -1 +1 @@
DIR="/var/ossec"
DIR="/var/ossec31"
Copy link
Member

Choose a reason for hiding this comment

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

Please restore the original value of this parameter. This is the default installation directory, it should still being /var/ossec.

#define SK_GNAME 8
#define SK_INODE 9
#define SK_NFIELDS 10
#define SK_SHA256 11
Copy link
Member

Choose a reason for hiding this comment

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

This definition exceeds the field size limit.

SK_NFIELDS is the number of fields, now there will be 11 fields. This parameter is used to evaluate all fields: https://github.com/wazuh/wazuh/blob/master/src/shared/syscheck_op.c#L104-L106

Please swap SK_NFIELDS and SK_SHA256:

#define SK_SHA256 10
#define SK_NFIELDS 11

@vikman90 vikman90 added the type/enhancement New feature or request label Feb 22, 2018
@arshad01 arshad01 changed the base branch from 3.1 to master February 22, 2018 03:31
@arshad01
Copy link
Contributor Author

Hi @vikman90

Thanks very much for reviewing.

I have changed base branch to master. Please note that I haven't tested this change on Windows so there may be some parts of this PR that need further changes.

Regards
Arshad

@arshad01
Copy link
Contributor Author

Hi @vikman90
I have committed the changes that you requested to PR. Please let me know if anymore changes are needed.
Thanks

@TJOSERAFAEL TJOSERAFAEL requested review from TJOSERAFAEL and removed request for TJOSERAFAEL March 6, 2018 17:38
@TJOSERAFAEL TJOSERAFAEL changed the base branch from master to sha256-fix March 6, 2018 17:43
@TJOSERAFAEL
Copy link
Contributor

First of all thank you for the hard work you did in this PR. We have tested it and it works but as we need retrocompatibility with older versions I have made changes over your code. I will merge this PR and upload the changes I made.

Again thank you for this!

@TJOSERAFAEL TJOSERAFAEL merged commit e5a1d7e into wazuh:sha256-fix Mar 6, 2018
@arshad01
Copy link
Contributor Author

arshad01 commented Mar 7, 2018

Thanks for accepting and merging the pull request.

@vikman90 vikman90 mentioned this pull request Jun 4, 2018
8 tasks
@vikman90 vikman90 added the module/fim File Integrity Monitoring label Jun 19, 2018
@soynof soynof mentioned this pull request Aug 7, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module/fim File Integrity Monitoring type/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants