[PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960
CVE: CVE-2019-14553
The series patches are to support HTTPS hostname validation feature. 
It fixes the issue exposed @ 
https://bugzilla.tianocore.org/show_bug.cgi?id=960.
In the patches, we add the new data type named "EfiTlsVerifyHost" and
the EFI_TLS_VERIFY_HOST_FLAG for the TLS protocol consumer (HTTP) to 
enable the host name check so as to avoid the potential 
Man-In-The-Middle attack.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Long Qin <qin.long@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>

Jiaxin Wu (4):
  MdePkg/Include/Protocol/Tls.h: Add the data type of EfiTlsVerifyHost(CVE-2019-14553)
  CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost"(CVE-2019-14553)
  NetworkPkg/TlsDxe: Add the support of host validation to TlsDxe driver(CVE-2019-14553)
  NetworkPkg/HttpDxe: Set the HostName for the verification(CVE-2019-14553)

 CryptoPkg/Include/Library/TlsLib.h   | 20 ++++++++
 CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++-
 MdePkg/Include/Protocol/Tls.h        | 68 +++++++++++++++++++++++-----
 NetworkPkg/HttpDxe/HttpProto.h       |  1 +
 NetworkPkg/HttpDxe/HttpsSupport.c    | 21 +++++++--
 NetworkPkg/TlsDxe/TlsProtocol.c      | 44 ++++++++++++++++--
 6 files changed, 173 insertions(+), 19 deletions(-)

-- 
2.17.1.windows.2

[PATCH v1 1/4] MdePkg/Include/Protocol/Tls.h: Add the data type of EfiTlsVerifyHost(CVE-2019-14553)

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960
CVE: CVE-2019-14553
In the patch, we add the new data type named "EfiTlsVerifyHost" and
the EFI_TLS_VERIFY_HOST_FLAG for the TLS protocol consumer (HTTP)
to enable the host name check so as to avoid the potential
Man-In-The-Middle attack.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Long Qin <qin.long@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 MdePkg/Include/Protocol/Tls.h | 68 +++++++++++++++++++++++++++++------
 1 file changed, 57 insertions(+), 11 deletions(-)

diff --git a/MdePkg/Include/Protocol/Tls.h b/MdePkg/Include/Protocol/Tls.h
index bf1b6727a1..af524ae2a6 100644
--- a/MdePkg/Include/Protocol/Tls.h
+++ b/MdePkg/Include/Protocol/Tls.h
@@ -39,14 +39,10 @@ typedef struct _EFI_TLS_PROTOCOL EFI_TLS_PROTOCOL;
 
 ///
 /// EFI_TLS_SESSION_DATA_TYPE
 ///
 typedef enum {
-  ///
-  /// Session Configuration
-  ///
-
   ///
   /// TLS session Version. The corresponding Data is of type EFI_TLS_VERSION.
   ///
   EfiTlsVersion,
   ///
@@ -84,15 +80,10 @@ typedef enum {
   ///
   /// TLS session data session state.
   /// The corresponding Data is of type EFI_TLS_SESSION_STATE.
   ///
   EfiTlsSessionState,
-
-  ///
-  /// Session information
-  ///
-
   ///
   /// TLS session data client random.
   /// The corresponding Data is of type EFI_TLS_RANDOM.
   ///
   EfiTlsClientRandom,
@@ -104,13 +95,19 @@ typedef enum {
   ///
   /// TLS session data key material.
   /// The corresponding Data is of type EFI_TLS_MASTER_SECRET.
   ///
   EfiTlsKeyMaterial,
+  ///
+  /// TLS session hostname for validation which is used to verify whether the name
+  /// within the peer certificate matches a given host name.
+  /// This parameter is invalid when EfiTlsVerifyMethod is EFI_TLS_VERIFY_NONE.
+  /// The corresponding Data is of type EFI_TLS_VERIFY_HOST.
+  ///
+  EfiTlsVerifyHost,
 
   EfiTlsSessionDataTypeMaximum
-
 } EFI_TLS_SESSION_DATA_TYPE;
 
 ///
 /// EFI_TLS_VERSION
 /// Note: The TLS version definition is from SSL3.0 to the latest TLS (e.g. 1.2).
@@ -176,19 +173,68 @@ typedef UINT32  EFI_TLS_VERIFY;
 /// The TLS/SSL handshake is immediately terminated with an alert message containing
 /// the reason for the certificate verification failure.
 ///
 #define EFI_TLS_VERIFY_PEER                  0x1
 ///
-/// TLS session will fail peer certificate is absent.
+/// EFI_TLS_VERIFY_FAIL_IF_NO_PEER_CERT is only meaningful in the server mode.
+/// TLS session will fail if client certificate is absent.
 ///
 #define EFI_TLS_VERIFY_FAIL_IF_NO_PEER_CERT  0x2
 ///
 /// TLS session only verify client once, and doesn't request certificate during
 /// re-negotiation.
 ///
 #define EFI_TLS_VERIFY_CLIENT_ONCE           0x4
 
+///
+/// EFI_TLS_VERIFY_HOST_FLAG
+///
+typedef UINT32 EFI_TLS_VERIFY_HOST_FLAG;
+///
+/// There is no additional flags set for hostname validation.
+/// Wildcards are supported and they match only in the left-most label.
+///
+#define EFI_TLS_VERIFY_FLAG_NONE                    0x00
+///
+/// Always check the Subject Distinguished Name (DN) in the peer certificate even if the
+/// certificate contains Subject Alternative Name (SAN).
+///
+#define EFI_TLS_VERIFY_FLAG_ALWAYS_CHECK_SUBJECT    0x01
+///
+/// Disable the match of all wildcards.
+///
+#define EFI_TLS_VERIFY_FLAG_NO_WILDCARDS            0x02
+///
+/// Disable the "*" as wildcard in labels that have a prefix or suffix (e.g. "www*" or "*www").
+///
+#define EFI_TLS_VERIFY_FLAG_NO_PARTIAL_WILDCARDS    0x04
+///
+/// Allow the "*" to match more than one labels. Otherwise, only matches a single label.
+///
+#define EFI_TLS_VERIFY_FLAG_MULTI_LABEL_WILDCARDS   0x08
+///
+/// Restrict to only match direct child sub-domains which start with ".".
+/// For example, a name of ".example.com" would match "www.example.com" with this flag,
+/// but would not match "www.sub.example.com".
+///
+#define EFI_TLS_VERIFY_FLAG_SINGLE_LABEL_SUBDOMAINS 0x10
+///
+/// Never check the Subject Distinguished Name (DN) even there is no
+/// Subject Alternative Name (SAN) in the certificate.
+///
+#define EFI_TLS_VERIFY_FLAG_NEVER_CHECK_SUBJECT     0x20
+
+///
+/// EFI_TLS_VERIFY_HOST
+///
+#pragma pack (1)
+typedef struct {
+  EFI_TLS_VERIFY_HOST_FLAG Flags;
+  CHAR8                    *HostName;
+} EFI_TLS_VERIFY_HOST;
+#pragma pack ()
+
 ///
 /// EFI_TLS_RANDOM
 /// Note: The definition of EFI_TLS_RANDOM is from "RFC 5246 A.4.1.
 ///       Hello Messages".
 ///
-- 
2.17.1.windows.2

[PATCH v1 2/4] CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost"(CVE-2019-14553)

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960
CVE: CVE-2019-14553
In the patch, we add the new API "TlsSetVerifyHost" for the TLS
protocol to set the specified host name that need to be verified.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Long Qin <qin.long@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 CryptoPkg/Include/Library/TlsLib.h   | 20 +++++++++++++++
 CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++++++++++++++-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/CryptoPkg/Include/Library/TlsLib.h b/CryptoPkg/Include/Library/TlsLib.h
index 9875cb6e74..3af7d4bc09 100644
--- a/CryptoPkg/Include/Library/TlsLib.h
+++ b/CryptoPkg/Include/Library/TlsLib.h
@@ -394,10 +394,30 @@ EFIAPI
 TlsSetVerify (
   IN     VOID                     *Tls,
   IN     UINT32                   VerifyMode
   );
 
+/**
+  Set the specified host name to be verified.
+
+  @param[in]  Tls           Pointer to the TLS object.
+  @param[in]  Flags         The setting flags during the validation.
+  @param[in]  HostName      The specified host name to be verified.
+
+  @retval  EFI_SUCCESS           The HostName setting was set successfully.
+  @retval  EFI_INVALID_PARAMETER The parameter is invalid.
+  @retval  EFI_ABORTED           Invalid HostName setting.
+
+**/
+EFI_STATUS
+EFIAPI
+TlsSetVerifyHost (
+  IN     VOID                     *Tls,
+  IN     UINT32                   Flags,
+  IN     CHAR8                    *HostName
+  );
+
 /**
   Sets a TLS/SSL session ID to be used during TLS/SSL connect.
 
   This function sets a session ID to be used when the TLS/SSL connection is
   to be established.
diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index 74b577d60e..2bf5aee7c0 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -1,9 +1,9 @@
 /** @file
   SSL/TLS Configuration Library Wrapper Implementation over OpenSSL.
 
-Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
 (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
@@ -495,10 +495,46 @@ TlsSetVerify (
   // Set peer certificate verification parameters with NULL callback.
   //
   SSL_set_verify (TlsConn->Ssl, VerifyMode, NULL);
 }
 
+/**
+  Set the specified host name to be verified.
+
+  @param[in]  Tls           Pointer to the TLS object.
+  @param[in]  Flags         The setting flags during the validation.
+  @param[in]  HostName      The specified host name to be verified.
+
+  @retval  EFI_SUCCESS           The HostName setting was set successfully.
+  @retval  EFI_INVALID_PARAMETER The parameter is invalid.
+  @retval  EFI_ABORTED           Invalid HostName setting.
+
+**/
+EFI_STATUS
+EFIAPI
+TlsSetVerifyHost (
+  IN     VOID                     *Tls,
+  IN     UINT32                   Flags,
+  IN     CHAR8                    *HostName
+  )
+{
+  TLS_CONNECTION  *TlsConn;
+
+  TlsConn = (TLS_CONNECTION *) Tls;
+  if (TlsConn == NULL || TlsConn->Ssl == NULL || HostName == NULL) {
+     return EFI_INVALID_PARAMETER;
+  }
+
+  SSL_set_hostflags(TlsConn->Ssl, Flags);
+
+  if (SSL_set1_host(TlsConn->Ssl, HostName) == 0) {
+    return EFI_ABORTED;
+  }
+
+  return EFI_SUCCESS;
+}
+
 /**
   Sets a TLS/SSL session ID to be used during TLS/SSL connect.
 
   This function sets a session ID to be used when the TLS/SSL connection is
   to be established.
-- 
2.17.1.windows.2

[PATCH v1 3/4] NetworkPkg/TlsDxe: Add the support of host validation to TlsDxe driver(CVE-2019-14553)

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960
CVE: CVE-2019-14553
The new data type named "EfiTlsVerifyHost" and the
EFI_TLS_VERIFY_HOST_FLAG are supported in TLS protocol.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Long Qin <qin.long@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/TlsDxe/TlsProtocol.c | 44 ++++++++++++++++++++++++++++++---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/NetworkPkg/TlsDxe/TlsProtocol.c b/NetworkPkg/TlsDxe/TlsProtocol.c
index a7a993fc6f..001e5400d0 100644
--- a/NetworkPkg/TlsDxe/TlsProtocol.c
+++ b/NetworkPkg/TlsDxe/TlsProtocol.c
@@ -1,9 +1,9 @@
 /** @file
   Implementation of EFI TLS Protocol Interfaces.
 
-  Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
 
@@ -54,16 +54,20 @@ TlsSetSessionData (
   EFI_STATUS                Status;
   TLS_INSTANCE              *Instance;
   UINT16                    *CipherId;
   CONST EFI_TLS_CIPHER      *TlsCipherList;
   UINTN                     CipherCount;
+  CONST EFI_TLS_VERIFY_HOST *TlsVerifyHost;
+  EFI_TLS_VERIFY            VerifyMethod;
+  UINTN                     VerifyMethodSize;
   UINTN                     Index;
 
   EFI_TPL                   OldTpl;
 
-  Status = EFI_SUCCESS;
-  CipherId = NULL;
+  Status           = EFI_SUCCESS;
+  CipherId         = NULL;
+  VerifyMethodSize = sizeof (EFI_TLS_VERIFY);
 
   if (This == NULL || Data == NULL || DataSize == 0) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -146,10 +150,44 @@ TlsSetSessionData (
       Status = EFI_INVALID_PARAMETER;
       goto ON_EXIT;
     }
 
     TlsSetVerify (Instance->TlsConn, *((UINT32 *) Data));
+    break;
+  case EfiTlsVerifyHost:
+    if (DataSize != sizeof (EFI_TLS_VERIFY_HOST)) {
+      Status = EFI_INVALID_PARAMETER;
+      goto ON_EXIT;
+    }
+
+    TlsVerifyHost = (CONST EFI_TLS_VERIFY_HOST *) Data;
+
+    if ((TlsVerifyHost->Flags & EFI_TLS_VERIFY_FLAG_ALWAYS_CHECK_SUBJECT) != 0 &&
+        (TlsVerifyHost->Flags & EFI_TLS_VERIFY_FLAG_NEVER_CHECK_SUBJECT) != 0) {
+      Status = EFI_INVALID_PARAMETER;
+      goto ON_EXIT;
+    }
+
+    if ((TlsVerifyHost->Flags & EFI_TLS_VERIFY_FLAG_NO_WILDCARDS) != 0 &&
+        ((TlsVerifyHost->Flags & EFI_TLS_VERIFY_FLAG_NO_PARTIAL_WILDCARDS) != 0 ||
+         (TlsVerifyHost->Flags & EFI_TLS_VERIFY_FLAG_MULTI_LABEL_WILDCARDS) != 0)) {
+      Status = EFI_INVALID_PARAMETER;
+      goto ON_EXIT;
+    }
+
+    Status = This->GetSessionData (This, EfiTlsVerifyMethod, &VerifyMethod, &VerifyMethodSize);
+    if (EFI_ERROR (Status)) {
+      goto ON_EXIT;
+    }
+
+    if ((VerifyMethod & EFI_TLS_VERIFY_PEER) == 0) {
+      Status = EFI_INVALID_PARAMETER;
+      goto ON_EXIT;
+    }
+
+    Status = TlsSetVerifyHost (Instance->TlsConn, TlsVerifyHost->Flags, TlsVerifyHost->HostName);
+
     break;
   case EfiTlsSessionID:
     if (DataSize != sizeof (EFI_TLS_SESSION_ID)) {
       Status = EFI_INVALID_PARAMETER;
       goto ON_EXIT;
-- 
2.17.1.windows.2

[PATCH v1 4/4] NetworkPkg/HttpDxe: Set the HostName for the verification(CVE-2019-14553)

[Wu, Jiaxin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960
CVE: CVE-2019-14553
Set the HostName by consuming TLS protocol to enable the host name
check so as to avoid the potential Man-In-The-Middle attack.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
Reviewed-by: Ye Ting <ting.ye@intel.com>
Reviewed-by: Long Qin <qin.long@intel.com>
Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 NetworkPkg/HttpDxe/HttpProto.h    |  1 +
 NetworkPkg/HttpDxe/HttpsSupport.c | 21 +++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/NetworkPkg/HttpDxe/HttpProto.h b/NetworkPkg/HttpDxe/HttpProto.h
index 6e1f51748a..34308e016d 100644
--- a/NetworkPkg/HttpDxe/HttpProto.h
+++ b/NetworkPkg/HttpDxe/HttpProto.h
@@ -80,10 +80,11 @@ typedef struct {
 
 typedef struct {
   EFI_TLS_VERSION               Version;
   EFI_TLS_CONNECTION_END        ConnectionEnd;
   EFI_TLS_VERIFY                VerifyMethod;
+  EFI_TLS_VERIFY_HOST           VerifyHost;
   EFI_TLS_SESSION_STATE         SessionState;
 } TLS_CONFIG_DATA;
 
 //
 // Callback data for HTTP_PARSER_CALLBACK()
diff --git a/NetworkPkg/HttpDxe/HttpsSupport.c b/NetworkPkg/HttpDxe/HttpsSupport.c
index 988bbcbce7..5dfb13bd60 100644
--- a/NetworkPkg/HttpDxe/HttpsSupport.c
+++ b/NetworkPkg/HttpDxe/HttpsSupport.c
@@ -621,17 +621,20 @@ TlsConfigureSession (
   EFI_STATUS                 Status;
 
   //
   // TlsConfigData initialization
   //
-  HttpInstance->TlsConfigData.ConnectionEnd = EfiTlsClient;
-  HttpInstance->TlsConfigData.VerifyMethod = EFI_TLS_VERIFY_PEER;
-  HttpInstance->TlsConfigData.SessionState = EfiTlsSessionNotStarted;
+  HttpInstance->TlsConfigData.ConnectionEnd       = EfiTlsClient;
+  HttpInstance->TlsConfigData.VerifyMethod        = EFI_TLS_VERIFY_PEER;
+  HttpInstance->TlsConfigData.VerifyHost.Flags    = EFI_TLS_VERIFY_FLAG_NO_WILDCARDS;
+  HttpInstance->TlsConfigData.VerifyHost.HostName = HttpInstance->RemoteHost;
+  HttpInstance->TlsConfigData.SessionState        = EfiTlsSessionNotStarted;
 
   //
   // EfiTlsConnectionEnd,
-  // EfiTlsVerifyMethod
+  // EfiTlsVerifyMethod,
+  // EfiTlsVerifyHost,
   // EfiTlsSessionState
   //
   Status = HttpInstance->Tls->SetSessionData (
                                 HttpInstance->Tls,
                                 EfiTlsConnectionEnd,
@@ -650,10 +653,20 @@ TlsConfigureSession (
                                 );
   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,
                                 EfiTlsSessionState,
                                 &(HttpInstance->TlsConfigData.SessionState),
                                 sizeof (EFI_TLS_SESSION_STATE)
-- 
2.17.1.windows.2

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Wang, Jian J]

For this patch series,
1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed any more.
  Remove it at push time and no need to send a v2.
2. Since it's security patch which had been reviewed separately, I see no reason for new r-b
  required. Please raise it asap if any objections.
3. Acked-by: Jian J Wang <jian.j.wang@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Jiaxin
> Sent: Friday, September 27, 2019 11:45 AM
> To: devel@edk2.groups.io
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
> Subject: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation
> feature(CVE-2019-14553)
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960
> CVE: CVE-2019-14553
> The series patches are to support HTTPS hostname validation feature.
> It fixes the issue exposed @
> https://bugzilla.tianocore.org/show_bug.cgi?id=960.
> In the patches, we add the new data type named "EfiTlsVerifyHost" and
> the EFI_TLS_VERIFY_HOST_FLAG for the TLS protocol consumer (HTTP) to
> enable the host name check so as to avoid the potential
> Man-In-The-Middle attack.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
> Reviewed-by: Ye Ting <ting.ye@intel.com>
> Reviewed-by: Long Qin <qin.long@intel.com>
> Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> Jiaxin Wu (4):
>   MdePkg/Include/Protocol/Tls.h: Add the data type of EfiTlsVerifyHost(CVE-
> 2019-14553)
>   CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost"(CVE-2019-14553)
>   NetworkPkg/TlsDxe: Add the support of host validation to TlsDxe driver(CVE-
> 2019-14553)
>   NetworkPkg/HttpDxe: Set the HostName for the verification(CVE-2019-14553)
> 
>  CryptoPkg/Include/Library/TlsLib.h   | 20 ++++++++
>  CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++-
>  MdePkg/Include/Protocol/Tls.h        | 68 +++++++++++++++++++++++-----
>  NetworkPkg/HttpDxe/HttpProto.h       |  1 +
>  NetworkPkg/HttpDxe/HttpsSupport.c    | 21 +++++++--
>  NetworkPkg/TlsDxe/TlsProtocol.c      | 44 ++++++++++++++++--
>  6 files changed, 173 insertions(+), 19 deletions(-)
> 
> --
> 2.17.1.windows.2
> 
> 
> 

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 09/29/19 08:09, Wang, Jian J wrote:
> For this patch series,
> 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed any more.
>   Remove it at push time and no need to send a v2.
> 2. Since it's security patch which had been reviewed separately, I see no reason for new r-b
>   required. Please raise it asap if any objections.
> 3. Acked-by: Jian J Wang <jian.j.wang@intel.com>


* Can you please confirm that these patches match those that we
discussed here:

https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18
https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19


* In the BZ, David and Bret raised some questions:

https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31
https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35
https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36

and

https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40

The latest comment in the bug is c#41. I'm not under the impression that
all concerns raised by David and Bret have been addressed (or
abandoned). I'd like David and Bret to ACK the patches.

Thanks,
Laszlo



>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Jiaxin
>> Sent: Friday, September 27, 2019 11:45 AM
>> To: devel@edk2.groups.io
>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>
>> Subject: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation
>> feature(CVE-2019-14553)
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960
>> CVE: CVE-2019-14553
>> The series patches are to support HTTPS hostname validation feature.
>> It fixes the issue exposed @
>> https://bugzilla.tianocore.org/show_bug.cgi?id=960.
>> In the patches, we add the new data type named "EfiTlsVerifyHost" and
>> the EFI_TLS_VERIFY_HOST_FLAG for the TLS protocol consumer (HTTP) to
>> enable the host name check so as to avoid the potential
>> Man-In-The-Middle attack.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
>> Reviewed-by: Ye Ting <ting.ye@intel.com>
>> Reviewed-by: Long Qin <qin.long@intel.com>
>> Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Jiaxin Wu (4):
>>   MdePkg/Include/Protocol/Tls.h: Add the data type of EfiTlsVerifyHost(CVE-
>> 2019-14553)
>>   CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost"(CVE-2019-14553)
>>   NetworkPkg/TlsDxe: Add the support of host validation to TlsDxe driver(CVE-
>> 2019-14553)
>>   NetworkPkg/HttpDxe: Set the HostName for the verification(CVE-2019-14553)
>>
>>  CryptoPkg/Include/Library/TlsLib.h   | 20 ++++++++
>>  CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++-
>>  MdePkg/Include/Protocol/Tls.h        | 68 +++++++++++++++++++++++-----
>>  NetworkPkg/HttpDxe/HttpProto.h       |  1 +
>>  NetworkPkg/HttpDxe/HttpsSupport.c    | 21 +++++++--
>>  NetworkPkg/TlsDxe/TlsProtocol.c      | 44 ++++++++++++++++--
>>  6 files changed, 173 insertions(+), 19 deletions(-)
>>
>> --
>> 2.17.1.windows.2
>>
>>
>>
> 
> 
> 
> 

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote:
> On 09/29/19 08:09, Wang, Jian J wrote:
> > For this patch series,
> > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed any more.
> >   Remove it at push time and no need to send a v2.
> > 2. Since it's security patch which had been reviewed separately, I see no reason for new r-b
> >   required. Please raise it asap if any objections.
> > 3. Acked-by: Jian J Wang <jian.j.wang@intel.com>
> 
> 
> * Can you please confirm that these patches match those that we
> discussed here:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19
> 
> 
> * In the BZ, David and Bret raised some questions:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36
> 
> and
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40
> 
> The latest comment in the bug is c#41. I'm not under the impression that
> all concerns raised by David and Bret have been addressed (or
> abandoned). I'd like David and Bret to ACK the patches.

I do not believe my comment #35 has been addressed, nor the requested
testing performed.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Wu, Jiaxin]

Hi David,

I just realized you have the comments on Bugzilla 960: 

>"...given that testing is failing and code inspection shows it would never have been expected to work."

Do you mean you didn't pass the verification if URLs with IPv6 literals (https://[2001:8b0:10b:1236::1]/)?  Can you also show me where the code inspection indicated it would never have been expected to work? We do pass the testing for the URLs with IPv6 if the CN or SAN in certificate has the corresponding IPv6 address (at least working with openssl 1.1.0). 

For the series patches here, we are intending to support the host name validation, I think we can commit the series patches since we pass the verification of IPV6 URL, what do you think?

Thanks,
Jiaxin    

> -----Original Message-----
> From: David Woodhouse <dwmw2@infradead.org>
> Sent: Tuesday, October 1, 2019 5:02 PM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName
> validation feature(CVE-2019-14553)
> 
> On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote:
> > On 09/29/19 08:09, Wang, Jian J wrote:
> > > For this patch series,
> > > 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not
> needed any more.
> > >   Remove it at push time and no need to send a v2.
> > > 2. Since it's security patch which had been reviewed separately, I see no
> reason for new r-b
> > >   required. Please raise it asap if any objections.
> > > 3. Acked-by: Jian J Wang <jian.j.wang@intel.com>
> >
> >
> > * Can you please confirm that these patches match those that we
> > discussed here:
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19
> >
> >
> > * In the BZ, David and Bret raised some questions:
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36
> >
> > and
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40
> >
> > The latest comment in the bug is c#41. I'm not under the impression that
> > all concerns raised by David and Bret have been addressed (or
> > abandoned). I'd like David and Bret to ACK the patches.
> 
> I do not believe my comment #35 has been addressed, nor the requested
> testing performed.

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote:
> Hi David,
> 
> I just realized you have the comments on Bugzilla 960: 
> 
> > "...given that testing is failing and code inspection shows it
> > would never have been expected to work."
> 
> Do you mean you didn't pass the verification if URLs with IPv6
> literals (https://[2001:8b0:10b:1236::1]/)?  Can you also show me
> where the code inspection indicated it would never have been expected
> to work? We do pass the testing for the URLs with IPv6 if the CN or
> SAN in certificate has the corresponding IPv6 address (at least
> working with openssl 1.1.0). 

I have not tested this, but I started looking when there was a message
on the edk2 list from someone who was reporting that it didn't work for
IPv6 URIs, IIRC.

You are using SSL_set1_host(), and I believe you're just passing in the
bare hostname part of the URI, be it "1.2.3.4" or "[2001:8b0:10b::5]".

That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the
SSL connection.

In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the
code simply iterates over the members of that list, calling
X509_check_host() for each one. It never calls X509_check_ip().

If you look in openssl/crypto/x509/v3_utl.c you can see the
X509_check_host() really does only check hostnames. You'd need to call
X509_check_ip_asc() to check hostnames. And something would need to
have stripped the [] which surround an IPv6 literal.

I can't see how this can work. Have you tested it since the report on
the list that it wasn't working? 

cf. https://github.com/openssl/openssl/pull/9201 which is being ignored
by the OpenSSL developers — OpenSSL really doesn't make life easy for
you here, which is a shame.


> For the series patches here, we are intending to support the host
> name validation, I think we can commit the series patches since we
> pass the verification of IPV6 URL, what do you think?

If it passes the verification of IPv6 literals, then all my analysis is
broken and so was the report on the list that prompted me to start
looking (or I'm misremembering that report). In that case, sure, go
ahead and commit.

> Thanks,
> Jiaxin    
> 
> > -----Original Message-----
> > From: David Woodhouse <dwmw2@infradead.org>
> > Sent: Tuesday, October 1, 2019 5:02 PM
> > To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Wang,
> > Jian J
> > <jian.j.wang@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Bret
> > Barkelew
> > <Bret.Barkelew@microsoft.com>
> > Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName
> > validation feature(CVE-2019-14553)
> > 
> > On Tue, 2019-10-01 at 01:21 +0200, Laszlo Ersek wrote:
> > > On 09/29/19 08:09, Wang, Jian J wrote:
> > > > For this patch series,
> > > > 1. " Contributed-under: TianoCore Contribution Agreement 1.1"
> > > > is not
> > 
> > needed any more.
> > > >   Remove it at push time and no need to send a v2.
> > > > 2. Since it's security patch which had been reviewed
> > > > separately, I see no
> > 
> > reason for new r-b
> > > >   required. Please raise it asap if any objections.
> > > > 3. Acked-by: Jian J Wang <jian.j.wang@intel.com>
> > > 
> > > 
> > > * Can you please confirm that these patches match those that we
> > > discussed here:
> > > 
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19
> > > 
> > > 
> > > * In the BZ, David and Bret raised some questions:
> > > 
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36
> > > 
> > > and
> > > 
> > > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40
> > > 
> > > The latest comment in the bug is c#41. I'm not under the
> > > impression that
> > > all concerns raised by David and Bret have been addressed (or
> > > abandoned). I'd like David and Bret to ACK the patches.
> > 
> > I do not believe my comment #35 has been addressed, nor the
> > requested
> > testing performed.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/01/19 01:21, Laszlo Ersek wrote:
> On 09/29/19 08:09, Wang, Jian J wrote:
>> For this patch series,
>> 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not needed any more.
>>   Remove it at push time and no need to send a v2.
>> 2. Since it's security patch which had been reviewed separately, I see no reason for new r-b
>>   required. Please raise it asap if any objections.
>> 3. Acked-by: Jian J Wang <jian.j.wang@intel.com>
> 
> 
> * Can you please confirm that these patches match those that we
> discussed here:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19

To answer my own question, I've now compared the patches from those BZ
comments linked above, with the present series. Here's a list of
differences.

(1) The subject lines now include the reference "(CVE-2019-14553)".

This is great, *but* please be sure to insert a space character before
the opening parenthesis! (In every patch.)

(2) The commit messages reference both the BZ and the CVE number.

Good.

(3) In the commit messages, the line

  Contributed-under: TianoCore Contribution Agreement 1.0

has been upgraded to

  Contributed-under: TianoCore Contribution Agreement 1.1

I think this is wrong. The lines should have been removed, due to the
SPDX adoption. Please update all the commit messages.

(4) Copyright notice updates are gone from the patches.

That's fine: the reason is that the underlying files have seen their
copyright notices updated, meanwhile.


Otherwise, the patches (code, commit messages, and feedback tags) are
identical.

Before you push the patches (or post a v2), please fix issues (1) and (3).

Now, regarding the other set of questions:

> * In the BZ, David and Bret raised some questions:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36
> 
> and
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40
> 
> The latest comment in the bug is c#41. I'm not under the impression that
> all concerns raised by David and Bret have been addressed (or
> abandoned). I'd like David and Bret to ACK the patches.

I'll first have to process the new comments down-thread.

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

Hi All,

(multi-hour composition ahead...)

On 10/09/19 09:53, David Woodhouse wrote:
> On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote:
>> Hi David,
>>
>> I just realized you have the comments on Bugzilla 960:
>>
>>> "...given that testing is failing and code inspection shows it
>>> would never have been expected to work."
>>
>> Do you mean you didn't pass the verification if URLs with IPv6
>> literals (https://[2001:8b0:10b:1236::1]/)?  Can you also show me
>> where the code inspection indicated it would never have been expected
>> to work? We do pass the testing for the URLs with IPv6 if the CN or
>> SAN in certificate has the corresponding IPv6 address (at least
>> working with openssl 1.1.0).
>
> I have not tested this, but I started looking when there was a message
> on the edk2 list from someone who was reporting that it didn't work
> for IPv6 URIs, IIRC.
>
> You are using SSL_set1_host(), and I believe you're just passing in
> the bare hostname part of the URI, be it "1.2.3.4" or
> "[2001:8b0:10b::5]".
>
> That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the
> SSL connection.
>
> In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the
> code simply iterates over the members of that list, calling
> X509_check_host() for each one. It never calls X509_check_ip().
>
> If you look in openssl/crypto/x509/v3_utl.c you can see the
> X509_check_host() really does only check hostnames. You'd need to call
> X509_check_ip_asc() to check hostnames. And something would need to
> have stripped the [] which surround an IPv6 literal.
>
> I can't see how this can work. Have you tested it since the report on
> the list that it wasn't working?
>
> cf. https://github.com/openssl/openssl/pull/9201 which is being
> ignored by the OpenSSL developers \x14 OpenSSL really doesn't make
> life easy for you here, which is a shame.
>
>
>> For the series patches here, we are intending to support the host
>> name validation, I think we can commit the series patches since we
>> pass the verification of IPV6 URL, what do you think?
>
> If it passes the verification of IPv6 literals, then all my analysis
> is broken and so was the report on the list that prompted me to start
> looking (or I'm misremembering that report). In that case, sure, go
> ahead and commit.

Here's a summary of my setup.

* I've generated a brand new CA certificate, and two HTTP server
  certificates, signed by the CA.

* One HTTP server certificate is for Common Name = 192.168.124.2

* The other HTTP server certificate is for Common Name =
  fd33:eb1b:9b36::2

* I have a "net-server" virtual machine that runs Apache on the above IP
  addresses (TCP port 443).

  - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons.

  - The DHCP servers send the following boot file names:

    - "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso"       [IPv4]
    - "https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" [IPv6]

* For sanity-checking the environment, I run the following two commands
  on the *host* (connecting to the "net-server" virtual machine):

  - curl           -I 'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso'
  - curl --globoff -I 'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso'

  - The host is configured to trust the brand new test CA certificate
    (see near the top).

  - When the certificates are assigned *correctly* to the IP addresses
    in the Apache configuration, the above "curl" commands complete just
    fine. If I add the "-v" option to "curl", it confirms the right
    certificates are used, and it confirms the test CA as issuer too.

  - When the certificates are (intentionally) *cross-assigned* to the IP
    addresses in the Apache configuration, then both "curl" commands
    break with the following error message:

> curl: (51) Unable to communicate securely with peer: requested domain
> name does not match the server's certificate.

  - If I add the "-v" option, I also see

> NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN)

  - As a side comment: Apache itself warns about the misconfig, in
    "/var/log/httpd/ssl_error_log":

> ... [ssl:warn] ... AH01909: RSA certificate configured for ...:443
> does NOT include an ID which matches the server name

* I have a "net-client" virtual machine, running OVMF.

  - The edk2 HTTPS/TLS client booting in this virtual machine is
    configured to trust the exact same set of CA certificates that the
    host trusts too.

  - In other words, HTTPS boot in the "net-client" VM accepts server
    certificates signed by the new test CA.

* The following is the test plan.

1. The patch set is *not* applied (that is, OVMF is built at current
   master, commit 976d0353a6ce).

  1. Properly assigned certificates:

    1. HTTPSv4 boot --> expect success (correct behavior, establishes
                                        baseline)

    2. HTTPSv6 boot --> expect success (correct behavior, establishes
                                        baseline)

  2. Cross-assigned certificates:

    1. HTTPSv4 boot --> expect success (for reproducing the bug)

    2. HTTPSv6 boot --> expect success (for reproducing the bug)

2. With the patch set applied:

  1. Properly assigned certificates:

    1. HTTPSv4 boot --> expect success (failure means a regression)

    2. HTTPSv6 boot --> expect success (failure means a regression)

  2. Cross-assigned certificates:

    1. HTTPSv4 boot --> expect failure (for verifying the bugfix)

    2. HTTPSv6 boot --> expect failure (for verifying the bugfix)

* Results:

- 1.1.1. as expected (HTTPSv4 baseline established)
- 1.1.2. as expected (HTTPSv6 baseline established)
- 1.2.1. as expected (HTTPSv4 MITM bug reproduced)
- 1.2.2. as expected (HTTPSv6 MITM bug reproduced)
- 2.1.1. as expected (HTTPSv4 not regressed by series)
- 2.1.2. as expected (HTTPSv6 not regressed by series)
- 2.2.1. as expected (HTTPSv4 MITM averted)
- 2.2.2. as expected (HTTPSv6 MITM averted)

* In cases 2.2.1. and 2.2.2.:

- The UEFI console contains, respectively:

> >>Start HTTP Boot over IPv4....
>   Station IP address is 192.168.124.106
>
>   URI: https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso
>
>   Error: Could not retrieve NBP file size from HTTP server.
>
>   Error: Unexpected network error.

> >>Start HTTP Boot over IPv6....
>   Station IPv6 address is FD33:EB1B:9B36:0:0:0:0:C8
>
>   URI: https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso
>
>   Error: Could not retrieve NBP file size from HTTP server.
>
>   Error: Unexpected network error.

- The OVMF log contains (in both cases):

> TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL
> TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86

- Decoding:

  - Library   0x14 -> ERR_LIB_SSL
  - Function 0x16F -> SSL_F_TLS_PROCESS_SERVER_CERTIFICATE
  - Reason    0x86 -> SSL_R_CERTIFICATE_VERIFY_FAILED

  - So this means that the ssl_verify_cert_chain() call fails in the
    tls_process_server_certificate() function, in
    "CryptoPkg/Library/OpensslLib/openssl/ssl/statem/statem_clnt.c".


Normally the above would be sufficient for me to give a "Tested-by" for
this patch set.

But now I'm uncertain whether (a) my results contradict David's
analysis, or (b) I tested something that David's analysis doesn't
*apply* to. (IOW if my test plan doesn't actually verify "IPv6
literals".)


FWIW, the brackets in the IPv6 notation are stripped in EfiHttpRequest()
[NetworkPkg/HttpDxe/HttpImpl.c], using the "HostName" local variable.
(The stripping comes from earlier commit 7191827f90b4
("NetworkPkg/HttpDxe: Strip square brackets in IPv6 expressed
HostName.", 2018-08-03).) Later in EfiHttpRequest(),
"HttpInstance->RemoteHost" is assigned "HostName".

Further, in TlsConfigureSession() [NetworkPkg/HttpDxe/HttpsSupport.c],
we set "HttpInstance->TlsConfigData.VerifyHost.HostName" to
"HttpInstance->RemoteHost". This is done in patch#4.

Also in patch#4, in the same function, we pass
"HttpInstance->TlsConfigData.VerifyHost" to SetSessionData(), from
patch#3.

There we pass "TlsVerifyHost->HostName" to TlsSetVerifyHost(), which
resides in patch#2.

At that point, we pass the hostname -- the IPv6 address, with the
brackets stripped -- to SSL_set1_host().

So, my take is that the comparison is done simply on the textual
representation (with the IPv6 brackets stripped), not the numerical
value.

Is that bad? The textual comparison may certainly report a mismatch when
the numerical values actually match (for an IPv4 example, "192.168.0.1"
would not match "192.168.000.001"). But that errs in the safe direction,
does it not?

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

Can you show result of 'openssl x509 -noout -text -in xxxxxx.pem' on your certs please.

Would like to check if you really have a cert for the hostname string "192.168.124.2" or to the IP address. They are different things.


On 9 October 2019 21:24:34 BST, Laszlo Ersek <lersek@redhat.com> wrote:
>Hi All,
>
>(multi-hour composition ahead...)
>
>On 10/09/19 09:53, David Woodhouse wrote:
>> On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote:
>>> Hi David,
>>>
>>> I just realized you have the comments on Bugzilla 960:
>>>
>>>> "...given that testing is failing and code inspection shows it
>>>> would never have been expected to work."
>>>
>>> Do you mean you didn't pass the verification if URLs with IPv6
>>> literals (https://[2001:8b0:10b:1236::1]/)?  Can you also show me
>>> where the code inspection indicated it would never have been
>expected
>>> to work? We do pass the testing for the URLs with IPv6 if the CN or
>>> SAN in certificate has the corresponding IPv6 address (at least
>>> working with openssl 1.1.0).
>>
>> I have not tested this, but I started looking when there was a
>message
>> on the edk2 list from someone who was reporting that it didn't work
>> for IPv6 URIs, IIRC.
>>
>> You are using SSL_set1_host(), and I believe you're just passing in
>> the bare hostname part of the URI, be it "1.2.3.4" or
>> "[2001:8b0:10b::5]".
>>
>> That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for
>the
>> SSL connection.
>>
>> In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the
>> code simply iterates over the members of that list, calling
>> X509_check_host() for each one. It never calls X509_check_ip().
>>
>> If you look in openssl/crypto/x509/v3_utl.c you can see the
>> X509_check_host() really does only check hostnames. You'd need to
>call
>> X509_check_ip_asc() to check hostnames. And something would need to
>> have stripped the [] which surround an IPv6 literal.
>>
>> I can't see how this can work. Have you tested it since the report on
>> the list that it wasn't working?
>>
>> cf. https://github.com/openssl/openssl/pull/9201 which is being
>> ignored by the OpenSSL developers \x14 OpenSSL really doesn't make
>> life easy for you here, which is a shame.
>>
>>
>>> For the series patches here, we are intending to support the host
>>> name validation, I think we can commit the series patches since we
>>> pass the verification of IPV6 URL, what do you think?
>>
>> If it passes the verification of IPv6 literals, then all my analysis
>> is broken and so was the report on the list that prompted me to start
>> looking (or I'm misremembering that report). In that case, sure, go
>> ahead and commit.
>
>Here's a summary of my setup.
>
>* I've generated a brand new CA certificate, and two HTTP server
>  certificates, signed by the CA.
>
>* One HTTP server certificate is for Common Name = 192.168.124.2
>
>* The other HTTP server certificate is for Common Name =
>  fd33:eb1b:9b36::2
>
>* I have a "net-server" virtual machine that runs Apache on the above
>IP
>  addresses (TCP port 443).
>
>  - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons.
>
>  - The DHCP servers send the following boot file names:
>
>- "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso"   
>   [IPv4]
>-
>"https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso"
>[IPv6]
>
>* For sanity-checking the environment, I run the following two commands
>  on the *host* (connecting to the "net-server" virtual machine):
>
>- curl           -I
>'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso'
>- curl --globoff -I
>'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso'
>
>  - The host is configured to trust the brand new test CA certificate
>    (see near the top).
>
>  - When the certificates are assigned *correctly* to the IP addresses
>   in the Apache configuration, the above "curl" commands complete just
>    fine. If I add the "-v" option to "curl", it confirms the right
>    certificates are used, and it confirms the test CA as issuer too.
>
> - When the certificates are (intentionally) *cross-assigned* to the IP
>    addresses in the Apache configuration, then both "curl" commands
>    break with the following error message:
>
>> curl: (51) Unable to communicate securely with peer: requested domain
>> name does not match the server's certificate.
>
>  - If I add the "-v" option, I also see
>
>> NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN)
>
>  - As a side comment: Apache itself warns about the misconfig, in
>    "/var/log/httpd/ssl_error_log":
>
>> ... [ssl:warn] ... AH01909: RSA certificate configured for ...:443
>> does NOT include an ID which matches the server name
>
>* I have a "net-client" virtual machine, running OVMF.
>
>  - The edk2 HTTPS/TLS client booting in this virtual machine is
>    configured to trust the exact same set of CA certificates that the
>    host trusts too.
>
>  - In other words, HTTPS boot in the "net-client" VM accepts server
>    certificates signed by the new test CA.
>
>* The following is the test plan.
>
>1. The patch set is *not* applied (that is, OVMF is built at current
>   master, commit 976d0353a6ce).
>
>  1. Properly assigned certificates:
>
>    1. HTTPSv4 boot --> expect success (correct behavior, establishes
>                                        baseline)
>
>    2. HTTPSv6 boot --> expect success (correct behavior, establishes
>                                        baseline)
>
>  2. Cross-assigned certificates:
>
>    1. HTTPSv4 boot --> expect success (for reproducing the bug)
>
>    2. HTTPSv6 boot --> expect success (for reproducing the bug)
>
>2. With the patch set applied:
>
>  1. Properly assigned certificates:
>
>    1. HTTPSv4 boot --> expect success (failure means a regression)
>
>    2. HTTPSv6 boot --> expect success (failure means a regression)
>
>  2. Cross-assigned certificates:
>
>    1. HTTPSv4 boot --> expect failure (for verifying the bugfix)
>
>    2. HTTPSv6 boot --> expect failure (for verifying the bugfix)
>
>* Results:
>
>- 1.1.1. as expected (HTTPSv4 baseline established)
>- 1.1.2. as expected (HTTPSv6 baseline established)
>- 1.2.1. as expected (HTTPSv4 MITM bug reproduced)
>- 1.2.2. as expected (HTTPSv6 MITM bug reproduced)
>- 2.1.1. as expected (HTTPSv4 not regressed by series)
>- 2.1.2. as expected (HTTPSv6 not regressed by series)
>- 2.2.1. as expected (HTTPSv4 MITM averted)
>- 2.2.2. as expected (HTTPSv6 MITM averted)
>
>* In cases 2.2.1. and 2.2.2.:
>
>- The UEFI console contains, respectively:
>
>> >>Start HTTP Boot over IPv4....
>>   Station IP address is 192.168.124.106
>>
>>   URI:
>https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso
>>
>>   Error: Could not retrieve NBP file size from HTTP server.
>>
>>   Error: Unexpected network error.
>
>> >>Start HTTP Boot over IPv6....
>>   Station IPv6 address is FD33:EB1B:9B36:0:0:0:0:C8
>>
>>   URI:
>https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso
>>
>>   Error: Could not retrieve NBP file size from HTTP server.
>>
>>   Error: Unexpected network error.
>
>- The OVMF log contains (in both cases):
>
>> TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL
>> TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86
>
>- Decoding:
>
>  - Library   0x14 -> ERR_LIB_SSL
>  - Function 0x16F -> SSL_F_TLS_PROCESS_SERVER_CERTIFICATE
>  - Reason    0x86 -> SSL_R_CERTIFICATE_VERIFY_FAILED
>
>  - So this means that the ssl_verify_cert_chain() call fails in the
>    tls_process_server_certificate() function, in
>    "CryptoPkg/Library/OpensslLib/openssl/ssl/statem/statem_clnt.c".
>
>
>Normally the above would be sufficient for me to give a "Tested-by" for
>this patch set.
>
>But now I'm uncertain whether (a) my results contradict David's
>analysis, or (b) I tested something that David's analysis doesn't
>*apply* to. (IOW if my test plan doesn't actually verify "IPv6
>literals".)
>
>
>FWIW, the brackets in the IPv6 notation are stripped in
>EfiHttpRequest()
>[NetworkPkg/HttpDxe/HttpImpl.c], using the "HostName" local variable.
>(The stripping comes from earlier commit 7191827f90b4
>("NetworkPkg/HttpDxe: Strip square brackets in IPv6 expressed
>HostName.", 2018-08-03).) Later in EfiHttpRequest(),
>"HttpInstance->RemoteHost" is assigned "HostName".
>
>Further, in TlsConfigureSession() [NetworkPkg/HttpDxe/HttpsSupport.c],
>we set "HttpInstance->TlsConfigData.VerifyHost.HostName" to
>"HttpInstance->RemoteHost". This is done in patch#4.
>
>Also in patch#4, in the same function, we pass
>"HttpInstance->TlsConfigData.VerifyHost" to SetSessionData(), from
>patch#3.
>
>There we pass "TlsVerifyHost->HostName" to TlsSetVerifyHost(), which
>resides in patch#2.
>
>At that point, we pass the hostname -- the IPv6 address, with the
>brackets stripped -- to SSL_set1_host().
>
>So, my take is that the comparison is done simply on the textual
>representation (with the IPv6 brackets stripped), not the numerical
>value.
>
>Is that bad? The textual comparison may certainly report a mismatch
>when
>the numerical values actually match (for an IPv4 example, "192.168.0.1"
>would not match "192.168.000.001"). But that errs in the safe
>direction,
>does it not?
>
>Thanks!
>Laszlo

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Wu, Jiaxin]

> 
> I have not tested this, but I started looking when there was a message
> on the edk2 list from someone who was reporting that it didn't work for
> IPv6 URIs, IIRC.
> 
> You are using SSL_set1_host(), and I believe you're just passing in the
> bare hostname part of the URI, be it "1.2.3.4" or "[2001:8b0:10b::5]".

Here, I want to highlight is that UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2:     
    "...TLS session hostname for validation which is used to verify whether the name within the peer certificate matches a given host name..." 
In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature by following the standard TLS HostName verification capability.
  
> 
> That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the
> SSL connection.

Yes.

> 
> In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the
> code simply iterates over the members of that list, calling
> X509_check_host() for each one. It never calls X509_check_ip().

Yes.

> 
> If you look in openssl/crypto/x509/v3_utl.c you can see the
> X509_check_host() really does only check hostnames. 

Yes.

> You'd need to call X509_check_ip_asc() to check hostnames. And something would need to
> have stripped the [] which surround an IPv6 literal.
> 

Disagree, why need check the IP here since we only focus on the hostname verification? For HttpDxe driver, it's the implementation choice to treat the IP in URI as hostname string format. As I said before in the email, if the CN or SAN (Seems only in X509 V3) in the certificate are set correctly, it should be OK to pass the verification. Laszlo and I already have verified that.

> I can't see how this can work. Have you tested it since the report on
> the list that it wasn't working?
> 

Sorry, I can't remember there is any failure of Ipv6 URI reported from edk2. If you can find it, that will be better.

> cf. https://github.com/openssl/openssl/pull/9201 which is being ignored
> by the OpenSSL developers — OpenSSL really doesn't make life easy for
> you here, which is a shame.
> 
> 
> > For the series patches here, we are intending to support the host
> > name validation, I think we can commit the series patches since we
> > pass the verification of IPV6 URL, what do you think?
> 
> If it passes the verification of IPv6 literals, then all my analysis is
> broken and so was the report on the list that prompted me to start
> looking (or I'm misremembering that report). In that case, sure, go
> ahead and commit.
> 
> > Thanks,
> > Jiaxin

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Wu, Jiaxin]

Hi  Laszlo,

Thanks the comments.

Best Regards!
Jiaxin  

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, October 9, 2019 11:55 PM
> To: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; David Woodhouse <dwmw2@infradead.org>; Bret
> Barkelew <Bret.Barkelew@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName
> validation feature(CVE-2019-14553)
> 
> On 10/01/19 01:21, Laszlo Ersek wrote:
> > On 09/29/19 08:09, Wang, Jian J wrote:
> >> For this patch series,
> >> 1. " Contributed-under: TianoCore Contribution Agreement 1.1" is not
> needed any more.
> >>   Remove it at push time and no need to send a v2.
> >> 2. Since it's security patch which had been reviewed separately, I see no
> reason for new r-b
> >>   required. Please raise it asap if any objections.
> >> 3. Acked-by: Jian J Wang <jian.j.wang@intel.com>
> >
> >
> > * Can you please confirm that these patches match those that we
> > discussed here:
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c18
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c19
> 
> To answer my own question, I've now compared the patches from those BZ
> comments linked above, with the present series. Here's a list of
> differences.
> 
> (1) The subject lines now include the reference "(CVE-2019-14553)".
> 
> This is great, *but* please be sure to insert a space character before
> the opening parenthesis! (In every patch.)
> 
> (2) The commit messages reference both the BZ and the CVE number.
> 
> Good.
> 
> (3) In the commit messages, the line
> 
>   Contributed-under: TianoCore Contribution Agreement 1.0
> 
> has been upgraded to
> 
>   Contributed-under: TianoCore Contribution Agreement 1.1
> 
> I think this is wrong. The lines should have been removed, due to the
> SPDX adoption. Please update all the commit messages.
> 
> (4) Copyright notice updates are gone from the patches.
> 
> That's fine: the reason is that the underlying files have seen their
> copyright notices updated, meanwhile.
> 
> 
> Otherwise, the patches (code, commit messages, and feedback tags) are
> identical.
> 
> Before you push the patches (or post a v2), please fix issues (1) and (3).
> 
> Now, regarding the other set of questions:
> 
> > * In the BZ, David and Bret raised some questions:
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c35
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c36
> >
> > and
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=960#c40
> >
> > The latest comment in the bug is c#41. I'm not under the impression that
> > all concerns raised by David and Bret have been addressed (or
> > abandoned). I'd like David and Bret to ACK the patches.
> 
> I'll first have to process the new comments down-thread.
> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Wu, Jiaxin]

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

Below summary of setup from Laszlo are also the part of our test cases. Beside those verification that only focus on the subject Common Name (CN), we also created the certificate contains Subject Alternative Name (SAN) to cover more test cases related to the subjectAltName. That’s also the parts of hostname verification. With the subjectAltName, we can set the multiple subject names (including IP address format defined in RFC 5280) in one certificate to pass the different URIs (IPv4/IPv6) retrieved from the DHCP server.

> So, my take is that the comparison is done simply on the textual
> representation (with the IPv6 brackets stripped), not the numerical
> value.
> Is that bad? The textual comparison may certainly report a mismatch when
> the numerical values actually match (for an IPv4 example, "192.168.0.1"
> would not match "192.168.000.001"). But that errs in the safe direction,
> does it not?

I think it’s not the problem here by setting textual IP in hostname since the numerical IP is not the target to be vitrificated. And I also post my explain in the previous email:



> UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe),

> not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2:

>    "...TLS session hostname for validation which is used to verify whether the name

>     within the peer certificate matches a given host name..."

> In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real

> FQDN (www.xxx.com<http://www.xxx.com>) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])),

> and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST.

> That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature

> by following the standard TLS HostName verification capability.



If above is incorrect, please correct me.

Anyway, thanks Laszlo and David’s comments.

Jiaxin

From: David Woodhouse <dwmw2@infradead.org>
Sent: Thursday, October 10, 2019 4:34 AM
To: Laszlo Ersek <lersek@redhat.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: Richard Levitte <levitte@openssl.org>
Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

Can you show result of 'openssl x509 -noout -text -in xxxxxx.pem' on your certs please.

Would like to check if you really have a cert for the hostname string "192.168.124.2" or to the IP address. They are different things.

On 9 October 2019 21:24:34 BST, Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:

Hi All,

(multi-hour composition ahead...)

On 10/09/19 09:53, David Woodhouse wrote:

On Tue, 2019-10-08 at 06:19 +0000, Wu, Jiaxin wrote:

 Hi David,

 I just realized you have the comments on Bugzilla 960:

"...given that testing is failing and code inspection shows it
would never have been expected to work."

 Do you mean you didn't pass the verification if URLs with IPv6
 literals (https://[2001:8b0:10b:1236::1]/)?  Can you also show me
 where the code inspection indicated it would never have been expected
 to work? We do pass the testing for the URLs with IPv6 if the CN or
 SAN in certificate has the corresponding IPv6 address (at least
 working with openssl 1.1.0).

 I have not tested this, but I started looking when there was a message
 on the edk2 list from someone who was reporting that it didn't work
 for IPv6 URIs, IIRC.

 You are using SSL_set1_host(), and I believe you're just passing in
 the bare hostname part of the URI, be it "1.2.3.4" or
 "[2001:8b0:10b::5]".

 That just adds it to the 'hosts' list in the X509_VERIFY_PARAM for the
 SSL connection.

 In the check_hosts() function in openssl/crypto/x509/v509_vfy.c, the
 code simply iterates over the members of that list, calling
 X509_check_host() for each one. It never calls X509_check_ip().

 If you look in openssl/crypto/x509/v3_utl.c you can see the
 X509_check_host() really does only check hostnames. You'd need to call
 X509_check_ip_asc() to check hostnames. And something would need to
 have stripped the [] which surround an IPv6 literal.

 I can't see how this can work. Have you tested it since the report on
 the list that it wasn't working?

 cf. https://github.com/openssl/openssl/pull/9201 which is being
 ignored by the OpenSSL developers \x14 OpenSSL really doesn't make
 life easy for you here, which is a shame.


For the series patches here, we are intending to support the host
name validation, I think we can commit the series patches since we
pass the verification of IPV6 URL, what do you think?

 If it passes the verification of IPv6 literals, then all my analysis
 is broken and so was the report on the list that prompted me to start
 looking (or I'm misremembering that report). In that case, sure, go
 ahead and commit.

Here's a summary of my setup.

* I've generated a brand new CA certificate, and two HTTP server
  certificates, signed by the CA.

* One HTTP server certificate is for Common Name = 192.168.124.2

* The other HTTP server certificate is for Common Name =
  fd33:eb1b:9b36::2

* I have a "net-server" virtual machine that runs Apache on the above IP
  addresses (TCP port 443).

  - This virtual machine also runs DHCP (v4) and DHCP (v6) daemons.

  - The DHCP servers send the following boot file names:

    - "https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso"       [IPv4]
    - "https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso" [IPv6]

* For sanity-checking the environment, I run the following two commands
  on the *host* (connecting to the "net-server" virtual machine):

  - curl           -I 'https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso'
  - curl --globoff -I 'https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso'

  - The host is configured to trust the brand new test CA certificate
    (see near the top).

  - When the certificates are assigned *correctly* to the IP addresses
    in the Apache configuration, the above "curl" commands complete just
    fine. If I add the "-v" option to "curl", it confirms the right
    certificates are used, and it confirms the test CA as issuer too.

  - When the certificates are (intentionally) *cross-assigned* to the IP
    addresses in the Apache configuration, then both "curl" commands
    break with the following error message:

curl: (51) Unable to communicate securely with peer: requested domain
name does not match the server's certificate.

  - If I add the "-v" option, I also see

NSS error -12276 (SSL_ERROR_BAD_CERT_DOMAIN)

  - As a side comment: Apache itself warns about the misconfig, in
    "/var/log/httpd/ssl_error_log":

... [ssl:warn] ... AH01909: RSA certificate configured for ...:443
does NOT include an ID which matches the server name

* I have a "net-client" virtual machine, running OVMF.

  - The edk2 HTTPS/TLS client booting in this virtual machine is
    configured to trust the exact same set of CA certificates that the
    host trusts too.

  - In other words, HTTPS boot in the "net-client" VM accepts server
    certificates signed by the new test CA.

* The following is the test plan.

1. The patch set is *not* applied (that is, OVMF is built at current
   master, commit 976d0353a6ce).

  1. Properly assigned certificates:

    1. HTTPSv4 boot --> expect success (correct behavior, establishes
                                        baseline)

    2. HTTPSv6 boot --> expect success (correct behavior, establishes
                                        baseline)

  2. Cross-assigned certificates:

    1. HTTPSv4 boot --> expect success (for reproducing the bug)

    2. HTTPSv6 boot --> expect success (for reproducing the bug)

2. With the patch set applied:

  1. Properly assigned certificates:

    1. HTTPSv4 boot --> expect success (failure means a regression)

    2. HTTPSv6 boot --> expect success (failure means a regression)

  2. Cross-assigned certificates:

    1. HTTPSv4 boot --> expect failure (for verifying the bugfix)

    2. HTTPSv6 boot --> expect failure (for verifying the bugfix)

* Results:

- 1.1.1. as expected (HTTPSv4 baseline established)
- 1.1.2. as expected (HTTPSv6 baseline established)
- 1.2.1. as expected (HTTPSv4 MITM bug reproduced)
- 1.2.2. as expected (HTTPSv6 MITM bug reproduced)
- 2.1.1. as expected (HTTPSv4 not regressed by series)
- 2.1.2. as expected (HTTPSv6 not regressed by series)
- 2.2.1. as expected (HTTPSv4 MITM averted)
- 2.2.2. as expected (HTTPSv6 MITM averted)

* In cases 2.2.1. and 2.2.2.:

- The UEFI console contains, respectively:

Start HTTP Boot over IPv4....

   Station IP address is 192.168.124.106

   URI: https://192.168.124.2/RHEL-7.4-20170711.0-Server-x86_64-boot.iso

   Error: Could not retrieve NBP file size from HTTP server.

   Error: Unexpected network error.



Start HTTP Boot over IPv6....

   Station IPv6 address is FD33:EB1B:9B36:0:0:0:0:C8

   URI: https://[fd33:eb1b:9b36::2]/RHEL-7.4-20170711.0-Server-x86_64-boot.iso

   Error: Could not retrieve NBP file size from HTTP server.

   Error: Unexpected network error.

- The OVMF log contains (in both cases):

TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL
TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86

- Decoding:

  - Library   0x14 -> ERR_LIB_SSL
  - Function 0x16F -> SSL_F_TLS_PROCESS_SERVER_CERTIFICATE
  - Reason    0x86 -> SSL_R_CERTIFICATE_VERIFY_FAILED

  - So this means that the ssl_verify_cert_chain() call fails in the
    tls_process_server_certificate() function, in
    "CryptoPkg/Library/OpensslLib/openssl/ssl/statem/statem_clnt.c".


Normally the above would be sufficient for me to give a "Tested-by" for
this patch set.

But now I'm uncertain whether (a) my results contradict David's
analysis, or (b) I tested something that David's analysis doesn't
*apply* to. (IOW if my test plan doesn't actually verify "IPv6
literals".)


FWIW, the brackets in the IPv6 notation are stripped in EfiHttpRequest()
[NetworkPkg/HttpDxe/HttpImpl.c], using the "HostName" local variable.
(The stripping comes from earlier commit 7191827f90b4
("NetworkPkg/HttpDxe: Strip square brackets in IPv6 expressed
HostName.", 2018-08-03).) Later in EfiHttpRequest(),
"HttpInstance->RemoteHost" is assigned "HostName".

Further, in TlsConfigureSession() [NetworkPkg/HttpDxe/HttpsSupport.c],
we set "HttpInstance->TlsConfigData.VerifyHost.HostName" to
"HttpInstance->RemoteHost". This is done in patch#4.

Also in patch#4, in the same function, we pass
"HttpInstance->TlsConfigData.VerifyHost" to SetSessionData(), from
patch#3.

There we pass "TlsVerifyHost->HostName" to TlsSetVerifyHost(), which
resides in patch#2.

At that point, we pass the hostname -- the IPv6 address, with the
brackets stripped -- to SSL_set1_host().

So, my take is that the comparison is done simply on the textual
representation (with the IPv6 brackets stripped), not the numerical
value.

Is that bad? The textual comparison may certainly report a mismatch when
the numerical values actually match (for an IPv4 example, "192.168.0.1"
would not match "192.168.000.001"). But that errs in the safe direction,
does it not?

Thanks!
Laszlo

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/09/19 22:34, David Woodhouse wrote:
> Can you show result of 'openssl x509 -noout -text -in xxxxxx.pem' on
> your certs please.

Sure. I had thought of that actually (I could have attached the
certificates at once), but I figured, let me not share crypto stuff
unless specifically asked for :)

> Would like to check if you really have a cert for the hostname string
> "192.168.124.2" or to the IP address. They are different things.

Very interesting! This makes me curious.

The *host* certificates were generated with the "genkey" utility ("Red
Hat Keypair Generation"), not with naked openssl tool invocations.
That's because Apache setup is not for the faint-hearted, and I had
decided up-front (when I first looked into setting up mod_ssl) that I'd
follow the official documentation, here:

  https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/7/html/system_administrators_guide/ch-web_servers

So I ran

  genkey 'fd33:eb1b:9b36::2'
  genkey 192.168.124.2

and went through the dialogs, which were already filled in with the
argument passed on the command line (which the genkey manual calls
"hostname").

The *CA* cert was generated with openssl directly, however. Also, for
signing the CSRs (certificate signing requests), produced by "genkey", I
also used openssl manually.

Please find the certificates below (including the CA certificate):

> Certificate:
>     Data:
>         Version: 3 (0x2)
>         Serial Number:
>             a7:b5:04:75:6a:2f:ee:7e
>     Signature Algorithm: sha256WithRSAEncryption
>         Issuer: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=lersek@redhat.com
>         Validity
>             Not Before: Oct  9 16:06:08 2019 GMT
>             Not After : Nov  8 16:06:08 2019 GMT
>         Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=lersek@redhat.com
>         Subject Public Key Info:
>             Public Key Algorithm: rsaEncryption
>                 Public-Key: (2048 bit)
>                 Modulus:
>                     00:d4:db:3b:fa:98:bd:15:02:3a:32:ea:64:d5:1d:
>                     d8:80:03:fa:fb:4e:d8:47:45:3b:57:6a:36:80:83:
>                     e1:6d:c4:0b:f7:24:00:ad:54:63:77:dd:86:71:f3:
>                     fc:f4:e5:81:d4:6c:7d:23:b9:58:9e:cc:93:ee:93:
>                     ed:24:62:8b:94:8f:de:3a:6a:a9:cd:1a:38:f0:df:
>                     17:91:7a:22:ca:35:94:3a:1f:cb:56:97:be:bb:69:
>                     1f:12:3a:7c:a4:35:6c:15:0e:27:a0:0a:2b:3d:61:
>                     bd:ed:a6:84:29:bc:0a:d8:0c:98:4c:e9:7d:6b:36:
>                     da:29:1d:49:57:8d:89:2a:62:e7:4f:00:56:46:9b:
>                     a7:21:9f:b6:83:c7:dd:69:b0:8b:a3:bf:fa:ef:50:
>                     a4:05:6f:b1:87:83:87:93:c7:ba:0e:cb:50:28:05:
>                     f5:a7:0a:fe:be:13:71:1a:4b:6c:7b:f6:c9:cc:b2:
>                     65:cd:62:29:e3:08:c7:da:5c:ca:4d:dd:74:a4:d1:
>                     37:52:ea:ad:72:87:cf:48:f3:85:af:15:ab:e8:dc:
>                     50:f2:f7:7a:59:ed:b7:69:4f:a2:55:39:e5:ae:09:
>                     75:45:69:a5:3a:a0:cd:52:ee:f2:15:d5:3c:fc:c0:
>                     00:b7:25:71:ba:00:ba:63:08:8a:fc:b6:af:94:a4:
>                     5b:cb
>                 Exponent: 65537 (0x10001)
>         X509v3 extensions:
>             X509v3 Subject Key Identifier:
>                 2F:A1:8A:8D:60:DC:74:D8:3C:46:B2:57:6C:E9:81:34:B1:72:82:F5
>             X509v3 Authority Key Identifier:
>                 keyid:2F:A1:8A:8D:60:DC:74:D8:3C:46:B2:57:6C:E9:81:34:B1:72:82:F5
>
>             X509v3 Basic Constraints:
>                 CA:TRUE
>     Signature Algorithm: sha256WithRSAEncryption
>          5f:fb:ca:7c:52:c6:81:f3:3f:48:02:c6:31:64:dc:a5:2f:a6:
>          83:ef:b2:9d:68:7f:1a:e6:1d:8d:e0:50:08:ed:96:4f:56:8a:
>          eb:cd:3a:ac:74:f6:e4:68:55:0d:73:b3:bf:45:cd:35:e0:4b:
>          70:bf:25:30:75:62:e1:5a:53:00:ff:ca:3c:c0:86:ad:44:73:
>          5b:64:d8:85:ea:32:38:3b:4b:60:85:95:e5:10:f3:92:19:0e:
>          55:67:26:c5:56:50:92:8c:d2:33:5b:5f:c6:27:c1:9a:6a:a5:
>          ad:1b:21:04:47:ce:94:f0:2a:38:26:43:5f:9b:c0:f5:33:80:
>          59:72:33:e8:06:89:e1:7e:44:5c:cb:67:fa:f4:de:27:94:9c:
>          44:1d:81:40:6f:46:bc:2f:93:89:30:48:99:bc:53:29:44:a7:
>          e1:9d:9c:05:98:14:3d:ab:e2:1c:3e:91:83:4a:74:9d:ed:14:
>          d4:86:2d:c0:4e:4e:20:fc:29:31:60:b9:63:50:23:52:b1:2f:
>          9f:50:b4:d7:23:3e:08:c2:b3:bf:d3:b6:84:16:6d:71:fe:2a:
>          b0:71:f0:94:67:84:1d:45:8d:22:3d:4a:f4:65:73:0c:81:8a:
>          4e:b7:52:b8:21:ab:ce:8d:50:e0:22:af:4f:2e:30:4f:95:8c:
>          9c:26:0d:fe

> Certificate:
>     Data:
>         Version: 1 (0x0)
>         Serial Number:
>             d1:ea:de:57:cb:4b:44:7b
>     Signature Algorithm: sha256WithRSAEncryption
>         Issuer: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=lersek@redhat.com
>         Validity
>             Not Before: Oct  9 16:22:49 2019 GMT
>             Not After : Nov  8 16:22:49 2019 GMT
>         Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=IPv6 cert, CN=fd33:eb1b:9b36::2
>         Subject Public Key Info:
>             Public Key Algorithm: rsaEncryption
>                 Public-Key: (2048 bit)
>                 Modulus:
>                     00:d5:94:80:83:2c:fa:35:20:89:2c:b5:e8:f4:48:
>                     9a:f7:77:77:5c:9e:f4:cc:a7:f5:c7:d3:e8:29:0c:
>                     95:36:9f:ca:5c:dd:c0:d1:f5:14:14:eb:89:19:cc:
>                     8c:0e:35:d2:02:02:b5:56:d1:56:c4:f1:61:6c:cd:
>                     ba:21:6d:27:e8:77:db:ec:12:c8:f0:c0:a9:97:2c:
>                     77:00:71:2d:36:b5:e7:08:10:d3:28:4b:96:ce:a1:
>                     4a:a6:ca:ca:9a:11:56:61:21:f2:2a:45:8c:02:a9:
>                     26:5d:46:d3:11:dd:35:b9:d8:19:26:cf:63:cf:d0:
>                     f6:04:4f:07:24:27:e3:91:8b:9b:4b:01:61:ab:0a:
>                     4b:c9:c7:04:36:99:9b:94:e8:56:be:b8:14:08:73:
>                     d4:f6:c1:0b:7f:10:20:bd:89:79:e5:9c:24:9f:03:
>                     d2:b8:6b:20:b5:d1:dc:fe:ce:0d:8d:2b:a5:9b:a5:
>                     26:c4:85:90:4f:b9:e9:39:7b:c6:d7:9a:8a:e1:9f:
>                     47:93:db:dd:bc:d6:56:6c:16:10:b7:08:49:6b:38:
>                     7b:43:55:18:b3:e4:d4:69:94:df:51:21:7f:da:02:
>                     79:0f:da:65:b0:11:25:70:93:99:9d:a2:ab:8c:c9:
>                     fb:ca:e2:7e:5f:b8:90:65:dd:fc:54:a7:ca:de:e3:
>                     6b:57
>                 Exponent: 65537 (0x10001)
>     Signature Algorithm: sha256WithRSAEncryption
>          4d:e8:e4:17:02:74:af:a7:19:99:de:4b:44:e7:c1:7e:78:99:
>          b2:e3:8f:a6:59:67:c7:b7:df:83:b9:2a:aa:aa:01:65:2d:6f:
>          0a:87:ec:49:31:01:9a:03:fd:66:36:37:cf:9a:d7:59:4f:ef:
>          a4:d8:c1:9d:55:b2:76:c4:1e:d7:98:3d:4e:12:25:1a:0a:51:
>          40:e3:a0:45:3c:37:ad:f0:f8:c3:af:a1:52:47:97:77:ef:3f:
>          ad:d0:56:97:e0:83:c5:e9:f0:d5:e5:74:57:fe:b2:85:86:e7:
>          cd:e9:36:c0:54:cf:aa:35:4d:b6:81:42:6f:3a:9c:c4:5b:84:
>          31:fc:3c:d6:42:1c:07:2d:62:59:ff:0b:f4:c4:56:e0:2c:bf:
>          41:f4:63:40:05:c3:27:08:6c:a0:04:4c:c2:d2:8d:23:a0:30:
>          7c:c2:58:92:ad:14:25:e3:39:3f:53:37:1d:3e:20:57:94:3f:
>          c9:64:d9:a3:11:d9:41:52:2e:28:00:49:72:0e:0c:96:89:02:
>          1c:10:7e:cf:81:61:6c:54:97:40:f2:06:98:96:51:da:62:3a:
>          c6:10:fb:3a:4c:47:65:ea:e7:86:b2:bb:94:3d:de:93:ff:72:
>          b5:29:a7:53:ec:32:f5:dd:7d:09:18:70:89:58:b9:e9:41:8c:
>          ca:0e:2a:5b

> Certificate:
>     Data:
>         Version: 1 (0x0)
>         Serial Number:
>             d1:ea:de:57:cb:4b:44:7a
>     Signature Algorithm: sha256WithRSAEncryption
>         Issuer: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=Certificate Authority, CN=Laszlo Ersek CA/emailAddress=lersek@redhat.com
>         Validity
>             Not Before: Oct  9 16:20:46 2019 GMT
>             Not After : Nov  8 16:20:46 2019 GMT
>         Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=IPv4 cert, CN=192.168.124.2
>         Subject Public Key Info:
>             Public Key Algorithm: rsaEncryption
>                 Public-Key: (2048 bit)
>                 Modulus:
>                     00:c8:e8:5a:b7:57:de:8c:5f:e9:71:8b:ca:4f:41:
>                     9c:6b:33:c7:d7:45:ab:41:6e:b0:77:00:bd:a6:39:
>                     ab:e0:fb:23:05:e6:69:c4:56:9b:12:ec:46:fb:86:
>                     75:7e:11:66:c7:8f:f4:db:ce:e7:ce:70:56:26:3c:
>                     e1:d9:81:6e:44:cd:56:90:9f:20:fd:ed:a7:be:ad:
>                     8a:7b:c3:98:3e:c9:ba:ec:eb:79:4d:a2:1c:93:89:
>                     89:0a:1a:4e:7b:31:c5:2f:cd:cc:90:d6:07:22:56:
>                     9c:e8:e9:51:65:18:e2:31:05:cd:49:52:54:8c:a1:
>                     1b:51:fa:8a:fa:66:9e:39:29:88:1a:e1:c5:31:ea:
>                     16:25:4a:95:47:4c:53:d1:20:dd:2c:29:32:e5:d8:
>                     0b:32:2d:52:c5:32:68:f7:7a:cf:15:d3:4f:95:03:
>                     c9:0b:8b:46:b9:c8:e4:46:77:01:08:d7:ad:c3:0a:
>                     15:e2:70:62:a1:6f:d8:08:f0:60:9f:ee:34:f6:95:
>                     5c:f3:c2:e0:e2:18:ec:ff:4c:74:9e:92:5a:8f:8b:
>                     be:38:f0:99:44:0f:95:34:a6:cb:f7:5d:59:72:30:
>                     58:f7:0c:3b:5a:23:d4:bc:26:71:47:7f:08:f0:fa:
>                     11:79:55:56:ed:bf:da:4f:df:55:d6:87:91:13:9e:
>                     54:8d
>                 Exponent: 65537 (0x10001)
>     Signature Algorithm: sha256WithRSAEncryption
>          a2:f7:e8:7c:28:be:66:97:73:80:9a:38:4f:58:de:44:6f:10:
>          97:b1:e6:d4:1b:00:e8:49:77:09:3f:b2:34:0c:cc:93:42:cc:
>          ff:7e:97:51:7e:ce:3d:f6:9f:e8:22:e5:d5:da:e5:db:f9:96:
>          4c:07:11:0b:ff:57:07:98:89:df:27:9d:30:f5:e3:6d:19:d0:
>          b4:50:36:63:63:a9:9c:e1:a9:80:14:c7:07:9b:f0:d1:58:a8:
>          00:3d:34:0c:7d:25:2f:a1:0c:4a:b8:98:61:9d:c8:8e:83:b0:
>          e2:b1:06:61:62:5d:67:28:32:f4:88:18:4a:13:52:55:ef:75:
>          23:f2:6b:ce:d2:64:f7:07:c4:1f:50:6e:f4:1f:1f:d5:0f:fe:
>          fa:33:62:e5:af:0a:1d:19:fb:e0:73:82:82:ed:6e:5b:21:81:
>          5e:62:b8:30:8a:20:f9:4f:a8:5b:f2:55:c2:68:92:ad:d8:b1:
>          ae:ea:a0:d5:39:60:fd:f2:5f:23:62:43:de:2e:b6:3e:11:ac:
>          12:7b:94:e3:b9:84:6c:2f:8e:04:8a:2b:af:5a:08:f4:7b:dc:
>          61:b4:9a:19:00:d5:fb:14:33:52:4c:49:c2:18:cd:73:8e:e8:
>          d1:45:9b:ba:57:6b:5a:db:31:d7:70:13:d5:a8:26:5c:77:88:
>          ba:be:09:d3

Futhermore, my Apache config is, for the "proper" certificate assignment
case (excerpt from "/etc/httpd/conf.d/ssl.conf"):

> <VirtualHost [fd33:eb1b:9b36::2]:443>
> SSLProtocol -all +TLSv1 +TLSv1.1 +TLSv1.2
> SSLCipherSuite HIGH:3DES:!aNULL:!MD5:!SEED:!IDEA
> SSLCertificateFile /etc/pki/tls/certs/fd33:eb1b:9b36::2.signedcert.pem
> SSLCertificateKeyFile /etc/pki/tls/private/fd33:eb1b:9b36::2.key

> <VirtualHost _default_:443>
> SSLProtocol -all +TLSv1 +TLSv1.1 +TLSv1.2
> SSLCipherSuite HIGH:3DES:!aNULL:!MD5:!SEED:!IDEA
> SSLCertificateFile /etc/pki/tls/certs/192.168.124.2.signedcert.pem
> SSLCertificateKeyFile /etc/pki/tls/private/192.168.124.2.key

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote:
> >          Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=IPv6 cert, CN=fd33:eb1b:9b36::2

Yeah, you're not actually testing the case I'm talking about. You want
a GEN_IP in the x509v3 Subject Alternative Name.

Compare with...

$ openssl s_client  -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl x509 -noout -text  | grep -A1 Alternative
            X509v3 Subject Alternative Name: 
                DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP Address:134.191.232.101

$ curl https://134.191.232.101/


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/10/19 17:45, David Woodhouse wrote:
> On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote:
>>>          Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office, OU=IPv6 cert, CN=fd33:eb1b:9b36::2
> 
> Yeah, you're not actually testing the case I'm talking about. You want
> a GEN_IP in the x509v3 Subject Alternative Name.
> 
> Compare with...
> 
> $ openssl s_client  -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl x509 -noout -text  | grep -A1 Alternative
>             X509v3 Subject Alternative Name: 
>                 DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP Address:134.191.232.101
> 
> $ curl https://134.191.232.101/
> 

OK, thank you.

I can imagine two failure modes, with the patches applied.

(1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server
certificate.

(2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid
(mismatched) server certificate.

Can we tell which failure mode applies?

(I can't test it easily myself, as I don't even know how to create a
server certificate with a SAN -- any kind of SAN, let alone GEN_IP.)

Case (2) is clearly bad, and it would be a sign that the patch series
does not fully fix the issue.

Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is
pretty rare in practice. Thus regressing it (perhaps temporarily) should
be an acceptable trade-off for fixing the current gaping hole (= subject
name not checked at all).

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Wu, Jiaxin]

Hi Laszlo & David,

I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so,  the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting).

IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc().

Post my explain again: 
> UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), 
> not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2:     
>    "...TLS session hostname for validation which is used to verify whether the name 
>     within the peer certificate matches a given host name..." 
> In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real 
> FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), 
> and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. 
> That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature 
> by following the standard TLS HostName verification capability.

Now, let's talking the iPAddress setting in SAN (same as email address),  if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here.

Thanks,
Jiaxin 





> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, October 11, 2019 2:04 AM
> To: David Woodhouse <dwmw2@infradead.org>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; devel@edk2.groups.io; Wang, Jian J
> <jian.j.wang@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: Richard Levitte <levitte@openssl.org>
> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName
> validation feature(CVE-2019-14553)
> 
> On 10/10/19 17:45, David Woodhouse wrote:
> > On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote:
> >>>          Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office,
> OU=IPv6 cert, CN=fd33:eb1b:9b36::2
> >
> > Yeah, you're not actually testing the case I'm talking about. You want
> > a GEN_IP in the x509v3 Subject Alternative Name.
> >
> > Compare with...
> >
> > $ openssl s_client  -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl
> x509 -noout -text  | grep -A1 Alternative
> >             X509v3 Subject Alternative Name:
> >                 DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP
> Address:134.191.232.101
> >
> > $ curl https://134.191.232.101/
> >
> 
> OK, thank you.
> 
> I can imagine two failure modes, with the patches applied.
> 
> (1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server
> certificate.
> 
> (2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid
> (mismatched) server certificate.
> 
> Can we tell which failure mode applies?
> 
> (I can't test it easily myself, as I don't even know how to create a
> server certificate with a SAN -- any kind of SAN, let alone GEN_IP.)
> 
> Case (2) is clearly bad, and it would be a sign that the patch series
> does not fully fix the issue.
> 
> Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is
> pretty rare in practice. Thus regressing it (perhaps temporarily) should
> be an acceptable trade-off for fixing the current gaping hole (= subject
> name not checked at all).
> 
> Thanks
> Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Fri, 2019-10-11 at 02:24 +0000, Wu, Jiaxin wrote:
> Hi Laszlo & David,
> 
> I think I have *repeated* several times that we are targeting to fix
> the HostName validation issue, not the IP or email address. *But*
> even so,  the series patches for UEFI TLS is also allowable to
> specify IP as host name for CN or dNSName of SAN in the certificate.
> That's why I said "if the CN or SAN in the certificate are set
> correctly, it should be OK to pass the verification". The failure you
> mentioned here is to set the IP in iPAddress of SAN, I agree it's the
> routine and suggested setting, *but* obviously, it's not the target
> we are supported according the implementation/description of
> TlsSetVerifyHost. We are targeting to the hostname verification, and
> meanwhile compatible with the IP in the URI (But need the *correct*
> certificate setting).
> 
> IP addresses stored in the DNS names and CN are of cause ignored by
> X509_check_ip & X509_check_ip_asc().

I cannot coherently express how disappointed I am by this response.

The current state is that EDK2 doesn't check the subject of the
certificate at all.

We're trying to fix that, and you have expended more effort typing in
poor excuses for doing an incomplete job, than the typing it would have
taken just to get it right in the first place.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Wu, Jiaxin]

I'm surprising my detailed and patient explanation become a poor excuses! If you think there is anything wrong with my explanation, please correct me instead of blaming directly. 

> > I think I have *repeated* several times that we are targeting to fix
> > the HostName validation issue, not the IP or email address. *But*
> > even so,  the series patches for UEFI TLS is also allowable to
> > specify IP as host name for CN or dNSName of SAN in the certificate.
> > That's why I said "if the CN or SAN in the certificate are set
> > correctly, it should be OK to pass the verification". The failure you
> > mentioned here is to set the IP in iPAddress of SAN, I agree it's the
> > routine and suggested setting, *but* obviously, it's not the target
> > we are supported according the implementation/description of
> > TlsSetVerifyHost. We are targeting to the hostname verification, and
> > meanwhile compatible with the IP in the URI (But need the *correct*
> > certificate setting).
> >
> > IP addresses stored in the DNS names and CN are of cause ignored by
> > X509_check_ip & X509_check_ip_asc().
> 
> I cannot coherently express how disappointed I am by this response.
> 
> The current state is that EDK2 doesn't check the subject of the
> certificate at all.

Highlight again: we do check the certificate peername in SAN & Subject CommonName (CN) instead of nothing.

> 
> We're trying to fix that, and you have expended more effort typing in
> poor excuses for doing an incomplete job, than the typing it would have
> taken just to get it right in the first place.

My typing is only poor excuses? I'm trying my best to explain the patch intention. I said in the previous email, "We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI". I also agree your suggestion & requires is reasonable & meaning to support the IP check in the certificate. So,  my friendly advice is to separate the issues you raised instead of mixing them up.


Thanks,
Jiaxin 

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/11/19 04:24, Wu, Jiaxin wrote:
> Hi Laszlo & David,
> 
> I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so,  the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting).
> 
> IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc().
> 
> Post my explain again: 
>> UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), 
>> not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2:     
>>    "...TLS session hostname for validation which is used to verify whether the name 
>>     within the peer certificate matches a given host name..." 
>> In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real 
>> FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), 
>> and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. 
>> That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature 
>> by following the standard TLS HostName verification capability.
> 
> Now, let's talking the iPAddress setting in SAN (same as email address),  if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here.

(To clarify my stance.)

Given the above statement of scope, and given the test env details that
I included in my previous message:

series
Tested-by: Laszlo Ersek <lersek@redhat.com>

I understand that my testing does not cover the use case described by David.

So my Tested-by is certainly *not* intended as the "last word" in this
thread, or anything like that. I'm only saying this patch set is good
enough for me, not that everyone should find it good enough for them.

Thanks
Laszlo


>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Friday, October 11, 2019 2:04 AM
>> To: David Woodhouse <dwmw2@infradead.org>; Wu, Jiaxin
>> <jiaxin.wu@intel.com>; devel@edk2.groups.io; Wang, Jian J
>> <jian.j.wang@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>
>> Cc: Richard Levitte <levitte@openssl.org>
>> Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName
>> validation feature(CVE-2019-14553)
>>
>> On 10/10/19 17:45, David Woodhouse wrote:
>>> On Thu, 2019-10-10 at 10:00 +0200, Laszlo Ersek wrote:
>>>>>          Subject: C=HU, ST=Pest, L=Budapest, O=Laszlo Ersek Home Office,
>> OU=IPv6 cert, CN=fd33:eb1b:9b36::2
>>>
>>> Yeah, you're not actually testing the case I'm talking about. You want
>>> a GEN_IP in the x509v3 Subject Alternative Name.
>>>
>>> Compare with...
>>>
>>> $ openssl s_client  -connect vpn-i-ha01.intel.com:443 2>/dev/null | openssl
>> x509 -noout -text  | grep -A1 Alternative
>>>             X509v3 Subject Alternative Name:
>>>                 DNS:vpn-int.intel.com, DNS:scsidcint01-a.intel.com, IP
>> Address:134.191.232.101
>>>
>>> $ curl https://134.191.232.101/
>>>
>>
>> OK, thank you.
>>
>> I can imagine two failure modes, with the patches applied.
>>
>> (1) Edk2 ignores the GEN_IP in the SAN, and rejects a matching server
>> certificate.
>>
>> (2) Edk2 is confused by the GEN_IP in the SAN, and accepts an invalid
>> (mismatched) server certificate.
>>
>> Can we tell which failure mode applies?
>>
>> (I can't test it easily myself, as I don't even know how to create a
>> server certificate with a SAN -- any kind of SAN, let alone GEN_IP.)
>>
>> Case (2) is clearly bad, and it would be a sign that the patch series
>> does not fully fix the issue.
>>
>> Case (1) would be tolerable, in my opinion. I assume a GEN_IP SAN is
>> pretty rare in practice. Thus regressing it (perhaps temporarily) should
>> be an acceptable trade-off for fixing the current gaping hole (= subject
>> name not checked at all).
>>
>> Thanks
>> Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Fri, 2019-10-11 at 12:55 +0200, Laszlo Ersek wrote:
> On 10/11/19 04:24, Wu, Jiaxin wrote:
> > Hi Laszlo & David,
> > 
> > I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so,  the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting).
> > 
> > IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc().
> > 
> > Post my explain again: 
> > > UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), 
> > > not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2:     
> > >     "...TLS session hostname for validation which is used to verify whether the name 
> > >      within the peer certificate matches a given host name..." 
> > > In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real 
> > > FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), 
> > > and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. 
> > > That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature 
> > > by following the standard TLS HostName verification capability.
> > 
> > Now, let's talking the iPAddress setting in SAN (same as email address),  if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here.
> 
> (To clarify my stance.)
> 
> Given the above statement of scope, and given the test env details that
> I included in my previous message:
> 
> series
> Tested-by: Laszlo Ersek <lersek@redhat.com>
> 
> I understand that my testing does not cover the use case described by David.
> 
> So my Tested-by is certainly *not* intended as the "last word" in this
> thread, or anything like that. I'm only saying this patch set is good
> enough for me, not that everyone should find it good enough for them.


As you wish.

Note for the record that that this is a functional regression. We go
from accepting all certs regardless of the target/subject, to rejecting
some valid certificates. I first started looking at this when it was
reported as such, on the list.

Given the length of time it's taken to fix this CVE already, I
understand that Laszlo is keen to get *something* committed, at last.

But I stand by the assertion that this could be done properly, without
the regression, with much less additional typing even than we've
already done in this thread the last couple of days. It just isn't that
hard.

Ultimately, though, do whatever you like. I am not the final arbiter of
engineering sanity and common sense for the EDK2 project.

If I were, the world would be a very different place.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/11/19 13:16, David Woodhouse wrote:
> On Fri, 2019-10-11 at 12:55 +0200, Laszlo Ersek wrote:
>> On 10/11/19 04:24, Wu, Jiaxin wrote:
>>> Hi Laszlo & David,
>>>
>>> I think I have *repeated* several times that we are targeting to fix the HostName validation issue, not the IP or email address. *But* even so,  the series patches for UEFI TLS is also allowable to specify IP as host name for CN or dNSName of SAN in the certificate. That's why I said "if the CN or SAN in the certificate are set correctly, it should be OK to pass the verification". The failure you mentioned here is to set the IP in iPAddress of SAN, I agree it's the routine and suggested setting, *but* obviously, it's not the target we are supported according the implementation/description of TlsSetVerifyHost. We are targeting to the hostname verification, and meanwhile compatible with the IP in the URI (But need the *correct* certificate setting).
>>>
>>> IP addresses stored in the DNS names and CN are of cause ignored by X509_check_ip & X509_check_ip_asc().
>>>
>>> Post my explain again: 
>>>> UEFI TLS only provides the *HostName* verification interface to upper driver (HttpDxe), 
>>>> not the IP/email verification capability. Please refer to UEFI Spec2.8 section 28.10.2:     
>>>>     "...TLS session hostname for validation which is used to verify whether the name 
>>>>      within the peer certificate matches a given host name..." 
>>>> In upper UEFI HTTP driver, we get the hostname from URI directly no matter it's the real 
>>>> FQDN (www.xxx.com) or IP address format string (1.2.3.4 or 2001:8b0:10b::5 (not "[2001:8b0:10b::5])), 
>>>> and set it to the TLS hostname filed via the interface -- EFI_TLS_VERIFY_HOST. 
>>>> That's implementation choice for HttpDxe to achieve the HTTPS HostName validation feature 
>>>> by following the standard TLS HostName verification capability.
>>>
>>> Now, let's talking the iPAddress setting in SAN (same as email address),  if you do need such feature that verify the IP in X509_check_ip & X509_check_ip_asc , please report new Bugzilla (need TLS Spec update the expose the setting interface), don't mix up the HTTPS hostname verification here.
>>
>> (To clarify my stance.)
>>
>> Given the above statement of scope, and given the test env details that
>> I included in my previous message:
>>
>> series
>> Tested-by: Laszlo Ersek <lersek@redhat.com>
>>
>> I understand that my testing does not cover the use case described by David.
>>
>> So my Tested-by is certainly *not* intended as the "last word" in this
>> thread, or anything like that. I'm only saying this patch set is good
>> enough for me, not that everyone should find it good enough for them.
> 
> 
> As you wish.
> 
> Note for the record that that this is a functional regression. We go
> from accepting all certs regardless of the target/subject, to rejecting
> some valid certificates.

Indeed that is the case; I'm aware of it. Thanks for pointing it out.

This functional regression does not translate to an actual end-user
regression, under my circumstances. From my perspective, UEFI HTTPS boot
is a feature that cannot be enabled *at all* in a production
environment, due to the possible MITM. In other words, I cannot build
OVMF with

  -D NETWORK_HTTP_BOOT_ENABLE -D NETWORK_TLS_ENABLE

and give the binary to users with a clear conscience. They'd see UEFI
HTTPS boot, say "hey this is secure!", and use it. That would be a false
sense of security.

With the patches applied, yes, some valid certificates would be
rejected. That would be a normal bug report, or a known limitation, or
even a "layered" feature request. Not a security bug. It would not
prevent the basic -- or let's say, "somewhat restricted" -- enablement
of UEFI HTTPS boot. Not a deal-breaker.

It would be possible for us to announce, "and now we're giving you UEFI
HTTPS boot that is secure to the best of our knowledge, only it does not
*yet* support GEN_IP in SAN. Stay tuned for that."

This would not be a regression, from an end-user perspective.

Again, I'm not attempting to "override" or "suppress" your (implied)
NACK. If the agreement turns out to be that we should *first* add
support for GEN_IP in the SAN, I will not campaign for merging the v1
patches *upstream*, right now. I respect your feedback. No patch should
be merged while there are outstanding questions, let alone well-founded
NACKs.


> I first started looking at this when it was
> reported as such, on the list.

I believe you. Can you somehow find that thread? I tried, but I couldn't
find it. My mailbox (going back 9 years) is indexed, but my searches
have failed. I must be using the wrong search terms. If I try "GEN_IP"
or "Subject Alternative Name", I only get this thread.


> Given the length of time it's taken to fix this CVE already, I
> understand that Laszlo is keen to get *something* committed, at last.

More precisely, I'd be happy to see patches committed that plug the
security hole. Which has been *completely* preventing us from enabling
this feature.

I agree that, while working towards a feature or a bugfix, already
functional things should *not* be regressed -- even temporarily, even if
the regression lasts only from patch#n to patch#(n+1) in a single
series. This is a very strong guideline, minimally for bisectability.

But to me, this feature has never existed in practice. I found and
reported the security issue (BZ#960) while working on the downstream
enablement of UEFI HTTPS boot. The problem caused me to postpone the
downstream enablement. So for me there's nothing to regress.

Similarly, knowledge of this issue prevented me from porting
NETWORK_TLS_ENABLE from OvmfPkg to ArmVirtPkg, up-stream:

  https://bugzilla.tianocore.org/show_bug.cgi?id=1007
  https://bugzilla.tianocore.org/show_bug.cgi?id=1009

For a long time, I couldn't publicly tell the truth about splitting
BZ#1009 off of BZ#1007. My *real* incentive was to save at least
ArmVirtPkg users from exposure. I called Ard on the phone, and we agreed
to postpone TLS in ArmVirPkg.

(I couldn't do the same for OVMF users, as I would have had to back out
TLS settings from OvmfPkg, and that would have drawn stares.)

I went on to intentionally ignore BZ#1009, hoping no-one would ask me
about it. Then Guillaume did. See my answer here:
<https://bugzilla.tianocore.org/show_bug.cgi?id=1009#c4>.

Therefore, UEFI HTTPS boot has not existed for me, in practice, even in
the *upstream* project, to the extent that I was able to block the
"spreading" of the feature.


> But I stand by the assertion that this could be done properly, without
> the regression, with much less additional typing even than we've
> already done in this thread the last couple of days. It just isn't that
> hard.

David: it *is* hard! It is hard for me. I wouldn't know where to begin.

I assume it may be hard for Jiaxin too.

If it's not hard for you, can you please post the patches? Feel free to
pick up this series as a starting point, rework it, and then post an
updated variant.

If it takes two more weeks and you can solve the problem to everyone's
satisfaction (including your own!), I'll be overjoyed. I'll be happy to
wait!

If it takes four more months... then I might apply the present patchset
down-stream only.

As always, I strongly favor "upstream first". Show us the code, please?


> Ultimately, though, do whatever you like. I am not the final arbiter of
> engineering sanity and common sense for the EDK2 project.

No well-founded NACK or pending question should be ignored in patch review.

> 
> If I were, the world would be a very different place.

No doubt! ;)

Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Fri, 2019-10-11 at 17:36 +0200, Laszlo Ersek wrote:
> On 10/11/19 13:16, David Woodhouse wrote:
> > I first started looking at this when it was
> > reported as such, on the list.
> 
> I believe you. Can you somehow find that thread? I tried, but I couldn't
> find it. My mailbox (going back 9 years) is indexed, but my searches
> have failed. I must be using the wrong search terms. If I try "GEN_IP"
> or "Subject Alternative Name", I only get this thread.

https://www.mail-archive.com/devel@edk2.groups.io/msg03339.html

In that thread you pointed me at the bug, and I immediately pointed out
the error in the patch series:
https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31

Followed by a bit more detail on how to fix it, with examples to look
at:
https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32

> David: it *is* hard! It is hard for me. I wouldn't know where to begin.

I suspect this is false modesty on your part. Given the pointers and
the examples above, I have lots of confidence that if this were the
task on your plate, you would accomplish it with ease.

I would, of course, be happy to provide further pointers, and even work
with upstream OpenSSL to make this even easier. Crypto libraries should
make it hard for application developers to get things wrong, and they
often let us down in that respect.

In fact, I did that last bit already:
https://bugzilla.tianocore.org/show_bug.cgi?id=960#c33

> As always, I strongly favor "upstream first". Show us the code, please?

It's already linked from that Bugzilla comment I referenced:
https://github.com/openssl/openssl/pull/9201

Pull that into your OpenSSL tree, then make a trivial change following
the example in that PR, to do

   if (SSL_set1_ip_asc(ssl, hostname) < 0)
       SSL_set1_host(ssl, hostname);

instead of just the SSL_set1_host() call.

That way, *if* the string happens to be a valid IPv6 or Legacy IP
address, the SSL_set1_ip_asc() call will work; otherwise it's treated
as a hostname.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/11/19 18:01, David Woodhouse wrote:
> On Fri, 2019-10-11 at 17:36 +0200, Laszlo Ersek wrote:
>> On 10/11/19 13:16, David Woodhouse wrote:
>>> I first started looking at this when it was
>>> reported as such, on the list.
>>
>> I believe you. Can you somehow find that thread? I tried, but I couldn't
>> find it. My mailbox (going back 9 years) is indexed, but my searches
>> have failed. I must be using the wrong search terms. If I try "GEN_IP"
>> or "Subject Alternative Name", I only get this thread.
> 
> https://www.mail-archive.com/devel@edk2.groups.io/msg03339.html

Thank you.

IIUC, Sivaraman attempted to integrate the patches from TianoCore#960,
and then they ran into the SAN/GEN_IP regression *in practice*.

Unfortunately, in retrospect, that has been a *complete* communication
fiasco.

- Sivaraman tested the patches from TianoCore#960, but he did not report
his findings (the regression) in the Bugzilla entry.

- Conversely, the discussion on the list was missed by Jiaxin.

- I participated in both places, and I failed to realize we were talking
about any kind of regression.

(To be honest, even if I re-read the initial message from Sivaraman, I
still find it very difficult to understand:

    When we create certificates with CN as Host Name and SAN as IP TLS
    Handshake works only for Host Name and it provides Handshake
    Error when the request are IP Based.

Even a *modicum* of punctuation would have helped with that *soup* of
acronyms. Such as:

    When we create certificates with CN as Host Name, and SAN as IP, TLS
    Handshake works only for Host Name, and it provides Handshake
    Error when the request are IP Based.

"SAN as IP" is the expression with 90% of the information content, and
it received a fraction of the space / focus.

Not to mention, back then I had zero idea about "Subject Alternative
Name" and "GEN_IP".)


> In that thread you pointed me at the bug, and I immediately pointed out
> the error in the patch series:
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c31

That's correct. The word "regression" was not uttered however. From the
rest of the discussion in the BZ, I gather that Jiaxin never treated the
problem as a regression.

If this were not a practical regression, I'd agree with Jiaxin that we
should consider the SAN/GEN_IP use case on top of the present patch set,
optionally, separately, incrementally.

But, with evidence that AMI reported their practical regression back in
June -- even though they failed to (a) describe their use case clearly,
(b) state that it was a regression, (c) make the statement in the same
place where the patches existed --, I'm now inclined to reverse my position.

*Unless* Jiaxin can show that what AMI is trying to do is outside of the
UEFI spec (v2.8), I agree we should fix the SAN/GEN_IP case in *this*
series.

(The spec writes,

  EfiTlsVerifyHost -- TLS session hostname for validation which is used
                      to verify whether the name within the peer
                      certificate matches a given host name. [...]

The "given host name" expression (EFI_TLS_VERIFY_HOST.HostName) seems to
map to "URL used in request".

The question is whether the "name within the peer certificate" part
includes GEN_IP in the Subject Alternative Name. If it does, then this
series is not complete.)


> Followed by a bit more detail on how to fix it, with examples to look
> at:
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> 
>> David: it *is* hard! It is hard for me. I wouldn't know where to begin.
> 
> I suspect this is false modesty on your part.

Nope.

> Given the pointers and
> the examples above, I have lots of confidence that if this were the
> task on your plate, you would accomplish it with ease.

Definitely not with ease.

> 
> I would, of course, be happy to provide further pointers, and even work
> with upstream OpenSSL to make this even easier. Crypto libraries should
> make it hard for application developers to get things wrong, and they
> often let us down in that respect.
> 
> In fact, I did that last bit already:
> https://bugzilla.tianocore.org/show_bug.cgi?id=960#c33
> 
>> As always, I strongly favor "upstream first". Show us the code, please?
> 
> It's already linked from that Bugzilla comment I referenced:
> https://github.com/openssl/openssl/pull/9201
> 
> Pull that into your OpenSSL tree, then make a trivial change following
> the example in that PR, to do
> 
>    if (SSL_set1_ip_asc(ssl, hostname) < 0)
>        SSL_set1_host(ssl, hostname);
> 
> instead of just the SSL_set1_host() call.
> 
> That way, *if* the string happens to be a valid IPv6 or Legacy IP
> address, the SSL_set1_ip_asc() call will work; otherwise it's treated
> as a hostname.

My understanding is that a fix purely in edk2 -- that is, without
advancing our openssl submodule reference at once -- is possible, based
on your comment

  https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32

Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library",
2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()).
The last argument of that call is currently NULL.

We should change that, to a callback function that implements what
ssl_app_verify_callback() and match_cert_hostname() do, in your source file

http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c

There seems to be a GEN_* switch inside a loop in there.

Jiaxin: I think David has a point. AMI's use case appears covered by the
UEFI-2.8 spec, to me anyway (though I'm not an OpenSSL expert!!!), and
they've encountered a practical regression with this patch set. Do you
think you can implement the verify callback outlined by David above, or
do you prefer if we wait until we can consume David's OpenSSL fix
through the next openssl submodule rebase (and then the edk2 change is
minimal)?

David: another way to prevent the regression is to commit the current
patches, but disable them with a BOOLEAN PCD, by default. (This need not
be a feature PCD; it could even be dynamic.) Then platforms accepting
the SAN/GEN_IP regression temporarily could enable the PCD. This
solution would permit a separate (follow-up) series for the SAN/GEN_IP
case. We could file a reminder BZ now, and implement the "easy" solution
when we next rebase the openssl submodule. Would that be tolerable?


Thanks,
Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/14/19 18:15, Laszlo Ersek wrote:

> David: another way to prevent the regression is to commit the current
> patches, but disable them with a BOOLEAN PCD, by default. (This need not
> be a feature PCD; it could even be dynamic.) Then platforms accepting
> the SAN/GEN_IP regression temporarily could enable the PCD. This
> solution would permit a separate (follow-up) series for the SAN/GEN_IP
> case. We could file a reminder BZ now, and implement the "easy" solution
> when we next rebase the openssl submodule. Would that be tolerable?

... to clarify, in this case, the upstream edk2 project should *not*
claim to have fixed CVE-2019-14553, until the reminder BZ is also
closed! The new BZ should actually block TianoCore#960.

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote:
> My understanding is that a fix purely in edk2 -- that is, without
> advancing our openssl submodule reference at once 

Haha, I love the fact that I am hoist by my own petard on patching
OpenSSL. I evidently did such a good job of upstreaming all the quirks
we need for EDK2, that we're now *incapable* of carrying any local
patches to OpenSSL.

I'll take that as a win, I suppose :)

> -- is possible, based
> on your comment
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> 
> Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library",
> 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()).
> The last argument of that call is currently NULL.
> 
> We should change that, to a callback function that implements what
> ssl_app_verify_callback() and match_cert_hostname() do, in your source file
> 
> http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c
> 
> There seems to be a GEN_* switch inside a loop in there.

That's harder than it needs to be; it's the version for OpenSSL < 1.0.2
where they made the users jump through *lots* of hoops to validate
certs correctly. These days it's much easier; you only need the version
at

http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c#l1369
which is called from the actual callback at
http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c#l1507

I'll see if I can throw something together for you at least as an
example.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote:
> My understanding is that a fix purely in edk2 -- that is, without
> advancing our openssl submodule reference at once -- is possible, based
> on your comment
> 
>   https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> 
> Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library",
> 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()).
> The last argument of that call is currently NULL.
> 
> We should change that, to a callback function that implements what
> ssl_app_verify_callback() and match_cert_hostname() do, in your source file
> 
> http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c


Hm, you are lost in a twisty maze of verify callbacks, all alike.

Actually the one you can set with SSL_set_verify() isn't the one you
want. That's a low-level one, called from within the generic
X509_verify_cert() function.

The "app callback" in my OpenConnect example is set on the SSL_CTX not
the SSL object, and is called from the top-level
ssl_verify_cert_chain() function *instead* of X509_verify_cert().

It is X509_verify_cert() which can do the hostname/IP checks for us, if
we can only tell it that we want it to. But the X509_VERIFY_PARAM
object is private to the SSL.

As discussed, we have the SSL_set1_host() accessor function which lets
us set the hostname. The implementation really is a simple one-liner,
calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way
for use to set the IP address from the outside, without an equivalent
accessor function for that (and without SSL_set1_host() spotting that
the string it's given is an IP address, and doing so).

But what we can do is stash the target string in some ex_data hanging
off the SSL object, then have an app callback — which *can* reach the
underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or
X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the
normal X509_verify_cert() function that it has overridden.

Something like this... and instead of calling SSL_set1_host(ssl, host)
your own code now has to call
SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));

diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c
index f9ad6f6b946c..add5810cc4bd 100644
--- a/CryptoPkg/Library/TlsLib/TlsInit.c
+++ b/CryptoPkg/Library/TlsLib/TlsInit.c
@@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 
 #include "InternalTlsLib.h"
 
+/* You are lost in a twisty maze of SSL cert verify callbacks, all
+ * alike.  All we really wanted to do was call SSL_set1_host() and
+ * have it work for IP addresses too, which OpenSSL PR#9201 will do
+ * for us. But until we update OpenSSL, that doesn't work. And we
+ * can't get at the underlying X509_VERIFY_PARAM to set the IP address
+ * for ourselves.
+ *
+ * So we install an app_verify_callback in the SSL_CTX (which is
+ * different to the per-SSL callback wae can use, because it happens
+ * sooner. All our callback does it set the hostname or IP address in
+ * the X509_VERIFY_PARAM like we wanted to in the first place, and
+ * then call X509_verify_param() which is the default function.
+ *
+ * How does it find the hostname/IP string? It's attached to the SSL
+ * as ex_data, using this index:
+ */
+static int ssl_target_idx;
+
+void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
+		    int idx, long argl, void *argp)
+{
+	/* Free it */
+}
+
+int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
+		   void *from_d, int idx, long argl, void *argp)
+{
+	/* strdup it */
+	return 0;
+}
+
+int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
+{
+	SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
+	char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
+	X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
+
+	if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
+		X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
+
+	return X509_verify_cert(ctx);
+}
+
 /**
   Initializes the OpenSSL library.
 
@@ -40,6 +83,9 @@ TlsInitialize (
     return FALSE;
   }
 
+  ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL,
+					ssl_target_dup, ssl_target_free);
+
   //
   // Initialize the pseudorandom number generator.
   //
@@ -106,6 +152,10 @@ TlsCtxNew (
   //
   SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
 
+  /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which
+   * we could have done as SSL_set_verify(). Twisty maze, remember? */
+  SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL);
+
   return (VOID *) TlsCtx;
 }
 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Tue, 2019-10-15 at 12:03 +0100, David Woodhouse wrote:
> 
> Something like this... and instead of calling SSL_set1_host(ssl, host)
> your own code now has to call
> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));

Here's how I tested that in the OpenSSL tree in userspace, FWIW...

diff --git a/demos/bio/sconnect.c b/demos/bio/sconnect.c
index 7e46bf0ad8..1fd2829a79 100644
--- a/demos/bio/sconnect.c
+++ b/demos/bio/sconnect.c
@@ -25,11 +25,40 @@
 #define HOSTPORT "localhost:4433"
 #define CAFILE "root.pem"
 
+static int ssl_target_idx;
+
+void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
+		    int idx, long argl, void *argp)
+{
+	printf("Freeing %s\n", ptr);
+	free(ptr);
+}
+
+int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
+		   void *from_d, int idx, long argl, void *argp)
+{
+	/* strdup it */
+	return 0;
+}
+
+int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
+{
+	SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
+	char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
+	X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
+
+	printf("Setting %s\n", hostname);
+	if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
+		X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
+
+	return X509_verify_cert(ctx);
+}
+
 int main(int argc, char *argv[])
 {
     const char *hostport = HOSTPORT;
     const char *CAfile = CAFILE;
-    char *hostname;
+    const char *hostname;
     char *cp;
     BIO *out = NULL;
     char buf[1024 * 10], *p;
@@ -43,10 +72,6 @@ int main(int argc, char *argv[])
     if (argc > 2)
         CAfile = argv[2];
 
-    hostname = OPENSSL_strdup(hostport);
-    if ((cp = strchr(hostname, ':')) != NULL)
-        *cp = 0;
-
 #ifdef WATT32
     dbug_init();
     sock_init();
@@ -54,6 +79,8 @@ int main(int argc, char *argv[])
 
     ssl_ctx = SSL_CTX_new(TLS_client_method());
 
+    SSL_CTX_set_cert_verify_callback(ssl_ctx, app_verify_callback, NULL);
+
     /* Enable trust chain verification */
     SSL_CTX_set_verify(ssl_ctx, SSL_VERIFY_PEER, NULL);
     SSL_CTX_load_verify_locations(ssl_ctx, CAfile, NULL);
@@ -62,9 +89,6 @@ int main(int argc, char *argv[])
     ssl = SSL_new(ssl_ctx);
     SSL_set_connect_state(ssl);
 
-    /* Enable peername verification */
-    if (SSL_set1_host(ssl, hostname) <= 0)
-        goto err;
 
     /* Use it inside an SSL BIO */
     ssl_bio = BIO_new(BIO_f_ssl());
@@ -73,6 +97,13 @@ int main(int argc, char *argv[])
     /* Lets use a connect BIO under the SSL BIO */
     out = BIO_new(BIO_s_connect());
     BIO_set_conn_hostname(out, hostport);
+
+    /* The BIO has parsed the host:port and even IPv6 literals in [] */
+    hostname = BIO_get_conn_hostname(out);
+
+    if (hostname)
+	    SSL_set_ex_data(ssl, ssl_target_idx, strdup(hostname));
+
     BIO_set_nbio(out, 1);
     out = BIO_push(ssl_bio, out);
 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/15/19 13:03, David Woodhouse wrote:
> On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote:
>> My understanding is that a fix purely in edk2 -- that is, without
>> advancing our openssl submodule reference at once -- is possible, based
>> on your comment
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
>>
>> Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library",
>> 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()).
>> The last argument of that call is currently NULL.
>>
>> We should change that, to a callback function that implements what
>> ssl_app_verify_callback() and match_cert_hostname() do, in your source file
>>
>> http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c
> 
> 
> Hm, you are lost in a twisty maze of verify callbacks, all alike.

Definitely.

> Actually the one you can set with SSL_set_verify() isn't the one you
> want. That's a low-level one, called from within the generic
> X509_verify_cert() function.

Well, above I referred to SSL_set_verify() only because you had named
that function in <https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32>:

> Note: the callback you're after is the final argument to
> SSL_set_verify(), which you are calling from TlsSetVerify() with a
> NULL argument. You need to point it at your own function of type
> SSL_verify_cb which does the checking correctly.

Back to the email:

On 10/15/19 13:03, David Woodhouse wrote:
> The "app callback" in my OpenConnect example is set on the SSL_CTX not
> the SSL object, and is called from the top-level
> ssl_verify_cert_chain() function *instead* of X509_verify_cert().
> 
> It is X509_verify_cert() which can do the hostname/IP checks for us, if
> we can only tell it that we want it to. But the X509_VERIFY_PARAM
> object is private to the SSL.
> 
> As discussed, we have the SSL_set1_host() accessor function which lets
> us set the hostname. The implementation really is a simple one-liner,
> calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way
> for use to set the IP address from the outside, without an equivalent
> accessor function for that (and without SSL_set1_host() spotting that
> the string it's given is an IP address, and doing so).
> 
> But what we can do is stash the target string in some ex_data hanging
> off the SSL object, then have an app callback — which *can* reach the
> underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or
> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the
> normal X509_verify_cert() function that it has overridden.
> 
> Something like this... and instead of calling SSL_set1_host(ssl, host)
> your own code now has to call
> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));
> 
> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c
> index f9ad6f6b946c..add5810cc4bd 100644
> --- a/CryptoPkg/Library/TlsLib/TlsInit.c
> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c
> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  #include "InternalTlsLib.h"
>  
> +/* You are lost in a twisty maze of SSL cert verify callbacks, all
> + * alike.  All we really wanted to do was call SSL_set1_host() and
> + * have it work for IP addresses too, which OpenSSL PR#9201 will do
> + * for us. But until we update OpenSSL, that doesn't work. And we
> + * can't get at the underlying X509_VERIFY_PARAM to set the IP address
> + * for ourselves.
> + *
> + * So we install an app_verify_callback in the SSL_CTX (which is
> + * different to the per-SSL callback wae can use, because it happens
> + * sooner. All our callback does it set the hostname or IP address in
> + * the X509_VERIFY_PARAM like we wanted to in the first place, and
> + * then call X509_verify_param() which is the default function.
> + *
> + * How does it find the hostname/IP string? It's attached to the SSL
> + * as ex_data, using this index:
> + */
> +static int ssl_target_idx;
> +
> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
> +		    int idx, long argl, void *argp)
> +{
> +	/* Free it */
> +}
> +
> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
> +		   void *from_d, int idx, long argl, void *argp)
> +{
> +	/* strdup it */
> +	return 0;
> +}
> +
> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
> +{
> +	SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
> +	char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
> +	X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
> +
> +	if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
> +		X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
> +
> +	return X509_verify_cert(ctx);
> +}
> +
>  /**
>    Initializes the OpenSSL library.
>  
> @@ -40,6 +83,9 @@ TlsInitialize (
>      return FALSE;
>    }
>  
> +  ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL,
> +					ssl_target_dup, ssl_target_free);
> +
>    //
>    // Initialize the pseudorandom number generator.
>    //
> @@ -106,6 +152,10 @@ TlsCtxNew (
>    //
>    SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
>  
> +  /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which
> +   * we could have done as SSL_set_verify(). Twisty maze, remember? */
> +  SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL);
> +
>    return (VOID *) TlsCtx;
>  }

Thank you very much for this code.

To me it seems like this patch should be squashed into patch#2 (which
modifies TlsLib), after adapting it to the edk2 coding style.

I have some comments / questions.


(1) In TlsSetVerifyHost(), you mention that SSL_set1_host() should be
replaced with SSL_set_ex_data(). OK.

In case the client does not use the TlsSetVerifyHost() function,
SSL_set_ex_data() would not be called. However, app_verify_callback()
would still be called.

Is my understanding correct that "hostname" (from SSL_get_ex_data())
would be NULL in that case, and therefore both
X509_VERIFY_PARAM_set1_ip_asc() and X509_VERIFY_PARAM_set1_host() would
be omitted?

(I mean this looks like the correct behavior to me; just asking if that
is indeed the intent of the code.)


(2) The documentation of the APIs seems a bit strange. I've found the
following "directory" web pages:

https://www.openssl.org/docs/man1.0.2/man3/
https://www.openssl.org/docs/man1.1.0/man3/
https://www.openssl.org/docs/man1.1.1/man3/

edk2 currently consumes "OpenSSL_1_1_1b", so I thought I should only
look at the last page. However, SSL_get_ex_new_index() is only
documented in the first page.

Is my understanding correct that each OpenSSL API should be looked up in
the latest documentation directory that appears to describe it?

In other words: assuming I look for SSL_get_ex_new_index() in the 1.1.1
manual, and fail to find it, does that mean that the API is deprecated,
or only that I should look at an earlier release of the manual?


(3) Anyway, SSL_get_ex_new_index() seems like a thin wrapper around
CRYPTO_get_ex_new_index().

https://www.openssl.org/docs/man1.0.2/man3/SSL_get_ex_new_index.html
https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html

Based on that, I think the second ("argp") parameter of our
SSL_get_ex_new_index() call should be NULL, as the "argp" parameters of
the "free" and "dup" functions are unused.

Do you agree?


(4) What happens if we call SSL_set_ex_data(), but a non-NULL value has
already been stored for the same index?

Do we have to first fetch it with SSL_get_ex_data() and free it, or will
it be automatically freed with "free_func"?

(Note: I think that, if we used a "new_func" for allocating anything,
this question could be relevant the very first time SSL_set_ex_data()
were called.)


(5) The most confusing part... If I look at the 1.0.2 documentation, it
says about "dup_func":

> dup_func() is called when a structure is being copied. [...] The
> from_d parameter is passed a pointer to the source application data
> when the function is called, when the function returns the value is
> copied to the destination: the application can thus modify the data
> pointed to by from_d and have different values in the source and
> destination

This makes no sense: I wouldn't want to modify the source application
data in-place, just to give the new parent object a modified instance of it!

Also, how would OpenSSL know how many bytes to copy?

So I looked at the code, and it turns out dup_func is called with a
(void**), not a (void*):

            if (!storage[i]->dup_func(to, from, &ptr, i,
                                      storage[i]->argl, storage[i]->argp))

The 1.1.1 documentation is more accurate; it says:

https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html

> The from_d parameter needs to be cast to a void **pptr as the API has
> currently the wrong signature; that will be changed in a future
> version. The *pptr is a pointer to the source exdata. When the
> dup_func() returns, the value in *pptr is copied to the destination
> ex_data. If the pointer contained in *pptr is not modified by the
> dup_func(), then both to and from will point to the same data

So let me summarize the documentation status:

