From fca7e61fa3ba21cbf6e89d75b23fea03af5d517e Mon Sep 17 00:00:00 2001 From: Laszlo Ersek Date: Tue, 8 Jun 2021 14:12:52 +0200 Subject: [PATCH 03/10] NetworkPkg/IScsiDxe: clean up "ISCSI_CHAP_AUTH_DATA.OutChallengeLength" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RH-Author: Laszlo Ersek RH-MergeRequest: 5: NetworkPkg/IScsiDxe: fix IScsiHexToBin() security and functionality bugs [rhel-8.5.0, post-rebase] RH-Commit: [3/10] cc7118399f64979f2d81fe9fc381ed22c3815f9e RH-Bugzilla: 1956408 RH-Acked-by: Philippe Mathieu-Daudé 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 Cc: Maciej Rabeda Cc: Philippe Mathieu-Daud Cc: Siyuan Fu Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3356 Signed-off-by: Laszlo Ersek Reviewed-by: Philippe Mathieu-Daud Reviewed-by: Maciej Rabeda Message-Id: <20210608121259.32451-4-lersek@redhat.com> (cherry picked from commit 95616b866187b00355042953efa5c198df07250f) --- NetworkPkg/IScsiDxe/IScsiCHAP.c | 3 +-- NetworkPkg/IScsiDxe/IScsiCHAP.h | 9 ++++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.c b/NetworkPkg/IScsiDxe/IScsiCHAP.c index df3c2eb120..9e192ce292 100644 --- a/NetworkPkg/IScsiDxe/IScsiCHAP.c +++ b/NetworkPkg/IScsiDxe/IScsiCHAP.c @@ -122,7 +122,7 @@ IScsiCHAPAuthTarget ( AuthData->AuthConfig->ReverseCHAPSecret, SecretSize, AuthData->OutChallenge, - AuthData->OutChallengeLength, + ISCSI_CHAP_RSP_LEN, // ChallengeLength VerifyRsp ); @@ -490,7 +490,6 @@ IScsiCHAPToSendReq ( // CHAP_C= // IScsiGenRandom ((UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN); - AuthData->OutChallengeLength = ISCSI_CHAP_RSP_LEN; IScsiBinToHex ( (UINT8 *) AuthData->OutChallenge, ISCSI_CHAP_RSP_LEN, diff --git a/NetworkPkg/IScsiDxe/IScsiCHAP.h b/NetworkPkg/IScsiDxe/IScsiCHAP.h index 1fc1d96ea3..35d5d6ec29 100644 --- a/NetworkPkg/IScsiDxe/IScsiCHAP.h +++ b/NetworkPkg/IScsiDxe/IScsiCHAP.h @@ -19,7 +19,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #define ISCSI_CHAP_ALGORITHM_MD5 5 -#define ISCSI_CHAP_AUTH_MAX_LEN 1024 /// /// MD5_HASHSIZE /// @@ -59,9 +58,13 @@ typedef struct _ISCSI_CHAP_AUTH_DATA { // // 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; /** -- 2.27.0