NetworkPkg/IScsiDxe:Fix for out of bound memory access for bz4207 (CVE-2024-38805)

[santhoshVenkat25]

Description

A malicious iSCSI target could reply to the iSCSI initiator with a malformed packet, causing out-of-bounds memory reads and writes. This most likely leads to a denial of service, as the write primitive should not be exploitable.

To Fix this,
In IScsiBuildKeyValueList, check if we have any data left (Len > 0) before advancing the Data pointer and reducing Len.
Avoids wrapping Len. Also Used SafeUint32SubSafeUint32Sub call to reduce the Len .

[santhoshVenkat25]

Hi, @lgao4

can you please review this commit and add this changes in to EDK202505 Tag

Thanks,
santhosh

[santhoshVenkat25]

@mdkinney

I have made changes for debug message review comments.
and force pushed the changes

[mdkinney]

The logic that manages the allocated buffers in this function is confusing. When ON_ERROR path is taken, all KeyValuePair buffers actually added to the linked list are freed. Line 1919 has to do an independent FreePool() call because the KeyValuePair allocated has not been added to the linked list yet. The new boundary check are done after KeyValuePair is inserted into the linked list, so ON_ERROR path will free the KeyValuePair being added.

It would be easier to follow if either
a) KeyValuePair is always added to the linked list before any condition check are made, and ON_ERROR does all free operations.
b) KeyValuePair is not added to the linked list until all condition checks are made and any condition check failure must free KeyValuePair buffer and then goto ON_ERROR to free all other linked list items.

[santhoshVenkat25]

@mdkinney

// Here Len must not be zero.
    // The value of Len is size of data buffer. Actually, Data is make up of strings.
    // AuthMethod=None\0TargetAlias=LIO Target\0 TargetPortalGroupTag=1\0
    // (1) Len == 0, *Data != '=' goto ON_ERROR
    // (2) *Data == '=', Len != 0 normal case.
    // (3) *Data == '=', Len == 0, Between Data and Len are mismatch, Len isn't all size of data, as error.
    if ((Len > 0) && (*Data == '=')) {
      *Data = '\0';
      Data++;
      Len--;
    } else {
      FreePool (KeyValuePair);
      goto ON_ERROR;
    }

    KeyValuePair->Value = Data;

    
    Status = SafeUint32Add (AsciiStrLen (KeyValuePair->Value), 1, &Result);
    if (EFI_ERROR (Status)) {
      DEBUG ((DEBUG_ERROR, "%a Memory Overflow is Detected.\n", __func__));
      FreePool (KeyValuePair);
      goto ON_ERROR;
    }

    Status = SafeUint32Sub (Len, Result, &Len);
    if (EFI_ERROR (Status)) {
      DEBUG ((DEBUG_ERROR, "%a Out of bound memory access Detected.\n", __func__));
      FreePool (KeyValuePair);
      goto ON_ERROR;
    }
    InsertTailList (ListHead, &KeyValuePair->List);
    Data += Result;
  }

Followed B) suggestion and made changes ,kindly review the code in comment section.

Thanks,
santhosh

[mdkinney]

Looks good to me