Bug 1665371 (CVE-2019-3813)

Does look like it can change program control flow assuming checks can be bypassed, an attacker can control the heap information past the end of the buffer, etc. memslot_get_virt looks to return chunks of memory back to callers that are then cast into different structs and used. Thus, information leaks may be possible, lots of other super-undefined behavior could potentially occur (i.e. code execution)

```
   │1426        qxl = (QXLCursor *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);                                            │
  >│1427        if (error) {                                                                                                                 │
   │1428            return false;                                                                                                            │
   │1429        }                                                                                                                            │
   │1430                                                                                                                                     │
   │1431        red->header.unique     = qxl->header.unique;                                                                                 │
   │1432        red->header.type       = qxl->header.type;                                                                                   │
```

another example of a use of memslot_get_virt:

```
   │1463    bool red_get_cursor_cmd(RedMemSlotInfo *slots, int group_id,                                                                     │
   │1464                            RedCursorCmd *red, QXLPHYSICAL addr)                                                                     │
   │1465    {                                                                                                                                │
   │1466        QXLCursorCmd *qxl;                                                                                                           │
   │1467        int error;                                                                                                                   │
   │1468                                                                                                                                     │
  >│1469        qxl = (QXLCursorCmd *)memslot_get_virt(slots, addr, sizeof(*qxl), group_id, &error);                                         │
   │1470        if (error) {                                                                                                                 │
   │1471            return false;                                                                                                            │
   │1472        }                                                                                                                            │
   │1473        red->release_info_ext.info      = &qxl->release_info;                                                                        │
   │1474        red->release_info_ext.group_id  = group_id;  
```

For an attacker to control the output memslot_get_virt, it looks like they need at least the following to occur:
1. Control group_id or the QXLPHYSICAL addr. Reporter indicated group_id may be unlikely, but QXLPHYSICAL might be possible.
2. If addr is controlled, set higher byte to slot_id such that "return addr >> info->memslot_id_shift" returns a number equal to info->num_memslots.
3. Control the heap such that the now out-of-bounds read of &info->mem_slots[group_id][slot_id] allows some control of the structure.
Attacker would then potentially be able to modify the following struct:

```c
typedef struct MemSlot {
    int generation;
    unsigned long virt_start_addr;
    unsigned long virt_end_addr;
    long address_delta;
} MemSlot;
```
4. slot->generation would need to be set to what memslot_get_generation returns, which I think is just more information encoded into the addr.
5. h_virt could then potentially be set to anything via:
```
h_virt += slot->address_delta;
```
6. memslot_validate_virt would need to be bypassed. With full control of the heap, I believe that wouldn't pose a problem. See the following line:
```c
if (virt < slot->virt_start_addr || (virt + add_size) > slot->virt_end_addr) {
```

After all of these hypotheticals, an attacker could then return a controlled void* memslot_validate_virt to callers, who would then start processing those chunks of memory leading to further decay.

That's my argument for potential (not easy, but possible) code execution. Highly likely I missed something, let me know. :)

Also looks like an attacker may be able to change the group_id via modifying the QXL guest driver.

```c
void spiceqxl_display_monitors_config(qxl_screen_t *qxl)
{
    spice_qxl_monitors_config_async(&qxl->display_sin, physical_address(qxl, qxl->monitors_config, 0),
                                    MEMSLOT_GROUP, 0);
}
```
MEMSLOT_GROUP is hardcoded to 0. So I believe an attacker can control this? Untested.

Original commit -- a70110c4e50aad99de7a844bb78eb868768e7841 on Sat Nov 21 22:42:38 2009 +0200

```bash
git tag --contains a70110c4e50aad99de7a844bb78eb868768e7841 | sort -V
0.5.2
0.5.3
0.6.0
0.6.1
0.6.2
0.6.3
0.6.4
0.7.0
0.7.1
0.7.2
0.7.3
0.8.0
0.8.1
0.8.2
0.8.3
0.9.0
0.9.1
0.10.0
0.10.1
0.11.0
0.11.3
0.12.0
v0.12.0
v0.12.2
v0.12.3
v0.12.4
v0.12.5
v0.12.6
v0.12.8
v0.13.0
v0.13.1
v0.13.2
v0.13.3
v0.13.90
v0.13.91
v0.14.0
v0.14.1
```

Thus, RHEL6 (spice-server 12.4) also impacted via the get_virt function in red_memslots.c.

(In reply to Scott Gayou from comment #6)
> Also looks like an attacker may be able to change the group_id via modifying
> the QXL guest driver.
> 
> ```c
> void spiceqxl_display_monitors_config(qxl_screen_t *qxl)
> {
>     spice_qxl_monitors_config_async(&qxl->display_sin, physical_address(qxl,
> qxl->monitors_config, 0),
>                                     MEMSLOT_GROUP, 0);
> }
> ```
> MEMSLOT_GROUP is hardcoded to 0. So I believe an attacker can control this?
> Untested.

spiceqxl_*.c is not part of the QXL driver which runs on the guest, it's a xorg driver which will directly link with spice-server, and will allow to export a xorg display over SPICE. I'm fairly confident group_id is never obtained from guest data, but is always hardcoded to a valid value by QEMU.

Acknowledgments:

Name: Christophe Fergeau (Red Hat)

Created spice tracking bugs for this issue:

Affects: fedora-all [bug 1670158]

Unembargoed.

This issue has been addressed in the following products:

  Red Hat Enterprise Linux 7

Via RHSA-2019:0231 https://access.redhat.com/errata/RHSA-2019:0231

This issue has been addressed in the following products:

  Red Hat Enterprise Linux 6

Via RHSA-2019:0232 https://access.redhat.com/errata/RHSA-2019:0232

Upstream Patch: https://gitlab.freedesktop.org/spice/spice/commit/a4a16ac42d2f19a17e36556546aa94d5cd83745f

This issue has been addressed in the following products:

  Red Hat Virtualization 4 for Red Hat Enterprise Linux 7

Via RHSA-2019:0457 https://access.redhat.com/errata/RHSA-2019:0457