[2/3] crypto: add pkcs7 parser

From: Nayna Jain <nayna@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.

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

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
Signed-off-by: Eric Richter <erichte@linux.ibm.com>
---
 libstb/Makefile.inc              |   1 +
 libstb/crypto/Makefile.inc       |   4 +-
 libstb/crypto/include/pkcs7.h    |  87 +++++++
 libstb/crypto/pkcs7/Makefile.inc |  11 +
 libstb/crypto/pkcs7/pkcs7.c      | 373 +++++++++++++++++++++++++++++++
 5 files changed, 475 insertions(+), 1 deletion(-)
 create mode 100644 libstb/crypto/include/pkcs7.h
 create mode 100644 libstb/crypto/pkcs7/Makefile.inc
 create mode 100644 libstb/crypto/pkcs7/pkcs7.c

On Thu, 2019-07-18 at 16:29 -0500, Eric Richter wrote:
> From: Nayna Jain <nayna@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.

> 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?
b) How are the restrictions of our parser communicated to that?

I'm a little concerned that you are creating an ABI problem but the
functionality is spread across OPAL, the kernel and userspace so it's
hard to tell what's going.

> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> Signed-off-by: Eric Richter <erichte@linux.ibm.com>
> ---
>  libstb/Makefile.inc              |   1 +
>  libstb/crypto/Makefile.inc       |   4 +-
>  libstb/crypto/include/pkcs7.h    |  87 +++++++
>  libstb/crypto/pkcs7/Makefile.inc |  11 +
>  libstb/crypto/pkcs7/pkcs7.c      | 373 +++++++++++++++++++++++++++++++
>  5 files changed, 475 insertions(+), 1 deletion(-)
>  create mode 100644 libstb/crypto/include/pkcs7.h
>  create mode 100644 libstb/crypto/pkcs7/Makefile.inc
>  create mode 100644 libstb/crypto/pkcs7/pkcs7.c
> 
> diff --git a/libstb/Makefile.inc b/libstb/Makefile.inc
> index 30904df0..93e55bb2 100644
> --- a/libstb/Makefile.inc
> +++ b/libstb/Makefile.inc
> @@ -13,6 +13,7 @@ include $(SRC)/$(LIBSTB_DIR)/tss/Makefile.inc
>  include $(SRC)/$(LIBSTB_DIR)/crypto/Makefile.inc
>  
>  CPPFLAGS += -I$(SRC)/$(LIBSTB_DIR)/crypto/mbedtls/include
> +CPPFLAGS += -I$(SRC)/$(LIBSTB_DIR)/crypto/include
>  
>  $(LIBSTB): $(LIBSTB_OBJS:%=$(LIBSTB_DIR)/%) $(DRIVERS) $(TSS) $(CRYPTO)
>  
> diff --git a/libstb/crypto/Makefile.inc b/libstb/crypto/Makefile.inc
> index 3d71b236..194859c1 100644
> --- a/libstb/crypto/Makefile.inc
> +++ b/libstb/crypto/Makefile.inc
> @@ -17,6 +17,8 @@ MBEDTLS_CFLAGS += $(CPPFLAGS)
>  $(MBEDTLS):
>  	@$(MAKE) -C $(SRC)/$(LIBSTB_DIR)/crypto/mbedtls/library/ CFLAGS="$(MBEDTLS_CFLAGS)" CC=$(CC) AR=$(AR) libmbedcrypto.a libmbedx509.a
>  
> +include $(CRYPTO_DIR)/pkcs7/Makefile.inc
> +
>  CRYPTO = $(CRYPTO_DIR)/built-in.a
>  
> -$(CRYPTO): $(MBEDTLS)
> +$(CRYPTO): $(MBEDTLS) $(PKCS7)
> diff --git a/libstb/crypto/include/pkcs7.h b/libstb/crypto/include/pkcs7.h
> new file mode 100644
> index 00000000..1f2503b0
> --- /dev/null
> +++ b/libstb/crypto/include/pkcs7.h
> @@ -0,0 +1,87 @@
> +/* Copyright 2013-2016 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +#ifndef SKIBOOT_PKCS7_H
> +#define SKIBOOT_PKCS7_H
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <mbedtls/asn1.h>
> +#include <mbedtls/config.h>
> +#include <mbedtls/x509.h>
> +#include <mbedtls/x509_crt.h>
> +#include <mbedtls/x509_crl.h>
> +
> +/** define the OID constants **/
> +#define PKCS7_SIGNED_DATA_OID "\x2a\x86\x48\x86\xf7\x0d\x01\x07\x02"
> +#define PKCS7_DATA_OID "\x2a\x86\x48\x86\xf7\x0d\x01\x07\x01"
> +#define PKCS7_SHA256_OID "\x60\x86\x48\x01\x65\x03\x04\x02\x01"

> +#define PKCS7_RSAeNCRYPTION_OID "\x2a\x86\x48\x86\xf7\x0d\x01\x01\x01"

what's with the random lowercase e? Also this doesn't appear to be
actually used anywhere even though it probably should be...

> +/** define the pkcs7 errors **/
> +#define PKCS7_UNSUPPORTED_CONTENT_TYPE        0x01
> +#define PKCS7_INVALID_VALUE                   0x02
> +#define PKCS7_CERTIFICATE_NOT_FOUND           0x03
> +#define PKCS7_PARSING_ERROR                   0x04
> +#define PKCS7_UNSUPPORTED_VERSION             0x05
> +#define PKCS7_UNSUPPORTED_DIGEST_ALGORITHM    0x06
> +#define PKCS7_UNSUPPORTED_SIGNING_ALGORITHM   0x07
> +
> +typedef mbedtls_asn1_buf pkcs7_buf;
> +
> +typedef mbedtls_asn1_named_data pkcs7_name;
> +
> +typedef mbedtls_asn1_sequence pkcs7_sequence;

use the mbedtls asn1 parser types. you aren't gaining anything by
wrapping them.

> +struct pkcs7_signer_info {
> +	int version;
> +	mbedtls_x509_buf serial;
> +	mbedtls_x509_name issuer;
> +	mbedtls_x509_buf issuer_raw;
> +	mbedtls_x509_buf alg_identifier;
> +	mbedtls_x509_buf sig_alg_identifier;
> +	mbedtls_x509_buf sig;
> +	struct pkcs7_signer_info *next;
> +};
> +
> +struct pkcs7_data {
> +	pkcs7_buf oid;
> +	pkcs7_buf data;
> +};
> +

> +struct pkcs7;
forward declaration isn't needed.

> +struct pkcs7_signed_data {
> +	int version;
> +	pkcs7_buf digest_alg_identifiers;
> +	struct pkcs7_data content;
> +	mbedtls_x509_crt certs;
> +	mbedtls_x509_crl crl;
> +	struct pkcs7_signer_info signers;
> +};
> +
> +struct pkcs7 {
> +	pkcs7_buf content_type_oid;
> +	struct pkcs7_signed_data signed_data;
> +};
> +
> +void pkcs7_printf(const unsigned char *buf, size_t buflen);
> +
> +int pkcs7_parse_message(const unsigned char *buf, const int buflen,
> +			struct pkcs7 *pkcs7);
> +
> +#endif
> diff --git a/libstb/crypto/pkcs7/Makefile.inc b/libstb/crypto/pkcs7/Makefile.inc
> new file mode 100644
> index 00000000..8f9bcd90
> --- /dev/null
> +++ b/libstb/crypto/pkcs7/Makefile.inc
> @@ -0,0 +1,11 @@
> +# -*-Makefile-*-
> +
> +PKCS7_DIR = libstb/crypto/pkcs7
> +
> +SUBDIRS += $(PKCS7_DIR)
> +
> +PKCS7_SRCS = pkcs7.c
> +PKCS7_OBJS = $(PKCS7_SRCS:%.c=%.o)
> +PKCS7 = $(PKCS7_DIR)/built-in.a
> +
> +$(PKCS7): $(PKCS7_OBJS:%=$(PKCS7_DIR)/%)
> diff --git a/libstb/crypto/pkcs7/pkcs7.c b/libstb/crypto/pkcs7/pkcs7.c
> new file mode 100644
> index 00000000..5df27cc8
> --- /dev/null
> +++ b/libstb/crypto/pkcs7/pkcs7.c
> @@ -0,0 +1,373 @@

> +/* Copyright 2013-2016 IBM Corp.
fix the year

> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *      http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */

Use an SPDX tag rather than a license blob. The format for apache2 is:

// SPDX-License-Identifier: Apache-2.0

> +#include <string.h>

> +#include <ccan/endian/endian.h>
Is this needed? It doesn't look like any endian conversion is being
done in this file.

> +#include <pkcs7.h>
> +
> +static int pkcs7_get_next_content_len(unsigned char **p, unsigned char *end,
> +			       size_t *len)
> +{
> +	int rc;
> +
> +	rc = mbedtls_asn1_get_tag(p, end, len, MBEDTLS_ASN1_CONSTRUCTED
> +					     | MBEDTLS_ASN1_CONTEXT_SPECIFIC);
> +
> +	return rc;
> +}

This could be reduced to a single return statement. I don't really see
the value in wrapping single function calls like this, especially in
this case since the meaning of the tag is dependent on the context.

> +/**
> + * version Version
> + * Version ::= INTEGER
> + **/
> +static int pkcs7_get_version(unsigned char **p, unsigned char *end, int *ver)
> +{
> +	int rc;
> +
> +	rc = mbedtls_asn1_get_int(p, end, ver);
> +
> +	return rc;
> +}
Same comment as above.

> +
> +/**
> + * ContentInfo ::= SEQUENCE {
> + *      contentType ContentType,
> + *      content
> + *              [0] EXPLICIT ANY DEFINED BY contentType OPTIONAL }
> + **/
> +static int pkcs7_get_content_info_type(unsigned char **p, unsigned char *end,
> +				pkcs7_buf *pkcs7)
> +{
> +	size_t len = 0;
> +	int rc;
> +
> +	rc = mbedtls_asn1_get_tag(p, end, &len, MBEDTLS_ASN1_CONSTRUCTED
> +					      | MBEDTLS_ASN1_SEQUENCE);
> +	if (rc)
> +		return rc;
> +
> +	rc = mbedtls_asn1_get_tag(p, end, &len, MBEDTLS_ASN1_OID);
> +	if (rc)
> +		return rc;
> +
> +	pkcs7->tag = MBEDTLS_ASN1_OID;
> +	pkcs7->len = len;
> +	pkcs7->p = *p;
> +
> +	return rc;
> +}
> +
> +/**
> + * DigestAlgorithmIdentifier ::= AlgorithmIdentifier
> + *
> + * This is from x509.h
> + **/
> +static int pkcs7_get_digest_algorithm(unsigned char **p, unsigned char *end,
> +			       mbedtls_x509_buf *alg)
> +{
> +	int rc;
> +
> +	rc = mbedtls_asn1_get_alg_null(p, end, alg);
> +
> +	return rc;
> +}
> +
> +/**
> + * DigestAlgorithmIdentifiers :: SET of DigestAlgorithmIdentifier
> + **/
> +static int pkcs7_get_digest_algorithm_set(unsigned char **p, unsigned char *end,
> +				   mbedtls_x509_buf *alg)
> +{
> +	size_t len = 0;
> +	int rc;
> +
> +	rc = mbedtls_asn1_get_tag(p, end, &len, MBEDTLS_ASN1_CONSTRUCTED
> +					      | MBEDTLS_ASN1_SET);
> +	if (rc)
> +		return rc;
> +
> +	end = *p + len;
> +
> +	/** For now, it assumes there is only one digest algorithm specified **/
> +	rc = mbedtls_asn1_get_alg_null(p, end, alg);
> +	if (rc)
> +		return rc;

There's a helper immediately above that for handling a single
DigestAlgorithmIdentifier so why isn't it being used here?

> +
> +	return rc;
> +}
> +
> +/**
> + * certificates :: SET OF ExtendedCertificateOrCertificate,
> + * ExtendedCertificateOrCertificate ::= CHOICE {
> + *      certificate Certificate -- x509,
> + *      extendedCertificate[0] IMPLICIT ExtendedCertificate }
> + **/
> +static int pkcs7_get_certificates(unsigned char **buf, size_t buflen,
> +		mbedtls_x509_crt *certs)
> +{
> +	int rc;
> +
> +	rc = mbedtls_x509_crt_parse(certs, *buf, buflen);
> +	if (rc)
> +		return rc;
> +
> +	return rc;
> +}
> +
> +/**
> + * EncryptedDigest ::= OCTET STRING
> + **/
> +static int pkcs7_get_signature(unsigned char **p, unsigned char *end,
> +		pkcs7_buf *signature)
> +{
> +	int rc;
> +	size_t len = 0;
> +
> +	rc = mbedtls_asn1_get_tag(p, end, &len, MBEDTLS_ASN1_OCTET_STRING);
> +	if (rc)
> +		return rc;
> +
> +	signature->tag = MBEDTLS_ASN1_OCTET_STRING;
> +	signature->len = len;
> +	signature->p = *p;
> +
> +	return rc;
> +}
> +
> +/**
> + * SignerInfo ::= SEQUENCE {
> + *      version Version;
> + *      issuerAndSerialNumber   IssuerAndSerialNumber,
> + *      digestAlgorithm DigestAlgorithmIdentifier,
> + *      authenticatedAttributes
> + *              [0] IMPLICIT Attributes OPTIONAL,
> + *      digestEncryptionAlgorithm DigestEncryptionAlgorithmIdentifier,
> + *      encryptedDigest EncryptedDigest,
> + *      unauthenticatedAttributes
> + *              [1] IMPLICIT Attributes OPTIONAL,
> + **/
> +static int pkcs7_get_signers_info_set(unsigned char **p, unsigned char *end,
> +			       struct pkcs7_signer_info *signers_set)

The comment is misleading since this function consumes a SignerInfo set
rather than a single sequence. As a general point it seems to me that
ignoring everything beyond the first element of the set is going to
create an ABI headache in the future. If we ever transition to a
different digest then we need to make sure the SHA256 entry is the
first element in the set otherwise the verifier on older firmware is
going to choke on it.

> +{
> +	unsigned char *end_set;
> +	int rc;
> +	size_t len = 0;
> +

> +	rc = mbedtls_asn1_get_tag(p, end, &len, MBEDTLS_ASN1_CONSTRUCTED
> +					      | MBEDTLS_ASN1_SET);
> +	if (rc) {
> +		printf("failed\n");
> +		return rc;
> +	}
> +	end_set = *p + len;
> +
> +	rc = mbedtls_asn1_get_tag(p, end_set, &len, MBEDTLS_ASN1_CONSTRUCTED
> +						  | MBEDTLS_ASN1_SEQUENCE);
> +	if (rc)
> +		return rc;
> +

> +	rc = mbedtls_asn1_get_int(p, end_set, &signers_set->version);
> +	if (rc)
> +		return rc;
Does this version need to be validated?

> +
> +	rc = mbedtls_asn1_get_tag(p, end_set, &len, MBEDTLS_ASN1_CONSTRUCTED
> +						  | MBEDTLS_ASN1_SEQUENCE);
> +	if (rc)
> +		return rc;
> +
> +	signers_set->issuer_raw.p = *p;
> +
> +	rc = mbedtls_asn1_get_tag(p, end_set, &len, MBEDTLS_ASN1_CONSTRUCTED
> +						  | MBEDTLS_ASN1_SEQUENCE);
> +	if (rc)
> +		return rc;
> +
> +	rc = mbedtls_x509_get_name(p, *p + len, &signers_set->issuer);
> +	if (rc)
> +		return rc;
> +
> +	signers_set->issuer_raw.len =  *p - signers_set->issuer_raw.p;
> +
> +	rc = mbedtls_x509_get_serial(p, end_set, &signers_set->serial);
> +	if (rc)
> +		return rc;
> +
> +	rc = pkcs7_get_digest_algorithm(p, end_set,
> +					&signers_set->alg_identifier);
> +	if (rc) {
> +		printf("error getting digest algorithms\n");
> +		return rc;
> +	}
> +
> +	rc = pkcs7_get_digest_algorithm(p, end_set,
> +					&signers_set->sig_alg_identifier);
> +	if (rc) {
> +		printf("error getting signature digest algorithms\n");
> +		return rc;
> +	}
> +
> +	rc = pkcs7_get_signature(p, end, &signers_set->sig);
> +	signers_set->next = NULL;
> +
> +	return rc;

I think a lot of this ASN.1 SEQUENCE parsing would be easier to follow
if you annotated the individual parser calls with the sequence element
being parsed. It's a little difficult to follow especially in the cases
where the parsed value is ignored.

> +}
> +
> +/**
> + * SignedData ::= SEQUENCE {
> + *      version Version,
> + *      digestAlgorithms DigestAlgorithmIdentifiers,
> + *      contentInfo ContentInfo,
> + *      certificates
> + *              [0] IMPLICIT ExtendedCertificatesAndCertificates
> + *                  OPTIONAL,
> + *      crls
> + *              [0] IMPLICIT CertificateRevocationLists OPTIONAL,
> + *      signerInfos SignerInfos }
> + */
> +static int pkcs7_get_signed_data(unsigned char *buf, size_t buflen,
> +			  struct pkcs7_signed_data *signed_data)
> +{
> +	unsigned char *p = buf;
> +	unsigned char *end = buf + buflen;
> +	size_t len = 0;
> +	size_t rc;
> +
> +	rc = mbedtls_asn1_get_tag(&p, end, &len, MBEDTLS_ASN1_CONSTRUCTED
> +					       | MBEDTLS_ASN1_SEQUENCE);
> +	if (rc)
> +		return rc;
> +
> +	/* get version of signed data */
> +	rc = pkcs7_get_version(&p, end, &signed_data->version);
> +	if (rc)
> +		return rc;
> +	printf("version is %d\n", signed_data->version);
> +
> +	/* if version != 1, return invalid version */
> +	if (signed_data->version != 1) {
> +		printf("invalid version\n");
> +		return PKCS7_UNSUPPORTED_VERSION;
> +	}
> +
> +	/* get digest algorithm */
> +	rc = pkcs7_get_digest_algorithm_set(&p, end,
> +					    &signed_data->digest_alg_identifiers);
> +	if (rc) {
> +		printf("error getting digest algorithms\n");
> +		return rc;
> +	}
> +

> +	if (signed_data->digest_alg_identifiers.len != strlen(PKCS7_SHA256_OID))
> +		return PKCS7_INVALID_VALUE;
> +
> +	if (memcmp(signed_data->digest_alg_identifiers.p, PKCS7_SHA256_OID,
> +		   signed_data->digest_alg_identifiers.len)) {
> +		printf("Digest Algorithm other than SHA256 is not supported\n");
> +		return PKCS7_UNSUPPORTED_DIGEST_ALGORITHM;
> +	}

These two checks should be swapped. If a digest other than sha256 is
used then odds are it'll choke on the length check first and return the
wrong error code.

> +	/* do not expect any content */
> +	rc = pkcs7_get_content_info_type(&p, end, &signed_data->content.oid);
> +	if (rc)
> +		return rc;
> +
> +	if (memcmp(signed_data->content.oid.p, PKCS7_DATA_OID,
> +		   signed_data->content.oid.len)) {
> +		printf("Invalid PKCS7 data\n");
> +		return PKCS7_INVALID_VALUE;
> +	}
> +
> +	p = p + signed_data->content.oid.len;
> +
> +	rc = pkcs7_get_next_content_len(&p, end, &len);
> +	if (rc)
> +		return rc;
> +
> +	/* get certificates */
> +	printf("----Loading Signer's certificate----\n");
> +	printf("\n");
> +
> +	mbedtls_x509_crt_init(&signed_data->certs);
> +	rc = pkcs7_get_certificates(&p, len, &signed_data->certs);
> +	if (rc)
> +		return rc;
> +
> +	p = p + len;
> +
> +	/* get signers info */
> +	printf("Loading signer's signature\n");
> +	rc = pkcs7_get_signers_info_set(&p, end, &signed_data->signers);
> +
> +	return rc;
> +}
> +

