internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Laszlo Ersek]

Hi Jiaxin, Siyuan,

setting *multiple* CA certificates for HTTPS server verification looks
possible, from the following call tree:

TlsConfigCertificate()      [NetworkPkg/HttpDxe/HttpsSupport.c]
  TlsConfigurationSetData() [NetworkPkg/TlsDxe/TlsConfigProtocol.c]
    TlsSetCaCertificate()   [CryptoPkg/Library/TlsLib/TlsConfig.c]
      X509_STORE_add_cert()

because the outermost TlsConfigCertificate() function implements a loop
over the EFI_TLS_CA_CERTIFICATE_VARIABLE contents.

Is there natural-language documentation available about the internal
structure of EFI_TLS_CA_CERTIFICATE_VARIABLE?

Because, OVMF should avoid taking one format of CA Cert list from QEMU
(i.e. from the virtualization host) and converting it to the format
expected by TlsConfigCertificate(). Instead, the "update-ca-trust"
command should be taught (on the host system) to generate a binary
certificate list file (somewhere under "/etc/pki/ca-trust/extracted", I
believe) such that the file can be used directly for setting
EFI_TLS_CA_CERTIFICATE_VARIABLE in the guest.

In order to write such an extractor for "update-ca-trust", the format of
EFI_TLS_CA_CERTIFICATE_VARIABLE should be publicly documented. Also, a
promise of stability wouldn't hurt. :)

(To refer back to the cipher suite list discussion
<https://lists.01.org/pipermail/edk2-devel/2018-February/020944.html>,
this stability / public documentation goal was guaranteed there, due to
EFI_TLS_CIPHER being specified publicly.)

Thanks!
Laszlo

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Fu, Siyuan]

Hi, Laszlo

The data structure of EFI_TLS_CA_CERTIFICATE_VARIABLE is EFI_SIGNATURE_LIST and we have documented this in HTTPs Boot wiki page: https://github.com/tianocore/tianocore.github.io/wiki/HTTPS-Boot

You can refer section 31.4.1 "Signature Database" in UEFI 2.7 A for a detail description of EFI_SIGNATURE_LIST structure.


BestRegards
Fu Siyuan


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, March 20, 2018 10:56 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Daniel P. Berrange
> <berrange@redhat.com>
> Subject: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE
> 
> Hi Jiaxin, Siyuan,
> 
> setting *multiple* CA certificates for HTTPS server verification looks
> possible, from the following call tree:
> 
> TlsConfigCertificate()      [NetworkPkg/HttpDxe/HttpsSupport.c]
>   TlsConfigurationSetData() [NetworkPkg/TlsDxe/TlsConfigProtocol.c]
>     TlsSetCaCertificate()   [CryptoPkg/Library/TlsLib/TlsConfig.c]
>       X509_STORE_add_cert()
> 
> because the outermost TlsConfigCertificate() function implements a loop
> over the EFI_TLS_CA_CERTIFICATE_VARIABLE contents.
> 
> Is there natural-language documentation available about the internal
> structure of EFI_TLS_CA_CERTIFICATE_VARIABLE?
> 
> Because, OVMF should avoid taking one format of CA Cert list from QEMU
> (i.e. from the virtualization host) and converting it to the format
> expected by TlsConfigCertificate(). Instead, the "update-ca-trust"
> command should be taught (on the host system) to generate a binary
> certificate list file (somewhere under "/etc/pki/ca-trust/extracted", I
> believe) such that the file can be used directly for setting
> EFI_TLS_CA_CERTIFICATE_VARIABLE in the guest.
> 
> In order to write such an extractor for "update-ca-trust", the format of
> EFI_TLS_CA_CERTIFICATE_VARIABLE should be publicly documented. Also, a
> promise of stability wouldn't hurt. :)
> 
> (To refer back to the cipher suite list discussion
> <https://lists.01.org/pipermail/edk2-devel/2018-February/020944.html>,
> this stability / public documentation goal was guaranteed there, due to
> EFI_TLS_CIPHER being specified publicly.)
> 
> Thanks!
> Laszlo

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Laszlo Ersek]

On 03/21/18 02:30, Fu, Siyuan wrote:
> The data structure of EFI_TLS_CA_CERTIFICATE_VARIABLE is
> EFI_SIGNATURE_LIST and we have documented this in HTTPs Boot wiki
> page:
> https://github.com/tianocore/tianocore.github.io/wiki/HTTPS-Boot
> 
> You can refer section 31.4.1 "Signature Database" in UEFI 2.7 A for a
> detail description of EFI_SIGNATURE_LIST structure.

