| Message ID | 20200430173630.15608-6-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | qemu: arm64: Add support for uefi firmware management protocol routines | expand |
On Thu, Apr 30, 2020 at 11:06:27PM +0530, Sughosh Ganu wrote: > The pkcs7 header parsing functionality is pretty generic, and can be > used by other features like capsule authentication. Make the function > as an extern, also changing it's name to efi_parse_pkcs7_header. The patch itself is fine to me, but is "pkcs7 header" a common term? -Takahiro Akashi > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org> > --- > include/efi_loader.h | 2 + > lib/efi_loader/efi_signature.c | 78 ++++++++++++++++++++++++++++++++ > lib/efi_loader/efi_variable.c | 82 +--------------------------------- > 3 files changed, 82 insertions(+), 80 deletions(-) > > diff --git a/include/efi_loader.h b/include/efi_loader.h > index b7638d5825..8d923451ce 100644 > --- a/include/efi_loader.h > +++ b/include/efi_loader.h > @@ -781,6 +781,8 @@ bool efi_secure_boot_enabled(void); > > bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp, > WIN_CERTIFICATE **auth, size_t *auth_len); > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen); > + > #endif /* CONFIG_EFI_SECURE_BOOT */ > > #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c > index bf6f39aab2..9897f5418e 100644 > --- a/lib/efi_loader/efi_signature.c > +++ b/lib/efi_loader/efi_signature.c > @@ -25,6 +25,84 @@ const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID; > > #ifdef CONFIG_EFI_SECURE_BOOT > > +static u8 pkcs7_hdr[] = { > + /* SEQUENCE */ > + 0x30, 0x82, 0x05, 0xc7, > + /* OID: pkcs7-signedData */ > + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, > + /* Context Structured? */ > + 0xa0, 0x82, 0x05, 0xb8, > +}; > + > +/** > + * efi_parse_pkcs7_header - parse a signature in variable > + * @buf: Pointer to the payload's value > + * @buflen: Length of @buf > + * > + * Parse a signature embedded in variable's value and instantiate > + * a pkcs7_message structure. Since pkcs7_parse_message() accepts only > + * pkcs7's signedData, some header needed be prepended for correctly > + * parsing authentication data, particularly for variable's. > + * > + * Return: Pointer to pkcs7_message structure on success, NULL on error > + */ > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen) > +{ > + u8 *ebuf; > + size_t ebuflen, len; > + struct pkcs7_message *msg; > + > + /* > + * This is the best assumption to check if the binary is > + * already in a form of pkcs7's signedData. > + */ > + if (buflen > sizeof(pkcs7_hdr) && > + !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) { > + msg = pkcs7_parse_message(buf, buflen); > + goto out; > + } > + > + /* > + * Otherwise, we should add a dummy prefix sequence for pkcs7 > + * message parser to be able to process. > + * NOTE: EDK2 also uses similar hack in WrapPkcs7Data() > + * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c > + * TODO: > + * The header should be composed in a more refined manner. > + */ > + debug("Makeshift prefix added to authentication data\n"); > + ebuflen = sizeof(pkcs7_hdr) + buflen; > + if (ebuflen <= 0x7f) { > + debug("Data is too short\n"); > + return NULL; > + } > + > + ebuf = malloc(ebuflen); > + if (!ebuf) { > + debug("Out of memory\n"); > + return NULL; > + } > + > + memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr)); > + memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen); > + len = ebuflen - 4; > + ebuf[2] = (len >> 8) & 0xff; > + ebuf[3] = len & 0xff; > + len = ebuflen - 0x13; > + ebuf[0x11] = (len >> 8) & 0xff; > + ebuf[0x12] = len & 0xff; > + > + msg = pkcs7_parse_message(ebuf, ebuflen); > + > + free(ebuf); > + > +out: > + if (IS_ERR(msg)) > + return NULL; > + > + return msg; > +} > + > /** > * efi_hash_regions - calculate a hash value > * @regs: List of regions > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 6c2dd82306..be34a2cadd 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -415,85 +415,7 @@ bool efi_secure_boot_enabled(void) > return efi_secure_boot; > } > > -#ifdef CONFIG_EFI_SECURE_BOOT > -static u8 pkcs7_hdr[] = { > - /* SEQUENCE */ > - 0x30, 0x82, 0x05, 0xc7, > - /* OID: pkcs7-signedData */ > - 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, > - /* Context Structured? */ > - 0xa0, 0x82, 0x05, 0xb8, > -}; > - > -/** > - * efi_variable_parse_signature - parse a signature in variable > - * @buf: Pointer to variable's value > - * @buflen: Length of @buf > - * > - * Parse a signature embedded in variable's value and instantiate > - * a pkcs7_message structure. Since pkcs7_parse_message() accepts only > - * pkcs7's signedData, some header needed be prepended for correctly > - * parsing authentication data, particularly for variable's. > - * > - * Return: Pointer to pkcs7_message structure on success, NULL on error > - */ > -static struct pkcs7_message *efi_variable_parse_signature(const void *buf, > - size_t buflen) > -{ > - u8 *ebuf; > - size_t ebuflen, len; > - struct pkcs7_message *msg; > - > - /* > - * This is the best assumption to check if the binary is > - * already in a form of pkcs7's signedData. > - */ > - if (buflen > sizeof(pkcs7_hdr) && > - !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) { > - msg = pkcs7_parse_message(buf, buflen); > - goto out; > - } > - > - /* > - * Otherwise, we should add a dummy prefix sequence for pkcs7 > - * message parser to be able to process. > - * NOTE: EDK2 also uses similar hack in WrapPkcs7Data() > - * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c > - * TODO: > - * The header should be composed in a more refined manner. > - */ > - debug("Makeshift prefix added to authentication data\n"); > - ebuflen = sizeof(pkcs7_hdr) + buflen; > - if (ebuflen <= 0x7f) { > - debug("Data is too short\n"); > - return NULL; > - } > - > - ebuf = malloc(ebuflen); > - if (!ebuf) { > - debug("Out of memory\n"); > - return NULL; > - } > - > - memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr)); > - memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen); > - len = ebuflen - 4; > - ebuf[2] = (len >> 8) & 0xff; > - ebuf[3] = len & 0xff; > - len = ebuflen - 0x13; > - ebuf[0x11] = (len >> 8) & 0xff; > - ebuf[0x12] = len & 0xff; > - > - msg = pkcs7_parse_message(ebuf, ebuflen); > - > - free(ebuf); > - > -out: > - if (IS_ERR(msg)) > - return NULL; > - > - return msg; > -} > +#ifdef CONFIG_SECURE_BOOT > > /** > * efi_variable_authenticate - authenticate a variable > @@ -591,7 +513,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable, > /* variable's signature list */ > if (auth->auth_info.hdr.dwLength < sizeof(auth->auth_info)) > goto err; > - var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, > + var_sig = efi_parse_pkcs7_header(auth->auth_info.cert_data, > auth->auth_info.hdr.dwLength > - sizeof(auth->auth_info)); > if (IS_ERR(var_sig)) { > -- > 2.17.1 >
On Thu, 7 May 2020 at 13:04, Akashi Takahiro <takahiro.akashi at linaro.org> wrote: > On Thu, Apr 30, 2020 at 11:06:27PM +0530, Sughosh Ganu wrote: > > The pkcs7 header parsing functionality is pretty generic, and can be > > used by other features like capsule authentication. Make the function > > as an extern, also changing it's name to efi_parse_pkcs7_header. > > The patch itself is fine to me, but is "pkcs7 header" a common term? > I haven't come across it in any other code base. I used it since in the concept of a capsule, the signature is prepended to the capsule payload. If you can think of a better name, please suggest so. I will change it in the next version. -sughosh > > -Takahiro Akashi > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org> > > --- > > include/efi_loader.h | 2 + > > lib/efi_loader/efi_signature.c | 78 ++++++++++++++++++++++++++++++++ > > lib/efi_loader/efi_variable.c | 82 +--------------------------------- > > 3 files changed, 82 insertions(+), 80 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index b7638d5825..8d923451ce 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -781,6 +781,8 @@ bool efi_secure_boot_enabled(void); > > > > bool efi_image_parse(void *efi, size_t len, struct efi_image_regions > **regp, > > WIN_CERTIFICATE **auth, size_t *auth_len); > > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t > buflen); > > + > > #endif /* CONFIG_EFI_SECURE_BOOT */ > > > > #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT > > diff --git a/lib/efi_loader/efi_signature.c > b/lib/efi_loader/efi_signature.c > > index bf6f39aab2..9897f5418e 100644 > > --- a/lib/efi_loader/efi_signature.c > > +++ b/lib/efi_loader/efi_signature.c > > @@ -25,6 +25,84 @@ const efi_guid_t efi_guid_cert_x509_sha256 = > EFI_CERT_X509_SHA256_GUID; > > > > #ifdef CONFIG_EFI_SECURE_BOOT > > > > +static u8 pkcs7_hdr[] = { > > + /* SEQUENCE */ > > + 0x30, 0x82, 0x05, 0xc7, > > + /* OID: pkcs7-signedData */ > > + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, > > + /* Context Structured? */ > > + 0xa0, 0x82, 0x05, 0xb8, > > +}; > > + > > +/** > > + * efi_parse_pkcs7_header - parse a signature in variable > > + * @buf: Pointer to the payload's value > > + * @buflen: Length of @buf > > + * > > + * Parse a signature embedded in variable's value and instantiate > > + * a pkcs7_message structure. Since pkcs7_parse_message() accepts only > > + * pkcs7's signedData, some header needed be prepended for correctly > > + * parsing authentication data, particularly for variable's. > > + * > > + * Return: Pointer to pkcs7_message structure on success, NULL on > error > > + */ > > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t > buflen) > > +{ > > + u8 *ebuf; > > + size_t ebuflen, len; > > + struct pkcs7_message *msg; > > + > > + /* > > + * This is the best assumption to check if the binary is > > + * already in a form of pkcs7's signedData. > > + */ > > + if (buflen > sizeof(pkcs7_hdr) && > > + !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) { > > + msg = pkcs7_parse_message(buf, buflen); > > + goto out; > > + } > > + > > + /* > > + * Otherwise, we should add a dummy prefix sequence for pkcs7 > > + * message parser to be able to process. > > + * NOTE: EDK2 also uses similar hack in WrapPkcs7Data() > > + * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c > > + * TODO: > > + * The header should be composed in a more refined manner. > > + */ > > + debug("Makeshift prefix added to authentication data\n"); > > + ebuflen = sizeof(pkcs7_hdr) + buflen; > > + if (ebuflen <= 0x7f) { > > + debug("Data is too short\n"); > > + return NULL; > > + } > > + > > + ebuf = malloc(ebuflen); > > + if (!ebuf) { > > + debug("Out of memory\n"); > > + return NULL; > > + } > > + > > + memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr)); > > + memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen); > > + len = ebuflen - 4; > > + ebuf[2] = (len >> 8) & 0xff; > > + ebuf[3] = len & 0xff; > > + len = ebuflen - 0x13; > > + ebuf[0x11] = (len >> 8) & 0xff; > > + ebuf[0x12] = len & 0xff; > > + > > + msg = pkcs7_parse_message(ebuf, ebuflen); > > + > > + free(ebuf); > > + > > +out: > > + if (IS_ERR(msg)) > > + return NULL; > > + > > + return msg; > > +} > > + > > /** > > * efi_hash_regions - calculate a hash value > > * @regs: List of regions > > diff --git a/lib/efi_loader/efi_variable.c > b/lib/efi_loader/efi_variable.c > > index 6c2dd82306..be34a2cadd 100644 > > --- a/lib/efi_loader/efi_variable.c > > +++ b/lib/efi_loader/efi_variable.c > > @@ -415,85 +415,7 @@ bool efi_secure_boot_enabled(void) > > return efi_secure_boot; > > } > > > > -#ifdef CONFIG_EFI_SECURE_BOOT > > -static u8 pkcs7_hdr[] = { > > - /* SEQUENCE */ > > - 0x30, 0x82, 0x05, 0xc7, > > - /* OID: pkcs7-signedData */ > > - 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, > > - /* Context Structured? */ > > - 0xa0, 0x82, 0x05, 0xb8, > > -}; > > - > > -/** > > - * efi_variable_parse_signature - parse a signature in variable > > - * @buf: Pointer to variable's value > > - * @buflen: Length of @buf > > - * > > - * Parse a signature embedded in variable's value and instantiate > > - * a pkcs7_message structure. Since pkcs7_parse_message() accepts only > > - * pkcs7's signedData, some header needed be prepended for correctly > > - * parsing authentication data, particularly for variable's. > > - * > > - * Return: Pointer to pkcs7_message structure on success, NULL on > error > > - */ > > -static struct pkcs7_message *efi_variable_parse_signature(const void > *buf, > > - size_t buflen) > > -{ > > - u8 *ebuf; > > - size_t ebuflen, len; > > - struct pkcs7_message *msg; > > - > > - /* > > - * This is the best assumption to check if the binary is > > - * already in a form of pkcs7's signedData. > > - */ > > - if (buflen > sizeof(pkcs7_hdr) && > > - !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) { > > - msg = pkcs7_parse_message(buf, buflen); > > - goto out; > > - } > > - > > - /* > > - * Otherwise, we should add a dummy prefix sequence for pkcs7 > > - * message parser to be able to process. > > - * NOTE: EDK2 also uses similar hack in WrapPkcs7Data() > > - * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c > > - * TODO: > > - * The header should be composed in a more refined manner. > > - */ > > - debug("Makeshift prefix added to authentication data\n"); > > - ebuflen = sizeof(pkcs7_hdr) + buflen; > > - if (ebuflen <= 0x7f) { > > - debug("Data is too short\n"); > > - return NULL; > > - } > > - > > - ebuf = malloc(ebuflen); > > - if (!ebuf) { > > - debug("Out of memory\n"); > > - return NULL; > > - } > > - > > - memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr)); > > - memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen); > > - len = ebuflen - 4; > > - ebuf[2] = (len >> 8) & 0xff; > > - ebuf[3] = len & 0xff; > > - len = ebuflen - 0x13; > > - ebuf[0x11] = (len >> 8) & 0xff; > > - ebuf[0x12] = len & 0xff; > > - > > - msg = pkcs7_parse_message(ebuf, ebuflen); > > - > > - free(ebuf); > > - > > -out: > > - if (IS_ERR(msg)) > > - return NULL; > > - > > - return msg; > > -} > > +#ifdef CONFIG_SECURE_BOOT > > > > /** > > * efi_variable_authenticate - authenticate a variable > > @@ -591,7 +513,7 @@ static efi_status_t efi_variable_authenticate(u16 > *variable, > > /* variable's signature list */ > > if (auth->auth_info.hdr.dwLength < sizeof(auth->auth_info)) > > goto err; > > - var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, > > + var_sig = efi_parse_pkcs7_header(auth->auth_info.cert_data, > > auth->auth_info.hdr.dwLength > > - > sizeof(auth->auth_info)); > > if (IS_ERR(var_sig)) { > > -- > > 2.17.1 > > >
On Thu, May 07, 2020 at 04:48:30PM +0530, Sughosh Ganu wrote: > On Thu, 7 May 2020 at 13:04, Akashi Takahiro <takahiro.akashi at linaro.org> > wrote: > > > On Thu, Apr 30, 2020 at 11:06:27PM +0530, Sughosh Ganu wrote: > > > The pkcs7 header parsing functionality is pretty generic, and can be > > > used by other features like capsule authentication. Make the function > > > as an extern, also changing it's name to efi_parse_pkcs7_header. > > > > The patch itself is fine to me, but is "pkcs7 header" a common term? > > > > I haven't come across it in any other code base. I used it since in the > concept of a capsule, the signature is prepended to the capsule payload. If > you can think of a better name, please suggest so. I will change it in the > next version. Simply, efi_parse_pkcs7_signature()? In addition, please update the function description, which still mentions "variable." -Takahiro Akashi > -sughosh > > > > > > -Takahiro Akashi > > > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org> > > > --- > > > include/efi_loader.h | 2 + > > > lib/efi_loader/efi_signature.c | 78 ++++++++++++++++++++++++++++++++ > > > lib/efi_loader/efi_variable.c | 82 +--------------------------------- > > > 3 files changed, 82 insertions(+), 80 deletions(-) > > > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > > index b7638d5825..8d923451ce 100644 > > > --- a/include/efi_loader.h > > > +++ b/include/efi_loader.h > > > @@ -781,6 +781,8 @@ bool efi_secure_boot_enabled(void); > > > > > > bool efi_image_parse(void *efi, size_t len, struct efi_image_regions > > **regp, > > > WIN_CERTIFICATE **auth, size_t *auth_len); > > > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t > > buflen); > > > + > > > #endif /* CONFIG_EFI_SECURE_BOOT */ > > > > > > #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT > > > diff --git a/lib/efi_loader/efi_signature.c > > b/lib/efi_loader/efi_signature.c > > > index bf6f39aab2..9897f5418e 100644 > > > --- a/lib/efi_loader/efi_signature.c > > > +++ b/lib/efi_loader/efi_signature.c > > > @@ -25,6 +25,84 @@ const efi_guid_t efi_guid_cert_x509_sha256 = > > EFI_CERT_X509_SHA256_GUID; > > > > > > #ifdef CONFIG_EFI_SECURE_BOOT > > > > > > +static u8 pkcs7_hdr[] = { > > > + /* SEQUENCE */ > > > + 0x30, 0x82, 0x05, 0xc7, > > > + /* OID: pkcs7-signedData */ > > > + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, > > > + /* Context Structured? */ > > > + 0xa0, 0x82, 0x05, 0xb8, > > > +}; > > > + > > > +/** > > > + * efi_parse_pkcs7_header - parse a signature in variable > > > + * @buf: Pointer to the payload's value > > > + * @buflen: Length of @buf > > > + * > > > + * Parse a signature embedded in variable's value and instantiate > > > + * a pkcs7_message structure. Since pkcs7_parse_message() accepts only > > > + * pkcs7's signedData, some header needed be prepended for correctly > > > + * parsing authentication data, particularly for variable's. > > > + * > > > + * Return: Pointer to pkcs7_message structure on success, NULL on > > error > > > + */ > > > +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t > > buflen) > > > +{ > > > + u8 *ebuf; > > > + size_t ebuflen, len; > > > + struct pkcs7_message *msg; > > > + > > > + /* > > > + * This is the best assumption to check if the binary is > > > + * already in a form of pkcs7's signedData. > > > + */ > > > + if (buflen > sizeof(pkcs7_hdr) && > > > + !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) { > > > + msg = pkcs7_parse_message(buf, buflen); > > > + goto out; > > > + } > > > + > > > + /* > > > + * Otherwise, we should add a dummy prefix sequence for pkcs7 > > > + * message parser to be able to process. > > > + * NOTE: EDK2 also uses similar hack in WrapPkcs7Data() > > > + * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c > > > + * TODO: > > > + * The header should be composed in a more refined manner. > > > + */ > > > + debug("Makeshift prefix added to authentication data\n"); > > > + ebuflen = sizeof(pkcs7_hdr) + buflen; > > > + if (ebuflen <= 0x7f) { > > > + debug("Data is too short\n"); > > > + return NULL; > > > + } > > > + > > > + ebuf = malloc(ebuflen); > > > + if (!ebuf) { > > > + debug("Out of memory\n"); > > > + return NULL; > > > + } > > > + > > > + memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr)); > > > + memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen); > > > + len = ebuflen - 4; > > > + ebuf[2] = (len >> 8) & 0xff; > > > + ebuf[3] = len & 0xff; > > > + len = ebuflen - 0x13; > > > + ebuf[0x11] = (len >> 8) & 0xff; > > > + ebuf[0x12] = len & 0xff; > > > + > > > + msg = pkcs7_parse_message(ebuf, ebuflen); > > > + > > > + free(ebuf); > > > + > > > +out: > > > + if (IS_ERR(msg)) > > > + return NULL; > > > + > > > + return msg; > > > +} > > > + > > > /** > > > * efi_hash_regions - calculate a hash value > > > * @regs: List of regions > > > diff --git a/lib/efi_loader/efi_variable.c > > b/lib/efi_loader/efi_variable.c > > > index 6c2dd82306..be34a2cadd 100644 > > > --- a/lib/efi_loader/efi_variable.c > > > +++ b/lib/efi_loader/efi_variable.c > > > @@ -415,85 +415,7 @@ bool efi_secure_boot_enabled(void) > > > return efi_secure_boot; > > > } > > > > > > -#ifdef CONFIG_EFI_SECURE_BOOT > > > -static u8 pkcs7_hdr[] = { > > > - /* SEQUENCE */ > > > - 0x30, 0x82, 0x05, 0xc7, > > > - /* OID: pkcs7-signedData */ > > > - 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, > > > - /* Context Structured? */ > > > - 0xa0, 0x82, 0x05, 0xb8, > > > -}; > > > - > > > -/** > > > - * efi_variable_parse_signature - parse a signature in variable > > > - * @buf: Pointer to variable's value > > > - * @buflen: Length of @buf > > > - * > > > - * Parse a signature embedded in variable's value and instantiate > > > - * a pkcs7_message structure. Since pkcs7_parse_message() accepts only > > > - * pkcs7's signedData, some header needed be prepended for correctly > > > - * parsing authentication data, particularly for variable's. > > > - * > > > - * Return: Pointer to pkcs7_message structure on success, NULL on > > error > > > - */ > > > -static struct pkcs7_message *efi_variable_parse_signature(const void > > *buf, > > > - size_t buflen) > > > -{ > > > - u8 *ebuf; > > > - size_t ebuflen, len; > > > - struct pkcs7_message *msg; > > > - > > > - /* > > > - * This is the best assumption to check if the binary is > > > - * already in a form of pkcs7's signedData. > > > - */ > > > - if (buflen > sizeof(pkcs7_hdr) && > > > - !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) { > > > - msg = pkcs7_parse_message(buf, buflen); > > > - goto out; > > > - } > > > - > > > - /* > > > - * Otherwise, we should add a dummy prefix sequence for pkcs7 > > > - * message parser to be able to process. > > > - * NOTE: EDK2 also uses similar hack in WrapPkcs7Data() > > > - * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c > > > - * TODO: > > > - * The header should be composed in a more refined manner. > > > - */ > > > - debug("Makeshift prefix added to authentication data\n"); > > > - ebuflen = sizeof(pkcs7_hdr) + buflen; > > > - if (ebuflen <= 0x7f) { > > > - debug("Data is too short\n"); > > > - return NULL; > > > - } > > > - > > > - ebuf = malloc(ebuflen); > > > - if (!ebuf) { > > > - debug("Out of memory\n"); > > > - return NULL; > > > - } > > > - > > > - memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr)); > > > - memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen); > > > - len = ebuflen - 4; > > > - ebuf[2] = (len >> 8) & 0xff; > > > - ebuf[3] = len & 0xff; > > > - len = ebuflen - 0x13; > > > - ebuf[0x11] = (len >> 8) & 0xff; > > > - ebuf[0x12] = len & 0xff; > > > - > > > - msg = pkcs7_parse_message(ebuf, ebuflen); > > > - > > > - free(ebuf); > > > - > > > -out: > > > - if (IS_ERR(msg)) > > > - return NULL; > > > - > > > - return msg; > > > -} > > > +#ifdef CONFIG_SECURE_BOOT > > > > > > /** > > > * efi_variable_authenticate - authenticate a variable > > > @@ -591,7 +513,7 @@ static efi_status_t efi_variable_authenticate(u16 > > *variable, > > > /* variable's signature list */ > > > if (auth->auth_info.hdr.dwLength < sizeof(auth->auth_info)) > > > goto err; > > > - var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, > > > + var_sig = efi_parse_pkcs7_header(auth->auth_info.cert_data, > > > auth->auth_info.hdr.dwLength > > > - > > sizeof(auth->auth_info)); > > > if (IS_ERR(var_sig)) { > > > -- > > > 2.17.1 > > > > >
On Fri, 8 May 2020 at 06:21, Akashi Takahiro <takahiro.akashi at linaro.org> wrote: > On Thu, May 07, 2020 at 04:48:30PM +0530, Sughosh Ganu wrote: > > On Thu, 7 May 2020 at 13:04, Akashi Takahiro <takahiro.akashi at linaro.org > > > > wrote: > > > > > On Thu, Apr 30, 2020 at 11:06:27PM +0530, Sughosh Ganu wrote: > > > > The pkcs7 header parsing functionality is pretty generic, and can be > > > > used by other features like capsule authentication. Make the function > > > > as an extern, also changing it's name to efi_parse_pkcs7_header. > > > > > > The patch itself is fine to me, but is "pkcs7 header" a common term? > > > > > > > I haven't come across it in any other code base. I used it since in the > > concept of a capsule, the signature is prepended to the capsule payload. > If > > you can think of a better name, please suggest so. I will change it in > the > > next version. > > Simply, efi_parse_pkcs7_signature()? > > In addition, please update the function description, which still > mentions "variable." > Ok. Will change. -sughosh
diff --git a/include/efi_loader.h b/include/efi_loader.h index b7638d5825..8d923451ce 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -781,6 +781,8 @@ bool efi_secure_boot_enabled(void); bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp, WIN_CERTIFICATE **auth, size_t *auth_len); +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen); + #endif /* CONFIG_EFI_SECURE_BOOT */ #ifdef CONFIG_EFI_HAVE_CAPSULE_SUPPORT diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c index bf6f39aab2..9897f5418e 100644 --- a/lib/efi_loader/efi_signature.c +++ b/lib/efi_loader/efi_signature.c @@ -25,6 +25,84 @@ const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID; #ifdef CONFIG_EFI_SECURE_BOOT +static u8 pkcs7_hdr[] = { + /* SEQUENCE */ + 0x30, 0x82, 0x05, 0xc7, + /* OID: pkcs7-signedData */ + 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, + /* Context Structured? */ + 0xa0, 0x82, 0x05, 0xb8, +}; + +/** + * efi_parse_pkcs7_header - parse a signature in variable + * @buf: Pointer to the payload's value + * @buflen: Length of @buf + * + * Parse a signature embedded in variable's value and instantiate + * a pkcs7_message structure. Since pkcs7_parse_message() accepts only + * pkcs7's signedData, some header needed be prepended for correctly + * parsing authentication data, particularly for variable's. + * + * Return: Pointer to pkcs7_message structure on success, NULL on error + */ +struct pkcs7_message *efi_parse_pkcs7_header(const void *buf, size_t buflen) +{ + u8 *ebuf; + size_t ebuflen, len; + struct pkcs7_message *msg; + + /* + * This is the best assumption to check if the binary is + * already in a form of pkcs7's signedData. + */ + if (buflen > sizeof(pkcs7_hdr) && + !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) { + msg = pkcs7_parse_message(buf, buflen); + goto out; + } + + /* + * Otherwise, we should add a dummy prefix sequence for pkcs7 + * message parser to be able to process. + * NOTE: EDK2 also uses similar hack in WrapPkcs7Data() + * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c + * TODO: + * The header should be composed in a more refined manner. + */ + debug("Makeshift prefix added to authentication data\n"); + ebuflen = sizeof(pkcs7_hdr) + buflen; + if (ebuflen <= 0x7f) { + debug("Data is too short\n"); + return NULL; + } + + ebuf = malloc(ebuflen); + if (!ebuf) { + debug("Out of memory\n"); + return NULL; + } + + memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr)); + memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen); + len = ebuflen - 4; + ebuf[2] = (len >> 8) & 0xff; + ebuf[3] = len & 0xff; + len = ebuflen - 0x13; + ebuf[0x11] = (len >> 8) & 0xff; + ebuf[0x12] = len & 0xff; + + msg = pkcs7_parse_message(ebuf, ebuflen); + + free(ebuf); + +out: + if (IS_ERR(msg)) + return NULL; + + return msg; +} + /** * efi_hash_regions - calculate a hash value * @regs: List of regions diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index 6c2dd82306..be34a2cadd 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -415,85 +415,7 @@ bool efi_secure_boot_enabled(void) return efi_secure_boot; } -#ifdef CONFIG_EFI_SECURE_BOOT -static u8 pkcs7_hdr[] = { - /* SEQUENCE */ - 0x30, 0x82, 0x05, 0xc7, - /* OID: pkcs7-signedData */ - 0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x0d, 0x01, 0x07, 0x02, - /* Context Structured? */ - 0xa0, 0x82, 0x05, 0xb8, -}; - -/** - * efi_variable_parse_signature - parse a signature in variable - * @buf: Pointer to variable's value - * @buflen: Length of @buf - * - * Parse a signature embedded in variable's value and instantiate - * a pkcs7_message structure. Since pkcs7_parse_message() accepts only - * pkcs7's signedData, some header needed be prepended for correctly - * parsing authentication data, particularly for variable's. - * - * Return: Pointer to pkcs7_message structure on success, NULL on error - */ -static struct pkcs7_message *efi_variable_parse_signature(const void *buf, - size_t buflen) -{ - u8 *ebuf; - size_t ebuflen, len; - struct pkcs7_message *msg; - - /* - * This is the best assumption to check if the binary is - * already in a form of pkcs7's signedData. - */ - if (buflen > sizeof(pkcs7_hdr) && - !memcmp(&((u8 *)buf)[4], &pkcs7_hdr[4], 11)) { - msg = pkcs7_parse_message(buf, buflen); - goto out; - } - - /* - * Otherwise, we should add a dummy prefix sequence for pkcs7 - * message parser to be able to process. - * NOTE: EDK2 also uses similar hack in WrapPkcs7Data() - * in CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyCommon.c - * TODO: - * The header should be composed in a more refined manner. - */ - debug("Makeshift prefix added to authentication data\n"); - ebuflen = sizeof(pkcs7_hdr) + buflen; - if (ebuflen <= 0x7f) { - debug("Data is too short\n"); - return NULL; - } - - ebuf = malloc(ebuflen); - if (!ebuf) { - debug("Out of memory\n"); - return NULL; - } - - memcpy(ebuf, pkcs7_hdr, sizeof(pkcs7_hdr)); - memcpy(ebuf + sizeof(pkcs7_hdr), buf, buflen); - len = ebuflen - 4; - ebuf[2] = (len >> 8) & 0xff; - ebuf[3] = len & 0xff; - len = ebuflen - 0x13; - ebuf[0x11] = (len >> 8) & 0xff; - ebuf[0x12] = len & 0xff; - - msg = pkcs7_parse_message(ebuf, ebuflen); - - free(ebuf); - -out: - if (IS_ERR(msg)) - return NULL; - - return msg; -} +#ifdef CONFIG_SECURE_BOOT /** * efi_variable_authenticate - authenticate a variable @@ -591,7 +513,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable, /* variable's signature list */ if (auth->auth_info.hdr.dwLength < sizeof(auth->auth_info)) goto err; - var_sig = efi_variable_parse_signature(auth->auth_info.cert_data, + var_sig = efi_parse_pkcs7_header(auth->auth_info.cert_data, auth->auth_info.hdr.dwLength - sizeof(auth->auth_info)); if (IS_ERR(var_sig)) {
The pkcs7 header parsing functionality is pretty generic, and can be used by other features like capsule authentication. Make the function as an extern, also changing it's name to efi_parse_pkcs7_header. Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org> --- include/efi_loader.h | 2 + lib/efi_loader/efi_signature.c | 78 ++++++++++++++++++++++++++++++++ lib/efi_loader/efi_variable.c | 82 +--------------------------------- 3 files changed, 82 insertions(+), 80 deletions(-)