Message-ID: <b4105f9b-98f7-f941-47db-2f72e0c5b8bd@linux.ibm.com>
Date:   Mon, 10 Jan 2022 22:26:23 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v2 5/6] ima: support fs-verity file digest based
 signatures
Content-Language: en-US
To:     Eric Biggers <ebiggers@kernel.org>,
        Mimi Zohar <zohar@linux.ibm.com>
Cc:     linux-integrity@vger.kernel.org, linux-fscrypt@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220109185517.312280-1-zohar@linux.ibm.com>
 <20220109185517.312280-6-zohar@linux.ibm.com> <Ydy3EA9ONY3kn1xr@gmail.com>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <Ydy3EA9ONY3kn1xr@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
Precedence: bulk


On 1/10/22 17:45, Eric Biggers wrote:
> On Sun, Jan 09, 2022 at 01:55:16PM -0500, Mimi Zohar wrote:
>> +	case IMA_VERITY_DIGSIG:
>> +		set_bit(IMA_DIGSIG, &iint->atomic_flags);
>> +
>> +		algo = iint->ima_hash->algo;
>> +		hash = kzalloc(sizeof(*hash) + hash_digest_size[algo],
>> +			       GFP_KERNEL);
>> +		if (!hash) {
>> +			*cause = "verity-hashing-error";
>> +			*status = INTEGRITY_FAIL;
>> +			break;
>> +		}
>> +
>> +		rc = calc_tbs_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo,
>> +				   iint->ima_hash->digest, hash);
>> +		if (rc) {
>> +			*cause = "verity-hashing-error";
>> +			*status = INTEGRITY_FAIL;
>> +			break;
>> +		}
>> +
>> +		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
>> +					     (const char *)xattr_value,
>> +					     xattr_len, hash->digest,
>> +					     hash->length);
> This is still verifying a raw hash value, which is wrong as I've explained
> several times.  Yes, you are now hashing the hash algorithm ID together with the
> original hash value, but at the end the thing being signed/verified is still a
> raw hash value, which is ambigious.
>
> I think I see where the confusion is.  If rsa-pkcs1pad is used, then the
> asymmetric algorithm is parameterized by a hash algorithm, and this hash
> algorithm's identifier is automatically built-in to the data which is
> signed/verified.  And the data being signed/verified is assumed to be a hash
> value of the same type.  So in this case, the caller doesn't need to handle
> disambiguating raw hashes.
>
> However, asymmetric_verify() also supports ecdsa and ecrdsa signatures.  As far
> as I can tell, those do *not* have the hash algorithm identifier built-in to the
> data which is signed/verified; they just sign/verify the data given.  That


The signatures are generated by evmctl by this code here, which works 
for RSA and ECDSA and likely also ECRDSA:

https://sourceforge.net/p/linux-ima/ima-evm-utils/ci/master/tree/src/libimaevm.c#l1036

    if (!EVP_PKEY_sign_init(ctx))
         goto err;
     st = "EVP_get_digestbyname";
     if (!(md = EVP_get_digestbyname(algo)))
         goto err;
     st = "EVP_PKEY_CTX_set_signature_md";
     if (!EVP_PKEY_CTX_set_signature_md(ctx, md))
         goto err;
     st = "EVP_PKEY_sign";
     sigsize = MAX_SIGNATURE_SIZE - sizeof(struct signature_v2_hdr) - 1;
     if (!EVP_PKEY_sign(ctx, hdr->sig, &sigsize, hash, size))
         goto err;
     len = (int)sigsize;

As far as I know, these are not raw signatures but generate the OIDs for 
RSA + shaXYZ or ECDSA + shaXYZ (1.2.840.10045.4.1 et al) and prepend 
them to the hash and then sign that.


> creates an ambiguity if the hash algorithm identifier is not included.  For
> example, someone might have intended to sign the SHA-256 hash
> 01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b.  However, the
> Streebog or SM3 hash
> 01ba4719c80b6fe911b091a7c05124b64eeece964e09c058ef8f9805daca546b would also pass
> the signature check too.  That's wrong; to have a valid cryptosystem, you
> mustn't let the adversary choose the crypto algorithms for you.

There's a hash algorithm identifier in the xattr in the header, which is 
prepended to the bytes of the actual signature. This hash algo identifer 
tells IMA which hash to use on the file data so that subsequent 
signature verification with the same hash works. That same hash 
identifier is then again embedded in the signature using the OID and 
thus has to match on the signature verification level.

The effectively double hashed data via calc_tbs_hash() above is not 
good. calc_tbs_hash() is unnecessary.

On the evmctl level the signature should be created from the digest 
retrieved via ioctl() [or similar I suppose] from fsverity on the file 
and fsverity presumably then also says what type of hash this is. So, 
fsverity ioctl response of hash + size of hash and hash_algo become 
input to the evmctl snippet above. On the kernel level the data from 
fsverity_get_digest() should be all it takes to verify against an xattr 
created by evmctl as described.


>
> I'm not sure how this can be reconciled, given the differences between
> rsa-pkcs1pad and ecdsa and ecrdsa.  Could you just use the lowest common
> denominator and prepend the hash algorithm ID to the hash value, or would that
> cause issues with rsa-pkcs1pad?  In any case, to move forward you're going to
> need to solve this problem.
>
> - Eric