Hi Philipp, Thanks for the report. I've filed https://bugs.openjdk.java.net/browse/JDK-8217375 and post your patch at
https://cr.openjdk.java.net/~weijun/8217375/webrev.00/ Some comments: 1. While the updated Manifest constructor is good, it's an API change. To avoid this change if we want to backport the change to JDK 11, in JarSigner::sign0, we can create oldManifest using mfRawBytes again. 2. Those TODOs, do you really need to do anything? Thanks, Max > On Jan 18, 2019, at 2:44 AM, Philipp Kunz <philipp.k...@paratix.ch> wrote: > > Hi, > > Related to JDK-6372077 i found that jarsigner breaks existing signatures in > cases the binary manifest encoding changes for individual sections. > That has potential to become more popular after JDK-6372077 changed the line > width from 70 to 72 bytes. Signing a jarfile already signed with a > pre-6372077 / JDK before 11 jarsigner again with a different signer with > another jarsigner version that has JDK-6372077 / JDK 11 or later will > invalidate many existing signatures. This affects individual section names as > with the jar file contents file names which can be expected to exceed 66 > bytes of length of their names often as well as digest headers with longer > digest values such as SHA-512. Apart from that, it can also happen if someone > changed the line breaks (crlf to lf or cr) or some line break positions > before signing again with a different signer. > > The same does not apply to manifest main attributes. Special treatment is in > place for those near JarSigner#findHeaderEnd. I wouldn't know why not also > for individual sections where all the jar file signed content files' digests > are. > > In course of developing this patch I also encountered a few other related > issues. > - Manifest copy constructor does not deeply copy individual section > Attributes but instead only copies the list of sections. See > ManifestCopyConstructor test > - ManifestDigester confuses manifest main attributes with an individual > section name of "Manifest-Main-Attributes". See ManifestMainAttributesDigest > test > - ManifestDigester fails to digest manifests that end with \r with an array > index out of bounds exception and such jar files cannot be signed. See > LineBreaks test > - JarSigner accepts an existing digest for a jar entry even if the > (upper/lower) case of the base64 encoded form does not match. See > ExistingManifestDigestEntryDontIgnoreCase test > - Compatibility test fails to detect such an incompatibility as mentioned. > > The chosen approach was to re-use Manifest-Digester to identify the input > portions for the digests and extend it so that it can reproduce them if a > section is unmodified to write unmodified portions of a manifest. > This made JarSigner#findHeaderEnd redundant as now replaced with > ManifestDigester#findSection. See > FindHeaderEndVsManifestDigesterFindFirstSection test. > > All existing and the new tests show it actually works. There are a few API > changes and sure more opportunities to optimize, refacter, add more test > coverage, personal preferences, project conventions, and so on. Possibly, > oldStyle or workaround could be removed from ManifestDigester, first. Or a > DigestOutputStream could be created that does not wrap another OutputStream, > only is one. Another point might be that the Compatibility test now takes > longer than before and because I don't know all the parameters, for example > the number of JDKs tested and if minor versions are included, and therefore > don't know if it wouldn't take too long the way it is now. If you run the > tests against the current code base, getMainAttsEntry will not be found in > ManifestMainAttributes, so replace it with > "get(ManifestDigester.MF_MAIN_ATTRS, " above. These comments are not intended > to be kept after compatibility with existing code has been established. > Probably that one whole test is obsolete once manifest main attributes > confusion in ManifestDigester is resolved. There still are some other TODOs > non of which too scary or intended to leave like that where I see potential > for improvement and I'd appreciate guidance. > > Recently I read that JDK 8 is still the most used major version, possibly > even much more so in production environments where signed jars are typically > expected to be found. If this patch could be backported to JDK 11 where > JDK-6372077 was introduced and which is an LTS release, I figure we might > save a few people some trouble with unexpectedly invalid signatures. But one > step after another. I'm curiously looking forward to any kind of feedback. I > cannot create an issue for it because I don't have the permissions and I > haven't found so far anything related either. Is this the right mailing list > or shall I post it to core-libs-dev or another one as well? Would someone > volunteer to sponsor? > > Regards, > Philipp > > > https://bugs.openjdk.java.net/browse/JDK-6372077 > https://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051707.html > > > <jarsignerbreaksexistingsignatures.patch>
