Extract edk2/cacerts.bin (RHBZ#1559580)

[lersek]

Please refer to https://bugzilla.redhat.com/show_bug.cgi?id=1559580 for the feature description.

I have successfully tested this against firmware code I've written (and in part pushed) for https://bugzilla.redhat.com/show_bug.cgi?id=1536624 . Testing instructions are attached to that BZ: https://bugzilla.redhat.com/attachment.cgi?id=1413960

The intended invocation by update-ca-trust is spelled out in the commit message of the second patch.

Thanks!
Laszlo

[coveralls]

Coverage decreased (-0.2%) to 80.238% when pulling a60e643 on lersek:extract-edk2-rhbz-1559580 into e8d5690 on p11-glue:master.

[gnutlsmirror]

Thank you! I'll make some comments inline. Would you like to add a test which extracts a fixed set of certs and compares the result with a set file? That would prevent future regressions in that code.

[gnutlsmirror]

To me the code looks fine. I've made some questions inline.

[gnutlsmirror]

I that pragma supported by gcc or clang? Isn't __attribute__((packed)) a better alternative for the compilers we use?

[gnutlsmirror]

Where is that GUID being use? Does it need to be unique per extraction?

[lersek]

I'm not really used to github pull requests (I work primarily on mailing lists), so please forgive if I don't get the format right etc... I'm unsure how I can respond to comments directly, so I'll do some manual quoting here.

(1) Who is "gnutlsmirror"? May I know the person behind the persona? :)

(2) Would you like to add a test which extracts a fixed set of certs and compares the result with a set file?

Looking at trust/test-extract.in, I think I could extend the same logic to this extractor. That is, first check that all three certificates are present in the binary output file (grep works on binary files as well), and then check that the blacklisted cert is missing from the binary output file. I think assert_contains and assert_not_contains can be reused as they are.

Would this work for you?

(3) Regarding #pragma pack:

Before I added that, I grepped the tree for __attribute__((packed)), #pragma pack, or any home-grown macros that might wrap these. To my surprise, while the tree lacks any use of the former, it does use #pragma pack already (see common/pkcs11.h).

So, I'm fine either way, but #pragma pack seems to fit both edk2 and p11-kit better.

Also, gcc has supported #pragma pack essentially forever; in RHEL-5 (that's not a typo) we ship gcc-4.1.2, and the documentation for that upstream gcc release lists the pragma:

https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Structure_002dPacking-Pragmas.html

(4) The "agent GUID" is used to populate the EFI_SIGNATURE_DATA.SignatureOwner field. The UEFI spec documents this field as follows: An identifier which identifies the agent which added the signature to the list.

Given that this extractor is the "same agent" for each of the extracted certificates, the same value should be used. (Obviously I tested this against the firmware and it works fine.)

Uniqueness is not required; for example Microsoft uses the same 77FA9ABD-0359-4D32-BD60-28F4E78F784B SignatureOwner GUID for at least three of their certificates:

• Microsoft Corporation KEK CA 2011
• Microsoft Windows Production PCA 2011
• Microsoft Corporation UEFI CA 2011

(For completeness I shoudl mention that those CA certificates are not for web server authentication but for Secure Boot driver authentication.)

[ueno]

Thank you for the patch! I have a couple of comments:

• for (2), you could look at trust/test-{bundle,cer,openssl}.c rather than test-extract, which is a program only run at make installcheck.
• for (3), although it improves code readability, I would prefer not to use packed structure at all for portability reasons. Instead, maybe you could write encoder functions?

[lersek]

OK, I'll check out the C language testers, thanks for the reference.

Regarding the avoidance of packed structures for better portability, I disagree. Packed structures are universally supported. They are used by all major projects that I'm involved with to various extents (edk2, QEMU, libvirt, kernel). Those projects build with multiple toolchains and for multiple OSes and architectures.

For example, edk2 (which uses #pragma pack) builds with every release of Visual Studio since VS2003, every release of GCC since 4.4, CLANG 3.5 and 3.8, GCC 4.3 on Cygwin, Intel C 9.1 and 11.1, ARM C 5.0, and XCODE 5.

Replacing the packed structures with stand-alone field writes doesn't just give up on "improvements" to code readability; it turns the code into an unreadable mess, and loses any resemblance of the UEFI spec (which is what defines the format).

A good example for such bad practice is trust/extract-jks.c, which:

• references a file by pathname in the "java sources" (whatever those might be) for the java keystore format, and
• writes naked fields with natural language comments such as /* The creation date: current time */.

This leaves the reader completely in the dark about the actual structure of the output file.

[lersek]

@ueno what I can do -- if you really dislike structure packing -- is:

• keep the structures but drop pragma #pack
• keep the code mostly unchanged (i.e., populate the structures in memory)
• when serializing the structures to the P11 buffer, serialize them field-wise, rather than whole-sale. After all, p11_buffer_add() works on individual fields too, not just on the full struct level.

This will take care of any unexpected padding between fields and at the end of the structures, and still keep the code readable. Is this acceptable?

[lersek]

BTW I think such custom encoders would be greatly helped if common/buffer.h offered convenience functions such as:

• p11_buffer_add_u8()
• p11_buffer_add_u16()
• p11_buffer_add_u32()

Can I add them?

[ueno]

I don't strongly oppose to using packed structures, but my concern is that we sometimes receive bug reports on AIX or IRIX (#101) and I don't know if that pragma is available on such platforms.

Anyway, given that those structures are not too complicated, I was thinking something like:

static bool buffer_add_uint16 (p11_buffer *buffer, uint16_t value);
static bool buffer_add_uint32 (p11_buffer *buffer, uint32_t value);
static bool buffer_add_efi_guid (p11_buffer *buffer, efi_guid *value);
static bool buffer_add_efi_signature_list (p11_buffer *buffer, efi_signature_list *value);

and:

static bool
buffer_add_efi_guid (p11_buffer *buffer, efi_guid *value)
{
    if (!buffer_add_uint32 (buffer, value->data1)
      return false;
    ...
}

There are similar code in p11-kit/rpc-message.c.

[lersek]

OK, thanks.

[nmav]

@gnutlsmirror was me, sorry for the confusion. I was logged with the wrong account.

[lersek]

v2 coming up