Date   

Re: [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

Brijesh Singh
 

On 6/8/21 3:17 AM, Laszlo Ersek wrote:

(3) Actually, no.

This patch should be reduced to the following files only:

- OvmfPkg/PlatformPei/AmdSev.c
- OvmfPkg/PlatformPei/PlatformPei.inf

and the following changes should be dropped completely:

- OvmfPkg/Include/Library/MemEncryptSevLib.h
- OvmfPkg/ResetVector/Ia32/PageTables64.asm
- OvmfPkg/ResetVector/ResetVector.nasmb

Specifically, the "SEC_SEV_ES_WORK_AREA.SevSnpEnabled" field should
never be introduced.

The reason is apparent only from patch #10 -- "OvmfPkg/PlatformPei:
register GHCB gpa for the SEV-SNP guest".

The core idea is that in patch#10, in the SEC module, you can implement
SevSnpIsEnabled() by just reading MSR_SEV_STATUS, and checking the SNP
bit. Namely, while the SevSnpIsEnabled() call is made in
SevEsProtocolCheck(), i.e., before exception handling is set up in the
SEC module -- and so you indeed cannot call CPUID --, you don't *have*
to call CPUID at that call site. Where you call SevSnpIsEnabled() in
SevEsProtocolCheck(), you already know that SEV-ES is enabled, so it's
safe to just read the exact same SEV status MSR that the SEV-ES status
comes from in the first place, without any CPUID safety check.
We must check the SNP Enabled inside the assembly code for the page
invalidate functions, and I decided to cache the value. A similar
SNP-enabled check is required in SEC phase before the
ProcessLibraryConstrctorList() is called. There are two options on how
we can go about doing the SNP enabled check inside the SEC phase
1. Call the SEV_STATUS MSR after reading the
SEC_SEV_ES_WORK_AREA.SevEnabled. As you said, we need to be sure that ES
is enabled before calling the SEV_STATUS MSR.
2. SEV_STATUS MSR is read in Reset vector for the SNP enabled check
purpose. Extend the SevEsWorkArea to cache the state.

 I chose #2 because it avoids checking for ES enabled before checking
the SNP enabled. I understand that in the current code path, SNP check
is called inside the SevEsProtocolCheck() -- ES is already enabled, and
its safe to call SEV_STATUS MSR. What if we need to check for the SNP
state outside the ES-specific code block in the future? Then we will
need to extend the SevEsWorkArea.
What would be the reason for this, ever?
One reason I can think of is if we ever decided validate the pages
before the SevEsProtocolCheck(). The version 2 of GHCB spec adds few new
NAE's that are SNP specific such as Page State Change. They are not
applicable to the ES guests. Currently, we do the page validation much
later and by then ProcessorConstructList() is called. Anyway, this is
not an important thing to consider right now. As I said, I will drop the
extending workarea to cache the SNP enable and Hypervisor feature values.



I think this ties in with another point (or question) I raised
elsewhere: the assembly code in the reset vector suggests *anyway* that
SNP is only available if ES is available, but I couldn't verify that
from any specs. If this dependency is an architectural fact (that is, if
ES is absent, then SNP may never be present), then I wouldn't like to
introduce a separate field for SNP presence in the SEC_SEV_ES_WORK_AREA
structure.
The SEV-SNP builds upon existing SEV and SEV-ES support and provides an
additional protection from the hypervisor. The SEV-SNP feature requires
both the SEV and SEV-ES must be enabled. There is some text about it in
APM volume 2 [1] chapter  15.36.


[1] https://www.amd.com/system/files/TechDocs/24593.pdf


thanks


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

Laszlo Ersek
 

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
];

--
2.19.1.3.g30247aa5d201


[PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP

Laszlo Ersek
 

Insert a SHA256 CHAP_HASH structure at the start of "mChapHash".

Update ISCSI_CHAP_MAX_DIGEST_SIZE to SHA256_DIGEST_SIZE (32).

This enables the initiator and the target to negotiate SHA256 for CHAP, in
preference to MD5.

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/IScsiDxe/IScsiCHAP.h | 3 ++-
NetworkPkg/IScsiDxe/IScsiCHAP.c | 12 ++++++++++++
2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 1e5cc0b287ed..e2df634c4e67 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -6,44 +6,45 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_

#define ISCSI_AUTH_METHOD_CHAP "CHAP"

#define ISCSI_KEY_CHAP_ALGORITHM "CHAP_A"
#define ISCSI_KEY_CHAP_IDENTIFIER "CHAP_I"
#define ISCSI_KEY_CHAP_CHALLENGE "CHAP_C"
#define ISCSI_KEY_CHAP_NAME "CHAP_N"
#define ISCSI_KEY_CHAP_RESPONSE "CHAP_R"

//
// Identifiers of supported CHAP hash algorithms:
// https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9
//
#define ISCSI_CHAP_ALGORITHM_MD5 5
+#define ISCSI_CHAP_ALGORITHM_SHA256 7

//
// Byte count of the largest digest over the above-listed
// ISCSI_CHAP_ALGORITHM_* hash algorithms.
//
-#define ISCSI_CHAP_MAX_DIGEST_SIZE MD5_DIGEST_SIZE
+#define ISCSI_CHAP_MAX_DIGEST_SIZE SHA256_DIGEST_SIZE

#define ISCSI_CHAP_STEP_ONE 1
#define ISCSI_CHAP_STEP_TWO 2
#define ISCSI_CHAP_STEP_THREE 3
#define ISCSI_CHAP_STEP_FOUR 4


#pragma pack(1)

typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {
UINT8 CHAPType;
CHAR8 CHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 CHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
CHAR8 ReverseCHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 ReverseCHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
} ISCSI_CHAP_AUTH_CONFIG_NVDATA;

#pragma pack()

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index f02ada6444ce..2ce53c1ea4af 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -1,36 +1,48 @@
/** @file
This file is for Challenge-Handshake Authentication Protocol (CHAP)
Configuration.

Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
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
+ },
+ //
+ // 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
},
};

//
// 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
--
2.19.1.3.g30247aa5d201


[PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP

Laszlo Ersek
 

Introduce the "mChapHash" table, containing the hash algorithms supported
for CHAP. Hash algos listed at the beginning of the table are preferred by
the initiator.

In ISCSI_CHAP_STEP_ONE, send such a CHAP_A value that is the
comma-separated, ordered list of algorithm identifiers from "mChapHash".
Pre-format this value string at driver startup, in the new function
IScsiCHAPInitHashList().

(In IScsiCHAPInitHashList(), also enforce that every hash algo's digest
size fit into ISCSI_CHAP_MAX_DIGEST_SIZE, as the latter controls the
digest, outgoing challenge, and hex *allocations*.)

In ISCSI_CHAP_STEP_TWO, allow the target to select one of the offered hash
algorithms, and remember the selection for the later steps. For
ISCSI_CHAP_STEP_THREE, hash the challenge from the target with the
selected hash algo.

In ISCSI_CHAP_STEP_THREE, send the correctly sized digest to the target.
If the initiator wants mutual authentication, then generate a challenge
with as many bytes as the target's digest will have, in
ISCSI_CHAP_STEP_FOUR.

In ISCSI_CHAP_STEP_FOUR (i.e., when mutual authentication is required by
the initiator), verify the target's response (digest) with the selected
algorithm.

Clear the selected hash algorithm before every login (remember that in
IScsiDxe, every login is a leading login).

There is no peer-observable change from this patch, as it only reworks the
current MD5 support into the new internal representation.

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/IScsiDxe/IScsiCHAP.h | 55 +++++++
NetworkPkg/IScsiDxe/IScsiCHAP.c | 158 +++++++++++++++++---
NetworkPkg/IScsiDxe/IScsiDriver.c | 2 +
NetworkPkg/IScsiDxe/IScsiProto.c | 3 +
4 files changed, 195 insertions(+), 23 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index b8811b7580f0..1e5cc0b287ed 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -31,47 +31,91 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

#define ISCSI_CHAP_STEP_ONE 1
#define ISCSI_CHAP_STEP_TWO 2
#define ISCSI_CHAP_STEP_THREE 3
#define ISCSI_CHAP_STEP_FOUR 4


#pragma pack(1)

typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {
UINT8 CHAPType;
CHAR8 CHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 CHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
CHAR8 ReverseCHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 ReverseCHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
} ISCSI_CHAP_AUTH_CONFIG_NVDATA;

#pragma pack()

+//
+// Typedefs for collecting sets of hash APIs from BaseCryptLib.
+//
+typedef
+UINTN
+(EFIAPI *CHAP_HASH_GET_CONTEXT_SIZE) (
+ VOID
+ );
+
+typedef
+BOOLEAN
+(EFIAPI *CHAP_HASH_INIT) (
+ OUT VOID *Context
+ );
+
+typedef
+BOOLEAN
+(EFIAPI *CHAP_HASH_UPDATE) (
+ IN OUT VOID *Context,
+ IN CONST VOID *Data,
+ IN UINTN DataSize
+ );
+
+typedef
+BOOLEAN
+(EFIAPI *CHAP_HASH_FINAL) (
+ IN OUT VOID *Context,
+ OUT UINT8 *HashValue
+ );
+
+typedef struct {
+ UINT8 Algorithm; // ISCSI_CHAP_ALGORITHM_*, CHAP_A
+ UINT32 DigestSize;
+ CHAP_HASH_GET_CONTEXT_SIZE GetContextSize;
+ CHAP_HASH_INIT Init;
+ CHAP_HASH_UPDATE Update;
+ CHAP_HASH_FINAL Final;
+} CHAP_HASH;
+
///
/// ISCSI CHAP Authentication Data
///
typedef struct _ISCSI_CHAP_AUTH_DATA {
ISCSI_CHAP_AUTH_CONFIG_NVDATA *AuthConfig;
UINT32 InIdentifier;
UINT8 InChallenge[1024];
UINT32 InChallengeLength;
//
+ // The hash algorithm (CHAP_A) that the target selects in
+ // ISCSI_CHAP_STEP_TWO.
+ //
+ CONST CHAP_HASH *Hash;
+ //
// Calculated CHAP Response (CHAP_R) value.
//
UINT8 CHAPResponse[ISCSI_CHAP_MAX_DIGEST_SIZE];

//
// Auth-data to be sent out for mutual authentication.
//
// While the challenge size is technically independent of the hashing
// algorithm, it is good practice to avoid hashing *fewer bytes* than the
// digest size. In other words, it's good practice to feed *at least as many
// bytes* to the hashing algorithm as the hashing algorithm will output.
//
UINT32 OutIdentifier;
UINT8 OutChallenge[ISCSI_CHAP_MAX_DIGEST_SIZE];
} ISCSI_CHAP_AUTH_DATA;

/**
This function checks the received iSCSI Login Response during the security
negotiation stage.
@@ -92,20 +136,31 @@ IScsiCHAPOnRspReceived (
This function fills the CHAP authentication information into the login PDU
during the security negotiation stage in the iSCSI connection login.

@param[in] Conn The iSCSI connection.
@param[in, out] Pdu The PDU to send out.

@retval EFI_SUCCESS All check passed and the phase-related CHAP
authentication info is filled into the iSCSI
PDU.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.

**/
EFI_STATUS
IScsiCHAPToSendReq (
IN ISCSI_CONNECTION *Conn,
IN OUT NET_BUF *Pdu
);

+/**
+ Initialize the CHAP_A=<A1,A2...> *value* string for the entire driver, to be
+ sent by the initiator in ISCSI_CHAP_STEP_ONE.
+
+ This function sanity-checks the internal table of supported CHAP hashing
+ algorithms, as well.
+**/
+VOID
+IScsiCHAPInitHashList (
+ VOID
+ );
#endif
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index 744824e63d23..f02ada6444ce 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -1,148 +1,194 @@
/** @file
This file is for Challenge-Handshake Authentication Protocol (CHAP)
Configuration.

Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
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_MD5,
+ MD5_DIGEST_SIZE,
+ Md5GetContextSize,
+ Md5Init,
+ Md5Update,
+ Md5Final
+ },
+};
+
+//
+// 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
+ ];
+
/**
Initiator calculates its own expected hash value.

@param[in] ChapIdentifier iSCSI CHAP identifier sent by authenticator.
@param[in] ChapSecret iSCSI CHAP secret of the authenticator.
@param[in] SecretLength The length of iSCSI CHAP secret.
@param[in] ChapChallenge The challenge message sent by authenticator.
@param[in] ChallengeLength The length of iSCSI CHAP challenge message.
+ @param[in] Hash Pointer to the CHAP_HASH structure that
+ determines the hashing algorithm to use. The
+ caller is responsible for making Hash point
+ to an "mChapHash" element.
@param[out] ChapResponse The calculation of the expected hash value.

@retval EFI_SUCCESS The expected hash value was calculatedly
successfully.
@retval EFI_PROTOCOL_ERROR The length of the secret should be at least
the length of the hash value for the hashing
algorithm chosen.
- @retval EFI_PROTOCOL_ERROR MD5 hash operation fail.
- @retval EFI_OUT_OF_RESOURCES Fail to allocate resource to complete MD5.
+ @retval EFI_PROTOCOL_ERROR Hash operation fails.
+ @retval EFI_OUT_OF_RESOURCES Failure to allocate resource to complete
+ hashing.

**/
EFI_STATUS
IScsiCHAPCalculateResponse (
IN UINT32 ChapIdentifier,
IN CHAR8 *ChapSecret,
IN UINT32 SecretLength,
IN UINT8 *ChapChallenge,
IN UINT32 ChallengeLength,
+ IN CONST CHAP_HASH *Hash,
OUT UINT8 *ChapResponse
)
{
- UINTN Md5ContextSize;
- VOID *Md5Ctx;
+ UINTN ContextSize;
+ VOID *Ctx;
CHAR8 IdByte[1];
EFI_STATUS Status;

if (SecretLength < ISCSI_CHAP_SECRET_MIN_LEN) {
return EFI_PROTOCOL_ERROR;
}

- Md5ContextSize = Md5GetContextSize ();
- Md5Ctx = AllocatePool (Md5ContextSize);
- if (Md5Ctx == NULL) {
+ ASSERT (Hash != NULL);
+
+ ContextSize = Hash->GetContextSize ();
+ Ctx = AllocatePool (ContextSize);
+ if (Ctx == NULL) {
return EFI_OUT_OF_RESOURCES;
}

Status = EFI_PROTOCOL_ERROR;

- if (!Md5Init (Md5Ctx)) {
+ if (!Hash->Init (Ctx)) {
goto Exit;
}

//
// Hash Identifier - Only calculate 1 byte data (RFC1994)
//
IdByte[0] = (CHAR8) ChapIdentifier;
- if (!Md5Update (Md5Ctx, IdByte, 1)) {
+ if (!Hash->Update (Ctx, IdByte, 1)) {
goto Exit;
}

//
// Hash Secret
//
- if (!Md5Update (Md5Ctx, ChapSecret, SecretLength)) {
+ if (!Hash->Update (Ctx, ChapSecret, SecretLength)) {
goto Exit;
}

//
// Hash Challenge received from Target
//
- if (!Md5Update (Md5Ctx, ChapChallenge, ChallengeLength)) {
+ if (!Hash->Update (Ctx, ChapChallenge, ChallengeLength)) {
goto Exit;
}

- if (Md5Final (Md5Ctx, ChapResponse)) {
+ if (Hash->Final (Ctx, ChapResponse)) {
Status = EFI_SUCCESS;
}

Exit:
- FreePool (Md5Ctx);
+ FreePool (Ctx);
return Status;
}

/**
The initiator checks the CHAP response replied by target against its own
calculation of the expected hash value.

@param[in] AuthData iSCSI CHAP authentication data.
@param[in] TargetResponse The response from target.

@retval EFI_SUCCESS The response from target passed
authentication.
@retval EFI_SECURITY_VIOLATION The response from target was not expected
value.
@retval Others Other errors as indicated.

**/
EFI_STATUS
IScsiCHAPAuthTarget (
IN ISCSI_CHAP_AUTH_DATA *AuthData,
IN UINT8 *TargetResponse
)
{
EFI_STATUS Status;
UINT32 SecretSize;
UINT8 VerifyRsp[ISCSI_CHAP_MAX_DIGEST_SIZE];

Status = EFI_SUCCESS;

SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
+
+ ASSERT (AuthData->Hash != NULL);
+
Status = IScsiCHAPCalculateResponse (
AuthData->OutIdentifier,
AuthData->AuthConfig->ReverseCHAPSecret,
SecretSize,
AuthData->OutChallenge,
- MD5_DIGEST_SIZE, // ChallengeLength
+ AuthData->Hash->DigestSize, // ChallengeLength
+ AuthData->Hash,
VerifyRsp
);

- if (CompareMem (VerifyRsp, TargetResponse, MD5_DIGEST_SIZE) != 0) {
+ if (CompareMem (VerifyRsp, TargetResponse,
+ AuthData->Hash->DigestSize) != 0) {
Status = EFI_SECURITY_VIOLATION;
}

return Status;
}


/**
This function checks the received iSCSI Login Response during the security
negotiation stage.

@param[in] Conn The iSCSI connection.

@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.
@retval Others Other errors as indicated.

**/
@@ -150,38 +196,39 @@ EFI_STATUS
IScsiCHAPOnRspReceived (
IN ISCSI_CONNECTION *Conn
)
{
EFI_STATUS Status;
ISCSI_SESSION *Session;
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
UINT8 *Data;
UINT32 Len;
LIST_ENTRY *KeyValueList;
UINTN Algorithm;
CHAR8 *Identifier;
CHAR8 *Challenge;
CHAR8 *Name;
CHAR8 *Response;
UINT8 TargetRsp[ISCSI_CHAP_MAX_DIGEST_SIZE];
UINT32 RspLen;
UINTN Result;
+ UINTN HashIndex;

ASSERT (Conn->CurrentStage == ISCSI_SECURITY_NEGOTIATION);
ASSERT (Conn->RspQue.BufNum != 0);

Session = Conn->Session;
AuthData = &Session->AuthData.CHAP;
Len = Conn->RspQue.BufSize;
Data = AllocateZeroPool (Len);
if (Data == NULL) {
return EFI_OUT_OF_RESOURCES;
}
//
// Copy the data in case the data spans over multiple PDUs.
//
NetbufQueCopy (&Conn->RspQue, 0, Len, Data);

//
// Build the key-value list from the data segment of the Login Response.
//
@@ -241,44 +288,54 @@ IScsiCHAPOnRspReceived (
// Transit to CHAP step one.
//
Conn->AuthStep = ISCSI_CHAP_STEP_ONE;
Status = EFI_SUCCESS;
break;

case ISCSI_CHAP_STEP_TWO:
//
// The Target replies with CHAP_A=<A> CHAP_I=<I> CHAP_C=<C>
//
Value = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_ALGORITHM
);
if (Value == NULL) {
goto ON_EXIT;
}

Algorithm = IScsiNetNtoi (Value);
- if (Algorithm != ISCSI_CHAP_ALGORITHM_MD5) {
+ for (HashIndex = 0; HashIndex < ARRAY_SIZE (mChapHash); HashIndex++) {
+ if (Algorithm == mChapHash[HashIndex].Algorithm) {
+ break;
+ }
+ }
+ if (HashIndex == ARRAY_SIZE (mChapHash)) {
//
// Unsupported algorithm is chosen by target.
//
goto ON_EXIT;
}
+ //
+ // Remember the target's chosen hash algorithm.
+ //
+ ASSERT (AuthData->Hash == NULL);
+ AuthData->Hash = &mChapHash[HashIndex];

Identifier = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_IDENTIFIER
);
if (Identifier == NULL) {
goto ON_EXIT;
}

Challenge = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_CHALLENGE
);
if (Challenge == NULL) {
goto ON_EXIT;
}
//
// Process the CHAP identifier and CHAP Challenge from Target.
// Calculate Response value.
@@ -289,76 +346,78 @@ IScsiCHAPOnRspReceived (
}

AuthData->InIdentifier = (UINT32) Result;
AuthData->InChallengeLength = (UINT32) sizeof (AuthData->InChallenge);
Status = IScsiHexToBin (
(UINT8 *) AuthData->InChallenge,
&AuthData->InChallengeLength,
Challenge
);
if (EFI_ERROR (Status)) {
Status = EFI_PROTOCOL_ERROR;
goto ON_EXIT;
}
Status = IScsiCHAPCalculateResponse (
AuthData->InIdentifier,
AuthData->AuthConfig->CHAPSecret,
(UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
AuthData->InChallenge,
AuthData->InChallengeLength,
+ AuthData->Hash,
AuthData->CHAPResponse
);

//
// Transit to next step.
//
Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
break;

case ISCSI_CHAP_STEP_THREE:
//
// One way CHAP authentication and the target would like to
// authenticate us.
//
Status = EFI_SUCCESS;
break;

case ISCSI_CHAP_STEP_FOUR:
ASSERT (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL);
//
// The forth step, CHAP_N=<N> CHAP_R=<R> is received from Target.
//
Name = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_NAME);
if (Name == NULL) {
goto ON_EXIT;
}

Response = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_RESPONSE
);
if (Response == NULL) {
goto ON_EXIT;
}

- RspLen = MD5_DIGEST_SIZE;
+ ASSERT (AuthData->Hash != NULL);
+ RspLen = AuthData->Hash->DigestSize;
Status = IScsiHexToBin (TargetRsp, &RspLen, Response);
- if (EFI_ERROR (Status) || RspLen != MD5_DIGEST_SIZE) {
+ if (EFI_ERROR (Status) || RspLen != AuthData->Hash->DigestSize) {
Status = EFI_PROTOCOL_ERROR;
goto ON_EXIT;
}

//
// Check the CHAP Name and Response replied by Target.
//
Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
break;

default:
break;
}

ON_EXIT:

if (KeyValueList != NULL) {
IScsiFreeKeyValueList (KeyValueList);
}
@@ -442,88 +501,141 @@ IScsiCHAPToSendReq (
Session->ConfigData->SessionConfigData.TargetName
);

if (Session->AuthType == ISCSI_AUTH_TYPE_NONE) {
Value = ISCSI_KEY_VALUE_NONE;
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
} else {
Value = ISCSI_AUTH_METHOD_CHAP;
}

IScsiAddKeyValuePair (Pdu, ISCSI_KEY_AUTH_METHOD, Value);

break;

case ISCSI_CHAP_STEP_ONE:
//
// First step, send the Login Request with CHAP_A=<A1,A2...> key-value
// pair.
//
- AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", ISCSI_CHAP_ALGORITHM_MD5);
- IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_ALGORITHM, ValueStr);
+ IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_ALGORITHM, mChapHashListString);

Conn->AuthStep = ISCSI_CHAP_STEP_TWO;
break;

case ISCSI_CHAP_STEP_THREE:
//
// Third step, send the Login Request with CHAP_N=<N> CHAP_R=<R> or
// CHAP_N=<N> CHAP_R=<R> CHAP_I=<I> CHAP_C=<C> if target authentication is
// required too.
//
// CHAP_N=<N>
//
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_CHAP_NAME,
(CHAR8 *) &AuthData->AuthConfig->CHAPName
);
//
// CHAP_R=<R>
//
+ ASSERT (AuthData->Hash != NULL);
BinToHexStatus = IScsiBinToHex (
(UINT8 *) AuthData->CHAPResponse,
- MD5_DIGEST_SIZE,
+ AuthData->Hash->DigestSize,
Response,
&RspLen
);
ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);

if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
//
// CHAP_I=<I>
//
IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
//
// CHAP_C=<C>
//
- IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, MD5_DIGEST_SIZE);
+ IScsiGenRandom ((UINT8 *) AuthData->OutChallenge,
+ AuthData->Hash->DigestSize);
BinToHexStatus = IScsiBinToHex (
(UINT8 *) AuthData->OutChallenge,
- MD5_DIGEST_SIZE,
+ AuthData->Hash->DigestSize,
Challenge,
&ChallengeLen
);
ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);

Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
}
//
// Set the stage transition flag.
//
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
break;

default:
Status = EFI_PROTOCOL_ERROR;
break;
}

