On Mon, 14 Aug 2023 18:49:25 GMT, Lance Andersen <lan...@openjdk.org> wrote:

>> This PR  updates the extra field validation added as part of 
>> [JDK-8302483](https://bugs.openjdk.org/browse/JDK-8302483)  to deal with 
>> issues seen with 3rd party tools/libraries where a ZipException may be 
>> encountered when opening select APK, ZIP or JAR files.  Please see refer to 
>> the links provided at the end the description for the more information ::
>> 
>>> ZipException: Invalid CEN header (invalid zip64 extra data field size)
>> 
>> 1. Extra field includes padding :
>> 
>> 
>> ----------------#1--------------------
>> [Central Directory Header]
>>   0x3374: Signature    : 0x02014b50
>>   0x3378: Created Zip Spec :    0xa [1.0]
>>   0x3379: Created OS   :    0x0 [MS-DOS]
>>   0x337a: VerMadeby    :    0xa [0, 1.0]
>>   0x337b: VerExtract   :    0xa [1.0]
>>   0x337c: Flag      :   0x800
>>   0x337e: Method     :    0x0 [STORED]
>>   0x3380: Last Mod Time  : 0x385ca437 [Thu Feb 28 20:33:46 EST 2008]
>>   0x3384: CRC       : 0x694c6952
>>   0x3388: Compressed Size :   0x624
>>   0x338c: Uncompressed Size:   0x624
>>   0x3390: Name Length   :   0x1b
>>  0x3392: Extra Length  :    0x7
>>              [tag=0xcafe, sz=0, data= ]
>>                              ->[tag=cafe, size=0]
>>   0x3394: Comment Length :    0x0
>>   0x3396: Disk Start   :    0x0
>>   0x3398: Attrs      :    0x0
>>   0x339a: AttrsEx     :    0x0
>>   0x339e: Loc Header Offset:    0x0
>>   0x33a2: File Name    : res/drawable/size_48x48.jpg
>> 
>> 
>> The extra field tag of `0xcafe` has its data size set to `0`.  and the extra 
>> length is `7`.   It is expected that you can use the  tag's data size to 
>> traverse the extra fields.
>> 
>> 
>> 2. The [BND tool](https://github.com/bndtools/bnd) added [problematic data 
>> to the extra field](https://issues.apache.org/jira/browse/FELIX-6622):
>> 
>> 
>> ----------------#359--------------------
>> [Central Directory Header]
>>    0x600b4: Signature        : 0x02014b50
>>    0x600b8: Created Zip Spec :       0x14 [2.0]
>>    0x600b9: Created OS       :        0x0 [MS-DOS]
>>    0x600ba: VerMadeby        :       0x14 [0, 2.0]
>>    0x600bb: VerExtract       :       0x14 [2.0]
>>    0x600bc: Flag             :      0x808
>>    0x600be: Method           :        0x8 [DEFLATED]
>>    0x600c0: Last Mod Time    : 0x2e418983 [Sat Feb 01 17:12:06 EST 2003]
>>    0x600c4: CRC              : 0xd8f689cb
>>    0x600c8: Compressed Size  :      0x23e
>>    0x600cc: Uncompressed Size:      0x392
>>    0x600d0: Name Length      :       0x20
>>    0x600d2: Extra Length     :        0x8
>>              [tag=0xbfef, sz=61373, data=        
>>   0x600d4: Comment Length   ...
>
> Lance Andersen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add newline

> > > I am not understanding your point. There is a specific order for the 
> > > Zip64 fields based on which fields have the Magic value. the spec does 
> > > also not suggest that an empty Zip64 extra field can be written to the 
> > > CEN when there is a Zip64 with data written to the LOC.
> > 
> > 
> > Yes, there is a specific order of fields that should be stored in the 
> > extended block if some of the data in the "body" is negative. But as you 
> > pointed out in this case the empty block or block bigger than necessary to 
> > store the size/csize/locoff is not prohibited by the spec. For example, 
> > take a look at the code in the 
> > [ZipEntry](https://github.com/openjdk/jdk/blob/c132176b932dd136d5c4314e08ac97d0fee7ba4d/src/java.base/share/classes/java/util/zip/ZipEntry.java#L553)
> >  where we accept any size of that block and just checked that it has 
> > required data in it.
> > If you disagree then point to the part of the spec which blocks such sizes.
> 
> This is how it is implemented by the "unzip" 
> https://github.com/madler/zlib/blob/04f42ceca40f73e2978b50e93806c2a18c1281fc/contrib/minizip/unzip.c#L1035C68-L1035C76
>  , the dataSize is accepted as is.

> 4.6.2 Third-party Extra Fields MUST include a Header ID using
>    the format defined in the section of this document 
>    titled Extensible Data Fields (section 4.5).
> 
>    The Data Size field indicates the size of the following
>    data block. Programs can use this value to skip to the
>    next header block, passing over any data blocks that are
>    not of interest.

Zip -T would also report errors with a BND modified jar:

 zip -T bad.jar

>  net/n3/nanoxml/CDATAReader.class bad extra-field entry:
>        EF block length (61373 bytes) exceeds remaining EF data (4 bytes)
>  test of bad.jar FAILED
> 
> zip error: Zip file invalid, could not spawn unzip, or wrong unzip (original 
> files unmodified)
> 

zipdetails would also fail with the above jar

-------------

PR Comment: https://git.openjdk.org/jdk/pull/15273#issuecomment-1678012762

Reply via email to