[PING] PoC for JDK-4347142: Need method to set Password protection to Zip entries
Yasumasa Suenaga
yasuenag at gmail.com
Wed Feb 10 14:34:06 UTC 2016
Hi Sherman,
I've refactored a patch for this enhancement:
http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.04/
1. I changed ZipCryption and implementation class to package private.
2. Encryption / Decryption key is allowed passphrase string.
3. I added passphrase and validation methods to ZipEntry.
I would like to hear your comment.
Thanks,
Yasumasa
On 2016/02/01 18:23, KUBOTA Yuji wrote:
> Hi Sherman and all,
>
> Could you please let know your thought and the past case about AES?
>
> Thanks,
> Yuji
>
>
> 2016-01-08 0:01 GMT+09:00 KUBOTA Yuji <kubota.yuji at gmail.com>:
>> Hi Sherman,
>>
>> Thank you for sharing!
>>
>> 2016-01-07 4:04 GMT+09:00 Xueming Shen <xueming.shen at oracle.com>:
>>> The reason that I'm not convinced that we really need a public interface of
>>> ZipCryption here
>>> is I don't know how useful/helpful/likely it would be going forward that
>>> someone might really
>>> use this interface to implement other encryption(s), especially the pkware
>>> proprietary one,
>>> I doubt it might be not that straightforward.
>>
>> In this proposal, we aim to support "traditional" because most people need it
>> in secure environment. BTW, could you please share the reason why you did
>> not support WinZip AES? Do you have a plan to support in the future?
>>
>> If you can share the reason, we want to decide the way of implementation with
>> consideration for your information. I think we can implement by two
>> way as below.
>>
>> 1. Implementing by reference to
>> http://cr.openjdk.java.net/~sherman/zipmisc/ZipFile.java
>> This is good simply API. If we need to implement other encryption(s),
>> try to refactor it.
>>
>> 2. Implementing with a package private interface of ZipCryption for next step.
>> This has two problems as your advice.
>>
>> We agree with that the "encryption" and "compression" should be
>> separated logically.
>> However, current implementation compress the encrypted data, and buffering it.
>> It is too tightly-coupled, so we need refactoring to separate the
>> managing buffer
>> processing and the stream processing of InflaterInputStream /
>> DeflaterOutputStream.
>>
>> About "push back the bytes belong to next entry", we think
>> InflaterInputStream.originBuf
>> of our PoC do not required the needed info. Do this implements have problem?
>>
>> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.00/src/java.base/share/classes/java/util/zip/InflaterInputStream.java.cdiff.html
>>
>> Thanks,
>> Yuji
>>
>>> In fact I did have a draft implementation that supports WinZip AES about 5-6
>>> years ago :-)
>>> (which also supports compression methods bzip and lzma, btw) Here is the
>>> top class, It appears
>>> a general interface might not be that helpful and it might be ideal to
>>> simply implement it inside
>>> the JDK, as what is proposed here, when it's really desired.
>>>
>>> http://cr.openjdk.java.net/~sherman/zipmisc/ZipFile.java
>>>
>>> It is a ZipFile based implementation, so it does not have the headache that
>>> ZipInputStream has,
>>> such as to push back the bytes belong to next entry, since the loc might not
>>> have the needed
>>> info regarding the size/csize in stream mode.
>>>
>>> From abstract point of view. The "encryption" and "compression" are
>>> different layers, it would
>>> be ideal to have them in separate classes logically, instead of mixing the
>>> encryption into
>>> compression. Sure, it might be convenient and probably may have better
>>> performance to mix
>>> them in certain use scenario, but the "encryption" should never appear in
>>> the public interface
>>> of those compression classes. Package private interface should be fine, if
>>> have to.
>>>
>>> -Sherman
>>>
>>>
>>>>
>>>> 2016-01-06 7:10 GMT+09:00 Xueming Shen <xueming.shen at oracle.com>:
>>>>>
>>>>> it appears that instead of adding "password" specific method to these
>>>>> classes directly, it might be more appropriate to extend the ZipEntry
>>>>> class
>>>>> for such "password" functionality. For example, with a pair of new
>>>>> methods
>>>>>
>>>>> boolean ZipEntry.isTraditionalEncryption().
>>>>> void ZipEntry.setTraditionalEncryption(String password);
>>>>
>>>> Thanks advice, I agree. We should re-design the API to extend the
>>>> ZipEntry class.
>>>>
>>>>> The encryption support should/can be added naturally/smoothly with
>>>>> ZipFile.getInputStream(e), ZipInputstream and
>>>>> ZipOutputStream.putNextEntry(e),
>>>>> with no extra new method in these two classes. The implementation checks
>>>>> the flag (bit0, no bit 6) first and then verifies the password, as an
>>>>> implementation details.
>>>>
>>>> Agree. For this proposal, we aim to support only traditional
>>>> encryption. So I think we should also check bit 6.
>>>>
>>>>> For ZipFile and ZipInputStream, we can add note to the api doc to force
>>>>> the
>>>>> invoker to check if the returned ZipEntry indicates it's an encrypted
>>>>> entry.
>>>>> If yes, it must to set the appropriate password to the returned ZipEntry
>>>>> via
>>>>> ZipEntry.setTraditionalEncryption(password); before reading any byte from
>>>>> the input stream.
>>>>
>>>> Yes, we have to add note the flow of codes to the JavaDoc.
>>>>
>>>>> Again, we should not have any "encryption" related public field/method in
>>>>> DeflaterOutputStream/InflaterInputStream. Ideally these two classes
>>>>> really
>>>>> should not be aware of "encryption" at all.
>>>>
>>>> Agree, but I think we might be faced technical difficulty about a
>>>> processing between zlib and the internal buffer of InflaterInputStream
>>>> / DeflaterOutputStream. Please give us time to implement.
>>>>
>>>>> -Sherman
>>>>
>>>> Thanks,
>>>> Yuji
>>>>
>>>>
>>>>> On 01/04/2016 06:26 AM, KUBOTA Yuji wrote:
>>>>>>
>>>>>> Hi Sherman and all,
>>>>>>
>>>>>> Happy new year to everyone!
>>>>>>
>>>>>> Please let know your feedback about this proposal. :-)
>>>>>>
>>>>>> Thanks,
>>>>>> Yuji
>>>>>>
>>>>>> 2015-12-21 22:38 GMT+09:00 KUBOTA Yuji<kubota.yuji at gmail.com>:
>>>>>>>
>>>>>>> Hi Sherman,
>>>>>>>
>>>>>>> 2015-12-20 16:35 GMT+09:00 Xueming Shen<xueming.shen at oracle.com>:
>>>>>>>>
>>>>>>>> It is no longer necessary to touch the native code (zip_util.c/h)
>>>>>>>> after
>>>>>>>> the
>>>>>>>> native ZipFile implementation has been moved up to the java level.
>>>>>>>> Those
>>>>>>>> native code are for vm access only now, which I dont think care about
>>>>>>>> the
>>>>>>>> password support at all.
>>>>>>>
>>>>>>> Thanks for your information. We do not take care the native.
>>>>>>>
>>>>>>> I discussed with Yasumasa, and our thought is as below.
>>>>>>>
>>>>>>>> (1) what's the benefit of exposing the public interface ZipCryption?
>>>>>>>> the
>>>>>>>> real
>>>>>>>> question is whether or not this interface is good enough for other
>>>>>>>> encryption
>>>>>>>> implementation to plugin their implementation to support the
>>>>>>>> ZipFile/Input/
>>>>>>>> OutputStream to their encryption spec.
>>>>>>>
>>>>>>> We aimed that the public interface ZipCryption supports the
>>>>>>> extensibillity for other encrypt engine. The JDK core libs developers
>>>>>>> have to implementation ZipyCryption only. If not provide, the JDK
>>>>>>> developers must implement ZipStream/Entry by JDK API to design the
>>>>>>> data structure of entry.
>>>>>>> If you want to use binary key data such as PKI, you can implement new
>>>>>>> encrypt/decrypt engine by ZipCryption interface.
>>>>>>> So we think we should provide this interface to be clearly how to
>>>>>>> implement a new engine, e.g., cipher algorithm, cipher strength and
>>>>>>> converting the header, etc.
>>>>>>>
>>>>>>>> (2) it seems like it might be possible to hide most of the
>>>>>>>> implementation
>>>>>>>> and only expose the "String password" (instead of the ZipCryption) as
>>>>>>>> the
>>>>>>>> public interface to support the "traditional" encryption. This depends
>>>>>>>> on the
>>>>>>>> result of (1) though.
>>>>>>>
>>>>>>> Thanks for your clues. We think the string password at first. However,
>>>>>>> we should also create a new binary interface given we support PKI in
>>>>>>> the future.
>>>>>>>
>>>>>>>> (3) I'm concerned of pushing ZipCryption into
>>>>>>>> InflaterInputStream/DeflaterOutputStream.
>>>>>>>> It might be worth considering to replace the ZipCryption
>>>>>>>> implementation
>>>>>>>> with
>>>>>>>> a pair of FilterOutput/InputStream. It would be easy and reasonable to
>>>>>>>> use
>>>>>>>> the FilterOutputStream for the ZipOutputStream and the
>>>>>>>> FilterInputStream
>>>>>>>> for the
>>>>>>>> ZipFile. The PushbackInputStream in ZipInputStream might be an issue
>>>>>>>> ...
>>>>>>>
>>>>>>> Thanks for your clues, too. Honestly speaking, we think the current
>>>>>>> zip implementation may break the data when used PushbackInputStream
>>>>>>> for the following reasons.
>>>>>>>
>>>>>>> * PushbackInputStream uses an unique internal buffer for re-read
>>>>>>> operation.
>>>>>>> * But, InflaterInputStream provide date to Inflater per reads and
>>>>>>> buffer by JNI (zlib).
>>>>>>> * So we think PushbackInputStream is poor compatibility with
>>>>>>> InflaterInputStream.
>>>>>>>
>>>>>>> We generally use InputStream through ZipEntry#getInputStream(). We do
>>>>>>> not touch FileInputStream for reading ZIP data. If we call unread()
>>>>>>> when we use PushbackInputStream as reading ZIP archive, we guess that
>>>>>>> it will break the reading data.
>>>>>>> So, our approach do not affect the PushbackInputStream.
>>>>>>> What do you think about this?
>>>>>>>
>>>>>>>> (4) It seems the ZipOutputStream only supports the "stream based"
>>>>>>>> password, while
>>>>>>>> the ZipInputStream supports the "entry based" password. Do we really
>>>>>>>> need
>>>>>>>> "entry based" support here?
>>>>>>>
>>>>>>> As your suggestion, we should support "entry based". We will start to
>>>>>>> implement "entry based" after this discussion is closed.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Yuji
>>>>>>>
>>>>>>>> On 12/17/15, 9:45 PM, Yasumasa Suenaga wrote:
>>>>>>>>>
>>>>>>>>> Hi Jason,
>>>>>>>>>
>>>>>>>>> Thank you for your comment.
>>>>>>>>> I've fixed it in new webrev:
>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.03/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> Yasumasa
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2015/12/17 0:33, Jason Mehrens wrote:
>>>>>>>>>>
>>>>>>>>>> The null check of 'entry' at line 351 of ZipFile.getInputStream is
>>>>>>>>>> in
>>>>>>>>>> conflict with line 350 and 348.
>>>>>>>>>>
>>>>>>>>>> ________________________________________
>>>>>>>>>> From: core-libs-dev<core-libs-dev-bounces at openjdk.java.net> on
>>>>>>>>>> behalf
>>>>>>>>>> of
>>>>>>>>>> Yasumasa Suenaga<yasuenag at gmail.com>
>>>>>>>>>> Sent: Wednesday, December 16, 2015 8:47 AM
>>>>>>>>>> To: Sergey Bylokhov; Xueming Shen
>>>>>>>>>> Cc: core-libs-dev at openjdk.java.net
>>>>>>>>>> Subject: Re: [PING] PoC for JDK-4347142: Need method to set Password
>>>>>>>>>> protection to Zip entries
>>>>>>>>>>
>>>>>>>>>> Hi Sergey,
>>>>>>>>>>
>>>>>>>>>> Thank you for your comment.
>>>>>>>>>>
>>>>>>>>>> I added that description in new webrev:
>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.02/
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 2015/12/16 22:19, Sergey Bylokhov wrote:
>>>>>>>>>>>
>>>>>>>>>>> Should the new methods describe how they will work in case of null
>>>>>>>>>>> params?
>>>>>>>>>>>
>>>>>>>>>>> On 16/12/15 16:04, Yasumasa Suenaga wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> I adapted this enhancement after JDK-8145260:
>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.01/
>>>>>>>>>>>>
>>>>>>>>>>>> Could you review it?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2015/12/12 21:23, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Sherman,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Our proposal is affected by JDK-8142508.
>>>>>>>>>>>>> We have to change ZipFile.java and and ZipFile.c .
>>>>>>>>>>>>> Thus we will create a new webrev for current (after 8142508)
>>>>>>>>>>>>> jdk9/dev
>>>>>>>>>>>>> repos.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Do you have any comments about current webrev?
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.00/
>>>>>>>>>>>>>
>>>>>>>>>>>>> If you have comments, we will fix them in new webrev.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2015/12/03 16:51, KUBOTA Yuji wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Sherman,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks for your quick response :)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I aimed to implement the "traditional" at this proposal by the
>>>>>>>>>>>>>> below
>>>>>>>>>>>>>> reasons.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> * We want to prepare API for encrypted zip files at first.
>>>>>>>>>>>>>> * Many people use the "traditional" in problem-free scope
>>>>>>>>>>>>>> like a
>>>>>>>>>>>>>> temporary file.
>>>>>>>>>>>>>> * We do not know which implementation of the "stronger" is
>>>>>>>>>>>>>> best
>>>>>>>>>>>>>> for
>>>>>>>>>>>>>> openjdk.
>>>>>>>>>>>>>> * PKWare claims that they have patents about the
>>>>>>>>>>>>>> "stronger"
>>>>>>>>>>>>>> on
>>>>>>>>>>>>>> Zip[1].
>>>>>>>>>>>>>> * OTOH, WinZip have the alternative implementation of the
>>>>>>>>>>>>>> "stronger" [2][3].
>>>>>>>>>>>>>> * Instead, we prepared the extensibility by ZipCryption
>>>>>>>>>>>>>> interface
>>>>>>>>>>>>>> to
>>>>>>>>>>>>>> implement other encrypt engine, such as the AES based.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thus, I think this PoC should support the "traditional" only.
>>>>>>>>>>>>>> In the future, anyone who want to implement the "stronger" can
>>>>>>>>>>>>>> easily
>>>>>>>>>>>>>> add their code by virtue of this proposal.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>> https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.3.TXT
>>>>>>>>>>>>>> (1.4 Permitted Use& 7.0 Strong Encryption
>>>>>>>>>>>>>> Specification)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [2]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> https://en.wikipedia.org/wiki/Zip_(file_format)#Strong_encryption_controversy
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> [3] http://www.winzip.com/aes_info.htm
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> Yuji
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> 2015-12-03 12:29 GMT+09:00 Xueming
>>>>>>>>>>>>>> Shen<xueming.shen at oracle.com>:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Yuji,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I will take a look at your PoC. Might need some time and even
>>>>>>>>>>>>>>> bring
>>>>>>>>>>>>>>> in the
>>>>>>>>>>>>>>> security guy
>>>>>>>>>>>>>>> to evaluate the proposal. It seems like you are only interested
>>>>>>>>>>>>>>> in
>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>> "traditional PKWare
>>>>>>>>>>>>>>> decryption", which is, based on the wiki, "known to be
>>>>>>>>>>>>>>> seriously
>>>>>>>>>>>>>>> flawed, and
>>>>>>>>>>>>>>> in particular
>>>>>>>>>>>>>>> is vulnerable to known-plaintext attacks":-) Any request to
>>>>>>>>>>>>>>> support
>>>>>>>>>>>>>>> "stronger" encryption
>>>>>>>>>>>>>>> mechanism, such as the AES based?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Regards,
>>>>>>>>>>>>>>> Sherman
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 12/2/15 6:48 PM, KUBOTA Yuji wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> We need reviewer(s) for this PoC.
>>>>>>>>>>>>>>>> Could you please review this proposal and PoC ?
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> Yuji
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> 2015-11-26 13:22 GMT+09:00 KUBOTA Yuji<kubota.yuji at gmail.com>:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> * Sorry for my mistake. I re-post this mail because I sent
>>>>>>>>>>>>>>>>> before
>>>>>>>>>>>>>>>>> get
>>>>>>>>>>>>>>>>> a response of subscription confirmation of core-libs-dev.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Our customers have to handle password-protected zip files.
>>>>>>>>>>>>>>>>> However,
>>>>>>>>>>>>>>>>> Java SE does not provide the APIs to handle it yet, so we
>>>>>>>>>>>>>>>>> must
>>>>>>>>>>>>>>>>> use
>>>>>>>>>>>>>>>>> third party library so far.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Recently, we found JDK-4347142: "Need method to set Password
>>>>>>>>>>>>>>>>> protection to Zip entries", and we tried to implement it.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> The current zlib in JDK is completely unaffected by this
>>>>>>>>>>>>>>>>> proposal.
>>>>>>>>>>>>>>>>> The
>>>>>>>>>>>>>>>>> traditional zip encryption encrypts a data after it is has
>>>>>>>>>>>>>>>>> been
>>>>>>>>>>>>>>>>> compressed by zlib.[1] So we do NOT need to change existing
>>>>>>>>>>>>>>>>> zlib
>>>>>>>>>>>>>>>>> implementation.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> We've created PoC and uploaded it as webrev:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.00/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Test code is as below. This code will let you know
>>>>>>>>>>>>>>>>> how
>>>>>>>>>>>>>>>>> this
>>>>>>>>>>>>>>>>> PoC
>>>>>>>>>>>>>>>>> works.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-4347142/webrev.00/Test.java
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> In NTT, a Japanese telecommunications company. We are
>>>>>>>>>>>>>>>>> providing
>>>>>>>>>>>>>>>>> many
>>>>>>>>>>>>>>>>> enterprise systems to customers. Some of them, we need to
>>>>>>>>>>>>>>>>> implement to
>>>>>>>>>>>>>>>>> handle password-protected zip file. I guess that this
>>>>>>>>>>>>>>>>> proposal
>>>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>>>> desired for many developers and users.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I'm working together with Yasumasa Suenaga, jdk9 committer
>>>>>>>>>>>>>>>>> (ysuenaga).
>>>>>>>>>>>>>>>>> We want to implement it if this proposal accepted.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> [1]:
>>>>>>>>>>>>>>>>> https://pkware.cachefly.net/webdocs/APPNOTE/APPNOTE-6.3.3.TXT
>>>>>>>>>>>>>>>>> (6.0 Traditional PKWARE Encryption)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>> Yuji
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>
More information about the core-libs-dev
mailing list