FreePool (Response);
FreePool (Challenge);

return Status;
}
+
+/**
+ Initialize the CHAP_A=<A1,A2...> *value* string for the entire driver, to be
+ sent by the initiator in ISCSI_CHAP_STEP_ONE.
+
+ This function sanity-checks the internal table of supported CHAP hashing
+ algorithms, as well.
+**/
+VOID
+IScsiCHAPInitHashList (
+ VOID
+ )
+{
+ CHAR8 *Position;
+ UINTN Left;
+ UINTN HashIndex;
+ CONST CHAP_HASH *Hash;
+ UINTN Printed;
+
+ Position = mChapHashListString;
+ Left = sizeof (mChapHashListString);
+ for (HashIndex = 0; HashIndex < ARRAY_SIZE (mChapHash); HashIndex++) {
+ Hash = &mChapHash[HashIndex];
+
+ //
+ // Format the next hash identifier.
+ //
+ // Assert that we can format at least one non-NUL character, i.e. that we
+ // can progress. Truncation is checked after printing.
+ //
+ ASSERT (Left >= 2);
+ Printed = AsciiSPrint (Position, Left, "%a%d", (HashIndex == 0) ? "" : ",",
+ Hash->Algorithm);
+ //
+ // There's no way to differentiate between the "buffer filled to the brim,
+ // but not truncated" result and the "truncated" result of AsciiSPrint().
+ // This is why "mChapHashListString" has an extra byte allocated, and the
+ // reason why we use the less-than (rather than the less-than-or-equal-to)
+ // relational operator in the assertion below -- we enforce "no truncation"
+ // by excluding the "completely used up" case too.
+ //
+ ASSERT (Printed + 1 < Left);
+
+ Position += Printed;
+ Left -= Printed;
+
+ //
+ // Sanity-check the digest size for Hash.
+ //
+ ASSERT (Hash->DigestSize <= ISCSI_CHAP_MAX_DIGEST_SIZE);
+ }
+}
diff --git a/NetworkPkg/IScsiDxe/IScsiDriver.c b/NetworkPkg/IScsiDxe/IScsiDriver.c
index 98b73308c118..485c92972113 100644
--- a/NetworkPkg/IScsiDxe/IScsiDriver.c
+++ b/NetworkPkg/IScsiDxe/IScsiDriver.c
@@ -1763,38 +1763,40 @@ IScsiDriverEntryPoint (
goto Error1;
}

//
// Install the iSCSI Initiator Name Protocol.
//
Status = gBS->InstallProtocolInterface (
&ImageHandle,
&gEfiIScsiInitiatorNameProtocolGuid,
EFI_NATIVE_INTERFACE,
&gIScsiInitiatorName
);
if (EFI_ERROR (Status)) {
goto Error2;
}

//
// Create the private data structures.
//
+ IScsiCHAPInitHashList ();
+
mPrivate = AllocateZeroPool (sizeof (ISCSI_PRIVATE_DATA));
if (mPrivate == NULL) {
Status = EFI_OUT_OF_RESOURCES;
goto Error3;
}

InitializeListHead (&mPrivate->NicInfoList);
InitializeListHead (&mPrivate->AttemptConfigs);

//
// Initialize the configuration form of iSCSI.
//
Status = IScsiConfigFormInit (gIScsiIp4DriverBinding.DriverBindingHandle);
if (EFI_ERROR (Status)) {
goto Error4;
}

//
// Create the Maximum Attempts.
diff --git a/NetworkPkg/IScsiDxe/IScsiProto.c b/NetworkPkg/IScsiDxe/IScsiProto.c
index 69d1b39dbb1f..e62736bc041f 100644
--- a/NetworkPkg/IScsiDxe/IScsiProto.c
+++ b/NetworkPkg/IScsiDxe/IScsiProto.c
@@ -416,38 +416,41 @@ IScsiGetIp6NicInfo (

return Status;
}

/**
Re-set any stateful session-level authentication information that is used by
the leading login / leading connection.

(Note that this driver only supports a single connection per session -- see
ISCSI_MAX_CONNS_PER_SESSION.)

@param[in,out] Session The iSCSI session.
**/
STATIC
VOID
IScsiSessionResetAuthData (
IN OUT ISCSI_SESSION *Session
)
{
+ if (Session->AuthType == ISCSI_AUTH_TYPE_CHAP) {
+ Session->AuthData.CHAP.Hash = NULL;
+ }
}

/**
Login the iSCSI session.

@param[in] Session The iSCSI session.

@retval EFI_SUCCESS The iSCSI session login procedure finished.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_NO_MEDIA There was a media error.
@retval Others Other errors as indicated.

**/
EFI_STATUS
IScsiSessionLogin (
IN ISCSI_SESSION *Session
)
{
EFI_STATUS Status;
--
2.19.1.3.g30247aa5d201


[PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes

Laszlo Ersek
 

IScsiDxe uses the ISCSI_CHAP_RSP_LEN macro for expressing the size of the
digest (16) that it solely supports at this point (MD5).
ISCSI_CHAP_RSP_LEN is used for both (a) *allocating* digest-related
buffers (binary buffers and hex encodings alike), and (b) *processing*
binary digest buffers (comparing them, filling them, reading them).

In preparation for adding other hash algorithms, split purpose (a) from
purpose (b). For purpose (a) -- buffer allocation --, introduce
ISCSI_CHAP_MAX_DIGEST_SIZE. For purpose (b) -- processing --, rely on
MD5_DIGEST_SIZE from <BaseCryptLib.h>.

Distinguishing these purposes is justified because purpose (b) --
processing -- must depend on the hashing algorithm negotiated between
initiator and target, while for purpose (a) -- allocation --, using the
maximum supported digest size is suitable. For now, because only MD5 is
supported, introduce ISCSI_CHAP_MAX_DIGEST_SIZE *as* MD5_DIGEST_SIZE.

Note that the argument for using the digest size as the size of the
outgoing challenge (in case mutual authentication is desired by the
initiator) remains in place. Because of this, the above two purposes are
distinguished for the "ISCSI_CHAP_AUTH_DATA.OutChallenge" field as well.

This patch is functionally a no-op, just yet.

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/IScsiDxe/IScsiCHAP.h | 17 +++++++++------
NetworkPkg/IScsiDxe/IScsiCHAP.c | 22 ++++++++++----------
2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index d6a90fc27fc3..b8811b7580f0 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -1,86 +1,91 @@
/** @file
The header file of CHAP configuration.

Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_

#define ISCSI_AUTH_METHOD_CHAP "CHAP"

#define ISCSI_KEY_CHAP_ALGORITHM "CHAP_A"
#define ISCSI_KEY_CHAP_IDENTIFIER "CHAP_I"
#define ISCSI_KEY_CHAP_CHALLENGE "CHAP_C"
#define ISCSI_KEY_CHAP_NAME "CHAP_N"
#define ISCSI_KEY_CHAP_RESPONSE "CHAP_R"

+//
+// Identifiers of supported CHAP hash algorithms:
+// https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9
+//
#define ISCSI_CHAP_ALGORITHM_MD5 5

-///
-/// MD5_HASHSIZE
-///
-#define ISCSI_CHAP_RSP_LEN 16
+//
+// Byte count of the largest digest over the above-listed
+// ISCSI_CHAP_ALGORITHM_* hash algorithms.
+//
+#define ISCSI_CHAP_MAX_DIGEST_SIZE MD5_DIGEST_SIZE

#define ISCSI_CHAP_STEP_ONE 1
#define ISCSI_CHAP_STEP_TWO 2
#define ISCSI_CHAP_STEP_THREE 3
#define ISCSI_CHAP_STEP_FOUR 4


#pragma pack(1)

typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {
UINT8 CHAPType;
CHAR8 CHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 CHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
CHAR8 ReverseCHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 ReverseCHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
} ISCSI_CHAP_AUTH_CONFIG_NVDATA;

#pragma pack()

///
/// ISCSI CHAP Authentication Data
///
typedef struct _ISCSI_CHAP_AUTH_DATA {
ISCSI_CHAP_AUTH_CONFIG_NVDATA *AuthConfig;
UINT32 InIdentifier;
UINT8 InChallenge[1024];
UINT32 InChallengeLength;
//
// Calculated CHAP Response (CHAP_R) value.
//
- UINT8 CHAPResponse[ISCSI_CHAP_RSP_LEN];
+ UINT8 CHAPResponse[ISCSI_CHAP_MAX_DIGEST_SIZE];

//
// Auth-data to be sent out for mutual authentication.
//
// While the challenge size is technically independent of the hashing
// algorithm, it is good practice to avoid hashing *fewer bytes* than the
// digest size. In other words, it's good practice to feed *at least as many
// bytes* to the hashing algorithm as the hashing algorithm will output.
//
UINT32 OutIdentifier;
- UINT8 OutChallenge[ISCSI_CHAP_RSP_LEN];
+ UINT8 OutChallenge[ISCSI_CHAP_MAX_DIGEST_SIZE];
} ISCSI_CHAP_AUTH_DATA;

/**
This function checks the received iSCSI Login Response during the security
negotiation stage.

@param[in] Conn The iSCSI connection.

@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.
@retval Others Other errors as indicated.

**/
EFI_STATUS
IScsiCHAPOnRspReceived (
IN ISCSI_CONNECTION *Conn
);
/**
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index bb84f4359d35..744824e63d23 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -96,90 +96,90 @@ IScsiCHAPCalculateResponse (

@param[in] AuthData iSCSI CHAP authentication data.
@param[in] TargetResponse The response from target.

@retval EFI_SUCCESS The response from target passed
authentication.
@retval EFI_SECURITY_VIOLATION The response from target was not expected
value.
@retval Others Other errors as indicated.

**/
EFI_STATUS
IScsiCHAPAuthTarget (
IN ISCSI_CHAP_AUTH_DATA *AuthData,
IN UINT8 *TargetResponse
)
{
EFI_STATUS Status;
UINT32 SecretSize;
- UINT8 VerifyRsp[ISCSI_CHAP_RSP_LEN];
+ UINT8 VerifyRsp[ISCSI_CHAP_MAX_DIGEST_SIZE];

Status = EFI_SUCCESS;

SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
Status = IScsiCHAPCalculateResponse (
AuthData->OutIdentifier,
AuthData->AuthConfig->ReverseCHAPSecret,
SecretSize,
AuthData->OutChallenge,
- ISCSI_CHAP_RSP_LEN, // ChallengeLength
+ MD5_DIGEST_SIZE, // ChallengeLength
VerifyRsp
);

- if (CompareMem (VerifyRsp, TargetResponse, ISCSI_CHAP_RSP_LEN) != 0) {
+ if (CompareMem (VerifyRsp, TargetResponse, MD5_DIGEST_SIZE) != 0) {
Status = EFI_SECURITY_VIOLATION;
}

return Status;
}


/**
This function checks the received iSCSI Login Response during the security
negotiation stage.

@param[in] Conn The iSCSI connection.

@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.
@retval Others Other errors as indicated.

**/
EFI_STATUS
IScsiCHAPOnRspReceived (
IN ISCSI_CONNECTION *Conn
)
{
EFI_STATUS Status;
ISCSI_SESSION *Session;
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
UINT8 *Data;
UINT32 Len;
LIST_ENTRY *KeyValueList;
UINTN Algorithm;
CHAR8 *Identifier;
CHAR8 *Challenge;
CHAR8 *Name;
CHAR8 *Response;
- UINT8 TargetRsp[ISCSI_CHAP_RSP_LEN];
+ UINT8 TargetRsp[ISCSI_CHAP_MAX_DIGEST_SIZE];
UINT32 RspLen;
UINTN Result;

ASSERT (Conn->CurrentStage == ISCSI_SECURITY_NEGOTIATION);
ASSERT (Conn->RspQue.BufNum != 0);

Session = Conn->Session;
AuthData = &Session->AuthData.CHAP;
Len = Conn->RspQue.BufSize;
Data = AllocateZeroPool (Len);
if (Data == NULL) {
return EFI_OUT_OF_RESOURCES;
}
//
// Copy the data in case the data spans over multiple PDUs.
//
NetbufQueCopy (&Conn->RspQue, 0, Len, Data);

//
@@ -324,41 +324,41 @@ IScsiCHAPOnRspReceived (

case ISCSI_CHAP_STEP_FOUR:
ASSERT (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL);
//
// The forth step, CHAP_N=<N> CHAP_R=<R> is received from Target.
//
Name = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_NAME);
if (Name == NULL) {
goto ON_EXIT;
}

Response = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_RESPONSE
);
if (Response == NULL) {
goto ON_EXIT;
}

- RspLen = ISCSI_CHAP_RSP_LEN;
+ RspLen = MD5_DIGEST_SIZE;
Status = IScsiHexToBin (TargetRsp, &RspLen, Response);
- if (EFI_ERROR (Status) || RspLen != ISCSI_CHAP_RSP_LEN) {
+ if (EFI_ERROR (Status) || RspLen != MD5_DIGEST_SIZE) {
Status = EFI_PROTOCOL_ERROR;
goto ON_EXIT;
}

//
// Check the CHAP Name and Response replied by Target.
//
Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
break;

default:
break;
}

ON_EXIT:

if (KeyValueList != NULL) {
IScsiFreeKeyValueList (KeyValueList);
}
@@ -395,45 +395,45 @@ IScsiCHAPToSendReq (
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
CHAR8 ValueStr[256];
CHAR8 *Response;
UINT32 RspLen;
CHAR8 *Challenge;
UINT32 ChallengeLen;
EFI_STATUS BinToHexStatus;

ASSERT (Conn->CurrentStage == ISCSI_SECURITY_NEGOTIATION);

Session = Conn->Session;
AuthData = &Session->AuthData.CHAP;
LoginReq = (ISCSI_LOGIN_REQUEST *) NetbufGetByte (Pdu, 0, 0);
if (LoginReq == NULL) {
return EFI_PROTOCOL_ERROR;
}
Status = EFI_SUCCESS;

- RspLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
+ RspLen = 2 * ISCSI_CHAP_MAX_DIGEST_SIZE + 3;
Response = AllocateZeroPool (RspLen);
if (Response == NULL) {
return EFI_OUT_OF_RESOURCES;
}

- ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
+ ChallengeLen = 2 * ISCSI_CHAP_MAX_DIGEST_SIZE + 3;
Challenge = AllocateZeroPool (ChallengeLen);
if (Challenge == NULL) {
FreePool (Response);
return EFI_OUT_OF_RESOURCES;
}

switch (Conn->AuthStep) {
case ISCSI_AUTH_INITIAL:
//
// It's the initial Login Request. Fill in the key=value pairs mandatory
// for the initial Login Request.
//
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_INITIATOR_NAME,
mPrivate->InitiatorName
);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_SESSION_TYPE, "Normal");
IScsiAddKeyValuePair (
@@ -466,59 +466,59 @@ IScsiCHAPToSendReq (

case ISCSI_CHAP_STEP_THREE:
//
// Third step, send the Login Request with CHAP_N=<N> CHAP_R=<R> or
// CHAP_N=<N> CHAP_R=<R> CHAP_I=<I> CHAP_C=<C> if target authentication is
// required too.
//
// CHAP_N=<N>
//
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_CHAP_NAME,
(CHAR8 *) &AuthData->AuthConfig->CHAPName
);
//
// CHAP_R=<R>
//
BinToHexStatus = IScsiBinToHex (
(UINT8 *) AuthData->CHAPResponse,
- ISCSI_CHAP_RSP_LEN,
+ MD5_DIGEST_SIZE,
Response,
&RspLen
);
ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);

if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
//
// CHAP_I=<I>
//
IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
//
// CHAP_C=<C>
//
- IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
+ IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, MD5_DIGEST_SIZE);
BinToHexStatus = IScsiBinToHex (
(UINT8 *) AuthData->OutChallenge,
- ISCSI_CHAP_RSP_LEN,
+ MD5_DIGEST_SIZE,
Challenge,
&ChallengeLen
);
ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);

Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
}
//
// Set the stage transition flag.
//
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
break;

