Check for presence of unexpected entries in the ZIP64 extra field

[Its-Just-Nans]

Hello,

I may have found an issue

EDIT: READ THE MR COMMENT #28 (comment)

PER appnote https://pkware.cachefly.net/webdocs/casestudies/APPNOTE.TXT

The order of the fields in the zip64 extended
information record is fixed, but the fields MUST
only appear if the corresponding Local or Central
directory record field is set to 0xFFFF or 0xFFFFFFFF.

So the previous fields are mandatory since the order is defined

example:

if we use zip64_uncompressed_size AND zip64_compressed_size we should have in the block

• zip64_uncompressed_size
• zip64_compressed_size

if we use only zip64_compressed_size we should have in the block

• zip64_uncompressed_size
• zip64_compressed_size

if we use only zip64_compressed_size we should have in the block

• zip64_uncompressed_size
• zip64_compressed_size

if we use only zip64_offset_local_dir we should have in the block

• zip64_uncompressed_size
• zip64_compressed_size
• zip64_offset_local_dir

[Its-Just-Nans]

I'm not understanding what's the goal of $assumeAllFieldsPresent

[pmqs]

Hey @Its-Just-Nans

your analysis is fine. apart from missing the start disk field.

4.4.13 disk number start: (2 bytes)

   The number of the disk on which this file begins.  If an 
   archive is in ZIP64 format and the value in this field is 
   0xFFFF, the size will be in the corresponding 4 byte zip64 
   extended information extra field.

That is what a application that creates zip files should do to keep in step with the APPNOTE.txt

But...

zipdetails is not an application that creates zip files. It's purpose is to display the metadata in a zip file, regardless of whether it conforms to the APPNOTE spec. There are a lot of badly formed zip files out in the wild that do crazy non-standard things. The handling of ZIP64 is one of the areas where there is a lot of variation.

The $assumeAllFieldsPresent variable is used to record the fact that the ZIP64 field is 28 bytes long. That's the maximum length it can be, and so (probably) means the ZIP64 field contains all four of the allowable fields. That is regardless of whether the values of the equivalent fields in the central/local header are 0xFFFF or not. The presence of $assumeAllFieldsPresent means the code will blindly decode the ZIP64 extra field value for uncompressed size, compressed size, offset value and start disk number.

I should probably get it to output a warning message if the ZIP64 fields contains unexpected value -- will add that to my TODO list.

[Its-Just-Nans]

your analysis is fine. apart from missing the start disk field.

Yes but it's not mandatory, so if I don't use it, it's not present !

I should probably get it to output a warning message if the ZIP64 fields contains unexpected value -- will add that to my TODO list.

Okay thanks for the feedback I updated my MR

[pmqs]

Looking at the change you've added. You've got most of the way there.

The patch needs to be applied to the develop branch -- main is for releases only.

Your change only prints an info message when full32 is true, so the actual bytes from the ZIP64 filed don't get printed at all. I expect that is why the test automation failed.

For example, in this section of code from your patch it either outputs the uncompressed size or it outputs an info message.

        my $data = substr $zip64Extended, 0, 8, "";
        my $value = unpack "Q<", $data;
        if (full32 $entry->std_compressedSize)
        {
            $entry->uncompressedSize($value);
            out2 $data, "Uncompressed Size", Value_Q($entry->uncompressedSize)
                if $display;
        }
        else
        {
            info $data, "Uncompressed Size " . Value_V($value) . " is not used since uncompressedSize is not set to 0xFFFFFFFF"
                if $display;
        }

That is not the way it should work. zipdetails outputs the complete metadata stored in the zip file, regardless of whether it is correct. If there is an issue with a particular field, an info/warning message can be output as well.

The logic needs to be more like this (untested)

        my $data = substr $zip64Extended, 0, 8, "";
        my $value = unpack "Q<", $data;
        
        $entry->uncompressedSize($value);
        out2 $data, "Uncompressed Size", Value_Q($entry->uncompressedSize);
    
        if (! full32 $entry->std_compressedSize)
        {
            info $data, "Uncompressed Size  should not be present in the ZIP64 extra field. Corresponding   uncompressedSize from local/central header is not set to 0xFFFFFFFF, value is  ". Value_V($value) 
                if $display;
        }

