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