Thanks! I have two more questions.

The nested loops that parse the signature list in TlsConfigCertificate()
currently ignore the contents of the following fields:
- EFI_SIGNATURE_LIST.SignatureType,
- EFI_SIGNATURE_DATA.SignatureOwner.

I'd like the generator / extractor tool to populate these fields
correctly right from the start, so that it remain compatible with future
features added to edk2.


So, I suggest that the tool set "EFI_SIGNATURE_LIST.SignatureType" to
EFI_CERT_X509_GUID ("This identifies a signature based on a DER-encoded
X.509 certificate"). Because, this is what the current edk2 code assumes
anyway -- in TlsSetCaCertificate(), we have a comment saying

  //
  // DER-encoded binary X.509 certificate or PEM-encoded X.509
  // certificate. Determine whether certificate is from DER encoding, if
  // so, translate it to X509 structure.
  //

(1) Do you agree EFI_CERT_X509_GUID is the right setting for
"EFI_SIGNATURE_LIST.SignatureType" (even though the edk2 code currently
ignores it)?

This would also imply that we set
"EFI_SIGNATURE_LIST.SignatureHeaderSize" to zero, according to the UEFI
spec.


Furthermore, what would you suggest for
"EFI_SIGNATURE_DATA.SignatureOwner"? According to the spec, it is "An
identifier which identifies the agent which added the signature to the
list", so in theory we could just generate any GUID for the tool with
"uuidgen". However, based on past experience, this may not be good
enough; for example, the Secure Boot Logo Test in the Microsoft HCK
expect the SignatureOwner field (on the Microsoft certificates) to be
constant 77FA9ABD-0359-4D32-BD60-28F4E78F784B. In other words, Microsoft
want the SignatureOwner field to stand for the organization that issued
the certificates (i.e., themselves), not for the agent that enrolled the
certificates.

(2) Do you foresee any such restrictions for the
"EFI_SIGNATURE_DATA.SignatureOwner" field in
EFI_TLS_CA_CERTIFICATE_VARIABLE? Or is it safe if we generate a GUID for
the tool with "uuidgen"?

Thanks!
Laszlo

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Wu, Jiaxin]

Hi Laszlo,

Insert my comments as below: 

> The nested loops that parse the signature list in TlsConfigCertificate()
> currently ignore the contents of the following fields:
> - EFI_SIGNATURE_LIST.SignatureType,
> - EFI_SIGNATURE_DATA.SignatureOwner.
> 
> I'd like the generator / extractor tool to populate these fields
> correctly right from the start, so that it remain compatible with future
> features added to edk2.
> 
> 
> So, I suggest that the tool set "EFI_SIGNATURE_LIST.SignatureType" to
> EFI_CERT_X509_GUID ("This identifies a signature based on a DER-encoded
> X.509 certificate"). Because, this is what the current edk2 code assumes
> anyway -- in TlsSetCaCertificate(), we have a comment saying
> 
>   //
>   // DER-encoded binary X.509 certificate or PEM-encoded X.509
>   // certificate. Determine whether certificate is from DER encoding, if
>   // so, translate it to X509 structure.
>   //
> 
> (1) Do you agree EFI_CERT_X509_GUID is the right setting for
> "EFI_SIGNATURE_LIST.SignatureType" (even though the edk2 code currently
> ignores it)?
> 
> This would also imply that we set
> "EFI_SIGNATURE_LIST.SignatureHeaderSize" to zero, according to the UEFI
> spec.
> 

Yes, exactly, EFI_CERT_X509_GUID is the correct SignatureType for the CACertificate. SignatureHeaderSize should be set to zero. We do miss the check in HttpDxe driver, I'm fine to add back the  SignatureType check in TlsConfigCertificate(). So, can you report the Bugzilla for this fixing? Thanks.


> 
> Furthermore, what would you suggest for
> "EFI_SIGNATURE_DATA.SignatureOwner"? According to the spec, it is "An
> identifier which identifies the agent which added the signature to the
> list", so in theory we could just generate any GUID for the tool with
> "uuidgen". However, based on past experience, this may not be good
> enough; for example, the Secure Boot Logo Test in the Microsoft HCK
> expect the SignatureOwner field (on the Microsoft certificates) to be
> constant 77FA9ABD-0359-4D32-BD60-28F4E78F784B. In other words,
> Microsoft
> want the SignatureOwner field to stand for the organization that issued
> the certificates (i.e., themselves), not for the agent that enrolled the
> certificates.
> 
> (2) Do you foresee any such restrictions for the
> "EFI_SIGNATURE_DATA.SignatureOwner" field in
> EFI_TLS_CA_CERTIFICATE_VARIABLE? Or is it safe if we generate a GUID for
> the tool with "uuidgen"?
> 

I don't think it's necessary to restrict/stand the GUID in the field of SignatureOwner for the CA certification (at least for now) since it's only used to identify the different venders (i.e, Microsoft) so as to avoid the following content check. In the UEFI part, we also didn't check the SignatureOwner for the any security consideration. So, I think it's safe to generate a GUID using the tool at present.

Thanks,
Jiaxin

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Laszlo Ersek]

On 03/22/18 03:02, Wu, Jiaxin wrote:
> On 03/21/18 14:39, Laszlo Ersek wrote:
>> (1) Do you agree EFI_CERT_X509_GUID is the right setting for
>> "EFI_SIGNATURE_LIST.SignatureType" (even though the edk2 code
>> currently ignores it)?
>>
>> This would also imply that we set
>> "EFI_SIGNATURE_LIST.SignatureHeaderSize" to zero, according to the
>> UEFI spec.
>>
> 
> Yes, exactly, EFI_CERT_X509_GUID is the correct SignatureType for the
> CACertificate. SignatureHeaderSize should be set to zero. We do miss
> the check in HttpDxe driver, I'm fine to add back the  SignatureType
> check in TlsConfigCertificate(). So, can you report the Bugzilla for
> this fixing? Thanks.

I've filed <https://bugzilla.tianocore.org/show_bug.cgi?id=909> and
assigned it to myself (for a v1 patch at least).

>> (2) Do you foresee any such restrictions for the
>> "EFI_SIGNATURE_DATA.SignatureOwner" field in
>> EFI_TLS_CA_CERTIFICATE_VARIABLE? Or is it safe if we generate a GUID
>> for the tool with "uuidgen"?
>>
> 
> I don't think it's necessary to restrict/stand the GUID in the field
> of SignatureOwner for the CA certification (at least for now) since
> it's only used to identify the different venders (i.e, Microsoft) so
> as to avoid the following content check. In the UEFI part, we also
> didn't check the SignatureOwner for the any security consideration.
> So, I think it's safe to generate a GUID using the tool at present.

Sounds great, thanks!

Laszlo

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Laszlo Ersek]

Hi,

On 03/21/18 02:30, Fu, Siyuan wrote:
> Hi, Laszlo
>
> The data structure of EFI_TLS_CA_CERTIFICATE_VARIABLE is
> EFI_SIGNATURE_LIST and we have documented this in HTTPs Boot wiki
> page:
> https://github.com/tianocore/tianocore.github.io/wiki/HTTPS-Boot
>
> You can refer section 31.4.1 "Signature Database" in UEFI 2.7 A for a
> detail description of EFI_SIGNATURE_LIST structure.

I implemented a "p11-kit" extractor so that it produces a sequence of
EFI_SIGNATURE_LIST objects, each siglist containing a single
EFI_SIGNATURE_DATA object (with an X509 CA certificate in it).

In order to test that, I started writing the OVMF enablement patches, on
top of the following series (pending review):

  [edk2] [PATCH 0/5] NetworkPkg: HTTP and TLS updates

In the OVMF platform code, I created EFI_TLS_CA_CERTIFICATE_VARIABLE as
a *volatile* and boot-service only variable, so that there would be no
flash impact, regardless of the number and size of the certificates that
the extractor produced.

Unfortunately, I still ran into a limitation here. The
EFI_SIGNATURE_LIST bundle produced by the extractor is 182 KB in size
(it comes from the Mozilla CA root certificate bundle, aka the
"ca-certificates" package). In order to create such a large variable, I
had to raise the following two PCDs for OVMF:

- PcdVariableStoreSize: this controls the cumulative storage size for
  *volatile* variables.

- PcdMaxVariableSize: this limits the individual variable size for
  non-authenticated variables, *regardless* of their volatility.

I set PcdMaxVariableSize to 256KB (to accommodate the 182KB size
mentioned above, and to leave a bit of margin). I set
PcdVariableStoreSize to 512KB (so that there would be quite a bit of
volatile space left after a 256KB volatile variable was created).

And this is where the variable driver runs into an assertion failure:

> ASSERT MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c(3809):
> GetNonVolatileMaxVariableSize () < (VariableStoreLength - sizeof
> (VARIABLE_STORE_HEADER))

