Date
1 - 8 of 8
SubRegionAuthLib RFC
Mackay, Curtis A <curtis.a.mackay@...>
Hi,
I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.
A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.
To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.
Please provide feedback and comments on the design.
Best regards,
Curtis Mackay
Mackay, Curtis A <curtis.a.mackay@...>
Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.
Best,
Curtis
From: Mackay, Curtis A
Sent: Tuesday, June 16, 2020 10:51 AM
To: 'rfc@edk2.groups.io' <rfc@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: SubRegionAuthLib RFC
Sent: Tuesday, June 16, 2020 10:51 AM
To: 'rfc@edk2.groups.io' <rfc@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: SubRegionAuthLib RFC
Hi,
I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.
A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.
To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.
Please provide feedback and comments on the design.
Best regards,
Curtis Mackay
Laszlo Ersek
Hi,
On 06/19/20 18:55, Mackay, Curtis A wrote:
long, I've briefly skimmed it:
- It's unclear whether the public data structures describe a wire
format. If they do, they should be packed, IMO.
- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because
SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION
afterwards. Personally I prefer comments to the struct hack that explain
where the CertData starts. Out-of-bounds subscripts for CertData --
having just one element -- are dubious; we shouldn't have more of that
(IMO).
- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to
be NUL-terminated, or not.
- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it
should either be CHAR8 or CHAR16. If we want it to be any binary string,
then it should be UINT8.
- the EFI_ prefix is used inconsistently on the type names (not sure if
that's intentional -- I can't tell the principle behind the current use)
- in SIGNED_SUB_REGION, the field names are not aligned with each other,
and edk2 uses VOID spelling rather than "void". Also "blob" should be
CamelCased as "Blob".
- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?
- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type
UINTN, but (*SubRegionDataSize) has type UINT32. I think these types
should be the same.
- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)
- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a
pointer into the input buffer? Is it a copy, allocated dynamically?
I have no higher-level comments (i.e. on the intended use case).
Thanks
Laszlo
On 06/19/20 18:55, Mackay, Curtis A wrote:
Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.I don't have a stake in this feature, but because the presentation isn't
long, I've briefly skimmed it:
- It's unclear whether the public data structures describe a wire
format. If they do, they should be packed, IMO.
- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because
SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION
afterwards. Personally I prefer comments to the struct hack that explain
where the CertData starts. Out-of-bounds subscripts for CertData --
having just one element -- are dubious; we shouldn't have more of that
(IMO).
- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to
be NUL-terminated, or not.
- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it
should either be CHAR8 or CHAR16. If we want it to be any binary string,
then it should be UINT8.
- the EFI_ prefix is used inconsistently on the type names (not sure if
that's intentional -- I can't tell the principle behind the current use)
- in SIGNED_SUB_REGION, the field names are not aligned with each other,
and edk2 uses VOID spelling rather than "void". Also "blob" should be
CamelCased as "Blob".
- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?
- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type
UINTN, but (*SubRegionDataSize) has type UINT32. I think these types
should be the same.
- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)
- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a
pointer into the input buffer? Is it a copy, allocated dynamically?
I have no higher-level comments (i.e. on the intended use case).
Thanks
Laszlo
From: Mackay, Curtis A
Sent: Tuesday, June 16, 2020 10:51 AM
To: 'rfc@edk2.groups.io' <rfc@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: SubRegionAuthLib RFC
Hi,
I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.
A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.
To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.
Please provide feedback and comments on the design.
Best regards,
Curtis Mackay
Sukerkar, Amol N
Hi Laszlo,
Please see my comments below.
Thanks,
Amol
toggle quoted message
Show quoted text
Please see my comments below.
Thanks,
Amol
-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Thursday, June 25, 2020 4:57 AM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC
Hi,
On 06/19/20 18:55, Mackay, Curtis A wrote:
- It's unclear whether the public data structures describe a wire format. If they do, they should be packed, IMO.
[ANS] We tries to follow the precedence in other EDK2 implementation. Any suggestions to improve are welcome.
- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION afterwards. Personally I prefer comments to the struct hack that explain where the CertData starts. Out-of-bounds subscripts for CertData -- having just one element -- are dubious; we shouldn't have more of that (IMO).
[ANS] You can ignore SIGNED_SUB_REGION for now. The only intention here is to show that EFI_SUB_REGION_AUTHENTICATION is the header (with signature) and void *blob is the payload. So, even if you consider SUB_REGION_VERIFICATION as a member of EFI_SUB_REGION_AUTHENTICATION struct, CertData is still at the bottom of the struct stack. And depending on the type of signature the size will vary. But due to the nature of signature as PKCS#7, openssl API will be able to parse it. How do you propose we improve the struct?
- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to be NUL-terminated, or not.
[ANS] Name is stored in a bounded array. It uses memcmp to validate which is limited to MAX_NAME bytes. If it exceeds and corrupts VendorGuid, the authentication will fail since VendorGuid is part of data over which signature is calculated.
- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it should either be CHAR8 or CHAR16. If we want it to be any binary string, then it should be UINT8.
[ANS] Agree.
- the EFI_ prefix is used inconsistently on the type names (not sure if that's intentional -- I can't tell the principle behind the current use)
[ANS] Could you suggest how we can make it compliant here?
- in SIGNED_SUB_REGION, the field names are not aligned with each other, and edk2 uses VOID spelling rather than "void". Also "blob" should be CamelCased as "Blob".
[ANS] Agree. Needs to change.
- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?
[ANS] SUB_REGION_HEADER.Length = sizeof(SUB_REGION_HEADER) + sizeof (CertData). Do you think anything else needs to be done?
- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type UINTN, but (*SubRegionDataSize) has type UINT32. I think these types should be the same.
[ANS] Agree. Needs to change.
- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)
[ANS] Agree.
- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a pointer into the input buffer? Is it a copy, allocated dynamically?
[ANS] it is a pointer to original sub-region data (without signature header). If authentication is unsuccessful, it will be NULL. But, I agree AuthenticateSubRegion should just authenticate the sub-region. Parsing the data should be left to the caller.
I have no higher-level comments (i.e. on the intended use case).
Thanks
Laszlo
From: Laszlo Ersek <lersek@...>
Sent: Thursday, June 25, 2020 4:57 AM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC
Hi,
On 06/19/20 18:55, Mackay, Curtis A wrote:
Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.I don't have a stake in this feature, but because the presentation isn't long, I've briefly skimmed it:
- It's unclear whether the public data structures describe a wire format. If they do, they should be packed, IMO.
[ANS] We tries to follow the precedence in other EDK2 implementation. Any suggestions to improve are welcome.
- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION afterwards. Personally I prefer comments to the struct hack that explain where the CertData starts. Out-of-bounds subscripts for CertData -- having just one element -- are dubious; we shouldn't have more of that (IMO).
[ANS] You can ignore SIGNED_SUB_REGION for now. The only intention here is to show that EFI_SUB_REGION_AUTHENTICATION is the header (with signature) and void *blob is the payload. So, even if you consider SUB_REGION_VERIFICATION as a member of EFI_SUB_REGION_AUTHENTICATION struct, CertData is still at the bottom of the struct stack. And depending on the type of signature the size will vary. But due to the nature of signature as PKCS#7, openssl API will be able to parse it. How do you propose we improve the struct?
- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to be NUL-terminated, or not.
[ANS] Name is stored in a bounded array. It uses memcmp to validate which is limited to MAX_NAME bytes. If it exceeds and corrupts VendorGuid, the authentication will fail since VendorGuid is part of data over which signature is calculated.
- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it should either be CHAR8 or CHAR16. If we want it to be any binary string, then it should be UINT8.
[ANS] Agree.
- the EFI_ prefix is used inconsistently on the type names (not sure if that's intentional -- I can't tell the principle behind the current use)
[ANS] Could you suggest how we can make it compliant here?
- in SIGNED_SUB_REGION, the field names are not aligned with each other, and edk2 uses VOID spelling rather than "void". Also "blob" should be CamelCased as "Blob".
[ANS] Agree. Needs to change.
- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?
[ANS] SUB_REGION_HEADER.Length = sizeof(SUB_REGION_HEADER) + sizeof (CertData). Do you think anything else needs to be done?
- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type UINTN, but (*SubRegionDataSize) has type UINT32. I think these types should be the same.
[ANS] Agree. Needs to change.
- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)
[ANS] Agree.
- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a pointer into the input buffer? Is it a copy, allocated dynamically?
[ANS] it is a pointer to original sub-region data (without signature header). If authentication is unsuccessful, it will be NULL. But, I agree AuthenticateSubRegion should just authenticate the sub-region. Parsing the data should be left to the caller.
I have no higher-level comments (i.e. on the intended use case).
Thanks
Laszlo
From: Mackay, Curtis A
Sent: Tuesday, June 16, 2020 10:51 AM
To: 'rfc@edk2.groups.io' <rfc@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J
<jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar,
Amol N <amol.n.sukerkar@...>; Agrawal, Sachin
<sachin.agrawal@...>
Subject: SubRegionAuthLib RFC
Hi,
I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.
A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.
To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.
Please provide feedback and comments on the design.
Best regards,
Curtis Mackay
Sean
Curtis,
Not sure I fully follow your proposal. Can you provide more on the use case? Is the "blob" a FV or is the signed_sub_region a raw section in a ffs file? or something else like binary at flash offset?
The PI spec has a filesystem that describes many options and the DxeCore has support for security validation / authentication state flags associated with FVs and FFS files.
I have also seen many designs that leverage section extraction and doing authentication thru that guided sections.
Can you provide more background as to why it is important to get this into edk2 as a "standard" and why it requires defining new structures and new library abstractions?
Finally a point on the policy. In many products (especially commercial products) you don't see "UEFI secure boot (PK/KEK/DB/DBX)" leveraged for trust prior to EndOfDxe. Since UEFI secure boot is often user controlled this opens up your "platform" to compromise that can be impossible to recover from.
Thanks
Sean
toggle quoted message
Show quoted text
Not sure I fully follow your proposal. Can you provide more on the use case? Is the "blob" a FV or is the signed_sub_region a raw section in a ffs file? or something else like binary at flash offset?
The PI spec has a filesystem that describes many options and the DxeCore has support for security validation / authentication state flags associated with FVs and FFS files.
I have also seen many designs that leverage section extraction and doing authentication thru that guided sections.
Can you provide more background as to why it is important to get this into edk2 as a "standard" and why it requires defining new structures and new library abstractions?
Finally a point on the policy. In many products (especially commercial products) you don't see "UEFI secure boot (PK/KEK/DB/DBX)" leveraged for trust prior to EndOfDxe. Since UEFI secure boot is often user controlled this opens up your "platform" to compromise that can be impossible to recover from.
Thanks
Sean
On 6/16/2020 10:50 AM, Mackay, Curtis A wrote:
Hi,
I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.
A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.
To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.
Please provide feedback and comments on the design.
Best regards,
Curtis Mackay
Laszlo Ersek
On 06/25/20 21:25, Sukerkar, Amol N wrote:
serialized from RAM into a disk file, or to the network, then the
structure should be packed. If it's only exchanged in RAM, on a single
computer, between modules that all conform to the UEFI spec (which
dictates natural alignment for structure members), then fields in the
structure should not be packed.
SUB_REGION_VERIFICATION *Verification;
UINT8 Value;
Verification = ...;
Value = Verification->CertData[1];
is undefined behavior. The compiler knows that CertData has 1 element,
IOW that subscript 1 is out of bounds for the array. Compilers get ever
more aggressive in exploiting undefined behavior, so we should not
introduce more of it.
Simply remove the CertData field from the structure, and replace it with
a comment that states (in natural language) more or less the same. For
an example, refer to the "Description", "FilePathList" and
"OptionalData" fields in EFI_LOAD_OPTION, in file
"MdePkg/Include/Uefi/UefiSpec.h". (Or, in the matching part of the UEFI
2.8 spec itself -- "3.1.3 Load Options".)
be used, for the structures introduced for this feature.
Instead, my point was that either *all* of the new structures should use
_EFI, or *none* should.
- SUB_REGION_HEADER: no EFI_ prefix
- SUB_REGION_VERIFICATION: no EFI_ prefix
- EFI_SUB_REGION_AUTHENTICATION: yes EFI_ prefix
- SIGNED_SUB_REGION: no EFI_ prefix
My suggestion is to either prepend EFI_ to SUB_REGION_HEADER,
SUB_REGION_VERIFICATION, and SIGNED_SUB_REGION, *or* to strip the prefix
from EFI_SUB_REGION_AUTHENTICATION. Whichever you choose is up to
whether this feature is going to be standardized. But, either way, the
prefix usage should be consistent.
(1) The expression you provided does not take into account any possible
padding between the Hdr field and the CertData field. (See also my note
on packing.) So minimally, we should say
Verification.Hdr.Length = (OFFSET_OF (
SUB_REGION_VERIFICATION,
CertData
) +
CertDataBytes);
(2) Are we sure a UINT16 length field can cover all CertData fields
encountered in practice? Will there ever be a CertData that's larger
than ~64KB?
(3) But, my question actually relates to the size of the Blob field.
SIGNED_SUB_REGION expresses the base address of the region, but not the
size thereof. Is that intentional?
input (base, size) pair, and adjust both components of that pair on
output. That's OK, in my opinion.
It's just that the caller needs to know that the output pointer points
into the same original blob (in other words, that on success,
(*SubRegionDataBuffer) *aliases* SignedSubRegionImage).
This is relevant to avoid memory management errors (double freeing of
buffers, or memory leaks). Every function ever that outputs a pointer,
needs to explain in a comment to the caller, who is responsible for
freeing the pointed-to object.
Thanks
Laszlo
Hi Laszlo,Well as far as I'm concerned it's a simple rule. If a structure is
Please see my comments below.
Thanks,
Amol
-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Thursday, June 25, 2020 4:57 AM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC
Hi,
On 06/19/20 18:55, Mackay, Curtis A wrote:Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.I don't have a stake in this feature, but because the presentation isn't long, I've briefly skimmed it:
- It's unclear whether the public data structures describe a wire format. If they do, they should be packed, IMO.
[ANS] We tries to follow the precedence in other EDK2 implementation. Any suggestions to improve are welcome.
serialized from RAM into a disk file, or to the network, then the
structure should be packed. If it's only exchanged in RAM, on a single
computer, between modules that all conform to the UEFI spec (which
dictates natural alignment for structure members), then fields in the
structure should not be packed.
The struct hack is not desirable because a snippet like
- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION afterwards. Personally I prefer comments to the struct hack that explain where the CertData starts. Out-of-bounds subscripts for CertData -- having just one element -- are dubious; we shouldn't have more of that (IMO).
[ANS] You can ignore SIGNED_SUB_REGION for now. The only intention here is to show that EFI_SUB_REGION_AUTHENTICATION is the header (with signature) and void *blob is the payload. So, even if you consider SUB_REGION_VERIFICATION as a member of EFI_SUB_REGION_AUTHENTICATION struct, CertData is still at the bottom of the struct stack. And depending on the type of signature the size will vary. But due to the nature of signature as PKCS#7, openssl API will be able to parse it. How do you propose we improve the struct?
SUB_REGION_VERIFICATION *Verification;
UINT8 Value;
Verification = ...;
Value = Verification->CertData[1];
is undefined behavior. The compiler knows that CertData has 1 element,
IOW that subscript 1 is out of bounds for the array. Compilers get ever
more aggressive in exploiting undefined behavior, so we should not
introduce more of it.
Simply remove the CertData field from the structure, and replace it with
a comment that states (in natural language) more or less the same. For
an example, refer to the "Description", "FilePathList" and
"OptionalData" fields in EFI_LOAD_OPTION, in file
"MdePkg/Include/Uefi/UefiSpec.h". (Or, in the matching part of the UEFI
2.8 spec itself -- "3.1.3 Load Options".)
That makes sense, but a comment should document it.
- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to be NUL-terminated, or not.
[ANS] Name is stored in a bounded array. It uses memcmp to validate which is limited to MAX_NAME bytes. If it exceeds and corrupts VendorGuid, the authentication will fail since VendorGuid is part of data over which signature is calculated.
My point was *not* whether the EFI_ prefix should be used, or should not
- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it should either be CHAR8 or CHAR16. If we want it to be any binary string, then it should be UINT8.
[ANS] Agree.
- the EFI_ prefix is used inconsistently on the type names (not sure if that's intentional -- I can't tell the principle behind the current use)
[ANS] Could you suggest how we can make it compliant here?
be used, for the structures introduced for this feature.
Instead, my point was that either *all* of the new structures should use
_EFI, or *none* should.
- SUB_REGION_HEADER: no EFI_ prefix
- SUB_REGION_VERIFICATION: no EFI_ prefix
- EFI_SUB_REGION_AUTHENTICATION: yes EFI_ prefix
- SIGNED_SUB_REGION: no EFI_ prefix
My suggestion is to either prepend EFI_ to SUB_REGION_HEADER,
SUB_REGION_VERIFICATION, and SIGNED_SUB_REGION, *or* to strip the prefix
from EFI_SUB_REGION_AUTHENTICATION. Whichever you choose is up to
whether this feature is going to be standardized. But, either way, the
prefix usage should be consistent.
I have three concerns with this:
- in SIGNED_SUB_REGION, the field names are not aligned with each other, and edk2 uses VOID spelling rather than "void". Also "blob" should be CamelCased as "Blob".
[ANS] Agree. Needs to change.
- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?
[ANS] SUB_REGION_HEADER.Length = sizeof(SUB_REGION_HEADER) + sizeof (CertData). Do you think anything else needs to be done?
(1) The expression you provided does not take into account any possible
padding between the Hdr field and the CertData field. (See also my note
on packing.) So minimally, we should say
Verification.Hdr.Length = (OFFSET_OF (
SUB_REGION_VERIFICATION,
CertData
) +
CertDataBytes);
(2) Are we sure a UINT16 length field can cover all CertData fields
encountered in practice? Will there ever be a CertData that's larger
than ~64KB?
(3) But, my question actually relates to the size of the Blob field.
SIGNED_SUB_REGION expresses the base address of the region, but not the
size thereof. Is that intentional?
- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type UINTN, but (*SubRegionDataSize) has type UINT32. I think these types should be the same.I think the general idea behind the current prototype is fine -- take an
[ANS] Agree. Needs to change.
- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)
[ANS] Agree.
- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a pointer into the input buffer? Is it a copy, allocated dynamically?
[ANS] it is a pointer to original sub-region data (without signature header). If authentication is unsuccessful, it will be NULL. But, I agree AuthenticateSubRegion should just authenticate the sub-region. Parsing the data should be left to the caller.
input (base, size) pair, and adjust both components of that pair on
output. That's OK, in my opinion.
It's just that the caller needs to know that the output pointer points
into the same original blob (in other words, that on success,
(*SubRegionDataBuffer) *aliases* SignedSubRegionImage).
This is relevant to avoid memory management errors (double freeing of
buffers, or memory leaks). Every function ever that outputs a pointer,
needs to explain in a comment to the caller, who is responsible for
freeing the pointed-to object.
Thanks
Laszlo
Sean
Thanks for the info.
I would like to break my continued comments into two distinct parts.
1. Still interested in why we need new abstractions for the data blob. From your use case i would suggest leveraging something like a signed section. if that is used then the PI spec already has most of the abstractions defined, edk2 has tool infrastructure already, and edk2 core services already have a "plugin" model defined. I would be curious to understand why building new would be better.
2. On the key management/authentication management. When you say a "3rd party needs to be able to add their own public keys to bios". Is that something the IHV works with bios author or is that IHV works with user and user adds the key? Just trying to understand if this is a B2B model or business to end user model. Secondly, i think there are some leanings and challenges from PK/KEK/DB/DBX model that should be incorporated in any new design. I am also curious if and how this impacts measured boot/TPM pcr values, etc.
Thanks
Sean
toggle quoted message
Show quoted text
I would like to break my continued comments into two distinct parts.
1. Still interested in why we need new abstractions for the data blob. From your use case i would suggest leveraging something like a signed section. if that is used then the PI spec already has most of the abstractions defined, edk2 has tool infrastructure already, and edk2 core services already have a "plugin" model defined. I would be curious to understand why building new would be better.
2. On the key management/authentication management. When you say a "3rd party needs to be able to add their own public keys to bios". Is that something the IHV works with bios author or is that IHV works with user and user adds the key? Just trying to understand if this is a B2B model or business to end user model. Secondly, i think there are some leanings and challenges from PK/KEK/DB/DBX model that should be incorporated in any new design. I am also curious if and how this impacts measured boot/TPM pcr values, etc.
Thanks
Sean
On 7/2/2020 11:17 AM, Mackay, Curtis A wrote:
Hi Sean,
The binary to be signed is a blob of data to be consumed by BIOS. In the example mentioned the signed blob is packaged in an FV, but the solution should be flexible to work with other firmware management blocks.
The target use case is that the blob may be generated and signed by a 3rd party downstream of the ODM/OEM. The 3rd party needs to be able to add their own public keys to BIOS to allow BIOS to verify their blob.
After some discussion with others CC'd, the design is trending to use a separate set of variables that are reserved for data, similar to UEFI secure boot's PK/KEK/DB/DBX. This will limit the threat of attack on these keys, as only the data blobs would be compromised in the case of a compromised key, instead of all UEFI secure boot being compromised.
Regards,
Curtis
-----Original Message-----
From: Sean Brogan <spbrogan@...>
Sent: Thursday, June 25, 2020 4:41 PM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC
Curtis,
Not sure I fully follow your proposal. Can you provide more on the use case? Is the "blob" a FV or is the signed_sub_region a raw section in a ffs file? or something else like binary at flash offset?
The PI spec has a filesystem that describes many options and the DxeCore has support for security validation / authentication state flags associated with FVs and FFS files.
I have also seen many designs that leverage section extraction and doing authentication thru that guided sections.
Can you provide more background as to why it is important to get this into edk2 as a "standard" and why it requires defining new structures and new library abstractions?
Finally a point on the policy. In many products (especially commercial
products) you don't see "UEFI secure boot (PK/KEK/DB/DBX)" leveraged for trust prior to EndOfDxe. Since UEFI secure boot is often user controlled this opens up your "platform" to compromise that can be impossible to recover from.
Thanks
Sean
On 6/16/2020 10:50 AM, Mackay, Curtis A wrote:Hi,
I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.
A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.
To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.
Please provide feedback and comments on the design.
Best regards,
Curtis Mackay
Mackay, Curtis A <curtis.a.mackay@...>
Hi Sean,
The binary to be signed is a blob of data to be consumed by BIOS. In the example mentioned the signed blob is packaged in an FV, but the solution should be flexible to work with other firmware management blocks.
The target use case is that the blob may be generated and signed by a 3rd party downstream of the ODM/OEM. The 3rd party needs to be able to add their own public keys to BIOS to allow BIOS to verify their blob.
After some discussion with others CC'd, the design is trending to use a separate set of variables that are reserved for data, similar to UEFI secure boot's PK/KEK/DB/DBX. This will limit the threat of attack on these keys, as only the data blobs would be compromised in the case of a compromised key, instead of all UEFI secure boot being compromised.
Regards,
Curtis
toggle quoted message
Show quoted text
The binary to be signed is a blob of data to be consumed by BIOS. In the example mentioned the signed blob is packaged in an FV, but the solution should be flexible to work with other firmware management blocks.
The target use case is that the blob may be generated and signed by a 3rd party downstream of the ODM/OEM. The 3rd party needs to be able to add their own public keys to BIOS to allow BIOS to verify their blob.
After some discussion with others CC'd, the design is trending to use a separate set of variables that are reserved for data, similar to UEFI secure boot's PK/KEK/DB/DBX. This will limit the threat of attack on these keys, as only the data blobs would be compromised in the case of a compromised key, instead of all UEFI secure boot being compromised.
Regards,
Curtis
-----Original Message-----
From: Sean Brogan <spbrogan@...>
Sent: Thursday, June 25, 2020 4:41 PM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC
Curtis,
Not sure I fully follow your proposal. Can you provide more on the use case? Is the "blob" a FV or is the signed_sub_region a raw section in a ffs file? or something else like binary at flash offset?
The PI spec has a filesystem that describes many options and the DxeCore has support for security validation / authentication state flags associated with FVs and FFS files.
I have also seen many designs that leverage section extraction and doing authentication thru that guided sections.
Can you provide more background as to why it is important to get this into edk2 as a "standard" and why it requires defining new structures and new library abstractions?
Finally a point on the policy. In many products (especially commercial
products) you don't see "UEFI secure boot (PK/KEK/DB/DBX)" leveraged for trust prior to EndOfDxe. Since UEFI secure boot is often user controlled this opens up your "platform" to compromise that can be impossible to recover from.
Thanks
Sean
On 6/16/2020 10:50 AM, Mackay, Curtis A wrote:
From: Sean Brogan <spbrogan@...>
Sent: Thursday, June 25, 2020 4:41 PM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC
Curtis,
Not sure I fully follow your proposal. Can you provide more on the use case? Is the "blob" a FV or is the signed_sub_region a raw section in a ffs file? or something else like binary at flash offset?
The PI spec has a filesystem that describes many options and the DxeCore has support for security validation / authentication state flags associated with FVs and FFS files.
I have also seen many designs that leverage section extraction and doing authentication thru that guided sections.
Can you provide more background as to why it is important to get this into edk2 as a "standard" and why it requires defining new structures and new library abstractions?
Finally a point on the policy. In many products (especially commercial
products) you don't see "UEFI secure boot (PK/KEK/DB/DBX)" leveraged for trust prior to EndOfDxe. Since UEFI secure boot is often user controlled this opens up your "platform" to compromise that can be impossible to recover from.
Thanks
Sean
On 6/16/2020 10:50 AM, Mackay, Curtis A wrote:
Hi,
I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.
A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.
To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.
Please provide feedback and comments on the design.
Best regards,
Curtis Mackay