default:
Status = EFI_PROTOCOL_ERROR;
break;
}

--
2.19.1.3.g30247aa5d201


[PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files

Laszlo Ersek
 

In the next patches, we'll need more room for various macro and parameter
names. For maintaining the current visual alignments, insert some
horizontal whitespace in preparation. "git show -b" produces no output for
this patch; the patch introduces no functional changes.

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/IScsiDxe/IScsiCHAP.h | 24 ++++++++++----------
NetworkPkg/IScsiDxe/IScsiCHAP.c | 12 +++++-----
2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 35d5d6ec29e3..d6a90fc27fc3 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -1,49 +1,49 @@
/** @file
The header file of CHAP configuration.

Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_

-#define ISCSI_AUTH_METHOD_CHAP "CHAP"
+#define ISCSI_AUTH_METHOD_CHAP "CHAP"

-#define ISCSI_KEY_CHAP_ALGORITHM "CHAP_A"
-#define ISCSI_KEY_CHAP_IDENTIFIER "CHAP_I"
-#define ISCSI_KEY_CHAP_CHALLENGE "CHAP_C"
-#define ISCSI_KEY_CHAP_NAME "CHAP_N"
-#define ISCSI_KEY_CHAP_RESPONSE "CHAP_R"
+#define ISCSI_KEY_CHAP_ALGORITHM "CHAP_A"
+#define ISCSI_KEY_CHAP_IDENTIFIER "CHAP_I"
+#define ISCSI_KEY_CHAP_CHALLENGE "CHAP_C"
+#define ISCSI_KEY_CHAP_NAME "CHAP_N"
+#define ISCSI_KEY_CHAP_RESPONSE "CHAP_R"

-#define ISCSI_CHAP_ALGORITHM_MD5 5
+#define ISCSI_CHAP_ALGORITHM_MD5 5

///
/// MD5_HASHSIZE
///
-#define ISCSI_CHAP_RSP_LEN 16
+#define ISCSI_CHAP_RSP_LEN 16

-#define ISCSI_CHAP_STEP_ONE 1
-#define ISCSI_CHAP_STEP_TWO 2
-#define ISCSI_CHAP_STEP_THREE 3
-#define ISCSI_CHAP_STEP_FOUR 4
+#define ISCSI_CHAP_STEP_ONE 1
+#define ISCSI_CHAP_STEP_TWO 2
+#define ISCSI_CHAP_STEP_THREE 3
+#define ISCSI_CHAP_STEP_FOUR 4


#pragma pack(1)

typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {
UINT8 CHAPType;
CHAR8 CHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 CHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
CHAR8 ReverseCHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 ReverseCHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
} ISCSI_CHAP_AUTH_CONFIG_NVDATA;

#pragma pack()

///
/// ISCSI CHAP Authentication Data
///
typedef struct _ISCSI_CHAP_AUTH_DATA {
ISCSI_CHAP_AUTH_CONFIG_NVDATA *AuthConfig;
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index 7e930c0d1eab..bb84f4359d35 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -14,44 +14,44 @@ SPDX-License-Identifier: BSD-2-Clause-Patent

@param[in] ChapIdentifier iSCSI CHAP identifier sent by authenticator.
@param[in] ChapSecret iSCSI CHAP secret of the authenticator.
@param[in] SecretLength The length of iSCSI CHAP secret.
@param[in] ChapChallenge The challenge message sent by authenticator.
@param[in] ChallengeLength The length of iSCSI CHAP challenge message.
@param[out] ChapResponse The calculation of the expected hash value.

@retval EFI_SUCCESS The expected hash value was calculatedly
successfully.
@retval EFI_PROTOCOL_ERROR The length of the secret should be at least
the length of the hash value for the hashing
algorithm chosen.
@retval EFI_PROTOCOL_ERROR MD5 hash operation fail.
@retval EFI_OUT_OF_RESOURCES Fail to allocate resource to complete MD5.

**/
EFI_STATUS
IScsiCHAPCalculateResponse (
- IN UINT32 ChapIdentifier,
- IN CHAR8 *ChapSecret,
- IN UINT32 SecretLength,
- IN UINT8 *ChapChallenge,
- IN UINT32 ChallengeLength,
- OUT UINT8 *ChapResponse
+ IN UINT32 ChapIdentifier,
+ IN CHAR8 *ChapSecret,
+ IN UINT32 SecretLength,
+ IN UINT8 *ChapChallenge,
+ IN UINT32 ChallengeLength,
+ OUT UINT8 *ChapResponse
)
{
UINTN Md5ContextSize;
VOID *Md5Ctx;
CHAR8 IdByte[1];
EFI_STATUS Status;

if (SecretLength < ISCSI_CHAP_SECRET_MIN_LEN) {
return EFI_PROTOCOL_ERROR;
}

Md5ContextSize = Md5GetContextSize ();
Md5Ctx = AllocatePool (Md5ContextSize);
if (Md5Ctx == NULL) {
return EFI_OUT_OF_RESOURCES;
}

Status = EFI_PROTOCOL_ERROR;

--
2.19.1.3.g30247aa5d201


[PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login

Laszlo Ersek
 

RFC 7143 explains that a single iSCSI session may use multiple TCP
connections. The first connection established is called the leading
connection. The login performed on the leading connection is called the
leading login. Before the session is considered full-featured, the leading
login must succeed. Further (non-leading) connections can be associated
with the session later.

(It's unclear to me from RFC 7143 whether the non-leading connections
require individual (non-leading) logins as well, but that particular
question is irrelevant from the perspective of this patch; see below.)

The data model in IScsiDxe exhibits some confusion, regarding connection /
session association:

- On one hand, the "ISCSI_SESSION.Conns" field is a *set* (it has type
LIST_ENTRY), and accordingly, connections can be added to, and removed
from, a session, with the IScsiAttatchConnection() and
IScsiDetatchConnection() functions.

- On the other hand, ISCSI_MAX_CONNS_PER_SESSION has value 1, therefore no
session will ever use more than 1 connection at a time (refer to
instances of "Session->MaxConnections" in
"NetworkPkg/IScsiDxe/IScsiProto.c").

This one-to-many confusion between ISCSI_SESSION and ISCSI_CONNECTION is
very visible in the CHAP logic, where the progress of the authentication
is maintained *per connection*, in the "ISCSI_CONNECTION.AuthStep" field
(with values such as ISCSI_AUTH_INITIAL, ISCSI_CHAP_STEP_ONE, etc), but
the *data* for the authentication are maintained *per session*, in the
"AuthType" and "AuthData" fields of ISCSI_SESSION. Clearly, this makes no
sense if multiple connections are eligible for logging in.

Knowing that IScsiDxe uses only one connection per session (put
differently: knowing that any connection is a leading connection, and any
login is a leading login), there is no functionality bug. But the data
model is still broken: "AuthType", "AuthData", and "AuthStep" should be
maintained at the *same* level -- be it "session-level" or "(leading)
connection-level".

Fixing this data model bug is more than what I'm signing up for. However,
I do need to add one function, in preparation for multi-hash support:
whenever a new login is attempted (put differently: whenever the leading
login is re-attempted), which always happens with a fresh connection, the
session-level authentication data needs to be rewound to a sane initial
state.

Introduce the IScsiSessionResetAuthData() function. Call it from the
central -- session-level -- IScsiSessionLogin() function, just before the
latter calls the -- connection-level -- IScsiConnLogin() function.

Right now, do nothing in IScsiSessionResetAuthData(); so functionally
speaking, the patch is a no-op.

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/IScsiDxe/IScsiProto.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)

diff --git a/NetworkPkg/IScsiDxe/IScsiProto.c b/NetworkPkg/IScsiDxe/IScsiProto.c
index 6983f0fa5973..69d1b39dbb1f 100644
--- a/NetworkPkg/IScsiDxe/IScsiProto.c
+++ b/NetworkPkg/IScsiDxe/IScsiProto.c
@@ -401,38 +401,55 @@ IScsiGetIp6NicInfo (
if (Ip6ModeData.GroupTable!= NULL) {
FreePool (Ip6ModeData.GroupTable);
}
if (Ip6ModeData.RouteTable!= NULL) {
FreePool (Ip6ModeData.RouteTable);
}
if (Ip6ModeData.NeighborCache!= NULL) {
FreePool (Ip6ModeData.NeighborCache);
}
if (Ip6ModeData.PrefixTable!= NULL) {
FreePool (Ip6ModeData.PrefixTable);
}
if (Ip6ModeData.IcmpTypeList!= NULL) {
FreePool (Ip6ModeData.IcmpTypeList);
}

return Status;
}

+/**
+ Re-set any stateful session-level authentication information that is used by
+ the leading login / leading connection.
+
+ (Note that this driver only supports a single connection per session -- see
+ ISCSI_MAX_CONNS_PER_SESSION.)
+
+ @param[in,out] Session The iSCSI session.
+**/
+STATIC
+VOID
+IScsiSessionResetAuthData (
+ IN OUT ISCSI_SESSION *Session
+ )
+{
+}
+
/**
Login the iSCSI session.

@param[in] Session The iSCSI session.

@retval EFI_SUCCESS The iSCSI session login procedure finished.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_NO_MEDIA There was a media error.
@retval Others Other errors as indicated.

**/
EFI_STATUS
IScsiSessionLogin (
IN ISCSI_SESSION *Session
)
{
EFI_STATUS Status;
ISCSI_CONNECTION *Conn;
VOID *Tcp;
@@ -454,38 +471,39 @@ IScsiSessionLogin (
//
CopyMem (Session->Isid, Session->ConfigData->SessionConfigData.IsId, 6);

RetryCount = 0;

do {
//
// Create a connection for the session.
//
Conn = IScsiCreateConnection (Session);
if (Conn == NULL) {
return EFI_OUT_OF_RESOURCES;
}

IScsiAttatchConnection (Session, Conn);

//
// Login through the newly created connection.
//
+ IScsiSessionResetAuthData (Session);
Status = IScsiConnLogin (Conn, Session->ConfigData->SessionConfigData.ConnectTimeout);
if (EFI_ERROR (Status)) {
IScsiConnReset (Conn);
IScsiDetatchConnection (Conn);
IScsiDestroyConnection (Conn);
}

if (Status != EFI_TIMEOUT) {
break;
}

RetryCount++;
} while (RetryCount <= Session->ConfigData->SessionConfigData.ConnectRetryCount);

if (!EFI_ERROR (Status)) {
Session->State = SESSION_STATE_LOGGED_IN;

if (!Conn->Ipv6Flag) {
ProtocolGuid = &gEfiTcp4ProtocolGuid;
--
2.19.1.3.g30247aa5d201


[PATCH 0/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP

Laszlo Ersek
 

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3355
Repo: https://pagure.io/lersek/edk2.git
Branch: iscsi_sha256_bz3355

Please find the Feature Request described in comment#0 of the BZ.

The patch series depends on:

[edk2-devel] [PUBLIC edk2 PATCH v2 00/10]
NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs

https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Message-Id: <20210608121259.32451-1-lersek@redhat.com>
https://listman.redhat.com/archives/edk2-devel-archive/2021-June/msg00316.html
https://edk2.groups.io/g/devel/message/76198

Please find the test matrix *template* in comment#2 of the BZ.

Actual test results, with this series applied:

Tests with no authentication Results
---------------------------- -------------------------
login result test result
------------ -----------
ok PASS


Tests with mutual authentication Results
---------------------------------- --------------------------------------
secret of ...
matches CHAP_A
target initiator ------------- -------------------
supports supports offered picked login test
SHA256 MD5 target init. by init. by target result result
-------- --------- ------ ----- -------- --------- --------- ------
no no n/a n/a 7 n/a targ abrt PASS
no yes no n/a 7,5 5 targ abrt PASS
no yes yes no 7,5 5 init abrt PASS
no yes yes yes 7,5 5 ok PASS
yes no no n/a 7 7 targ abrt PASS
yes no yes no 7 7 init abrt PASS
yes no yes yes 7 7 ok PASS
yes yes no n/a 7,5 7 targ abrt PASS
yes yes yes no 7,5 7 init abrt PASS
yes yes yes yes 7,5 7 ok PASS

Notes:

- iSCSI communication was monitored with wireshark.

- RHEL-7.6 was used as the target without SHA256 support. RHEL-7.9 was
used as the target with SHA256 support.

- The expression "initiator doesn't support MD5" means building the
series with "-D NETWORK_ISCSI_MD5_ENABLE=FALSE".

- SHA256 support is always present in the initiator (simply by virtue of
the series being applied). MD5 support is always present in the
target.

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>

Thanks,
Laszlo

Laszlo Ersek (6):
NetworkPkg/IScsiDxe: re-set session-level authentication state before
login
NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files
NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest
sizes
NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP
NetworkPkg/IScsiDxe: support SHA256 in CHAP
NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro

NetworkPkg/IScsiDxe/IScsiCHAP.c | 192 ++++++++++++++++----
NetworkPkg/IScsiDxe/IScsiCHAP.h | 95 ++++++++--
NetworkPkg/IScsiDxe/IScsiDriver.c | 2 +
NetworkPkg/IScsiDxe/IScsiProto.c | 21 +++
NetworkPkg/NetworkBuildOptions.dsc.inc | 2 +-
NetworkPkg/NetworkDefines.dsc.inc | 20 ++
6 files changed, 281 insertions(+), 51 deletions(-)

--
2.19.1.3.g30247aa5d201


Re: [PATCH v1 0/8] Measured SEV boot with kernel/initrd/cmdline

Ard Biesheuvel
 

On Tue, 8 Jun 2021 at 12:59, Laszlo Ersek <lersek@redhat.com> wrote:

Ard,

do you have any comments please, on the topic at the bottom?

My comments follow:

On 06/08/21 11:57, Dov Murik wrote:


On 04/06/2021 14:26, Laszlo Ersek wrote:
On 06/04/21 12:30, Dov Murik wrote:
...


[Ard, please see this one question:]

- A major complication for hashing all three of: kernel, initrd,
cmdline, is that the *fetching* of this triplet is split between
two places. (Well, it is split between *three* places in fact, but
I'm going to ignore LinuxInitrdDynamicShellCommand for now, because
the AmdSevX64 platform sets BUILD_SHELL to FALSE for production.)

The kernel and the initrd are fetched in QemuKernelLoaderFsDxe, but
the command line is fetched in (both) QemuLoadImageLib instances.
This requires that all these modules be littered with hashing as
well, which I find *really bad*. Even if we factor out the actual
logic, I strongly dislike having *just hooks* for hashing in
multiple modules.

Now, please refer to efc52d67e157 ("OvmfPkg/QemuKernelLoaderFsDxe:
don't expose kernel command line", 2020-03-05). If we first

(a) reverted that commit, and

(b) modified *both* QemuLoadImageLib instances, to load the kernel
command line from the *synthetic filesystem* (rather than directly
from fw_cfg),

then we could centralize the hashing to just QemuKernelLoaderFsDxe.

Ard -- what's your thought on this?
I understand there's agreement here, and that both this suggested
change (use the synthetic filesystem) and my patch series (add hash
verification) touch the same code areas. How do you envision this
process in the mailing list? Seperate patch serieses with
dependency? One long patch series with both changes? What goes
first?
Good point. I do have a kind of patch order laid out in my mind, but
I didn't think of whether we should have the patches in one patch
series, or in two "waves".

OK, let's go with two patch sets.

In the first set, we should just focus on the above steps (a) and
(b). Step (a) shouldn't be too hard. In step (b), you'd modify both
QemuLoadImageLib instances (two separate patches), replacing the
QemuFwCfgLib APIs for fetching the cmdline with
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and EFI_FILE_PROTOCOL APIs.

Speaking from memory, the synthetic filesystem has a unique device
path, so the first step would be calling gBS->LocateDevicePath(), for
finding SimpleFs on the unique device path. Once you have the
SimpleFs interface, you can call OpenVolume, then open the "cmdline"
file using the EFI_FILE_PROTOCOL output by OpenVolume.

Once we merge this series (basically just three patches), there is no
QemuFwCfgLib dependency left in either QemuLoadImageLib instance, I
reckon.
I started working on that, and managed to remove all QemuFwCfg* calls
in the main path of QemuLoadKernelImage (so far working on
X86QemuLoadImageLib.c). That works fine: I read the content of the
"cmdline" synthetic file, and I check the size of the synthetic
"initrd" file. I used Library/FileHandleLib.h; I hope that's fine.
The lib class header says "Provides interface to EFI_FILE_HANDLE
functionality", which is not too bad; but I don't immediately see what
those APIs save us -- the APIs that I believe to be relevant to this use
case all seem to be thin wrappers around EFI_FILE_PROTOCOL. (The only
instance of FileHandleLib is
"MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf".) While not
necessarily a problem, it doesn't seem an obvious win (unless it saves
you much complexity and/or code in a way that I'm missing).

In OVMF, the following executables use UefiFileHandleLib at the moment:

- MdeModulePkg/Universal/BdsDxe/BdsDxe.inf
- ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf
- ShellPkg/DynamicCommand/HttpDynamicCommand/HttpDynamicCommand.inf
- OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
- ShellPkg/Application/Shell/Shell.inf

The last four are shell-related, so "prior art" is really just BdsDxe...

However, there's another path (which I don't reach with my test
setup), which is the call to QemuLoadLegacyImage, which has a lot of
calls to QemuFwCfg* in its body.

Am I expected to change that legacy path as well?
Or is it in a "it's working don't touch" state?
If I modify this, how do I test it?
The use case that you foresee for this feature is really important here.

When you say that you don't reach QemuLoadLegacyImage(), that means your
guest kernel is built with the UEFI stub.

(1) If you can make the Linux UEFI stub a *required* part of the use
case, then:

(1a) switch the QemuLoadImageLib class resolution from
"OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf" to
"OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf" in
"OvmfPkg/AmdSev/AmdSevX64.dsc",

(1b) modify "OvmfPkg/Library/GenericQemuLoadImageLib" only (your
modifications thus far should be easy to transplant to this lib
instance); ignore "OvmfPkg/Library/X86QemuLoadImageLib". There is no
QemuLoadLegacyImage() in GenericQemuLoadImageLib.

This makes sense especially because "OvmfPkg/AmdSev/AmdSevX64.dsc" does
not offer Secure Boot support, so there's not going to be a case when
gBS->LoadImage() rejects the UEFI stubbed Linux binary due to Secure
Boot verification failing (EFI_SECURITY_VIOLATION, EFI_ACCESS_DENIED).


(2) If you cannot make the Linux UEFI stub a required part of the use
case, then X86QemuLoadImageLib needs to be modified indeed.

(2a) Unfortunately, in this case we have to add a hack, because the
synthetic filesystem only exposes the concatenated "setup data + kernel
image" blob. The following would have to be preserved (as the only
remaining QemuFwCfgLib access):

QemuFwCfgSelectItem (QemuFwCfgItemKernelSetupSize);
SetupSize = (UINTN)QemuFwCfgRead32 ();

(2b) and then the kernel blob from the synthetic fs would have to be
split in two (= setup, kernel), within QemuLoadLegacyImage().


I'm sorry for missing this aspect previously! I really hope we can go
with (1)!
The legacy x86 loader should only be kept if there is a strong need to
do so. Keeping it around for some theoretical compatibility concern is
simply not worth it, IMHO.

Having two boot paths to reason about in terms of security is not the
only problem: another concern is that the legacy fallback path is
strictly x86, and is tightly coupled with the kernel's struct
boot_params, which is only documented by the .h file that happens to
declare it (and some outdated prose under Documentation/ perhaps)

Also, the EFI stub does some non-trivial work at boot, and having this
uniform between architectures is an important goal, especially for
non-x86 folks like me. We have introduced an initrd loader mechanism
that is fully arch agnostic, and there are patches on the list to move
the measurement of the initrd into the EFI stub if it was loaded using
this mechanism.

In summary, please stick with the generic loader unless you *really* can't.

--
Ard.


Re: [PATCH edk2-test 1/1] uefi-sct/SctPkg: IHV: type mismatch in Simple Network test

G Edhaya Chandran
 

This patch is upstreamed by the commit-id: 
https://github.com/tianocore/edk2-test/commit/b68e40293485ce3c6bf50294900d068bbaeba213


Re: [PATCH edk2-test 1/1] uefi-sct/SctPkg: IHV: type mismatch in Simple Network test

G Edhaya Chandran
 

Reviewed-by: G Edhaya Chandran<edhaya.chandran@...>


Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

Min Xu
 

On 06/04/2021 12:12 AM, Laszlo wrote:

(22) EmuVariableFvbRuntimeDxe

Ouch, this is an unpleasant surprise.

First, if you know for a fact that pflash is not part of the *board* in any TDX
setup, then pulling

OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf

into the firmware platform is useless, as it is mutually exclusive with

OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf

(via dynamic means -- a dynamic PCD).

Note that the FDF file places QemuFlashFvbServicesRuntimeDxe in APRIORI
DXE when SMM_REQUIRE is FALSE. This driver checks for pflash presence,
and lets EmuVariableFvbRuntimeDxe perform its in-RAM flash emulation
only in case pflash is not found.
Right, pflash is not part of the *board* in any TDX setup.
This is a limitation from the Qemu (the tdx-qemu) side. TDVF is copied into
RAM. Generic loader is the one for it.
In this case (pflash is not found), FvbServiceRuntimeDxe.inf will return
EFI_WRITE_PROTECTED in its FvbInitialize().
EmuVariableFvbRuntimeDxe will then perform its in-RAM flash emulation.

So this is again in favor of a separate platform -- if we know pflash is never
part of the board, then QemuFlashFvbServicesRuntimeDxe is never needed,
but you cannot remove it from the traditional DSC/FDF files.
Yes, if TDVF is a separate DSD/FDF, QemuFlashFvbServicesRuntimeDxe will be
removed from the image.

Second, EmuVariableFvbRuntimeDxe consumes the PlatformFvbLib class, for
the PlatformFvbDataWritten() API (among other things). This lib class is
implemented by two instances in OvmfPkg, PlatformFvbLibNull and
EmuVariableFvbLib. The latter instance allows Platform BDS to hook an event
(for signaling) via "PcdEmuVariableEvent" into the
EmuVariableFvbRuntimeDxe driver.

In old (very old) QEMU board configurations, namely those without pflash,
this (mis)feature is used by OVMF's PlatformBootManagerLib to write out all
variables to the EFI system partition in a regular file called \NvVars, with the
help of NvVarsFileLib, whenever EmuVariableFvbRuntimeDxe writes out an
emulated "flash" block. For this purpose, the traditional OVMF DSC files link
EmuVariableFvbLib into EmuVariableFvbRuntimeDxe.
Thanks for the explanation. It's very helpful for me to understand the code.

But it counts as an absolute disaster nowadays, and should not be revived in
any platform. If you don't have pflash in TDX guests, just accept that you
won't have non-volatile variables. And link PlatformFvbLibNull into
EmuVariableFvbRuntimeDxe. You're going to need a separate
PlatformBootManagerLib instance anyway.
I have a question here, that if PlatformFvbLibNull is linked into EmuVaiableFvRuntimeDxe,
Does it mean it cannot write to the in-RAM variable store?


(We should have removed EmuVariableFvbRuntimeDxe a long time ago from
the traditional OVMF platforms, i.e. made pflash a hard requirement, even
when SMM is not built into the platform -- but whenever I tried that, Jordan
always shot me down.)
I am afraid in TDVF we have to use EmuVariableFvRuntimeDxe to emulate the
in-RAM, as I explained pflash is not part of the *board* in TDX setup.
In the traditional EmuVariableFvRuntimeDxe, the FV contents will be initialized.
But TDVF has its own requirement, that the SB keys in CFV need to be copied
into the FV contents. That's why EmuVariableFvRuntimeDxe is updated in
TDVF project.


My point is: using EmuVariableFvbRuntimeDxe in the TDX platform may be
defensible per se, but we must be very clear that it will never provide a
standards-conformant service for non-volatile UEFI variables, and we must
keep as much of the \NvVars mess out of EmuVariableFvbRuntimeDxe as
possible. This will require a separate PlatformBootManagerLib instance for
TDX anyway (or maybe share PlatformBootManagerLibGrub with
"OvmfPkg/AmdSev/AmdSevX64.dsc").

Apart from the volatility aspect, let's assume we have this in-RAM emulated
"flash device", storing authenticated UEFI variables for Secure Boot purposes.
And we don't have SMM.

What protects this in-RAM variable store from tampering by the guest OS?
It's all guest RAM only, after all. What provides the privilege barrier between
the guest firmware and the guest OS?
A good question and we will answer it a bit later. Thanks for your patience.

Thanks
Laszlo


[PUBLIC edk2 PATCH v2 10/10] NetworkPkg/IScsiDxe: check IScsiHexToBin() return values

Laszlo Ersek
 

IScsiDxe (that is, the initiator) receives two hex-encoded strings from
the iSCSI target:

- CHAP_C, where the target challenges the initiator,

- CHAP_R, where the target answers the challenge from the initiator (in
case the initiator wants mutual authentication).

Accordingly, we have two IScsiHexToBin() call sites:

- At the CHAP_C decoding site, check whether the decoding succeeds. The
decoded buffer ("AuthData->InChallenge") can accommodate 1024 bytes,
which is a permissible restriction on the target, per
<https://tools.ietf.org/html/rfc7143#section-12.1.3>. Shorter challenges
from the target are acceptable.

- At the CHAP_R decoding site, enforce that the decoding both succeed, and
provide exactly ISCSI_CHAP_RSP_LEN bytes. CHAP_R contains the digest
calculated by the target, therefore it must be of fixed size. We may
only call IScsiCHAPAuthTarget() if "TargetRsp" has been fully populated.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index dbe3c8ef46f9..7e930c0d1eab 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -274,43 +274,47 @@ IScsiCHAPOnRspReceived (

Challenge = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_CHALLENGE
);
if (Challenge == NULL) {
goto ON_EXIT;
}
//
// Process the CHAP identifier and CHAP Challenge from Target.
// Calculate Response value.
//
Result = IScsiNetNtoi (Identifier);
if (Result > 0xFF) {
goto ON_EXIT;
}

AuthData->InIdentifier = (UINT32) Result;
AuthData->InChallengeLength = (UINT32) sizeof (AuthData->InChallenge);
- IScsiHexToBin (
- (UINT8 *) AuthData->InChallenge,
- &AuthData->InChallengeLength,
- Challenge
- );
+ Status = IScsiHexToBin (
+ (UINT8 *) AuthData->InChallenge,
+ &AuthData->InChallengeLength,
+ Challenge
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_PROTOCOL_ERROR;
+ goto ON_EXIT;
+ }
Status = IScsiCHAPCalculateResponse (
AuthData->InIdentifier,
AuthData->AuthConfig->CHAPSecret,
(UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
AuthData->InChallenge,
AuthData->InChallengeLength,
AuthData->CHAPResponse
);

//
// Transit to next step.
//
Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
break;

case ISCSI_CHAP_STEP_THREE:
//
// One way CHAP authentication and the target would like to
// authenticate us.
@@ -321,39 +325,43 @@ IScsiCHAPOnRspReceived (
case ISCSI_CHAP_STEP_FOUR:
ASSERT (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL);
//
// The forth step, CHAP_N=<N> CHAP_R=<R> is received from Target.
//
Name = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_NAME);
if (Name == NULL) {
goto ON_EXIT;
}

Response = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_RESPONSE
);
if (Response == NULL) {
goto ON_EXIT;
}

RspLen = ISCSI_CHAP_RSP_LEN;
- IScsiHexToBin (TargetRsp, &RspLen, Response);
+ Status = IScsiHexToBin (TargetRsp, &RspLen, Response);
+ if (EFI_ERROR (Status) || RspLen != ISCSI_CHAP_RSP_LEN) {
+ Status = EFI_PROTOCOL_ERROR;
+ goto ON_EXIT;
+ }

//
// Check the CHAP Name and Response replied by Target.
//
Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
break;

default:
break;
}

ON_EXIT:

if (KeyValueList != NULL) {
IScsiFreeKeyValueList (KeyValueList);
}

FreePool (Data);

--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 09/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow

Laszlo Ersek
 

The IScsiHexToBin() function documents the EFI_BUFFER_TOO_SMALL return
condition, but never actually checks whether the decoded buffer fits into
the caller-provided room (i.e., the input value of "BinLength"), and
EFI_BUFFER_TOO_SMALL is never returned. The decoding of "HexStr" can
overflow "BinBuffer".

This is remotely exploitable, as shown in a subsequent patch, which adds
error checking to the IScsiHexToBin() call sites. This issue allows the
target to compromise the initiator.

Introduce EFI_BAD_BUFFER_SIZE, in addition to the existent
EFI_BUFFER_TOO_SMALL, for reporting a special case of the buffer overflow,
plus actually catch the buffer overflow.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 3 +++
NetworkPkg/IScsiDxe/IScsiMisc.c | 20 +++++++++++++++++---
2 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 404a482e57f3..fddef4f466dc 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -156,38 +156,41 @@ IScsiAsciiStrToIp (
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);

/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
@retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
+ @retval EFI_BAD_BUFFER_SIZE The length of HexStr is too large for decoding:
+ the decoded size cannot be expressed in
+ BinLength on output.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

@return The numerical value.

**/
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index f0f4992b07c7..406954786751 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -361,89 +361,103 @@ IScsiBinToHex (
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}

HexStr[Index * 2 + 2] = '\0';

return EFI_SUCCESS;
}


/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
@retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
+ @retval EFI_BAD_BUFFER_SIZE The length of HexStr is too large for decoding:
+ the decoded size cannot be expressed in
+ BinLength on output.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
+ UINTN BinLengthMin;
+ UINT32 BinLengthProvided;
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];

ZeroMem (TemStr, sizeof (TemStr));

//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
HexStr += 2;
}

Length = AsciiStrLen (HexStr);

//
// Reject an empty hex string; reject a stray nibble.
//
if (Length == 0 || Length % 2 != 0) {
return EFI_INVALID_PARAMETER;
}
+ //
+ // Check if the caller provides enough room for the decoded blob.
+ //
+ BinLengthMin = Length / 2;
+ if (BinLengthMin > MAX_UINT32) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+ BinLengthProvided = *BinLength;
+ *BinLength = (UINT32)BinLengthMin;
+ if (BinLengthProvided < BinLengthMin) {
+ return EFI_BUFFER_TOO_SMALL;
+ }

for (Index = 0; Index < Length; Index ++) {
TemStr[0] = HexStr[Index];
Digit = (UINT8) AsciiStrHexToUint64 (TemStr);
if (Digit == 0 && TemStr[0] != '0') {
//
// Invalid Hex Char.
//
return EFI_INVALID_PARAMETER;
}
if ((Index & 1) == 0) {
BinBuffer [Index/2] = Digit;
} else {
BinBuffer [Index/2] = (UINT8) ((BinBuffer [Index/2] << 4) + Digit);
}
}
-
- *BinLength = (UINT32) ((Index + 1)/2);
-
return EFI_SUCCESS;
}


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

@return The numerical value.

**/
UINTN
IScsiNetNtoi (
IN CHAR8 *Str
)
{
if ((Str[0] == '0') && ((Str[1] == 'x') || (Str[1] == 'X'))) {
Str += 2;
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 08/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing

Laszlo Ersek
 

The IScsiHexToBin() function has the following parser issues:

(1) If the *subject sequence* in "HexStr" is empty, the function returns
EFI_SUCCESS (with "BinLength" set to 0 on output). Such inputs should
be rejected.

(2) The function mis-handles a "HexStr" that ends with a stray nibble. For
example, if "HexStr" is "0xABC", the function decodes it to the bytes
{0xAB, 0x0C}, sets "BinLength" to 2 on output, and returns
EFI_SUCCESS. Such inputs should be rejected.

(3) If an invalid hex char is found in "HexStr", the function treats it as
end-of-hex-string, and returns EFI_SUCCESS. Such inputs should be
rejected.

All of the above cases are remotely triggerable, as shown in a subsequent
patch, which adds error checking to the IScsiHexToBin() call sites. While
the initiator is not immediately compromised, incorrectly parsing CHAP_R
from the target, in case of mutual authentication, is not great.

Extend the interface contract of IScsiHexToBin() with
EFI_INVALID_PARAMETER, for reporting issues (1) through (3), and implement
the new checks.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 28cf408cd5c5..404a482e57f3 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -155,38 +155,39 @@ IScsiAsciiStrToIp (

**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);

/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
+ @retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

@return The numerical value.

**/
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index 014700e87a5f..f0f4992b07c7 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -360,72 +360,80 @@ IScsiBinToHex (
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}

HexStr[Index * 2 + 2] = '\0';

return EFI_SUCCESS;
}


/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
+ @retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];

ZeroMem (TemStr, sizeof (TemStr));

//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
HexStr += 2;
}