The GetNonVolatileMaxVariableSize() function calculates the maximum of
PcdMaxVariableSize and PcdMaxAuthVariableSize. This is not good because
I didn't mean to raise PcdMaxVariableSize for non-volatile purposes; I
only meant to raise it for volatile storage.

Then the InitNonVolatileVariableStore() function attempts to fit this
maximum into the *flash* storage. Obviously, individual 256KB variables
(which I never meant to place into flash) don't fit into flash, and the
ASSERT() fires.

The issue is that the variable driver does not distinguish the max
variable size between volatile and non-volatile storage, and the
non-volatile availability effectively limits the volatile one.

I think we should have three PCDs:

- PcdMaxVariableSize: non-authenticated, non-volatile
- PcdMaxAuthVariableSize: authenticated, non-volatile
- PcdMaxVolatileVariableSize: non-authenticated, volatile

(The fourth variation needs no PCD because authenticated volatile
variables make no sense.)

How can we solve this?

Thanks,
Laszlo

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Zeng, Star]

Is there a PCD pointers to the siglist?


For adding PcdMaxVolatileVariableSize: non-authenticated, volatile, I think it is acceptable if there are use cases.


Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Wednesday, March 28, 2018 10:32 AM
To: Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Daniel P. Berrange <berrange@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

Hi,

On 03/21/18 02:30, Fu, Siyuan wrote:
> Hi, Laszlo
>
> The data structure of EFI_TLS_CA_CERTIFICATE_VARIABLE is 
> EFI_SIGNATURE_LIST and we have documented this in HTTPs Boot wiki
> page:
> https://github.com/tianocore/tianocore.github.io/wiki/HTTPS-Boot
>
> You can refer section 31.4.1 "Signature Database" in UEFI 2.7 A for a 
> detail description of EFI_SIGNATURE_LIST structure.

I implemented a "p11-kit" extractor so that it produces a sequence of EFI_SIGNATURE_LIST objects, each siglist containing a single EFI_SIGNATURE_DATA object (with an X509 CA certificate in it).

In order to test that, I started writing the OVMF enablement patches, on top of the following series (pending review):

  [edk2] [PATCH 0/5] NetworkPkg: HTTP and TLS updates

In the OVMF platform code, I created EFI_TLS_CA_CERTIFICATE_VARIABLE as a *volatile* and boot-service only variable, so that there would be no flash impact, regardless of the number and size of the certificates that the extractor produced.

Unfortunately, I still ran into a limitation here. The EFI_SIGNATURE_LIST bundle produced by the extractor is 182 KB in size (it comes from the Mozilla CA root certificate bundle, aka the "ca-certificates" package). In order to create such a large variable, I had to raise the following two PCDs for OVMF:

- PcdVariableStoreSize: this controls the cumulative storage size for
  *volatile* variables.

- PcdMaxVariableSize: this limits the individual variable size for
  non-authenticated variables, *regardless* of their volatility.

I set PcdMaxVariableSize to 256KB (to accommodate the 182KB size mentioned above, and to leave a bit of margin). I set PcdVariableStoreSize to 512KB (so that there would be quite a bit of volatile space left after a 256KB volatile variable was created).

And this is where the variable driver runs into an assertion failure:

> ASSERT MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c(3809):
> GetNonVolatileMaxVariableSize () < (VariableStoreLength - sizeof
> (VARIABLE_STORE_HEADER))

The GetNonVolatileMaxVariableSize() function calculates the maximum of PcdMaxVariableSize and PcdMaxAuthVariableSize. This is not good because I didn't mean to raise PcdMaxVariableSize for non-volatile purposes; I only meant to raise it for volatile storage.

Then the InitNonVolatileVariableStore() function attempts to fit this maximum into the *flash* storage. Obviously, individual 256KB variables (which I never meant to place into flash) don't fit into flash, and the
ASSERT() fires.

The issue is that the variable driver does not distinguish the max variable size between volatile and non-volatile storage, and the non-volatile availability effectively limits the volatile one.

I think we should have three PCDs:

- PcdMaxVariableSize: non-authenticated, non-volatile
- PcdMaxAuthVariableSize: authenticated, non-volatile
- PcdMaxVolatileVariableSize: non-authenticated, volatile

(The fourth variation needs no PCD because authenticated volatile variables make no sense.)

How can we solve this?

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Laszlo Ersek]

Hi Star,

thanks for following up; comments below:

On 03/28/18 05:28, Zeng, Star wrote:
> Is there a PCD pointers to the siglist?

We discussed that earlier, but because HttpDxe -- which consumes the
certificate list -- is a UEFI driver, we decided that it should not
consume dynamic PCDs. The alternative (specified in the UEFI spec) was
variables.

The earlier discussion wasn't exactly about the trusted CA cert list.
Instead, it was about the trusted cipher algo list. However, both of
these knobs pose the same "info channel" questions. So here's the link
into the cipher algo list discussion:

http://mid.mail-archive.com/895558F6EA4E3B41AC93A00D163B72741637DE9E@SHSMSX103.ccr.corp.intel.com

> For adding PcdMaxVolatileVariableSize: non-authenticated, volatile, I think it is acceptable if there are use cases.

Thank you for accepting the idea in theory :)

Do you think it is a simple change? Or is it intrusive?

If it is intrusive, then I'd prefer if one of the variable driver
maintainers wrote the patch. It's a complex driver and there can be
hidden assumptions and relationships that I might miss.

If it's a reasonably simple change then I'm happy to work on it.

Thanks!
Laszlo

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Zeng, Star]

Laszlo,

There should be some places need to care about. I also need to check the detail.

I am ok to make the patch if it is not so urgent.


Thanks,
Star
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, March 28, 2018 6:07 PM
To: Zeng, Star <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Daniel P. Berrange <berrange@redhat.com>
Subject: Re: [edk2] internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

Hi Star,

thanks for following up; comments below:

On 03/28/18 05:28, Zeng, Star wrote:
> Is there a PCD pointers to the siglist?

We discussed that earlier, but because HttpDxe -- which consumes the certificate list -- is a UEFI driver, we decided that it should not consume dynamic PCDs. The alternative (specified in the UEFI spec) was variables.

The earlier discussion wasn't exactly about the trusted CA cert list.
Instead, it was about the trusted cipher algo list. However, both of these knobs pose the same "info channel" questions. So here's the link into the cipher algo list discussion:

http://mid.mail-archive.com/895558F6EA4E3B41AC93A00D163B72741637DE9E@SHSMSX103.ccr.corp.intel.com

> For adding PcdMaxVolatileVariableSize: non-authenticated, volatile, I think it is acceptable if there are use cases.

Thank you for accepting the idea in theory :)

Do you think it is a simple change? Or is it intrusive?

If it is intrusive, then I'd prefer if one of the variable driver maintainers wrote the patch. It's a complex driver and there can be hidden assumptions and relationships that I might miss.

If it's a reasonably simple change then I'm happy to work on it.

Thanks!
Laszlo

Re: internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE

[Laszlo Ersek]

On 03/28/18 13:10, Zeng, Star wrote:
> Laszlo,
> 
> There should be some places need to care about. I also need to check the detail.
> 
> I am ok to make the patch if it is not so urgent.

Thank you, Star!

I'll keep it on my TODO list as well, and I might post something if I
get there first.

Thanks!
Laszlo


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Wednesday, March 28, 2018 6:07 PM
> To: Zeng, Star <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Daniel P. Berrange <berrange@redhat.com>
> Subject: Re: [edk2] internal structure of EFI_TLS_CA_CERTIFICATE_VARIABLE
> 
> Hi Star,
> 
> thanks for following up; comments below:
> 
> On 03/28/18 05:28, Zeng, Star wrote:
>> Is there a PCD pointers to the siglist?
> 
> We discussed that earlier, but because HttpDxe -- which consumes the certificate list -- is a UEFI driver, we decided that it should not consume dynamic PCDs. The alternative (specified in the UEFI spec) was variables.
> 
> The earlier discussion wasn't exactly about the trusted CA cert list.
> Instead, it was about the trusted cipher algo list. However, both of these knobs pose the same "info channel" questions. So here's the link into the cipher algo list discussion:
> 
> http://mid.mail-archive.com/895558F6EA4E3B41AC93A00D163B72741637DE9E@SHSMSX103.ccr.corp.intel.com
> 
>> For adding PcdMaxVolatileVariableSize: non-authenticated, volatile, I think it is acceptable if there are use cases.
> 
> Thank you for accepting the idea in theory :)
> 
> Do you think it is a simple change? Or is it intrusive?
> 
> If it is intrusive, then I'd prefer if one of the variable driver maintainers wrote the patch. It's a complex driver and there can be hidden assumptions and relationships that I might miss.
> 
> If it's a reasonably simple change then I'm happy to work on it.
> 
> Thanks!
> Laszlo
>