[Skiboot] [PATCH v2 4/5] secvar/backend: redo hash algorithm handling for auth structures

[Nayna]

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,

      - Nayna