RFR 8217375: jarsigner breaks old signature with long lines in manifest
Weijun Wang
weijun.wang at oracle.com
Mon Jul 1 07:41:03 UTC 2019
https://cr.openjdk.java.net/~weijun/8217375/webrev.02 uploaded. There are still several trailing spaces and I've removed them.
I just ran a test job: SignTwice.java failed on Windows with `failed to clean up files after test`. Most likely a file is not closed.
The src change looks good. In fact, I am wondering if we need to support ManifestDigester.get(MF_MAIN_ATTRS, b) at all. Is it possible we miss a coding error because of it?
I'll take a look at the tests.
Thanks,
Max
> On Jun 30, 2019, at 7:38 PM, Philipp Kunz <philipp.kunz at paratix.ch> wrote:
>
> Hi John, Max, and everyone
>
> Finally found some time again for this, please excuse the delay.
>
> Even though I think adding a keyalg parameter next to genkeypair is a different topic, it should not do any harm, and I added it.
>
> I find your proposal for how to print the manifests good. I preferred to display \n as well because I would otherwise think it was not there sooner or later and confuse myself. In that context also, I intentionally did not add an echoManifest method overload with a String parameter because it could and I believe also actually would confuse filenames and manifests both represented as String. Only a few times "Utils.echoManifest(Utils.readJarManifestBytes(" occurs which might be more verbose than necessary. I have also some doubts that all manifest are actually valid UTF-8. More fine-tuning might be desired.
>
> I also found running jarsigner slow which is why I suggested adding it as a tool like jar and others in a different thread and which was not accepted then. As for PreserveRawManifestEntryAndDigest, there is use of sun.security.tools.jarsigner.Main.main or run. Using sun.security.tools.jarsigner.Main re-uses loading of the keystore while avoiding another JVM which I believe to be almost perfect apart from the API used not looking very public. Also in terms of performance I think it should be equivalent to JarSigner::sign. When the output should be caputered, there is no way without another JVM (without the jarsigner tool).
>
> DigestDontIgnoreCase.getManifestBytes could reformat the manifest: agreed and changed accordingly. Now Utils.readJarManifestBytes. Note that it does not use a Manifest like before with JarFile.getManifest.
>
> EmptyIndividualSectionName.testNameEmptyTrusted: I honestly have no clue how this can be made to work. That is at least what I remember from when I wrote it half a year ago. We might as well come eventually to the conclusion that such a test is not necessary. The idea seems to be to invoke getTrustedAttributes("") on a manifest but it is not public. Help appreciated.
>
> About the several TODOs, FIXMEs, and FIXED_8217375: The tests in the patch have to be "kind of calibrated" to make sure things don't change with the patch that should not. Ideally these tests would have existed before but the have not. The tests that make sure that some cases still work the same now also contain some changes. In a usual case one would let the tests of a patch run against the old impl and see what has failed before, then apply the main source part of the patch and see something fixed, and not touching any other test guarantees that nothing else changes. In this particular case I found that not working. It is also not possible to create an independent patch with tests only because the tests closely relate to the patch. My idea was that all the FIXMEs and TODOs are removed before actually commiting the patch, after someone has confirmed that all the tests run fine against the previous impl which should make sure certain behavior has not changed. One of the tests does not even compile without the impl changes. After everyone has agreed that there are enough tests to make sure nothing changed by accident, we can remove those FIXMEs and TODOs and have then to pay close attention that no tests are removed or changed anymore. For tracking such changes, git would be an option, but in this case I figured I could not use it myself because I thought I should create a patch against the head branch and you could not see my branches anyway but maybe someone of you can create a branch and share it with me. In any case, the result should be only one commit ending up in the head branch (tip). If that sounds complicated it's what I encountered too. If someone has a better idea how to deal with it, please let me know.
>
> backwars - backwards - changed
>
> ManifestSigester - ManifestDigester - changed
>
> att - attr - changed
>
> Paths.get - Path.of - changed
>
> MainAttributesConfused.java - It is not necessary that a file with that name exists in order to show the effect. The signature gets confused if a manifest section exists. Such a section would be created when signing a jar file that contains a file with that name. I don't remember if I actually tried it with a file but testAddManifestMainAttributesSection shows that and how the patch makes a difference. Do you think another test should be created which signs a file with that name?
>
> PreserveRawManifestEntryAndDigest.java - I hope with the improved manifest encoding (\r and \n) on the console that an output overflow does not happen anymore. I remember having tried to reduce the number of tests as much as possible and justifyable already. I don't currently see a promising approach as how to substantially simplify it, hence, any hint appreciated, or I might come back to optimization at some later point. For now I was able to simplify getExpectedJarSignerOutputUpdatedContentNotValidatedBySignerA even though I would have preferred to be able to utilize OutputAnalyzer. I think to remember that verifying a jar file by opening it in verifying mode verifies all signers and in this case there is only one valid.
>
> Thanks for offering to look into the change in OutputAnalyzer.java because alternative would include/increase copy-paste.
>
> And from the other mail:
>
> ManifestDigester.java: if (!findSection(0, pos)) throw new IllegalStateException - moved exception back to JarSigner.SignatureFile.
>
> removed redundant 696 mfCreated = true;
>
> "You added a new check. Do you mean if the signer is the same then the SF will be rewritten anyway and there is no need to retain old MANIFEST.MF bytes?" - Yes, exactly. Added isBlockOrSF around line 900. See also WasSignedByOtherSigner test.
>
> "MainAttributesConfused.java, PreserveRawManifestEntryAndDigest.java, RemoveDifferentKeyAlgBlockFile.java, and SectionNameContinuedVsLineBreak.java fail on Windows because a JarFile object is not closed. You might want to use try-with-resources on them." - Hope I got them all.
>
> About tests timing out, I haven't encountered that but of course it has to run quick enough also on other machines. Before going into too much optimization I would appreciate and prefer to either get some (however remote or informal) confirmation that the set of test cases is appropriate or to get rid of superfluous tests before making them faster. Even more important is probably before anything else if the tests currently present are sufficient or if some test case has slipped and is still missing.
>
> One last note for now concerning Compatibility test. I hope it currently demonstrates what I think it should, verify jar signatures between jdk versions while preserving behavior with default parameters. The code itself requires probably a major clean-up before it can really be merged. I tried not to change more than necessary for now but it does not look now in an as good shape as how I originally encountered it. I'm also still thinking about Max's suggestion to add another test with a jarfile commited along with the tests to check compatibility but could still not yet convince myself that this would be comprehensive enough to replace the Compatibility additions. On the other hand, these compatibility test extensions have slightly more or different scope than the remainder of the patch, I guess, and leave some room for opinions and interpretation. Ideally, compatibility would have been covered with tests before and breaking signatures with increasing the manifest line width would have been noticed earlier. In my opinion there still is no better time than now to add such tests. At least we should make us confident not to break anything again this time.
>
> Thank you very much for all the good points. I appreciate all your input and find it very valuable.
>
> Regards,
> Philipp
>
>
>
>
>
> On Fri, 2019-06-21 at 11:32 +0800, Weijun Wang wrote:
>> The tests.
>>
>> - For all:
>>
>> We are thinking about removing default value for -keyalg for "keytool -genkeypair" some day, so it will be better to add an explicit "-keyalg RSA" or "-keyalg EC" now.
>>
>> There are several tests printing out the manifest in a sequence of "[[0]=83='S', ...". While it is precise, I find it not very easy to check. How about we just print out the string format but for each "\r" we just print "\r" literally and keep printing "\n" with a new line, like this:
>>
>> Signature-Version: 1.0\r
>> Created-By: 13-internal (Oracle Corporation)\r
>> SHA-256-Digest-Manifest: Pxklci7ELW1zzSh2jZ+oz7TPR0WK2uTsAIiHar1m6Eo=\r
>> SHA-256-Digest-Manifest-Main-Attributes: ETfvdTNx3yN/ClSZz20wR0SM9ta8H7O\r
>> E7U0F4uZ9JV8=\r
>> \r
>> Name: abc\r
>> SHA-256-Digest: uTZ8UM3iJLPtl8W7+NEC/AC2auI7LP2Gu53ajj6jLgg=\r
>> \r
>>
>> If you find running jarsigner slow, is the JarSigner::sign API any better? The verify side can also usually be replaced with open the file and read everything inside unless you want to grab certain outputs, but those can also be done with JarEntry::getSigners.
>>
>> - DigestDontIgnoreCase.java:
>>
>> getManifestBytes(). This could reformat the manifest (I understand it's irrelevant in this test), you can just return
>>
>> jf.getInputStream(jf.getJarEntry(JarFile.MANIFEST_NAME)).readAllBytes()
>>
>> - EmptyIndividualSectionName.java:
>>
>> Are you going to do something around testNameEmptyTrusted()?
>>
>> - FindHeaderEndVsManifestDigesterFindFirstSection.java:
>>
>> Several TODO and FIXME.
>>
>> 52: typo: backwars-
>>
>> 76: No need to maintain FIXED_8217375 anymore
>>
>> 229: Typo: ManifestSigester
>>
>> - InsufficientSectionDelimiter.java:
>>
>> We usually abbreviate "attribute" to "attr" instead of "att".
>>
>> We usually use Path.of() instead of Paths.get() these days.
>>
>> - MainAttributesConfused.java:
>>
>> Have you tried adding a file named "Manifest-Main-Attributes" into a jar and see if it would mess up anything?
>>
>> - PreserveRawManifestEntryAndDigest.java:
>>
>> This test is running for a long time. The output is also quite big and you can see "Output overflow" inside which means if there is a failure it might not show. Is it possible to simplify it?
>>
>> getExpectedJarSignerOutputUpdatedContentNotValidatedBySignerA(). The output might change often. If you only focus on several lines, it's better to check them with OutputAnalyzer::shouldMatch***. We can always manually construct an OutputAnalyzer object.
>>
>> - DigestInput.java and FindSection.java:
>>
>> static final boolean FIXED_8217375 = true;
>>
>> Is this still needed?
>>
>> - OutputAnalyzer.java:
>>
>> This is a shared utility class and you changed the behavior. It might be correct (start matching *after* the from line) but I cannot be sure if it would break anything. I'll ask people about it.
>>
>> Thanks,
>> Max
>>
>>
>>
>>
>>
>>>
>>> On Jun 11, 2019, at 3:08 PM, Weijun Wang <
>>> weijun.wang at oracle.com
>>> > wrote:
>>>
>>> Hi Philipp,
>>>
>>> I'll start reviewing this code change. Since this is a P3 bug fix, we still have a month's time (before RDP 2 starts on 7/18) to work on it.
>>>
>>> Also, I've included John as a reviewer. He is the author of the Compatibility.java test.
>>>
>>> Thanks,
>>> Max
>>>
>>>
>>>>
>>>> On May 23, 2019, at 9:49 AM, Weijun Wang <
>>>> weijun.wang at oracle.com
>>>> > wrote:
>>>>
>>>> Hi Philipp,
>>>>
>>>> I've just uploaded your patch to
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~weijun/8217375/webrev.01/
>>>
>>>
>>>
>>
>>
>>
> <8217375-jarsignerbreaksexistingsignatures-20190630.patch>
More information about the security-dev
mailing list