NetworkPkg/IScsiDxe: bound value length in IScsiBuildKeyValueList

IScsiBuildKeyValueList parses the data segment of a received iSCSI
login, text or CHAP response into key=value pairs. After locating '='
within the remaining length, it sets KeyValuePair->Value and calls
AsciiStrLen(Value) to measure the value before subtracting it from the
remaining length. AsciiStrLen has no length cap, and the data segment
copied from the received PDU (AllocatePool(Len) + NetbufQueCopy of the
data-segment length) is not guaranteed to be NUL-terminated.

A malicious or redirecting target can send a data segment whose final
value lacks a trailing NUL (e.g. the 3 bytes "X=Y"), so AsciiStrLen
reads past the end of the segment allocation, an attacker-controlled
out-of-bounds read. The SafeUint32Sub bound check only runs after the
over-read.

Replace AsciiStrLen(Value) with AsciiStrnLenS(Value, Len), capping the
scan to the bytes remaining from Value onward. An unterminated value
then returns Len and the existing SafeUint32Sub rejects the segment.
The single change covers all three callers (login redirect, operational
parameter negotiation and CHAP).

Signed-off-by: Syed Mohammed Nayyar <jmestwa@gmail.com>

1 files changed, 1 insertions, 1 deletions

diff --git a/NetworkPkg/IScsiDxe/IScsiProto.c b/NetworkPkg/IScsiDxe/IScsiProto.c
index 13394dbfc6..beb63ebc9c 100644
--- a/NetworkPkg/IScsiDxe/IScsiProto.c
+++ b/NetworkPkg/IScsiDxe/IScsiProto.c
@@ -1922,7 +1922,7 @@ IScsiBuildKeyValueList (
 

     KeyValuePair->Value = Data;

 

-    Status = SafeUint32Add ((UINT32)AsciiStrLen (KeyValuePair->Value), 1, &Result);

+    Status = SafeUint32Add ((UINT32)AsciiStrnLenS (KeyValuePair->Value, Len), 1, &Result);

     if (EFI_ERROR (Status)) {

       DEBUG ((DEBUG_ERROR, "%a Memory Overflow is Detected.\n", __func__));

       FreePool (KeyValuePair);