[Skiboot] [PATCH v2 4/5] secvar/backend: redo hash algorithm handling for auth structures
nayna at linux.vnet.ibm.com
Tue Jun 22 11:29:09 AEST 2021
On 6/21/21 4:26 AM, Daniel Axtens wrote:
> We only handle sha256 hashes in auth structures.
> In the process of verifying an auth structure, we extract the pkcs7
> message and we calculate the hopefully-matching hash, which is
> sha256(name || vendor guid || attributes || timestamp || newcontent)
> We then verify that the PKCS#7 signature matches that calculated hash.
> However, at no point do we check that the PKCS#7 hash algorithm is
> sha256. So if the PKCS#7 message says that it is a signature on a sha512,
> mbedtls will compare 64 bytes of hash from the signature with 64 bytes
> from our hash, resulting - if I've got the mbedtls flow correct - in a
> 32 byte overread.
This seems good check to add. Thanks for looking at it.
> - Verify that the hash algorithm in the PKCS#7 message is sha256.
> [there is an additional complexity in the spec which our code doesn't
> handle. the digest algorithms are specified in 2 places:
> DigestAlgorithmIdentifiers, and SignerInfos.<number>.digestAlgorithm.
> Having them not match would be a spec violation, but nonetheless fairly
> trivial to construct. However, mbedtls_pkcs7_signed_hash_verify
> considers only the first element of DigestAlgorithmIdentifiers, so this
> is safe for now at least.]
> - Stop passing hash lengths around, they're constant.
> - Pass 32 into mbedtls as the hash length. I contemplated using 0,
> which is code for "just use what the hash algorithm says the length
> is", but I'm worried about what might happen if we move to an mbedtls
> that supports pkcs7 messages with multiple signers: what if a message
> contains both a sha256 and then a sha512 signature? Our new check
> won't catch it. At least with passing 32 there is the possibility that
> mbedtls can do the right thing and not over-read. If we pass in 0, then
> mbedtls has no way of knowing that we have only 32 bytes allocated.
> Hopefully the upshot of this is that the code is safer and if we do end up
> using another hash algorithm, we're forced to at least consider the
> possibility that the hash length is different.
> [If we ever use a version of mbedtls that supports multiple digests
> then we should of course review this code again.
> Arguably multiple digests are also going to need an mbedtls API change,
> it's difficult to figure out how a hash length value of 0 could be safe.]
I would go with assuming only single signer support now. Multiple
signers support might need not only hash handling but some other changes
also in pkcs7 code.
Nick and I are going to test the patches.
Thanks & Regards,
More information about the Skiboot