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: Allow clamd to start normally when a whitelist is present. #1309

Conversation

userwiths
Copy link
Contributor

This PR is an attempt to fix issue #1174

The given functionality was introduced in the following commit and after taking a look at the cli_cvdverify function I believe it is meant to verify only CVDs.

That is, it throws an error when encountering a clear text (whitelist, .ign,.ign2) file. I mimicked the way the allowed extensions in the Database folder were handled, and excluded the 2 clear text extensions mentioned above. This has locally allowed for the normal execution of clamd.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

Thank you for working on a solution to this bug.

The same issue occurs when adding individual signature files outside of a CVD or CLD archive. E.g. if you drop in "clamav.hdb" file containing a hash-based signature, then clamd loading will still fail with:


❯ ./install/sbin/clamd -F
LibClamAV Error: cli_cvdverify: Can't read CVD header
LibClamAV Error: cl_cvdgetage: cvdgetfileage() failed for /home/micah/workspace/clamav-micah-5/build/install/share/clamav/clamav.hdb

So rather than having cl_cvdgetage ignore specific extensions like ".ign2", it would be better to only verify for ".cvd" and ".cld".

@userwiths
Copy link
Contributor Author

Yeah, I'm not familiar with all of the extensions so the clarification is more than welcome.
As discussed now the cl_cvdgetage checks only CVD and CLD files.

@userwiths userwiths requested a review from micahsnyder July 16, 2024 15:20
@micahsnyder
Copy link
Contributor

Hi @userwiths I thought of one other issue. If anyone tries to run clamscan where they pass in specific databases rather than using a database directory, it will still fail.

E.g.:

❯ clamscan -d ~/clamav.hdb -d ~/.cvdupdate/database/bytecode.cvd --fail-if-cvd-older-than=5 /path/to/file
LibClamAV Error: cli_cvdverify: Can't read CVD header
ERROR: Broken or not a CVD file

I think to fix this, we'll need to check if the database path ends with ".cvd" or ".cld" in this location: https://github.com/userwiths/clamav/blob/8474e6a86d45ba2b084bd7d3719947db90aa5543/clamscan/manager.c#L1253-L1254

What do you think?

@userwiths
Copy link
Contributor Author

I think that you are correct.
Despite that, I saw a specific scenario for which I took a decision that might be worth a notice.

I noticed that clamscan proceeds without a notice even if a file/directory passed with -d does not exist.
Don't know if that is intentional or not, my current changes exit with an error and log on the screen a message with the path to the missing file.

In case you think it would be better to keep the behavior as it was, just say so and I will change it accordingly.

@micahsnyder
Copy link
Contributor

I noticed that clamscan proceeds without a notice even if a file/directory passed with -d does not exist.

I wasn't able to reproduce this, or else I don't fully understand what you mean. I believe it should fail if the -d file or directory does not exist. So I'm good with your change either way.

Your PR seems good to me. I was doing some local testing after a rebase. I'm going to squash it down to one commit as well and then force-push that.

@micahsnyder micahsnyder force-pushed the issue-1174-FailIfCvdOlderThan-error-on-whitelist branch from ca96f27 to 2c7860f Compare July 23, 2024 17:46
Clamscan and ClamD will throw an error if you use the
'--fail-if-cvd-older-than=DAYS' / 'FailIfCvdOlderThan' option and
try to load any plaintext signature files.
That is, it throws an error when encountering plain signature files like
`.ign2`, `.ldb`, `.hdb`, etc.
This feature should only verify CVD / CLD files.

The feature (and bug) was introduced in ClamAV 1.1.0, here:
Cisco-Talos@e4fe665

With this change, the `cl_cvdgetage` checks will skip any file that is
not a CVD or CLD.

Fixes: Cisco-Talos#1174
@micahsnyder micahsnyder force-pushed the issue-1174-FailIfCvdOlderThan-error-on-whitelist branch from 2c7860f to 9a7b186 Compare July 23, 2024 20:01
@micahsnyder micahsnyder merged commit 6c0d644 into Cisco-Talos:main Jul 23, 2024
23 of 24 checks passed
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.

2 participants