Length = AsciiStrLen (HexStr);

+ //
+ // Reject an empty hex string; reject a stray nibble.
+ //
+ if (Length == 0 || Length % 2 != 0) {
+ return EFI_INVALID_PARAMETER;
+ }
+
for (Index = 0; Index < Length; Index ++) {
TemStr[0] = HexStr[Index];
Digit = (UINT8) AsciiStrHexToUint64 (TemStr);
if (Digit == 0 && TemStr[0] != '0') {
//
- // Invalid Lun Char.
+ // Invalid Hex Char.
//
- break;
+ return EFI_INVALID_PARAMETER;
}
if ((Index & 1) == 0) {
BinBuffer [Index/2] = Digit;
} else {
BinBuffer [Index/2] = (UINT8) ((BinBuffer [Index/2] << 4) + Digit);
}
}

*BinLength = (UINT32) ((Index + 1)/2);

return EFI_SUCCESS;
}


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 06/10] NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds

Laszlo Ersek
 

IScsiBinToHex() is called for encoding:

- the answer to the target's challenge; that is, CHAP_R;

- the challenge for the target, in case mutual authentication is enabled;
that is, CHAP_C.

The initiator controls the size of both blobs, the sizes of their hex
encodings are correctly calculated in "RspLen" and "ChallengeLen".
Therefore the IScsiBinToHex() calls never fail; assert that.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 27 +++++++++++---------
1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index 9e192ce292e8..dbe3c8ef46f9 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -375,38 +375,39 @@ IScsiCHAPOnRspReceived (
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.

**/
EFI_STATUS
IScsiCHAPToSendReq (
IN ISCSI_CONNECTION *Conn,
IN OUT NET_BUF *Pdu
)
{
EFI_STATUS Status;
ISCSI_SESSION *Session;
ISCSI_LOGIN_REQUEST *LoginReq;
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
CHAR8 ValueStr[256];
CHAR8 *Response;
UINT32 RspLen;
CHAR8 *Challenge;
UINT32 ChallengeLen;
+ EFI_STATUS BinToHexStatus;

ASSERT (Conn->CurrentStage == ISCSI_SECURITY_NEGOTIATION);

Session = Conn->Session;
AuthData = &Session->AuthData.CHAP;
LoginReq = (ISCSI_LOGIN_REQUEST *) NetbufGetByte (Pdu, 0, 0);
if (LoginReq == NULL) {
return EFI_PROTOCOL_ERROR;
}
Status = EFI_SUCCESS;

RspLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
Response = AllocateZeroPool (RspLen);
if (Response == NULL) {
return EFI_OUT_OF_RESOURCES;
}

ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
Challenge = AllocateZeroPool (ChallengeLen);
@@ -455,63 +456,65 @@ IScsiCHAPToSendReq (
Conn->AuthStep = ISCSI_CHAP_STEP_TWO;
break;

case ISCSI_CHAP_STEP_THREE:
//
// Third step, send the Login Request with CHAP_N=<N> CHAP_R=<R> or
// CHAP_N=<N> CHAP_R=<R> CHAP_I=<I> CHAP_C=<C> if target authentication is
// required too.
//
// CHAP_N=<N>
//
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_CHAP_NAME,
(CHAR8 *) &AuthData->AuthConfig->CHAPName
);
//
// CHAP_R=<R>
//
- IScsiBinToHex (
- (UINT8 *) AuthData->CHAPResponse,
- ISCSI_CHAP_RSP_LEN,
- Response,
- &RspLen
- );
+ BinToHexStatus = IScsiBinToHex (
+ (UINT8 *) AuthData->CHAPResponse,
+ ISCSI_CHAP_RSP_LEN,
+ Response,
+ &RspLen
+ );
+ ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);

if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
//
// CHAP_I=<I>
//
IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
//
// CHAP_C=<C>
//
IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
- IScsiBinToHex (
- (UINT8 *) AuthData->OutChallenge,
- ISCSI_CHAP_RSP_LEN,
- Challenge,
- &ChallengeLen
- );
+ BinToHexStatus = IScsiBinToHex (
+ (UINT8 *) AuthData->OutChallenge,
+ ISCSI_CHAP_RSP_LEN,
+ Challenge,
+ &ChallengeLen
+ );
+ ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);

Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
}
//
// Set the stage transition flag.
//
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
break;

default:
Status = EFI_PROTOCOL_ERROR;
break;
}

FreePool (Response);
FreePool (Challenge);

return Status;
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 07/10] NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block

Laszlo Ersek
 

We'll need further return values for IScsiHexToBin() in a subsequent
patch; make room for them in the leading comment block of the function.
While at it, rewrap the comment block to 80 characters width.

No functional changes.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 14 +++++++-------
NetworkPkg/IScsiDxe/IScsiMisc.c | 14 +++++++-------
2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 231413993b08..28cf408cd5c5 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -149,46 +149,46 @@ IScsiAsciiStrToIp (

@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
@retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.

**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);

/**
Convert the hexadecimal string into a binary encoded buffer.

- @param[in, out] BinBuffer The binary buffer.
- @param[in, out] BinLength Length of the binary buffer.
- @param[in] HexStr The hexadecimal string.
-
- @retval EFI_SUCCESS The hexadecimal string is converted into a binary
- encoded buffer.
- @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the converted data.
+ @param[in, out] BinBuffer The binary buffer.
+ @param[in, out] BinLength Length of the binary buffer.
+ @param[in] HexStr The hexadecimal string.

+ @retval EFI_SUCCESS The hexadecimal string is converted into a
+ binary encoded buffer.
+ @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
+ converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);


/**
Convert the decimal-constant string or hex-constant string into a numerical value.

@param[in] Str String in decimal or hex.

@return The numerical value.

**/
UINTN
IScsiNetNtoi (
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index 42988e15cb06..014700e87a5f 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -354,46 +354,46 @@ IScsiBinToHex (
// Prefix for Hex String.
//
HexStr[0] = '0';
HexStr[1] = 'x';

for (Index = 0; Index < BinLength; Index++) {
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}

HexStr[Index * 2 + 2] = '\0';

return EFI_SUCCESS;
}


/**
Convert the hexadecimal string into a binary encoded buffer.

- @param[in, out] BinBuffer The binary buffer.
- @param[in, out] BinLength Length of the binary buffer.
- @param[in] HexStr The hexadecimal string.
-
- @retval EFI_SUCCESS The hexadecimal string is converted into a binary
- encoded buffer.
- @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the converted data.
+ @param[in, out] BinBuffer The binary buffer.
+ @param[in, out] BinLength Length of the binary buffer.
+ @param[in] HexStr The hexadecimal string.

+ @retval EFI_SUCCESS The hexadecimal string is converted into a
+ binary encoded buffer.
+ @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
+ converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];

ZeroMem (TemStr, sizeof (TemStr));

//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 04/10] NetworkPkg/IScsiDxe: clean up library class dependencies

Laszlo Ersek
 

Sort the library class dependencies in the #include directives and in the
INF file. Remove the DpcLib class from the #include directives -- it is
not listed in the INF file, and IScsiDxe doesn't call either DpcLib API
(QueueDpc(), DispatchDpc()). No functional changes.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiDxe.inf | 6 +++---
NetworkPkg/IScsiDxe/IScsiImpl.h | 17 ++++++++---------
2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiDxe.inf b/NetworkPkg/IScsiDxe/IScsiDxe.inf
index 0ffb340ce05e..543c4083026a 100644
--- a/NetworkPkg/IScsiDxe/IScsiDxe.inf
+++ b/NetworkPkg/IScsiDxe/IScsiDxe.inf
@@ -49,53 +49,53 @@ [Sources]
IScsiDriver.c
IScsiDriver.h
IScsiExtScsiPassThru.c
IScsiIbft.c
IScsiIbft.h
IScsiInitiatorName.c
IScsiImpl.h
IScsiMisc.c
IScsiMisc.h
IScsiProto.c
IScsiProto.h

[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
CryptoPkg/CryptoPkg.dec
NetworkPkg/NetworkPkg.dec

[LibraryClasses]
+ BaseCryptLib
BaseLib
BaseMemoryLib
DebugLib
DevicePathLib
HiiLib
MemoryAllocationLib
NetLib
- TcpIoLib
PrintLib
+ TcpIoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
+ UefiHiiServicesLib
UefiLib
UefiRuntimeServicesTableLib
- UefiHiiServicesLib
- BaseCryptLib

[Protocols]
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES ## SystemTable
gEfiDriverBindingProtocolGuid ## SOMETIMES_PRODUCES
gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiIp4Config2ProtocolGuid ## SOMETIMES_CONSUMES
gEfiIp6ConfigProtocolGuid ## SOMETIMES_CONSUMES
gEfiTcp4ProtocolGuid ## TO_START
gEfiTcp6ProtocolGuid ## TO_START
gEfiTcp4ServiceBindingProtocolGuid ## TO_START
gEfiTcp6ServiceBindingProtocolGuid ## TO_START
diff --git a/NetworkPkg/IScsiDxe/IScsiImpl.h b/NetworkPkg/IScsiDxe/IScsiImpl.h
index 387ab9765e9e..d895c7feb947 100644
--- a/NetworkPkg/IScsiDxe/IScsiImpl.h
+++ b/NetworkPkg/IScsiDxe/IScsiImpl.h
@@ -19,53 +19,52 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/DevicePath.h>
#include <Protocol/HiiConfigAccess.h>

#include <Protocol/Ip6.h>
#include <Protocol/Dhcp4.h>
#include <Protocol/Dhcp6.h>
#include <Protocol/Dns4.h>
#include <Protocol/Dns6.h>
#include <Protocol/Tcp4.h>
#include <Protocol/Tcp6.h>
#include <Protocol/Ip4Config2.h>
#include <Protocol/Ip6Config.h>

#include <Protocol/AuthenticationInfo.h>
#include <Protocol/IScsiInitiatorName.h>
#include <Protocol/ScsiPassThruExt.h>
#include <Protocol/AdapterInformation.h>
#include <Protocol/NetworkInterfaceIdentifier.h>

-#include <Library/HiiLib.h>
-#include <Library/UefiHiiServicesLib.h>
-#include <Library/DevicePathLib.h>
-#include <Library/DebugLib.h>
+#include <Library/BaseCryptLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/HiiLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/NetLib.h>
#include <Library/PrintLib.h>
+#include <Library/TcpIoLib.h>
#include <Library/UefiBootServicesTableLib.h>
-#include <Library/UefiRuntimeServicesTableLib.h>
+#include <Library/UefiHiiServicesLib.h>
#include <Library/UefiLib.h>
-#include <Library/DpcLib.h>
-#include <Library/NetLib.h>
-#include <Library/TcpIoLib.h>
-#include <Library/BaseCryptLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>

#include <Guid/MdeModuleHii.h>
#include <Guid/EventGroup.h>
#include <Guid/Acpi.h>

#include "IScsiConfigNVDataStruc.h"
#include "IScsiDriver.h"
#include "IScsiProto.h"
#include "IScsiCHAP.h"
#include "IScsiDhcp.h"
#include "IScsiDhcp6.h"

#include "IScsiIbft.h"
#include "IScsiMisc.h"
#include "IScsiDns.h"
#include "IScsiConfig.h"

#define ISCSI_AUTH_INITIAL 0

--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 05/10] NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex()

Laszlo Ersek
 

Considering IScsiBinToHex():

if (((*HexLength) - 3) < BinLength * 2) {
*HexLength = BinLength * 2 + 3;
}
the following subexpressions are problematic:

(*HexLength) - 3
BinLength * 2
BinLength * 2 + 3

The first one may wrap under zero, the latter two may wrap over
MAX_UINT32.

Rewrite the calculation using SafeIntLib.

While at it, change the type of the "Index" variable from UINTN to UINT32.
The largest "Index"-based value that we calculate is

Index * 2 + 2 (with (Index == BinLength))

Because the patch makes

BinLength * 2 + 3

safe to calculate in UINT32, using UINT32 for

Index * 2 + 2 (with (Index == BinLength))

is safe too. Consistently using UINT32 improves readability.

This patch is best reviewed with "git show -W".

The integer overflows that this patch fixes are theoretical; a subsequent
patch in the series will audit the IScsiBinToHex() call sites, and show
that none of them can fail.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiDxe.inf | 1 +
NetworkPkg/IScsiDxe/IScsiImpl.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.c | 19 +++++++++++++++----
4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiDxe.inf b/NetworkPkg/IScsiDxe/IScsiDxe.inf
index 543c4083026a..1dde56d00ca2 100644
--- a/NetworkPkg/IScsiDxe/IScsiDxe.inf
+++ b/NetworkPkg/IScsiDxe/IScsiDxe.inf
@@ -58,38 +58,39 @@ [Sources]
IScsiProto.c
IScsiProto.h

[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
CryptoPkg/CryptoPkg.dec
NetworkPkg/NetworkPkg.dec

[LibraryClasses]
BaseCryptLib
BaseLib
BaseMemoryLib
DebugLib
DevicePathLib
HiiLib
MemoryAllocationLib
NetLib
PrintLib
+ SafeIntLib
TcpIoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiHiiServicesLib
UefiLib
UefiRuntimeServicesTableLib

[Protocols]
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES ## SystemTable
gEfiDriverBindingProtocolGuid ## SOMETIMES_PRODUCES
gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/NetworkPkg/IScsiDxe/IScsiImpl.h b/NetworkPkg/IScsiDxe/IScsiImpl.h
index d895c7feb947..ac3a25730efb 100644
--- a/NetworkPkg/IScsiDxe/IScsiImpl.h
+++ b/NetworkPkg/IScsiDxe/IScsiImpl.h
@@ -28,38 +28,39 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/Tcp6.h>
#include <Protocol/Ip4Config2.h>
#include <Protocol/Ip6Config.h>

#include <Protocol/AuthenticationInfo.h>
#include <Protocol/IScsiInitiatorName.h>
#include <Protocol/ScsiPassThruExt.h>
#include <Protocol/AdapterInformation.h>
#include <Protocol/NetworkInterfaceIdentifier.h>

#include <Library/BaseCryptLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/HiiLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/NetLib.h>
#include <Library/PrintLib.h>
+#include <Library/SafeIntLib.h>
#include <Library/TcpIoLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiHiiServicesLib.h>
#include <Library/UefiLib.h>
#include <Library/UefiRuntimeServicesTableLib.h>

#include <Guid/MdeModuleHii.h>
#include <Guid/EventGroup.h>
#include <Guid/Acpi.h>

#include "IScsiConfigNVDataStruc.h"
#include "IScsiDriver.h"
#include "IScsiProto.h"
#include "IScsiCHAP.h"
#include "IScsiDhcp.h"
#include "IScsiDhcp6.h"

#include "IScsiIbft.h"
#include "IScsiMisc.h"
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 46c725aab3a4..231413993b08 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -134,38 +134,39 @@ IScsiMacAddrToStr (
**/
EFI_STATUS
IScsiAsciiStrToIp (
IN CHAR8 *Str,
IN UINT8 IpMode,
OUT EFI_IP_ADDRESS *Ip
);

/**
Convert the binary encoded buffer into a hexadecimal encoded string.

@param[in] BinBuffer The buffer containing the binary data.
@param[in] BinLength Length of the binary buffer.
@param[in, out] HexStr Pointer to the string.
@param[in, out] HexLength The length of the string.

@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
+ @retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.

**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);

/**
Convert the hexadecimal string into a binary encoded buffer.

@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.

@retval EFI_SUCCESS The hexadecimal string is converted into a binary
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index b8fef3ff6f5a..42988e15cb06 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -300,61 +300,72 @@ IScsiMacAddrToStr (
String = &Str[3 * Index - 1] ;
if (VlanId != 0) {
String += UnicodeSPrint (String, 6 * sizeof (CHAR16), L"\\%04x", (UINTN) VlanId);
}

*String = L'\0';
}

/**
Convert the binary encoded buffer into a hexadecimal encoded string.

@param[in] BinBuffer The buffer containing the binary data.
@param[in] BinLength Length of the binary buffer.
@param[in, out] HexStr Pointer to the string.
@param[in, out] HexLength The length of the string.

@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
+ @retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.

**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
)
{
- UINTN Index;
+ UINT32 HexLengthMin;
+ UINT32 HexLengthProvided;
+ UINT32 Index;

if ((HexStr == NULL) || (BinBuffer == NULL) || (BinLength == 0)) {
return EFI_INVALID_PARAMETER;
}

- if (((*HexLength) - 3) < BinLength * 2) {
- *HexLength = BinLength * 2 + 3;
+ //
+ // Safely calculate: HexLengthMin := BinLength * 2 + 3.
+ //
+ if (RETURN_ERROR (SafeUint32Mult (BinLength, 2, &HexLengthMin)) ||
+ RETURN_ERROR (SafeUint32Add (HexLengthMin, 3, &HexLengthMin))) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
+ HexLengthProvided = *HexLength;
+ *HexLength = HexLengthMin;
+ if (HexLengthProvided < HexLengthMin) {
return EFI_BUFFER_TOO_SMALL;
}

- *HexLength = BinLength * 2 + 3;
//
// Prefix for Hex String.
//
HexStr[0] = '0';
HexStr[1] = 'x';

for (Index = 0; Index < BinLength; Index++) {
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}

HexStr[Index * 2 + 2] = '\0';

return EFI_SUCCESS;
}


/**
Convert the hexadecimal string into a binary encoded buffer.
--
2.19.1.3.g30247aa5d201


[PUBLIC edk2 PATCH v2 03/10] NetworkPkg/IScsiDxe: clean up "ISCSI_CHAP_AUTH_DATA.OutChallengeLength"

Laszlo Ersek
 

The "ISCSI_CHAP_AUTH_DATA.OutChallenge" field is declared as a UINT8 array
with ISCSI_CHAP_AUTH_MAX_LEN (1024) elements. However, when the challenge
is generated and formatted, only ISCSI_CHAP_RSP_LEN (16) octets are used
in the array.

Change the array size to ISCSI_CHAP_RSP_LEN, and remove the (now unused)
ISCSI_CHAP_AUTH_MAX_LEN macro.

Remove the "ISCSI_CHAP_AUTH_DATA.OutChallengeLength" field, which is
superfluous too.

Most importantly, explain in a new comment *why* tying the challenge size
to the digest size (ISCSI_CHAP_RSP_LEN) has always made sense. (See also
Linux kernel commit 19f5f88ed779, "scsi: target: iscsi: tie the challenge
length to the hash digest size", 2019-11-06.) For sure, the motivation
that the new comment now explains has always been there, and has always
been the same, for IScsiDxe; it's just that now we spell it out too.

No change in peer-visible behavior.

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=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 9 ++++++---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 3 +--
2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 1fc1d96ea3f3..35d5d6ec29e3 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -3,39 +3,38 @@

Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent

**/

#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_

#define ISCSI_AUTH_METHOD_CHAP "CHAP"

#define ISCSI_KEY_CHAP_ALGORITHM "CHAP_A"
#define ISCSI_KEY_CHAP_IDENTIFIER "CHAP_I"
#define ISCSI_KEY_CHAP_CHALLENGE "CHAP_C"
#define ISCSI_KEY_CHAP_NAME "CHAP_N"
#define ISCSI_KEY_CHAP_RESPONSE "CHAP_R"

#define ISCSI_CHAP_ALGORITHM_MD5 5

-#define ISCSI_CHAP_AUTH_MAX_LEN 1024
///
/// MD5_HASHSIZE
///
#define ISCSI_CHAP_RSP_LEN 16

#define ISCSI_CHAP_STEP_ONE 1
#define ISCSI_CHAP_STEP_TWO 2
#define ISCSI_CHAP_STEP_THREE 3
#define ISCSI_CHAP_STEP_FOUR 4


#pragma pack(1)

typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {
UINT8 CHAPType;
CHAR8 CHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 CHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
CHAR8 ReverseCHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 ReverseCHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
@@ -43,41 +42,45 @@ typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {

#pragma pack()

///
/// ISCSI CHAP Authentication Data
///
typedef struct _ISCSI_CHAP_AUTH_DATA {
ISCSI_CHAP_AUTH_CONFIG_NVDATA *AuthConfig;
UINT32 InIdentifier;
UINT8 InChallenge[1024];
UINT32 InChallengeLength;
//
// Calculated CHAP Response (CHAP_R) value.
//
UINT8 CHAPResponse[ISCSI_CHAP_RSP_LEN];

//
// Auth-data to be sent out for mutual authentication.
//
+ // While the challenge size is technically independent of the hashing
+ // algorithm, it is good practice to avoid hashing *fewer bytes* than the
+ // digest size. In other words, it's good practice to feed *at least as many
+ // bytes* to the hashing algorithm as the hashing algorithm will output.
+ //
UINT32 OutIdentifier;
- UINT8 OutChallenge[ISCSI_CHAP_AUTH_MAX_LEN];
- UINT32 OutChallengeLength;
+ UINT8 OutChallenge[ISCSI_CHAP_RSP_LEN];
} ISCSI_CHAP_AUTH_DATA;

/**
This function checks the received iSCSI Login Response during the security
negotiation stage.

@param[in] Conn The iSCSI connection.

@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.
@retval Others Other errors as indicated.

**/
EFI_STATUS
IScsiCHAPOnRspReceived (
IN ISCSI_CONNECTION *Conn
);
/**
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index df3c2eb1200a..9e192ce292e8 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -106,39 +106,39 @@ IScsiCHAPCalculateResponse (
**/
EFI_STATUS
IScsiCHAPAuthTarget (
IN ISCSI_CHAP_AUTH_DATA *AuthData,
IN UINT8 *TargetResponse
)
{
EFI_STATUS Status;
UINT32 SecretSize;
UINT8 VerifyRsp[ISCSI_CHAP_RSP_LEN];

Status = EFI_SUCCESS;

SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
Status = IScsiCHAPCalculateResponse (
AuthData->OutIdentifier,
AuthData->AuthConfig->ReverseCHAPSecret,
SecretSize,
AuthData->OutChallenge,
- AuthData->OutChallengeLength,
+ ISCSI_CHAP_RSP_LEN, // ChallengeLength
VerifyRsp
);

if (CompareMem (VerifyRsp, TargetResponse, ISCSI_CHAP_RSP_LEN) != 0) {
Status = EFI_SECURITY_VIOLATION;
}

return Status;
}


/**
This function checks the received iSCSI Login Response during the security
negotiation stage.

@param[in] Conn The iSCSI connection.

@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@@ -474,39 +474,38 @@ IScsiCHAPToSendReq (
IScsiBinToHex (
(UINT8 *) AuthData->CHAPResponse,
ISCSI_CHAP_RSP_LEN,
Response,
&RspLen
);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);

if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
//
// CHAP_I=<I>
//
IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
//
// CHAP_C=<C>
//
IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
- AuthData->OutChallengeLength = ISCSI_CHAP_RSP_LEN;
IScsiBinToHex (
(UINT8 *) AuthData->OutChallenge,
ISCSI_CHAP_RSP_LEN,
Challenge,
&ChallengeLen
);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);

Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
}
//
// Set the stage transition flag.
//
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
break;

default:
Status = EFI_PROTOCOL_ERROR;
break;
--
2.19.1.3.g30247aa5d201

4641 - 4660 of 80786