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>

Reply via email to