Stefan Berger <stef...@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 <javi...@redhat.com> # EKU support >> Signed-off-by: Daniel Axtens <d...@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 <d...@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 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel