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

Philipp Kunz philipp.kunz at paratix.ch
Sat Jul 20 20:49:50 UTC 2019


Hi Max and everyone,
 * Indeed, [^\"] does not work with quotes inside CN. I agree to your
change.
 * I went through the patch again myself and tried to understand what I
did half a year ago and added and changed some comments and renamed a
few identifiers so that I hope it becomes clearer. Particularly
PreserveRawManifestEntryAndDigest is worth mentioning and had a bug in
lineWidth70. Noteworthy is the test case with
emptyJarTrailingSeqAddFile which is now better documented.
 * I also renamed atts to attrs.
 * It also still works without the Resources changes.
 * I removed testNameEmptyTrusted from EmptyIndividualSectionName after
having convinced myself it is not necessary. Any other opinion on this
welcome.
 * I moved some common functions used in many tests to Utils.
Utils.echoManifest now prints manifests with non-printable characters
escaped in Java style but that was in the previous patch already I
think. It's used in more tests now than before.
 * I added a test testAddManifestMainAttributesFile in
MainAttributesConfused that shows where the problems were with a file
named Manifest-Main-Attributes inside a signed jar and that it works
now.
 * I increased the timeout of PreserveRawManifestEntryAndDigest after
it timed out on my machine a few times. The current timeout value is
still pretty much arbitrary.
 * I defined KS_ARGS in various tests to be re-used with repeated
jarsigner command line arguments.



I'm sorry for recently not responding quickly. I admit that my personal
interest has not increased with the latest development of webstart and
I also have other duties or distractions. I welcome everyone to
contribute missing parts or a review or an opinion. As I currently see
it, the following parts are not yet complete.
1. I suggest Compatibility test be run with different JDK versions
configured just like before with a focus on TSA now also with jar
signinig compatibility across JDK versions on a central and powerful
test infrastructure which is achieved by invoking Compatibility test
again with different arguments. SignTwice is an example for that with
only one JDK version.
2. Compatibility test should probably be thoroughly refactored. This
can be done well after the merge in my opinion if no seriously harmful
changes are revealed in a review.
3. I still did not have time to pick up Max's idea to add a jar file in
binary form to the sources that would allow to test a current JDK
version against it by trying to reproduce it identically. That would at
least make incompatibilities detected earlier than with Compatibility
test. How much such a test would overlap with the current Compatibility
test once integrated in a build like before with TSA can be told only
after it exists. In my opinion this makes a lot of sense in any case.
4. Somehow I still have not the feeling that someone has thoroughly
reviewed the whole patch. It was mentioned that the main source changes
look good but is that what a review takes? There are so many subtle
changes mostly in ManifestDigester that I doubt there is way around a
close look into the tests. It may be all right to check the details
only after all the trivial checks passed but that patch has accumulated
so much effort until now I really would appreciate some confirmation
that everything looks somewhat promising so far, however non-committal. 
I might have missed a hint and feel still quite new as openjdk
contributor.
5. Remove all the FIXED_8217375 stuff. In my opinion this requires that
after the patch is more or less stable that someone verifies that all
the test cases that did not exist before the patch continue to pass
unchanged and test coverage is appropriate to calibrate existing
behavior. Usually tests should exist before but this here is certainly
one and hopefully a rare exception. It would be of no use if some tests
were wrong claiming some behavior has not changed and with signed jars
and digesting manifests every single difference invalidates the
signatures without mercy. Actually removing the FIXED_8217375 stuff is
then trivial.

Lot of fuzzy talk written, please accept my apology for spamming this
way. I always appreciate any comment, hint, advice or anything.
Regards,Philipp


On Tue, 2019-07-16 at 21:29 +0800, Weijun Wang wrote:
> I made another small change
> 
> diff -iu
> b/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.j
> ava
> b/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.j
> ava
> ---
> b/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.j
> ava
> +++
> b/test/jdk/sun/security/tools/jarsigner/compatibility/Compatibility.j
> ava
> @@ -717,7 +717,7 @@
>                      + "  Digest algorithm: " +
> signItem.expectedDigestAlg()
>                      + (signItem.tsaIndex < 0 ? "" :
>                        ")|("
> -                    + "Timestamped by \"[^\"]+\" on .*"
> +                    + "Timestamped by \".+\" on .*"
>                      + ")|("
>                      + "  Timestamp digest algorithm: "
>                              + signItem.expectedTsaDigestAlg()
> 
> This is because there can be quotation marks inside CN. For example,
> 
>   Timestamped by "CN="Sectigo RSA Time Stamping Signer #1", O=Sectigo
> Limited, L=Salford, ST=Greater Manchester, C=GB" on Tue Jul 16
> 12:23:29 UTC 2019
> 
> Thanks,
> Max
> 
> 
> > On Jul 16, 2019, at 2:13 PM, Weijun Wang <weijun.wang at oracle.com>
> > wrote:
> > 
> > I'd also like to revert the changes to
> > src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Resource
> > s.java. Your change strips some trailing whitespaces but not all of
> > them. This will trigger a re-translation of all Resources files and
> > I think that is unnecessary.
> > 
> > Will any of your new tests fail because of the revert? I'll run all
> > tests soon.
> > 
> > Thanks,
> > Max
> > 
> > 
> > > On Jul 16, 2019, at 11:34 AM, Weijun Wang <weijun.wang at oracle.com
> > > > wrote:
> > > 
> > > 7/18 is near. If there is no new webrev, I'll push webrev.02 with
> > > SignTwice.java disabled. We can still modify tests before 8/8.
> > > 
> > > Thanks,
> > > Max
> > > 
> > > > On Jul 10, 2019, at 10:34 AM, Weijun Wang <weijun.wang at oracle.c
> > > > om> wrote:
> > > > 
> > > > [off the list]
> > > > 
> > > > Hi Philipp,
> > > > 
> > > > Do you have a new patch? The RDP 2 [1] starts on 7/18 and after
> > > > that we will only be able to fix P1-P2 bugs after approvals.
> > > > 
> > > > Let's get the changeset integrated before RDP 2 once there is
> > > > no problem in src/. We can adjust the tests before RC (8/8).
> > > > 
> > > > Thanks,
> > > > Max
> > > > 
> > > > [1] https://openjdk.java.net/jeps/3#rdp-2
> > > > 
> > > > > On Jul 3, 2019, at 6:27 PM, Weijun Wang <weijun.wang at oracle.c
> > > > > om> wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > > On Jul 3, 2019, at 4:06 PM, Philipp Kunz <philipp.kunz at para
> > > > > > tix.ch> wrote:
> > > > > > 
> > > > > > Hi Max,
> > > > > > 
> > > > > > Do I understand correctly that your question refers to
> > > > > >     if (e == null && MF_MAIN_ATTRS.equals(name)) {
> > > > > >         e = getMainAttsEntry();
> > > > > >     }
> > > > > > ?
> > > > > 
> > > > > Yes.
> > > > > 
> > > > > > If so, because this sticks closest to the previous
> > > > > > behavior. According to src/java.base/share/classes/module-
> > > > > > info.java, ManifestDigester is not publicly exported but
> > > > > > how can it then be used then in JarVerifier in
> > > > > > java.util.jar? If you or someone can confirm that we can
> > > > > > identify all callers, I'd prefer to remove above three
> > > > > > lines of code.
> > > > > 
> > > > > It's not publicly exported therefore cannot be used by an
> > > > > application, but it's accessible by java.util.jar which is
> > > > > also in the java.base module.
> > > > > 
> > > > > It can also be accessed by JDK modules listed in the "exports
> > > > > sun.security.util to" block of
> > > > > java.base/share/classes/module-info.java.
> > > > > 
> > > > > Anyway any caller must be inside JDK. IntelliJ IDEA tells me
> > > > > its only SignatureFileVerifier and JarSigner. Hopefully
> > > > > whatever IDE you are using it gives the same result.
> > > > > 
> > > > > > Otherwise, with above three lines, main attributes and an
> > > > > > entry with name MF_MAIN_ATTRS will still be confused for
> > > > > > all those code continuing to call get as before the patch
> > > > > > if an entry with such a name exists just like before.
> > > > > > However, with the patch, all (known) places that call
> > > > > > ManifestDigester.get* are lines 539 and 606 in
> > > > > > SignatureFileVerifier with the patch, 539 calling
> > > > > > getMainAttsEntry without any ambiguity and 606 calling
> > > > > > get(String, b) always expecting the entry for a section
> > > > > > that is present and would otherwise not be known to pass as
> > > > > > an argument anyway, provided the signature file does not
> > > > > > contain more entries than the manifest or entries have not
> > > > > > been removed from the manifest after signing, should work
> > > > > > correctly, I guess. If we cannot get rid of those three
> > > > > > lines, another test for those edge cases should probably be
> > > > > > added, shouldn't it, if not to support the previous
> > > > > > confusion at least to show it now works as expected?
> > > > > > 
> > > > > > What exactly did you mean with "coding error"?
> > > > > 
> > > > > Not sure. I don't know if any bad thing would happen if a
> > > > > file named "Manifest-Main-Attributes" is out inside a jar
> > > > > file.
> > > > > 
> > > > > > Along with removing above three lines of code, two tests
> > > > > > would have to be adjusted.
> > > > > > FindHeaderEndVsManifestDigesterFindFirstSection on lne 235
> > > > > > and DigestInput on line 270.
> > > > > 
> > > > > That's OK. No real app should call methods in this class
> > > > > directly.
> > > > > 
> > > > > Thanks,
> > > > > Max
> > > > > 
> > > > > > Regards,
> > > > > > Philipp
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Mon, 2019-07-01 at 15:41 +0800, Weijun Wang wrote:
> > > > > > > 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
> > > > > > > 
> > > > > > > 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20190720/7e5b7f65/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 8217375-jarsignerbreaksexistingsignatures-20190719.patch
Type: text/x-patch
Size: 313248 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20190720/7e5b7f65/8217375-jarsignerbreaksexistingsignatures-20190719-0001.patch>


More information about the security-dev mailing list