On 1/18/22 19:49, Eric Biggers wrote:
On Sat, Jan 15, 2022 at 10:31:40PM -0500, Stefan Berger wrote:On 1/15/22 01:21, Eric Biggers wrote:On Sat, Jan 15, 2022 at 08:31:01AM +0300, Vitaly Chikunov wrote:Eric, On Sun, Jan 09, 2022 at 01:07:18PM -0800, Eric Biggers wrote:On Sun, Jan 09, 2022 at 11:45:37PM +0300, Vitaly Chikunov wrote:On Wed, Jan 05, 2022 at 03:37:39PM -0800, Eric Biggers wrote:On Fri, Dec 31, 2021 at 10:35:00AM -0500, Mimi Zohar wrote:On Thu, 2021-12-02 at 14:07 -0800, Eric Biggers wrote:On Thu, Dec 02, 2021 at 04:55:06PM -0500, Mimi Zohar wrote:case IMA_VERITY_DIGSIG: - fallthrough; + set_bit(IMA_DIGSIG, &iint->atomic_flags); + + /* + * The IMA signature is based on a hash of IMA_VERITY_DIGSIG + * and the fs-verity file digest, not directly on the + * fs-verity file digest. Both digests should probably be + * included in the IMA measurement list, but for now this + * digest is only used for verifying the IMA signature. + */ + verity_digest[0] = IMA_VERITY_DIGSIG; + memcpy(verity_digest + 1, iint->ima_hash->digest, + iint->ima_hash->length); + + hash.hdr.algo = iint->ima_hash->algo; + hash.hdr.length = iint->ima_hash->length;This is still wrong because the bytes being signed don't include the hash algorithm. Unless you mean for it to be implicitly always SHA-256? fs-verity supports SHA-512 too, and it may support other hash algorithms in the future.IMA assumes that the file hash algorithm and the signature algorithm are the same. If they're not the same, for whatever reason, the signature verification would simply fail. Based on the v2 signature header 'type' field, IMA can differentiate between regular IMA file hash based signatures and fs-verity file digest based signatures. The digest field (d-ng) in the IMA meausrement list prefixes the digest with the hash algorithm. I'm missing the reason for needing to hash fs-verity's file digest with other metadata, and sign that hash rather than fs-verity's file digest directly.Because if someone signs a raw hash, then they also implicitly sign the same hash value for all supported hash algorithms that produce the same length hash.Unless there is broken hash algorithm allowing for preimage attacks this is irrelevant. If there is two broken algorithms allowing for collisions, colliding hashes could be prepared even if algo id is hashed too.Only one algorithm needs to be broken. For example, SM3 has the same hash length as SHA-256. If SM3 support were to be added to fs-verity, and if someone were to find a way to find an input that has a specific SM3 digest, then they could also make it match a specific SHA-256 digest. Someone might intend to sign a SHA-256 digest, but if they are only signing the raw 32 bytes of the digest, then they would also be signing the corresponding SM3 digest. That's why the digest that is signed *must* also include the algorithm used in the digest (not the algorithm(s) used in the signature, which is different).I think it will be beneficial if we pass hash algo id to the akcipher_alg::verify. In fact, ecrdsa should only be used with streebog. And perhaps, sm2 with sm3, pkcs1 with md/sha/sm3, and ecdsa with sha family hashes.I was going to reply to this thread again, but I got a bit distracted by everything else being broken. Yes, the kernel needs to be restricting which hash algorithms can be used with each public key algorithm, along the lines of what you said. I asked the BoringSSL maintainers for advice, and they confirmed that ECDSA just signs/verifies a raw hash, and in fact it *must* be a raw hash for it to be secure. This is a design flaw in ECDSA, which was fixed in newer algorithms such as EdDSA and SM2 as those have a hash built-in to the signature scheme. To mitigate it, the allowed hash algorithms must be restricted; in the case of ECDSA, that means to the SHA family (preferably excluding SHA-1). akcipher_alg::verify doesn't actually know which hash algorithm is used, except in the case of rsa-pkcs1pad where it is built into the name of the algorithm. So it can't check the hash algorithm. I believe it needs to happen in public_key_verify_signature() (and I'm working on a patch for that). Now, SM2 is different from ECDSA and ECRDSA in that it uses the modern design that includes the hash into the signature algorithm. This means that it must be used to sign/verify *data*, not a hash. (Well, you can sign/verify a hash, but SM2 will hash it again internally.) Currently, public_key_verify_signature() allows SM2 to be used to sign/verify a hash, skipping the SM2 internal hash, and IMA uses this. This is broken and must be removed, since it isn't actually the SM2 algorithm as specified anymore, but rather some homebrew thing with unknown security properties. (Well, I'm not confident about SM2, but homebrew is worse.) Adding fs-verity support to IMA also complicates things, as doing it naively would introduce an ambiguity about what is signed. Naively, the *data* that is signed (considering the hash as part of the signature algorithm) would be either the whole file, in the case of traditional IMA, or the fsverity_descriptor struct, in the case of IMA with fs-verity. However, a file could have contents which match an fsverity_descriptor struct; that would create an ambiguity. Assuming that it needs to be allowed that the same key can sign files for both traditional and fs-verity hashing, solving this problem will require a second hash. The easiest way to do this would be sign/verify the following struct: struct ima_file_id { u8 is_fsverity; u8 hash_algorithm; u8 hash[]; };To calrify, I suppose that for ECDSA NIST P256 you would allow pairing with any of the SHA family hashes (also as defined by the existing OIDs) and as the standard allows today? And the same then applies for NIST p384 etc.? Further, I suppose similar restriction would apply for ECRDSA to pair it with Streebog only, as Vitaly said.I don't have any better ideas.What's happening now is that to verify a signature, IMA/integrity subsystem fills out the following structure: struct public_key_signature pks; pks.hash_algo = hash_algo_name[hdr->hash_algo]; // name of hash algo will go into this here, e.g., 'sha256' pks.pkey_algo = pk->pkey_algo; // this is either 'rsa', 'ecdsa-', 'ecrdsa-' or 'sm2' string It then calls: ret = verify_signature(key, &pks); IMO, in the call path down this function the pairing of public key and hash algo would have to be enforced in order to enforce the standards. Would this not be sufficient to be able to stay with the standards ?That sounds right, though there are a number of other issues including SM2 being implemented incorrectly, the "encoding" string isn't validated, and it not being enforced that public_key_signature::pkey_algo actually matches public_key::pkey_algo.
I don't know enough about SM2. Which call path are you looking at for "encoding" ?For IMA's signature verification with public keys we will necessarily get into:
public_key_verfiy_signature: https://elixir.bootlin.com/linux/v5.14.21/source/crypto/asymmetric_keys/public_key.c#L311
sig->encoding is at least then used in software_key_determine_akcipher: https://elixir.bootlin.com/linux/v5.14.21/source/crypto/asymmetric_keys/public_key.c#L66
It doesn't *seem* to be used elsewhere down this call path. Is this not enough of looking at 'encoding' that is used to form the alg_name?
Regarding matching of public_key_signature::pkey_algo and public_key::pkey_algo: What could be the implications of this not matching? Does it matter? Could one accidentally succeed in verifying a signature with the wrong type of key?
As for the proposed patch. I would need to split this up into 3 patches with their corresponding fixes tag, either SM2 or ECDRSA in the first depending on which one is oldest. But not knowing about SM2 I would probably skip this one.
File hashes: IMA calculates the hash over a file itself by calling crypto functions, so at least the digest's bytes are trusted input in that respect and using the sha family type of hashes directly with ECDSA should work. Which algorithm IMA is supposed to use for the hashing is given in the xattr bytestream header. IMA could then take that type of hash, lookup the hash function, perform the hashing on the data, and let verify_signature enforce the pairing, rejecting file signatures with wrong pairing. This way the only thing that is needed is 'enforcement of pairing'. Fsverity: How much control does a user have over the hash family fsverity is using? Can IMA ECDSA/RSA users tell it to use a sha family hash and ECRDSA users make it use a Streebog hash so that also the pairing of hash and key type can work 'naturally' and we don't need the level of indirection via your structure above?The hash algorithm used by fs-verity is configurable and is always returned along with the file digest. Currently, only SHA-256 and SHA-512 are supported. Keep in mind that if you sign the fs-verity file digest directly with RSA, ECDSA, or ECRDSA, the *data* you are actually signing is the fsverity_descriptor -- the struct which the hash is a hash of. That creates an ambiguity when full file hashes are also signed by the same key, as I previously mentioned. A level of indirection is needed to avoid that. In the naive method, the *data* being signed would also be different with SM2. The level of indirection would avoid that.
So in the fsverity case that level of indirection is needed, for the existing file signatures I don't think we need it.
Stefan
- Eric
|