> +void pkcs7_printf(const unsigned char *buf, size_t buflen)
> +{
> +	unsigned int i;
> +	char *sbuf;
> +	int j = 0;
i, j, k, etc are fine names for iterator variables, but this isn't an
iterator variable. Give it a meaningful name.

> +	sbuf = malloc(buflen*2 + 1);
> +	memset(sbuf, 0, buflen*2 + 1);
> +
> +	for (i = 0; i < buflen; i++)
> +		j += snprintf(sbuf+j, sizeof(sbuf), "%02x", buf[i]);
> +
> +	printf("Length of sbuf is %lu\n", strlen(sbuf));
> +	printf("%s\n", sbuf);
> +	printf("\n");

use prlog() with an appropriate log level. that goes for all the random
printf()s used throughout the file too. If you want to have random bits
of extra debug then consider doing what libflash does and define a
XXX_DEBUG() macro that is usually #ifdefed out.

> +
> +	free(sbuf);
> +}
> +

> +int pkcs7_parse_message(const unsigned char *buf, const int buflen,
> +			struct pkcs7 *pkcs7)
> +{
> +	unsigned char *start;
> +	unsigned char *end;
> +	size_t len = 0;
> +	int rc;

> +	/* use internal buffer for parsing */
comment seems wrong

> +	start = (unsigned char *)buf;
> +	end = start + buflen;

Is casting away the const here necessary because mbedtls got the API
wrong? Or does the parsing code actually modify the buffer? In either
case, this needs a comment explaining why the cast is there.

> +	rc = pkcs7_get_content_info_type(&start, end, &(pkcs7->content_type_oid));

The parens are pointless. -> and . are both highest precedence so
&pkcs7->signed_data is fine. Normally extra parens don't bother me, but
the &struct->field pattern is extremely common so it looks odd.

> +	if (rc)
> +		goto out;
> +
> +	if (memcmp(pkcs7->content_type_oid.p, PKCS7_SIGNED_DATA_OID,
> +		   pkcs7->content_type_oid.len)) {

If len is longer than PKCS7_SIGNED_DATA_OID then this will walk off the
end of the const buffer and into undefined behaviour land.

> +		printf("PKCS7 is not the signed data\n");
> +		rc =  PKCS7_UNSUPPORTED_CONTENT_TYPE;
extra space
> +		goto out;
> +	}
> +
> +	printf("Content type is signedData, continue...\n");
> +
> +	start = start + pkcs7->content_type_oid.len;
> +
> +	rc = pkcs7_get_next_content_len(&start, end, &len);
> +	if (rc)
> +		goto out;

Parsing the ContentInfo field is open coded here and in
_get_signed_data. Might as well move it into a helper that checks for
the right OID constant and advances the parser state or throws an
error.

> +
> +	rc = pkcs7_get_signed_data(start, len, &(pkcs7->signed_data));
> +	if (rc)
> +		goto out;
> +
> +out:
> +	return rc;

1. why use "goto out;" when there's nothing being cleaned up?
2. there's a lot of code here with the pattern of:

     rc = thing();
     if(rc)
          return rc;
     return rc;

there's no real reason to keep around that sort of boilerplate, so just
do:
     return thing();

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@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.


>
>> 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.


> 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.


>
> I'm a little concerned that you are creating an ABI problem but the
> functionality is spread across OPAL, the kernel and userspace so it's
> hard to tell what's going.


Do the answers above address your concerns, or are we missing something 
in understanding your concerns ?


Thanks for the code feedback as well. We will address them in the next 
version.


Thanks & Regards,
         - Nayna

Nayna <nayna@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@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.