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

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

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: http_and_tls_updates

Patch #4 fixes TianoCore BZ#909
<https://bugzilla.tianocore.org/show_bug.cgi?id=909>.

Patches #2 and #3 are cleanups / preparation for patch #4.

Patch #1 fixes an independent typo that I noticed in the code while
configuring my DHCP server for HTTP(S) booting. It's isolated, so I put
it first in the series.

Patch #5 is preparation for future platform enablement, so that a
platform can create both "TlsCaCertificate" and "HttpTlsCipherList"
variables on every boot from scratch as volatile variables (without
flash varstore footprint).

I regression-tested this series with a successful HTTPS boot of an ISO
image from OVMF, using a DER-formatted self-signed certificate that I
enrolled with TlsAuthConfigDxe.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>

Thanks,
Laszlo

Laszlo Ersek (5):
  NetworkPkg/HttpBootDxe: fix typo in DHCPv4 packet parsing
  NetworkPkg/HttpDxe: use error handler epilogue in
    TlsConfigCertificate()
  NetworkPkg/HttpDxe: drop misleading comment / status code in cert
    config
  NetworkPkg/HttpDxe: sanity-check the TlsCaCertificate variable before
    use
  NetworkPkg/TlsAuthConfigDxe: preserve TlsCaCertificate variable
    attributes

 NetworkPkg/HttpBootDxe/HttpBootDhcp4.c          |  4 +-
 NetworkPkg/HttpDxe/HttpDxe.inf                  |  3 +-
 NetworkPkg/HttpDxe/HttpsSupport.c               | 74 ++++++++++++++++++--
 NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 15 ++--
 4 files changed, 80 insertions(+), 16 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 1/5] NetworkPkg/HttpBootDxe: fix typo in DHCPv4 packet parsing

[Laszlo Ersek]

The string "HTTPClient" has 10 non-NUL characters; the value 9 is a
copy-paste leftover from "PXEClient". Check for all 10 characters in the
vendor-class-identifier option when determining whether the DHCP offer is
an HTTP offer.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/HttpBootDxe/HttpBootDhcp4.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c b/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c
index 421ce6eda40e..229e6cb0ec6a 100644
--- a/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c
+++ b/NetworkPkg/HttpBootDxe/HttpBootDhcp4.c
@@ -332,8 +332,8 @@ HttpBootParseDhcp4Packet (
   // The offer with "HTTPClient" is a Http offer.
   //
   Option = Options[HTTP_BOOT_DHCP4_TAG_INDEX_CLASS_ID];
-  if ((Option != NULL) && (Option->Length >= 9) &&
-      (CompareMem (Option->Data, DEFAULT_CLASS_ID_DATA, 9) == 0)) {
+  if ((Option != NULL) && (Option->Length >= 10) &&
+      (CompareMem (Option->Data, DEFAULT_CLASS_ID_DATA, 10) == 0)) {
     IsHttpOffer = TRUE;
   }
 
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 2/5] NetworkPkg/HttpDxe: use error handler epilogue in TlsConfigCertificate()

[Laszlo Ersek]

Introduce a FreeCACert label near the end of the function, so that we can
keep the FreePool(CACert) statement centralized for error and success
exits.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/HttpDxe/HttpsSupport.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c
index 5105a2014c25..9103987a0e4c 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -425,9 +425,8 @@ TlsConfigCertificate (
     // GetVariable still error or the variable is corrupted.
     // Fall back to the default value.
     //
-    FreePool (CACert);
-
-    return EFI_NOT_FOUND;
+    Status = EFI_NOT_FOUND;
+    goto FreeCACert;
   }
 
   ASSERT (CACert != NULL);
@@ -451,8 +450,7 @@ TlsConfigCertificate (
                                                  CertList->SignatureSize - sizeof (Cert->SignatureOwner)
                                                  );
       if (EFI_ERROR (Status)) {
-        FreePool (CACert);
-        return Status;
+        goto FreeCACert;
       }
 
       Cert = (EFI_SIGNATURE_DATA *) ((UINT8 *) Cert + CertList->SignatureSize);
@@ -462,6 +460,7 @@ TlsConfigCertificate (
     CertList = (EFI_SIGNATURE_LIST *) ((UINT8 *) CertList + CertList->SignatureListSize);
   }
 
+FreeCACert:
   FreePool (CACert);
   return Status;
 }
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 3/5] NetworkPkg/HttpDxe: drop misleading comment / status code in cert config

[Laszlo Ersek]

For TlsConfigureSession(), it makes sense to exempt EFI_NOT_FOUND from
TlsConfigCipherList() / gRT->GetVariable(), because there is a default
cipher list (SSL_DEFAULT_CIPHER_LIST) we can fall back to.

