RFR: 8313765: Invalid CEN header (invalid zip64 extra data field size) [v2]

[Volker Simonis]

On Mon, 14 Aug 2023 17:33:30 GMT, Lance Andersen <lancea at 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 with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Merge branch 'master' into extraHeaders-JDK-8313765
>  - Minor comment word smithing
>  - Fix for JDK-8313765

Hi Lance,
In general it looks good, but I have some suggestion that I think could slightly improve the patch.

src/java.base/share/classes/java/util/zip/ZipFile.java line 1320:

> 1318:                 zerror("Invalid CEN header (invalid zip64 extra data field size)");
> 1319:             }
> 1320:             // if ZIP64_EXTID blocksize == 0, validate csize and size

If you put this block in front of the call to `isZip64ExtBlockSizeValid()` we don't have to handle the `blockSize == 0` case in `isZip64ExtBlockSizeValid()`.

This will also make the following comment true again:

            // Note we do not need to check blockSize is >= 8 as
            // we know its length is at least 8 from the call to
            // isZip64ExtBlockSizeValid()

src/java.base/share/classes/java/util/zip/ZipFile.java line 1364:

> 1362:              * As the fields must appear in order, the block size indicates which
> 1363:              * fields to expect:
> 1364:              *  0 - May be written out by Ant and Apache Commons Compress Library

I don't like that `isZip64ExtBlockSizeValid()` still accepts `0` as *valid* input. I think we should fully handle the zero case in `checkZip64ExtraFieldValues()` (also see my comments there).

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 3099:

> 3097:                         throw new ZipException("Invalid CEN header (invalid zip64 extra data field size)");
> 3098:                     }
> 3099:                     // if ZIP64_EXTID blocksize == 0, validate csize, size and

Same here. Just put this block before the call to isZip64ExtBlockSizeValid() than you don't have to handle the `sz == 0` case there.

src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java line 3216:

> 3214:              */
> 3215:             return switch(blockSize) {
> 3216:                 case 0, 8, 16, 24, 28 -> true;

Don't need to handle the zero case here if you rearrange the code in `readExtra()` as suggested above.

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

PR Review: https://git.openjdk.org/jdk/pull/15273#pullrequestreview-1577293869
PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293755895
PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293751880
PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293761720
PR Review Comment: https://git.openjdk.org/jdk/pull/15273#discussion_r1293762891