[PATCH] hw/core/loader: Fix possible crash in rom_copy()

[PATCH] hw/core/loader: Fix possible crash in rom_copy()

[Thomas Huth]

Both, "rom->addr" and "addr" are derived from the binary image
that can be loaded with the "-kernel" paramer. The code in
rom_copy() then calculates:

    d = dest + (rom->addr - addr);

and uses "d" as destination in a memcpy() some lines later. Now with
bad kernel images, it is possible that rom->addr is smaller than addr,
thus "rom->addr - addr" gets negative and the memcpy() then tries to
copy contents from the image to a bad memory location. In the best case,
this just crashes QEMU, in the worst case, this could maybe be used to
inject code from the kernel image into the QEMU binary, so we better fix
it with an additional sanity check here.

Cc: qemu-stable@nongnu.org
Reported-by: Guangming Liu
Buglink: https://bugs.launchpad.net/qemu/+bug/1844635
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/core/loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 0d60219364..5099f27dc8 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1281,7 +1281,7 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
         if (rom->addr + rom->romsize < addr) {
             continue;
         }
-        if (rom->addr > end) {
+        if (rom->addr > end || rom->addr < addr) {
             break;
         }
 
-- 
2.18.1

Re: [PATCH] hw/core/loader: Fix possible crash in rom_copy()

[Michael S. Tsirkin]

On Wed, Sep 25, 2019 at 03:03:31PM +0200, Thomas Huth wrote:
> Both, "rom->addr" and "addr" are derived from the binary image
> that can be loaded with the "-kernel" paramer. The code in
> rom_copy() then calculates:
> 
>     d = dest + (rom->addr - addr);
> 
> and uses "d" as destination in a memcpy() some lines later. Now with
> bad kernel images, it is possible that rom->addr is smaller than addr,
> thus "rom->addr - addr" gets negative and the memcpy() then tries to
> copy contents from the image to a bad memory location. In the best case,
> this just crashes QEMU, in the worst case, this could maybe be used to
> inject code from the kernel image into the QEMU binary, so we better fix
> it with an additional sanity check here.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Guangming Liu
> Buglink: https://bugs.launchpad.net/qemu/+bug/1844635
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  hw/core/loader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 0d60219364..5099f27dc8 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1281,7 +1281,7 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
>          if (rom->addr + rom->romsize < addr) {
>              continue;
>          }
> -        if (rom->addr > end) {
> +        if (rom->addr > end || rom->addr < addr) {
>              break;
>          }
>  
> -- 
> 2.18.1

Re: [PATCH] hw/core/loader: Fix possible crash in rom_copy()

[Paolo Bonzini]

On 25/09/19 15:22, Michael S. Tsirkin wrote:
>> Both, "rom->addr" and "addr" are derived from the binary image
>> that can be loaded with the "-kernel" paramer. The code in
>> rom_copy() then calculates:
>> 
>>     d = dest + (rom->addr - addr);
>> 
>> and uses "d" as destination in a memcpy() some lines later. Now with
>> bad kernel images, it is possible that rom->addr is smaller than addr,
>> thus "rom->addr - addr" gets negative and the memcpy() then tries to
>> copy contents from the image to a bad memory location. In the best case,
>> this just crashes QEMU, in the worst case, this could maybe be used to
>> inject code from the kernel image into the QEMU binary, so we better fix
>> it with an additional sanity check here.
>> 
>> Cc: qemu-stable@nongnu.org
>> Reported-by: Guangming Liu
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1844635
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Queued, thanks.

Paolo

Re: [PATCH] hw/core/loader: Fix possible crash in rom_copy()

[Philippe Mathieu-Daudé]

Hi Thomas,

On 9/25/19 3:03 PM, Thomas Huth wrote:
> Both, "rom->addr" and "addr" are derived from the binary image
> that can be loaded with the "-kernel" paramer. The code in
> rom_copy() then calculates:
> 
>     d = dest + (rom->addr - addr);
> 
> and uses "d" as destination in a memcpy() some lines later. Now with
> bad kernel images, it is possible that rom->addr is smaller than addr,
> thus "rom->addr - addr" gets negative and the memcpy() then tries to
> copy contents from the image to a bad memory location. In the best case,
> this just crashes QEMU, in the worst case, this could maybe be used to
> inject code from the kernel image into the QEMU binary, so we better fix
> it with an additional sanity check here.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Guangming Liu
> Buglink: https://bugs.launchpad.net/qemu/+bug/1844635

"This page does not exist, or you may not have permission to see it."

This seems security related. Shouldn't we open a CVE for this?
https://wiki.qemu.org/SecurityProcess#CVE_allocation

Let's say I have write access to a LAN TFTP server used by some PXE
bootloader where I can store my crafted nasty kernel, then I get this score:

https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:A/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H/E:P/RL:O/RC:C&version=3.1

CVSS Base Score:     9.6
CVSS Temporal Score: 8.6

Which seems quite high.

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/core/loader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 0d60219364..5099f27dc8 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1281,7 +1281,7 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)

$ git show 235f86ef014
Date:   Thu Nov 12 21:53:11 2009 +0100

This function is old and poorly documented.

>          if (rom->addr + rom->romsize < addr) {
>              continue;
>          }
> -        if (rom->addr > end) {
> +        if (rom->addr > end || rom->addr < addr) {

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>              break;
>          }
>  
> 

Re: [PATCH] hw/core/loader: Fix possible crash in rom_copy()

[Thomas Huth]

On 25/09/2019 22.51, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 9/25/19 3:03 PM, Thomas Huth wrote:
>> Both, "rom->addr" and "addr" are derived from the binary image
>> that can be loaded with the "-kernel" paramer. The code in
>> rom_copy() then calculates:
>>
>>     d = dest + (rom->addr - addr);
>>
>> and uses "d" as destination in a memcpy() some lines later. Now with
>> bad kernel images, it is possible that rom->addr is smaller than addr,
>> thus "rom->addr - addr" gets negative and the memcpy() then tries to
>> copy contents from the image to a bad memory location. In the best case,
>> this just crashes QEMU, in the worst case, this could maybe be used to
>> inject code from the kernel image into the QEMU binary, so we better fix
>> it with an additional sanity check here.
>>
>> Cc: qemu-stable@nongnu.org
>> Reported-by: Guangming Liu
>> Buglink: https://bugs.launchpad.net/qemu/+bug/1844635
> 
> "This page does not exist, or you may not have permission to see it."
> 
> This seems security related. Shouldn't we open a CVE for this?
> https://wiki.qemu.org/SecurityProcess#CVE_allocation

I wrote to the security team before writing the patch, so I assume a CVE
number is already on the way. I'll reply to this thread when it is
available.

 Thomas

Re: [PATCH] hw/core/loader: Fix possible crash in rom_copy()

[Thomas Huth]

On 25/09/2019 22.51, Philippe Mathieu-Daudé wrote:
[...]
> Let's say I have write access to a LAN TFTP server used by some PXE
> bootloader where I can store my crafted nasty kernel, then I get this score:
> 
> https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:A/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H/E:P/RL:O/RC:C&version=3.1
> 
> CVSS Base Score:     9.6
> CVSS Temporal Score: 8.6
> 
> Which seems quite high.

I don't think you can trigger this bug this way. If you load your kernel
via a PXE server, the ELF parsing will be done by the bootloader, won't
it? I think you can only trigger this bug here if you load your kernel
via the "-kernel" command line parameter of QEMU (or the generic-loader
device), so it's not a real guest escape, as far as I can see.

 Thomas

Re: [PATCH] hw/core/loader: Fix possible crash in rom_copy()

[Philippe Mathieu-Daudé]

On 9/26/19 7:58 AM, Thomas Huth wrote:
> On 25/09/2019 22.51, Philippe Mathieu-Daudé wrote:
> [...]
>> Let's say I have write access to a LAN TFTP server used by some PXE
>> bootloader where I can store my crafted nasty kernel, then I get this score:
>>
>> https://nvd.nist.gov/vuln-metrics/cvss/v3-calculator?vector=AV:A/AC:L/PR:N/UI:N/S:C/C:H/I:H/A:H/E:P/RL:O/RC:C&version=3.1
>>
>> CVSS Base Score:     9.6
>> CVSS Temporal Score: 8.6
>>
>> Which seems quite high.
> 
> I don't think you can trigger this bug this way. If you load your kernel
> via a PXE server, the ELF parsing will be done by the bootloader, won't
> it? I think you can only trigger this bug here if you load your kernel
> via the "-kernel" command line parameter of QEMU (or the generic-loader
> device), so it's not a real guest escape, as far as I can see.

Ah indeed you are correct. You have to use the -kernel option to set
kernel_filename. This reduce the scores:

CVSS Base Score: 8.4
CVSS Temporal Score: 7.6
Exploitability Subscore: 1.7

You could load a kernel stored on a NFS server, but it is unlikely a
production case :)