The same is not true of TlsConfigCertificate(), because there is no
default CA cert list. The platform (or the user of the Setup utility) is
required to configure a CA cert list first.

Remove the misleading comment and status code mapping in
TlsConfigCertificate().

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/HttpDxe/HttpsSupport.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c
index 9103987a0e4c..baab77225fdf 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -423,9 +423,7 @@ TlsConfigCertificate (
   if (EFI_ERROR (Status)) {
     //
     // GetVariable still error or the variable is corrupted.
-    // Fall back to the default value.
     //
-    Status = EFI_NOT_FOUND;
     goto FreeCACert;
   }
 
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 4/5] NetworkPkg/HttpDxe: sanity-check the TlsCaCertificate variable before use

[Laszlo Ersek]

In TlsConfigCertificate(), make sure that the set of EFI_SIGNATURE_LIST
objects that the platform stored to "TlsCaCertificate" is well-formed.

In addition, because HttpInstance->TlsConfiguration->SetData() expects
X509 certificates only, ensure that the EFI_SIGNATURE_LIST objects only
report X509 certificates, as described under EFI_CERT_X509_GUID in the
UEFI-2.7 spec.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=909
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/HttpDxe/HttpDxe.inf    |  3 +-
 NetworkPkg/HttpDxe/HttpsSupport.c | 65 ++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/NetworkPkg/HttpDxe/HttpDxe.inf b/NetworkPkg/HttpDxe/HttpDxe.inf
index 938e894d9f09..6c0688d1305b 100644
--- a/NetworkPkg/HttpDxe/HttpDxe.inf
+++ b/NetworkPkg/HttpDxe/HttpDxe.inf
@@ -75,9 +75,10 @@ [Protocols]
 [Guids]
   gEfiTlsCaCertificateGuid                         ## SOMETIMES_CONSUMES  ## Variable:L"TlsCaCertificate"
   gEdkiiHttpTlsCipherListGuid                      ## SOMETIMES_CONSUMES  ## Variable:L"HttpTlsCipherList"
+  gEfiCertX509Guid                                 ## SOMETIMES_CONSUMES  ## GUID  # Check the cert type
 
 [Pcd]
   gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections       ## CONSUMES  
 
 [UserExtensions.TianoCore."ExtraFiles"]
