Re: [Bug-cpio] [PATCH] fix 1-byte out-of-bounds write

[Kristýna Streitová]

Hello Pavel,

Thank you for your patch. This approach definitely suits upstream better
than the minimalistic maintenance patch.

I tested it with the reproducers I sent you in my previous mail and with
the reproducer for CVE-2016-2037 and it works fine for me.

>> Basically, there are 2 problems:
>>
>> 1) file_hdr.c_namesize is not defined for tar and ustar archive formats. 
>> Therefore we have a random memory here and that's the reason why it's 
>> reproducible randomly.
> 
> It means that we should initialize c_namesize I guess, even though this is
> different issue.

That's a good idea. Maybe it would be worth to initialize other
variables that are not initialized yet. For tar archives it's for
example c_ino, c_dev_maj and c_dev_min variables. But that's definitely
a different issue.

>> 2) file_hdr.c_name uses static memory [1] for tar and ustar archive 
>> formats. Therefore 'xrealloc(file_hdr.c_name, 2)' call causes a dump for 
>> these archive types as we are trying to realloc static memory.
> 
> Yes, that's something which complicated fixing and reading of the code, so
> the attached patch should rather use buffer on heap.  It is still not very
> clean approach, and not as trivial patch as before - but the code should be
> a bit cleaner I hope.

Thank you for it. I have just small comments on your patch:

1) It seems that the first hunk slightly changes the logic.
Before your patch, the 'is_tar_filename_too_long()' function was called
for tar and ustar archives only but now it's called for all archive types.

As this function checks whether the filename is too long to fit in a tar
header, it should be called for tar/ustar archives only, I believe.

2) It's just a small thing but it seems that you forgot to use your new
'cpio_set_c_name()' function for 'file_hdr.c_name = CPIO_TRAILER_NAME'
in the copyout.c file.

Best regards,
Kristyna Streitova

signature.asc
Description: OpenPGP digital signature