* [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login
2021-06-08 13:06 [PATCH 0/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
@ 2021-06-08 13:06 ` Laszlo Ersek
2021-06-11 11:30 ` Maciej Rabeda
2021-06-18 9:45 ` Philippe Mathieu-Daudé
2021-06-08 13:06 ` [PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files Laszlo Ersek
` (4 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-08 13:06 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login
2021-06-08 13:06 ` [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login Laszlo Ersek
@ 2021-06-11 11:30 ` Maciej Rabeda
2021-06-18 9:45 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 22+ messages in thread
From: Maciej Rabeda @ 2021-06-11 11:30 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Jiaxin Wu, Philippe Mathieu-Daudé, Siyuan Fu
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
On 08-Jun-21 15:06, Laszlo Ersek wrote:
> 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;
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login
2021-06-08 13:06 ` [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login Laszlo Ersek
2021-06-11 11:30 ` Maciej Rabeda
@ 2021-06-18 9:45 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-18 9:45 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Jiaxin Wu, Maciej Rabeda, Siyuan Fu
On 6/8/21 3:06 PM, Laszlo Ersek wrote:
> 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(+)
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files
2021-06-08 13:06 [PATCH 0/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
2021-06-08 13:06 ` [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login Laszlo Ersek
@ 2021-06-08 13:06 ` Laszlo Ersek
2021-06-09 10:35 ` Philippe Mathieu-Daudé
2021-06-11 11:32 ` Maciej Rabeda
2021-06-08 13:06 ` [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes Laszlo Ersek
` (3 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-08 13:06 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files
2021-06-08 13:06 ` [PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files Laszlo Ersek
@ 2021-06-09 10:35 ` Philippe Mathieu-Daudé
2021-06-11 11:32 ` Maciej Rabeda
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-09 10:35 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Jiaxin Wu, Maciej Rabeda, Siyuan Fu
On 6/8/21 3:06 PM, Laszlo Ersek wrote:
> 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(-)
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files
2021-06-08 13:06 ` [PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files Laszlo Ersek
2021-06-09 10:35 ` Philippe Mathieu-Daudé
@ 2021-06-11 11:32 ` Maciej Rabeda
1 sibling, 0 replies; 22+ messages in thread
From: Maciej Rabeda @ 2021-06-11 11:32 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Jiaxin Wu, Philippe Mathieu-Daudé, Siyuan Fu
[-- Attachment #1: Type: text/plain, Size: 5384 bytes --]
Nicely aligned spacing...
Feels Good Man by Daniel 168
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
On 08-Jun-21 15:06, Laszlo Ersek wrote:
> 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;
>
[-- Attachment #2: Type: text/html, Size: 6117 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes
2021-06-08 13:06 [PATCH 0/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
2021-06-08 13:06 ` [PATCH 1/6] NetworkPkg/IScsiDxe: re-set session-level authentication state before login Laszlo Ersek
2021-06-08 13:06 ` [PATCH 2/6] NetworkPkg/IScsiDxe: add horizontal whitespace to IScsiCHAP files Laszlo Ersek
@ 2021-06-08 13:06 ` Laszlo Ersek
2021-06-09 10:43 ` Philippe Mathieu-Daudé
2021-06-08 13:06 ` [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP Laszlo Ersek
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-08 13:06 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes
2021-06-08 13:06 ` [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes Laszlo Ersek
@ 2021-06-09 10:43 ` Philippe Mathieu-Daudé
2021-06-09 13:46 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-09 10:43 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Jiaxin Wu, Maciej Rabeda, Siyuan Fu
Hi Laszlo,
On 6/8/21 3:06 PM, Laszlo Ersek wrote:
> 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>.
Matter of taste probably, I'd rather see this patch split in 2, as you
identified. (b) first then (a). Regardless:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> 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(-)
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes
2021-06-09 10:43 ` Philippe Mathieu-Daudé
@ 2021-06-09 13:46 ` Laszlo Ersek
2021-06-11 11:38 ` Maciej Rabeda
0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-09 13:46 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Siyuan Fu
On 06/09/21 12:43, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
>
> On 6/8/21 3:06 PM, Laszlo Ersek wrote:
>> 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>.
>
> Matter of taste probably, I'd rather see this patch split in 2, as you
> identified. (b) first then (a). Regardless:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
Interesting; I thought that showing all uses of ISCSI_CHAP_RSP_LEN in
one patch, and classifying each use one way or another at once, was the
best for reviewer understanding. Basically it's a single "mental loop"
over all uses, and in the "loop body", we have an "if" (classification)
-- allocation vs. processing.
What you propose is basically "two loops". In that approach, in the
first patch (= the first "mental loop"), only "processing" uses would be
updated; the "allocation sites" wouldn't be shown at all. I feel that
this approach is counter-intuitive:
>From the body of the first patch,
- the reviewer can check the *correctness* of the patch (that is,
whether everything that I converted is indeed "processing"),
- but they can't check the *completeness* of the patch (that is, whether
there is a "processing" site that I should have converted, but missed).
For the reviewer to verify the completeness of the first patch, they
have to apply it (or check out the branch at that stage), and go over
all the *remaining* ISCSI_CHAP_RSP_LEN instances, to see if I missed
something. And, if the reviewer has to check every single instance of
ISCSI_CHAP_RSP_LEN right after the first patch, they end up doing the
same work as if they had just reviewed this particular patch.
I think your approach boils down to the following idea:
The completeness of the first patch would be proved by the correctness
of the second patch.
That is, *after* you reviewed the second patch (and see that every site
converted is indeed an allocation site, and that the ISCSI_CHAP_RSP_LEN
macro definition is being removed, so no other ISCSI_CHAP_RSP_LEN
instance remains), you could be sure that no processing site was missed
in the first patch.
Technically / mathematically, this is entirely true; I just prefer
avoiding situations where you have to review patch (N+X) to see the
validity (completeness) of patch (N). I quite dislike jumping between
patches during review.
Does my explanation make sense?
If you still prefer the split, I'm OK to do it.
Thanks!
Laszlo
>
>> 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(-)
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes
2021-06-09 13:46 ` Laszlo Ersek
@ 2021-06-11 11:38 ` Maciej Rabeda
0 siblings, 0 replies; 22+ messages in thread
From: Maciej Rabeda @ 2021-06-11 11:38 UTC (permalink / raw)
To: Laszlo Ersek, Philippe Mathieu-Daudé, edk2-devel-groups-io
Cc: Jiaxin Wu, Siyuan Fu
To cut to the chase on this patch:
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
On 09-Jun-21 15:46, Laszlo Ersek wrote:
> On 06/09/21 12:43, Philippe Mathieu-Daudé wrote:
>> Hi Laszlo,
>>
>> On 6/8/21 3:06 PM, Laszlo Ersek wrote:
>>> 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>.
>> Matter of taste probably, I'd rather see this patch split in 2, as you
>> identified. (b) first then (a). Regardless:
>> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
> Interesting; I thought that showing all uses of ISCSI_CHAP_RSP_LEN in
> one patch, and classifying each use one way or another at once, was the
> best for reviewer understanding. Basically it's a single "mental loop"
> over all uses, and in the "loop body", we have an "if" (classification)
> -- allocation vs. processing.
>
> What you propose is basically "two loops". In that approach, in the
> first patch (= the first "mental loop"), only "processing" uses would be
> updated; the "allocation sites" wouldn't be shown at all. I feel that
> this approach is counter-intuitive:
>
> From the body of the first patch,
>
> - the reviewer can check the *correctness* of the patch (that is,
> whether everything that I converted is indeed "processing"),
>
> - but they can't check the *completeness* of the patch (that is, whether
> there is a "processing" site that I should have converted, but missed).
>
> For the reviewer to verify the completeness of the first patch, they
> have to apply it (or check out the branch at that stage), and go over
> all the *remaining* ISCSI_CHAP_RSP_LEN instances, to see if I missed
> something. And, if the reviewer has to check every single instance of
> ISCSI_CHAP_RSP_LEN right after the first patch, they end up doing the
> same work as if they had just reviewed this particular patch.
>
> I think your approach boils down to the following idea:
>
> The completeness of the first patch would be proved by the correctness
> of the second patch.
>
> That is, *after* you reviewed the second patch (and see that every site
> converted is indeed an allocation site, and that the ISCSI_CHAP_RSP_LEN
> macro definition is being removed, so no other ISCSI_CHAP_RSP_LEN
> instance remains), you could be sure that no processing site was missed
> in the first patch.
>
> Technically / mathematically, this is entirely true; I just prefer
> avoiding situations where you have to review patch (N+X) to see the
> validity (completeness) of patch (N). I quite dislike jumping between
> patches during review.
>
> Does my explanation make sense?
>
> If you still prefer the split, I'm OK to do it.
>
> Thanks!
> Laszlo
>
>>> 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(-)
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP
2021-06-08 13:06 [PATCH 0/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
` (2 preceding siblings ...)
2021-06-08 13:06 ` [PATCH 3/6] NetworkPkg/IScsiDxe: distinguish "maximum" and "selected" CHAP digest sizes Laszlo Ersek
@ 2021-06-08 13:06 ` Laszlo Ersek
2021-06-11 11:54 ` Maciej Rabeda
2021-06-08 13:06 ` [PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
2021-06-08 13:06 ` [PATCH 6/6] NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro Laszlo Ersek
5 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-08 13:06 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP
2021-06-08 13:06 ` [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP Laszlo Ersek
@ 2021-06-11 11:54 ` Maciej Rabeda
2021-06-22 15:57 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Maciej Rabeda @ 2021-06-11 11:54 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Jiaxin Wu, Philippe Mathieu-Daudé, Siyuan Fu
Laszlo,
Comments inline.
On 08-Jun-21 15:06, Laszlo Ersek wrote:
> 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
Any chance to define an enum type for Algorithm? IMHO it brings more
meaning to that number and limits the set of values it is expected to have.
> + 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;
> }
Either break like regular function call or leave it in a single line for
readability, since UEFI coding standard section 5.1.1 allows for lines
up to 120.
I opt for the latter, since param break will look ugly, but if it looks
better in your editor than a line over 80 chars in size - go for it.
>
> 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);
Coding standard. Parameter break.
> 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);
Coding standard. Parameter break.
> + //
> + // 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;
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP
2021-06-11 11:54 ` Maciej Rabeda
@ 2021-06-22 15:57 ` Laszlo Ersek
2021-06-25 14:56 ` [edk2-devel] " Maciej Rabeda
0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-22 15:57 UTC (permalink / raw)
To: Rabeda, Maciej, edk2-devel-groups-io
Cc: Jiaxin Wu, Philippe Mathieu-Daudé, Siyuan Fu
On 06/11/21 13:54, Rabeda, Maciej wrote:
> Laszlo,
>
> Comments inline.
>
> On 08-Jun-21 15:06, Laszlo Ersek wrote:
>> 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
> Any chance to define an enum type for Algorithm? IMHO it brings more
> meaning to that number and limits the set of values it is expected to have.
I picked UINT8 intentionally; I wanted to stay close to the definition at
https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9
which says, "A one octet field". (Hence the reference to "CHAP_A" in the
comment as well.)
If we want an enum here, then I need to prepend a patch, for first
replacing the existent ISCSI_CHAP_ALGORITHM_MD5 macro with an enum type
/ constant.
I still like UINT8 for this, but if you strongly prefer the enum, I can
certainly do it. Please advise :)
>> + 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;
>> }
> Either break like regular function call or leave it in a single line for
> readability, since UEFI coding standard section 5.1.1 allows for lines
> up to 120.
> I opt for the latter, since param break will look ugly, but if it looks
> better in your editor than a line over 80 chars in size - go for it.
My mistake, sorry -- I forgot that the "condensed style" that we
actually prefer in ArmVirtPkg and OvmfPkg is not accepted in other packages.
So, I will insert the line breaks rigorously -- as explained elsewhere,
my strict 80 chars/line limit is not due to my editor, but to my desire
to fit two source code windows side-by-side, combined with my
not-so-great eyesight, and inability to use multiple monitors at the
same time (or one huge monitor).
I'll fix up the other instances of this same style issue as well.
Thanks!
Laszlo
>> 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);
> Coding standard. Parameter break.
>> 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);
> Coding standard. Parameter break.
>> + //
>> + // 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;
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [edk2-devel] [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP
2021-06-22 15:57 ` Laszlo Ersek
@ 2021-06-25 14:56 ` Maciej Rabeda
2021-06-28 14:44 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Maciej Rabeda @ 2021-06-25 14:56 UTC (permalink / raw)
To: devel, lersek; +Cc: Jiaxin Wu, Philippe Mathieu-Daudé, Siyuan Fu
More comments inline.
On 22-Jun-21 17:57, Laszlo Ersek wrote:
> On 06/11/21 13:54, Rabeda, Maciej wrote:
>> Laszlo,
>>
>> Comments inline.
>>
>> On 08-Jun-21 15:06, Laszlo Ersek wrote:
>>> 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
>> Any chance to define an enum type for Algorithm? IMHO it brings more
>> meaning to that number and limits the set of values it is expected to have.
> I picked UINT8 intentionally; I wanted to stay close to the definition at
>
> https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9
>
> which says, "A one octet field". (Hence the reference to "CHAP_A" in the
> comment as well.)
>
> If we want an enum here, then I need to prepend a patch, for first
> replacing the existent ISCSI_CHAP_ALGORITHM_MD5 macro with an enum type
> / constant.
>
> I still like UINT8 for this, but if you strongly prefer the enum, I can
> certainly do it. Please advise :)
Okay, let's stick to UINT8 based on IANA definition.
>
>
>>> + 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;
>>> }
>> Either break like regular function call or leave it in a single line for
>> readability, since UEFI coding standard section 5.1.1 allows for lines
>> up to 120.
>> I opt for the latter, since param break will look ugly, but if it looks
>> better in your editor than a line over 80 chars in size - go for it.
> My mistake, sorry -- I forgot that the "condensed style" that we
> actually prefer in ArmVirtPkg and OvmfPkg is not accepted in other packages.
Making it the other way around. Apart from historical reasons (EDK2
coding style evolving) - why is a different style accepted in other
packages? We have a EDK2 coding style document for a reason.
If it is not perfect, it does not suit our needs or simply hurts our
eyes (which 'param per line' in if statements does to me), we can try to
change it to the "condensed style".
> So, I will insert the line breaks rigorously -- as explained elsewhere,
> my strict 80 chars/line limit is not due to my editor, but to my desire
> to fit two source code windows side-by-side, combined with my
> not-so-great eyesight, and inability to use multiple monitors at the
> same time (or one huge monitor).
>
> I'll fix up the other instances of this same style issue as well.
>
> Thanks!
> Laszlo
>
>
>>> 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);
>> Coding standard. Parameter break.
>>> 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);
>> Coding standard. Parameter break.
>>> + //
>>> + // 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;
>
>
>
>
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [edk2-devel] [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP
2021-06-25 14:56 ` [edk2-devel] " Maciej Rabeda
@ 2021-06-28 14:44 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-28 14:44 UTC (permalink / raw)
To: Rabeda, Maciej, devel; +Cc: Jiaxin Wu, Philippe Mathieu-Daudé, Siyuan Fu
Hi Maciej,
[snipping liberally, comments below]
On 06/25/21 16:56, Rabeda, Maciej wrote:
> On 22-Jun-21 17:57, Laszlo Ersek wrote:
>> On 06/11/21 13:54, Rabeda, Maciej wrote:
>>> On 08-Jun-21 15:06, Laszlo Ersek wrote:
>>>> +typedef struct {
>>>> + UINT8 Algorithm; //
>>>> ISCSI_CHAP_ALGORITHM_*, CHAP_A
>>> Any chance to define an enum type for Algorithm? IMHO it brings more
>>> meaning to that number and limits the set of values it is expected
>>> to have.
>> I picked UINT8 intentionally; I wanted to stay close to the
>> definition at
>>
>> https://www.iana.org/assignments/ppp-numbers/ppp-numbers.xhtml#ppp-numbers-9
>>
>>
>> which says, "A one octet field". (Hence the reference to "CHAP_A" in
>> the comment as well.)
>>
>> If we want an enum here, then I need to prepend a patch, for first
>> replacing the existent ISCSI_CHAP_ALGORITHM_MD5 macro with an enum
>> type / constant.
>>
>> I still like UINT8 for this, but if you strongly prefer the enum, I
>> can certainly do it. Please advise :)
> Okay, let's stick to UINT8 based on IANA definition.
Thanks!
>>>> - if (CompareMem (VerifyRsp, TargetResponse, MD5_DIGEST_SIZE) !=
>>>> 0) {
>>>> + if (CompareMem (VerifyRsp, TargetResponse,
>>>> + AuthData->Hash->DigestSize) != 0) {
>>>> Status = EFI_SECURITY_VIOLATION;
>>>> }
>>> Either break like regular function call or leave it in a single line
>>> for readability, since UEFI coding standard section 5.1.1 allows for
>>> lines up to 120. I opt for the latter, since param break will look
>>> ugly, but if it looks better in your editor than a line over 80
>>> chars in size - go for it.
>> My mistake, sorry -- I forgot that the "condensed style" that we
>> actually prefer in ArmVirtPkg and OvmfPkg is not accepted in other
>> packages.
> Making it the other way around. Apart from historical reasons (EDK2
> coding style evolving) - why is a different style accepted in other
> packages? We have a EDK2 coding style document for a reason.
> If it is not perfect, it does not suit our needs or simply hurts our
> eyes (which 'param per line' in if statements does to me), we can try
> to change it to the "condensed style".
* Submitted on 11 Aug 2017:
[edk2] [edk2-CCodingStandardsSpecification PATCH 0/2] improvements related to line wrapping
[edk2] [edk2-CCodingStandardsSpecification PATCH 1/2] Source Files / General Rules: limit line lengths to 80 columns
[edk2] [edk2-CCodingStandardsSpecification PATCH 2/2] Source Files / Spacing / Multi-line func. calls: allow condensed arguments
http://mid.mail-archive.com/20170811164851.9466-1-lersek@redhat.com
http://mid.mail-archive.com/20170811164851.9466-2-lersek@redhat.com
http://mid.mail-archive.com/20170811164851.9466-3-lersek@redhat.com
The condensed arguments style is described in patch 2/2.
* Filed in September 2017, from the discussion under patch 2/2:
https://bugzilla.tianocore.org/show_bug.cgi?id=714
In CONFIRMED status currently.
(The link to the original email discussion (in comment#0 of the BZ) no
longer works, because Intel has killed off the old <lists.01.org>
archive links meanwhile. But the <mail-archive.com> links should still
work.)
Thanks
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP
2021-06-08 13:06 [PATCH 0/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
` (3 preceding siblings ...)
2021-06-08 13:06 ` [PATCH 4/6] NetworkPkg/IScsiDxe: support multiple hash algorithms for CHAP Laszlo Ersek
@ 2021-06-08 13:06 ` Laszlo Ersek
2021-06-09 10:37 ` Philippe Mathieu-Daudé
2021-06-11 11:54 ` Maciej Rabeda
2021-06-08 13:06 ` [PATCH 6/6] NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro Laszlo Ersek
5 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-08 13:06 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP
2021-06-08 13:06 ` [PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
@ 2021-06-09 10:37 ` Philippe Mathieu-Daudé
2021-06-11 11:54 ` Maciej Rabeda
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-09 10:37 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Jiaxin Wu, Maciej Rabeda, Siyuan Fu
On 6/8/21 3:06 PM, Laszlo Ersek wrote:
> 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(-)
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP
2021-06-08 13:06 ` [PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
2021-06-09 10:37 ` Philippe Mathieu-Daudé
@ 2021-06-11 11:54 ` Maciej Rabeda
1 sibling, 0 replies; 22+ messages in thread
From: Maciej Rabeda @ 2021-06-11 11:54 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Jiaxin Wu, Philippe Mathieu-Daudé, Siyuan Fu
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
On 08-Jun-21 15:06, Laszlo Ersek wrote:
> 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 6/6] NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro
2021-06-08 13:06 [PATCH 0/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
` (4 preceding siblings ...)
2021-06-08 13:06 ` [PATCH 5/6] NetworkPkg/IScsiDxe: support SHA256 in CHAP Laszlo Ersek
@ 2021-06-08 13:06 ` Laszlo Ersek
2021-06-11 11:55 ` Maciej Rabeda
2021-06-17 15:51 ` Philippe Mathieu-Daudé
5 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2021-06-08 13:06 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 6/6] NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro
2021-06-08 13:06 ` [PATCH 6/6] NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro Laszlo Ersek
@ 2021-06-11 11:55 ` Maciej Rabeda
2021-06-17 15:51 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 22+ messages in thread
From: Maciej Rabeda @ 2021-06-11 11:55 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io
Cc: Jiaxin Wu, Philippe Mathieu-Daudé, Siyuan Fu
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
On 08-Jun-21 15:06, Laszlo Ersek wrote:
> Introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro for NetworkPkg.
> When explicitly set to FALSE, remove MD5 from IScsiDxe's CHAP algorithm
> list.
>
> Set NETWORK_ISCSI_MD5_ENABLE to TRUE by default, for compatibility
> reasons. Not just to minimize the disruption for platforms that currently
> include IScsiDxe, but also because RFC 7143 mandates MD5 for CHAP, and
> some vendors' iSCSI targets support MD5 only.
>
> With MD5 enabled, IScsiDxe will suggest SHA256, and then fall back to MD5
> if the target requests it. With MD5 disabled, IScsiDxe will suggest
> SHA256, and break off the connection (and session) if the target doesn't
> support SHA256.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> NetworkPkg/NetworkBuildOptions.dsc.inc | 2 +-
> NetworkPkg/NetworkDefines.dsc.inc | 20 ++++++++++++++++++++
> NetworkPkg/IScsiDxe/IScsiCHAP.c | 2 ++
> 3 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/NetworkPkg/NetworkBuildOptions.dsc.inc b/NetworkPkg/NetworkBuildOptions.dsc.inc
> index 42d980d9543d..738da2222f7e 100644
> --- a/NetworkPkg/NetworkBuildOptions.dsc.inc
> +++ b/NetworkPkg/NetworkBuildOptions.dsc.inc
> @@ -1,22 +1,22 @@
> ## @file
> # Network DSC include file for [BuildOptions] sections of all Architectures.
> #
> # This file can be included in the [BuildOptions*] section(s) of a platform DSC file
> # by using "!include NetworkPkg/NetworkBuildOptions.dsc.inc", to specify the C language
> # feature test macros (eg., API deprecation macros) according to the flags described
> # in "NetworkDefines.dsc.inc".
> #
> # Supported tool chain families: "GCC", "INTEL", "MSFT", "RVCT".
> #
> # Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent
> #
> ##
>
> -!if $(NETWORK_ISCSI_ENABLE) == TRUE
> +!if $(NETWORK_ISCSI_ENABLE) == TRUE && $(NETWORK_ISCSI_MD5_ENABLE) == TRUE
> MSFT:*_*_*_CC_FLAGS = /D ENABLE_MD5_DEPRECATED_INTERFACES
> INTEL:*_*_*_CC_FLAGS = /D ENABLE_MD5_DEPRECATED_INTERFACES
> GCC:*_*_*_CC_FLAGS = -D ENABLE_MD5_DEPRECATED_INTERFACES
> RVCT:*_*_*_CC_FLAGS = -DENABLE_MD5_DEPRECATED_INTERFACES
> !endif
> diff --git a/NetworkPkg/NetworkDefines.dsc.inc b/NetworkPkg/NetworkDefines.dsc.inc
> index 54deb6342aaa..e39a9cb3dc09 100644
> --- a/NetworkPkg/NetworkDefines.dsc.inc
> +++ b/NetworkPkg/NetworkDefines.dsc.inc
> @@ -3,38 +3,39 @@
> #
> # This file can be included to the [Defines] section of a platform DSC file by
> # using "!include NetworkPkg/NetworkDefines.dsc.inc" to set default value of
> # flags if they are not defined somewhere else, and also check the value to see
> # if there is any conflict.
> #
> # These flags can be defined before the !include line, or changed on the command
> # line to enable or disable related feature support.
> # -D FLAG=VALUE
> # The default value of these flags are:
> # DEFINE NETWORK_ENABLE = TRUE
> # DEFINE NETWORK_SNP_ENABLE = TRUE
> # DEFINE NETWORK_IP4_ENABLE = TRUE
> # DEFINE NETWORK_IP6_ENABLE = TRUE
> # DEFINE NETWORK_TLS_ENABLE = TRUE
> # DEFINE NETWORK_HTTP_ENABLE = FALSE
> # DEFINE NETWORK_HTTP_BOOT_ENABLE = TRUE
> # DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
> # DEFINE NETWORK_ISCSI_ENABLE = FALSE
> +# DEFINE NETWORK_ISCSI_MD5_ENABLE = TRUE
> # DEFINE NETWORK_VLAN_ENABLE = TRUE
> #
> # Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> # (C) Copyright 2020 Hewlett Packard Enterprise Development LP<BR>
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent
> #
> ##
>
> !ifndef NETWORK_ENABLE
> #
> # This flag is to enable or disable the whole network stack.
> #
> DEFINE NETWORK_ENABLE = TRUE
> !endif
>
> !ifndef NETWORK_SNP_ENABLE
> #
> # This flag is to include the common SNP driver or not.
> @@ -101,33 +102,52 @@
> # Both the "https://" and "http://" URI schemes are permitted. Otherwise, HTTP
> # connections are denied. Only the "https://" URI scheme is permitted.
> #
> DEFINE NETWORK_ALLOW_HTTP_CONNECTIONS = FALSE
> !endif
>
> !ifndef NETWORK_ISCSI_ENABLE
> #
> # This flag is to enable or disable iSCSI feature.
> #
> # Note: This feature depends on the OpenSSL building. To enable this feature, please
> # follow the instructions found in the file "OpenSSL-HOWTO.txt" located in
> # CryptoPkg\Library\OpensslLib to enable the OpenSSL building first.
> # Both OpensslLib.inf and OpensslLibCrypto.inf library instance can be used
> # since libssl is not required for iSCSI.
> #
> DEFINE NETWORK_ISCSI_ENABLE = FALSE
> !endif
>
> +!ifndef NETWORK_ISCSI_MD5_ENABLE
> + #
> + # This flag enables the deprecated MD5 hash algorithm in iSCSI CHAP
> + # authentication.
> + #
> + # Note: The NETWORK_ISCSI_MD5_ENABLE flag only makes a difference if
> + # NETWORK_ISCSI_ENABLE is TRUE; otherwise, NETWORK_ISCSI_MD5_ENABLE is
> + # ignored.
> + #
> + # With NETWORK_ISCSI_MD5_ENABLE set to TRUE, MD5 is enabled as the
> + # least preferred CHAP hash algorithm. With NETWORK_ISCSI_MD5_ENABLE
> + # set to FALSE, MD5 is disabled statically, at build time.
> + #
> + # The default value is TRUE, because RFC 7143 mandates MD5, and because
> + # several vendors' iSCSI targets only support MD5, for CHAP.
> + #
> + DEFINE NETWORK_ISCSI_MD5_ENABLE = TRUE
> +!endif
> +
> !if $(NETWORK_ENABLE) == TRUE
> #
> # Check the flags to see if there is any conflict.
> #
> !if ($(NETWORK_IP4_ENABLE) == FALSE) AND ($(NETWORK_IP6_ENABLE) == FALSE)
> !error "Must enable at least IP4 or IP6 stack if NETWORK_ENABLE is set to TRUE!"
> !endif
>
> !if ($(NETWORK_HTTP_BOOT_ENABLE) == TRUE) OR ($(NETWORK_HTTP_ENABLE) == TRUE)
> !if ($(NETWORK_TLS_ENABLE) == FALSE) AND ($(NETWORK_ALLOW_HTTP_CONNECTIONS) == FALSE)
> !error "Must enable TLS to support HTTPS, or allow unsecured HTTP connection, if NETWORK_HTTP_BOOT_ENABLE or NETWORK_HTTP_ENABLE is set to TRUE!"
> !endif
> !endif
> !endif
> diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
> index 2ce53c1ea4af..57163e9eb97f 100644
> --- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
> +++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
> @@ -7,50 +7,52 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> **/
>
> #include "IScsiImpl.h"
>
> //
> // Supported CHAP hash algorithms, mapped to sets of BaseCryptLib APIs and
> // macros. CHAP_HASH structures at lower subscripts in the array are preferred
> // by the initiator.
> //
> STATIC CONST CHAP_HASH mChapHash[] = {
> {
> ISCSI_CHAP_ALGORITHM_SHA256,
> SHA256_DIGEST_SIZE,
> Sha256GetContextSize,
> Sha256Init,
> Sha256Update,
> Sha256Final
> },
> +#ifdef ENABLE_MD5_DEPRECATED_INTERFACES
> //
> // Keep the deprecated MD5 entry at the end of the array (making MD5 the
> // least preferred choice of the initiator).
> //
> {
> ISCSI_CHAP_ALGORITHM_MD5,
> MD5_DIGEST_SIZE,
> Md5GetContextSize,
> Md5Init,
> Md5Update,
> Md5Final
> },
> +#endif // ENABLE_MD5_DEPRECATED_INTERFACES
> };
>
> //
> // Ordered list of mChapHash[*].Algorithm values. It is formatted for the
> // CHAP_A=<A1,A2...> value string, by the IScsiCHAPInitHashList() function. It
> // is sent by the initiator in ISCSI_CHAP_STEP_ONE.
> //
> STATIC CHAR8 mChapHashListString[
> 3 + // UINT8 identifier in
> // decimal
> (1 + 3) * (ARRAY_SIZE (mChapHash) - 1) + // comma prepended for
> // entries after the
> // first
> 1 + // extra character for
> // AsciiSPrint()
> // truncation check
> 1 // terminating NUL
> ];
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 6/6] NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro
2021-06-08 13:06 ` [PATCH 6/6] NetworkPkg: introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro Laszlo Ersek
2021-06-11 11:55 ` Maciej Rabeda
@ 2021-06-17 15:51 ` Philippe Mathieu-Daudé
1 sibling, 0 replies; 22+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-17 15:51 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-groups-io; +Cc: Jiaxin Wu, Maciej Rabeda, Siyuan Fu
On 6/8/21 3:06 PM, Laszlo Ersek wrote:
> Introduce the NETWORK_ISCSI_MD5_ENABLE feature test macro for NetworkPkg.
> When explicitly set to FALSE, remove MD5 from IScsiDxe's CHAP algorithm
> list.
>
> Set NETWORK_ISCSI_MD5_ENABLE to TRUE by default, for compatibility
> reasons. Not just to minimize the disruption for platforms that currently
> include IScsiDxe, but also because RFC 7143 mandates MD5 for CHAP, and
> some vendors' iSCSI targets support MD5 only.
>
> With MD5 enabled, IScsiDxe will suggest SHA256, and then fall back to MD5
> if the target requests it. With MD5 disabled, IScsiDxe will suggest
> SHA256, and break off the connection (and session) if the target doesn't
> support SHA256.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3355
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> NetworkPkg/NetworkBuildOptions.dsc.inc | 2 +-
> NetworkPkg/NetworkDefines.dsc.inc | 20 ++++++++++++++++++++
> NetworkPkg/IScsiDxe/IScsiCHAP.c | 2 ++
> 3 files changed, 23 insertions(+), 1 deletion(-)
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
^ permalink raw reply [flat|nested] 22+ messages in thread