Re: [PATCH v1 4/5] ima: support fs-verity file digest based signatures

[Stefan Berger <stefanb@linux.ibm.com>]

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