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...@intel.com> Cc: Maciej Rabeda <maciej.rab...@linux.intel.com> Cc: Philippe Mathieu-Daudé <phi...@redhat.com> Cc: Siyuan Fu <siyuan...@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356 Signed-off-by: Laszlo Ersek <ler...@redhat.com> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> Reviewed-by: Maciej Rabeda <maciej.rab...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76201): https://edk2.groups.io/g/devel/message/76201 Mute This Topic: https://groups.io/mt/83394109/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