- SSL_get_ex_new_index is not documented in 1.1.1,

- SSL_get_ex_new_index is not documented in 1.1.0,

- SSL_get_ex_new_index has a "shim" documentation in 1.0.2, and it
refers the reader to RSA_get_ex_new_index(),

- RSA_get_ex_new_index is documented in 1.0.2 *incorrectly* (with regard
to dup_func),

- the valid documentation is in 1.1.1, under CRYPTO_get_ex_new_index --
but for learning about that function, I had to look at the library
source code.

When you suggested the IP checking would be easy to imlement, you were
joking, right? :)


(6) Given that we are introducing callbacks (from
CryptoPkg/Library/OpensslLib to CryptoPkg/Library/TlsLib), and OpenSSL
does not declare these function prototypes with EFIAPI, the callbacks
too must be defined with "native" (not EFIAPI) calling convention.

Correct?

Thanks
Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Tue, 2019-10-15 at 15:54 +0200, Laszlo Ersek wrote:
> On 10/15/19 13:03, David Woodhouse wrote:
> > On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote:
> > > My understanding is that a fix purely in edk2 -- that is, without
> > > advancing our openssl submodule reference at once -- is possible, based
> > > on your comment
> > > 
> > >   https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32
> > > 
> > > Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library",
> > > 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify()).
> > > The last argument of that call is currently NULL.
> > > 
> > > We should change that, to a callback function that implements what
> > > ssl_app_verify_callback() and match_cert_hostname() do, in your source file
> > > 
> > > http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openssl.c
> > 
> > 
> > Hm, you are lost in a twisty maze of verify callbacks, all alike.
> 
> Definitely.
> 
> > Actually the one you can set with SSL_set_verify() isn't the one you
> > want. That's a low-level one, called from within the generic
> > X509_verify_cert() function.
> 
> Well, above I referred to SSL_set_verify() only because you had named
> that function in <https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32>:

Yeah, I was wrong.

You were the one who called me an expert; I didn't :)

> Thank you very much for this code.
> 
> To me it seems like this patch should be squashed into patch#2 (which
> modifies TlsLib), after adapting it to the edk2 coding style.
> 
> I have some comments / questions.
> 
> 
> (1) In TlsSetVerifyHost(), you mention that SSL_set1_host() should be
> replaced with SSL_set_ex_data(). OK.
> 
> In case the client does not use the TlsSetVerifyHost() function,
> SSL_set_ex_data() would not be called. However, app_verify_callback()
> would still be called.
> 
> Is my understanding correct that "hostname" (from SSL_get_ex_data())
> would be NULL in that case, and therefore both
> X509_VERIFY_PARAM_set1_ip_asc() and X509_VERIFY_PARAM_set1_host() would
> be omitted?
> 
> (I mean this looks like the correct behavior to me; just asking if that
> is indeed the intent of the code.)

Yep. In the case where there's no such ex_data attached to the SSL
object, our own app_verify_callback() function would do nothing
special, just call through to the standard X509_verify_cert() function
that would have been called if we hadn't registered our own override.

You may call this "correct". I think you're right, in the context you
meant it.

But I suggest there is a conversation to be had about whether it's ever
"correct" for a crypto library to silently accept any bloody
certificate it sees, regardless of what the Subject of that certificate
is.

Or whether perhaps it ought to fail safe, and require you to jump
through extra hoops if you *really* want to accept any certificate.

In a world where crypto libraries made it hard for the users to Do The
Wrong Thing, CVE-2019-14553 would never have happened.

But we digress. Is it late enough in the day for me to start drinking
yet?

> (2) The documentation of the APIs seems a bit strange. I've found the
> following "directory" web pages:
> 
> https://www.openssl.org/docs/man1.0.2/man3/
> https://www.openssl.org/docs/man1.1.0/man3/
> https://www.openssl.org/docs/man1.1.1/man3/
> 
> edk2 currently consumes "OpenSSL_1_1_1b", so I thought I should only
> look at the last page. However, SSL_get_ex_new_index() is only
> documented in the first page.
> 
> Is my understanding correct that each OpenSSL API should be looked up in
> the latest documentation directory that appears to describe it?
> 
> In other words: assuming I look for SSL_get_ex_new_index() in the 1.1.1
> manual, and fail to find it, does that mean that the API is deprecated,
> or only that I should look at an earlier release of the manual?

I was just looking at the code. I happened to be looking at OpenSSL
HEAD but the sconnect demo I showed in my second email works with
1.1.1.

> 
> (3) Anyway, SSL_get_ex_new_index() seems like a thin wrapper around
> CRYPTO_get_ex_new_index().
> 
> https://www.openssl.org/docs/man1.0.2/man3/SSL_get_ex_new_index.html
> https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html
> 
> Based on that, I think the second ("argp") parameter of our
> SSL_get_ex_new_index() call should be NULL, as the "argp" parameters of
> the "free" and "dup" functions are unused.
> 
> Do you agree?

Yeah, I had just cargo-culted that from elsewhere, and was assuming it
was performing some kind of de-duplication or alternative lookup
functionality. It does look like you can leave it NULL and it doesn't
matter.

> 
> (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has
> already been stored for the same index?
> 
> Do we have to first fetch it with SSL_get_ex_data() and free it, or will
> it be automatically freed with "free_func"?

It will be automatically freed with free_func. Note that this doesn't
work in the sconnect.c patch I sent, but only because I'd forgotten to
actually add the call to SSL_get_ex_new_index(). After I fixed that and
wasn't just using an index of zero, I saw the printf() that I'd added
in my free_func.


> (Note: I think that, if we used a "new_func" for allocating anything,
> this question could be relevant the very first time SSL_set_ex_data()
> were called.)

I believe new_func is only for when a new SSL object is created. We
don't need that.

> (5) The most confusing part... If I look at the 1.0.2 documentation, it
> says about "dup_func":
> 
> > dup_func() is called when a structure is being copied. [...] The
> > from_d parameter is passed a pointer to the source application data
> > when the function is called, when the function returns the value is
> > copied to the destination: the application can thus modify the data
> > pointed to by from_d and have different values in the source and
> > destination
> 
> This makes no sense: I wouldn't want to modify the source application
> data in-place, just to give the new parent object a modified instance of it!
> 
> Also, how would OpenSSL know how many bytes to copy?
> 
> So I looked at the code, and it turns out dup_func is called with a
> (void**), not a (void*):
> 
>             if (!storage[i]->dup_func(to, from, &ptr, i,
>                                       storage[i]->argl, storage[i]->argp))
> 
> The 1.1.1 documentation is more accurate; it says:
> 
> https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html
> 
> > The from_d parameter needs to be cast to a void **pptr as the API has
> > currently the wrong signature; that will be changed in a future
> > version. The *pptr is a pointer to the source exdata. When the
> > dup_func() returns, the value in *pptr is copied to the destination
> > ex_data. If the pointer contained in *pptr is not modified by the
> > dup_func(), then both to and from will point to the same data


Yes, I think dup_func wants to do something like:

int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
		   void *from_d, int idx, long argl, void *argp)
{
	char **p = from_d;
	if (*p) {
		*p = strdup(*p);
		return !!*p;
	}
	return 1;
}

> So let me summarize the documentation status:
> 
> - SSL_get_ex_new_index is not documented in 1.1.1,
> 
> - SSL_get_ex_new_index is not documented in 1.1.0,
> 
> - SSL_get_ex_new_index has a "shim" documentation in 1.0.2, and it
> refers the reader to RSA_get_ex_new_index(),
> 
> - RSA_get_ex_new_index is documented in 1.0.2 *incorrectly* (with regard
> to dup_func),
> 
> - the valid documentation is in 1.1.1, under CRYPTO_get_ex_new_index --
> but for learning about that function, I had to look at the library
> source code.
> 
> When you suggested the IP checking would be easy to imlement, you were
> joking, right? :)

I confess I hadn't realised that we needed to jump through ex_data
hoops to even get the hostname information to the right place in the
callback. But I don't think I actually said "easy", did I? Only that it
would take less typing than we'd already expended in trying to make
excuses for the regression.

I also said you could manage it, which you seem to have proved by
working out the details I had... erm... left as an exercise for you :)

> (6) Given that we are introducing callbacks (from
> CryptoPkg/Library/OpensslLib to CryptoPkg/Library/TlsLib), and OpenSSL
> does not declare these function prototypes with EFIAPI, the callbacks
> too must be defined with "native" (not EFIAPI) calling convention.

Yes, that makes sense.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Thu, 2019-10-10 at 20:03 +0200, Laszlo Ersek wrote:
> (I can't test it easily myself, as I don't even know how to create a
> server certificate with a SAN -- any kind of SAN, let alone GEN_IP.)

I had to look it up again, but here goes...

$ cat v3.ext 
subjectAltName = @alt_names
[alt_names]
DNS.1 = lersek-test.redhat.com
IP.2 = 192.168.124.2
IP.3 = fd33:eb1b:9b36::2
$ openssl req -nodes -newkey rsa:2048 -keyout key.pem -out cert.csr
 ...  
$ openssl x509 -signkey ca-key.pem -in cert.csr -req -days 3650 -out cert.pem -extfile v3.ext
Signature ok
subject=C = AU, ST = Some-State, O = Internet Widgits Pty Ltd
Getting Private key
$ openssl x509 -in cert.pem -noout -text
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            56:c5:33:0f:b1:2d:e5:b5:1e:89:e5:a7:a2:45:a9:06:43:1f:4a:1e
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: C = AU, ST = Some-State, O = Internet Widgits Pty Ltd
        Validity
            Not Before: Oct 15 15:56:11 2019 GMT
            Not After : Oct 12 15:56:11 2029 GMT
        Subject: C = AU, ST = Some-State, O = Internet Widgits Pty Ltd
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                RSA Public-Key: (2432 bit)
                Modulus:
                    00:b4:6b:27:98:25:af:c1:ff:1e:ca:b0:7e:f4:d8:
                    bc:ed:43:86:67:54:5d:da:b4:1e:c2:90:5f:83:3c:
                    02:11:fc:13:72:85:b2:88:a4:65:41:0b:76:5f:23:
                    be:8a:9f:fe:79:4b:73:3b:2e:c7:4b:3c:bf:16:c9:
                    97:55:35:17:f3:a1:72:4b:30:c2:e0:27:94:12:f3:
                    56:00:e6:ce:82:4b:11:5d:a4:1e:9b:fa:fa:b9:1b:
                    2a:4d:18:b5:ba:a5:e6:0c:c7:a8:a8:a1:6d:aa:88:
                    84:dc:96:0e:b2:6c:1c:35:aa:e7:c7:94:3d:f9:d5:
                    c7:c2:a2:0d:4b:b3:6e:7a:f7:08:5f:c5:09:cd:15:
                    93:1a:f7:98:df:2a:4c:66:89:24:ed:1f:d0:16:63:
                    81:65:a5:58:3b:a1:cd:25:62:9b:99:81:54:08:17:
                    18:ec:7c:2f:08:a2:3b:28:57:32:9d:17:47:0a:86:
                    fb:62:b1:41:99:e6:fb:de:a8:ea:20:7e:f3:1b:ee:
                    ba:ea:9a:21:64:29:92:f2:ad:73:e5:19:05:9d:37:
                    53:e2:11:9f:18:5f:22:ba:e2:8b:0d:00:8c:9e:2f:
                    a7:87:3d:40:be:4a:a2:a5:92:08:0c:2e:61:c0:58:
                    7c:9a:99:e1:d6:ac:83:39:25:cf:3e:1b:ed:eb:a3:
                    6d:9d:cb:c5:38:de:c1:c7:6e:9b:34:14:be:30:3e:
                    82:90:1e:b9:4a:9a:76:e4:ef:33:0c:46:a2:31:72:
                    f6:c3:61:0b:f8:aa:67:89:f4:a5:e5:76:37:a1:29:
                    9f:80:79:aa:75
                Exponent: 65537 (0x10001)
        X509v3 extensions:
            X509v3 Subject Alternative Name: 
                DNS:lersek-test.redhat.com, IP Address:192.168.124.2, IP Address:FD33:EB1B:9B36:0:0:0:0:2
    Signature Algorithm: sha256WithRSAEncryption
         37:8c:17:6c:4d:5f:05:b6:70:b9:96:49:0a:e3:f6:3c:bd:3b:
         d0:fe:56:ee:ad:58:15:6e:a6:79:a8:3b:d4:fa:09:f9:7d:85:
         8a:8b:14:7b:e4:db:bf:2d:8d:32:28:26:d6:37:a5:51:90:e9:
         75:25:b9:9d:63:db:35:29:8a:58:61:56:b2:2a:5a:d3:80:b7:
         1d:4c:05:0b:49:da:6f:ec:67:f5:3d:09:f2:58:92:43:8d:39:
         d7:f4:f3:3c:bd:9b:16:a2:c9:c0:63:5d:c9:1a:c3:a7:24:fa:
         31:8c:7c:3e:98:98:87:8f:5b:fb:00:f5:41:15:16:89:c6:e3:
         c4:63:3a:3d:3e:b8:b5:b7:af:cb:11:1a:13:f4:b2:df:c4:f4:
         a1:a2:9c:d1:05:20:84:65:70:91:41:be:f4:26:c2:63:07:46:
         d0:63:bf:27:3f:42:9c:69:22:e1:d6:6a:41:dc:97:51:2d:ef:
         a1:11:20:ed:89:57:d6:d2:ad:6c:7f:88:69:ae:31:51:e8:cb:
         9e:3a:e1:49:48:01:5b:d5:ab:93:53:5e:cb:2f:72:6e:84:af:
         d0:c2:91:41:29:6f:3c:0b:df:c6:9c:77:14:fd:29:fc:65:0b:
         2d:6c:61:69:a6:72:19:38:5f:a1:83:fd:6c:22:02:d7:b6:81:
         9e:05:7c:58:2c:c9:eb:c0:09:aa:07:d1:b7:15:a1:e3:ea:27:
         b1:f7:70:87:fe:d6:16:57:67:70:fe:65:9a:0f:1b:11:be:22:
         08:2f:21:50:30:a4:35:99:d3:fb:4d:40:22:39:2c:f3


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/15/19 15:54, Laszlo Ersek wrote:
> On 10/15/19 13:03, David Woodhouse wrote:

>> The "app callback" in my OpenConnect example is set on the SSL_CTX not
>> the SSL object, and is called from the top-level
>> ssl_verify_cert_chain() function *instead* of X509_verify_cert().
>>
>> It is X509_verify_cert() which can do the hostname/IP checks for us, if
>> we can only tell it that we want it to. But the X509_VERIFY_PARAM
>> object is private to the SSL.
>>
>> As discussed, we have the SSL_set1_host() accessor function which lets
>> us set the hostname. The implementation really is a simple one-liner,
>> calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way
>> for use to set the IP address from the outside, without an equivalent
>> accessor function for that (and without SSL_set1_host() spotting that
>> the string it's given is an IP address, and doing so).
>>
>> But what we can do is stash the target string in some ex_data hanging
>> off the SSL object, then have an app callback — which *can* reach the
>> underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or
>> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the
>> normal X509_verify_cert() function that it has overridden.
>>
>> Something like this... and instead of calling SSL_set1_host(ssl, host)
>> your own code now has to call
>> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));
>>
>> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c
>> index f9ad6f6b946c..add5810cc4bd 100644
>> --- a/CryptoPkg/Library/TlsLib/TlsInit.c
>> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c
>> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>  
>>  #include "InternalTlsLib.h"
>>  
>> +/* You are lost in a twisty maze of SSL cert verify callbacks, all
>> + * alike.  All we really wanted to do was call SSL_set1_host() and
>> + * have it work for IP addresses too, which OpenSSL PR#9201 will do
>> + * for us. But until we update OpenSSL, that doesn't work. And we
>> + * can't get at the underlying X509_VERIFY_PARAM to set the IP address
>> + * for ourselves.
>> + *
>> + * So we install an app_verify_callback in the SSL_CTX (which is
>> + * different to the per-SSL callback wae can use, because it happens
>> + * sooner. All our callback does it set the hostname or IP address in
>> + * the X509_VERIFY_PARAM like we wanted to in the first place, and
>> + * then call X509_verify_param() which is the default function.
>> + *
>> + * How does it find the hostname/IP string? It's attached to the SSL
>> + * as ex_data, using this index:
>> + */
>> +static int ssl_target_idx;
>> +
>> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
>> +		    int idx, long argl, void *argp)
>> +{
>> +	/* Free it */
>> +}
>> +
>> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
>> +		   void *from_d, int idx, long argl, void *argp)
>> +{
>> +	/* strdup it */
>> +	return 0;
>> +}
>> +
>> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
>> +{
>> +	SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
>> +	char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
>> +	X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
>> +
>> +	if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
>> +		X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
>> +
>> +	return X509_verify_cert(ctx);
>> +}
>> +
>>  /**
>>    Initializes the OpenSSL library.
>>  
>> @@ -40,6 +83,9 @@ TlsInitialize (
>>      return FALSE;
>>    }
>>  
>> +  ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL,
>> +					ssl_target_dup, ssl_target_free);
>> +
>>    //
>>    // Initialize the pseudorandom number generator.
>>    //
>> @@ -106,6 +152,10 @@ TlsCtxNew (
>>    //
>>    SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
>>  
>> +  /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which
>> +   * we could have done as SSL_set_verify(). Twisty maze, remember? */
>> +  SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL);
>> +
>>    return (VOID *) TlsCtx;
>>  }

