Re: [PATCH 6/6] NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro

["Maciej Rabeda" <maciej.rabeda@linux.intel.com>]

Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>

On 08-Jun-21 15:06, Laszlo Ersek wrote:
> Introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro for NetworkPkg.
> When explicitly set to FALSE, remove MD5 from IScsiDxe's CHAP algorithm
> list.
>
> Set NETWORK_ISCSI_MD5_ENABLE to TRUE by default, for compatibility
> reasons. Not just to minimize the disruption for platforms that currently
> include IScsiDxe, but also because RFC 7143 mandates MD5 for CHAP, and
> some vendors' iSCSI targets support MD5 only.
>
> With MD5 enabled, IScsiDxe will suggest SHA256, and then fall back to MD5
> if the target requests it. With MD5 disabled, IScsiDxe will suggest
> SHA256, and break off the connection (and session) if the target doesn't
> support SHA256.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   NetworkPkg/NetworkBuildOptions.dsc.inc |  2 +-
>   NetworkPkg/NetworkDefines.dsc.inc      | 20 ++++++++++++++++++++
>   NetworkPkg/IScsiDxe/IScsiCHAP.c        |  2 ++
>   3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/NetworkBuildOptions.dsc.inc b/NetworkPkg/NetworkBuildOptions.dsc.inc
> index 42d980d9543d..738da2222f7e 100644
> --- a/NetworkPkg/NetworkBuildOptions.dsc.inc
> +++ b/NetworkPkg/NetworkBuildOptions.dsc.inc
> @@ -1,22 +1,22 @@
>   ## @file
>   # Network DSC include file for [BuildOptions] sections of all Architectures.
>   #
>   # This file can be included in the [BuildOptions*] section(s) of a platform DSC file
>   # by using "!include NetworkPkg/NetworkBuildOptions.dsc.inc", to specify the C language
>   # feature test macros (eg., API deprecation macros) according to the flags described
>   # in "NetworkDefines.dsc.inc".
>   #
>   # Supported tool chain families: "GCC", "INTEL", "MSFT", "RVCT".
>   #
>   # Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
>   #
>   #    SPDX-License-Identifier: BSD-2-Clause-Patent
>   #
>   ##
>   
> -!if $(NETWORK_ISCSI_ENABLE) == TRUE
> +!if $(NETWORK_ISCSI_ENABLE) == TRUE && $(NETWORK_ISCSI_MD5_ENABLE) == TRUE
>     MSFT:*_*_*_CC_FLAGS = /D ENABLE_MD5_DEPRECATED_INTERFACES
>     INTEL:*_*_*_CC_FLAGS = /D ENABLE_MD5_DEPRECATED_INTERFACES
>     GCC:*_*_*_CC_FLAGS = -D ENABLE_MD5_DEPRECATED_INTERFACES
>     RVCT:*_*_*_CC_FLAGS = -DENABLE_MD5_DEPRECATED_INTERFACES
>   !endif
> diff --git a/NetworkPkg/NetworkDefines.dsc.inc b/NetworkPkg/NetworkDefines.dsc.inc
> index 54deb6342aaa..e39a9cb3dc09 100644
> --- a/NetworkPkg/NetworkDefines.dsc.inc
> +++ b/NetworkPkg/NetworkDefines.dsc.inc
> @@ -3,38 +3,39 @@
>   #
>   # This file can be included to the [Defines] section of a platform DSC file by
>   # using "!include NetworkPkg/NetworkDefines.dsc.inc" to set default value of
>   # flags if they are not defined somewhere else, and also check the value to see
>   # if there is any conflict.
>   #
>   # These flags can be defined before the !include line, or changed on the command
>   # line to enable or disable related feature support.
>   #   -D FLAG=VALUE
>   # The default value of these flags are:
>   #   DEFINE NETWORK_ENABLE                 = TRUE
>   #   DEFINE NETWORK_SNP_ENABLE             = TRUE
>   #   DEFINE NETWORK_IP4_ENABLE             = TRUE
>   #   DEFINE NETWORK_IP6_ENABLE             = TRUE
>   #   DEFINE NETWORK_TLS_ENABLE             = TRUE
>   #   DEFINE NETWORK_HTTP_ENABLE            = FALSE
>   #   DEFINE NETWORK_HTTP_BOOT_ENABLE       = TRUE
>   #   DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
>   #   DEFINE NETWORK_ISCSI_ENABLE           = FALSE
> +#   DEFINE NETWORK_ISCSI_MD5_ENABLE       = TRUE
>   #   DEFINE NETWORK_VLAN_ENABLE            = TRUE
>   #
>   # Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
>   # (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
>   #
>   #    SPDX-License-Identifier: BSD-2-Clause-Patent
>   #
>   ##
>   
>   !ifndef NETWORK_ENABLE
>     #
>     # This flag is to enable or disable the whole network stack.
>     #
>     DEFINE NETWORK_ENABLE = TRUE
>   !endif
>   
>   !ifndef NETWORK_SNP_ENABLE
>     #
>     # This flag is to include the common SNP driver or not.
> @@ -101,33 +102,52 @@
>     #       Both the "https://" and "http://" URI schemes are permitted. Otherwise, HTTP
>     #       connections are denied. Only the "https://" URI scheme is permitted.
>     #
>     DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
>   !endif
>   
>   !ifndef NETWORK_ISCSI_ENABLE
>     #
>     # This flag is to enable or disable iSCSI feature.
>     #
>     # Note: This feature depends on the OpenSSL building. To enable this feature, please
>     #       follow the instructions found in the file "OpenSSL-HOWTO.txt" located in
>     #       CryptoPkg\Library\OpensslLib to enable the OpenSSL building first.
>     #       Both OpensslLib.inf and OpensslLibCrypto.inf library instance can be used
>     #       since libssl is not required for iSCSI.
>     #
>     DEFINE NETWORK_ISCSI_ENABLE = FALSE
>   !endif
>   
> +!ifndef NETWORK_ISCSI_MD5_ENABLE
> +  #
> +  # This flag enables the deprecated MD5 hash algorithm in iSCSI CHAP
> +  # authentication.
> +  #
> +  # Note: The NETWORK_ISCSI_MD5_ENABLE flag only makes a difference if
> +  #       NETWORK_ISCSI_ENABLE is TRUE; otherwise, NETWORK_ISCSI_MD5_ENABLE is
> +  #       ignored.
> +  #
> +  #       With NETWORK_ISCSI_MD5_ENABLE set to TRUE, MD5 is enabled as the
> +  #       least preferred CHAP hash algorithm. With NETWORK_ISCSI_MD5_ENABLE
> +  #       set to FALSE, MD5 is disabled statically, at build time.
> +  #
> +  #       The default value is TRUE, because RFC 7143 mandates MD5, and because
> +  #       several vendors' iSCSI targets only support MD5, for CHAP.
> +  #
> +  DEFINE NETWORK_ISCSI_MD5_ENABLE = TRUE
> +!endif
> +
>   !if $(NETWORK_ENABLE) == TRUE
>     #
>     # Check the flags to see if there is any conflict.
>     #
>     !if ($(NETWORK_IP4_ENABLE) == FALSE) AND ($(NETWORK_IP6_ENABLE) == FALSE)
>       !error "Must enable at least IP4 or IP6 stack if NETWORK_ENABLE is set to TRUE!"
>     !endif
>   
>     !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
>       !if ($(NETWORK_TLS_ENABLE) == FALSE) AND ($(NETWORK_ALLOW_HTTP_CONNECTIONS) == FALSE)
>         !error "Must enable TLS to support HTTPS, or allow unsecured HTTP connection, if NETWORK_HTTP_BOOT_ENABLE or NETWORK_HTTP_ENABLE is set to TRUE!"
>       !endif
>     !endif
>   !endif
> diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
> index 2ce53c1ea4af..57163e9eb97f 100644
> --- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
> +++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
> @@ -7,50 +7,52 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
>   
>   #include "IScsiImpl.h"
>   
>   //
>   // Supported CHAP hash algorithms, mapped to sets of BaseCryptLib APIs and
>   // macros. CHAP_HASH structures at lower subscripts in the array are preferred
>   // by the initiator.
>   //
>   STATIC CONST CHAP_HASH mChapHash[] = {
>     {
>       ISCSI_CHAP_ALGORITHM_SHA256,
>       SHA256_DIGEST_SIZE,
>       Sha256GetContextSize,
>       Sha256Init,
>       Sha256Update,
>       Sha256Final
>     },
> +#ifdef ENABLE_MD5_DEPRECATED_INTERFACES
>     //
>     // Keep the deprecated MD5 entry at the end of the array (making MD5 the
>     // least preferred choice of the initiator).
>     //
>     {
>       ISCSI_CHAP_ALGORITHM_MD5,
>       MD5_DIGEST_SIZE,
>       Md5GetContextSize,
>       Md5Init,
>       Md5Update,
>       Md5Final
>     },
> +#endif // ENABLE_MD5_DEPRECATED_INTERFACES
>   };
>   
>   //
>   // Ordered list of mChapHash[*].Algorithm values. It is formatted for the
>   // CHAP_A=<A1,A2...> value string, by the IScsiCHAPInitHashList() function. It
>   // is sent by the initiator in ISCSI_CHAP_STEP_ONE.
>   //
>   STATIC CHAR8 mChapHashListString[
>                  3 +                                      // UINT8 identifier in
>                                                           //   decimal
>                  (1 + 3) * (ARRAY_SIZE (mChapHash) - 1) + // comma prepended for
>                                                           //   entries after the
>                                                           //   first
>                  1 +                                      // extra character for
>                                                           //   AsciiSPrint()
>                                                           //   truncation check
>                  1                                        // terminating NUL
>                  ];
>