[Toybox] [landley/toybox] file segfaults (#99)

[Rob Landley]

On 07/26/2018 06:55 PM, Elliott Hughes wrote:
> rob didn't claim to be looking at this already when i asked, and since i've
> spent the afternoon on shell/utilities bugs...

Sorry, it's been crazy at work. (They use the "perpetual sprint" management fad
and I'm back in Austin this coming week so I had to try to fit 3 weeks of work
into 2.)

I've barely looked at toybox or mkroot all week. So much backlog. (Going off
caffeine a week ago didn't help, of course. :)

> so it looks like we need to protect ourselves against nonsense sizes too.

Sigh, I always assume any file I'm parsing was created by an attacking troll,
looks like I missed one.

> i'll send this patch to the mailing list...

$ file modem.b25
modem.b25: ELF 32-bit LSB  shared object, version 1 (SYSV), dynamically linked,
stripped

The ubuntu one is saying it's a .o file, but it's been linked and stripped. (Is
that even possible? I remember working out how to make busybox both an
executable and a shared library to export the zlib stuff way back when, although
I don't think anybody ever did it. But an already linked shared _object_?)

Let's see...

> @@ -160,13 +160,14 @@ static void do_elf_file(int fd)
>      } else if (sh_type == 7 /*SHT_NOTE*/) {
>        char *note = map+sh_offset;
>  
> -      if (sh_offset+sh_size>TT.len) goto bad;
> -
>        // An ELF note is a sequence of entries, each consisting of an
>        // ndhr followed by n_namesz+n_descsz bytes of data (each of those
>        // rounded up to the next 4 bytes, without this being reflected in
>        // the header byte counts themselves).
>        while (sh_size >= 3*4) { // Don't try to read a truncated entry.
> +        // Sanity check (https://github.com/landley/toybox/issues/99).
> +        if (sh_offset+sh_size>TT.len) goto bad;

Hmmm, the point of the original check is to see if sh_size will run off the end
of TT.file, and then we test there's enough for the 3 ints at the start of the
loop, then test each string field we're dereferencing, so why... Ah, integer
overflow. And those ints should be unsigned anyway.

Lemme change the type and move the code _after_ the declarations (it's a C vs
C++ thing I've taken as code style for the project).

Rob