> (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has
> already been stored for the same index?
> 
> Do we have to first fetch it with SSL_get_ex_data() and free it, or will
> it be automatically freed with "free_func"?
> 
> (Note: I think that, if we used a "new_func" for allocating anything,
> this question could be relevant the very first time SSL_set_ex_data()
> were called.)

A similar question:

is it possible that app_verify_callback() is called more frequently than
SSL_set_ex_data() (in TlsSetVerify())?

Because that means that the frequency of SSL_set1_host() calls changes.
Previously we'd call SSL_set1_host() once per TlsSetVerify(), but now it
could be called multiple times per TlsSetVerify(). Is that the case?

If it is, is it OK?

To me the ownership of these strings (i.e., what component is
responsible for freeing the strings) is impenetrable. :(

Even the UEFI 2.8 spec doesn't explain whether EFI_TLS_SET_SESSION_DATA
saves the array of characters pointed-to by
"EFI_TLS_VERIFY_HOST.HostName", or just the pointer itself. :/

Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/15/19 17:57, David Woodhouse wrote:
> On Thu, 2019-10-10 at 20:03 +0200, Laszlo Ersek wrote:
>> (I can't test it easily myself, as I don't even know how to create a
>> server certificate with a SAN -- any kind of SAN, let alone GEN_IP.)
> 
> I had to look it up again, but here goes...
> 
> $ cat v3.ext 
> subjectAltName = @alt_names
> [alt_names]
> DNS.1 = lersek-test.redhat.com
> IP.2 = 192.168.124.2
> IP.3 = fd33:eb1b:9b36::2
> $ openssl req -nodes -newkey rsa:2048 -keyout key.pem -out cert.csr
>  ...  
> $ openssl x509 -signkey ca-key.pem -in cert.csr -req -days 3650 -out cert.pem -extfile v3.ext

I'm not familiar with this x509 invocation ("-signkey").

Thus far I've used x509 to sign self-signed certificate requests with a
CA key:

openssl x509 -req -in request.csr -out signedcert.pem \
  -CA ca-cert.pem -CAkey ca-key.pem [-CAcreateserial]

I guess "-signkey ca-key.pem" is a shorthand for the (-CA, -CAkey) pair?
(I've tried to look at the manual; I couldn't say I'm wiser now.)

Either way: why do we add the subject alternative names when the CA
signs the request? Shouldn't the *original* certificate request state
what alternative names can stand for the same subject?

(I don't even understand how a CA can usefully insert such an extension;
after all, it cannot be signed by the original certificate requestor!)

The "openssl req" command too seems to accept "-extensions" -- why are
we not required to use that? To me it seems like the only acceptable
place, to add alternative names.

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/15/19 18:56, Laszlo Ersek wrote:
> On 10/15/19 15:54, Laszlo Ersek wrote:
>> On 10/15/19 13:03, David Woodhouse wrote:
> 
>>> The "app callback" in my OpenConnect example is set on the SSL_CTX not
>>> the SSL object, and is called from the top-level
>>> ssl_verify_cert_chain() function *instead* of X509_verify_cert().
>>>
>>> It is X509_verify_cert() which can do the hostname/IP checks for us, if
>>> we can only tell it that we want it to. But the X509_VERIFY_PARAM
>>> object is private to the SSL.
>>>
>>> As discussed, we have the SSL_set1_host() accessor function which lets
>>> us set the hostname. The implementation really is a simple one-liner,
>>> calling X509_VERIFY_PARAM_set1_host(s->param, …). But there's no way
>>> for use to set the IP address from the outside, without an equivalent
>>> accessor function for that (and without SSL_set1_host() spotting that
>>> the string it's given is an IP address, and doing so).
>>>
>>> But what we can do is stash the target string in some ex_data hanging
>>> off the SSL object, then have an app callback — which *can* reach the
>>> underlying X509_VERIFY_PARAM — call X509_VERIFY_PARAM_set1_host() or
>>> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the
>>> normal X509_verify_cert() function that it has overridden.
>>>
>>> Something like this... and instead of calling SSL_set1_host(ssl, host)
>>> your own code now has to call
>>> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host));
>>>
>>> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c
>>> index f9ad6f6b946c..add5810cc4bd 100644
>>> --- a/CryptoPkg/Library/TlsLib/TlsInit.c
>>> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c
>>> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>>>
>>>  #include "InternalTlsLib.h"
>>>
>>> +/* You are lost in a twisty maze of SSL cert verify callbacks, all
>>> + * alike.  All we really wanted to do was call SSL_set1_host() and
>>> + * have it work for IP addresses too, which OpenSSL PR#9201 will do
>>> + * for us. But until we update OpenSSL, that doesn't work. And we
>>> + * can't get at the underlying X509_VERIFY_PARAM to set the IP address
>>> + * for ourselves.
>>> + *
>>> + * So we install an app_verify_callback in the SSL_CTX (which is
>>> + * different to the per-SSL callback wae can use, because it happens
>>> + * sooner. All our callback does it set the hostname or IP address in
>>> + * the X509_VERIFY_PARAM like we wanted to in the first place, and
>>> + * then call X509_verify_param() which is the default function.
>>> + *
>>> + * How does it find the hostname/IP string? It's attached to the SSL
>>> + * as ex_data, using this index:
>>> + */
>>> +static int ssl_target_idx;
>>> +
>>> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
>>> +		    int idx, long argl, void *argp)
>>> +{
>>> +	/* Free it */
>>> +}
>>> +
>>> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
>>> +		   void *from_d, int idx, long argl, void *argp)
>>> +{
>>> +	/* strdup it */
>>> +	return 0;
>>> +}
>>> +
>>> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy)
>>> +{
>>> +	SSL *ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
>>> +	char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
>>> +	X509_VERIFY_PARAM *vpm = X509_STORE_CTX_get0_param(ctx);
>>> +
>>> +	if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname))
>>> +		X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);
>>> +
>>> +	return X509_verify_cert(ctx);
>>> +}
>>> +
>>>  /**
>>>    Initializes the OpenSSL library.
>>>
>>> @@ -40,6 +83,9 @@ TlsInitialize (
>>>      return FALSE;
>>>    }
>>>
>>> +  ssl_target_idx = SSL_get_ex_new_index(0, "TLS target hosthame/IP", NULL,
>>> +					ssl_target_dup, ssl_target_free);
>>> +
>>>    //
>>>    // Initialize the pseudorandom number generator.
>>>    //
>>> @@ -106,6 +152,10 @@ TlsCtxNew (
>>>    //
>>>    SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
>>>
>>> +  /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), which
>>> +   * we could have done as SSL_set_verify(). Twisty maze, remember? */
>>> +  SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL);
>>> +
>>>    return (VOID *) TlsCtx;
>>>  }
> 
>> (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has
>> already been stored for the same index?
>>
>> Do we have to first fetch it with SSL_get_ex_data() and free it, or will
>> it be automatically freed with "free_func"?
>>
>> (Note: I think that, if we used a "new_func" for allocating anything,
>> this question could be relevant the very first time SSL_set_ex_data()
>> were called.)
> 
> A similar question:
> 
> is it possible that app_verify_callback() is called more frequently than
> SSL_set_ex_data() (in TlsSetVerify())?
> 
> Because that means that the frequency of SSL_set1_host() calls changes.
> Previously we'd call SSL_set1_host() once per TlsSetVerify(), but now it
> could be called multiple times per TlsSetVerify(). Is that the case?
> 
> If it is, is it OK?

Ehh, I failed to ask the actual question.

Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically,
every time just before we call X509_verify_cert()?

My concern is not with the crypto functionality, but whether we could be
leaking memory allocations.

Thanks
Laszlo

> To me the ownership of these strings (i.e., what component is
> responsible for freeing the strings) is impenetrable. :(
> 
> Even the UEFI 2.8 spec doesn't explain whether EFI_TLS_SET_SESSION_DATA
> saves the array of characters pointed-to by
> "EFI_TLS_VERIFY_HOST.HostName", or just the pointer itself. :/
> 
> Laszlo
> 
> 
> 

[RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

SSL_set1_host() in TlsSetVerifyHost() ignores GEN_IP entries in the peer
certificate's Subject Alternative Name (SAN) extension. This leads to the
rejection of any valid peer certificate that matches the dot-decimal IPv4,
or colon-hexadecimal IPv6, host part of an URL *only* through SAN/GEN_IP,
and not through the Common Name.

Based on David's guidance, replace SSL_set1_host() in TlsSetVerifyHost()
with application specific data ("ExData") that is associated with the SSL
object. Namely, pass the host part of the URL as "application specific
data" into a new peer certificate verification callback. In the callback,
first try to parse the host part of the URL as a numeric IP address, for
certificate subject verification. If that parsing fails, fall back to
interpreting the host part as a DNS hostname.

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Richard Levitte <levitte@openssl.org>
Cc: Sivaraman Nainar <sivaramann@amiindia.co.in>
Ref: http://mid.mail-archive.com/B4DE137BDB63634BAC03BD9DE765F197028B24CA23@VENUS1.in.megatrends.com
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=960
Ref: https://edk2.groups.io/g/devel/message/42022
Suggested-by: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Unfortunately, there are two problems with this patch:
    
    (1) X509_VERIFY_PARAM_set1_ip_asc() does not accept IPv4 addresses in
        dot-decimal notation (unless I messed up the code). My log file
        contains:
    
    > TlsDxe:TlsCertVerify: verifying peer certificate with DNS hostname "192.168.124.2"
    > TlsDxe:TlsCertVerify: peer certificate accepted
    
    (2) X509_VERIFY_PARAM_set1_ip_asc() does accept IPv6 addresses. However,
        in that case, the server certificate that I had generated with
        "genkey" (where I entered the IPv6 address in the Common Name field)
        is rejected:
    
    > TlsDxe:TlsCertVerify: verifying peer certificate with numerical IP address "fd33:eb1b:9b36::2"
    > TlsDxe:TlsCertVerify: peer certificate rejected
    > TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL
    > TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86
    
        If I do not apply the present patch on top of Jiaxin's v1 4/4 (at
        <http://mid.mail-archive.com/20190927034441.3096-5-Jiaxin.wu@intel.com>),
        then the certificate is accepted fine.
    
    Not sure how to address these.

 CryptoPkg/Library/TlsLib/TlsLib.inf       |   1 +
 CryptoPkg/Library/TlsLib/InternalTlsLib.h |  33 +++
 CryptoPkg/Library/TlsLib/TlsConfig.c      |  17 +-
 CryptoPkg/Library/TlsLib/TlsExData.c      | 301 ++++++++++++++++++++
 CryptoPkg/Library/TlsLib/TlsInit.c        |  35 ++-
 5 files changed, 385 insertions(+), 2 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index 2f3ce695c33e..1f65eea516d4 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -24,12 +24,13 @@ [Defines]
 
 [Sources]
   InternalTlsLib.h
   TlsInit.c
   TlsConfig.c
   TlsProcess.c
+  TlsExData.c
 
 [Packages]
   MdePkg/MdePkg.dec
   CryptoPkg/CryptoPkg.dec
 
 [LibraryClasses]
diff --git a/CryptoPkg/Library/TlsLib/InternalTlsLib.h b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
index ce7f4ced4a30..c8762befd31c 100644
--- a/CryptoPkg/Library/TlsLib/InternalTlsLib.h
+++ b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
@@ -34,8 +34,41 @@ typedef struct {
   //
   // Memory BIO for the TLS/SSL Writing operations.
   //
   BIO                             *OutBio;
 } TLS_CONNECTION;
 
+//
+// See the documentation for "mPeerSubjectNameKey",
+// TlsPeerSubjectNameDuplicate(), TlsPeerSubjectNameFree(), and TlsCertVerify()
+// in "TlsExData.c".
+//
+extern INT32 mPeerSubjectNameKey;
+
+INT32
+TlsPeerSubjectNameDuplicate (
+  OUT    CRYPTO_EX_DATA       *DestinationExData,
+  IN     CONST CRYPTO_EX_DATA *SourceExData,
+  IN OUT VOID                 *PeerSubjectNameAddress,
+  IN     INT32                ExDataType,
+  IN     long                 ArgLong,
+  IN     VOID                 *ArgPtr
+  );
+
+VOID
+TlsPeerSubjectNameFree (
+  IN VOID           *ParentSsl,
+  IN VOID           *PeerSubjectName OPTIONAL,
+  IN CRYPTO_EX_DATA *ExData,
+  IN INT32          ExDataType,
+  IN long           ArgLong,
+  IN VOID           *ArgPtr
+  );
+
+INT32
+TlsCertVerify (
+  IN X509_STORE_CTX *PeerCertificateChain,
+  IN VOID           *Arg
+  );
+
 #endif
 
diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index 2bf5aee7c093..114168dfb020 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -504,32 +504,47 @@ TlsSetVerify (
   @param[in]  Flags         The setting flags during the validation.
   @param[in]  HostName      The specified host name to be verified.
 
   @retval  EFI_SUCCESS           The HostName setting was set successfully.
   @retval  EFI_INVALID_PARAMETER The parameter is invalid.
   @retval  EFI_ABORTED           Invalid HostName setting.
+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failure.
 
 **/
 EFI_STATUS
 EFIAPI
 TlsSetVerifyHost (
   IN     VOID                     *Tls,
   IN     UINT32                   Flags,
   IN     CHAR8                    *HostName
   )
 {
   TLS_CONNECTION  *TlsConn;
+  CHAR8           *PeerSubjectName;
 
   TlsConn = (TLS_CONNECTION *) Tls;
   if (TlsConn == NULL || TlsConn->Ssl == NULL || HostName == NULL) {
      return EFI_INVALID_PARAMETER;
   }
 
+  PeerSubjectName = AllocateCopyPool (
+                      AsciiStrSize (HostName),
+                      HostName
+                      );
+  if (PeerSubjectName == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
   SSL_set_hostflags(TlsConn->Ssl, Flags);
 
-  if (SSL_set1_host(TlsConn->Ssl, HostName) == 0) {
+  if (SSL_set_ex_data (
+        TlsConn->Ssl,
+        mPeerSubjectNameKey,
+        PeerSubjectName
+        ) == 0) {
+    FreePool (PeerSubjectName);
     return EFI_ABORTED;
   }
 
   return EFI_SUCCESS;
 }
 
diff --git a/CryptoPkg/Library/TlsLib/TlsExData.c b/CryptoPkg/Library/TlsLib/TlsExData.c
new file mode 100644
index 000000000000..9671234f8416
--- /dev/null
+++ b/CryptoPkg/Library/TlsLib/TlsExData.c
@@ -0,0 +1,301 @@
+/** @file
+  OpenSSL callback functions for:
+
+  - duplicating and freeing the Peer Subject Name strings that we associate
+    with SSL objects as application data ("ExData"),
+
+  - verifying peer certificates against the Subject Name stings associated with
+    SSL objects.
+
+  Copyright (C) 2019, Red Hat, Inc.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include "InternalTlsLib.h"
+
+//
+// We attach the Subject Name that we expect the peer certificate to match to
+// the SSL object as an application-specific datum. This type of
+// application-specific data first needs to be registered with OpenSSL. The
+// registration identifier is stored in the object below.
+//
+// We define the associated data type as (CHAR8*), pointing to a
+// dynamically-allocated, NUL-terminated ASCII string. The string may contain a
+// DNS hostname, or an IPv4 address in dotted decimal notation, or an IPv6
+// address in colon-separated hexadecimal notation (without the surrounding
+// brackets used in URLs). The condensed "::" notation is supported for IPv6
+// addresses.
+//
+INT32 mPeerSubjectNameKey;
+
+/**
+  OpenSSL callback function for duplicating the Subject Name when its parent
+  SSL object is duplicated.
+
+  Because this function is an OpenSSL callback, it must not be declared EFIAPI.
+
+  @param[out] DestinationExData          The ExData object in the new SSL
+                                         object. DestinationExData is the
+                                         dictionary in which
+                                         mPeerSubjectNameKey identifies the new
+                                         (duplicated) subject name. Ignored.
+
+  @param[in] SourceExData                The ExData object in the original SSL
+                                         object. SourceExData is the dictionary
+                                         in which mPeerSubjectNameKey
+                                         identifies the subject name to
+                                         duplicate. Ignored.
+
+  @param[in,out] PeerSubjectNameAddress  On input,
+                                         *(VOID**)PeerSubjectNameAddress points
+                                         to the Subject Name in SourceExData.
+                                         On output,
+                                         *(VOID**)PeerSubjectNameAddress points
+                                         to the newly allocated copy of the
+                                         Subject Name, to be stored in
+                                         DestinationExData. On input,
+                                         PeerSubjectNameAddress must not be
+                                         NULL, but
+                                         *(VOID**)PeerSubjectNameAddress may be
+                                         NULL.
+
+  @param[in] ExDataType                  Equals mPeerSubjectNameKey. Ignored.
+
+  @param[in] ArgLong                     Zero; ignored.
+
+  @param[in] ArgPtr                      NULL; ignored.
+
+  @retval 0  Memory allocation failure.
+
+  @retval 1  Successful duplication (including a NULL subject name, when
+             nothing is done).
+**/
+INT32
+TlsPeerSubjectNameDuplicate (
+  OUT    CRYPTO_EX_DATA       *DestinationExData,
+  IN     CONST CRYPTO_EX_DATA *SourceExData,
+  IN OUT VOID                 *PeerSubjectNameAddress,
+  IN     INT32                ExDataType,
+  IN     long                 ArgLong,
+  IN     VOID                 *ArgPtr
+  )
+{
+  CHAR8 *PeerSubjectName;
+  CHAR8 *NewPeerSubjectName;
+
+  //
+  // Assert that these input parameters match what we passed to
+  // SSL_get_ex_new_index() in TlsInitialize().
+  //
+  ASSERT (ExDataType == mPeerSubjectNameKey);
+  ASSERT (ArgLong == 0);
+  ASSERT (ArgPtr == NULL);
+
+  //
+  // Further assert non-nullity for PeerSubjectNameAddress.
+  //
+  ASSERT (PeerSubjectNameAddress != NULL);
+
+  PeerSubjectName = *(VOID **)PeerSubjectNameAddress;
+  if (PeerSubjectName == NULL) {
+    DEBUG ((DEBUG_VERBOSE, "%a:%a: nothing to copy\n", gEfiCallerBaseName,
+      __FUNCTION__));
+    //
+    // Exit with success.
+    //
+    return 1;
+  }
+
+  NewPeerSubjectName = AllocateCopyPool (
+                         AsciiStrSize (PeerSubjectName),
+                         PeerSubjectName
+                         );
+  if (NewPeerSubjectName == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: failed to allocate memory\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    return 0;
+  }
+
+  *(VOID **)PeerSubjectNameAddress = NewPeerSubjectName;
+  DEBUG ((DEBUG_VERBOSE,
+    "%a:%a: copied peer subject name \"%a\" from %p to %p\n",
+    gEfiCallerBaseName, __FUNCTION__, PeerSubjectName, (VOID *)PeerSubjectName,
+    (VOID *)NewPeerSubjectName));
+  return 1;
+}
+
+/**
+  OpenSSL callback function for freeing the Subject Name when its parent SSL
+  object is freed.
+
+  Because this function is an OpenSSL callback, it must not be declared EFIAPI.
+
+  @param[in] ParentSsl        The parent SSL object being freed. Ignored.
+
+  @param[in] PeerSubjectName  The subject name to release. May be NULL.
+
+  @param[in] ExData           The ExData object in ParentSsl. ExData is the
+                              dictionary in which mPeerSubjectNameKey
+                              identifies the subject name to release. Ignored.
+
+  @param[in] ExDataType       Equals mPeerSubjectNameKey. Ignored.
+
+  @param[in] ArgLong          Zero; ignored.
+
+  @param[in] ArgPtr           NULL; ignored.
+**/
+VOID
+TlsPeerSubjectNameFree (
+  IN VOID           *ParentSsl,
+  IN VOID           *PeerSubjectName OPTIONAL,
+  IN CRYPTO_EX_DATA *ExData,
+  IN INT32          ExDataType,
+  IN long           ArgLong,
+  IN VOID           *ArgPtr
+  )
+{
+  //
+  // Assert that these input parameters match what we passed to
+  // SSL_get_ex_new_index() in TlsInitialize().
+  //
+  ASSERT (ExDataType == mPeerSubjectNameKey);
+  ASSERT (ArgLong == 0);
+  ASSERT (ArgPtr == NULL);
+
+  if (PeerSubjectName == NULL) {
+    return;
+  }
+
+  DEBUG ((DEBUG_VERBOSE, "%a:%a: freeing peer subject name \"%a\" at %p\n",
+    gEfiCallerBaseName, __FUNCTION__, (CHAR8 *)PeerSubjectName,
+    PeerSubjectName));
+  FreePool (PeerSubjectName);
+}
+
+/**
+  OpenSSL callback function for discovering and verifying the X509 peer
+  certificate chain during SSL/TLS handshake.
+
+  This function wraps the X509_verify_cert() OpenSSL function; it ensures that
+  both DNS host names and numeric IPv4/IPv6 addresses are matched in peer
+  certificates as Subject Names.
+
+  Because this function is an OpenSSL callback, it must not be declared EFIAPI.
+
+  @param[in] PeerCertificateChain  The certificate chain of the peer to verify.
+                                   The function checks whether
+                                   PeerCertificateChain matches the Peer
+                                   Subject Name that we've associated with the
+                                   SSL object of the network connection.
+
+  @param[in] Arg                   NULL; ignored.
+
+  @retval 1  Verification success.
+
+  @retval 0  Verification failure.
+**/
+INT32
+TlsCertVerify (
+  IN X509_STORE_CTX *PeerCertificateChain,
+  IN VOID           *Arg
+  )
+{
+  SSL               *Ssl;
+  X509_VERIFY_PARAM *VerifyParams;
+  CHAR8             *SubjectName;
+  INT32             ParamStatus;
+  INT32             VerifyStatus;
+
+  //
+  // Assert that these input parameters match what we passed to
+  // SSL_CTX_set_cert_verify_callback() in TlsCtxNew().
+  //
+  ASSERT (Arg == NULL);
+
+  //
+  // Retrieve the SSL object associated with the network connection for which
+  // the peer certificate is being verified in the SSL/TLS handshake.
+  //
+  Ssl = X509_STORE_CTX_get_ex_data (
+          PeerCertificateChain,
+          SSL_get_ex_data_X509_STORE_CTX_idx ()
+          );
+  if (Ssl == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: SSL object not found\n", gEfiCallerBaseName,
+      __FUNCTION__));
+    //
+    // Reject the certificate.
+    //
+    return 0;
+  }
+
+  //
+  // Fetch the certificate verification parameters.
+  //
+  VerifyParams = X509_STORE_CTX_get0_param (PeerCertificateChain);
+  if (VerifyParams == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: verification parameters not found\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    return 0;
+  }
+
+  //
+  // Retrieve the Peer Subject Name that we *may* have associated with the SSL
+  // object in TlsSetVerifyHost().
+  //
+  SubjectName = SSL_get_ex_data (Ssl, mPeerSubjectNameKey);
+  //
+  // If SubjectName is NULL or empty, explicitly clear the list of host names
+  // in VerifyParams, and perform no name checks on the peer certificate.
+  //
+  // Otherwise, attempt to parse the Peer Subject Name as an IPv4 or IPv6
+  // address. If this succeeds, then the parsed address is used for verifying
+  // the peer certificate.
+  //
+  // Otherwise, verify the peer certificate with SubjectName taken as a DNS
+  // hostname.
+  //
+  if (SubjectName == NULL || SubjectName[0] == '\0') {
+    ParamStatus = X509_VERIFY_PARAM_set1_host (VerifyParams, SubjectName, 0);
+
+    DEBUG ((DEBUG_WARN, "%a:%a: verifying peer certificate without subject "
+      "name check (MITM risk)!\n", gEfiCallerBaseName, __FUNCTION__));
+  } else {
+    ParamStatus = X509_VERIFY_PARAM_set1_ip_asc (VerifyParams, SubjectName);
+
+    if (ParamStatus == 1) {
+      DEBUG ((DEBUG_VERBOSE,
+        "%a:%a: verifying peer certificate with numerical IP address \"%a\"\n",
+        gEfiCallerBaseName, __FUNCTION__, SubjectName));
+    } else {
+      ParamStatus = X509_VERIFY_PARAM_set1_host (VerifyParams, SubjectName, 0);
+
+      DEBUG ((DEBUG_VERBOSE,
+        "%a:%a: verifying peer certificate with DNS hostname \"%a\"\n",
+        gEfiCallerBaseName, __FUNCTION__, SubjectName));
+    }
+  }
+
+  if (ParamStatus == 0) {
+    DEBUG ((DEBUG_ERROR,
+      "%a:%a: unexpected failure to set verification parameters\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    //
+    // Reject the certificate.
+    //
+    return 0;
+  }
+
+  VerifyStatus = X509_verify_cert (PeerCertificateChain);
+
+  if (VerifyStatus > 0) {
+    DEBUG ((DEBUG_VERBOSE, "%a:%a: peer certificate accepted\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    return 1;
+  }
+
+  DEBUG ((DEBUG_ERROR, "%a:%a: peer certificate rejected\n",
+    gEfiCallerBaseName, __FUNCTION__));
+  return 0;
+}
diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c
index f9ad6f6b946c..c7918364a4c7 100644
--- a/CryptoPkg/Library/TlsLib/TlsInit.c
+++ b/CryptoPkg/Library/TlsLib/TlsInit.c
@@ -24,29 +24,53 @@ BOOLEAN
 EFIAPI
 TlsInitialize (
   VOID
   )
 {
   INTN            Ret;
+  BOOLEAN         RandomIsSeeded;
 
   //
   // Performs initialization of crypto and ssl library, and loads required
   // algorithms.
   //
   Ret = OPENSSL_init_ssl (
           OPENSSL_INIT_LOAD_SSL_STRINGS | OPENSSL_INIT_LOAD_CRYPTO_STRINGS,
           NULL
           );
   if (Ret != 1) {
     return FALSE;
   }
+  //
+  // OPENSSL_init_ssl() cannot, and need not, be rolled back, if the rest of
+  // this function fails.
+  //
+
+  mPeerSubjectNameKey = SSL_get_ex_new_index (
+                          0,                           // "argl": unneeded
+                          NULL,                        // "argp": unneeded
+                          NULL,                        // "new_func": unneeded
+                          TlsPeerSubjectNameDuplicate, // "dup_func"
+                          TlsPeerSubjectNameFree       // "free_func"
+                          );
+  if (mPeerSubjectNameKey == -1) {
+    return FALSE;
+  }
 
   //
   // Initialize the pseudorandom number generator.
   //
-  return RandomSeed (NULL, 0);
+  RandomIsSeeded = RandomSeed (NULL, 0);
+  if (!RandomIsSeeded) {
+    goto DeregisterPeerSubjectName;
+  }
+  return TRUE;
+
+DeregisterPeerSubjectName:
+  CRYPTO_free_ex_index (CRYPTO_EX_INDEX_SSL, mPeerSubjectNameKey);
+  return FALSE;
 }
 
 /**
   Free an allocated SSL_CTX object.
 
   @param[in]  TlsCtx    Pointer to the SSL_CTX object to be released.
@@ -103,12 +127,21 @@ TlsCtxNew (
   //
   // Treat as minimum accepted versions by setting the minimal bound.
   // Client can use higher TLS version if server supports it
   //
   SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
 
+  //
+  // Set peer certificate verification procedure.
+  //
+  SSL_CTX_set_cert_verify_callback (
+    TlsCtx,
+    TlsCertVerify,
+    NULL           // "arg": unneeded
+    );
+
   return (VOID *) TlsCtx;
 }
 
 /**
   Free an allocated TLS object.
 
-- 
2.19.1.3.g30247aa5d201

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Wu, Jiaxin]

These days I'm too busy with other things. Just take the time to review the email discussion & researched the correct behavior of HTTPS cert verification. 

I did never though my patch caused the function regression, and I'm also not loop in the long ago email discussion reported from AMI. 

Now, I understand the testing *gap* between us is that we set the IP address string in the CN or dNSName filed of SAN when creating the certificate, while you don't prefer an IP address string setting in dNSName but setting the OCTET string to iPAddress of SAN. That's why the failure happen with different certificates setting (I've explained it many times). 

With my patches to UEFI part, I believe you can pass the test if setting the IP in CN or dNSName of SAN, but no one shared me any significant evidence to indicate it's the mistake idea if URI is specified as an IP address rather than a Hostname. On the contrary, my explains to my thought/viewpoint when creating the patch was treated as poor excuse:(.

Now, I post my own investigation here to correct my mistake thought:

According my previous experiment and RFC5280, the dNSName of subjectAltName is IA5String, which means the string of an IPv4 or IPv6 address is also legal to be included in the dNSName:
   SubjectAltName ::= GeneralNames
   GeneralName ::= CHOICE {
        otherName                       [0]     OtherName,
        rfc822Name                      [1]     IA5String,
        dNSName                         [2]     IA5String,
        x400Address                     [3]     ORAddress,
        directoryName                   [4]     Name,
        ediPartyName                    [5]     EDIPartyName,
        uniformResourceIdentifier       [6]     IA5String,
        iPAddress                       [7]     OCTET STRING,
        registeredID                    [8]     OBJECT IDENTIFIER }

If so, I thought it should be doable and make sense to treat the IP in URI as hostname in CN or dNSName of SAN. Besides, it can also pass the verification if the certificate only has the CN (NO SAN case, I think it's also the failure case Laszlo met in Note(2)). I believed it's the implementation choice before if no below finding in RFC 6125. But with the series discussion here, I'm starting to wonder whether there is any RFC or document to restrict the TLS & certificate checking roles , for example:
1) IP in the URI can be treated as Hostname or it only can be used for the iPAddress of SAN verification?
2) Certificate must have the iPAddress or dNSName of SAN?                                      

Fortunately, I get my wanted answer in RFC6125, SECTION 3.1 :

   If a subjectAltName extension of type dNSName is present, that MUST
   be used as the identity.  Otherwise, the (most specific) Common Name
   field in the Subject field of the certificate MUST be used.  Although
   the use of the Common Name is existing practice, it is deprecated and
   Certification Authorities are encouraged to use the dNSName instead.
   ...
   In some cases, the URI is specified as an IP address rather than a
   Hostname .  In this case, the iPAddress subjectAltName must be present
   in the certificate and must exactly match the IP in the URI.

So, the question is more clear here:
1) IP in the URI is used for the iPAddress of SAN verification, but *not say no* if upper driver set to dNSName of SAN.
2) iPAddress of SAN MUST be present in the certificate and must exactly match the IP in the URI. If not, we can treat as failure.

Ok, with above declaim in RFC, I agree there is the function issue in my series patches to treat IP address in URI as string setting to hostname,  which breaks the above 2 rules defined in RFC 6125.

Now, to resolve the problem, I think the best compatibility can actually be reached by setting the IP address both as iPAddress and dNSName in SAN. To achieve that, we have two methods:
1) TLS provides the separate interfaces to upper driver to set the iPAddress & dNSName (even email address), which means HTTPS driver need follow RFC6125. But that will need spec changes. 
2) Just as David's suggestion and the patch send out by Laszlo, fix it in UEFI TLS part. I reviewed the existing UEFI 2.8, looks we can treat the HostName in Spec as the widely name (can be IP, DNS or email address name). 

To make things simple & easier, I also prefer 2). So, I reviewed the patch from Laszlo:

Comment1: How about setting the IP address both as iPAddress and dNSName in SAN for the compatibility, if so, I think below note2 failure will be gone.
Comment2: do we really need the app_verify_callback function setting? Why not call X509_VERIFY_PARAM_set1_ip_asc (TlsConn->Ssl->param, HostName) in TlsSetVerifyHost directly? anything I missed in the discussion?

Please correct me if anything wrong.


Thanks,
Jiaxin



> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Wednesday, October 16, 2019 7:09 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>; David Woodhouse
> <dwmw2@infradead.org>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Jiaxin
> <jiaxin.wu@intel.com>; Richard Levitte <levitte@openssl.org>; Sivaraman
> Nainar <sivaramann@amiindia.co.in>
> Subject: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via
> both DNS names and IP addresses
> 
> SSL_set1_host() in TlsSetVerifyHost() ignores GEN_IP entries in the peer
> certificate's Subject Alternative Name (SAN) extension. This leads to the
> rejection of any valid peer certificate that matches the dot-decimal IPv4,
> or colon-hexadecimal IPv6, host part of an URL *only* through SAN/GEN_IP,
> and not through the Common Name.
> 
> Based on David's guidance, replace SSL_set1_host() in TlsSetVerifyHost()
> with application specific data ("ExData") that is associated with the SSL
> object. Namely, pass the host part of the URL as "application specific
> data" into a new peer certificate verification callback. In the callback,
> first try to parse the host part of the URL as a numeric IP address, for
> certificate subject verification. If that parsing fails, fall back to
> interpreting the host part as a DNS hostname.
> 
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Richard Levitte <levitte@openssl.org>
> Cc: Sivaraman Nainar <sivaramann@amiindia.co.in>
> Ref: http://mid.mail-
> archive.com/B4DE137BDB63634BAC03BD9DE765F197028B24CA23@VENUS1.i
> n.megatrends.com
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=960
> Ref: https://edk2.groups.io/g/devel/message/42022
> Suggested-by: David Woodhouse <dwmw2@infradead.org>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     Unfortunately, there are two problems with this patch:
> 
>     (1) X509_VERIFY_PARAM_set1_ip_asc() does not accept IPv4 addresses in
>         dot-decimal notation (unless I messed up the code). My log file
>         contains:
> 
>     > TlsDxe:TlsCertVerify: verifying peer certificate with DNS hostname
> "192.168.124.2"
>     > TlsDxe:TlsCertVerify: peer certificate accepted
> 
>     (2) X509_VERIFY_PARAM_set1_ip_asc() does accept IPv6 addresses.
> However,
>         in that case, the server certificate that I had generated with
>         "genkey" (where I entered the IPv6 address in the Common Name field)
>         is rejected:
> 
>     > TlsDxe:TlsCertVerify: verifying peer certificate with numerical IP address
> "fd33:eb1b:9b36::2"
>     > TlsDxe:TlsCertVerify: peer certificate rejected
>     > TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL
>     > TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86
> 
>         If I do not apply the present patch on top of Jiaxin's v1 4/4 (at
>         <http://mid.mail-archive.com/20190927034441.3096-5-
> Jiaxin.wu@intel.com>),
>         then the certificate is accepted fine.
> 
>     Not sure how to address these.
> 
>  CryptoPkg/Library/TlsLib/TlsLib.inf       |   1 +
>  CryptoPkg/Library/TlsLib/InternalTlsLib.h |  33 +++
>  CryptoPkg/Library/TlsLib/TlsConfig.c      |  17 +-
>  CryptoPkg/Library/TlsLib/TlsExData.c      | 301 ++++++++++++++++++++
>  CryptoPkg/Library/TlsLib/TlsInit.c        |  35 ++-
>  5 files changed, 385 insertions(+), 2 deletions(-)
> 
> diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf
> b/CryptoPkg/Library/TlsLib/TlsLib.inf
> index 2f3ce695c33e..1f65eea516d4 100644
> --- a/CryptoPkg/Library/TlsLib/TlsLib.inf
> +++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
> @@ -24,12 +24,13 @@ [Defines]
> 
>  [Sources]
>    InternalTlsLib.h
>    TlsInit.c
>    TlsConfig.c
>    TlsProcess.c
> +  TlsExData.c
> 
>  [Packages]
>    MdePkg/MdePkg.dec
>    CryptoPkg/CryptoPkg.dec
> 
>  [LibraryClasses]
> diff --git a/CryptoPkg/Library/TlsLib/InternalTlsLib.h
> b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
> index ce7f4ced4a30..c8762befd31c 100644
> --- a/CryptoPkg/Library/TlsLib/InternalTlsLib.h
> +++ b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
> @@ -34,8 +34,41 @@ typedef struct {
>    //
>    // Memory BIO for the TLS/SSL Writing operations.
>    //
>    BIO                             *OutBio;
>  } TLS_CONNECTION;
> 
> +//
> +// See the documentation for "mPeerSubjectNameKey",
> +// TlsPeerSubjectNameDuplicate(), TlsPeerSubjectNameFree(), and
> TlsCertVerify()
> +// in "TlsExData.c".
> +//
> +extern INT32 mPeerSubjectNameKey;
> +
> +INT32
> +TlsPeerSubjectNameDuplicate (
> +  OUT    CRYPTO_EX_DATA       *DestinationExData,
> +  IN     CONST CRYPTO_EX_DATA *SourceExData,
> +  IN OUT VOID                 *PeerSubjectNameAddress,
> +  IN     INT32                ExDataType,
> +  IN     long                 ArgLong,
> +  IN     VOID                 *ArgPtr
> +  );
> +
> +VOID
> +TlsPeerSubjectNameFree (
> +  IN VOID           *ParentSsl,
> +  IN VOID           *PeerSubjectName OPTIONAL,
> +  IN CRYPTO_EX_DATA *ExData,
> +  IN INT32          ExDataType,
> +  IN long           ArgLong,
> +  IN VOID           *ArgPtr
> +  );
> +
> +INT32
> +TlsCertVerify (
> +  IN X509_STORE_CTX *PeerCertificateChain,
> +  IN VOID           *Arg
> +  );
> +
>  #endif
> 
> diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c
> b/CryptoPkg/Library/TlsLib/TlsConfig.c
> index 2bf5aee7c093..114168dfb020 100644
> --- a/CryptoPkg/Library/TlsLib/TlsConfig.c
> +++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
> @@ -504,32 +504,47 @@ TlsSetVerify (
>    @param[in]  Flags         The setting flags during the validation.
>    @param[in]  HostName      The specified host name to be verified.
> 
>    @retval  EFI_SUCCESS           The HostName setting was set successfully.
>    @retval  EFI_INVALID_PARAMETER The parameter is invalid.
>    @retval  EFI_ABORTED           Invalid HostName setting.
> +  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failure.
> 
>  **/
>  EFI_STATUS
>  EFIAPI
>  TlsSetVerifyHost (
>    IN     VOID                     *Tls,
>    IN     UINT32                   Flags,
>    IN     CHAR8                    *HostName
>    )
>  {
>    TLS_CONNECTION  *TlsConn;
> +  CHAR8           *PeerSubjectName;
> 
>    TlsConn = (TLS_CONNECTION *) Tls;
>    if (TlsConn == NULL || TlsConn->Ssl == NULL || HostName == NULL) {
>       return EFI_INVALID_PARAMETER;
>    }
> 
> +  PeerSubjectName = AllocateCopyPool (
> +                      AsciiStrSize (HostName),
> +                      HostName
> +                      );
> +  if (PeerSubjectName == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
>    SSL_set_hostflags(TlsConn->Ssl, Flags);
> 
> -  if (SSL_set1_host(TlsConn->Ssl, HostName) == 0) {
> +  if (SSL_set_ex_data (
> +        TlsConn->Ssl,
> +        mPeerSubjectNameKey,
> +        PeerSubjectName
> +        ) == 0) {
> +    FreePool (PeerSubjectName);
>      return EFI_ABORTED;
>    }
> 
>    return EFI_SUCCESS;
>  }
> 
> diff --git a/CryptoPkg/Library/TlsLib/TlsExData.c
> b/CryptoPkg/Library/TlsLib/TlsExData.c
> new file mode 100644
> index 000000000000..9671234f8416
> --- /dev/null
> +++ b/CryptoPkg/Library/TlsLib/TlsExData.c
> @@ -0,0 +1,301 @@
> +/** @file
> +  OpenSSL callback functions for:
> +
> +  - duplicating and freeing the Peer Subject Name strings that we associate
> +    with SSL objects as application data ("ExData"),
> +
> +  - verifying peer certificates against the Subject Name stings associated
> with
> +    SSL objects.
> +
> +  Copyright (C) 2019, Red Hat, Inc.
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#include "InternalTlsLib.h"
> +
> +//
> +// We attach the Subject Name that we expect the peer certificate to match
> to
> +// the SSL object as an application-specific datum. This type of
> +// application-specific data first needs to be registered with OpenSSL. The
> +// registration identifier is stored in the object below.
> +//
> +// We define the associated data type as (CHAR8*), pointing to a
> +// dynamically-allocated, NUL-terminated ASCII string. The string may
> contain a
> +// DNS hostname, or an IPv4 address in dotted decimal notation, or an IPv6
> +// address in colon-separated hexadecimal notation (without the
> surrounding
> +// brackets used in URLs). The condensed "::" notation is supported for IPv6
> +// addresses.
> +//
> +INT32 mPeerSubjectNameKey;
> +
> +/**
> +  OpenSSL callback function for duplicating the Subject Name when its
> parent
> +  SSL object is duplicated.
> +
> +  Because this function is an OpenSSL callback, it must not be declared
> EFIAPI.
> +
> +  @param[out] DestinationExData          The ExData object in the new SSL
> +                                         object. DestinationExData is the
> +                                         dictionary in which
> +                                         mPeerSubjectNameKey identifies the new
> +                                         (duplicated) subject name. Ignored.
> +
> +  @param[in] SourceExData                The ExData object in the original SSL
> +                                         object. SourceExData is the dictionary
> +                                         in which mPeerSubjectNameKey
> +                                         identifies the subject name to
> +                                         duplicate. Ignored.
> +
> +  @param[in,out] PeerSubjectNameAddress  On input,
> +                                         *(VOID**)PeerSubjectNameAddress points
> +                                         to the Subject Name in SourceExData.
> +                                         On output,
> +                                         *(VOID**)PeerSubjectNameAddress points
> +                                         to the newly allocated copy of the
> +                                         Subject Name, to be stored in
> +                                         DestinationExData. On input,
> +                                         PeerSubjectNameAddress must not be
> +                                         NULL, but
> +                                         *(VOID**)PeerSubjectNameAddress may be
> +                                         NULL.
> +
> +  @param[in] ExDataType                  Equals mPeerSubjectNameKey. Ignored.
> +
> +  @param[in] ArgLong                     Zero; ignored.
> +
> +  @param[in] ArgPtr                      NULL; ignored.
> +
> +  @retval 0  Memory allocation failure.
> +
> +  @retval 1  Successful duplication (including a NULL subject name, when
> +             nothing is done).
> +**/
> +INT32
> +TlsPeerSubjectNameDuplicate (
> +  OUT    CRYPTO_EX_DATA       *DestinationExData,
> +  IN     CONST CRYPTO_EX_DATA *SourceExData,
> +  IN OUT VOID                 *PeerSubjectNameAddress,
> +  IN     INT32                ExDataType,
> +  IN     long                 ArgLong,
> +  IN     VOID                 *ArgPtr
> +  )
> +{
> +  CHAR8 *PeerSubjectName;
> +  CHAR8 *NewPeerSubjectName;
> +
> +  //
> +  // Assert that these input parameters match what we passed to
> +  // SSL_get_ex_new_index() in TlsInitialize().
> +  //
> +  ASSERT (ExDataType == mPeerSubjectNameKey);
> +  ASSERT (ArgLong == 0);
> +  ASSERT (ArgPtr == NULL);
> +
> +  //
> +  // Further assert non-nullity for PeerSubjectNameAddress.
> +  //
> +  ASSERT (PeerSubjectNameAddress != NULL);
> +
> +  PeerSubjectName = *(VOID **)PeerSubjectNameAddress;
> +  if (PeerSubjectName == NULL) {
> +    DEBUG ((DEBUG_VERBOSE, "%a:%a: nothing to copy\n",
> gEfiCallerBaseName,
> +      __FUNCTION__));
> +    //
> +    // Exit with success.
> +    //
> +    return 1;
> +  }
> +
> +  NewPeerSubjectName = AllocateCopyPool (
> +                         AsciiStrSize (PeerSubjectName),
> +                         PeerSubjectName
> +                         );
> +  if (NewPeerSubjectName == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a:%a: failed to allocate memory\n",
> +      gEfiCallerBaseName, __FUNCTION__));
> +    return 0;
> +  }
> +
> +  *(VOID **)PeerSubjectNameAddress = NewPeerSubjectName;
> +  DEBUG ((DEBUG_VERBOSE,
> +    "%a:%a: copied peer subject name \"%a\" from %p to %p\n",
> +    gEfiCallerBaseName, __FUNCTION__, PeerSubjectName, (VOID
> *)PeerSubjectName,
> +    (VOID *)NewPeerSubjectName));
> +  return 1;
> +}
> +
> +/**
> +  OpenSSL callback function for freeing the Subject Name when its parent
> SSL
> +  object is freed.
> +
> +  Because this function is an OpenSSL callback, it must not be declared
> EFIAPI.
> +
> +  @param[in] ParentSsl        The parent SSL object being freed. Ignored.
> +
> +  @param[in] PeerSubjectName  The subject name to release. May be NULL.
> +
> +  @param[in] ExData           The ExData object in ParentSsl. ExData is the
> +                              dictionary in which mPeerSubjectNameKey
> +                              identifies the subject name to release. Ignored.
> +
> +  @param[in] ExDataType       Equals mPeerSubjectNameKey. Ignored.
> +
> +  @param[in] ArgLong          Zero; ignored.
> +
> +  @param[in] ArgPtr           NULL; ignored.
> +**/
> +VOID
> +TlsPeerSubjectNameFree (
> +  IN VOID           *ParentSsl,
> +  IN VOID           *PeerSubjectName OPTIONAL,
> +  IN CRYPTO_EX_DATA *ExData,
> +  IN INT32          ExDataType,
> +  IN long           ArgLong,
> +  IN VOID           *ArgPtr
> +  )
> +{
> +  //
> +  // Assert that these input parameters match what we passed to
> +  // SSL_get_ex_new_index() in TlsInitialize().
> +  //
> +  ASSERT (ExDataType == mPeerSubjectNameKey);
> +  ASSERT (ArgLong == 0);
> +  ASSERT (ArgPtr == NULL);
> +
> +  if (PeerSubjectName == NULL) {
> +    return;
> +  }
> +
> +  DEBUG ((DEBUG_VERBOSE, "%a:%a: freeing peer subject name \"%a\"
> at %p\n",
> +    gEfiCallerBaseName, __FUNCTION__, (CHAR8 *)PeerSubjectName,
> +    PeerSubjectName));
> +  FreePool (PeerSubjectName);
> +}
> +
> +/**
> +  OpenSSL callback function for discovering and verifying the X509 peer
> +  certificate chain during SSL/TLS handshake.
> +
> +  This function wraps the X509_verify_cert() OpenSSL function; it ensures
> that
> +  both DNS host names and numeric IPv4/IPv6 addresses are matched in
> peer
> +  certificates as Subject Names.
> +
> +  Because this function is an OpenSSL callback, it must not be declared
> EFIAPI.
> +
> +  @param[in] PeerCertificateChain  The certificate chain of the peer to verify.
> +                                   The function checks whether
> +                                   PeerCertificateChain matches the Peer
> +                                   Subject Name that we've associated with the
> +                                   SSL object of the network connection.
> +
> +  @param[in] Arg                   NULL; ignored.
> +
> +  @retval 1  Verification success.
> +
> +  @retval 0  Verification failure.
> +**/
> +INT32
> +TlsCertVerify (
> +  IN X509_STORE_CTX *PeerCertificateChain,
> +  IN VOID           *Arg
> +  )
> +{
> +  SSL               *Ssl;
> +  X509_VERIFY_PARAM *VerifyParams;
> +  CHAR8             *SubjectName;
> +  INT32             ParamStatus;
> +  INT32             VerifyStatus;
> +
> +  //
> +  // Assert that these input parameters match what we passed to
> +  // SSL_CTX_set_cert_verify_callback() in TlsCtxNew().
> +  //
> +  ASSERT (Arg == NULL);
> +
> +  //
> +  // Retrieve the SSL object associated with the network connection for
> which
> +  // the peer certificate is being verified in the SSL/TLS handshake.
> +  //
> +  Ssl = X509_STORE_CTX_get_ex_data (
> +          PeerCertificateChain,
> +          SSL_get_ex_data_X509_STORE_CTX_idx ()
> +          );
> +  if (Ssl == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a:%a: SSL object not found\n",
> gEfiCallerBaseName,
> +      __FUNCTION__));
> +    //
> +    // Reject the certificate.
> +    //
> +    return 0;
> +  }
> +
> +  //
> +  // Fetch the certificate verification parameters.
> +  //
> +  VerifyParams = X509_STORE_CTX_get0_param (PeerCertificateChain);
> +  if (VerifyParams == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a:%a: verification parameters not found\n",
> +      gEfiCallerBaseName, __FUNCTION__));
> +    return 0;
> +  }
> +
> +  //
> +  // Retrieve the Peer Subject Name that we *may* have associated with
> the SSL
> +  // object in TlsSetVerifyHost().
> +  //
> +  SubjectName = SSL_get_ex_data (Ssl, mPeerSubjectNameKey);
> +  //
> +  // If SubjectName is NULL or empty, explicitly clear the list of host names
> +  // in VerifyParams, and perform no name checks on the peer certificate.
> +  //
> +  // Otherwise, attempt to parse the Peer Subject Name as an IPv4 or IPv6
> +  // address. If this succeeds, then the parsed address is used for verifying
> +  // the peer certificate.
> +  //
> +  // Otherwise, verify the peer certificate with SubjectName taken as a DNS
> +  // hostname.
> +  //
> +  if (SubjectName == NULL || SubjectName[0] == '\0') {
> +    ParamStatus = X509_VERIFY_PARAM_set1_host (VerifyParams,
> SubjectName, 0);
> +
> +    DEBUG ((DEBUG_WARN, "%a:%a: verifying peer certificate without
> subject "
> +      "name check (MITM risk)!\n", gEfiCallerBaseName, __FUNCTION__));
> +  } else {
> +    ParamStatus = X509_VERIFY_PARAM_set1_ip_asc (VerifyParams,
> SubjectName);
> +
> +    if (ParamStatus == 1) {
> +      DEBUG ((DEBUG_VERBOSE,
> +        "%a:%a: verifying peer certificate with numerical IP address \"%a\"\n",
> +        gEfiCallerBaseName, __FUNCTION__, SubjectName));
> +    } else {
> +      ParamStatus = X509_VERIFY_PARAM_set1_host (VerifyParams,
> SubjectName, 0);
> +
> +      DEBUG ((DEBUG_VERBOSE,
> +        "%a:%a: verifying peer certificate with DNS hostname \"%a\"\n",
> +        gEfiCallerBaseName, __FUNCTION__, SubjectName));
> +    }
> +  }
> +
> +  if (ParamStatus == 0) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a:%a: unexpected failure to set verification parameters\n",
> +      gEfiCallerBaseName, __FUNCTION__));
> +    //
> +    // Reject the certificate.
> +    //
> +    return 0;
> +  }
> +
> +  VerifyStatus = X509_verify_cert (PeerCertificateChain);
> +
> +  if (VerifyStatus > 0) {
> +    DEBUG ((DEBUG_VERBOSE, "%a:%a: peer certificate accepted\n",
> +      gEfiCallerBaseName, __FUNCTION__));
> +    return 1;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "%a:%a: peer certificate rejected\n",
> +    gEfiCallerBaseName, __FUNCTION__));
> +  return 0;
> +}
> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c
> b/CryptoPkg/Library/TlsLib/TlsInit.c
> index f9ad6f6b946c..c7918364a4c7 100644
> --- a/CryptoPkg/Library/TlsLib/TlsInit.c
> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c
> @@ -24,29 +24,53 @@ BOOLEAN
>  EFIAPI
>  TlsInitialize (
>    VOID
>    )
>  {
>    INTN            Ret;
> +  BOOLEAN         RandomIsSeeded;
> 
>    //
>    // Performs initialization of crypto and ssl library, and loads required
>    // algorithms.
>    //
>    Ret = OPENSSL_init_ssl (
>            OPENSSL_INIT_LOAD_SSL_STRINGS |
> OPENSSL_INIT_LOAD_CRYPTO_STRINGS,
>            NULL
>            );
>    if (Ret != 1) {
>      return FALSE;
>    }
> +  //
> +  // OPENSSL_init_ssl() cannot, and need not, be rolled back, if the rest of
> +  // this function fails.
> +  //
> +
> +  mPeerSubjectNameKey = SSL_get_ex_new_index (
> +                          0,                           // "argl": unneeded
> +                          NULL,                        // "argp": unneeded
> +                          NULL,                        // "new_func": unneeded
> +                          TlsPeerSubjectNameDuplicate, // "dup_func"
> +                          TlsPeerSubjectNameFree       // "free_func"
> +                          );
> +  if (mPeerSubjectNameKey == -1) {
> +    return FALSE;
> +  }
> 
>    //
>    // Initialize the pseudorandom number generator.
>    //
> -  return RandomSeed (NULL, 0);
> +  RandomIsSeeded = RandomSeed (NULL, 0);
> +  if (!RandomIsSeeded) {
> +    goto DeregisterPeerSubjectName;
> +  }
> +  return TRUE;
> +
> +DeregisterPeerSubjectName:
> +  CRYPTO_free_ex_index (CRYPTO_EX_INDEX_SSL,
> mPeerSubjectNameKey);
> +  return FALSE;
>  }
> 
>  /**
>    Free an allocated SSL_CTX object.
> 
>    @param[in]  TlsCtx    Pointer to the SSL_CTX object to be released.
> @@ -103,12 +127,21 @@ TlsCtxNew (
>    //
>    // Treat as minimum accepted versions by setting the minimal bound.
>    // Client can use higher TLS version if server supports it
>    //
>    SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
> 
> +  //
> +  // Set peer certificate verification procedure.
> +  //
> +  SSL_CTX_set_cert_verify_callback (
> +    TlsCtx,
> +    TlsCertVerify,
> +    NULL           // "arg": unneeded
> +    );
> +
>    return (VOID *) TlsCtx;
>  }
> 
>  /**
>    Free an allocated TLS object.
> 
> --
> 2.19.1.3.g30247aa5d201
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#49034): https://edk2.groups.io/g/devel/message/49034
> Mute This Topic: https://groups.io/mt/34551672/1787330
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [jiaxin.wu@intel.com]
> -=-=-=-=-=-=

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

Hi Jiaxin,

On 10/16/19 07:18, Wu, Jiaxin wrote:
> These days I'm too busy with other things. Just take the time to
> review the email discussion & researched the correct behavior of HTTPS
> cert verification.
>
> I did never though my patch caused the function regression, and I'm
> also not loop in the long ago email discussion reported from AMI.
>
> Now, I understand the testing *gap* between us is that we set the IP
> address string in the CN or dNSName filed of SAN when creating the
> certificate, while you don't prefer an IP address string setting in
> dNSName but setting the OCTET string to iPAddress of SAN. That's why
> the failure happen with different certificates setting (I've explained
> it many times).

You have misunderstood.

It's not my *preference* to set an "OCTET string in the SAN iPAddress".

It is so much *not* my preference that I didn't even know how to
generate such a server certificate until David showed me!

I gave Tested-by for your patch set because my personal validation
mapped cleanly to your validation. In fact I think your validation was
more extensive than mine, and so my testing was a *proper subset* of
yours.

But, that's not the whole story. When these patches were first exposed
to the infosec group in TianoCore#960, in June, AMI reported a practical
regression with them, to the list. Unfortunately, at that time, that
regression report didn't reach you (it should have been made in the BZ,
not on the list, because the patches were also attached to the BZ -- at
that time, anyway).

Therefore, we have to figure out what to do about the regression
encountered by AMI. We have two options:

- We can call their use case "invalid", assuming we can prove this from
  the UEFI spec.

- We can attempt to accommodate their use case.

The present patch is an *attempt* to accommodate their use case, based
on David's guidance. Unfortunately, it does not work. (See the "Notes"
section in the patch for the symptoms.) So we can decide to stop
pursuing this use case, or we can continue researching it. We can *also*
call AMI's use case invalid still, if we can prove that from the UEFI
spec.

To me, AMI's use case looked valid (= covered under the UEFI spec), but
that's just me. We can contact the USWG for help with the spec
interpretation.


> With my patches to UEFI part, I believe you can pass the test if
> setting the IP in CN or dNSName of SAN,

Yes, I fully agree about that. That's why I gave Tested-by earlier in
this thread, for your patches 1-4. Please see:

  http://mid.mail-archive.com/6939ba4e-6c77-0769-4ac2-c3ba1ea9a0b7@redhat.com
  https://edk2.groups.io/g/devel/message/48813


> but no one shared me any significant evidence to indicate it's the
> mistake idea if URI is specified as an IP address rather than a
> Hostname.

I have not shared such evidence because I do not *believe* that storing
the IP address in the Common Name of the certificate is a mistake.

The "genkey" utility that Red Hat recommends (in the official docs) for
setting up mod_ssl with Apache places the IP address in the Common Name,
if the user enters an IP address. David argues that this is wrong, but
I'm not convinced just yet.

Therefore, I agree that your v1 patches 1-4 cover a valid use case.

The issue is that they break a *different* use case, which *may* also be
valid. (Dependent on how we interpret the UEFI spec.)


> On the contrary, my explains to my thought/viewpoint when creating the
> patch was treated as poor excuse:(.

Not by me, for sure.


> Now, I post my own investigation here to correct my mistake thought:
>
> According my previous experiment and RFC5280, the dNSName of
> subjectAltName is IA5String, which means the string of an IPv4 or IPv6
> address is also legal to be included in the dNSName:
>    SubjectAltName ::= GeneralNames
>    GeneralName ::= CHOICE {
>         otherName                       [0]     OtherName,
>         rfc822Name                      [1]     IA5String,
>         dNSName                         [2]     IA5String,
>         x400Address                     [3]     ORAddress,
>         directoryName                   [4]     Name,
>         ediPartyName                    [5]     EDIPartyName,
>         uniformResourceIdentifier       [6]     IA5String,
>         iPAddress                       [7]     OCTET STRING,
>         registeredID                    [8]     OBJECT IDENTIFIER }
>
> If so, I thought it should be doable and make sense to treat the IP in
> URI as hostname in CN or dNSName of SAN. Besides, it can also pass the
> verification if the certificate only has the CN (NO SAN case, I think
> it's also the failure case Laszlo met in Note(2)). I believed it's the
> implementation choice before if no below finding in RFC 6125. But with
> the series discussion here, I'm starting to wonder whether there is
> any RFC or document to restrict the TLS & certificate checking roles ,
> for example:
> 1) IP in the URI can be treated as Hostname or it only can be used for
> the iPAddress of SAN verification?
> 2) Certificate must have the iPAddress or dNSName of SAN?
>
> Fortunately, I get my wanted answer in RFC6125, SECTION 3.1 :
>
>    If a subjectAltName extension of type dNSName is present, that MUST
>    be used as the identity.  Otherwise, the (most specific) Common
>    Name field in the Subject field of the certificate MUST be used.
>    Although the use of the Common Name is existing practice, it is
>    deprecated and Certification Authorities are encouraged to use the
>    dNSName instead.
>    ...
>    In some cases, the URI is specified as an IP address rather than a
>    Hostname .  In this case, the iPAddress subjectAltName must be
>    present in the certificate and must exactly match the IP in the
>    URI.

Wow!

This seems to prove David right, and it suggests that symptom (2)
encountered with my patch is actually *valid* behavior -- the
certificate I generated with "genkey" is *not* valid for a URI that
specifies an IP address!

This is not good news: the "curl" utility also accepts such a
certificate as valid (IP address in URL, but the certificate only
contains a CN,  matching the IP address in textual form). Is that a bug
in "curl" then?


> So, the question is more clear here:
> 1) IP in the URI is used for the iPAddress of SAN verification, but
> *not say no* if upper driver set to dNSName of SAN.

I'm not sure what you mean by "not say no if upper driver set to dNSName
of SAN".

Do you mean that we can ignore the IP address in the SAN if the URL
contains a hostname? (And then the hostname matches either the dNSName
in the SAN (high prio), or the CN (low prio)?) I agree about that.


> 2) iPAddress of SAN MUST be present in the certificate and must
> exactly match the IP in the URI. If not, we can treat as failure.

Yes, I'm compelled to agree.


> Ok, with above declaim in RFC, I agree there is the function issue in
> my series patches to treat IP address in URI as string setting to
> hostname,  which breaks the above 2 rules defined in RFC 6125.
>
> Now, to resolve the problem, I think the best compatibility can
> actually be reached by setting the IP address both as iPAddress and
> dNSName in SAN.

Wait, I don't understand the word "setting". "Setting" is an activity
that the server admin performs. The UEFI TLS code is on the client side,
and has no control over the elements of the certificate.

Do you mean: the best compatibility can be reached by *matching* the IP
address (from the URL) in both SAN.iPAddress and SAN.dNSName?

I think I disagree with that. The part you quoted above from the RFC
states that an IP-based URL can only be accepted via SAN.iPAddress.


> To achieve that, we have two methods:
> 1) TLS provides the separate interfaces to upper driver to set the
> iPAddress & dNSName (even email address), which means HTTPS driver
> need follow RFC6125. But that will need spec changes.

I think this is not a good approach.

David's pull request for OpenSSL, namely
<https://github.com/openssl/openssl/pull/9201>, makes the argument that
recognizing and prioritizing an IP address over a DNS hostname should be
*internal* to OpenSSL itself.

  https://github.com/openssl/openssl/pull/9201/commits/da280b92520f5a0e41efb777a4d39bc907c42ecf

> diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
> index c481e292fc88..393e1a704d2e 100644
> --- a/ssl/ssl_lib.c
> +++ b/ssl/ssl_lib.c
> @@ -965,11 +965,21 @@ int SSL_set_trust(SSL *s, int trust)
>
>  int SSL_set1_host(SSL *s, const char *hostname)
>  {
> +    /* If a hostname is provided and parses as an IP address,
> +     * treat it as such. */
> +    if (hostname && X509_VERIFY_PARAM_set1_ip_asc(s->param, hostname) == 1)
> +        return 1;
> +
>      return X509_VERIFY_PARAM_set1_host(s->param, hostname, 0);
>  }
>
>  int SSL_add1_host(SSL *s, const char *hostname)
>  {
> +    /* If a hostname is provided and parses as an IP address,
> +     * treat it as such. */
> +    if (hostname && X509_VERIFY_PARAM_set1_ip_asc(s->param, hostname) == 1)
> +        return 1;
> +
>      return X509_VERIFY_PARAM_add1_host(s->param, hostname, 0);
>  }
>

