* [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs
@ 2021-06-08 12:12 Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters Laszlo Ersek
` (10 more replies)
0 siblings, 11 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Repo: https://pagure.io/lersek/edk2.git
Branch: iscsi_overflow_bz3356
The main goal of this series is to fix a remotely exploitable buffer
overflow in the IScsiHexToBin() function.
This posting corresponds to:
https://bugzilla.tianocore.org/show_bug.cgi?id=3356#c22
meaning that it corresponds to the v2 patches attached to, and tested
in,
https://bugzilla.tianocore.org/show_bug.cgi?id=3356#c17
and that it carries Phil's and Maciej's R-b's that were given up to
comment#22.
Today is the Public Date for this embargoed security issue; I intend to
merge the patches tomorrow, based on Maciej's (already given) R-b.
(Simultaneously with this posting, I'm opening up the BZ publicly.) No
further review is required; the one day delay on the list is just to
give the community a (brief) opportunity to speak up, before the patches
are merged.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Thanks,
Laszlo
Laszlo Ersek (10):
NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
NetworkPkg/IScsiDxe: simplify "ISCSI_CHAP_AUTH_DATA.InChallenge" size
NetworkPkg/IScsiDxe: clean up
"ISCSI_CHAP_AUTH_DATA.OutChallengeLength"
NetworkPkg/IScsiDxe: clean up library class dependencies
NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex()
NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds
NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block
NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing
NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow
NetworkPkg/IScsiDxe: check IScsiHexToBin() return values
NetworkPkg/IScsiDxe/IScsiCHAP.c | 108 +++++++++++++++-----
NetworkPkg/IScsiDxe/IScsiCHAP.h | 14 ++-
NetworkPkg/IScsiDxe/IScsiDxe.inf | 7 +-
NetworkPkg/IScsiDxe/IScsiImpl.h | 18 ++--
NetworkPkg/IScsiDxe/IScsiMisc.c | 65 +++++++++---
NetworkPkg/IScsiDxe/IScsiMisc.h | 19 ++--
6 files changed, 166 insertions(+), 65 deletions(-)
--
2.19.1.3.g30247aa5d201
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-11 11:37 ` [edk2-devel] " Ni, Ray
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 02/10] NetworkPkg/IScsiDxe: simplify "ISCSI_CHAP_AUTH_DATA.InChallenge" size Laszlo Ersek
` (9 subsequent siblings)
10 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
Working with overlong lines is difficult for me; rewrap the CHAP-related
source files in IScsiDxe to 80 characters width. No functional changes.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 3 +-
NetworkPkg/IScsiDxe/IScsiCHAP.c | 90 +++++++++++++++-----
2 files changed, 71 insertions(+), 22 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 140bba0dcd76..5e59fb678bd7 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -72,31 +72,32 @@ typedef struct _ISCSI_CHAP_AUTH_DATA {
@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
);
/**
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.
+ 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
);
#endif
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index 355c6f129f68..cbbc56ae5b43 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -1,42 +1,45 @@
/** @file
- This file is for Challenge-Handshake Authentication Protocol (CHAP) Configuration.
+ 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"
/**
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[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_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
)
{
UINTN Md5ContextSize;
VOID *Md5Ctx;
CHAR8 IdByte[1];
EFI_STATUS Status;
@@ -78,40 +81,42 @@ IScsiCHAPCalculateResponse (
goto Exit;
}
if (Md5Final (Md5Ctx, ChapResponse)) {
Status = EFI_SUCCESS;
}
Exit:
FreePool (Md5Ctx);
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 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];
Status = EFI_SUCCESS;
SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
Status = IScsiCHAPCalculateResponse (
AuthData->OutIdentifier,
AuthData->AuthConfig->ReverseCHAPSecret,
@@ -177,187 +182,211 @@ IScsiCHAPOnRspReceived (
//
NetbufQueCopy (&Conn->RspQue, 0, Len, Data);
//
// Build the key-value list from the data segment of the Login Response.
//
KeyValueList = IScsiBuildKeyValueList ((CHAR8 *) Data, Len);
if (KeyValueList == NULL) {
Status = EFI_OUT_OF_RESOURCES;
goto ON_EXIT;
}
Status = EFI_PROTOCOL_ERROR;
switch (Conn->AuthStep) {
case ISCSI_AUTH_INITIAL:
//
// The first Login Response.
//
- Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_TARGET_PORTAL_GROUP_TAG);
+ Value = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_TARGET_PORTAL_GROUP_TAG
+ );
if (Value == NULL) {
goto ON_EXIT;
}
Result = IScsiNetNtoi (Value);
if (Result > 0xFFFF) {
goto ON_EXIT;
}
Session->TargetPortalGroupTag = (UINT16) Result;
- Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_AUTH_METHOD);
+ Value = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_AUTH_METHOD
+ );
if (Value == NULL) {
goto ON_EXIT;
}
//
- // Initiator mandates CHAP authentication but target replies without "CHAP", or
- // initiator suggets "None" but target replies with some kind of auth method.
+ // Initiator mandates CHAP authentication but target replies without
+ // "CHAP", or initiator suggets "None" but target replies with some kind of
+ // auth method.
//
if (Session->AuthType == ISCSI_AUTH_TYPE_NONE) {
if (AsciiStrCmp (Value, ISCSI_KEY_VALUE_NONE) != 0) {
goto ON_EXIT;
}
} else if (Session->AuthType == ISCSI_AUTH_TYPE_CHAP) {
if (AsciiStrCmp (Value, ISCSI_AUTH_METHOD_CHAP) != 0) {
goto ON_EXIT;
}
} else {
goto ON_EXIT;
}
//
// 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);
+ Value = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_CHAP_ALGORITHM
+ );
if (Value == NULL) {
goto ON_EXIT;
}
Algorithm = IScsiNetNtoi (Value);
if (Algorithm != ISCSI_CHAP_ALGORITHM_MD5) {
//
// Unsupported algorithm is chosen by target.
//
goto ON_EXIT;
}
- Identifier = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_IDENTIFIER);
+ Identifier = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_CHAP_IDENTIFIER
+ );
if (Identifier == NULL) {
goto ON_EXIT;
}
- Challenge = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_CHALLENGE);
+ Challenge = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_CHAP_CHALLENGE
+ );
if (Challenge == NULL) {
goto ON_EXIT;
}
//
// Process the CHAP identifier and CHAP Challenge from Target.
// Calculate Response value.
//
Result = IScsiNetNtoi (Identifier);
if (Result > 0xFF) {
goto ON_EXIT;
}
AuthData->InIdentifier = (UINT32) Result;
AuthData->InChallengeLength = ISCSI_CHAP_AUTH_MAX_LEN;
- IScsiHexToBin ((UINT8 *) AuthData->InChallenge, &AuthData->InChallengeLength, Challenge);
+ IScsiHexToBin (
+ (UINT8 *) AuthData->InChallenge,
+ &AuthData->InChallengeLength,
+ Challenge
+ );
Status = IScsiCHAPCalculateResponse (
AuthData->InIdentifier,
AuthData->AuthConfig->CHAPSecret,
(UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
AuthData->InChallenge,
AuthData->InChallengeLength,
AuthData->CHAPResponse
);
//
// Transit to next step.
//
Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
break;
case ISCSI_CHAP_STEP_THREE:
//
// One way CHAP authentication and the target would like to
// authenticate us.
//
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);
+ Response = IScsiGetValueByKeyFromList (
+ KeyValueList,
+ ISCSI_KEY_CHAP_RESPONSE
+ );
if (Response == NULL) {
goto ON_EXIT;
}
RspLen = ISCSI_CHAP_RSP_LEN;
IScsiHexToBin (TargetRsp, &RspLen, Response);
//
// Check the CHAP Name and Response replied by Target.
//
Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
break;
default:
break;
}
ON_EXIT:
if (KeyValueList != NULL) {
IScsiFreeKeyValueList (KeyValueList);
}
FreePool (Data);
return Status;
}
/**
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.
+ 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
)
{
EFI_STATUS Status;
ISCSI_SESSION *Session;
ISCSI_LOGIN_REQUEST *LoginReq;
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
CHAR8 ValueStr[256];
CHAR8 *Response;
UINT32 RspLen;
CHAR8 *Challenge;
@@ -376,95 +405,114 @@ IScsiCHAPToSendReq (
RspLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
Response = AllocateZeroPool (RspLen);
if (Response == NULL) {
return EFI_OUT_OF_RESOURCES;
}
ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
Challenge = AllocateZeroPool (ChallengeLen);
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_INITIATOR_NAME,
+ mPrivate->InitiatorName
+ );
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_SESSION_TYPE, "Normal");
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_TARGET_NAME,
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.
+ // 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);
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);
+ IScsiAddKeyValuePair (
+ Pdu,
+ ISCSI_KEY_CHAP_NAME,
+ (CHAR8 *) &AuthData->AuthConfig->CHAPName
+ );
//
// CHAP_R=<R>
//
- IScsiBinToHex ((UINT8 *) AuthData->CHAPResponse, ISCSI_CHAP_RSP_LEN, Response, &RspLen);
+ IScsiBinToHex (
+ (UINT8 *) AuthData->CHAPResponse,
+ ISCSI_CHAP_RSP_LEN,
+ Response,
+ &RspLen
+ );
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);
if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
//
// CHAP_I=<I>
//
IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
//
// CHAP_C=<C>
//
IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
AuthData->OutChallengeLength = ISCSI_CHAP_RSP_LEN;
- IScsiBinToHex ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN, Challenge, &ChallengeLen);
+ IScsiBinToHex (
+ (UINT8 *) AuthData->OutChallenge,
+ ISCSI_CHAP_RSP_LEN,
+ Challenge,
+ &ChallengeLen
+ );
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);
Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
}
//
// Set the stage transition flag.
//
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
break;
default:
Status = EFI_PROTOCOL_ERROR;
break;
}
FreePool (Response);
FreePool (Challenge);
return Status;
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 02/10] NetworkPkg/IScsiDxe: simplify "ISCSI_CHAP_AUTH_DATA.InChallenge" size
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 03/10] NetworkPkg/IScsiDxe: clean up "ISCSI_CHAP_AUTH_DATA.OutChallengeLength" Laszlo Ersek
` (8 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
The ISCSI_CHAP_AUTH_MAX_LEN macro is defined with value 1024.
The usage of this macro currently involves a semantic (not functional)
bug, which we're going to fix in a subsequent patch, eliminating
ISCSI_CHAP_AUTH_MAX_LEN altogether.
For now, remove the macro's usage from all
"ISCSI_CHAP_AUTH_DATA.InChallenge" contexts. This is doable without
duplicating open-coded constants.
No changes in functionality.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 2 +-
NetworkPkg/IScsiDxe/IScsiCHAP.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 5e59fb678bd7..1fc1d96ea3f3 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -33,39 +33,39 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#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[ISCSI_CHAP_AUTH_MAX_LEN];
+ UINT8 InChallenge[1024];
UINT32 InChallengeLength;
//
// Calculated CHAP Response (CHAP_R) value.
//
UINT8 CHAPResponse[ISCSI_CHAP_RSP_LEN];
//
// Auth-data to be sent out for mutual authentication.
//
UINT32 OutIdentifier;
UINT8 OutChallenge[ISCSI_CHAP_AUTH_MAX_LEN];
UINT32 OutChallengeLength;
} ISCSI_CHAP_AUTH_DATA;
/**
This function checks the received iSCSI Login Response during the security
negotiation stage.
@param[in] Conn The iSCSI connection.
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index cbbc56ae5b43..df3c2eb1200a 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -273,39 +273,39 @@ IScsiCHAPOnRspReceived (
}
Challenge = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_CHALLENGE
);
if (Challenge == NULL) {
goto ON_EXIT;
}
//
// Process the CHAP identifier and CHAP Challenge from Target.
// Calculate Response value.
//
Result = IScsiNetNtoi (Identifier);
if (Result > 0xFF) {
goto ON_EXIT;
}
AuthData->InIdentifier = (UINT32) Result;
- AuthData->InChallengeLength = ISCSI_CHAP_AUTH_MAX_LEN;
+ AuthData->InChallengeLength = (UINT32) sizeof (AuthData->InChallenge);
IScsiHexToBin (
(UINT8 *) AuthData->InChallenge,
&AuthData->InChallengeLength,
Challenge
);
Status = IScsiCHAPCalculateResponse (
AuthData->InIdentifier,
AuthData->AuthConfig->CHAPSecret,
(UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
AuthData->InChallenge,
AuthData->InChallengeLength,
AuthData->CHAPResponse
);
//
// Transit to next step.
//
Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
break;
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 03/10] NetworkPkg/IScsiDxe: clean up "ISCSI_CHAP_AUTH_DATA.OutChallengeLength"
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 02/10] NetworkPkg/IScsiDxe: simplify "ISCSI_CHAP_AUTH_DATA.InChallenge" size Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 04/10] NetworkPkg/IScsiDxe: clean up library class dependencies Laszlo Ersek
` (7 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
The "ISCSI_CHAP_AUTH_DATA.OutChallenge" field is declared as a UINT8 array
with ISCSI_CHAP_AUTH_MAX_LEN (1024) elements. However, when the challenge
is generated and formatted, only ISCSI_CHAP_RSP_LEN (16) octets are used
in the array.
Change the array size to ISCSI_CHAP_RSP_LEN, and remove the (now unused)
ISCSI_CHAP_AUTH_MAX_LEN macro.
Remove the "ISCSI_CHAP_AUTH_DATA.OutChallengeLength" field, which is
superfluous too.
Most importantly, explain in a new comment *why* tying the challenge size
to the digest size (ISCSI_CHAP_RSP_LEN) has always made sense. (See also
Linux kernel commit 19f5f88ed779, "scsi: target: iscsi: tie the challenge
length to the hash digest size", 2019-11-06.) For sure, the motivation
that the new comment now explains has always been there, and has always
been the same, for IScsiDxe; it's just that now we spell it out too.
No change in peer-visible behavior.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.h | 9 ++++++---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 3 +--
2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
index 1fc1d96ea3f3..35d5d6ec29e3 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
@@ -3,39 +3,38 @@
Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_
#define ISCSI_AUTH_METHOD_CHAP "CHAP"
#define ISCSI_KEY_CHAP_ALGORITHM "CHAP_A"
#define ISCSI_KEY_CHAP_IDENTIFIER "CHAP_I"
#define ISCSI_KEY_CHAP_CHALLENGE "CHAP_C"
#define ISCSI_KEY_CHAP_NAME "CHAP_N"
#define ISCSI_KEY_CHAP_RESPONSE "CHAP_R"
#define ISCSI_CHAP_ALGORITHM_MD5 5
-#define ISCSI_CHAP_AUTH_MAX_LEN 1024
///
/// MD5_HASHSIZE
///
#define ISCSI_CHAP_RSP_LEN 16
#define ISCSI_CHAP_STEP_ONE 1
#define ISCSI_CHAP_STEP_TWO 2
#define ISCSI_CHAP_STEP_THREE 3
#define ISCSI_CHAP_STEP_FOUR 4
#pragma pack(1)
typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {
UINT8 CHAPType;
CHAR8 CHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 CHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
CHAR8 ReverseCHAPName[ISCSI_CHAP_NAME_STORAGE];
CHAR8 ReverseCHAPSecret[ISCSI_CHAP_SECRET_STORAGE];
@@ -43,41 +42,45 @@ typedef struct _ISCSI_CHAP_AUTH_CONFIG_NVDATA {
#pragma pack()
///
/// ISCSI CHAP Authentication Data
///
typedef struct _ISCSI_CHAP_AUTH_DATA {
ISCSI_CHAP_AUTH_CONFIG_NVDATA *AuthConfig;
UINT32 InIdentifier;
UINT8 InChallenge[1024];
UINT32 InChallengeLength;
//
// Calculated CHAP Response (CHAP_R) value.
//
UINT8 CHAPResponse[ISCSI_CHAP_RSP_LEN];
//
// Auth-data to be sent out for mutual authentication.
//
+ // While the challenge size is technically independent of the hashing
+ // algorithm, it is good practice to avoid hashing *fewer bytes* than the
+ // digest size. In other words, it's good practice to feed *at least as many
+ // bytes* to the hashing algorithm as the hashing algorithm will output.
+ //
UINT32 OutIdentifier;
- UINT8 OutChallenge[ISCSI_CHAP_AUTH_MAX_LEN];
- UINT32 OutChallengeLength;
+ UINT8 OutChallenge[ISCSI_CHAP_RSP_LEN];
} ISCSI_CHAP_AUTH_DATA;
/**
This function checks the received iSCSI Login Response during the security
negotiation stage.
@param[in] Conn The iSCSI connection.
@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.
@retval Others Other errors as indicated.
**/
EFI_STATUS
IScsiCHAPOnRspReceived (
IN ISCSI_CONNECTION *Conn
);
/**
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index df3c2eb1200a..9e192ce292e8 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -106,39 +106,39 @@ IScsiCHAPCalculateResponse (
**/
EFI_STATUS
IScsiCHAPAuthTarget (
IN ISCSI_CHAP_AUTH_DATA *AuthData,
IN UINT8 *TargetResponse
)
{
EFI_STATUS Status;
UINT32 SecretSize;
UINT8 VerifyRsp[ISCSI_CHAP_RSP_LEN];
Status = EFI_SUCCESS;
SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
Status = IScsiCHAPCalculateResponse (
AuthData->OutIdentifier,
AuthData->AuthConfig->ReverseCHAPSecret,
SecretSize,
AuthData->OutChallenge,
- AuthData->OutChallengeLength,
+ ISCSI_CHAP_RSP_LEN, // ChallengeLength
VerifyRsp
);
if (CompareMem (VerifyRsp, TargetResponse, ISCSI_CHAP_RSP_LEN) != 0) {
Status = EFI_SECURITY_VIOLATION;
}
return Status;
}
/**
This function checks the received iSCSI Login Response during the security
negotiation stage.
@param[in] Conn The iSCSI connection.
@retval EFI_SUCCESS The Login Response passed the CHAP validation.
@retval EFI_OUT_OF_RESOURCES Failed to allocate memory.
@@ -474,39 +474,38 @@ IScsiCHAPToSendReq (
IScsiBinToHex (
(UINT8 *) AuthData->CHAPResponse,
ISCSI_CHAP_RSP_LEN,
Response,
&RspLen
);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);
if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
//
// CHAP_I=<I>
//
IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
//
// CHAP_C=<C>
//
IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
- AuthData->OutChallengeLength = ISCSI_CHAP_RSP_LEN;
IScsiBinToHex (
(UINT8 *) AuthData->OutChallenge,
ISCSI_CHAP_RSP_LEN,
Challenge,
&ChallengeLen
);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);
Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
}
//
// Set the stage transition flag.
//
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
break;
default:
Status = EFI_PROTOCOL_ERROR;
break;
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 04/10] NetworkPkg/IScsiDxe: clean up library class dependencies
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
` (2 preceding siblings ...)
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 03/10] NetworkPkg/IScsiDxe: clean up "ISCSI_CHAP_AUTH_DATA.OutChallengeLength" Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 05/10] NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex() Laszlo Ersek
` (6 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
Sort the library class dependencies in the #include directives and in the
INF file. Remove the DpcLib class from the #include directives -- it is
not listed in the INF file, and IScsiDxe doesn't call either DpcLib API
(QueueDpc(), DispatchDpc()). No functional changes.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiDxe.inf | 6 +++---
NetworkPkg/IScsiDxe/IScsiImpl.h | 17 ++++++++---------
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiDxe.inf b/NetworkPkg/IScsiDxe/IScsiDxe.inf
index 0ffb340ce05e..543c4083026a 100644
--- a/NetworkPkg/IScsiDxe/IScsiDxe.inf
+++ b/NetworkPkg/IScsiDxe/IScsiDxe.inf
@@ -49,53 +49,53 @@ [Sources]
IScsiDriver.c
IScsiDriver.h
IScsiExtScsiPassThru.c
IScsiIbft.c
IScsiIbft.h
IScsiInitiatorName.c
IScsiImpl.h
IScsiMisc.c
IScsiMisc.h
IScsiProto.c
IScsiProto.h
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
CryptoPkg/CryptoPkg.dec
NetworkPkg/NetworkPkg.dec
[LibraryClasses]
+ BaseCryptLib
BaseLib
BaseMemoryLib
DebugLib
DevicePathLib
HiiLib
MemoryAllocationLib
NetLib
- TcpIoLib
PrintLib
+ TcpIoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
+ UefiHiiServicesLib
UefiLib
UefiRuntimeServicesTableLib
- UefiHiiServicesLib
- BaseCryptLib
[Protocols]
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES ## SystemTable
gEfiDriverBindingProtocolGuid ## SOMETIMES_PRODUCES
gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiIp4Config2ProtocolGuid ## SOMETIMES_CONSUMES
gEfiIp6ConfigProtocolGuid ## SOMETIMES_CONSUMES
gEfiTcp4ProtocolGuid ## TO_START
gEfiTcp6ProtocolGuid ## TO_START
gEfiTcp4ServiceBindingProtocolGuid ## TO_START
gEfiTcp6ServiceBindingProtocolGuid ## TO_START
diff --git a/NetworkPkg/IScsiDxe/IScsiImpl.h b/NetworkPkg/IScsiDxe/IScsiImpl.h
index 387ab9765e9e..d895c7feb947 100644
--- a/NetworkPkg/IScsiDxe/IScsiImpl.h
+++ b/NetworkPkg/IScsiDxe/IScsiImpl.h
@@ -19,53 +19,52 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/DevicePath.h>
#include <Protocol/HiiConfigAccess.h>
#include <Protocol/Ip6.h>
#include <Protocol/Dhcp4.h>
#include <Protocol/Dhcp6.h>
#include <Protocol/Dns4.h>
#include <Protocol/Dns6.h>
#include <Protocol/Tcp4.h>
#include <Protocol/Tcp6.h>
#include <Protocol/Ip4Config2.h>
#include <Protocol/Ip6Config.h>
#include <Protocol/AuthenticationInfo.h>
#include <Protocol/IScsiInitiatorName.h>
#include <Protocol/ScsiPassThruExt.h>
#include <Protocol/AdapterInformation.h>
#include <Protocol/NetworkInterfaceIdentifier.h>
-#include <Library/HiiLib.h>
-#include <Library/UefiHiiServicesLib.h>
-#include <Library/DevicePathLib.h>
-#include <Library/DebugLib.h>
+#include <Library/BaseCryptLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DevicePathLib.h>
+#include <Library/HiiLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/NetLib.h>
#include <Library/PrintLib.h>
+#include <Library/TcpIoLib.h>
#include <Library/UefiBootServicesTableLib.h>
-#include <Library/UefiRuntimeServicesTableLib.h>
+#include <Library/UefiHiiServicesLib.h>
#include <Library/UefiLib.h>
-#include <Library/DpcLib.h>
-#include <Library/NetLib.h>
-#include <Library/TcpIoLib.h>
-#include <Library/BaseCryptLib.h>
+#include <Library/UefiRuntimeServicesTableLib.h>
#include <Guid/MdeModuleHii.h>
#include <Guid/EventGroup.h>
#include <Guid/Acpi.h>
#include "IScsiConfigNVDataStruc.h"
#include "IScsiDriver.h"
#include "IScsiProto.h"
#include "IScsiCHAP.h"
#include "IScsiDhcp.h"
#include "IScsiDhcp6.h"
#include "IScsiIbft.h"
#include "IScsiMisc.h"
#include "IScsiDns.h"
#include "IScsiConfig.h"
#define ISCSI_AUTH_INITIAL 0
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 05/10] NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex()
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
` (3 preceding siblings ...)
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 04/10] NetworkPkg/IScsiDxe: clean up library class dependencies Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 06/10] NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds Laszlo Ersek
` (5 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
Considering IScsiBinToHex():
> if (((*HexLength) - 3) < BinLength * 2) {
> *HexLength = BinLength * 2 + 3;
> }
the following subexpressions are problematic:
(*HexLength) - 3
BinLength * 2
BinLength * 2 + 3
The first one may wrap under zero, the latter two may wrap over
MAX_UINT32.
Rewrite the calculation using SafeIntLib.
While at it, change the type of the "Index" variable from UINTN to UINT32.
The largest "Index"-based value that we calculate is
Index * 2 + 2 (with (Index == BinLength))
Because the patch makes
BinLength * 2 + 3
safe to calculate in UINT32, using UINT32 for
Index * 2 + 2 (with (Index == BinLength))
is safe too. Consistently using UINT32 improves readability.
This patch is best reviewed with "git show -W".
The integer overflows that this patch fixes are theoretical; a subsequent
patch in the series will audit the IScsiBinToHex() call sites, and show
that none of them can fail.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiDxe.inf | 1 +
NetworkPkg/IScsiDxe/IScsiImpl.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.c | 19 +++++++++++++++----
4 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiDxe.inf b/NetworkPkg/IScsiDxe/IScsiDxe.inf
index 543c4083026a..1dde56d00ca2 100644
--- a/NetworkPkg/IScsiDxe/IScsiDxe.inf
+++ b/NetworkPkg/IScsiDxe/IScsiDxe.inf
@@ -58,38 +58,39 @@ [Sources]
IScsiProto.c
IScsiProto.h
[Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
CryptoPkg/CryptoPkg.dec
NetworkPkg/NetworkPkg.dec
[LibraryClasses]
BaseCryptLib
BaseLib
BaseMemoryLib
DebugLib
DevicePathLib
HiiLib
MemoryAllocationLib
NetLib
PrintLib
+ SafeIntLib
TcpIoLib
UefiBootServicesTableLib
UefiDriverEntryPoint
UefiHiiServicesLib
UefiLib
UefiRuntimeServicesTableLib
[Protocols]
gEfiAcpiTableProtocolGuid ## SOMETIMES_CONSUMES ## SystemTable
gEfiDriverBindingProtocolGuid ## SOMETIMES_PRODUCES
gEfiPciIoProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDhcp6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns4ProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ServiceBindingProtocolGuid ## SOMETIMES_CONSUMES
gEfiDns6ProtocolGuid ## SOMETIMES_CONSUMES
diff --git a/NetworkPkg/IScsiDxe/IScsiImpl.h b/NetworkPkg/IScsiDxe/IScsiImpl.h
index d895c7feb947..ac3a25730efb 100644
--- a/NetworkPkg/IScsiDxe/IScsiImpl.h
+++ b/NetworkPkg/IScsiDxe/IScsiImpl.h
@@ -28,38 +28,39 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/Tcp6.h>
#include <Protocol/Ip4Config2.h>
#include <Protocol/Ip6Config.h>
#include <Protocol/AuthenticationInfo.h>
#include <Protocol/IScsiInitiatorName.h>
#include <Protocol/ScsiPassThruExt.h>
#include <Protocol/AdapterInformation.h>
#include <Protocol/NetworkInterfaceIdentifier.h>
#include <Library/BaseCryptLib.h>
#include <Library/BaseLib.h>
#include <Library/BaseMemoryLib.h>
#include <Library/DebugLib.h>
#include <Library/DevicePathLib.h>
#include <Library/HiiLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/NetLib.h>
#include <Library/PrintLib.h>
+#include <Library/SafeIntLib.h>
#include <Library/TcpIoLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Library/UefiHiiServicesLib.h>
#include <Library/UefiLib.h>
#include <Library/UefiRuntimeServicesTableLib.h>
#include <Guid/MdeModuleHii.h>
#include <Guid/EventGroup.h>
#include <Guid/Acpi.h>
#include "IScsiConfigNVDataStruc.h"
#include "IScsiDriver.h"
#include "IScsiProto.h"
#include "IScsiCHAP.h"
#include "IScsiDhcp.h"
#include "IScsiDhcp6.h"
#include "IScsiIbft.h"
#include "IScsiMisc.h"
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 46c725aab3a4..231413993b08 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -134,38 +134,39 @@ IScsiMacAddrToStr (
**/
EFI_STATUS
IScsiAsciiStrToIp (
IN CHAR8 *Str,
IN UINT8 IpMode,
OUT EFI_IP_ADDRESS *Ip
);
/**
Convert the binary encoded buffer into a hexadecimal encoded string.
@param[in] BinBuffer The buffer containing the binary data.
@param[in] BinLength Length of the binary buffer.
@param[in, out] HexStr Pointer to the string.
@param[in, out] HexLength The length of the string.
@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
+ @retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);
/**
Convert the hexadecimal string into a binary encoded buffer.
@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.
@retval EFI_SUCCESS The hexadecimal string is converted into a binary
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index b8fef3ff6f5a..42988e15cb06 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -300,61 +300,72 @@ IScsiMacAddrToStr (
String = &Str[3 * Index - 1] ;
if (VlanId != 0) {
String += UnicodeSPrint (String, 6 * sizeof (CHAR16), L"\\%04x", (UINTN) VlanId);
}
*String = L'\0';
}
/**
Convert the binary encoded buffer into a hexadecimal encoded string.
@param[in] BinBuffer The buffer containing the binary data.
@param[in] BinLength Length of the binary buffer.
@param[in, out] HexStr Pointer to the string.
@param[in, out] HexLength The length of the string.
@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
+ @retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
)
{
- UINTN Index;
+ UINT32 HexLengthMin;
+ UINT32 HexLengthProvided;
+ UINT32 Index;
if ((HexStr == NULL) || (BinBuffer == NULL) || (BinLength == 0)) {
return EFI_INVALID_PARAMETER;
}
- if (((*HexLength) - 3) < BinLength * 2) {
- *HexLength = BinLength * 2 + 3;
+ //
+ // Safely calculate: HexLengthMin := BinLength * 2 + 3.
+ //
+ if (RETURN_ERROR (SafeUint32Mult (BinLength, 2, &HexLengthMin)) ||
+ RETURN_ERROR (SafeUint32Add (HexLengthMin, 3, &HexLengthMin))) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+
+ HexLengthProvided = *HexLength;
+ *HexLength = HexLengthMin;
+ if (HexLengthProvided < HexLengthMin) {
return EFI_BUFFER_TOO_SMALL;
}
- *HexLength = BinLength * 2 + 3;
//
// Prefix for Hex String.
//
HexStr[0] = '0';
HexStr[1] = 'x';
for (Index = 0; Index < BinLength; Index++) {
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}
HexStr[Index * 2 + 2] = '\0';
return EFI_SUCCESS;
}
/**
Convert the hexadecimal string into a binary encoded buffer.
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 06/10] NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
` (4 preceding siblings ...)
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 05/10] NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex() Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 07/10] NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block Laszlo Ersek
` (4 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
IScsiBinToHex() is called for encoding:
- the answer to the target's challenge; that is, CHAP_R;
- the challenge for the target, in case mutual authentication is enabled;
that is, CHAP_C.
The initiator controls the size of both blobs, the sizes of their hex
encodings are correctly calculated in "RspLen" and "ChallengeLen".
Therefore the IScsiBinToHex() calls never fail; assert that.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 27 +++++++++++---------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index 9e192ce292e8..dbe3c8ef46f9 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -375,38 +375,39 @@ IScsiCHAPOnRspReceived (
@retval EFI_PROTOCOL_ERROR Some kind of protocol error occurred.
**/
EFI_STATUS
IScsiCHAPToSendReq (
IN ISCSI_CONNECTION *Conn,
IN OUT NET_BUF *Pdu
)
{
EFI_STATUS Status;
ISCSI_SESSION *Session;
ISCSI_LOGIN_REQUEST *LoginReq;
ISCSI_CHAP_AUTH_DATA *AuthData;
CHAR8 *Value;
CHAR8 ValueStr[256];
CHAR8 *Response;
UINT32 RspLen;
CHAR8 *Challenge;
UINT32 ChallengeLen;
+ EFI_STATUS BinToHexStatus;
ASSERT (Conn->CurrentStage == ISCSI_SECURITY_NEGOTIATION);
Session = Conn->Session;
AuthData = &Session->AuthData.CHAP;
LoginReq = (ISCSI_LOGIN_REQUEST *) NetbufGetByte (Pdu, 0, 0);
if (LoginReq == NULL) {
return EFI_PROTOCOL_ERROR;
}
Status = EFI_SUCCESS;
RspLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
Response = AllocateZeroPool (RspLen);
if (Response == NULL) {
return EFI_OUT_OF_RESOURCES;
}
ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
Challenge = AllocateZeroPool (ChallengeLen);
@@ -455,63 +456,65 @@ IScsiCHAPToSendReq (
Conn->AuthStep = ISCSI_CHAP_STEP_TWO;
break;
case ISCSI_CHAP_STEP_THREE:
//
// Third step, send the Login Request with CHAP_N=<N> CHAP_R=<R> or
// CHAP_N=<N> CHAP_R=<R> CHAP_I=<I> CHAP_C=<C> if target authentication is
// required too.
//
// CHAP_N=<N>
//
IScsiAddKeyValuePair (
Pdu,
ISCSI_KEY_CHAP_NAME,
(CHAR8 *) &AuthData->AuthConfig->CHAPName
);
//
// CHAP_R=<R>
//
- IScsiBinToHex (
- (UINT8 *) AuthData->CHAPResponse,
- ISCSI_CHAP_RSP_LEN,
- Response,
- &RspLen
- );
+ BinToHexStatus = IScsiBinToHex (
+ (UINT8 *) AuthData->CHAPResponse,
+ ISCSI_CHAP_RSP_LEN,
+ Response,
+ &RspLen
+ );
+ ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);
if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
//
// CHAP_I=<I>
//
IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
//
// CHAP_C=<C>
//
IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
- IScsiBinToHex (
- (UINT8 *) AuthData->OutChallenge,
- ISCSI_CHAP_RSP_LEN,
- Challenge,
- &ChallengeLen
- );
+ BinToHexStatus = IScsiBinToHex (
+ (UINT8 *) AuthData->OutChallenge,
+ ISCSI_CHAP_RSP_LEN,
+ Challenge,
+ &ChallengeLen
+ );
+ ASSERT_EFI_ERROR (BinToHexStatus);
IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);
Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
}
//
// Set the stage transition flag.
//
ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
break;
default:
Status = EFI_PROTOCOL_ERROR;
break;
}
FreePool (Response);
FreePool (Challenge);
return Status;
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 07/10] NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
` (5 preceding siblings ...)
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 06/10] NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 08/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing Laszlo Ersek
` (3 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
We'll need further return values for IScsiHexToBin() in a subsequent
patch; make room for them in the leading comment block of the function.
While at it, rewrap the comment block to 80 characters width.
No functional changes.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 14 +++++++-------
NetworkPkg/IScsiDxe/IScsiMisc.c | 14 +++++++-------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 231413993b08..28cf408cd5c5 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -149,46 +149,46 @@ IScsiAsciiStrToIp (
@retval EFI_SUCCESS The binary data is converted to the hexadecimal string
and the length of the string is updated.
@retval EFI_BUFFER_TOO_SMALL The string is too small.
@retval EFI_BAD_BUFFER_SIZE BinLength is too large for hex encoding.
@retval EFI_INVALID_PARAMETER The IP string is malformatted.
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);
/**
Convert the hexadecimal string into a binary encoded buffer.
- @param[in, out] BinBuffer The binary buffer.
- @param[in, out] BinLength Length of the binary buffer.
- @param[in] HexStr The hexadecimal string.
-
- @retval EFI_SUCCESS The hexadecimal string is converted into a binary
- encoded buffer.
- @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the converted data.
+ @param[in, out] BinBuffer The binary buffer.
+ @param[in, out] BinLength Length of the binary buffer.
+ @param[in] HexStr The hexadecimal string.
+ @retval EFI_SUCCESS The hexadecimal string is converted into a
+ binary encoded buffer.
+ @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
+ converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);
/**
Convert the decimal-constant string or hex-constant string into a numerical value.
@param[in] Str String in decimal or hex.
@return The numerical value.
**/
UINTN
IScsiNetNtoi (
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index 42988e15cb06..014700e87a5f 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -354,46 +354,46 @@ IScsiBinToHex (
// Prefix for Hex String.
//
HexStr[0] = '0';
HexStr[1] = 'x';
for (Index = 0; Index < BinLength; Index++) {
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}
HexStr[Index * 2 + 2] = '\0';
return EFI_SUCCESS;
}
/**
Convert the hexadecimal string into a binary encoded buffer.
- @param[in, out] BinBuffer The binary buffer.
- @param[in, out] BinLength Length of the binary buffer.
- @param[in] HexStr The hexadecimal string.
-
- @retval EFI_SUCCESS The hexadecimal string is converted into a binary
- encoded buffer.
- @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the converted data.
+ @param[in, out] BinBuffer The binary buffer.
+ @param[in, out] BinLength Length of the binary buffer.
+ @param[in] HexStr The hexadecimal string.
+ @retval EFI_SUCCESS The hexadecimal string is converted into a
+ binary encoded buffer.
+ @retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
+ converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];
ZeroMem (TemStr, sizeof (TemStr));
//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 08/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
` (6 preceding siblings ...)
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 07/10] NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 09/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow Laszlo Ersek
` (2 subsequent siblings)
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
The IScsiHexToBin() function has the following parser issues:
(1) If the *subject sequence* in "HexStr" is empty, the function returns
EFI_SUCCESS (with "BinLength" set to 0 on output). Such inputs should
be rejected.
(2) The function mis-handles a "HexStr" that ends with a stray nibble. For
example, if "HexStr" is "0xABC", the function decodes it to the bytes
{0xAB, 0x0C}, sets "BinLength" to 2 on output, and returns
EFI_SUCCESS. Such inputs should be rejected.
(3) If an invalid hex char is found in "HexStr", the function treats it as
end-of-hex-string, and returns EFI_SUCCESS. Such inputs should be
rejected.
All of the above cases are remotely triggerable, as shown in a subsequent
patch, which adds error checking to the IScsiHexToBin() call sites. While
the initiator is not immediately compromised, incorrectly parsing CHAP_R
from the target, in case of mutual authentication, is not great.
Extend the interface contract of IScsiHexToBin() with
EFI_INVALID_PARAMETER, for reporting issues (1) through (3), and implement
the new checks.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 1 +
NetworkPkg/IScsiDxe/IScsiMisc.c | 12 ++++++++++--
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 28cf408cd5c5..404a482e57f3 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -155,38 +155,39 @@ IScsiAsciiStrToIp (
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);
/**
Convert the hexadecimal string into a binary encoded buffer.
@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.
@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
+ @retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);
/**
Convert the decimal-constant string or hex-constant string into a numerical value.
@param[in] Str String in decimal or hex.
@return The numerical value.
**/
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index 014700e87a5f..f0f4992b07c7 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -360,72 +360,80 @@ IScsiBinToHex (
HexStr[Index * 2 + 2] = IScsiHexString[BinBuffer[Index] >> 4];
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}
HexStr[Index * 2 + 2] = '\0';
return EFI_SUCCESS;
}
/**
Convert the hexadecimal string into a binary encoded buffer.
@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.
@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
+ @retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];
ZeroMem (TemStr, sizeof (TemStr));
//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
HexStr += 2;
}
Length = AsciiStrLen (HexStr);
+ //
+ // Reject an empty hex string; reject a stray nibble.
+ //
+ if (Length == 0 || Length % 2 != 0) {
+ return EFI_INVALID_PARAMETER;
+ }
+
for (Index = 0; Index < Length; Index ++) {
TemStr[0] = HexStr[Index];
Digit = (UINT8) AsciiStrHexToUint64 (TemStr);
if (Digit == 0 && TemStr[0] != '0') {
//
- // Invalid Lun Char.
+ // Invalid Hex Char.
//
- break;
+ return EFI_INVALID_PARAMETER;
}
if ((Index & 1) == 0) {
BinBuffer [Index/2] = Digit;
} else {
BinBuffer [Index/2] = (UINT8) ((BinBuffer [Index/2] << 4) + Digit);
}
}
*BinLength = (UINT32) ((Index + 1)/2);
return EFI_SUCCESS;
}
/**
Convert the decimal-constant string or hex-constant string into a numerical value.
@param[in] Str String in decimal or hex.
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 09/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
` (7 preceding siblings ...)
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 08/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 10/10] NetworkPkg/IScsiDxe: check IScsiHexToBin() return values Laszlo Ersek
2021-06-09 17:33 ` [edk2-devel] [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
The IScsiHexToBin() function documents the EFI_BUFFER_TOO_SMALL return
condition, but never actually checks whether the decoded buffer fits into
the caller-provided room (i.e., the input value of "BinLength"), and
EFI_BUFFER_TOO_SMALL is never returned. The decoding of "HexStr" can
overflow "BinBuffer".
This is remotely exploitable, as shown in a subsequent patch, which adds
error checking to the IScsiHexToBin() call sites. This issue allows the
target to compromise the initiator.
Introduce EFI_BAD_BUFFER_SIZE, in addition to the existent
EFI_BUFFER_TOO_SMALL, for reporting a special case of the buffer overflow,
plus actually catch the buffer overflow.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
NetworkPkg/IScsiDxe/IScsiMisc.h | 3 +++
NetworkPkg/IScsiDxe/IScsiMisc.c | 20 +++++++++++++++++---
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.h b/NetworkPkg/IScsiDxe/IScsiMisc.h
index 404a482e57f3..fddef4f466dc 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.h
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.h
@@ -156,38 +156,41 @@ IScsiAsciiStrToIp (
**/
EFI_STATUS
IScsiBinToHex (
IN UINT8 *BinBuffer,
IN UINT32 BinLength,
IN OUT CHAR8 *HexStr,
IN OUT UINT32 *HexLength
);
/**
Convert the hexadecimal string into a binary encoded buffer.
@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.
@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
@retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
+ @retval EFI_BAD_BUFFER_SIZE The length of HexStr is too large for decoding:
+ the decoded size cannot be expressed in
+ BinLength on output.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
);
/**
Convert the decimal-constant string or hex-constant string into a numerical value.
@param[in] Str String in decimal or hex.
@return The numerical value.
**/
diff --git a/NetworkPkg/IScsiDxe/IScsiMisc.c b/NetworkPkg/IScsiDxe/IScsiMisc.c
index f0f4992b07c7..406954786751 100644
--- a/NetworkPkg/IScsiDxe/IScsiMisc.c
+++ b/NetworkPkg/IScsiDxe/IScsiMisc.c
@@ -361,89 +361,103 @@ IScsiBinToHex (
HexStr[Index * 2 + 3] = IScsiHexString[BinBuffer[Index] & 0xf];
}
HexStr[Index * 2 + 2] = '\0';
return EFI_SUCCESS;
}
/**
Convert the hexadecimal string into a binary encoded buffer.
@param[in, out] BinBuffer The binary buffer.
@param[in, out] BinLength Length of the binary buffer.
@param[in] HexStr The hexadecimal string.
@retval EFI_SUCCESS The hexadecimal string is converted into a
binary encoded buffer.
@retval EFI_INVALID_PARAMETER Invalid hex encoding found in HexStr.
+ @retval EFI_BAD_BUFFER_SIZE The length of HexStr is too large for decoding:
+ the decoded size cannot be expressed in
+ BinLength on output.
@retval EFI_BUFFER_TOO_SMALL The binary buffer is too small to hold the
converted data.
**/
EFI_STATUS
IScsiHexToBin (
IN OUT UINT8 *BinBuffer,
IN OUT UINT32 *BinLength,
IN CHAR8 *HexStr
)
{
+ UINTN BinLengthMin;
+ UINT32 BinLengthProvided;
UINTN Index;
UINTN Length;
UINT8 Digit;
CHAR8 TemStr[2];
ZeroMem (TemStr, sizeof (TemStr));
//
// Find out how many hex characters the string has.
//
if ((HexStr[0] == '0') && ((HexStr[1] == 'x') || (HexStr[1] == 'X'))) {
HexStr += 2;
}
Length = AsciiStrLen (HexStr);
//
// Reject an empty hex string; reject a stray nibble.
//
if (Length == 0 || Length % 2 != 0) {
return EFI_INVALID_PARAMETER;
}
+ //
+ // Check if the caller provides enough room for the decoded blob.
+ //
+ BinLengthMin = Length / 2;
+ if (BinLengthMin > MAX_UINT32) {
+ return EFI_BAD_BUFFER_SIZE;
+ }
+ BinLengthProvided = *BinLength;
+ *BinLength = (UINT32)BinLengthMin;
+ if (BinLengthProvided < BinLengthMin) {
+ return EFI_BUFFER_TOO_SMALL;
+ }
for (Index = 0; Index < Length; Index ++) {
TemStr[0] = HexStr[Index];
Digit = (UINT8) AsciiStrHexToUint64 (TemStr);
if (Digit == 0 && TemStr[0] != '0') {
//
// Invalid Hex Char.
//
return EFI_INVALID_PARAMETER;
}
if ((Index & 1) == 0) {
BinBuffer [Index/2] = Digit;
} else {
BinBuffer [Index/2] = (UINT8) ((BinBuffer [Index/2] << 4) + Digit);
}
}
-
- *BinLength = (UINT32) ((Index + 1)/2);
-
return EFI_SUCCESS;
}
/**
Convert the decimal-constant string or hex-constant string into a numerical value.
@param[in] Str String in decimal or hex.
@return The numerical value.
**/
UINTN
IScsiNetNtoi (
IN CHAR8 *Str
)
{
if ((Str[0] == '0') && ((Str[1] == 'x') || (Str[1] == 'X'))) {
Str += 2;
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PUBLIC edk2 PATCH v2 10/10] NetworkPkg/IScsiDxe: check IScsiHexToBin() return values
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
` (8 preceding siblings ...)
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 09/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow Laszlo Ersek
@ 2021-06-08 12:12 ` Laszlo Ersek
2021-06-09 17:33 ` [edk2-devel] [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-08 12:12 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
IScsiDxe (that is, the initiator) receives two hex-encoded strings from
the iSCSI target:
- CHAP_C, where the target challenges the initiator,
- CHAP_R, where the target answers the challenge from the initiator (in
case the initiator wants mutual authentication).
Accordingly, we have two IScsiHexToBin() call sites:
- At the CHAP_C decoding site, check whether the decoding succeeds. The
decoded buffer ("AuthData->InChallenge") can accommodate 1024 bytes,
which is a permissible restriction on the target, per
<https://tools.ietf.org/html/rfc7143#section-12.1.3>. Shorter challenges
from the target are acceptable.
- At the CHAP_R decoding site, enforce that the decoding both succeed, and
provide exactly ISCSI_CHAP_RSP_LEN bytes. CHAP_R contains the digest
calculated by the target, therefore it must be of fixed size. We may
only call IScsiCHAPAuthTarget() if "TargetRsp" has been fully populated.
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Siyuan Fu <siyuan.fu@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
---
NetworkPkg/IScsiDxe/IScsiCHAP.c | 20 ++++++++++++++------
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
index dbe3c8ef46f9..7e930c0d1eab 100644
--- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
+++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
@@ -274,43 +274,47 @@ IScsiCHAPOnRspReceived (
Challenge = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_CHALLENGE
);
if (Challenge == NULL) {
goto ON_EXIT;
}
//
// Process the CHAP identifier and CHAP Challenge from Target.
// Calculate Response value.
//
Result = IScsiNetNtoi (Identifier);
if (Result > 0xFF) {
goto ON_EXIT;
}
AuthData->InIdentifier = (UINT32) Result;
AuthData->InChallengeLength = (UINT32) sizeof (AuthData->InChallenge);
- IScsiHexToBin (
- (UINT8 *) AuthData->InChallenge,
- &AuthData->InChallengeLength,
- Challenge
- );
+ Status = IScsiHexToBin (
+ (UINT8 *) AuthData->InChallenge,
+ &AuthData->InChallengeLength,
+ Challenge
+ );
+ if (EFI_ERROR (Status)) {
+ Status = EFI_PROTOCOL_ERROR;
+ goto ON_EXIT;
+ }
Status = IScsiCHAPCalculateResponse (
AuthData->InIdentifier,
AuthData->AuthConfig->CHAPSecret,
(UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
AuthData->InChallenge,
AuthData->InChallengeLength,
AuthData->CHAPResponse
);
//
// Transit to next step.
//
Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
break;
case ISCSI_CHAP_STEP_THREE:
//
// One way CHAP authentication and the target would like to
// authenticate us.
@@ -321,39 +325,43 @@ IScsiCHAPOnRspReceived (
case ISCSI_CHAP_STEP_FOUR:
ASSERT (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL);
//
// The forth step, CHAP_N=<N> CHAP_R=<R> is received from Target.
//
Name = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_NAME);
if (Name == NULL) {
goto ON_EXIT;
}
Response = IScsiGetValueByKeyFromList (
KeyValueList,
ISCSI_KEY_CHAP_RESPONSE
);
if (Response == NULL) {
goto ON_EXIT;
}
RspLen = ISCSI_CHAP_RSP_LEN;
- IScsiHexToBin (TargetRsp, &RspLen, Response);
+ Status = IScsiHexToBin (TargetRsp, &RspLen, Response);
+ if (EFI_ERROR (Status) || RspLen != ISCSI_CHAP_RSP_LEN) {
+ Status = EFI_PROTOCOL_ERROR;
+ goto ON_EXIT;
+ }
//
// Check the CHAP Name and Response replied by Target.
//
Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
break;
default:
break;
}
ON_EXIT:
if (KeyValueList != NULL) {
IScsiFreeKeyValueList (KeyValueList);
}
FreePool (Data);
--
2.19.1.3.g30247aa5d201
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
` (9 preceding siblings ...)
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 10/10] NetworkPkg/IScsiDxe: check IScsiHexToBin() return values Laszlo Ersek
@ 2021-06-09 17:33 ` Laszlo Ersek
10 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-09 17:33 UTC (permalink / raw)
To: edk2-devel-groups-io
Cc: Jiaxin Wu, Maciej Rabeda, Philippe Mathieu-Daudé, Siyuan Fu
On 06/08/21 14:12, Laszlo Ersek wrote:
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
> Repo: https://pagure.io/lersek/edk2.git
> Branch: iscsi_overflow_bz3356
>
> The main goal of this series is to fix a remotely exploitable buffer
> overflow in the IScsiHexToBin() function.
>
> This posting corresponds to:
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=3356#c22
>
> meaning that it corresponds to the v2 patches attached to, and tested
> in,
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=3356#c17
>
> and that it carries Phil's and Maciej's R-b's that were given up to
> comment#22.
>
> Today is the Public Date for this embargoed security issue; I intend to
> merge the patches tomorrow, based on Maciej's (already given) R-b.
> (Simultaneously with this posting, I'm opening up the BZ publicly.) No
> further review is required; the one day delay on the list is just to
> give the community a (brief) opportunity to speak up, before the patches
> are merged.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
>
> Thanks,
> Laszlo
>
> Laszlo Ersek (10):
> NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
> NetworkPkg/IScsiDxe: simplify "ISCSI_CHAP_AUTH_DATA.InChallenge" size
> NetworkPkg/IScsiDxe: clean up
> "ISCSI_CHAP_AUTH_DATA.OutChallengeLength"
> NetworkPkg/IScsiDxe: clean up library class dependencies
> NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex()
> NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds
> NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block
> NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing
> NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow
> NetworkPkg/IScsiDxe: check IScsiHexToBin() return values
>
> NetworkPkg/IScsiDxe/IScsiCHAP.c | 108 +++++++++++++++-----
> NetworkPkg/IScsiDxe/IScsiCHAP.h | 14 ++-
> NetworkPkg/IScsiDxe/IScsiDxe.inf | 7 +-
> NetworkPkg/IScsiDxe/IScsiImpl.h | 18 ++--
> NetworkPkg/IScsiDxe/IScsiMisc.c | 65 +++++++++---
> NetworkPkg/IScsiDxe/IScsiMisc.h | 19 ++--
> 6 files changed, 166 insertions(+), 65 deletions(-)
>
Merged as commit range 702ba436ed8e..b8649cf2a3e6, via
<https://github.com/tianocore/edk2/pull/1698>.
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters Laszlo Ersek
@ 2021-06-11 11:37 ` Ni, Ray
2021-06-11 11:39 ` Maciej Rabeda
0 siblings, 1 reply; 15+ messages in thread
From: Ni, Ray @ 2021-06-11 11:37 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com
Cc: Wu, Jiaxin, Maciej Rabeda, Philippe Mathieu-Daudé,
Fu, Siyuan
No objection on the patch. Just curious what editor you are using😊
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Tuesday, June 8, 2021 8:13 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Maciej Rabeda <maciej.rabeda@linux.intel.com>; Philippe Mathieu-Daudé
> <philmd@redhat.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [edk2-devel] [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
>
> Working with overlong lines is difficult for me; rewrap the CHAP-related
> source files in IScsiDxe to 80 characters width. No functional changes.
>
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> NetworkPkg/IScsiDxe/IScsiCHAP.h | 3 +-
> NetworkPkg/IScsiDxe/IScsiCHAP.c | 90 +++++++++++++++-----
> 2 files changed, 71 insertions(+), 22 deletions(-)
>
> diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
> index 140bba0dcd76..5e59fb678bd7 100644
> --- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
> +++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
> @@ -72,31 +72,32 @@ typedef struct _ISCSI_CHAP_AUTH_DATA {
>
> @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
> );
> /**
> 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.
> + 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
> );
>
> #endif
> diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
> index 355c6f129f68..cbbc56ae5b43 100644
> --- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
> +++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
> @@ -1,42 +1,45 @@
> /** @file
> - This file is for Challenge-Handshake Authentication Protocol (CHAP) Configuration.
> + 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"
>
> /**
> 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[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_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
> )
> {
> UINTN Md5ContextSize;
> VOID *Md5Ctx;
> CHAR8 IdByte[1];
> EFI_STATUS Status;
>
> @@ -78,40 +81,42 @@ IScsiCHAPCalculateResponse (
> goto Exit;
> }
>
> if (Md5Final (Md5Ctx, ChapResponse)) {
> Status = EFI_SUCCESS;
> }
>
> Exit:
> FreePool (Md5Ctx);
> 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 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];
>
> Status = EFI_SUCCESS;
>
> SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
> Status = IScsiCHAPCalculateResponse (
> AuthData->OutIdentifier,
> AuthData->AuthConfig->ReverseCHAPSecret,
> @@ -177,187 +182,211 @@ IScsiCHAPOnRspReceived (
> //
> NetbufQueCopy (&Conn->RspQue, 0, Len, Data);
>
> //
> // Build the key-value list from the data segment of the Login Response.
> //
> KeyValueList = IScsiBuildKeyValueList ((CHAR8 *) Data, Len);
> if (KeyValueList == NULL) {
> Status = EFI_OUT_OF_RESOURCES;
> goto ON_EXIT;
> }
>
> Status = EFI_PROTOCOL_ERROR;
>
> switch (Conn->AuthStep) {
> case ISCSI_AUTH_INITIAL:
> //
> // The first Login Response.
> //
> - Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_TARGET_PORTAL_GROUP_TAG);
> + Value = IScsiGetValueByKeyFromList (
> + KeyValueList,
> + ISCSI_KEY_TARGET_PORTAL_GROUP_TAG
> + );
> if (Value == NULL) {
> goto ON_EXIT;
> }
>
> Result = IScsiNetNtoi (Value);
> if (Result > 0xFFFF) {
> goto ON_EXIT;
> }
>
> Session->TargetPortalGroupTag = (UINT16) Result;
>
> - Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_AUTH_METHOD);
> + Value = IScsiGetValueByKeyFromList (
> + KeyValueList,
> + ISCSI_KEY_AUTH_METHOD
> + );
> if (Value == NULL) {
> goto ON_EXIT;
> }
> //
> - // Initiator mandates CHAP authentication but target replies without "CHAP", or
> - // initiator suggets "None" but target replies with some kind of auth method.
> + // Initiator mandates CHAP authentication but target replies without
> + // "CHAP", or initiator suggets "None" but target replies with some kind of
> + // auth method.
> //
> if (Session->AuthType == ISCSI_AUTH_TYPE_NONE) {
> if (AsciiStrCmp (Value, ISCSI_KEY_VALUE_NONE) != 0) {
> goto ON_EXIT;
> }
> } else if (Session->AuthType == ISCSI_AUTH_TYPE_CHAP) {
> if (AsciiStrCmp (Value, ISCSI_AUTH_METHOD_CHAP) != 0) {
> goto ON_EXIT;
> }
> } else {
> goto ON_EXIT;
> }
>
> //
> // 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);
> + Value = IScsiGetValueByKeyFromList (
> + KeyValueList,
> + ISCSI_KEY_CHAP_ALGORITHM
> + );
> if (Value == NULL) {
> goto ON_EXIT;
> }
>
> Algorithm = IScsiNetNtoi (Value);
> if (Algorithm != ISCSI_CHAP_ALGORITHM_MD5) {
> //
> // Unsupported algorithm is chosen by target.
> //
> goto ON_EXIT;
> }
>
> - Identifier = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_IDENTIFIER);
> + Identifier = IScsiGetValueByKeyFromList (
> + KeyValueList,
> + ISCSI_KEY_CHAP_IDENTIFIER
> + );
> if (Identifier == NULL) {
> goto ON_EXIT;
> }
>
> - Challenge = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_CHALLENGE);
> + Challenge = IScsiGetValueByKeyFromList (
> + KeyValueList,
> + ISCSI_KEY_CHAP_CHALLENGE
> + );
> if (Challenge == NULL) {
> goto ON_EXIT;
> }
> //
> // Process the CHAP identifier and CHAP Challenge from Target.
> // Calculate Response value.
> //
> Result = IScsiNetNtoi (Identifier);
> if (Result > 0xFF) {
> goto ON_EXIT;
> }
>
> AuthData->InIdentifier = (UINT32) Result;
> AuthData->InChallengeLength = ISCSI_CHAP_AUTH_MAX_LEN;
> - IScsiHexToBin ((UINT8 *) AuthData->InChallenge, &AuthData->InChallengeLength, Challenge);
> + IScsiHexToBin (
> + (UINT8 *) AuthData->InChallenge,
> + &AuthData->InChallengeLength,
> + Challenge
> + );
> Status = IScsiCHAPCalculateResponse (
> AuthData->InIdentifier,
> AuthData->AuthConfig->CHAPSecret,
> (UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
> AuthData->InChallenge,
> AuthData->InChallengeLength,
> AuthData->CHAPResponse
> );
>
> //
> // Transit to next step.
> //
> Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
> break;
>
> case ISCSI_CHAP_STEP_THREE:
> //
> // One way CHAP authentication and the target would like to
> // authenticate us.
> //
> 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);
> + Response = IScsiGetValueByKeyFromList (
> + KeyValueList,
> + ISCSI_KEY_CHAP_RESPONSE
> + );
> if (Response == NULL) {
> goto ON_EXIT;
> }
>
> RspLen = ISCSI_CHAP_RSP_LEN;
> IScsiHexToBin (TargetRsp, &RspLen, Response);
>
> //
> // Check the CHAP Name and Response replied by Target.
> //
> Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
> break;
>
> default:
> break;
> }
>
> ON_EXIT:
>
> if (KeyValueList != NULL) {
> IScsiFreeKeyValueList (KeyValueList);
> }
>
> FreePool (Data);
>
> return Status;
> }
>
>
> /**
> 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.
> + 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
> )
> {
> EFI_STATUS Status;
> ISCSI_SESSION *Session;
> ISCSI_LOGIN_REQUEST *LoginReq;
> ISCSI_CHAP_AUTH_DATA *AuthData;
> CHAR8 *Value;
> CHAR8 ValueStr[256];
> CHAR8 *Response;
> UINT32 RspLen;
> CHAR8 *Challenge;
> @@ -376,95 +405,114 @@ IScsiCHAPToSendReq (
> RspLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
> Response = AllocateZeroPool (RspLen);
> if (Response == NULL) {
> return EFI_OUT_OF_RESOURCES;
> }
>
> ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
> Challenge = AllocateZeroPool (ChallengeLen);
> 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_INITIATOR_NAME,
> + mPrivate->InitiatorName
> + );
> IScsiAddKeyValuePair (Pdu, ISCSI_KEY_SESSION_TYPE, "Normal");
> IScsiAddKeyValuePair (
> Pdu,
> ISCSI_KEY_TARGET_NAME,
> 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.
> + // 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);
>
> 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);
> + IScsiAddKeyValuePair (
> + Pdu,
> + ISCSI_KEY_CHAP_NAME,
> + (CHAR8 *) &AuthData->AuthConfig->CHAPName
> + );
> //
> // CHAP_R=<R>
> //
> - IScsiBinToHex ((UINT8 *) AuthData->CHAPResponse, ISCSI_CHAP_RSP_LEN, Response, &RspLen);
> + IScsiBinToHex (
> + (UINT8 *) AuthData->CHAPResponse,
> + ISCSI_CHAP_RSP_LEN,
> + Response,
> + &RspLen
> + );
> IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);
>
> if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
> //
> // CHAP_I=<I>
> //
> IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
> AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
> IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
> //
> // CHAP_C=<C>
> //
> IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
> AuthData->OutChallengeLength = ISCSI_CHAP_RSP_LEN;
> - IScsiBinToHex ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN, Challenge, &ChallengeLen);
> + IScsiBinToHex (
> + (UINT8 *) AuthData->OutChallenge,
> + ISCSI_CHAP_RSP_LEN,
> + Challenge,
> + &ChallengeLen
> + );
> IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);
>
> Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
> }
> //
> // Set the stage transition flag.
> //
> ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
> break;
>
> default:
> Status = EFI_PROTOCOL_ERROR;
> break;
> }
>
> FreePool (Response);
> FreePool (Challenge);
>
> return Status;
> --
> 2.19.1.3.g30247aa5d201
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
2021-06-11 11:37 ` [edk2-devel] " Ni, Ray
@ 2021-06-11 11:39 ` Maciej Rabeda
2021-06-22 10:39 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Maciej Rabeda @ 2021-06-11 11:39 UTC (permalink / raw)
To: devel, ray.ni, lersek@redhat.com
Cc: Wu, Jiaxin, Philippe Mathieu-Daudé, Fu, Siyuan
I second Ray on the question!
I can only assume it is vi without syntax coloring... :)
On 11-Jun-21 13:37, Ni, Ray wrote:
> No objection on the patch. Just curious what editor you are using😊
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Tuesday, June 8, 2021 8:13 PM
>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Maciej Rabeda <maciej.rabeda@linux.intel.com>; Philippe Mathieu-Daudé
>> <philmd@redhat.com>; Fu, Siyuan <siyuan.fu@intel.com>
>> Subject: [edk2-devel] [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
>>
>> Working with overlong lines is difficult for me; rewrap the CHAP-related
>> source files in IScsiDxe to 80 characters width. No functional changes.
>>
>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>> Cc: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Siyuan Fu <siyuan.fu@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Reviewed-by: Maciej Rabeda <maciej.rabeda@linux.intel.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>> NetworkPkg/IScsiDxe/IScsiCHAP.h | 3 +-
>> NetworkPkg/IScsiDxe/IScsiCHAP.c | 90 +++++++++++++++-----
>> 2 files changed, 71 insertions(+), 22 deletions(-)
>>
>> diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h
>> index 140bba0dcd76..5e59fb678bd7 100644
>> --- a/NetworkPkg/IScsiDxe/IScsiCHAP.h
>> +++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h
>> @@ -72,31 +72,32 @@ typedef struct _ISCSI_CHAP_AUTH_DATA {
>>
>> @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
>> );
>> /**
>> 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.
>> + 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
>> );
>>
>> #endif
>> diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c
>> index 355c6f129f68..cbbc56ae5b43 100644
>> --- a/NetworkPkg/IScsiDxe/IScsiCHAP.c
>> +++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c
>> @@ -1,42 +1,45 @@
>> /** @file
>> - This file is for Challenge-Handshake Authentication Protocol (CHAP) Configuration.
>> + 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"
>>
>> /**
>> 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[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_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
>> )
>> {
>> UINTN Md5ContextSize;
>> VOID *Md5Ctx;
>> CHAR8 IdByte[1];
>> EFI_STATUS Status;
>>
>> @@ -78,40 +81,42 @@ IScsiCHAPCalculateResponse (
>> goto Exit;
>> }
>>
>> if (Md5Final (Md5Ctx, ChapResponse)) {
>> Status = EFI_SUCCESS;
>> }
>>
>> Exit:
>> FreePool (Md5Ctx);
>> 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 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];
>>
>> Status = EFI_SUCCESS;
>>
>> SecretSize = (UINT32) AsciiStrLen (AuthData->AuthConfig->ReverseCHAPSecret);
>> Status = IScsiCHAPCalculateResponse (
>> AuthData->OutIdentifier,
>> AuthData->AuthConfig->ReverseCHAPSecret,
>> @@ -177,187 +182,211 @@ IScsiCHAPOnRspReceived (
>> //
>> NetbufQueCopy (&Conn->RspQue, 0, Len, Data);
>>
>> //
>> // Build the key-value list from the data segment of the Login Response.
>> //
>> KeyValueList = IScsiBuildKeyValueList ((CHAR8 *) Data, Len);
>> if (KeyValueList == NULL) {
>> Status = EFI_OUT_OF_RESOURCES;
>> goto ON_EXIT;
>> }
>>
>> Status = EFI_PROTOCOL_ERROR;
>>
>> switch (Conn->AuthStep) {
>> case ISCSI_AUTH_INITIAL:
>> //
>> // The first Login Response.
>> //
>> - Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_TARGET_PORTAL_GROUP_TAG);
>> + Value = IScsiGetValueByKeyFromList (
>> + KeyValueList,
>> + ISCSI_KEY_TARGET_PORTAL_GROUP_TAG
>> + );
>> if (Value == NULL) {
>> goto ON_EXIT;
>> }
>>
>> Result = IScsiNetNtoi (Value);
>> if (Result > 0xFFFF) {
>> goto ON_EXIT;
>> }
>>
>> Session->TargetPortalGroupTag = (UINT16) Result;
>>
>> - Value = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_AUTH_METHOD);
>> + Value = IScsiGetValueByKeyFromList (
>> + KeyValueList,
>> + ISCSI_KEY_AUTH_METHOD
>> + );
>> if (Value == NULL) {
>> goto ON_EXIT;
>> }
>> //
>> - // Initiator mandates CHAP authentication but target replies without "CHAP", or
>> - // initiator suggets "None" but target replies with some kind of auth method.
>> + // Initiator mandates CHAP authentication but target replies without
>> + // "CHAP", or initiator suggets "None" but target replies with some kind of
>> + // auth method.
>> //
>> if (Session->AuthType == ISCSI_AUTH_TYPE_NONE) {
>> if (AsciiStrCmp (Value, ISCSI_KEY_VALUE_NONE) != 0) {
>> goto ON_EXIT;
>> }
>> } else if (Session->AuthType == ISCSI_AUTH_TYPE_CHAP) {
>> if (AsciiStrCmp (Value, ISCSI_AUTH_METHOD_CHAP) != 0) {
>> goto ON_EXIT;
>> }
>> } else {
>> goto ON_EXIT;
>> }
>>
>> //
>> // 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);
>> + Value = IScsiGetValueByKeyFromList (
>> + KeyValueList,
>> + ISCSI_KEY_CHAP_ALGORITHM
>> + );
>> if (Value == NULL) {
>> goto ON_EXIT;
>> }
>>
>> Algorithm = IScsiNetNtoi (Value);
>> if (Algorithm != ISCSI_CHAP_ALGORITHM_MD5) {
>> //
>> // Unsupported algorithm is chosen by target.
>> //
>> goto ON_EXIT;
>> }
>>
>> - Identifier = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_IDENTIFIER);
>> + Identifier = IScsiGetValueByKeyFromList (
>> + KeyValueList,
>> + ISCSI_KEY_CHAP_IDENTIFIER
>> + );
>> if (Identifier == NULL) {
>> goto ON_EXIT;
>> }
>>
>> - Challenge = IScsiGetValueByKeyFromList (KeyValueList, ISCSI_KEY_CHAP_CHALLENGE);
>> + Challenge = IScsiGetValueByKeyFromList (
>> + KeyValueList,
>> + ISCSI_KEY_CHAP_CHALLENGE
>> + );
>> if (Challenge == NULL) {
>> goto ON_EXIT;
>> }
>> //
>> // Process the CHAP identifier and CHAP Challenge from Target.
>> // Calculate Response value.
>> //
>> Result = IScsiNetNtoi (Identifier);
>> if (Result > 0xFF) {
>> goto ON_EXIT;
>> }
>>
>> AuthData->InIdentifier = (UINT32) Result;
>> AuthData->InChallengeLength = ISCSI_CHAP_AUTH_MAX_LEN;
>> - IScsiHexToBin ((UINT8 *) AuthData->InChallenge, &AuthData->InChallengeLength, Challenge);
>> + IScsiHexToBin (
>> + (UINT8 *) AuthData->InChallenge,
>> + &AuthData->InChallengeLength,
>> + Challenge
>> + );
>> Status = IScsiCHAPCalculateResponse (
>> AuthData->InIdentifier,
>> AuthData->AuthConfig->CHAPSecret,
>> (UINT32) AsciiStrLen (AuthData->AuthConfig->CHAPSecret),
>> AuthData->InChallenge,
>> AuthData->InChallengeLength,
>> AuthData->CHAPResponse
>> );
>>
>> //
>> // Transit to next step.
>> //
>> Conn->AuthStep = ISCSI_CHAP_STEP_THREE;
>> break;
>>
>> case ISCSI_CHAP_STEP_THREE:
>> //
>> // One way CHAP authentication and the target would like to
>> // authenticate us.
>> //
>> 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);
>> + Response = IScsiGetValueByKeyFromList (
>> + KeyValueList,
>> + ISCSI_KEY_CHAP_RESPONSE
>> + );
>> if (Response == NULL) {
>> goto ON_EXIT;
>> }
>>
>> RspLen = ISCSI_CHAP_RSP_LEN;
>> IScsiHexToBin (TargetRsp, &RspLen, Response);
>>
>> //
>> // Check the CHAP Name and Response replied by Target.
>> //
>> Status = IScsiCHAPAuthTarget (AuthData, TargetRsp);
>> break;
>>
>> default:
>> break;
>> }
>>
>> ON_EXIT:
>>
>> if (KeyValueList != NULL) {
>> IScsiFreeKeyValueList (KeyValueList);
>> }
>>
>> FreePool (Data);
>>
>> return Status;
>> }
>>
>>
>> /**
>> 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.
>> + 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
>> )
>> {
>> EFI_STATUS Status;
>> ISCSI_SESSION *Session;
>> ISCSI_LOGIN_REQUEST *LoginReq;
>> ISCSI_CHAP_AUTH_DATA *AuthData;
>> CHAR8 *Value;
>> CHAR8 ValueStr[256];
>> CHAR8 *Response;
>> UINT32 RspLen;
>> CHAR8 *Challenge;
>> @@ -376,95 +405,114 @@ IScsiCHAPToSendReq (
>> RspLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
>> Response = AllocateZeroPool (RspLen);
>> if (Response == NULL) {
>> return EFI_OUT_OF_RESOURCES;
>> }
>>
>> ChallengeLen = 2 * ISCSI_CHAP_RSP_LEN + 3;
>> Challenge = AllocateZeroPool (ChallengeLen);
>> 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_INITIATOR_NAME,
>> + mPrivate->InitiatorName
>> + );
>> IScsiAddKeyValuePair (Pdu, ISCSI_KEY_SESSION_TYPE, "Normal");
>> IScsiAddKeyValuePair (
>> Pdu,
>> ISCSI_KEY_TARGET_NAME,
>> 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.
>> + // 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);
>>
>> 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);
>> + IScsiAddKeyValuePair (
>> + Pdu,
>> + ISCSI_KEY_CHAP_NAME,
>> + (CHAR8 *) &AuthData->AuthConfig->CHAPName
>> + );
>> //
>> // CHAP_R=<R>
>> //
>> - IScsiBinToHex ((UINT8 *) AuthData->CHAPResponse, ISCSI_CHAP_RSP_LEN, Response, &RspLen);
>> + IScsiBinToHex (
>> + (UINT8 *) AuthData->CHAPResponse,
>> + ISCSI_CHAP_RSP_LEN,
>> + Response,
>> + &RspLen
>> + );
>> IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_RESPONSE, Response);
>>
>> if (AuthData->AuthConfig->CHAPType == ISCSI_CHAP_MUTUAL) {
>> //
>> // CHAP_I=<I>
>> //
>> IScsiGenRandom ((UINT8 *) &AuthData->OutIdentifier, 1);
>> AsciiSPrint (ValueStr, sizeof (ValueStr), "%d", AuthData->OutIdentifier);
>> IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_IDENTIFIER, ValueStr);
>> //
>> // CHAP_C=<C>
>> //
>> IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN);
>> AuthData->OutChallengeLength = ISCSI_CHAP_RSP_LEN;
>> - IScsiBinToHex ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN, Challenge, &ChallengeLen);
>> + IScsiBinToHex (
>> + (UINT8 *) AuthData->OutChallenge,
>> + ISCSI_CHAP_RSP_LEN,
>> + Challenge,
>> + &ChallengeLen
>> + );
>> IScsiAddKeyValuePair (Pdu, ISCSI_KEY_CHAP_CHALLENGE, Challenge);
>>
>> Conn->AuthStep = ISCSI_CHAP_STEP_FOUR;
>> }
>> //
>> // Set the stage transition flag.
>> //
>> ISCSI_SET_FLAG (LoginReq, ISCSI_LOGIN_REQ_PDU_FLAG_TRANSIT);
>> break;
>>
>> default:
>> Status = EFI_PROTOCOL_ERROR;
>> break;
>> }
>>
>> FreePool (Response);
>> FreePool (Challenge);
>>
>> return Status;
>> --
>> 2.19.1.3.g30247aa5d201
>>
>>
>>
>>
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters
2021-06-11 11:39 ` Maciej Rabeda
@ 2021-06-22 10:39 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2021-06-22 10:39 UTC (permalink / raw)
To: Rabeda, Maciej, devel, ray.ni
Cc: Wu, Jiaxin, Philippe Mathieu-Daudé, Fu, Siyuan
On 06/11/21 13:39, Rabeda, Maciej wrote:
> I second Ray on the question!
> I can only assume it is vi without syntax coloring... :)
>
> On 11-Jun-21 13:37, Ni, Ray wrote:
>> No objection on the patch. Just curious what editor you are using :)
I'll quote myself from [1]:
"""
[...] my eyesight isn't the greatest, I totally depend on two code
windows being shown side by side, and I *also* can't work with multiple
monitors (I've tried it, I just can't). So... one monitor, mid-size
fonts, two columns of text --> 80 chars per column.
"""
If lines are longer than 80 characters, I either can't place two source
code windows side by side, or I must shrink the font so much that I
can't read it comfortably.
It's also not helped by a larger monitor: I use 22" and 24" regularly
(for different things), and 24" is already my limit. I wouldn't be
helped by 27" or wider -- that's about the same situation as two
monitors. I just can't work efficiently if I need to turn my head
constantly, whenever my gaze shifts. The whole point of having two
source code windows side by side is to compare two code snippets or even
*two patches*, with as few and as non-intrusive saccades as possible.
In a way, it's about making context switches cheap, and my eyesight is a
limiting factor.
Thanks,
Laszlo
[1] https://edk2.groups.io/g/devel/message/74657
http://mid.mail-archive.com/2232673b-69db-43ca-7c93-347b3d4fa62f@redhat.com
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-06-22 10:40 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-06-08 12:12 [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 01/10] NetworkPkg/IScsiDxe: wrap IScsiCHAP source files to 80 characters Laszlo Ersek
2021-06-11 11:37 ` [edk2-devel] " Ni, Ray
2021-06-11 11:39 ` Maciej Rabeda
2021-06-22 10:39 ` Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 02/10] NetworkPkg/IScsiDxe: simplify "ISCSI_CHAP_AUTH_DATA.InChallenge" size Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 03/10] NetworkPkg/IScsiDxe: clean up "ISCSI_CHAP_AUTH_DATA.OutChallengeLength" Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 04/10] NetworkPkg/IScsiDxe: clean up library class dependencies Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 05/10] NetworkPkg/IScsiDxe: fix potential integer overflow in IScsiBinToHex() Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 06/10] NetworkPkg/IScsiDxe: assert that IScsiBinToHex() always succeeds Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 07/10] NetworkPkg/IScsiDxe: reformat IScsiHexToBin() leading comment block Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 08/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() hex parsing Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 09/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() buffer overflow Laszlo Ersek
2021-06-08 12:12 ` [PUBLIC edk2 PATCH v2 10/10] NetworkPkg/IScsiDxe: check IScsiHexToBin() return values Laszlo Ersek
2021-06-09 17:33 ` [edk2-devel] [PUBLIC edk2 PATCH v2 00/10] NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox