JDK-6695402: Jarsigner with multi-byte characters in class names

Sun Sep 17 19:25:44 UTC 2017

Hello Vincent

I narrowed the error down so far and suspect now that it is an effect of 
sun.security.util.ManifestDigester's constructor, lines 134 and 163-164, 
in combination with java.util.jar.Manifest.make72Safe. Manifest breaks 
multi-byte characters in manifest section names across lines and 
ManifestDigester fails to restore them correctly, which both sounds 
wrong but ManifestDigester sure is.

Just fixing it would be too tempting but then I would not know (I mean 
not know for sure with evidence from tests) if any existing 
jar-signature would still be valid and I don't want to break existing 
signatures. When I had a look at the tests specific for Manifest and 
ManifestDigester I found very little. There are many more different 
situations and corner cases and combinations thereof to cover with tests 
than it looks like at first glance so that writing tests that cover all 
relevant cases looks to me like quite an effort. I have the impression 
that test coverage was not a priority when the subjected code was 
developed or at least the tests might not have been contributed to the 
open JDK project. Hence, while I'm continuing to complete these tests I 
miss, if someone has an idea how to simplify that so that I can still 
convince at least myself that no existing signature will break or where 
possibly more testcases are that I haven't discovered so far, please let 
me know.

I'm also wondering how much performance should be taken into 
consideration. There are a few hints such as reusing byte arrays that 
suggest that it is an issue. Will a patch be rejected if it slows down 
some tests beyond a certain limit for so little added value or what 
might be the acceptance criteria here? I don't expect insuperable 
difficulties with performance but would still appreciate some general idea.

My desired or currently preferred approach to fix JDK-6695402 would be 
to let ManifestDigester any kind of use or extend Manifest in order to 
let Manifest identify the manifest sections thereby preventing the 
parsing duplicated that identifies the manifest sections.

As a new contributor I could use and would appreciate some guidance 
particularly about what kind and completeness of test coverage and 
performance tuning applies or is suggested in the context of the given 
bug. Otherwise I fear I might contribute too many tests which would have 
to be reviewed and maintained or quite some work would be for nothing if 
a patch would not satisfy performance requirements.

Regards,
Philipp

On 01.09.2017 15:20, Vincent Ryan wrote:
> That all sounds fine. Let me know when your patch is ready to submit.
> Thanks.
>
>
>> On 1 Sep 2017, at 13:15, Philipp Kunz <philipp.kunz at paratix.ch 
>> <mailto:philipp.kunz at paratix.ch>> wrote:
>>
>> Hello Vincent
>>
>> Thank you for sponsoring!
>> So far, I have become a contributor by signing the OCA which has been 
>> accepted. Dalibor Topic wrote that he can confirm and it's also here: 
>> http://www.oracle.com/technetwork/community/oca-486395.html#p -> 
>> Paratix GmbH
>> Therefore I think I have followed the steps in 
>> http://openjdk.java.net/contribute/ at least the ones before actual 
>> patch submission.
>>
>> Currently, I'm working on getting the existing test cases running 
>> locally. Unfortunately, I started with jdk 9 and switched to 10 now. 
>> I figured these commands run at least the relevant tests with 9 and 
>> hope this also applies to 10:
>> make run-test-tier1
>> make run-test TEST="jdk/test"
>> they report errors and failures but it hasn't been completed and 
>> released which might explain it. If I run the tests I assume relevant 
>> for my patch
>> make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner 
>> jtreg:jdk/test/java/util/jar"
>> then it reports zero errors and failures which may be a good starting 
>> point.
>> Do you think that sounds reasonable or do you have another suggestion 
>> how to run the tests?
>>
>> Next, I will add a test for JDK-6695402 before actually fixing it. As 
>> an example, I'll try something in the style of 
>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java. 
>> This way, I try to demonstrate the improvement.
>>
>> I guess I have identified the following line as the cause: value = 
>> new String(vb, 0, 0, vb.length);
>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
>> So I'll try to remove it first including the whole four line if block.
>>
>> Philipp
>>
>>
>> On 01.09.2017 10:00, Vincent Ryan wrote:
>>> Hello Philipp,
>>>
>>> I’m happy to sponsor your fix for JDK 10. Have you followed these 
>>> steps: http://openjdk.java.net/contribute/ ?
>>>
>>> Thanks.
>>>
>>>
>>>> On 1 Sep 2017, at 08:58, Vincent Ryan <vincent.x.ryan at oracle.com 
>>>> <mailto:vincent.x.ryan at oracle.com>> wrote:
>>>>
>>>> Moved to security-dev
>>>>
>>>>
>>>>> On 1 Sep 2017, at 08:28, Philipp Kunz <philipp.kunz at paratix.ch 
>>>>> <mailto:philipp.kunz at paratix.ch>> wrote:
>>>>>
>>>>> Hello everyone
>>>>>
>>>>> I have been developing with Java for around 17 years now and when 
>>>>> I encountered some bug I decided to attempt to fix it: 
>>>>> https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks 
>>>>> like it may not be too big a piece for a first contribution.
>>>>>
>>>>> I read through quite some guides and all kinds of documents but 
>>>>> could not yet help myself with the following questions:
>>>>>
>>>>> May I login to jira to add comments to bugs? If so, how would I 
>>>>> request or receive credentials? Or are mailing lists preferred?
>>>>>
>>>>> Another question is whether I should apply it to jdk9, but it may 
>>>>> be too late now, or to jdk10, and backporting can be considered 
>>>>> later. Probably it wouldn't even make much a difference for the 
>>>>> patch itself.
>>>>>
>>>>> One more question I have is how or where to find the sources from 
>>>>> before migration to mercurial. Because some lines of code I intend 
>>>>> to change go back farther and in the history I find only 'initial 
>>>>> commit'. With such a history I might be able better to understand 
>>>>> why it's there and prevent to make the same mistake again.
>>>>>
>>>>> I guess the appropriate mailing list for above mentioned bug is 
>>>>> security-dev. Is it correct that I can send a patch there and just 
>>>>> hope for some sponsor to pick it up? Of course I'd be glad if some 
>>>>> sponsor would contact me and maybe provide some assistance or if 
>>>>> someone would confirm that sending a patch to the mailing list is 
>>>>> the right way to find a sponsor.
>>>>>
>>>>> Philipp Kunz
>>>>
>>>
>>
>

-- 

Gruss Philipp

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

Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
philipp.kunz at paratix.ch <mailto:philipp.kunz at paratix.ch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20170917/796bc984/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: image/png
Size: 5134 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20170917/796bc984/attachment.png>