Therefore we should not burden the HTTPS driver in edk2 with this; and
we should even less complicate the UEFI spec with it.

Back to your email:


On 10/16/19 07:18, Wu, Jiaxin wrote:
> 2) Just as David's suggestion and the patch send out by Laszlo, fix it
> in UEFI TLS part. I reviewed the existing UEFI 2.8, looks we can treat
> the HostName in Spec as the widely name (can be IP, DNS or email
> address name).

I don't understand how email enters the picture, but otherwise I agree.


> To make things simple & easier, I also prefer 2). So, I reviewed the
> patch from Laszlo:
>
> Comment1: How about setting the IP address both as iPAddress and
> dNSName in SAN for the compatibility, if so, I think below note2
> failure will be gone.

See above -- failure (2) appears valid, based on the part you quoted
from the RFC. I'll have to generate a better certificate.

Also, curl appears wrong to accept the certificate. (I'm stunned by
this!)


> Comment2: do we really need the app_verify_callback function setting?
> Why not call X509_VERIFY_PARAM_set1_ip_asc (TlsConn->Ssl->param,
> HostName) in TlsSetVerifyHost directly? anything I missed in the
> discussion?

I don't think client code should access "Ssl->param" directly. SSL
should be treated as an opaque data structure.

However, I think you may have a point. Formally, the SSL_get0_param()
function could be called to retrieve X509_VERIFY_PARAM.

  https://www.openssl.org/docs/man1.1.1/man3/SSL_get0_param.html

And then we could call X509_VERIFY_PARAM_set1_ip_asc() on that, perhaps.
This would make both the ExData stuff and the custom certificate
verification procedure unnecessary.

TBH, I'm quite confused by the OpenSSL API: after all, just *what* is
the object that X509_VERIFY_PARAM is associated with? Does it belong to
the SSL object, or to the peer certificate (X509_STORE_CTX)?

Thank you Jiaxin for following up!
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

Here's an extra point:

On 10/16/19 09:36, Laszlo Ersek wrote:
> On 10/16/19 07:18, Wu, Jiaxin wrote:

>> Fortunately, I get my wanted answer in RFC6125, SECTION 3.1 :
>>
>>    If a subjectAltName extension of type dNSName is present, that MUST
>>    be used as the identity.  Otherwise, the (most specific) Common
>>    Name field in the Subject field of the certificate MUST be used.
>>    Although the use of the Common Name is existing practice, it is
>>    deprecated and Certification Authorities are encouraged to use the
>>    dNSName instead.
>>    ...
>>    In some cases, the URI is specified as an IP address rather than a
>>    Hostname .  In this case, the iPAddress subjectAltName must be
>>    present in the certificate and must exactly match the IP in the
>>    URI.
> 
> Wow!
> 
> This seems to prove David right, and it suggests that symptom (2)
> encountered with my patch is actually *valid* behavior -- the
> certificate I generated with "genkey" is *not* valid for a URI that
> specifies an IP address!
> 
> This is not good news: the "curl" utility also accepts such a
> certificate as valid (IP address in URL, but the certificate only
> contains a CN,  matching the IP address in textual form). Is that a bug
> in "curl" then?

Check this out:

https://wiki.openssl.org/index.php/Hostname_validation

(The latest version of the article, which I'm looking at now, is
<https://wiki.openssl.org/index.php?title=Hostname_validation&oldid=2665>.)

This wiki article on the openssl website explains how to do hostname
validation, and it also quotes "curl" code.

Notice that it does not consider IP address validation specifically. The
example code for OpenSSL 1.1.0 only calls SSL_set1_host().

And, the code quoted from "curl" seems to suffer from two issues:

- matches_subject_alternative_name() ignores GEN_IP -- it only compares
the host part of the URL against GEN_DNS entries

- if matches_subject_alternative_name() fails, matches_common_name() can
still accept the certificate, even if the URL specified a numeric IP
address.

How are we supposed to get this right if even the reference code in the
wiki appears wrong (per RFC6125)?

Anyway: once David's PR#9201 is merged into OpenSSL, the example code in
the wiki page that calls SSL_set1_host() will become right.

Thanks
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[David Woodhouse]

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

On Wed, 2019-10-16 at 09:54 +0200, Laszlo Ersek wrote:
> Anyway: once David's PR#9201 is merged into OpenSSL, the example code in
> the wiki page that calls SSL_set1_host() will become right.

Crypto libraries should make it hard for users to get it wrong.

Film at 11.




[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

On 10/16/19 09:36, Laszlo Ersek wrote:
> On 10/16/19 07:18, Wu, Jiaxin wrote:

>> Comment2: do we really need the app_verify_callback function setting?
>> Why not call X509_VERIFY_PARAM_set1_ip_asc (TlsConn->Ssl->param,
>> HostName) in TlsSetVerifyHost directly? anything I missed in the
>> discussion?
> 
> I don't think client code should access "Ssl->param" directly. SSL
> should be treated as an opaque data structure.
> 
> However, I think you may have a point. Formally, the SSL_get0_param()
> function could be called to retrieve X509_VERIFY_PARAM.
> 
>   https://www.openssl.org/docs/man1.1.1/man3/SSL_get0_param.html
> 
> And then we could call X509_VERIFY_PARAM_set1_ip_asc() on that, perhaps.
> This would make both the ExData stuff and the custom certificate
> verification procedure unnecessary.

To be clear, this is meant to say:

Jiaxin, I'm totally fine to drop "RFC v1 5/4".

Just please post a v2, and use

- SSL_get0_param()
- X509_VERIFY_PARAM_set1_ip_asc()
- X509_VERIFY_PARAM_set1_host()

rather than

  SSL_set1_host()

in patch v2 2/4 -- if it works, that is! :)

Thanks!
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[David Woodhouse]

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

On Wed, 2019-10-16 at 05:18 +0000, Wu, Jiaxin wrote:
> Fortunately, I get my wanted answer in RFC6125, SECTION 3.1 :
> 
>    If a subjectAltName extension of type dNSName is present, that MUST
>    be used as the identity.  Otherwise, the (most specific) Common Name
>    field in the Subject field of the certificate MUST be used.  Although
>    the use of the Common Name is existing practice, it is deprecated and
>    Certification Authorities are encouraged to use the dNSName instead.
>    ...
>    In some cases, the URI is specified as an IP address rather than a
>    Hostname .  In this case, the iPAddress subjectAltName must be present
>    in the certificate and must exactly match the IP in the URI.
> 
> So, the question is more clear here:
> 1) IP in the URI is used for the iPAddress of SAN verification, but *not say no* if upper driver set to dNSName of SAN.
> 2) iPAddress of SAN MUST be present in the certificate and must exactly match the IP in the URI. If not, we can treat as failure.

Good find. 

RFC5280 §4.2.1.6 is a bit poorly drafted here; I've filed an erratum.

It's very prescriptive about email addresses and DNS names, clearly
starting that those MUST be stored in rfc822Name or dNSName form
respectively:

   When the subjectAltName extension contains an Internet mail address,
   the address MUST be stored in the rfc822Name. 
...
   When the subjectAltName extension contains a domain name system
   label, the domain name MUST be stored in the dNSName…

But it seems less clear about IP addresses, merely saying that *if* you
choose to use the iPAddress form, it must be big-endian:

   When the subjectAltName extension contains an iPAddress, the address
   MUST be stored in the octet string in "network byte order"…

I think that's a drafting error though, and it did mean to say that IP
addresses MUST be represented as an iPAddress.

It does go on to make it clear that the dNSName isn't just any old
IA5String, but must be in the "preferred name syntax", with explicit
reference to RFC1034 §3.5. So no, you can't stick a textual
representation of an IP address in there, even if you're going to
completely ignore how broken that would be because even Legacy IP
addresses can be represented in non-canonical forms.

Either way, your find in RFC6125 makes it clear that implementations
MUST NOT accept certificates which have only a textual representation
of the IP address in their Subject CN or a dNSName SAN, when the IP
address has been used in the URI.

IP addresses are IP addresses. Domain names are domain names.

In X.509 you never compare one to the other.

If your caller gives you a "hostname" which is actually an IP literal,
it only makes sense to compare that with an iPAddress field in the SAN,
never anything else.



