Re: [Xen-devel] [PATCH v4] xen/tools: Introduce QNX IFS loader

[Oleksandr Tyshchenko <oleksandr.tyshchenko@xxxxxxxxxxxxxxx>]

Hi Ian.
Thank you for your review.

On Fri, Sep 26, 2014 at 5:37 PM, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> Oleksandr Tyshchenko writes ("[PATCH v4] xen/tools: Introduce QNX IFS 
> loader"):
>> This patch was developed according to instruction:
>> http://www.qnx.com/developers/docs/6.4.1/neutrino/building/load_process.html
> ...
>> +static int xc_dom_probe_qnx_ifs(struct xc_dom_image *dom)
>> +{
> ...
>> +    /* Performs a checksums on the startup and the OS image filesystem */
>> +    if ( (calc_checksum((uint32_t *)startup_hdr, startup_hdr->startup_size) 
>> != 0) ||
>> +         (calc_checksum((uint32_t *)startup_hdr + 
>> startup_hdr->startup_size/4,
>> +          startup_hdr->stored_size - startup_hdr->startup_size) != 0) )
>
> Suppose that the incoming image is corrupt or malicious and
> startup_header.startup_size and dom->kernel_size are both equal to
> sizeof(startup_header)+1.
>
> (By hand I count startup_header to have size 64, so assuming that's
> right, and writing things in hex:)
ok. Maybe, do you mean that (stored_size == kernel_size) instead of
(startup_size == kernel_size)?
1. If No (startup_size == kernel_size) -> I think, there is no issue.
We will not calculate checksum in this case.
Before calculating checksums we check stored_size too.
- assume that (stored_size != kernel_size):
it this case probe will fail.
- assume that (stored_size == kernel_size):
it this case probe will fail too, because stored_size must be greater
than startup_size.

2. If Yes (stored_size == kernel_size)
Let's look in to additional requirements:
- startup_size should be corrupted too so that (startup_size < stored_size)
- first checksum verification should returns success.
Is it possible ?

>
> Then the first call to calc_checksum looks like this:
>    calc_checksum( dom->kernel_blob, 0x41 )
>
> For the first 0x10 iterations calc_checksum will read successive
> uint32_t's from kernel_blob (for a total of 0x40 bytes) and reduce
> size to 0x01.
>
> The next iteration of calc_checksum will read a uint32_t from
> dom->kernel_blob+0x40.  But kernel_size==0x41 so this is a 3-byte
> buffer read overrun - a vulnerability, technically, I think.
>
> But worse happens next.  calc_checksum then has size=0x01 and does
>    size -= 4;
> leaving size with the value 0xfffffffd.  Because size is a uint32_t
> this is positive, not negative, and satisfies the test in the loop.
>
> I.e. calc_checksum will continue to iterate forever.  It will keep
> reading memory at ever increasing addresses until it hits an invalid
> address, and then crash.
>
> I'm afraid I think this is a readily exploitable denial of service
> vulnerability.
I agree with you that size should be int32_t and ((size % 4) == 0).
It is my mistake.

>
> Ian.



-- 

Oleksandr Tyshchenko | Embedded Dev
GlobalLogic
www.globallogic.com

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel