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...@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: Maciej Rabeda <maciej.rab...@linux.intel.com> Reviewed-by: Philippe Mathieu-Daudé <phi...@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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76207): https://edk2.groups.io/g/devel/message/76207 Mute This Topic: https://groups.io/mt/83394119/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-