Summary

This was originally reported at https://bugzilla.tianocore.org/show_bug.cgi?id=4207.

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.

Details

iSCSI login responses are processed by the IScsiProcessLoginRsp() function. Below, the PDU's data segment is located, and is then passed to IScsiUpdateTargetAddress():

EFI_STATUS
IScsiProcessLoginRsp (
  IN OUT ISCSI_CONNECTION  *Conn,
  IN OUT NET_BUF           *Pdu
  )
{
  ...
  ISCSI_LOGIN_RESPONSE  *LoginRsp;
  ...
  UINT8                 *DataSeg;
  UINT32                DataSegLen;
  ...
  LoginRsp  = (ISCSI_LOGIN_RESPONSE *) NetbufGetByte (Pdu, 0, NULL);
  ...
  DataSegLen = ISCSI_GET_DATASEG_LEN (LoginRsp);
  if (DataSegLen != 0) { 
    DataSeg = NetbufGetByte (Pdu, sizeof (ISCSI_LOGIN_RESPONSE), NULL);
  } else {
    DataSeg = NULL;
  }

  switch (LoginRsp->StatusClass) {
  ...
  case ISCSI_LOGIN_STATUS_REDIRECTION:
    ... 
    Status = IScsiUpdateTargetAddress (Session, (CHAR8 *) DataSeg, DataSegLen);
    ...

Within IScsiUpdateTargetAddress(), we see that it calls the IScsiBuildKeyValueList() function. This function is used to parse the data segment in order to build a list of key-value pairs:

EFI_STATUS
IScsiUpdateTargetAddress (
  IN OUT ISCSI_SESSION         *Session,
  IN     CHAR8                 *Data,
  IN     UINT32                Len
  )
{
  LIST_ENTRY                   *KeyValueList;
  ...
  KeyValueList = IScsiBuildKeyValueList (Data, Len);
  ...

Then, in IScsiBuildKeyValueList() we discover the vulnerability in how the key-value pairs are parsed. Under normal circumstances, each pair is delimited by a NULL byte, and keys are separated from values with the "=" character. This would appear as:

key1=val1\0key2=val2\0

However, imagine a malformed input packet that ends with the "=" character, as in:

key1=val1\0key2=val2\0key3=

In this situation, the Data pointer would be advanced beyond the end of the data segment, and Len would be decremented beyond 0 and would wrap to a large positive 32-bit integer. This is revealed by the highlighted lines below.

LIST_ENTRY *
IScsiBuildKeyValueList (
  IN CHAR8  *Data,
  IN UINT32 Len
  )
{
  LIST_ENTRY            *ListHead;
  ISCSI_KEY_VALUE_PAIR  *KeyValuePair;

  ListHead = AllocatePool (sizeof (LIST_ENTRY));
  ...
  InitializeListHead (ListHead);

  while (Len > 0) {
    KeyValuePair = AllocatePool (sizeof (ISCSI_KEY_VALUE_PAIR));
    ...
    InitializeListHead (&KeyValuePair->List);
    KeyValuePair->Key = Data;

    while ((Len > 0) && (*Data != '=')) {
      Len--;
      Data++;
    }

    // NCC: if the data segment ends with a "=" character these lines will 
    // advance the Data pointer too far and will decrement Len beyond zero.
    if (*Data == '=') {
      *Data = '\0';
      Data++;
      Len--;
    } else {
      FreePool (KeyValuePair);
      goto ON_ERROR;
    }

    KeyValuePair->Value = Data;
    InsertTailList (ListHead, &KeyValuePair->List);;

    Data += AsciiStrLen (KeyValuePair->Value) + 1;
    Len -= (UINT32) AsciiStrLen (KeyValuePair->Value) + 1;
  }

  return ListHead;

EDK2/NetworkPkg/IScsiDxe/IScsiProto.c

On the subsequent iteration of the while-loop, Len will be very large, which will cause the loop to iterate through an excessive amount of memory past the end of Data, as it continues to search for "=" characters. Each time it encounters such a character, it will overwrite it with a "\0" byte, corrupting memory. Although, such a memory corruption primitive is not likely to allow an attacker to achieve code execution.

Impact

This bug was found by manual code review, not by testing a specific target device. Uncertain about exploitability.

Mitigation release plan

Patch files are available now via https://bugzilla.tianocore.org/show_bug.cgi?id=4207. Patch will be integrated for a future EDK2 release.