Syslinux - Feb 2016 - [PATCH 4/5] installers: fix a possible buffer overflow when looking for LDLINUX_MAGIC

If the ldlinux being processed is garbage, the search for LDLINUX_MAGIC 
will overflow its buffer - fix that.
I did encounter this issue in Rufus as, due to notorious 
incompatibilities between different versions of ldlinux.sys and the 
com32's residing on an ISO, we download a version specific ldlinux.sys 
from our server... which may get trashed if the user sits behind one of 
these corporate firewalls that modifies the download payload and 
replaces it with something like "You are not authorized to download this 
file"...
-------------- next part --------------

On 2/24/2016 08:02, Pete Batard via Syslinux wrote:
> If the ldlinux being processed is garbage, the search for 
> LDLINUX_MAGIC will overflow its buffer - fix that.
> I did encounter this issue in Rufus as, due to notorious 
> incompatibilities between different versions of ldlinux.sys and the 
> com32's residing on an ISO, we download a version specific ldlinux.sys 
> from our server... which may get trashed if the user sits behind one 
> of these corporate firewalls that modifies the download payload and 
> replaces it with something like "You are not authorized to download 
> this file"...
$0.02:
- Casting to a uintptr_t is ugly (and not C89, not that Syslinux cares 
about that)
- The boot_image + boot_image_len calculation is loop invariant, so some 
kind of boot_image_end or wpe pointer before the loop might be nicer.  
Unfortunately, the 8-bit boot_image_len (bytes) versus the 32-bit wp 
stride complicates things

Maybe something like:

   const uint32_t * wpe = (const uint32_t *) boot_image + boot_len / 
sizeof *wpe;

where the 'for' check could then: ... && wp < wpe

We all know that it wouldn't actually bite us, but technically, if 
boot_image's memory's size isn't evenly divisible by 32 bits, the
wp++
can land us in a position where a read access would yield undefined 
behaviour.  Maybe boot_image memory always will be a multiple of 4; I 
don't know.

With the proposed patch's uintptr_t stuff, if the magic isn't found:

              * <------- boot_image_len dictates the final byte
            * <-------- wp is less than boot_image_len
00001111222233XXYYYY <- XX are out-of-bound bytes
                 * <---- Where wp is when the loop breaks
               * <------ As far as any kind of pointer should point
             * <-------- When the loop should break, as 33XX can't 
contain the magic

Fingers crossed that my math is working.

--
- Synthetel
http://Shao Miller

On 2016.03.07 03:27, Shao Miller via Syslinux wrote:
> - Casting to a uintptr_t is ugly (and not C89, not that Syslinux cares
> about that)
Yeah, I'd have liked to avoid that too, but some compilers will complain 
about pointer arithmetic logic, unless you specifically use uintptr_t.

But, considering your other very valid point, let me see if I can work 
something better here, that could eliminate this cast.
> With the proposed patch's uintptr_t stuff, if the magic isn't
found:
>
>               * <------- boot_image_len dictates the final byte
>             * <-------- wp is less than boot_image_len
> 00001111222233XXYYYY <- XX are out-of-bound bytes
>                  * <---- Where wp is when the loop breaks
>                * <------ As far as any kind of pointer should point
>              * <-------- When the loop should break, as 33XX can't
> contain the magic
You're right. This overflow prevention still does not prevent all 
possible overflows, so I need to review my patch.

I'll try to work something better, either today or tomorrow, and submit 
a new patch. Thanks for pointing the issue.

Regards,

/Pete