[Skiboot] [PATCH 2/3] crypto: add pkcs7 parser

[Stewart Smith]

Nayna <nayna at linux.vnet.ibm.com> writes:
> On 07/22/2019 04:57 AM, Oliver O'Halloran wrote:
>> On Thu, 2019-07-18 at 16:29 -0500, Eric Richter wrote:
>>> From: Nayna Jain <nayna at linux.ibm.com>
>>>
>>> The secure boot key management involves verification of the key updates
>>> which are signed using PKCS7 structure. Though the mbedtls crypto API
>>> comes with various crypto API support, it doesn't support PKCS7.
>> Is there any reason why you aren't trying to contribute this upstream?
>> I'd be a hell of a lot more willing to trust that the code is correct
>> if it was reviewed upstream.
>
>
> Thanks Oliver for the review.
> We would really like to contribute it upstream, however due to time 
> constraints we have only implemented the limited subset of features 
> needed for our secure boot support.

It'd certainly be better if it went upstream and received some review
though. This is security critical code after all.

>>> This patch implements the PKCS7 parser that extracts the signer's info
>>> and the signature using mbedtls ASN.1 parsing library. The pkcs7 parser
>>> is not fully implemented, but limited to the OpenPOWER key update
>>> authentication requirements (eg. single certificate, no CRLs, single
>>> signer info, NULL content data, NULL parametes for digest algorithms).
>>>
>>> It currently supports the following validation checks:
>>> * Supports only signed data
>>> * Version should be 1
>>> * Supports only SHA256 hash algorithm
>> Two questions:
>>
>> a) What is going to be producing the PKCS#7 blob?
>
> That is produced by the edk2-compatible userspace tools.

Have these been ported over?

It doesn't look like there's any unit tests added to ensure the code is
functioning correctly and all paths are covered. Since this is security
critical, we don't want some random error code path to be exploitable or
horrifically buggy.

>> b) How are the restrictions of our parser communicated to that?
>
>
> These are inferred by the backend type (edk2-compatible-v1) exposed in 
> the device-tree, which specifies a format, algorithm, etc to use for the 
> updates. Use of unsupported formats, etc will be rejected during update 
> processing.

Things will  want to be *really* well documented on limitations then,
and we have to be okay with living with those limitations for a *very*
long time.


-- 
Stewart Smith
OPAL Architect, IBM.