RFR 8217375: jarsigner breaks old signature with long lines in manifest

[Philipp Kunz]

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)).readAllBy
> tes()
> 
> - 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/
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190630/2c11f16f/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8217375-jarsignerbreaksexistingsignatures-20190630.patch
Type: text/x-patch
Size: 310270 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20190630/2c11f16f/8217375-jarsignerbreaksexistingsignatures-20190630.patch>