You can update the change if you like, or I can fix it up and merge into my dev copy.

[Its-Just-Nans]

I updated my MR

Also another question

• zipdetails/bin/zipdetails

  Lines 4180 to 4181 in 84e84cf

  	# Zip64 in local header should not have localHeaderOffset or disk number
  	# but some zip files do

Where does this came from ?

[pmqs]

I updated my MR

Will take a look later but after a superficial look I don't see anything amiss.

Also another question

• zipdetails/bin/zipdetails

  Lines 4180 to 4181 in 84e84cf

  	# Zip64 in local header should not have localHeaderOffset or disk number
  	# but some zip files do

Where does this came from ?

Can't remember offhand. There are a lot of crazy zip files that do crazy things. I'll see if I can find an example.

[pmqs]

This change needs to be made against the develop branch. I can always copy the patch & apply manually if needed.

[Its-Just-Nans]

This change needs to be made against the develop branch. I can always copy the patch & apply manually if needed.

It is no ?

[pmqs]

This change needs to be made against the develop branch. I can always copy the patch & apply manually if needed.

It is no ?

Sorry, you are correct. It is going into the develop branch.

[pmqs]

Also another question

• zipdetails/bin/zipdetails

  Lines 4180 to 4181 in 84e84cf

  	# Zip64 in local header should not have localHeaderOffset or disk number
  	# but some zip files do

Where does this came from ?

Here is an example zip file that fully populates the ZIP64 extra field in the Local header. -- test_zip64.zip. It originated in this ticket ananthakumaran/zstream#18

0000 LOCAL HEADER #1       04034B50 (67324752)
0004 Extract Zip Spec      2D (45) '4.5'
0005 Extract OS            00 (0) 'MS-DOS'
0006 General Purpose Flag  0808 (2056)
     [Bit  3]              1 'Streamed'
     [Bit 11]              1 'Language Encoding'
0008 Compression Method    0000 (0) 'Stored'
000A Modification Time     56385E3D (1446534717) 'Tue Jan 24 11:49:58 2023'
000E CRC                   00000000 (0)
0012 Compressed Size       00000000 (0)
0016 Uncompressed Size     00000000 (0)
001A Filename Length       000B (11)
001C Extra Length          0020 (32)
001E Filename              'test/10.txt'
0029 Extra ID #1           0001 (1) 'ZIP64'
002B   Length              001C (28)
002D   Uncompressed Size   0000000000000000 (0)
0035   Compressed Size     0000000000000000 (0)
003D   Offset to Local Dir 0000000000000000 (0)
0045   Disk Number         00000000 (0)
0049 PAYLOAD               123456789.

0053 DATA DESCRIPTOR       08074B50 (134695760)
0057 CRC                   E0117757 (3759241047)
005B Compressed Size       000000000000000A (10)
0063 Uncompressed Size     000000000000000A (10)

The values are all zero because this is a streaming zip file. This is from the General Purpose Bitflag

    Bit 3: If this bit is set, the fields crc-32, compressed 
           size and uncompressed size are set to zero in the 
           local header.  The correct values are put in the 
           data descriptor immediately following the compressed
           data.  (Note: PKZIP version 2.04g for DOS only 
           recognizes this bit for method 8 compression, newer 
           versions of PKZIP recognize this bit for any 
           compression method.)

The presence of streaming will need to be checked when working with the local header

if (! full32 $entry->std_compressedSize && ! $entry->streamed)

I'll sort that while I'm fixing the failed tests.

[Its-Just-Nans]

Here is an example zip file that fully populates the ZIP64 extra field in the Local header. -- test_zip64.zip. It originated in this ticket ananthakumaran/zstream#18

Okay thanks for the example. But for me the comment is still incorrect since it's inside the local header function but the comment relates to the data descriptor (which only have crc-32, compressed size and uncompressed size)

no?