-  HttpDxeExtra.uni
\ No newline at end of file
+  HttpDxeExtra.uni
diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c
index baab77225fdf..d658512f6d9f 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -384,6 +384,7 @@ TlsConfigCertificate (
   UINT32              Index;
   EFI_SIGNATURE_LIST  *CertList;
   EFI_SIGNATURE_DATA  *Cert;
+  UINTN               CertArraySizeInBytes;
   UINTN               CertCount;
   UINT32              ItemDataSize;
 
@@ -429,6 +430,70 @@ TlsConfigCertificate (
 
   ASSERT (CACert != NULL);
 
+  //
+  // Sanity check
+  //
+  Status = EFI_INVALID_PARAMETER;
+  CertCount = 0;
+  ItemDataSize = (UINT32) CACertSize;
+  while (ItemDataSize > 0) {
+    if (ItemDataSize < sizeof (EFI_SIGNATURE_LIST)) {
+      DEBUG ((DEBUG_ERROR, "%a: truncated EFI_SIGNATURE_LIST header\n",
+        __FUNCTION__));
+      goto FreeCACert;
+    }
+
+    CertList = (EFI_SIGNATURE_LIST *) (CACert + (CACertSize - ItemDataSize));
+
+    if (CertList->SignatureListSize < sizeof (EFI_SIGNATURE_LIST)) {
+      DEBUG ((DEBUG_ERROR,
+        "%a: SignatureListSize too small for EFI_SIGNATURE_LIST\n",
+        __FUNCTION__));
+      goto FreeCACert;
+    }
+
+    if (CertList->SignatureListSize > ItemDataSize) {
+      DEBUG ((DEBUG_ERROR, "%a: truncated EFI_SIGNATURE_LIST body\n",
+        __FUNCTION__));
+      goto FreeCACert;
+    }
+
+    if (!CompareGuid (&CertList->SignatureType, &gEfiCertX509Guid)) {
+      DEBUG ((DEBUG_ERROR, "%a: only X509 certificates are supported\n",
+        __FUNCTION__));
+      Status = EFI_UNSUPPORTED;
+      goto FreeCACert;
+    }
+
+    if (CertList->SignatureHeaderSize != 0) {
+      DEBUG ((DEBUG_ERROR, "%a: SignatureHeaderSize must be 0 for X509\n",
+        __FUNCTION__));
+      goto FreeCACert;
+    }
+
+    if (CertList->SignatureSize < sizeof (EFI_SIGNATURE_DATA)) {
+      DEBUG ((DEBUG_ERROR,
+        "%a: SignatureSize too small for EFI_SIGNATURE_DATA\n", __FUNCTION__));
+      goto FreeCACert;
+    }
+
+    CertArraySizeInBytes = (CertList->SignatureListSize -
+                            sizeof (EFI_SIGNATURE_LIST));
+    if (CertArraySizeInBytes % CertList->SignatureSize != 0) {
+      DEBUG ((DEBUG_ERROR,
+        "%a: EFI_SIGNATURE_DATA array not a multiple of SignatureSize\n",
+        __FUNCTION__));
+      goto FreeCACert;
+    }
+
+    CertCount += CertArraySizeInBytes / CertList->SignatureSize;
+    ItemDataSize -= CertList->SignatureListSize;
+  }
+  if (CertCount == 0) {
+    DEBUG ((DEBUG_ERROR, "%a: no X509 certificates provided\n", __FUNCTION__));
+    goto FreeCACert;
+  }
+
   //
   // Enumerate all data and erasing the target item.
   //
-- 
2.14.1.3.gb7cf6e02401b

[PATCH 5/5] NetworkPkg/TlsAuthConfigDxe: preserve TlsCaCertificate variable attributes

[Laszlo Ersek]

If the platform creates the "TlsCaCertificate" variable as volatile, then
EnrollX509toVariable() shouldn't fail to extend it just because
TLS_AUTH_CONFIG_VAR_BASE_ATTR contains the EFI_VARIABLE_NON_VOLATILE
attribute.

Thus, if the variable exists, add the EFI_VARIABLE_APPEND_WRITE attribute
to the variable's current attributes. This is what DeleteCert() does
already.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
index faefc72d0efa..cbdd5f0664bd 100644
--- a/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
+++ b/NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c
@@ -813,6 +813,7 @@ EnrollX509toVariable (
   CACert        = NULL;
   CACertData    = NULL;
   Data          = NULL;
+  Attr          = 0;
 
   Status = ReadFileContent (
              Private->FileContext->FHandle,
@@ -847,22 +848,22 @@ EnrollX509toVariable (
   CopyMem ((UINT8* ) (CACertData->SignatureData), X509Data, X509DataSize);
 
   //
-  // Check if signature database entry has been already existed.
-  // If true, use EFI_VARIABLE_APPEND_WRITE attribute to append the
-  // new signature data to original variable
+  // Check if the signature database entry already exists. If it does, use the
+  // EFI_VARIABLE_APPEND_WRITE attribute to append the new signature data to
+  // the original variable, plus preserve the original variable attributes.
   //
-  Attr = TLS_AUTH_CONFIG_VAR_BASE_ATTR;
-
   Status = gRT->GetVariable(
                   VariableName,
                   &gEfiTlsCaCertificateGuid,
-                  NULL,
+                  &Attr,
                   &DataSize,
                   NULL
                   );
   if (Status == EFI_BUFFER_TOO_SMALL) {
     Attr |= EFI_VARIABLE_APPEND_WRITE;
-  } else if (Status != EFI_NOT_FOUND) {
+  } else if (Status == EFI_NOT_FOUND) {
+    Attr = TLS_AUTH_CONFIG_VAR_BASE_ATTR;
+  } else {
     goto ON_EXIT;
   }
 
-- 
2.14.1.3.gb7cf6e02401b

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

[Laszlo Ersek]

Siyuan, Jiaxin,

do you have any comments please?

Thanks,
Laszlo

On 03/22/18 17:39, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: http_and_tls_updates
> 
> Patch #4 fixes TianoCore BZ#909
> <https://bugzilla.tianocore.org/show_bug.cgi?id=909>.
> 
> Patches #2 and #3 are cleanups / preparation for patch #4.
> 
> Patch #1 fixes an independent typo that I noticed in the code while
> configuring my DHCP server for HTTP(S) booting. It's isolated, so I put
> it first in the series.
> 
> Patch #5 is preparation for future platform enablement, so that a
> platform can create both "TlsCaCertificate" and "HttpTlsCipherList"
> variables on every boot from scratch as volatile variables (without
> flash varstore footprint).
> 
> I regression-tested this series with a successful HTTPS boot of an ISO
> image from OVMF, using a DER-formatted self-signed certificate that I
> enrolled with TlsAuthConfigDxe.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (5):
>   NetworkPkg/HttpBootDxe: fix typo in DHCPv4 packet parsing
>   NetworkPkg/HttpDxe: use error handler epilogue in
>     TlsConfigCertificate()
>   NetworkPkg/HttpDxe: drop misleading comment / status code in cert
>     config
>   NetworkPkg/HttpDxe: sanity-check the TlsCaCertificate variable before
>     use
>   NetworkPkg/TlsAuthConfigDxe: preserve TlsCaCertificate variable
>     attributes
> 
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.c          |  4 +-
>  NetworkPkg/HttpDxe/HttpDxe.inf                  |  3 +-
>  NetworkPkg/HttpDxe/HttpsSupport.c               | 74 ++++++++++++++++++--
>  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 15 ++--
>  4 files changed, 80 insertions(+), 16 deletions(-)
> 

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

[Fu, Siyuan]

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, March 22, 2018 9:39 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH 0/5] NetworkPkg: HTTP and TLS updates
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: http_and_tls_updates
> 
> Patch #4 fixes TianoCore BZ#909
> <https://bugzilla.tianocore.org/show_bug.cgi?id=909>.
> 
> Patches #2 and #3 are cleanups / preparation for patch #4.
> 
> Patch #1 fixes an independent typo that I noticed in the code while
> configuring my DHCP server for HTTP(S) booting. It's isolated, so I put
> it first in the series.
> 
> Patch #5 is preparation for future platform enablement, so that a
> platform can create both "TlsCaCertificate" and "HttpTlsCipherList"
> variables on every boot from scratch as volatile variables (without
> flash varstore footprint).
> 
> I regression-tested this series with a successful HTTPS boot of an ISO
> image from OVMF, using a DER-formatted self-signed certificate that I
> enrolled with TlsAuthConfigDxe.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (5):
>   NetworkPkg/HttpBootDxe: fix typo in DHCPv4 packet parsing
>   NetworkPkg/HttpDxe: use error handler epilogue in
>     TlsConfigCertificate()
>   NetworkPkg/HttpDxe: drop misleading comment / status code in cert
>     config
>   NetworkPkg/HttpDxe: sanity-check the TlsCaCertificate variable before
>     use
>   NetworkPkg/TlsAuthConfigDxe: preserve TlsCaCertificate variable
>     attributes
> 
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.c          |  4 +-
>  NetworkPkg/HttpDxe/HttpDxe.inf                  |  3 +-
>  NetworkPkg/HttpDxe/HttpsSupport.c               | 74 ++++++++++++++++++--
>  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 15 ++--
>  4 files changed, 80 insertions(+), 16 deletions(-)
> 
> --
> 2.14.1.3.gb7cf6e02401b

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

[Wu, Jiaxin]

Series Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>

Thanks,
Jiaxin

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, March 23, 2018 12:39 AM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH 0/5] NetworkPkg: HTTP and TLS updates
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: http_and_tls_updates
> 
> Patch #4 fixes TianoCore BZ#909
> <https://bugzilla.tianocore.org/show_bug.cgi?id=909>.
> 
> Patches #2 and #3 are cleanups / preparation for patch #4.
> 
> Patch #1 fixes an independent typo that I noticed in the code while
> configuring my DHCP server for HTTP(S) booting. It's isolated, so I put
> it first in the series.
> 
> Patch #5 is preparation for future platform enablement, so that a
> platform can create both "TlsCaCertificate" and "HttpTlsCipherList"
> variables on every boot from scratch as volatile variables (without
> flash varstore footprint).
> 
> I regression-tested this series with a successful HTTPS boot of an ISO
> image from OVMF, using a DER-formatted self-signed certificate that I
> enrolled with TlsAuthConfigDxe.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (5):
>   NetworkPkg/HttpBootDxe: fix typo in DHCPv4 packet parsing
>   NetworkPkg/HttpDxe: use error handler epilogue in
>     TlsConfigCertificate()
>   NetworkPkg/HttpDxe: drop misleading comment / status code in cert
>     config
>   NetworkPkg/HttpDxe: sanity-check the TlsCaCertificate variable before
>     use
>   NetworkPkg/TlsAuthConfigDxe: preserve TlsCaCertificate variable
>     attributes
> 
>  NetworkPkg/HttpBootDxe/HttpBootDhcp4.c          |  4 +-
>  NetworkPkg/HttpDxe/HttpDxe.inf                  |  3 +-
>  NetworkPkg/HttpDxe/HttpsSupport.c               | 74 ++++++++++++++++++--
>  NetworkPkg/TlsAuthConfigDxe/TlsAuthConfigImpl.c | 15 ++--
>  4 files changed, 80 insertions(+), 16 deletions(-)
> 
> --
> 2.14.1.3.gb7cf6e02401b

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

[Laszlo Ersek]

On 03/28/18 07:35, Wu, Jiaxin wrote:
> Series Reviewed-by: Jiaxin Wu <jiaxin.wu@intel.com>

Thank you both for reviewing this series while being out-of-office!

Pushed: 699a2c30cb6e..b90c335fbbb6.

Thanks!
Laszlo