[PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Nickle Wang]

Provide an option for caller to disable TLS host verify in HttpDxe
driver. When web server uses self-signed certificate and caller has no
way to get root CA from web server, caller can use this option to
disable TLS host verify function. This option is similar to the "-k"
option in "curl" tool.

Signed-off-by: Nickle Wang <nicklew@nvidia.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Cc: Abner Chang <abner.chang@amd.com>
Cc: Igor Kulchytskyy <igork@ami.com>
Cc: Nick Ramirez <nramirez@nvidia.com>
---
 MdePkg/Include/Protocol/Http.h    |  5 +++
 NetworkPkg/HttpDxe/HttpProto.h    |  2 ++
 NetworkPkg/HttpDxe/HttpImpl.c     |  2 ++
 NetworkPkg/HttpDxe/HttpsSupport.c | 53 +++++++++++++++++--------------
 4 files changed, 38 insertions(+), 24 deletions(-)

diff --git a/MdePkg/Include/Protocol/Http.h b/MdePkg/Include/Protocol/Http.h
index 28e6221593..21a782eaac 100644
--- a/MdePkg/Include/Protocol/Http.h
+++ b/MdePkg/Include/Protocol/Http.h
@@ -6,6 +6,7 @@
 
   Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
   (C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP<BR>
+  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
   @par Revision Reference:
@@ -161,6 +162,10 @@ typedef struct {
   /// this instance will use EFI_DNS6_PROTOCOL and EFI_TCP6_PROTOCOL.
   ///
   BOOLEAN             LocalAddressIsIPv6;
+  ///
+  /// Verify server certificate during HTTPS handshake.
+  ///
+  BOOLEAN             HostCertificateVerifyDisabled;
 
   union {
     ///
diff --git a/NetworkPkg/HttpDxe/HttpProto.h b/NetworkPkg/HttpDxe/HttpProto.h
index 620eb39158..72d6b2b3b7 100644
--- a/NetworkPkg/HttpDxe/HttpProto.h
+++ b/NetworkPkg/HttpDxe/HttpProto.h
@@ -3,6 +3,7 @@
 
 Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
 (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
+Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -176,6 +177,7 @@ typedef struct _HTTP_PROTOCOL {
   EFI_TLS_PROTOCOL                  *Tls;
   EFI_TLS_CONFIGURATION_PROTOCOL    *TlsConfiguration;
   EFI_TLS_SESSION_STATE             TlsSessionState;
+  BOOLEAN                           TlsVerifyHost;
 
   //
   // TlsTxData used for transmitting TLS related messages.
diff --git a/NetworkPkg/HttpDxe/HttpImpl.c b/NetworkPkg/HttpDxe/HttpImpl.c
index 7c5c925cf7..df382acf33 100644
--- a/NetworkPkg/HttpDxe/HttpImpl.c
+++ b/NetworkPkg/HttpDxe/HttpImpl.c
@@ -3,6 +3,7 @@
 
   Copyright (c) 2015 - 2021, Intel Corporation. All rights reserved.<BR>
   (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
+  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -162,6 +163,7 @@ EfiHttpConfigure (
     HttpInstance->TimeOutMillisec    = HttpConfigData->TimeOutMillisec;
     HttpInstance->LocalAddressIsIPv6 = HttpConfigData->LocalAddressIsIPv6;
     HttpInstance->ConnectionClose    = FALSE;
+    HttpInstance->TlsVerifyHost      = (HttpConfigData->HostCertificateVerifyDisabled ? FALSE : TRUE);
 
     if (HttpConfigData->LocalAddressIsIPv6) {
       CopyMem (
diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c
index ad611e7c38..685a24b737 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -3,6 +3,7 @@
 
 Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
 (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
+Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -666,24 +667,26 @@ TlsConfigureSession (
     return Status;
   }
 
-  Status = HttpInstance->Tls->SetSessionData (
-                                HttpInstance->Tls,
-                                EfiTlsVerifyMethod,
-                                &HttpInstance->TlsConfigData.VerifyMethod,
-                                sizeof (EFI_TLS_VERIFY)
-                                );
-  if (EFI_ERROR (Status)) {
-    return Status;
-  }
+  if (HttpInstance->TlsVerifyHost) {
+    Status = HttpInstance->Tls->SetSessionData (
+                                  HttpInstance->Tls,
+                                  EfiTlsVerifyMethod,
+                                  &HttpInstance->TlsConfigData.VerifyMethod,
+                                  sizeof (EFI_TLS_VERIFY)
+                                  );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
 
-  Status = HttpInstance->Tls->SetSessionData (
-                                HttpInstance->Tls,
-                                EfiTlsVerifyHost,
-                                &HttpInstance->TlsConfigData.VerifyHost,
-                                sizeof (EFI_TLS_VERIFY_HOST)
-                                );
-  if (EFI_ERROR (Status)) {
-    return Status;
+    Status = HttpInstance->Tls->SetSessionData (
+                                  HttpInstance->Tls,
+                                  EfiTlsVerifyHost,
+                                  &HttpInstance->TlsConfigData.VerifyHost,
+                                  sizeof (EFI_TLS_VERIFY_HOST)
+                                  );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
   }
 
   Status = HttpInstance->Tls->SetSessionData (
@@ -705,13 +708,15 @@ TlsConfigureSession (
     return Status;
   }
 
-  //
-  // Tls Config Certificate
-  //
-  Status = TlsConfigCertificate (HttpInstance);
-  if (EFI_ERROR (Status)) {
-    DEBUG ((DEBUG_ERROR, "TLS Certificate Config Error!\n"));
-    return Status;
+  if (HttpInstance->TlsVerifyHost) {
+    //
+    // Tls Config Certificate
+    //
+    Status = TlsConfigCertificate (HttpInstance);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "TLS Certificate Config Error!\n"));
+      return Status;
+    }
   }
 
   //
-- 
2.39.1.windows.1

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Michael Brown]

On 01/02/2023 03:46, Nickle Wang via groups.io wrote:
> diff --git a/MdePkg/Include/Protocol/Http.h b/MdePkg/Include/Protocol/Http.h
> index 28e6221593..21a782eaac 100644
> --- a/MdePkg/Include/Protocol/Http.h
> +++ b/MdePkg/Include/Protocol/Http.h
> @@ -6,6 +6,7 @@
>   
>     Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
>     (C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP<BR>
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>     @par Revision Reference:
> @@ -161,6 +162,10 @@ typedef struct {
>     /// this instance will use EFI_DNS6_PROTOCOL and EFI_TCP6_PROTOCOL.
>     ///
>     BOOLEAN             LocalAddressIsIPv6;
> +  ///
> +  /// Verify server certificate during HTTPS handshake.
> +  ///
> +  BOOLEAN             HostCertificateVerifyDisabled;
>   
>     union {
>       ///

This change would break the ABI by changing the layout of a data 
structure defined in the UEFI specification.

Even worse, it does so by inserting a field into the middle of a 
structure: an ABI mismatch would result in one side attempting to 
dereference the BOOLEAN value as a pointer.

Nacked-by: Michael Brown <mcb30@ipxe.org>

Thanks,

Michael

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Nickle Wang]

[-- Attachment #1: Type: text/plain, Size: 2055 bytes --]

Hi Michael,

Thanks for catching this. To prevent the change to data structure, would you suggest me to create new interface in EFI_HTTP_PROTOCOL and disable TLS host verify?

Regards,
Nickle
________________________________
From: Michael Brown <mcb30@ipxe.org>
Sent: Wednesday, February 1, 2023 6:47 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; Nickle Wang <nicklew@nvidia.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy <igork@ami.com>; Nick Ramirez <nramirez@nvidia.com>
Subject: Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

External email: Use caution opening links or attachments


On 01/02/2023 03:46, Nickle Wang via groups.io wrote:
> diff --git a/MdePkg/Include/Protocol/Http.h b/MdePkg/Include/Protocol/Http.h
> index 28e6221593..21a782eaac 100644
> --- a/MdePkg/Include/Protocol/Http.h
> +++ b/MdePkg/Include/Protocol/Http.h
> @@ -6,6 +6,7 @@
>
>     Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
>     (C) Copyright 2015-2017 Hewlett Packard Enterprise Development LP<BR>
> +  Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>     SPDX-License-Identifier: BSD-2-Clause-Patent
>
>     @par Revision Reference:
> @@ -161,6 +162,10 @@ typedef struct {
>     /// this instance will use EFI_DNS6_PROTOCOL and EFI_TCP6_PROTOCOL.
>     ///
>     BOOLEAN             LocalAddressIsIPv6;
> +  ///
> +  /// Verify server certificate during HTTPS handshake.
> +  ///
> +  BOOLEAN             HostCertificateVerifyDisabled;
>
>     union {
>       ///

This change would break the ABI by changing the layout of a data
structure defined in the UEFI specification.

Even worse, it does so by inserting a field into the middle of a
structure: an ABI mismatch would result in one side attempting to
dereference the BOOLEAN value as a pointer.

Nacked-by: Michael Brown <mcb30@ipxe.org>

Thanks,

Michael


[-- Attachment #2: Type: text/html, Size: 4402 bytes --]

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Michael Brown]

On 01/02/2023 11:06, Nickle Wang via groups.io wrote:
> Thanks for catching this. To prevent the change to data structure, would 
> you suggest me to create new interface in EFI_HTTP_PROTOCOL and disable 
> TLS host verify?

Adding an interface to EFI_HTTP_PROTOCOL would also break the ABI by 
changing the layout of a data structure defined in the UEFI 
specification, and so can't be done.

I took a quick look through Http.h and I can't immediately see any way 
you can convey the information you want without making a breaking 
change.  There are no flags fields (that could be extended with extra 
flags in the same memory slot), no structure version number fields (that 
could allow structures to be extended, subject to a version number 
check), and no general-purpose "additional information" extension 
mechanism besides the one for passing arbitrary HTTP headers.

I suspect you'll need to either make a new protocol (lots of work, very 
ugly) or find some sideband mechanism you can use to work around the 
problem, like a PCD to globally enable/disable host verification.

It may be worth waiting for one of the HttpDxe maintainers to offer an 
opinion on this, since I am totally unfamiliar with this part of the 
codebase.

Sorry,

Michael

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Nickle Wang]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

Hi Michael,



Thank you very much for your feedback. PCD was my idea too, but this may have impact to other HTTPS connection. I like to only disable TLS host verify on Redfish connection between BIOS and BMC.



Hi @Maciej Rabeda<mailto:maciej.rabeda@linux.intel.com>, @Siyuan Fu<mailto:siyuan.fu@intel.com>,



May I have your comments about this challenge? I am looking for a way of passing a flag to HTTP instance and this flag will disable TLS host verification.



Thanks,

Nickle



-----Original Message-----
From: Michael Brown <mcb30@ipxe.org>
Sent: Wednesday, February 1, 2023 7:28 PM
To: devel@edk2.groups.io; Nickle Wang <nicklew@nvidia.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu <siyuan.fu@intel.com>; Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy <igork@ami.com>; Nick Ramirez <nramirez@nvidia.com>
Subject: Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify



External email: Use caution opening links or attachments





On 01/02/2023 11:06, Nickle Wang via groups.io wrote:

> Thanks for catching this. To prevent the change to data structure,

> would you suggest me to create new interface in EFI_HTTP_PROTOCOL and

> disable TLS host verify?



Adding an interface to EFI_HTTP_PROTOCOL would also break the ABI by changing the layout of a data structure defined in the UEFI specification, and so can't be done.



I took a quick look through Http.h and I can't immediately see any way you can convey the information you want without making a breaking change.  There are no flags fields (that could be extended with extra flags in the same memory slot), no structure version number fields (that could allow structures to be extended, subject to a version number check), and no general-purpose "additional information" extension mechanism besides the one for passing arbitrary HTTP headers.



I suspect you'll need to either make a new protocol (lots of work, very

ugly) or find some sideband mechanism you can use to work around the problem, like a PCD to globally enable/disable host verification.



It may be worth waiting for one of the HttpDxe maintainers to offer an opinion on this, since I am totally unfamiliar with this part of the codebase.



Sorry,



Michael



[-- Attachment #2: Type: text/html, Size: 5716 bytes --]

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Nickle Wang]

[-- Attachment #1: Type: text/plain, Size: 3958 bytes --]

Hi @Michael Brown<mailto:mcb30@ipxe.org>, @Maciej Rabeda<mailto:maciej.rabeda@linux.intel.com>, @Siyuan Fu<mailto:siyuan.fu@intel.com>,

I got an idea to handle this issue.

EFI_HTTP_SERVICE_BINDING_PROTOCOL is defined in UEFI specification for caller to create HTTP protocol on child instance. How about I propose a new service binding protocol called EFI_HTTP_NO_TLS_HOST_VERIFY_SERVICE_BINDING_PROTOCOL, and the EFI_HTTP_PROTOCOL created by this service binding protocol will not do TLS host verify during HTTPS communication.

When caller like to disable host verify on HTTPS communication, caller use this service binding protocol to create special HTTP instance. For other case, caller use regular EFI_HTTP_SERVICE_BINDING_PROTOCOL to get normal EFI_HTTP_PROTOCOL instance.

What do you think about this idea?

Thanks,
Nickle

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Nickle Wang via groups.io
Sent: Thursday, February 2, 2023 2:35 PM
To: Michael Brown <mcb30@ipxe.org>; devel@edk2.groups.io; Maciej Rabeda <maciej.rabeda@linux.intel.com>; Siyuan Fu <siyuan.fu@intel.com>
Cc: Abner Chang <abner.chang@amd.com>; Igor Kulchytskyy <igork@ami.com>; Nick Ramirez <nramirez@nvidia.com>
Subject: Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

External email: Use caution opening links or attachments


Hi Michael,



Thank you very much for your feedback. PCD was my idea too, but this may have impact to other HTTPS connection. I like to only disable TLS host verify on Redfish connection between BIOS and BMC.



Hi @Maciej Rabeda<mailto:maciej.rabeda@linux.intel.com>, @Siyuan Fu<mailto:siyuan.fu@intel.com>,



May I have your comments about this challenge? I am looking for a way of passing a flag to HTTP instance and this flag will disable TLS host verification.



Thanks,

Nickle



-----Original Message-----
From: Michael Brown <mcb30@ipxe.org<mailto:mcb30@ipxe.org>>
Sent: Wednesday, February 1, 2023 7:28 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Nickle Wang <nicklew@nvidia.com<mailto:nicklew@nvidia.com>>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com<mailto:maciej.rabeda@linux.intel.com>>; Siyuan Fu <siyuan.fu@intel.com<mailto:siyuan.fu@intel.com>>; Abner Chang <abner.chang@amd.com<mailto:abner.chang@amd.com>>; Igor Kulchytskyy <igork@ami.com<mailto:igork@ami.com>>; Nick Ramirez <nramirez@nvidia.com<mailto:nramirez@nvidia.com>>
Subject: Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify



External email: Use caution opening links or attachments





On 01/02/2023 11:06, Nickle Wang via groups.io wrote:

> Thanks for catching this. To prevent the change to data structure,

> would you suggest me to create new interface in EFI_HTTP_PROTOCOL and

> disable TLS host verify?



Adding an interface to EFI_HTTP_PROTOCOL would also break the ABI by changing the layout of a data structure defined in the UEFI specification, and so can't be done.



I took a quick look through Http.h and I can't immediately see any way you can convey the information you want without making a breaking change.  There are no flags fields (that could be extended with extra flags in the same memory slot), no structure version number fields (that could allow structures to be extended, subject to a version number check), and no general-purpose "additional information" extension mechanism besides the one for passing arbitrary HTTP headers.



I suspect you'll need to either make a new protocol (lots of work, very

ugly) or find some sideband mechanism you can use to work around the problem, like a PCD to globally enable/disable host verification.



It may be worth waiting for one of the HttpDxe maintainers to offer an opinion on this, since I am totally unfamiliar with this part of the codebase.



Sorry,



Michael




[-- Attachment #2: Type: text/html, Size: 9850 bytes --]

Re: [edk2-devel] [PATCH 1/2] NetworkPkg/HttpDxe: provide function to disable TLS host verify

[Michael Brown]

On 07/03/2023 08:21, Nickle Wang via groups.io wrote:
> I got an idea to handle this issue.
> 
> EFI_HTTP_SERVICE_BINDING_PROTOCOL is defined in UEFI specification for 
> caller to create HTTP protocol on child instance. How about I propose a 
> new service binding protocol called 
> EFI_HTTP_*NO_TLS_HOST_VERIFY*_SERVICE_BINDING_PROTOCOL, and the 
> EFI_HTTP_PROTOCOL created by this service binding protocol will not do 
> TLS host verify during HTTPS communication.
> 
> When caller like to disable host verify on HTTPS communication, caller 
> use this service binding protocol to create special HTTP instance. For 
> other case, caller use regular EFI_HTTP_SERVICE_BINDING_PROTOCOL to get 
> normal EFI_HTTP_PROTOCOL instance.

That seems very hacky, and does not help to address the general problem 
of being able to more flexibly configure HTTP connections.

 From a quick look through the UEFI spec, it looks as though 
EFI_TLS_PROTOCOL.SetSessionData() should already allow you to set 
EfiTlsVerifyMethod with a value of EFI_TLS_VERIFY_NONE.

The implementation of HttpDxe makes it very messy to gain access to the 
EFI_TLS_PROTOCOL instance, since it will be created only when 
EFI_HTTP_PROTOCOL.Request() is called.  I think you may have to use 
gBS->RegisterProtocolNotify() in order to intercept the point at which 
EFI_TLS_PROTOCOL is installed.  In your notification event callback, you 
would then check to see if the handle is a child of the 
EFI_HTTP_PROTOCOL handle and, if so, call 
EFI_TLS_PROTOCOL.SetSessionData() to disable host verification.

You would need to be using a newly created EFI_HTTP_PROTOCOL instance, 
so that you could be sure that there was no existing EFI_TLS_PROTOCOL 
instance already in place.

I haven't tested any of the above, but it looks as though it should work 
and allow you to disable host verification for a single 
EFI_HTTP_PROTOCOL instance, without any specification changes.

Michael