- Add external FIPS 140-2 / 140-3 compliant signatures for the Clam Databases. #1344
Conversation
…atabases. - Added tools to extract the public exponent and modulus of generated keys so that they internal defines holding the hard coded key validation can be updated by the signing authority. - Added the script sigext/cvd_hash_sha256_sign.sh which uses a private RSA key and openssl to sign ClamAV Databases and produces an external signature file which can then be used to validate the contents of the ClamAV database updates.
|
This pull request is related to the following issues:
This pull request also appears to provide backwards compatibility whereas others have not: Merging this into main would be greatly appreciated as it negatively affects a majority of federal entities around the world. |
|
Hi @mark-carey-sap and @aghassemlouei! This is a nice proposal for a workaround to enable CVD signature signing and verification with FIPS compliance. It is prompting some discussion in our team. We'll discuss it some more and get back to you. |
There was a problem hiding this comment.
Hi @mark-carey-sap and @aghassemlouei!
This is a nice proposal for a workaround to enable CVD signature signing and verification with FIPS compliance. It is prompting some discussion in our team. We'll discuss it some more and get back to you.
Thank you! I'm happy to help fix this. I know a lot of folks are bound by FIPS regulatory standards and really, really prefere ClamAV over all other solutions. Including me!
|
While this seems to solve the FIPS concerns now, it does not provide crypto agility, so for the next transition that's coming up (use post-quantum safe cryptography), we'll end up in the same corner. Can ClamAV not use a signature format that provides cryptographic agility continues to be maintained? Maybe re-use sigstore? |
|
Nice input @neverpanic! The goal with this pull request is to address the immediate issue with clamav not functioning on FIPS enabled operating systems such as RHEL 9 or Ubuntu 20.04 which has a massive impact on the entire security industry, potentially negatively impacting the use of ClamAV and further adoption. We are more than happy to collaborate on post-quantum and safe cryptographic approaches, but right now we have a large set of non-functional systems. I'll let @mark-carey-sap either chime in or push a change since it's their pull request after all! |
…ncludes a demo of post-quantum encryption using the open-quantum-safe project providers for OpenSSL v3 and later. You must have liboqs (https://github.com/open-quantum-safe/liboqs) installed, and the oqsprovider for OpenSSL v3 and later (https://github.com/open-quantum-safe/oqs-provider). Please note that you will also need to have altered the openssl.cnf file to include the oqsprovider in your configuration. Instructions are on the oqsprovider page. I have tested dilithium2 quantum resistant signatures and have included the source in a way that it will not cause faults if the provider is not installed. It should be noted that at the present time NIST has NOT approved dilithium2 for use in FIPS systems. I have put hard blockers on MD5 and SHA1 when running in FIPS mode to prevent mishaps. The signing bash script will now hash and sign files in a flexible manner. Algorithms support for SHA256 and SHA3-256 have been added to the existing hashing functions of the library. I have validated functionality and ran valgrind against this commit. I can find no bugs, but would always love to have a second or third set of eyes to help spot what I missed. Shell script name changed to be more generic in the spirit of supporting arbitrary hashing and signing algorithms. Script is now called: clamav/sigext/cvd_ext_sigh.sh If needed, I can add Elliptical Curve keys and signatures, as well, but it should be fairly elementary for anyone to do so now.
|
@neverpanic I agree. I have submitted an additional commit that adds full cryptographic agility in the form of hashing and signing algorithm selectors in the external signature file. I have implemented (and tested) several hashing algorithms (md5, sha1, sha2-256, sha3-512), and two signing algorithms (RSA, and Dilithium2 - from the oqsprovider). While NIST does not currently support quantum resistant algorithms, we will be ready when they do. @micahsnyder I think this should add a bit more of a complete patch set to allow you to move in whichever direction you wish to on the hashing and signature algorithms, and should be easily extendable to prevent painting ourselves into a corner later down the road. I hope this helps and is a good contribution. I also wanted to note that if using the OpenSSL version 1.1.x of the library, I have elected to leave only RSA signatures enabled. This is due to the lack of provider module functionality in that series of libraries. I think this makes sense, but please tell me if I still have work to do. Thank you! |
| md = EVP_get_digestbyname(alg); | ||
| // If OpenSSL is running in FIPS mode, we need to use the FIPS-compliant md5 lookup of the algorithm | ||
| if (cli_fips_mode && !strncasecmp(alg, "md5", 3)) | ||
| md = EVP_MD_fetch(NULL, alg, "-fips"); |
There was a problem hiding this comment.
I know this PR is much bigger than #959, but this section of code appears to be the same as that PR. Is this introducing the same issues as that, and would this have the same potential memory leak mentioned on that PR? I didn't see any added calls to EVP_MD_free in this PR.
There was a problem hiding this comment.
Hi David! So per the OpenSSL documentation, we don't really need to worry about freeing this. The first element of the OpenSSL documentation I want to cite is:
- EVP_MD_free()
Decrements the reference count for the fetched EVP_MD structure. If the reference count drops to 0 then the structure is freed. If the argument is NULL, nothing is done.
So, we're really dealing with a reference count. While this means it's not really an issue, I also like to be tidy with reference counts, but that leads to the second section of documentation I wanted to highlight:
EVP_MD_fetch()
Fetches the digest implementation for the given algorithm from any provider offering it, within the criteria given by the properties. See "ALGORITHM FETCHING" in crypto(7) for further information.
The returned value must eventually be freed with EVP_MD_free().
Fetched EVP_MD structures are reference counted.
So, we should free when using EVP_MD_fetch. However we also have:
EVP_get_digestbyname(), EVP_get_digestbynid(), EVP_get_digestbyobj()
Returns an EVP_MD structure when passed a digest name, a digest NID or an ASN1_OBJECT structure respectively.
The EVP_get_digestbyname() function is present for backwards compatibility with OpenSSL prior to version 3 and is different to the EVP_MD_fetch() function since it does not attempt to "fetch" an implementation of the cipher. Additionally, it only knows about digests that are built-in to OpenSSL and have an associated NID. Similarly EVP_get_digestbynid() and EVP_get_digestbyobj() also return objects without an associated implementation.
When the digest objects returned by these functions are used (such as in a call to EVP_DigestInit_ex()) an implementation of the digest will be implicitly fetched from the loaded providers. This fetch could fail if no suitable implementation is available. Use EVP_MD_fetch() instead to explicitly fetch the algorithm and an associated implementation from a provider.
See "ALGORITHM FETCHING" in crypto(7) for more information about fetching.
The digest objects returned from these functions do not need to be freed with EVP_MD_free().
So, now we do not have to free when using EVP_get_digestbyname. Terribly confusing. That said, I've added the logic to free according to the documentation. Please see the updated commit.
I also added ECC secp521r1 support for NIST Approved Elliptical Curve signing, as well.
There was a problem hiding this comment.
Thanks! I appreciate the detailed explanation. I agree, that documentation seems inconsistent with itself.
|
I was advised to write a general comment about the architecture and code of this pull request to outline how it works, why the design decisions were made along the way, and what it means for FIPS 140-2/3 compliance. Tagging folks for visibility: @micahsnyder @neverpanic @dkontyko @aghassemlouei Architecture:The objective of this patch was to add a FIPS compliant signature method to ClamAV. The reason behind this is that a great many organizations who would much prefer to use ClamAV over other solutions are not able to due to compliance regulations that govern their software use. Since the legacy header is only 512 bytes long, and we can not maintain backwards compatibility with the installed base if we add a 470 byte hash and signature to the existing header (as well as blocking cryptographic agility in the future), I chose to make an external signature file. The format is simple, I chose to add a few different signature types for OpenSSL library versions beyond 3.0. Versions on OpenSSL 1.x.x are left with only RSA support due to the lack of provider support. The signature types today are RSA (FIPS demands 2048 bit keys), Dilithium2 (for an example of post quantum resistant signatures), and Elliptical Curve (specifically NIST approved secp521r1). These should prove enough to get started. The code will select the algorithm based on the integer value in the field of the signature. It's important to note that we continue to use MD5 to validate the database's internal hash, but the external signature takes precedence. This occurs ONLY if FIPS mode is selected by the presence of OpenSSL's FIPS provider. It is also vital to note that for FIPS compliance, the only algorithms supported at this time (since NIST has not ruled on post quantum resistant cryptographic algorithms as of this patch date) are RSA and EC secp521a. Notes about the code:The signature, key generation, and key value extraction for inclusion of the definitions for embedding the public key constants in the main body of the code are in the The key generations functions are located in Freshclam was modified to check the FIPS compliant signature PRIOR to all other signature validation if Hashing support for SHA3-512 was also added in the same style as the other algorithms. The key for algorithms supported by the signature file: Hashing: Signature: Please note that in order to use the Dilithium2 algorithm ConclusionThis should provide a decent extensible framework for external signatures, several hashing algorithms, a few signature algorithms, and has been validate against various memory protection tools, and code coverage executed for all hashing and signature algorithms. Hopefully this helps clarify the pull request's intent and scope. Thank you! |
| @@ -627,6 +630,14 @@ cl_error_t cli_cvdload(FILE *fs, struct cl_engine *engine, unsigned int *signo, | |||
|
|
|||
| cli_dbgmsg("in cli_cvdload()\n"); | |||
|
|
|||
| // FIPS verification MUST preclude other verification if in FIPS mode. | |||
| if (cli_get_fips_mode()) { | |||
| ret = cli_sigver_external(filename); | |||
There was a problem hiding this comment.
This call should not be guarded by cli_fips_mode, external signature file should be preferred if present.
There was a problem hiding this comment.
I concur. If we're going to add a better way to validate signing, it should be preferred. We should only fallback to the legacy signature verification if A. the new-style signature is not present and B. not FIPS-mode.
There was a problem hiding this comment.
Rather than adding comments all over the code to this extent. Every place that guards the validation/fetching of the external signature file with cli_fips_mode should be removed so that the new format is the default and only trusted signature in versions that support it.
|
@mark-carey-sap Thanks for the updates to the PR. Now that we're finally done with the security patch release, I have a more time to focus on finding a short term solution and reviewing your proposal for a long term solution to ClamAV CVD signing for FIPS compliance. FYI, I also received your email through internal Cisco channels. Please be patient. My team has been impacted by "limited restructuring" a couple times in the past year. So I have more weighing on me, personally. I deeply appreciate you bringing in security experts to help with design and validation. I am also seeking help from @tj-cisco and other peers within Talos as we evaluate this PR and discuss any other options. |
|
I should also add -- I am not keen to add new capabilities or modules in C if I can help it. Regardless, your proposal is very valuable from a design standpoint even if I will lobby to use Rust for the end product. I am not asking you to rewrite it in Rust. I'm setting an expectation that I won't want to merge this but will want to learn from it. |
|
Some more feedback here. This current solution allows for a race condition between the signature file content and the datafile content. Freshclam can download the cvd file during the signature publishing window, when it then fetches the signature file it may contain a signature for a newer datafile than the one that it just fetched. In order to avoid this the signature file should be named in the same way as cdiff files, Additionally the signature file should support containing multiple lines with the strongest signing algorithm first in the file, if the engine does not support the signing algorithm on the first line it should read the next line and check that, repeating until it finds on that it does support. |
Apologies for not being patient @micahsnyder, we appreciate your acknowledgement of the pull request and are here for you all not only for this issue but for future issues. Just for some context we are in the middle of a few assessments and a large number of our production environments were impacted so we needed to ensure that a sustainable solution was raised to satisfy our external assessors. If there is anything we can do to better support you all - from either an open source or corporate perspective please let us as we want to make sure that this project is supported so that you and the team can take some time off and decompress. Again, we genuinely appreciate all that you do as ClamAV is amazing and very close to our hearts. |
I agree with this approach. Ensuring that there is sync between the definition and the signature file is vital. Logic to reject the database if the signature is not found should be added. Since the C version of ClamAV is unlikely to up updated, should I hold off on making another commit to address all of this? @micahsnyder @aghassemlouei |
|
Oops I didn't mean to add the "bug" label. @mark-carey-sap my team needs to discuss it further. Please hold for now. |
Hi @micahsnyder! The signing script will sign arbitrary files, so it should be good to sign all of the database files. In my testing I signed main, daily, and bytecode. Since they are all 512 byte headers with a tar file after that, it knows how to handle any of the DB files. I tried to make it as complete as possible, but if I missed anything, I'm always happy to help out and fix it 😄. Thanks for all the hard work you do on this. Maintainin' ain't easy. |
|
I see so much of back and forth here, is there any kind of solution for clamAV scanning to work on FIPs enabled Ubuntu22.04 which is failing due to uncompliant MD5 signatures |
I'm submitting a PR to add external FIPS compliant signatures to the ClamAV database files.
This PR contains the needed code to generate new keys (2048 bit or more for FIPS compliance), sign the database files using a SHA256 hash and signature, place the signatures in an external file adjunct to the database, and all the needed support code to validate the signatures, and pull them using freshclam.
Hopefully it's a simple solution that addresses the problems of header size in the databases, weak hashing (md5) in the DB header, and backwards compatibility. I've ran it thought valgrind, validated all the attack paths I could think of, and generally tried to make it super simple to add into both the source base, and the signature generation portion that Cisco-Talos maintains.
I hope this helps, and addresses the FIPS compliance issues that a lot of us are dealing with right now. Please feel free to reach out and I can answer any questions, or pop on a call.
Thank you for your consideration!
-Mark Carey.