Jarsigner compatibility issue invalidating existing signatures

[Weijun Wang]

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.kunz at 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>