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

[Daniel Axtens]

Stefan Berger <stefanb@linux.ibm.com> writes:

> On 6/30/21 4:40 AM, Daniel Axtens wrote:
>
>> 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
>>     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 single purpose, that is code
>>     signing. This is required because Red Hat certificates have both Key
>>     Usage and Extended Key Usage extensions present.
>>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> # EKU support
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>
> A few comments below.
>
>
>>
>> ---
>>
>> v2 changes:
>>
>>   - Handle the Extended Key Usage extension
>>   - Fix 2 leaks in x509 cert parsing
>>   - Improve x509 parser function name
>>   - Constify the data parameter in parser signatures
>>   - Support multiple signers in a pkcs7 message. Accept any passing sig.
>>   - Allow padding after a pkcs7 message in an appended signature, required
>>      to support my model for signers separated in time.
>>   - Fix a test that used GRUB_ERR_NONE rather than ASN1_SUCCESS. They're
>>      both 0 so no harm was done, but better to be correct.
>>   - Various code and comment cleanups.
>>
>> Thanks to Nayna Jain and Stefan Berger for their reviews.
>>
>> revert
>>
>> Signed-off-by: Daniel Axtens <dja@axtens.net>
>> ---
>>   grub-core/commands/appendedsig/appendedsig.h |  118 ++
>>   grub-core/commands/appendedsig/asn1util.c    |  103 ++
>>   grub-core/commands/appendedsig/pkcs7.c       |  509 +++++++++
>>   grub-core/commands/appendedsig/x509.c        | 1079 ++++++++++++++++++
>>   4 files changed, 1809 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 000000000000..327d68ddb1b7
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/appendedsig.h
>> @@ -0,0 +1,118 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2020  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/crypto.h>
>> +#include <grub/libtasn1.h>
>> +
>> +extern asn1_node _gnutls_gnutls_asn;
>> +extern asn1_node _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 000000000000..6b508222a081
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/asn1util.c
>> @@ -0,0 +1,103 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2020  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 = ASN1_TYPE_EMPTY;
>> +asn1_node _gnutls_pkix_asn = ASN1_TYPE_EMPTY;
>> +
>> +extern const ASN1_ARRAY_TYPE gnutls_asn1_tab[];
>> +extern const ASN1_ARRAY_TYPE pkix_asn1_tab[];
>> +
>> +/*
>> + * 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"),
>> +                 friendly_name, asn1_strerror (result));
>> +      grub_errno = GRUB_ERR_BAD_FILE_TYPE;
>> +      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;
>> +      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;
>> +      return NULL;
>> +    }
>> +
>> +  *content_size = tmpstr_size;
>> +
>> +  return tmpstr;
>> +}
>> +
>> +int
>> +asn1_init (void)
>> +{
>> +  int res;
>> +  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 000000000000..845f58a53e83
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/pkcs7.c
>> @@ -0,0 +1,509 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2020  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
>> + */
>> +const char *signedData_oid = "1.2.840.113549.1.7.2";
>> +
>> +/*
>> + * RFC 4055 s 2.1
>> + */
>> +const char *sha256_oid = "2.16.840.1.101.3.4.2.1";
>> +const char *sha512_oid = "2.16.840.1.101.3.4.2.3";

Made these static too.

>> +      goto cleanup_signerInfos;
>> +    }
>> +
>> +      result_buf =
>> +    grub_asn1_allocate_and_read (signed_part, si_sig_path,
>> +                                 "signature data", &result_size);
>> +      if (!result_buf)
>> +    {
>> +      err = grub_errno;
>> +      grub_free (si_sig_path);
>> +      goto cleanup_signerInfos;
>> +    }
>> +      grub_free (si_sig_path);
>
> Nit: You could probably move this before the if statement so you only 
> have to write this once.
>

Yep, fixed.
>
>> +
>> +      gcry_err =
>> +    gcry_mpi_scan (&(msg->signerInfos[i].sig_mpi), GCRYMPI_FMT_USG,
>> +                   result_buf, result_size, NULL);
>> +      if (gcry_err != GPG_ERR_NO_ERROR)
>> +    {
>> +      err =
>> +        grub_error (GRUB_ERR_BAD_SIGNATURE,
>> +                    "Error loading signature %d into MPI structure: %d",
>> +                    i, gcry_err);
>> +      grub_free (result_buf);
>> +      goto cleanup_signerInfos;
>> +    }
>> +
>> +      grub_free (result_buf);
>
> Same with this one.
>
>
Fixed.

>> +
>> +      /* use msg->signerInfo_count to track fully populated signerInfos so 
>> we
>> +         know how many we need to clean up */
>> +      msg->signerInfo_count++;
>> +    }

>> +/*
>> + * 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)
>> +{
>> +  grub_ssize_t i;
>> +  for (i = 0; i < msg->signerInfo_count; i++)
>
>
> Nit: probably empty line after variable declaration

Done
>
>
>> +    {
>> +      gcry_mpi_release (msg->signerInfos[i].sig_mpi);
>> +    }
>> +  grub_free (msg->signerInfos);
>> +}
>> diff --git a/grub-core/commands/appendedsig/x509.c 
>> b/grub-core/commands/appendedsig/x509.c
>> new file mode 100644
>> index 000000000000..a17a46102872
>> --- /dev/null
>> +++ b/grub-core/commands/appendedsig/x509.c
>> @@ -0,0 +1,1079 @@
>> +/*
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2020  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"
>> +
>> +static char asn1_error[ASN1_MAX_ERROR_DESCRIPTION_SIZE];
>> +
>> +/*
>> + * RFC 3279 2.3.1  RSA Keys
>> + */
>> +const char *rsaEncryption_oid = "1.2.840.113549.1.1.1";
>> +
>> +/*
>> + * RFC 5280 Appendix A
>> + */
>> +const char *commonName_oid = "2.5.4.3";
>> +
>> +/*
>> + * RFC 5280 4.2.1.3 Key Usage
>> + */
>> +const char *keyUsage_oid = "2.5.29.15";
>> +
>> +const grub_uint8_t digitalSignatureUsage = 0x80;
>> +
>> +/*
>> + * RFC 5280 4.2.1.9 Basic Constraints
>> + */
>> +const char *basicConstraints_oid = "2.5.29.19";
>> +
>> +/*
>> + * RFC 5280 4.2.1.12 Extended Key Usage
>> + */
>> +const char *extendedKeyUsage_oid = "2.5.29.37";
>> +const char *codeSigningUsage_oid = "1.3.6.1.5.5.7.3.3";
>
>
> Should they be visible to other modules or only used here and can be 
> 'static'?
>
Indeed you are right. Marked as static.


>> +  result = asn1_der_decoding2 (&extendedasn, value, &value_size,
>> +                           ASN1_DECODE_FLAG_STRICT_DER, asn1_error);
>> +  if (result != ASN1_SUCCESS)
>> +    {
>> +      err =
>> +    grub_error (GRUB_ERR_BAD_FILE_TYPE,
>> +                "Error parsing DER for Extended Key Usage: %s",
>> +                asn1_error);
>> +      goto cleanup;
>> +    }
>> +
>> +  /*
>> +   * If EKUs are present, there must be exactly 1 and it must be a
>> +   * codeSigning usage.
>
>
> Is this comment correct? It looks like your code requires one EKU.
>

That function will only be called if we have an EKU extension in
verify_extensions. I will clarify the comment: what it should be saying
is if we have an EKU extension, there must be at least 1 usage.

Thank you for the detailed review, I'm sorry it's taken me so long to respond!

Kind regards,
Daniel