Re: [PATCH v2 09/21] appended signatures: parse PKCS#7 signedData and X.509 certificates

[sudhakar <sudhakar@linux.ibm.com>]

Hi Daniel,

Thank you for your valuable review. addressed all the below review 
comments in v3.

Thanks,
Sudhakar Kuppusmay

On 2025-05-28 22:14, Daniel Kiper wrote:
> On Thu, Mar 27, 2025 at 01:02:30AM +0530, Sudhakar Kuppusamy wrote:
>> From: Daniel Axtens <dja@axtens.net>
>> 
>> This code allows us to parse:
>> 
>>  - PKCS#7 signedData messages. Only a single signerInfo is supported,
>>    which is all that the Linux sign-file utility supports creating
>>    out-of-the-box. Only RSA, SHA-256 and SHA-512 are supported.
>>    Any certificate embedded in the PKCS#7 message will be ignored.
>> 
>>  - X.509 certificates: at least enough to verify the signatures on the
>>    PKCS#7 messages. We expect that the certificates embedded in grub 
>> will
> 
> s/grub/GRUB/
> 
>>    be leaf certificates, not CA certificates. The parser enforces 
>> this.
>> 
>>  - X.509 certificates support the Extended Key Usage extension and 
>> handle
>>    it by verifying that the certificate has a Code Signing usage.
> 
> Could not you split this patch to 2 or 3 logical parts?
> 
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> # EKU 
>> support
>> Reported-by: Michal Suchanek <msuchanek@suse.com> # key usage issue
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>
>> ---
>>  grub-core/commands/appendedsig/appendedsig.h | 110 +++
>>  grub-core/commands/appendedsig/asn1util.c    |  98 ++
>>  grub-core/commands/appendedsig/pkcs7.c       | 460 +++++++++
>>  grub-core/commands/appendedsig/x509.c        | 953 
>> +++++++++++++++++++
>>  4 files changed, 1621 insertions(+)
>>  create mode 100644 grub-core/commands/appendedsig/appendedsig.h
>>  create mode 100644 grub-core/commands/appendedsig/asn1util.c
>>  create mode 100644 grub-core/commands/appendedsig/pkcs7.c
>>  create mode 100644 grub-core/commands/appendedsig/x509.c
>> 
>> diff --git a/grub-core/commands/appendedsig/appendedsig.h 
>> b/grub-core/commands/appendedsig/appendedsig.h
>> new file mode 100644
>> index 000000000..3f4d7002e
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/appendedsig.h
>> @@ -0,0 +1,110 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2020, 2022 Free Software Foundation, Inc.
>> + *  Copyright (C) 2020, 2022 IBM Corporation
> 
> s/2020, 2022/2020, 2022, 2025/ everywhere please...
> 
>> + *
>> + *  GRUB is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published 
>> by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <grub/crypto.h>
>> +#include <grub/libtasn1.h>
>> +
>> +extern asn1_node _gnutls_gnutls_asn;
> 
> s/_gnutls_gnutls_asn/grub_gnutls_gnutls_asn/
> 
>> +extern asn1_node _gnutls_pkix_asn;
> 
> s/_gnutls_pkix_asn/grub_gnutls_pkix_asn/
> 
>> +#define MAX_OID_LEN 32
>> +
>> +/*
>> + * One or more x509 certificates.
>> + * We do limited parsing: extracting only the serial, CN and RSA 
>> public key.
>> + */
>> +struct x509_certificate
>> +{
>> +  struct x509_certificate *next;
>> +  grub_uint8_t *serial;
>> +  grub_size_t serial_len;
>> +  char *subject;
>> +  grub_size_t subject_len;
>> +  /* We only support RSA public keys. This encodes [modulus, 
>> publicExponent] */
>> +  gcry_mpi_t mpis[2];
>> +};
>> +
>> +/*
>> + * A PKCS#7 signedData signerInfo.
>> + */
>> +struct pkcs7_signerInfo
>> +{
>> +  const gcry_md_spec_t *hash;
>> +  gcry_mpi_t sig_mpi;
>> +};
>> +
>> +/*
>> + * A PKCS#7 signedData message.
>> + * We make no attempt to match intelligently, so we don't save any 
>> info about
>> + * the signer.
>> + */
>> +struct pkcs7_signedData
>> +{
>> +  int signerInfo_count;
>> +  struct pkcs7_signerInfo *signerInfos;
>> +};
>> +
>> +/* Do libtasn1 init */
>> +int
>> +asn1_init (void);
>> +
>> +/*
>> + * Import a DER-encoded certificate at 'data', of size 'size'.
>> + * Place the results into 'results', which must be already allocated.
>> + */
>> +grub_err_t
>> +parse_x509_certificate (const void *data, grub_size_t size, struct 
>> x509_certificate *results);
>> +
>> +/*
>> + * Release all the storage associated with the x509 certificate.
>> + * If the caller dynamically allocated the certificate, it must free 
>> it.
>> + * The caller is also responsible for maintenance of the linked list.
>> + */
>> +void
>> +certificate_release (struct x509_certificate *cert);
>> +
>> +/*
>> + * Parse a PKCS#7 message, which must be a signedData message.
>> + * The message must be in 'sigbuf' and of size 'data_size'. The 
>> result is
>> + * placed in 'msg', which must already be allocated.
>> + */
>> +grub_err_t
>> +parse_pkcs7_signedData (const void *sigbuf, grub_size_t data_size, 
>> struct pkcs7_signedData *msg);
>> +
>> +/*
>> + * Release all the storage associated with the PKCS#7 message.
>> + * If the caller dynamically allocated the message, it must free it.
>> + */
>> +void
>> +pkcs7_signedData_release (struct pkcs7_signedData *msg);
>> +
>> +/*
>> + * Read a value from an ASN1 node, allocating memory to store it.
>> + * It will work for anything where the size libtasn1 returns is 
>> right:
>> + *  - Integers
>> + *  - Octet strings
>> + *  - DER encoding of other structures
>> + * It will _not_ work for things where libtasn1 size requires 
>> adjustment:
>> + *  - Strings that require an extra null byte at the end
>> + *  - Bit strings because libtasn1 returns the length in bits, not 
>> bytes.
>> + *
>> + * If the function returns a non-NULL value, the caller must free it.
>> + */
>> +void *
>> +grub_asn1_allocate_and_read (asn1_node node, const char *name, const 
>> char *friendly_name, int *content_size);
>> diff --git a/grub-core/commands/appendedsig/asn1util.c 
>> b/grub-core/commands/appendedsig/asn1util.c
>> new file mode 100644
>> index 000000000..06c3b61c1
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/asn1util.c
>> @@ -0,0 +1,98 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2020, 2022 Free Software Foundation, Inc.
>> + *  Copyright (C) 2020, 2022 IBM Corporation
>> + *
>> + *  GRUB is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published 
>> by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <grub/libtasn1.h>
>> +#include <grub/types.h>
>> +#include <grub/err.h>
>> +#include <grub/mm.h>
>> +#include <grub/crypto.h>
>> +#include <grub/misc.h>
>> +#include <grub/gcrypt/gcrypt.h>
>> +
>> +#include "appendedsig.h"
>> +
>> +asn1_node _gnutls_gnutls_asn = NULL;
>> +asn1_node _gnutls_pkix_asn = NULL;
> 
> Global variables should be prefixed with grub_...
> 
>> +extern const asn1_static_node gnutls_asn1_tab[];
>> +extern const asn1_static_node pkix_asn1_tab[];
> 
> Ditto.
> 
>> +/*
>> + * Read a value from an ASN1 node, allocating memory to store it.
>> + * It will work for anything where the size libtasn1 returns is 
>> right:
>> + *  - Integers
>> + *  - Octet strings
>> + *  - DER encoding of other structures
>> + * It will _not_ work for things where libtasn1 size requires 
>> adjustment:
>> + *  - Strings that require an extra NULL byte at the end
>> + *  - Bit strings because libtasn1 returns the length in bits, not 
>> bytes.
>> + * If the function returns a non-NULL value, the caller must free it.
>> + */
>> +void *
>> +grub_asn1_allocate_and_read (asn1_node node, const char *name, const 
>> char *friendly_name, int *content_size)
>> +{
>> +  int result;
>> +  grub_uint8_t *tmpstr = NULL;
>> +  int tmpstr_size = 0;
>> +
>> +  result = asn1_read_value (node, name, NULL, &tmpstr_size);
>> +  if (result != ASN1_MEM_ERROR)
>> +    {
>> +      grub_snprintf (grub_errmsg, sizeof (grub_errmsg),
>> +                     _("Reading size of %s did not return expected 
>> status: %s"),
> 
> Please drop _().
> 
>> +                     friendly_name, asn1_strerror (result));
>> +      grub_errno = GRUB_ERR_BAD_FILE_TYPE;
> 
> Why do not you use grub_error() here?
> 
>> +      return NULL;
>> +    }
>> +
>> +  tmpstr = grub_malloc (tmpstr_size);
>> +  if (tmpstr == NULL)
>> +    {
>> +      grub_snprintf (grub_errmsg, sizeof (grub_errmsg),
>> +                     "Could not allocate memory to store %s", 
>> friendly_name);
>> +      grub_errno = GRUB_ERR_OUT_OF_MEMORY;
> 
> Ditto.
> 
>> +      return NULL;
>> +    }
>> +
>> +  result = asn1_read_value (node, name, tmpstr, &tmpstr_size);
>> +  if (result != ASN1_SUCCESS)
>> +    {
>> +      grub_free (tmpstr);
>> +      grub_snprintf (grub_errmsg, sizeof (grub_errmsg), "Error 
>> reading %s: %s",
>> +                     friendly_name, asn1_strerror (result));
>> +      grub_errno = GRUB_ERR_BAD_FILE_TYPE;
> 
> Ditto.
> 
>> +      return NULL;
>> +    }
>> +
>> +  *content_size = tmpstr_size;
>> +
>> +  return tmpstr;
>> +}
>> +
>> +int
>> +asn1_init (void)
>> +{
>> +  int res;
> 
> Missing empty line.
> 
>> +  res = asn1_array2tree (gnutls_asn1_tab, &_gnutls_gnutls_asn, NULL);
>> +  if (res != ASN1_SUCCESS)
>> +    return res;
>> +
>> +  res = asn1_array2tree (pkix_asn1_tab, &_gnutls_pkix_asn, NULL);
>> +  return res;
>> +}
>> diff --git a/grub-core/commands/appendedsig/pkcs7.c 
>> b/grub-core/commands/appendedsig/pkcs7.c
>> new file mode 100644
>> index 000000000..31a5eab6a
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/pkcs7.c
>> @@ -0,0 +1,460 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2020, 2022 Free Software Foundation, Inc.
>> + *  Copyright (C) 2020, 2022 IBM Corporation
>> + *
>> + *  GRUB is free software: you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License as published 
>> by
>> + *  the Free Software Foundation, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *  GNU General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "appendedsig.h"
>> +#include <grub/misc.h>
>> +#include <grub/crypto.h>
>> +#include <grub/gcrypt/gcrypt.h>
>> +#include <sys/types.h>
>> +
>> +static char asn1_error[ASN1_MAX_ERROR_DESCRIPTION_SIZE];
>> +
>> +/*
>> + * RFC 5652 s 5.1
>> + */
> 
> Please put this comment in one line and s/s/section/?
> 
>> +static const char *signedData_oid = "1.2.840.113549.1.7.2";
>> +
>> +/*
>> + * RFC 4055 s 2.1
>> + */
> 
> Ditto. Please fix similar problems everywhere...
> 
>> +static const char *sha256_oid = "2.16.840.1.101.3.4.2.1";
>> +static const char *sha512_oid = "2.16.840.1.101.3.4.2.3";
>> +
>> +static grub_err_t
>> +process_content (grub_uint8_t *content, int size, struct 
>> pkcs7_signedData *msg)
>> +{
>> +  int res;
>> +  asn1_node signed_part;
>> +  grub_err_t err = GRUB_ERR_NONE;
>> +  char algo_oid[MAX_OID_LEN];
>> +  int algo_oid_size = sizeof (algo_oid);
>> +  int algo_count;
>> +  int signer_count;
>> +  int i;
>> +  char version;
>> +  int version_size = sizeof (version);
>> +  grub_uint8_t *result_buf;
>> +  int result_size = 0;
>> +  int crls_size = 0;
>> +  gcry_error_t gcry_err;
>> +  bool sha256_in_da, sha256_in_si, sha512_in_da, sha512_in_si;
>> +  char *da_path;
>> +  char *si_sig_path;
>> +  char *si_da_path;
>> +
>> +  res = asn1_create_element (_gnutls_pkix_asn, 
>> "PKIX1.pkcs-7-SignedData", &signed_part);
>> +  if (res != ASN1_SUCCESS)
>> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY,
>> +                       "Could not create ASN.1 structure for PKCS#7 
>> signed part.");
> 
> All error messages in the core GRUB code start with lowercase and do 
> not
> have full stop at the end. Please fix it in all patches.
> 
>> +  res = asn1_der_decoding2 (&signed_part, content, &size,
>> +                            ASN1_DECODE_FLAG_STRICT_DER, asn1_error);
>> +  if (res != ASN1_SUCCESS)
>> +    {
>> +      err = grub_error (GRUB_ERR_BAD_SIGNATURE,
>> +                        "Error reading PKCS#7 signed data: %s", 
>> asn1_error);
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  /*
>> +   * SignedData ::= SEQUENCE {
>> +   *     version CMSVersion,
>> +   *     digestAlgorithms DigestAlgorithmIdentifiers,
>> +   *     encapContentInfo EncapsulatedContentInfo,
>> +   *     certificates [0] IMPLICIT CertificateSet OPTIONAL,
>> +   *     crls [1] IMPLICIT RevocationInfoChoices OPTIONAL,
>> +   *     signerInfos SignerInfos }
>> +   */
>> +
>> +  /* version per the algo in 5.1, must be 1 */
> 
> Could you expand this comment? It has to be clear.
> 
>> +  res = asn1_read_value (signed_part, "version", &version, 
>> &version_size);
>> +  if (res != ASN1_SUCCESS)
>> +    {
>> +      err = grub_error (GRUB_ERR_BAD_SIGNATURE, "Error reading 
>> signedData version: %s",
>> +                        asn1_strerror (res));
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  if (version != 1)
>> +    {
>> +      err = grub_error (GRUB_ERR_BAD_SIGNATURE,
>> +                        "Unexpected signature version v%d, only v1 
>> supported", version);
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  /*
>> +   * digestAlgorithms DigestAlgorithmIdentifiers
>> +   *
>> +   * DigestAlgorithmIdentifiers ::= SET OF DigestAlgorithmIdentifier
>> +   * DigestAlgorithmIdentifer is an X.509 AlgorithmIdentifier 
>> (10.1.1)
>> +   *
>> +   * RFC 4055 s 2.1:
>> +   * sha256Identifier  AlgorithmIdentifier  ::=  { id-sha256, NULL }
>> +   * sha512Identifier  AlgorithmIdentifier  ::=  { id-sha512, NULL }
>> +   *
>> +   * We only support 1 element in the set, and we do not check 
>> parameters atm.
>> +   */
>> +  res = asn1_number_of_elements (signed_part, "digestAlgorithms", 
>> &algo_count);
>> +  if (res != ASN1_SUCCESS)
>> +    {
>> +      err = grub_error (GRUB_ERR_BAD_SIGNATURE, "Error counting 
>> number of digest algorithms: %s",
>> +                        asn1_strerror (res));
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  if (algo_count <= 0)
>> +    {
>> +      err = grub_error (GRUB_ERR_BAD_SIGNATURE,
>> +                        "A minimum of 1 digest algorithm is 
>> required");
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  if (algo_count > 2)
>> +    {
>> +      err = grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
>> +                        "A maximum of 2 digest algorithms is 
>> supported");
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  sha256_in_da = false;
>> +  sha512_in_da = false;
>> +
>> +  for (i = 0; i < algo_count; i++)
>> +    {
>> +      da_path = grub_xasprintf ("digestAlgorithms.?%d.algorithm", i + 
>> 1);
>> +      if (!da_path)
>> +        {
>> +          err = grub_error (GRUB_ERR_OUT_OF_MEMORY,
>> +                            "Could not allocate path for digest 
>> algorithm "
>> +                            "parsing path");
>> +          goto cleanup_signed_part;
>> +        }
>> +
>> +      algo_oid_size = sizeof (algo_oid);
>> +      res = asn1_read_value (signed_part, da_path, algo_oid, 
>> &algo_oid_size);
>> +      if (res != ASN1_SUCCESS)
>> +        {
>> +          err = grub_error (GRUB_ERR_BAD_SIGNATURE, "Error reading 
>> digest algorithm: %s",
>> +                            asn1_strerror (res));
>> +          grub_free (da_path);
>> +          goto cleanup_signed_part;
>> +        }
>> +
>> +      if (grub_strncmp (sha512_oid, algo_oid, algo_oid_size) == 0)
>> +        {
>> +          if (!sha512_in_da)
> 
> sha512_in_da == false (for bools) please everywhere...
> 
>> +            sha512_in_da = true;
>> +          else
>> +            {
>> +              err = grub_error (GRUB_ERR_BAD_SIGNATURE,
>> +                                "SHA-512 specified twice in digest 
>> algorithm list");
>> +              grub_free (da_path);
>> +              goto cleanup_signed_part;
>> +            }
>> +        }
>> +      else if (grub_strncmp (sha256_oid, algo_oid, algo_oid_size) == 
>> 0)
>> +        {
>> +          if (!sha256_in_da)
>> +            sha256_in_da = true;
>> +          else
>> +            {
>> +              err = grub_error (GRUB_ERR_BAD_SIGNATURE,
>> +                                "SHA-256 specified twice in digest 
>> algorithm list");
>> +              grub_free (da_path);
>> +              goto cleanup_signed_part;
>> +            }
>> +        }
>> +      else
>> +        {
>> +          err = grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, "Only 
>> SHA-256 and SHA-512 hashes are supported, found OID %s",
>> +                            algo_oid);
>> +          grub_free (da_path);
>> +          goto cleanup_signed_part;
>> +        }
>> +
>> +      grub_free (da_path);
>> +    }
>> +
>> +  /* at this point, at least one of sha{256,512}_in_da must be true 
>> */
>> +
>> +  /*
>> +   * We ignore the certificates, but we don't permit CRLs.
>> +   * A CRL entry might be revoking the certificate we're using, and 
>> we have
>> +   * no way of dealing with that at the moment.
>> +   */
>> +  res = asn1_read_value (signed_part, "crls", NULL, &crls_size);
>> +  if (res != ASN1_ELEMENT_NOT_FOUND)
>> +    {
>> +      err = grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
>> +                        "PKCS#7 messages with embedded CRLs are not 
>> supported");
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  /* read the signatures */
>> +
>> +  res = asn1_number_of_elements (signed_part, "signerInfos", 
>> &signer_count);
>> +  if (res != ASN1_SUCCESS)
>> +    {
>> +      err = grub_error (GRUB_ERR_BAD_SIGNATURE, "Error counting 
>> number of signers: %s",
>> +                        asn1_strerror (res));
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  if (signer_count <= 0)
>> +    {
>> +      err = grub_error (GRUB_ERR_BAD_SIGNATURE, "A minimum of 1 
>> signer is required");
>> +      goto cleanup_signed_part;
>> +    }
>> +
>> +  msg->signerInfos = grub_calloc (signer_count, sizeof (struct 
>> pkcs7_signerInfo));
>> +  if (!msg->signerInfos)
> 
> msg->signerInfos == NULL (for pointers) everywhere please...
> 
> Additionally, all (goto) labels should be prefixed with one space.
> 
> Daniel

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel