Antoine Pelisse <apelisse@xxxxxxxxx> writes:
> On Wed, Nov 27, 2013 at 1:00 PM, Thomas Gummerer <t.gummerer@xxxxxxxxx> wrote:
>> +static int verify_hdr(void *mmap, unsigned long size)
>> +{
>> + uint32_t *filecrc;
>> + unsigned int header_size;
>> + struct cache_header *hdr;
>> + struct cache_header_v5 *hdr_v5;
>> +
>> + if (size < sizeof(struct cache_header)
>> + + sizeof (struct cache_header_v5) + 4)
>> + die("index file smaller than expected");
>> +
>> + hdr = mmap;
>> + hdr_v5 = ptr_add(mmap, sizeof(*hdr));
>> + /* Size of the header + the size of the extensionoffsets */
>> + header_size = sizeof(*hdr) + sizeof(*hdr_v5) + hdr_v5->hdr_nextension * 4;
>> + /* Initialize crc */
>> + filecrc = ptr_add(mmap, header_size);
>> + if (!check_crc32(0, hdr, header_size, ntohl(*filecrc)))
>> + return error("bad index file header crc signature");
>> + return 0;
>> +}
>
> I find it curious that we actually need a value from the header (and
> use it for pointer arithmetic) to check that the header is valid. The
> application will crash before the crc is checked if
> hdr_v5->hdr_nextensions is corrupted. Or am I missing something ?
Good catch, I'm the one that was missing something here. We still need
to use the value from the header before calculating the crc, but should
check if header_size - 4 is less than the total size of the index file.
Then even if the header is corrupted we won't read anything that is not
mmap'ed and thus won't crash.
This guard should also be included for everything else that checks the
crc checksum, as that has the same problems and the calculated place in
the file for the crc might be after the end of the file.
Thanks, will fix in the re-roll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html