Summary

A guest inside a VirtualBox VM using the virtio-net network adapter can trigger an intra-object out-of-bounds write in src/VBox/Devices/Network/DevVirtioNet.cpp to cause a denial-of-service or escape the hypervisor and compromise the host.

Severity

High - An attacker with high privileges in the guest can cause a denial-of-service or escape the hypervisor and compromise the host.

Proof of Concept

The following function handles a VIRTIONET_CTRL_VLAN control command which fetches a 16bit uVlanId value from the guest:

static uint8_t virtioNetR3CtrlVlan(PVIRTIONET pThis, PVIRTIONET_CTRL_HDR_T pCtrlPktHdr, PVIRTQBUF pVirtqBuf)
{
    LogFunc(("[%s] Processing CTRL VLAN command\n", pThis->szInst));

    uint16_t uVlanId;
    size_t cbRemaining = pVirtqBuf->cbPhysSend - sizeof(*pCtrlPktHdr);

    AssertMsgReturn(cbRemaining > sizeof(uVlanId),
        ("DESC chain too small for VIRTIONET_CTRL_VLAN cmd processing"), VIRTIONET_ERROR);

    /* Fetch VLAN ID from guest buffer */
    virtioCoreR3VirtqBufDrain(&pThis->Virtio, pVirtqBuf, &uVlanId, sizeof(uVlanId));

    AssertMsgReturn(uVlanId > VIRTIONET_MAX_VLAN_ID,
        ("%s VLAN ID out of range (VLAN ID=%u)\n", pThis->szInst, uVlanId), VIRTIONET_ERROR);

    LogFunc(("[%s] uCommand=%u VLAN ID=%u\n", pThis->szInst, pCtrlPktHdr->uCmd, uVlanId));

    switch (pCtrlPktHdr->uCmd)
    {
        case VIRTIONET_CTRL_VLAN_ADD:
            ASMBitSet(pThis->aVlanFilter, uVlanId);
            break;
        case VIRTIONET_CTRL_VLAN_DEL:
            ASMBitClear(pThis->aVlanFilter, uVlanId);
            break;
        default:
            LogRelFunc(("Unrecognized VLAN subcommand in CTRL pkt from guest\n"));
            return VIRTIONET_ERROR;
    }
    return VIRTIONET_OK;
}

However, the condition used in AssertMsgReturn(uVlanId > VIRTIONET_MAX_VLAN_ID) check is wrong. Instead, it should be AssertMsgReturn(uVlanId < VIRTIONET_MAX_VLAN_ID). Due to this confusion, this function always returns error unless an invalid uVlanId is given. This has severe consequences, as the uVlanId is used as an index in the pThis->aVlanFilter bitmap.

There are additional bugs in the code:

• The function virtioCoreR3VirtqBufDrain() called in virtioNetR3Ctrl() already decreases pVirtqBuf->cbPhysSend, hence the pVirtqBuf->cbPhysSend - sizeof(*pCtrlPktHdr) calculation is wrong. It should be pVirtqBuf->cbPhysSend.
• The check cbRemaining > sizeof(cVirtqPairs) is off-by-one, it should be cbRemaining >= sizeof(cVirtqPairs).

Timeline

Date reported: 08/15/2023
Date fixed: 10/17/2023
Date disclosed: 11/16/2023