[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[David Woodhouse]

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

On Wed, 2019-10-16 at 09:36 +0200, Laszlo Ersek wrote:
> On 10/16/19 07:18, Wu, Jiaxin wrote:
> >    In some cases, the URI is specified as an IP address rather than a
> >    Hostname .  In this case, the iPAddress subjectAltName must be
> >    present in the certificate and must exactly match the IP in the
> >    URI.
> 
> Wow!
> 
> This seems to prove David right, and it suggests that symptom (2)
> encountered with my patch is actually *valid* behavior -- the
> certificate I generated with "genkey" is *not* valid for a URI that
> specifies an IP address!
> 
> This is not good news: the "curl" utility also accepts such a
> certificate as valid (IP address in URL, but the certificate only
> contains a CN,  matching the IP address in textual form). Is that a bug
> in "curl" then?

Yes, I believe so.
Please report it at https://github.com/curl/curl/issues


> > To achieve that, we have two methods:
> > 1) TLS provides the separate interfaces to upper driver to set the
> > iPAddress & dNSName (even email address), which means HTTPS driver
> > need follow RFC6125. But that will need spec changes.
> 
> I think this is not a good approach.

> David's pull request for OpenSSL, namely
> <https://github.com/openssl/openssl/pull/9201>, makes the argument that
> recognizing and prioritizing an IP address over a DNS hostname should be
> *internal* to OpenSSL itself.
> 
>   https://github.com/openssl/openssl/pull/9201/commits/da280b92520f5a0e41efb777a4d39bc907c42ecf


Indeed; I think we all agree here. I believe it was presented as a
straw man option just for completeness and even Jiaxin didn't seriously
intend that we'd do it.

But for the record, let me make it abundantly clear how wrong this
option would be...

I will *always* make the argument that crypto libraries should make it
trivial for users to get things like this right. I'm dismayed that we
even have to have this discussion. This stuff should Just Work™.

That applies just as much to the API that you present from the TlsLib.

If you give your users a function that takes a hostname and silently
does a completely bogus wrong thing when it's passed an IP address
instead, that's a *bad* API design.

Forcing users to work it out for themselves and call a *different*
function if they happen to have an IP address, and still doing that
bogus wrong thing if they call the original function, is just
compounding the error.

And yes, I'll argue quite strongly that OpenSSL shouldn't do that.

OpenSSL did it because of the way its API evolved, and I will continue
to have the discussion with them upstream about not designing APIs that
set their users up for failure. It is an ongoing theme.

But for UEFI? To even *propose* that we make such an appalling design
choice in TlsLib?

Just NO. Don't even contemplate it.

As noted above, I choose to believe that it was only being suggested as
a straw man, and only for completeness. I just wanted to make it
*absolutely* clear that it was entirely unacceptable.

We should apply that "don't set your users up for failure" principle
everywhere.

I will strongly assert, for example, that TlsLib shouldn't make the
same mistake in its API design that led to CVE-2019-14553 in the first
place. If a caller "forgets" to specify the hostname (or IP) that
they're connecting to, the connection should FAIL. It shouldn't default
to just connecting to anything. That's just batshit insane.

If you have to fix the UEFI spec to make setting the hostname
mandatory, then do so.

Don't set your users up for failure by design.


> > Comment2: do we really need the app_verify_callback function setting?
> > Why not call X509_VERIFY_PARAM_set1_ip_asc (TlsConn->Ssl->param,
> > HostName) in TlsSetVerifyHost directly? anything I missed in the
> > discussion?
> 
> I don't think client code should access "Ssl->param" directly. SSL
> should be treated as an opaque data structure.
> 
> However, I think you may have a point. Formally, the SSL_get0_param()
> function could be called to retrieve X509_VERIFY_PARAM.
> 
>   https://www.openssl.org/docs/man1.1.1/man3/SSL_get0_param.html
> 
> And then we could call X509_VERIFY_PARAM_set1_ip_asc() on that, perhaps.
> This would make both the ExData stuff and the custom certificate
> verification procedure unnecessary.

Haha, I had *looked* for that but somehow failed to find it. I can't
for the life of me work out how I missed it, now.

Yes, that would be a whole lot easier than the callback and ex_data
nonsense. It's really just as simple as

 X509_VERIFY_PARAM *vpm = SSL_get0_param(ssl);

 if (!X509_VERIFY_PARAM_set_ip_asc(vpm, hostname))
     /* Didn't like that, must be an actual hostname then */
     X509_VERIFY_PARAM_set1_host(vpm, hostname, 0);


So what did we say about false modesty, Laszlo?

In the end you did actually solve it all for yourself — based on the
pointer I'd given in bugzilla, and then ignoring my subsequent
misdirection about callbacks and my overly complex attempt at doing it
myself :)





[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[David Woodhouse]

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

On Tue, 2019-10-15 at 19:34 +0200, Laszlo Ersek wrote:
> Ehh, I failed to ask the actual question.
> 
> Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically,
> every time just before we call X509_verify_cert()?
> 
> My concern is not with the crypto functionality, but whether we could be
> leaking memory allocations.

You had to ask yourself that before approving the original version of
TlsSetVerifyHost(), didn't you? Because the TlsLib API hasn't imposed
any restriction on calling TlsSetVerifyHost() more than once...

The answer is yes, btw — it's fine. 

Note also my observation that we should insist on TlsSetVerifyHost
being called at *least* once, or the connection should fail.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

[Laszlo Ersek]

On 10/16/19 11:40, David Woodhouse wrote:
> On Tue, 2019-10-15 at 19:34 +0200, Laszlo Ersek wrote:
>> Ehh, I failed to ask the actual question.
>>
>> Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically,
>> every time just before we call X509_verify_cert()?
>>
>> My concern is not with the crypto functionality, but whether we could be
>> leaking memory allocations.
> 
> You had to ask yourself that before approving the original version of
> TlsSetVerifyHost(), didn't you? Because the TlsLib API hasn't imposed
> any restriction on calling TlsSetVerifyHost() more than once...

You are correct, of course. I seem to recall that I hand-waved that
question away, seeing that TlsSetVerifyHost() simply passed the hostname
(the pointer to the char array) into an OpenSSL API. I guess when I
first looked at that call with any kind of focus, I wasn't *that*
concerned about the life-cycle yet...

> 
> The answer is yes, btw — it's fine. 

Thanks!

> 
> Note also my observation that we should insist on TlsSetVerifyHost
> being called at *least* once, or the connection should fail.
> 

I wonder if we could make this an implementation detail in edk2 *first*,
while a matching USWG Mantis ticket were in progress.

Thanks
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[David Woodhouse]

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

On Wed, 2019-10-16 at 01:08 +0200, Laszlo Ersek wrote:
> Notes:
>     Unfortunately, there are two problems with this patch:
>     
>     (1) X509_VERIFY_PARAM_set1_ip_asc() does not accept IPv4 addresses in
>         dot-decimal notation (unless I messed up the code). My log file
>         contains:
>     
>     > TlsDxe:TlsCertVerify: verifying peer certificate with DNS hostname "192.168.124.2"
>     > TlsDxe:TlsCertVerify: peer certificate accepted


That's odd; X509_VERIFY_PARAM_set1_ip_asc() shouldn't have failed on
that string.

I recommend staring very hard at the implementation of ipv4_from_asc()
in openssl/crypto/x509/v3_utl.c. Especially the 'scanf' part.

This one's going to bite you even when ditch my overly complex set of
callbacks and ex_data, and just use SSL_get0_param() sensibly.

>     (2) X509_VERIFY_PARAM_set1_ip_asc() does accept IPv6 addresses. However,
>         in that case, the server certificate that I had generated with
>         "genkey" (where I entered the IPv6 address in the Common Name field)
>         is rejected:
>     
>     > TlsDxe:TlsCertVerify: verifying peer certificate with numerical IP address "fd33:eb1b:9b36::2"
>     > TlsDxe:TlsCertVerify: peer certificate rejected
>     > TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL
>     > TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86
>     
>         If I do not apply the present patch on top of Jiaxin's v1 4/4 (at
>         <http://mid.mail-archive.com/20190927034441.3096-5-Jiaxin.wu@intel.com>),
>         then the certificate is accepted fine.

As I believe you have already concluded, this is correct behaviour.

That certificate is bogus, because your 'genkey' tool is not behaving
correctly (qv) and should have produced a cert with an iPAddress in its
SAN. The bogus certificate is rightly being rejected after your patch.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

On 10/16/19 11:19, David Woodhouse wrote:
> On Wed, 2019-10-16 at 09:36 +0200, Laszlo Ersek wrote:
>> On 10/16/19 07:18, Wu, Jiaxin wrote:
>>>    In some cases, the URI is specified as an IP address rather than
>>>    a Hostname .  In this case, the iPAddress subjectAltName must be
>>>    present in the certificate and must exactly match the IP in the
>>>    URI.
>>
>> Wow!
>>
>> This seems to prove David right, and it suggests that symptom (2)
>> encountered with my patch is actually *valid* behavior -- the
>> certificate I generated with "genkey" is *not* valid for a URI that
>> specifies an IP address!
>>
>> This is not good news: the "curl" utility also accepts such a
>> certificate as valid (IP address in URL, but the certificate only
>> contains a CN,  matching the IP address in textual form). Is that a
>> bug in "curl" then?
>
> Yes, I believe so.
> Please report it at https://github.com/curl/curl/issues

<https://github.com/curl/curl/issues/new> directs me to
<https://hackerone.com/curl>, for security-related issues, so I've now
registered on that site, and started writing up a report.

However. When I wanted to include a reference to RFC6125, I looked more
closely at the section that Jiaxin quoted. More specifically, the
surroundings of that section.

It turns out that the quoted section "3.1. Server Identity" is actually
part of Appendix B.2 ("HTTP (2000)"):

  https://tools.ietf.org/html/rfc6125#appendix-B.2

which is further part of Appendix B ("Prior Art"):

  https://tools.ietf.org/html/rfc6125#appendix-B

Appendix B starts with the statement

> (This section is non-normative.)

and Appendix B.2 starts with

>    In 2000, [HTTP-TLS] specified the following text regarding
>    application service identity verification in HTTP:

Thus, Appendix B of RFC6125 doesn't require us to do anything! (Other
specs may still require us, of course.)

Now, does RFC6125 say anything about IP addresses? Yes, it does, in
section 1.7.2, "Out of Scope":

https://tools.ietf.org/html/rfc6125#section-1.7.2

>    o  Identifiers other than fully qualified DNS domain names.
>
>       Some certification authorities issue server certificates based on
>       IP addresses, but preliminary evidence indicates that such
>       certificates are a very small percentage (less than 1%) of issued
>       certificates.  Furthermore, IP addresses are not necessarily
>       reliable identifiers for application services because of the
>       existence of private internets [PRIVATE], host mobility, multiple
>       interfaces on a given host, Network Address Translators (NATs)
>       resulting in different addresses for a host from different
>       locations on the network, the practice of grouping many hosts
>       together behind a single IP address, etc.  Most fundamentally,
>       most users find DNS domain names much easier to work with than IP
>       addresses, which is why the domain name system was designed in the
>       first place.  We prefer to define best practices for the much more
>       common use case and not to complicate the rules in this
>       specification.
>
>       Furthermore, we focus here on application service identities, not
>       specific resources located at such services.  Therefore this
>       document discusses Uniform Resource Identifiers [URI] only as a
>       way to communicate a DNS domain name (via the URI "host" component
>       or its equivalent), not as a way to communicate other aspects of a
>       service such as a specific resource (via the URI "path" component)
>       or parameters (via the URI "query" component).
>
>       We also do not discuss attributes unrelated to DNS domain names,
>       such as those defined in [X.520] and other such specifications
>       (e.g., organizational attributes, geographical attributes, company
>       logos, and the like).

So... I think we can (and should) proceed just the same, but we should
reference:

  https://tools.ietf.org/html/rfc2818#section-3.1

rather than RFC6125.

Accordingly, I've filed the curl report with reference to RFC-2818:

  https://hackerone.com/reports/715413

[...]

On 10/16/19 11:19, David Woodhouse wrote:
> So what did we say about false modesty, Laszlo?

It *was* hard; and in the first place, I truly hadn't expected myself to
send an edk2 RFC, at all... It was made possible by your code.

> In the end you did actually solve it all for yourself -- based on the
> pointer I'd given in bugzilla, and then ignoring my subsequent
> misdirection about callbacks and my overly complex attempt at doing it
> myself :)

You're too kind; I followed your code closely. I only updated the coding
style, added a bunch of comments, and supplied some error checks and
debug messages. In addition, Jiaxin suggested we should try to set the
smart verification params in TlsSetVerifyHost() at once.

Anyway: we still have the issue that X509_VERIFY_PARAM_set_ip_asc()
appears to reject IPv4 address literals. Could you check that please?

(Using a hosted (Linux userspace) program like "sconnect", it must be
easier to debug. I tried connecting gdb to QEMU, running OVMF, but it
crashed gdb. :)

I also looked at the OpenSSL code parsing the IPv4 literal, and nothing
struck me as obviously wrong. (Well, sscanf() is generally improper for
parsing untrusted data, but that's beside the point, for my test case
with a valid IPv4 literal.))

Thanks!
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[David Woodhouse]

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

On Wed, 2019-10-16 at 13:41 +0200, Laszlo Ersek wrote:
> On 10/16/19 11:19, David Woodhouse wrote:
> > In the end you did actually solve it all for yourself -- based on the
> > pointer I'd given in bugzilla, and then ignoring my subsequent
> > misdirection about callbacks and my overly complex attempt at doing it
> > myself :)
> 
> You're too kind; I followed your code closely. I only updated the coding
> style, added a bunch of comments, and supplied some error checks and
> debug messages. 

Hm, I am not widely renowned for my kindness. I'm sure there are those
who would suggest to you that there really must be some other
explanation than that.

They might point out that you also threw away all of the parts from my
actual code where I'd led you in the wrong direction, going back to my
initial suggestion that had been in Bugzilla in the first place. 

They might suggest that in posting poorly-formatted straw man patches
to TlsLib, all I have *actually* managed to do is what people so often
do to me — post a patch which is sufficiently incompetent that it has
provoked you into finding the time to do it properly for yourself :)

> In addition, Jiaxin suggested we should try to set the
> smart verification params in TlsSetVerifyHost() at once.
> 
> Anyway: we still have the issue that X509_VERIFY_PARAM_set_ip_asc()
> appears to reject IPv4 address literals. Could you check that please?
> 
> (Using a hosted (Linux userspace) program like "sconnect", it must be
> easier to debug. I tried connecting gdb to QEMU, running OVMF, but it
> crashed gdb. :)

Ah, but if you were using a hosted Linux userspace program like
sconnect, then your sscanf() implementation wouldn't look like this:

$ grep -B1 -A4 sscanf CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
/* Read formatted data from a string */
int sscanf (const char *buffer, const char *format, ...)
{
  //
  // Null sscanf() function implementation to satisfy the linker, since
  // no direct functionality logic dependency in present UEFI cases.
  //
  return 0;
}

I told you to stare hard at that, didn't I :)

I'm sure that OpenSSL upstream would welcome a patch to ditch that use
of the non-recommended sscanf() function and use inet_ntoa() where it's
available instead (although that might sensibly be guarded on
OPENSSL_NO_SOCK, which you set for the EDK2 build).

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

On 10/16/19 15:35, David Woodhouse wrote:
> On Wed, 2019-10-16 at 13:41 +0200, Laszlo Ersek wrote:

>> Anyway: we still have the issue that X509_VERIFY_PARAM_set_ip_asc()
>> appears to reject IPv4 address literals. Could you check that please?
>>
>> (Using a hosted (Linux userspace) program like "sconnect", it must be
>> easier to debug. I tried connecting gdb to QEMU, running OVMF, but it
>> crashed gdb. :)
> 
> Ah, but if you were using a hosted Linux userspace program like
> sconnect, then your sscanf() implementation wouldn't look like this:
> 
> $ grep -B1 -A4 sscanf CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c
> /* Read formatted data from a string */
> int sscanf (const char *buffer, const char *format, ...)
> {
>   //
>   // Null sscanf() function implementation to satisfy the linker, since
>   // no direct functionality logic dependency in present UEFI cases.
>   //
>   return 0;
> }

Hahaha ROTFL :)

I have no clue why I didn't realize this :) I looked at the sscanf()
call, and it never occurred to me that sscanf() is a standard C function! :)

> I told you to stare hard at that, didn't I :)

You did!

> I'm sure that OpenSSL upstream would welcome a patch to ditch that use
> of the non-recommended sscanf() function and use inet_ntoa() where it's
> available instead (although that might sensibly be guarded on
> OPENSSL_NO_SOCK, which you set for the EDK2 build).

Hrmpf. Too many functions here, for OpenSSL proper:

- inet_addr() is in POSIX:

https://pubs.opengroup.org/onlinepubs/9699919799/functions/inet_addr.html

but its failure mode is not nice (the error value aliases 255.255.255.255).

- inet_aton() is good, but it's not in POSIX. (BSD extension)

- inet_pton() is good and in POSIX. Best choice?

(Not volunteering for the OpenSSL patch at the moment -- I have my hands
full, and I'd have to go through the CLA thingy first.)


Regarding the current edk2 patch set, I think we should do the following:

- use X509_VERIFY_PARAM_set1_ip() rather than
X509_VERIFY_PARAM_set1_ip_asc()

- incorporate "StdLib/BsdSocketLib/inet_pton.c" from the edk2-libc
project (which used to be part of edk2 itself) into TlsLib, and call
inet_pton() for parsing the address as both IPv4 and IPv6.

The source file mentioned above seems to depend only on the strchr() and
memcpy() functions, and "CryptoPkg/Library/Include/CrtLibSupport.h"
already provides macros for those.

Jiaxin, what's your opinion?

Thanks
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[David Woodhouse]

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

On Wed, 2019-10-16 at 16:43 +0200, Laszlo Ersek wrote:
> Regarding the current edk2 patch set, I think we should do the following:
> 
> - use X509_VERIFY_PARAM_set1_ip() rather than
> X509_VERIFY_PARAM_set1_ip_asc()
> 
> - incorporate "StdLib/BsdSocketLib/inet_pton.c" from the edk2-libc
> project (which used to be part of edk2 itself) into TlsLib, and call
> inet_pton() for parsing the address as both IPv4 and IPv6.

That makes sense.

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

Jiaxin,

On 10/16/19 17:25, David Woodhouse wrote:
> On Wed, 2019-10-16 at 16:43 +0200, Laszlo Ersek wrote:
>> Regarding the current edk2 patch set, I think we should do the following:
>>
>> - use X509_VERIFY_PARAM_set1_ip() rather than
>> X509_VERIFY_PARAM_set1_ip_asc()
>>
>> - incorporate "StdLib/BsdSocketLib/inet_pton.c" from the edk2-libc
>> project (which used to be part of edk2 itself) into TlsLib, and call
>> inet_pton() for parsing the address as both IPv4 and IPv6.
> 
> That makes sense.

Please wait a little before starting work on this. I've been made aware,
in <https://hackerone.com/reports/715413>, of the practices of various
certificate authorities:

[1] https://www.geocerts.com/support/ip-address-in-ssl-certificate
[2]
https://www.leaderssl.com/articles/381-issuing-ssl-certificate-for-an-ip-address
[3]
https://support.globalsign.com/customer/en/portal/articles/1216536-securing-a-public-ip-address---ssl-certificates

What's most worrisome is [3], which writes:

    If you are targeting Windows 10 and later, you can populate the IP
    address in either field. If however, you are targeting Windows 8.1
    and earlier, you should only specify the IP address as the common
    name.

Keyword being "only".

Assuming the above quote precisely reflects reality: if we made edk2
strictly insist on the IP address being in the SAN.iPAddress field, then
edk2 could not HTTPS-boot from such web servers that intend to serve
Windows clients up to 8.1.

Reference [2] advises to put the IP address in both CN and SAN.iPAddress
for best compatibility, and that would be fine, for
X509_VERIFY_PARAM_set1_ip(). But the word "only" in [3] is really bad
for X509_VERIFY_PARAM_set1_ip().

Thanks
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[David Woodhouse]

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

On Thu, 2019-10-17 at 17:35 +0200, Laszlo Ersek wrote:
> Reference [2] advises to put the IP address in both CN and
> SAN.iPAddress
> for best compatibility, and that would be fine, for
> X509_VERIFY_PARAM_set1_ip(). But the word "only" in [3] is really bad
> for X509_VERIFY_PARAM_set1_ip().

I don't believe it's true, and it conflicts with what's in [2] which
suggests that you do it properly *and* put it in the legacy CN for the
benefit of broken clients.

None of this convinces me that EDK2 should deliberately be one of those
"broken clients". Just fix it. Let people worry about compatibility
with historical buggy versions of proprietary operating systems when
they issue their certs.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

On 10/17/19 17:49, David Woodhouse wrote:
> On Thu, 2019-10-17 at 17:35 +0200, Laszlo Ersek wrote:
>> Reference [2] advises to put the IP address in both CN and
>> SAN.iPAddress
>> for best compatibility, and that would be fine, for
>> X509_VERIFY_PARAM_set1_ip(). But the word "only" in [3] is really bad
>> for X509_VERIFY_PARAM_set1_ip().
> 
> I don't believe it's true, and it conflicts with what's in [2] which
> suggests that you do it properly *and* put it in the legacy CN for the
> benefit of broken clients.
> 
> None of this convinces me that EDK2 should deliberately be one of those
> "broken clients". Just fix it. Let people worry about compatibility
> with historical buggy versions of proprietary operating systems when
> they issue their certs.
> 

Personally I'm OK with this.

Thanks
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

On 10/17/19 17:49, David Woodhouse wrote:
> On Thu, 2019-10-17 at 17:35 +0200, Laszlo Ersek wrote:
>> Reference [2] advises to put the IP address in both CN and
>> SAN.iPAddress
>> for best compatibility, and that would be fine, for
>> X509_VERIFY_PARAM_set1_ip(). But the word "only" in [3] is really bad
>> for X509_VERIFY_PARAM_set1_ip().
> 
> I don't believe it's true, and it conflicts with what's in [2] which
> suggests that you do it properly *and* put it in the legacy CN for the
> benefit of broken clients.
> 
> None of this convinces me that EDK2 should deliberately be one of those
> "broken clients". Just fix it. Let people worry about compatibility
> with historical buggy versions of proprietary operating systems when
> they issue their certs.

I have four patches, to be inserted in the middle (between v1 patches #2
and #3).

I plan to submit a v2 series (8 patches in total) this week, after
testing. If I can't manage this week, then it'll take a longer (possibly
2+ weeks).

Jiaxin, I'm assigning the bug to myself, if that's OK with you.

Thanks
Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Wu, Jiaxin]

> 
> However. When I wanted to include a reference to RFC6125, I looked more
> closely at the section that Jiaxin quoted. More specifically, the
> surroundings of that section.
> 
> It turns out that the quoted section "3.1. Server Identity" is actually
> part of Appendix B.2 ("HTTP (2000)"):
> 
>   https://tools.ietf.org/html/rfc6125#appendix-B.2
> 
> which is further part of Appendix B ("Prior Art"):
> 
>   https://tools.ietf.org/html/rfc6125#appendix-B
> 
> Appendix B starts with the statement
> 
> > (This section is non-normative.)
> 
> and Appendix B.2 starts with
> 
> >    In 2000, [HTTP-TLS] specified the following text regarding
> >    application service identity verification in HTTP:
> 
> Thus, Appendix B of RFC6125 doesn't require us to do anything! (Other
> specs may still require us, of course.)
> 
> Now, does RFC6125 say anything about IP addresses? Yes, it does, in
> section 1.7.2, "Out of Scope":
> 
> https://tools.ietf.org/html/rfc6125#section-1.7.2
> 
> >    o  Identifiers other than fully qualified DNS domain names.
> >
> >       Some certification authorities issue server certificates based on
> >       IP addresses, but preliminary evidence indicates that such
> >       certificates are a very small percentage (less than 1%) of issued
> >       certificates.  Furthermore, IP addresses are not necessarily
> >       reliable identifiers for application services because of the
> >       existence of private internets [PRIVATE], host mobility, multiple
> >       interfaces on a given host, Network Address Translators (NATs)
> >       resulting in different addresses for a host from different
> >       locations on the network, the practice of grouping many hosts
> >       together behind a single IP address, etc.  Most fundamentally,
> >       most users find DNS domain names much easier to work with than IP
> >       addresses, which is why the domain name system was designed in the
> >       first place.  We prefer to define best practices for the much more
> >       common use case and not to complicate the rules in this
> >       specification.
> >
> >       Furthermore, we focus here on application service identities, not
> >       specific resources located at such services.  Therefore this
> >       document discusses Uniform Resource Identifiers [URI] only as a
> >       way to communicate a DNS domain name (via the URI "host"
> component
> >       or its equivalent), not as a way to communicate other aspects of a
> >       service such as a specific resource (via the URI "path" component)
> >       or parameters (via the URI "query" component).
> >
> >       We also do not discuss attributes unrelated to DNS domain names,
> >       such as those defined in [X.520] and other such specifications
> >       (e.g., organizational attributes, geographical attributes, company
> >       logos, and the like).
> 
> So... I think we can (and should) proceed just the same, but we should
> reference:
> 
>   https://tools.ietf.org/html/rfc2818#section-3.1
> 
> rather than RFC6125.
> 
> Accordingly, I've filed the curl report with reference to RFC-2818:
> 
>   https://hackerone.com/reports/715413
> 
> [...]
> 

Yeah, agree, Good Catch! Thanks. 

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Wu, Jiaxin]

> 
> 
> Regarding the current edk2 patch set, I think we should do the following:
> 
> - use X509_VERIFY_PARAM_set1_ip() rather than
> X509_VERIFY_PARAM_set1_ip_asc()
> 
> - incorporate "StdLib/BsdSocketLib/inet_pton.c" from the edk2-libc
> project (which used to be part of edk2 itself) into TlsLib, and call
> inet_pton() for parsing the address as both IPv4 and IPv6.
> 
> The source file mentioned above seems to depend only on the strchr() and
> memcpy() functions, and "CryptoPkg/Library/Include/CrtLibSupport.h"
> already provides macros for those.
> 
> Jiaxin, what's your opinion?
> 

Agree. It's makes sense to me. 

> Thanks
> Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Wu, Jiaxin]

> >> Reference [2] advises to put the IP address in both CN and
> >> SAN.iPAddress
> >> for best compatibility, and that would be fine, for
> >> X509_VERIFY_PARAM_set1_ip(). But the word "only" in [3] is really bad
> >> for X509_VERIFY_PARAM_set1_ip().

That was also what I suggested before:
"Now, to resolve the problem, I think the *best compatibility* can actually be reached by setting the IP address both as iPAddress and dNSName in SAN..."

Above setting means to call X509_VERIFY_PARAM_set1_ip and X509_VERIFY_PARAM_set1_host parallelly instead of exclusively (if else).

> >
> > I don't believe it's true, and it conflicts with what's in [2] which
> > suggests that you do it properly *and* put it in the legacy CN for the
> > benefit of broken clients.
> >
> > None of this convinces me that EDK2 should deliberately be one of those
> > "broken clients". Just fix it. Let people worry about compatibility
> > with historical buggy versions of proprietary operating systems when
> > they issue their certs.
> >


Okay, good point here, it's also make sense to me.


> 
> Personally I'm OK with this.
> 
> Thanks
> Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Wu, Jiaxin]

> 
> I have four patches, to be inserted in the middle (between v1 patches #2
> and #3).
> 
> I plan to submit a v2 series (8 patches in total) this week, after
> testing. If I can't manage this week, then it'll take a longer (possibly
> 2+ weeks).
> 
> Jiaxin, I'm assigning the bug to myself, if that's OK with you.
> 

Thanks Laszlo, it's appreciated you can continue help on that.:) Please go ahead for the new series patches to fix the gap.



> Thanks
> Laszlo

Re: [edk2-devel] [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

[Laszlo Ersek]

On 10/25/19 04:12, Wu, Jiaxin wrote:
>>>> Reference [2] advises to put the IP address in both CN and
>>>> SAN.iPAddress
>>>> for best compatibility, and that would be fine, for
>>>> X509_VERIFY_PARAM_set1_ip(). But the word "only" in [3] is really bad
>>>> for X509_VERIFY_PARAM_set1_ip().
> 
> That was also what I suggested before:
> "Now, to resolve the problem, I think the *best compatibility* can actually be reached by setting the IP address both as iPAddress and dNSName in SAN..."
> 
> Above setting means to call X509_VERIFY_PARAM_set1_ip and X509_VERIFY_PARAM_set1_host parallelly instead of exclusively (if else).

I think this is a misunderstanding.

AIUI, the "best compat" idea applies to certificate *creation*. IOW, the
IP address should be placed in both elements, in the certificate.

But it does not apply to certificate *verification*. RFC-2818
<https://tools.ietf.org/html/rfc2818#section-3.1> is clear about the
matching:

    In some cases, the URI is specified as an IP address rather than a
    hostname. In this case, the iPAddress subjectAltName must be present
    in the certificate and must exactly match the IP in the URI.

IIUC this is also what David argues for:

>>> I don't believe it's true, and it conflicts with what's in [2] which
>>> suggests that you do it properly *and* put it in the legacy CN for the
>>> benefit of broken clients.
>>>
>>> None of this convinces me that EDK2 should deliberately be one of those
>>> "broken clients". Just fix it. Let people worry about compatibility
>>> with historical buggy versions of proprietary operating systems when
>>> they issue their certs.
>>>

Ah, I see you commented too:

> Okay, good point here, it's also make sense to me.

OK then!

Thanks
Laszlo