Re: hello, im a new contributor

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: hello, im a new contributor

Vincent Ryan
Moved to security-dev


> On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]> wrote:
>
> Hello everyone
>
> I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may not be too big a piece for a first contribution.
>
> I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:
>
> May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?
>
> Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.
>
> One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.
>
> I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.
>
> Philipp Kunz

Reply | Threaded
Open this post in threaded view
|

Re: hello, im a new contributor

Vincent Ryan
Hello Philipp,

I’m happy to sponsor your fix for JDK 10. Have you followed these steps: http://openjdk.java.net/contribute/ ?

Thanks.


On 1 Sep 2017, at 08:58, Vincent Ryan <[hidden email]> wrote:

Moved to security-dev


On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]> wrote:

Hello everyone

I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may not be too big a piece for a first contribution.

I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:

May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?

Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.

One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.

I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.

Philipp Kunz


Reply | Threaded
Open this post in threaded view
|

Re: hello, im a new contributor

Philipp Kunz
Hello Vincent

Thank you for sponsoring!
So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here: http://www.oracle.com/technetwork/community/oca-486395.html#p -> Paratix GmbH
Therefore I think I have followed the steps in http://openjdk.java.net/contribute/ at least the ones before actual patch submission.

Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
make run-test-tier1
make run-test TEST="jdk/test"
they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
then it reports zero errors and failures which may be a good starting point.
Do you think that sounds reasonable or do you have another suggestion how to run the tests?

Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java. This way, I try to demonstrate the improvement.

I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
So I'll try to remove it first including the whole four line if block.

Philipp


On 01.09.2017 10:00, Vincent Ryan wrote:
Hello Philipp,

I’m happy to sponsor your fix for JDK 10. Have you followed these steps: http://openjdk.java.net/contribute/ ?

Thanks.


On 1 Sep 2017, at 08:58, Vincent Ryan <[hidden email]> wrote:

Moved to security-dev


On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]> wrote:

Hello everyone

I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may not be too big a piece for a first contribution.

I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:

May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?

Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.

One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.

I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.

Philipp Kunz



Reply | Threaded
Open this post in threaded view
|

Re: hello, im a new contributor

Vincent Ryan
That all sounds fine. Let me know when your patch is ready to submit.
Thanks.


On 1 Sep 2017, at 13:15, Philipp Kunz <[hidden email]> wrote:

Hello Vincent

Thank you for sponsoring!
So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here: http://www.oracle.com/technetwork/community/oca-486395.html#p -> Paratix GmbH
Therefore I think I have followed the steps in http://openjdk.java.net/contribute/ at least the ones before actual patch submission.

Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
make run-test-tier1
make run-test TEST="jdk/test"
they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
then it reports zero errors and failures which may be a good starting point.
Do you think that sounds reasonable or do you have another suggestion how to run the tests?

Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java. This way, I try to demonstrate the improvement.

I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
So I'll try to remove it first including the whole four line if block.

Philipp


On 01.09.2017 10:00, Vincent Ryan wrote:
Hello Philipp,

I’m happy to sponsor your fix for JDK 10. Have you followed these steps: http://openjdk.java.net/contribute/ ?

Thanks.


On 1 Sep 2017, at 08:58, Vincent Ryan <[hidden email]> wrote:

Moved to security-dev


On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]> wrote:

Hello everyone

I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may not be too big a piece for a first contribution.

I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:

May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?

Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.

One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.

I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.

Philipp Kunz




Reply | Threaded
Open this post in threaded view
|

JDK-6695402: Jarsigner with multi-byte characters in class names

Philipp Kunz
Hello Vincent

I narrowed the error down so far and suspect now that it is an effect of sun.security.util.ManifestDigester's constructor, lines 134 and 163-164, in combination with java.util.jar.Manifest.make72Safe. Manifest breaks multi-byte characters in manifest section names across lines and ManifestDigester fails to restore them correctly, which both sounds wrong but ManifestDigester sure is.

Just fixing it would be too tempting but then I would not know (I mean not know for sure with evidence from tests) if any existing jar-signature would still be valid and I don't want to break existing signatures. When I had a look at the tests specific for Manifest and ManifestDigester I found very little. There are many more different situations and corner cases and combinations thereof to cover with tests than it looks like at first glance so that writing tests that cover all relevant cases looks to me like quite an effort. I have the impression that test coverage was not a priority when the subjected code was developed or at least the tests might not have been contributed to the open JDK project. Hence, while I'm continuing to complete these tests I miss, if someone has an idea how to simplify that so that I can still convince at least myself that no existing signature will break or where possibly more testcases are that I haven't discovered so far, please let me know.

I'm also wondering how much performance should be taken into consideration. There are a few hints such as reusing byte arrays that suggest that it is an issue. Will a patch be rejected if it slows down some tests beyond a certain limit for so little added value or what might be the acceptance criteria here? I don't expect insuperable difficulties with performance but would still appreciate some general idea.

My desired or currently preferred approach to fix JDK-6695402 would be to let ManifestDigester any kind of use or extend Manifest in order to let Manifest identify the manifest sections thereby preventing the parsing duplicated that identifies the manifest sections.

As a new contributor I could use and would appreciate some guidance particularly about what kind and completeness of test coverage and performance tuning applies or is suggested in the context of the given bug. Otherwise I fear I might contribute too many tests which would have to be reviewed and maintained or quite some work would be for nothing if a patch would not satisfy performance requirements.

Regards,
Philipp



On 01.09.2017 15:20, Vincent Ryan wrote:
That all sounds fine. Let me know when your patch is ready to submit.
Thanks.


On 1 Sep 2017, at 13:15, Philipp Kunz <[hidden email]> wrote:

Hello Vincent

Thank you for sponsoring!
So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here: http://www.oracle.com/technetwork/community/oca-486395.html#p -> Paratix GmbH
Therefore I think I have followed the steps in http://openjdk.java.net/contribute/ at least the ones before actual patch submission.

Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
make run-test-tier1
make run-test TEST="jdk/test"
they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
then it reports zero errors and failures which may be a good starting point.
Do you think that sounds reasonable or do you have another suggestion how to run the tests?

Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java. This way, I try to demonstrate the improvement.

I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
So I'll try to remove it first including the whole four line if block.

Philipp


On 01.09.2017 10:00, Vincent Ryan wrote:
Hello Philipp,

I’m happy to sponsor your fix for JDK 10. Have you followed these steps: http://openjdk.java.net/contribute/ ?

Thanks.


On 1 Sep 2017, at 08:58, Vincent Ryan <[hidden email]> wrote:

Moved to security-dev


On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]> wrote:

Hello everyone

I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may not be too big a piece for a first contribution.

I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:

May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?

Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.

One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.

I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.

Philipp Kunz





--


Gruss Philipp







Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Philipp Kunz
Hello Vincent

Here may be the fix for JDK-6695402. First a test.

BEGIN /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
/*
 * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/*
 * @test
 * @bug JDK-6695402
 * @summary verify signatures of jars containing classes with names with multi-
 *          byte unicode characters broken across lines
 * @library /test/lib
 * @modules jdk.jartool/sun.tools.jar
 *          jdk.jartool/sun.security.tools.jarsigner
 * @build jdk.test.lib.JDKToolLauncher
 *        jdk.test.lib.process.*
 * @run main MultibyteUnicodeName
 */

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.jar.JarFile;
import java.util.jar.Manifest;
import java.util.stream.Collectors;
import java.util.Arrays;
import java.util.Map;
import java.util.jar.Attributes.Name;
import java.util.jar.JarEntry;

import static java.nio.charset.StandardCharsets.UTF_8;

import sun.security.tools.jarsigner.Resources;

import jdk.test.lib.JDKToolLauncher;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.process.ProcessTools;

public class MultibyteUnicodeName {
   
    public static void main(String[] args) throws Throwable {
        try {
            prepare();
           
            testSignJar("test.jar");
            testSignJarNoManifest("test-no-manifest.jar");
            testSignJarUpdate("test-update.jar");
            testSignJarWithIndex("test-index.jar");
            testSignJarAddIndex("test-add-index.jar");
           
        } finally {
            Files.deleteIfExists(Paths.get(keystoreFileName));
            Files.deleteIfExists(Paths.get(ManifestFileName));
            Files.deleteIfExists(Paths.get(refClassFilename));
            Files.deleteIfExists(Paths.get(testClassFilename));
        }
    }
   
    static final String alias = "a";
    static final String keystoreFileName = "test.jks";
    static final String ManifestFileName = "MANIFEST.MF";
   
    static class A1234567890B1234567890C1234567890D12345678exyz { }
    static class A1234567890B1234567890C1234567890D12345678éxyz { }
   
    static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() +
            "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
    static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() +
            "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
   
    static void prepare() throws Throwable {
        tool("keytool", "-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
                "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
                "-validity", "366")
                .shouldHaveExitValue(0);
       
        Files.write(Paths.get(ManifestFileName),
                (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
        copyClassToFile(refClassFilename);
        copyClassToFile(testClassFilename);
    }
   
    static void copyClassToFile(String classFilename) throws IOException {
        try (
            InputStream asStream = MultibyteUnicodeName.class.getResourceAsStream(classFilename);
            ByteArrayOutputStream buf = new ByteArrayOutputStream();
        ) {
            int b;
            while ((b = asStream.read()) != -1) buf.write(b);
            Files.write(Paths.get(classFilename), buf.toByteArray());
        }
    }

    static void testSignJar(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvfm", jarFileName,
                    ManifestFileName, refClassFilename, testClassFilename)
                    .shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }

    static void testSignJarNoManifest(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvf", jarFileName, refClassFilename, testClassFilename)
                .shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }
   
    static void testSignJarUpdate(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvfm", jarFileName, ManifestFileName, refClassFilename)
                .shouldHaveExitValue(0);
            tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
                    "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0);
            tool("jar", "uvf", jarFileName, testClassFilename)
                .shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }
   
    static void testSignJarWithIndex(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvfm", jarFileName,
                    ManifestFileName, refClassFilename, testClassFilename)
                .shouldHaveExitValue(0);
            tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }

    static void testSignJarAddIndex(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvfm", jarFileName,
                    ManifestFileName, refClassFilename, testClassFilename)
                .shouldHaveExitValue(0);
            tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
                    "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0);
            tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }
   
    static Map<String, String> jarsignerResources = Arrays.stream(new Resources().getContents()).
            collect(Collectors.toMap(e -> (String) e[0], e -> (String) e[1]));
           
    static void verifyJarSignature(String jarFileName) throws Throwable {
        // actually sign the jar
        tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
                "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0);
       
        verifyClassNameLineBroken(jarFileName);
       
        // check for no unsigned entries
        tool("jarsigner", "-verify", "-keystore", keystoreFileName, "-storetype", "JKS",
                "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0)
                .shouldNotContain(jarsignerResources.get(
                        "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));
       
        // check that both classes with and without multi-byte unicode characters in their names are signed
        tool("jarsigner", "-verify", "-verbose", "-keystore", keystoreFileName, "-storetype", "JKS",
                "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0)
                .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + refClassFilename.replaceAll("\\$", "\\\\\\$") + "$")
                .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + testClassFilename.replaceAll("\\$", "\\\\\\$") + "$");
       
        // check that both classes with and without multi-byte unicode characters in their names have signing certificates
        tool("jarsigner", "-verify", "-verbose", "-certs", "-keystore", keystoreFileName,
                "-storetype", "JKS", "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0)
                .shouldMatch(refClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)")
                .shouldMatch(testClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)");
    }
   
    /**
     * it would be too easy to miss the actual test case by just renaming an identifier so that
     * the multi-byte encoded character would not any longer be broken across a line break.
     *
     * this test verifies that the actual test case is tested based on the manifest
     * and not based on the signature file because at the moment, the signature file
     * does not even contain the desired entry at all.
     *
     * this relies on the Manifest breaking lines unaware of bytes that belong to the same
     * multi-ybte utf characters.
     */
    static void verifyClassNameLineBroken(String jarFileName) throws IOException {
        byte[] eAcute = "é".getBytes(UTF_8);
        byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
       
        try (
            JarFile jar = new JarFile(jarFileName);
        ) {
            if (jar.getManifest().getAttributes(testClassFilename) == null) {
                throw new AssertionError(testClassFilename + " not found in manifest");
            }
           
            JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
            try (
                InputStream manifestIs = jar.getInputStream(manifestEntry);
            ) {
                int bytesMatched = 0;
                for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
                    if ((byte) b == eAcuteBroken[bytesMatched]) {
                        bytesMatched++;
                        if (bytesMatched == eAcuteBroken.length) {
                            break;
                        }
                    } else {
                        bytesMatched = 0;
                    }
                }
                if (bytesMatched < eAcuteBroken.length) {
                    throw new AssertionError("self-test failed: "
                            + "multi-byte utf-8 character not broken across lines");
                }
            }
        }
    }
   
    static OutputAnalyzer tool(String tool, String... args)
            throws Throwable {
        JDKToolLauncher l = JDKToolLauncher.createUsingTestJDK(tool);
        for (String arg : args) {
            if (arg .startsWith("-J")) {
                l.addVMArg(arg.substring(2));
            } else {
                l.addToolArg(arg);
            }
        }
        return ProcessTools.executeCommand(l.getCommand());
    }
   
}
END /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java


There are three e with acutes, on lines 84, 89, and 227, just in case the mail suffers bad encoding which might not be absolutely unconceivable regarding the bug's subject.

In contrast to the bug description it uses a nested class with a two-byte UTF-8 character rather than one in its own file. I chose to do it that way because then the complete test goes into one file and therefore I assume the overview is kept easier. The test still demonstrates exactly JDK-6695402's problem.

It may not have been necessary to also test jar files with indexes but i consider it necessary to have signing unsigned as well as partially signed jars tested, so why not have one more case tested, too?

There might also be a more elegant approach to get class files into a jar file as to get their contents through the class loader. I chose to use real class files rather than dummy contents in files named *.class or for instance plain text files in the jar the signing of which to be tested in order to stay as close to the original bug problem as possible even though I don't have any notion that it would make a difference. The amount of code used to copy the class file contents around is comparatively small with respect to the whole test case amount of code.
 
For the demonstration that the multi-byte character actually makes the alleged difference, I concluded that it was necessary to have another class name not previously affected by the bug in order to compare the effects of just different names. Otherwise the signing could fail to any other reason undetectably.

In order to express a condition to tell successful from failed test runs the best approach I found was to analyze the jarsigner tool's output which is in my opinion not the most desirable option. I'd have preferred output from a clearer structured api but then I guess the output format will not change too often in the foreseeable future. For instance the check for the s, m, and k flags is more pragmatical approach than obviously perfectly failsafe when operating with regular expressions to match it with the output. Other parts such as 'X.509' and 'CN=' are not even jarsigner tool resources. I put at least a reference to sun.security.tools.jarsigner.Resources.

Finally, I afforded kind of a self-test that protects the test from undetected failing because renaming the main class which would move the two-byte unicode character to a position not suitable for the test. It may not be absolutely necessary.



BEGIN PATH
diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
--- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
+++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Wed Sep 20 00:56:15 2017 +0200
@@ -28,6 +28,7 @@
 import java.security.*;
 import java.util.HashMap;
 import java.io.ByteArrayOutputStream;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 /**
  * This class is used to compute digests on sections of the Manifest.
@@ -112,7 +113,7 @@
         rawBytes = bytes;
         entries = new HashMap<>();
 
-        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
 
         Position pos = new Position();
 
@@ -131,50 +132,40 @@
 
             if (len > 6) {
                 if (isNameAttr(bytes, start)) {
-                    StringBuilder nameBuf = new StringBuilder(sectionLen);
+                    nameBuf.reset();
+                    nameBuf.write(bytes, start+6, len-6);
 
-                    try {
-                        nameBuf.append(
-                            new String(bytes, start+6, len-6, "UTF8"));
+                    int i = start + len;
+                    if ((i-start) < sectionLen) {
+                        if (bytes[i] == '\r') {
+                            i += 2;
+                        } else {
+                            i += 1;
+                        }
+                    }
 
-                        int i = start + len;
-                        if ((i-start) < sectionLen) {
-                            if (bytes[i] == '\r') {
-                                i += 2;
-                            } else {
-                                i += 1;
-                            }
+                    while ((i-start) < sectionLen) {
+                        if (bytes[i++] == ' ') {
+                            // name is wrapped
+                            int wrapStart = i;
+                            while (((i-start) < sectionLen)
+                                    && (bytes[i++] != '\n'));
+                            if (bytes[i-1] != '\n')
+                                return; // XXX: exception?
+                            int wrapLen;
+                            if (bytes[i-2] == '\r')
+                                wrapLen = i-wrapStart-2;
+                            else
+                                wrapLen = i-wrapStart-1;
+
+                            nameBuf.write(bytes, wrapStart, wrapLen);
+                        } else {
+                            break;
                         }
+                    }
 
-                        while ((i-start) < sectionLen) {
-                            if (bytes[i++] == ' ') {
-                                // name is wrapped
-                                int wrapStart = i;
-                                while (((i-start) < sectionLen)
-                                        && (bytes[i++] != '\n'));
-                                    if (bytes[i-1] != '\n')
-                                        return; // XXX: exception?
-                                    int wrapLen;
-                                    if (bytes[i-2] == '\r')
-                                        wrapLen = i-wrapStart-2;
-                                    else
-                                        wrapLen = i-wrapStart-1;
-
-                            nameBuf.append(new String(bytes, wrapStart,
-                                                      wrapLen, "UTF8"));
-                            } else {
-                                break;
-                            }
-                        }
-
-                        entries.put(nameBuf.toString(),
-                            new Entry(start, sectionLen, sectionLenWithBlank,
-                                rawBytes));
-
-                    } catch (java.io.UnsupportedEncodingException uee) {
-                        throw new IllegalStateException(
-                            "UTF8 not available on platform");
-                    }
+                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
+                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
                 }
             }
             start = pos.startOfNext;
END PATCH


The patch is mostly so big because indentation has changed in many lines.

There was baos there before without apparent use. The patch renames it and puts it into use. It came in very handy because if it would not have been there already, I would have had to defined one of my own.

The main point of the patch is that manifest section names that are broken across lines at possibly arbitrary bytes are no longer converted into strings for each manifest line and then joined. Parts of names broken across lines are now joined first when they are still byte sequences and only when the complete byte sequence is available after processing all manifest lines that contain the same manifest section name decoded into a unicode string.

For decoding the bytes into a string I chose a different string constructor than was used before that does not any longer declare UnsupportedEncodingException rendering the try/catch redundant. It couldn't have ever occurred anyway taking into consideration that UTF-8 is mandatory for every Java platform, StandardCharsets says. The difference according to the documentation is that the previously used String constructor returned undefined strings for invalid byte sequences whereas the one used in the patch will replace unparseable portions with valid 'unknown' characters.

One question I cannot still answer yet is how the ManifestDigester can be changed at all without complete test coverage or risking to break existing signatures. I already started on that but it's quite a piece of work to almost formally prove that all manifests will continue to produce identical digests, except of course for the ones now fixed.

Regards,
Philipp


On 17.09.2017 21:25, Philipp Kunz wrote:
Hello Vincent

I narrowed the error down so far and suspect now that it is an effect of sun.security.util.ManifestDigester's constructor, lines 134 and 163-164, in combination with java.util.jar.Manifest.make72Safe. Manifest breaks multi-byte characters in manifest section names across lines and ManifestDigester fails to restore them correctly, which both sounds wrong but ManifestDigester sure is.

Just fixing it would be too tempting but then I would not know (I mean not know for sure with evidence from tests) if any existing jar-signature would still be valid and I don't want to break existing signatures. When I had a look at the tests specific for Manifest and ManifestDigester I found very little. There are many more different situations and corner cases and combinations thereof to cover with tests than it looks like at first glance so that writing tests that cover all relevant cases looks to me like quite an effort. I have the impression that test coverage was not a priority when the subjected code was developed or at least the tests might not have been contributed to the open JDK project. Hence, while I'm continuing to complete these tests I miss, if someone has an idea how to simplify that so that I can still convince at least myself that no existing signature will break or where possibly more testcases are that I haven't discovered so far, please let me know.

I'm also wondering how much performance should be taken into consideration. There are a few hints such as reusing byte arrays that suggest that it is an issue. Will a patch be rejected if it slows down some tests beyond a certain limit for so little added value or what might be the acceptance criteria here? I don't expect insuperable difficulties with performance but would still appreciate some general idea.

My desired or currently preferred approach to fix JDK-6695402 would be to let ManifestDigester any kind of use or extend Manifest in order to let Manifest identify the manifest sections thereby preventing the parsing duplicated that identifies the manifest sections.

As a new contributor I could use and would appreciate some guidance particularly about what kind and completeness of test coverage and performance tuning applies or is suggested in the context of the given bug. Otherwise I fear I might contribute too many tests which would have to be reviewed and maintained or quite some work would be for nothing if a patch would not satisfy performance requirements.

Regards,
Philipp



On 01.09.2017 15:20, Vincent Ryan wrote:
That all sounds fine. Let me know when your patch is ready to submit.
Thanks.


On 1 Sep 2017, at 13:15, Philipp Kunz <[hidden email]> wrote:

Hello Vincent

Thank you for sponsoring!
So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here: http://www.oracle.com/technetwork/community/oca-486395.html#p -> Paratix GmbH
Therefore I think I have followed the steps in http://openjdk.java.net/contribute/ at least the ones before actual patch submission.

Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
make run-test-tier1
make run-test TEST="jdk/test"
they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
then it reports zero errors and failures which may be a good starting point.
Do you think that sounds reasonable or do you have another suggestion how to run the tests?

Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java. This way, I try to demonstrate the improvement.

I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
So I'll try to remove it first including the whole four line if block.

Philipp


On 01.09.2017 10:00, Vincent Ryan wrote:
Hello Philipp,

I’m happy to sponsor your fix for JDK 10. Have you followed these steps: http://openjdk.java.net/contribute/ ?

Thanks.


On 1 Sep 2017, at 08:58, Vincent Ryan <[hidden email]> wrote:

Moved to security-dev


On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]> wrote:

Hello everyone

I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may not be too big a piece for a first contribution.

I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:

May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?

Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.

One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.

I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.

Philipp Kunz












Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang
Hi Philipp

The change mostly looks fine. You might want to put everything into a patch file so Vincent can recreate a webrev and post it to cr.openjdk.java.net.

One thing I would suggest for the test is that instead of using jarsigner -verify and check the text in output you can open it as a JarFile, consume the content of an entry, and look into its getCertificates().

Also, you might want to reuse test/lib/jdk/test/lib/SecurityTools.java, and there is also JarUtils.java you can use. Maybe Files.copy(InputStream,Path) can be used in copyClassToFile().

Thanks
Max

> On Sep 20, 2017, at 7:41 AM, Philipp Kunz <[hidden email]> wrote:
>
> Hello Vincent
>
> Here may be the fix for JDK-6695402. First a test.
>
> BEGIN /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
> /*
>  * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>  *
>  * This code is free software; you can redistribute it and/or modify it
>  * under the terms of the GNU General Public License version 2 only, as
>  * published by the Free Software Foundation.
>  *
>  * This code is distributed in the hope that it will be useful, but WITHOUT
>  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>  * version 2 for more details (a copy is included in the LICENSE file that
>  * accompanied this code).
>  *
>  * You should have received a copy of the GNU General Public License version
>  * 2 along with this work; if not, write to the Free Software Foundation,
>  * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>  *
>  * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>  * or visit www.oracle.com if you need additional information or have any
>  * questions.
>  */
>
> /*
>  * @test
>  * @bug JDK-6695402
>  * @summary verify signatures of jars containing classes with names with multi-
>  *          byte unicode characters broken across lines
>  * @library /test/lib
>  * @modules jdk.jartool/sun.tools.jar
>  *          jdk.jartool/sun.security.tools.jarsigner
>  * @build jdk.test.lib.JDKToolLauncher
>  *        jdk.test.lib.process.*
>  * @run main MultibyteUnicodeName
>  */
>
> import java.io.ByteArrayOutputStream;
> import java.io.IOException;
> import java.io.InputStream;
> import java.io.UnsupportedEncodingException;
> import java.nio.file.Files;
> import java.nio.file.Paths;
> import java.util.jar.JarFile;
> import java.util.jar.Manifest;
> import java.util.stream.Collectors;
> import java.util.Arrays;
> import java.util.Map;
> import java.util.jar.Attributes.Name;
> import java.util.jar.JarEntry;
>
> import static java.nio.charset.StandardCharsets.UTF_8;
>
> import sun.security.tools.jarsigner.Resources;
>
> import jdk.test.lib.JDKToolLauncher;
> import jdk.test.lib.process.OutputAnalyzer;
> import jdk.test.lib.process.ProcessTools;
>
> public class MultibyteUnicodeName {
>    
>     public static void main(String[] args) throws Throwable {
>         try {
>             prepare();
>            
>             testSignJar("test.jar");
>             testSignJarNoManifest("test-no-manifest.jar");
>             testSignJarUpdate("test-update.jar");
>             testSignJarWithIndex("test-index.jar");
>             testSignJarAddIndex("test-add-index.jar");
>            
>         } finally {
>             Files.deleteIfExists(Paths.get(keystoreFileName));
>             Files.deleteIfExists(Paths.get(ManifestFileName));
>             Files.deleteIfExists(Paths.get(refClassFilename));
>             Files.deleteIfExists(Paths.get(testClassFilename));
>         }
>     }
>    
>     static final String alias = "a";
>     static final String keystoreFileName = "test.jks";
>     static final String ManifestFileName = "MANIFEST.MF";
>    
>     static class A1234567890B1234567890C1234567890D12345678exyz { }
>     static class A1234567890B1234567890C1234567890D12345678éxyz { }
>    
>     static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() +
>             "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
>     static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() +
>             "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
>    
>     static void prepare() throws Throwable {
>         tool("keytool", "-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
>                 "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
>                 "-validity", "366")
>                 .shouldHaveExitValue(0);
>        
>         Files.write(Paths.get(ManifestFileName),
>                 (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
>         copyClassToFile(refClassFilename);
>         copyClassToFile(testClassFilename);
>     }
>    
>     static void copyClassToFile(String classFilename) throws IOException {
>         try (
>             InputStream asStream = MultibyteUnicodeName.class.getResourceAsStream(classFilename);
>             ByteArrayOutputStream buf = new ByteArrayOutputStream();
>         ) {
>             int b;
>             while ((b = asStream.read()) != -1) buf.write(b);
>             Files.write(Paths.get(classFilename), buf.toByteArray());
>         }
>     }
>
>     static void testSignJar(String jarFileName) throws Throwable {
>         try {
>             tool("jar", "cvfm", jarFileName,
>                     ManifestFileName, refClassFilename, testClassFilename)
>                     .shouldHaveExitValue(0);
>             verifyJarSignature(jarFileName);
>
>         } finally {
>             Files.deleteIfExists(Paths.get(jarFileName));
>         }
>     }
>
>     static void testSignJarNoManifest(String jarFileName) throws Throwable {
>         try {
>             tool("jar", "cvf", jarFileName, refClassFilename, testClassFilename)
>                 .shouldHaveExitValue(0);
>             verifyJarSignature(jarFileName);
>
>         } finally {
>             Files.deleteIfExists(Paths.get(jarFileName));
>         }
>     }
>    
>     static void testSignJarUpdate(String jarFileName) throws Throwable {
>         try {
>             tool("jar", "cvfm", jarFileName, ManifestFileName, refClassFilename)
>                 .shouldHaveExitValue(0);
>             tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>                     "-storepass", "changeit", "-debug", jarFileName, alias)
>                 .shouldHaveExitValue(0);
>             tool("jar", "uvf", jarFileName, testClassFilename)
>                 .shouldHaveExitValue(0);
>             verifyJarSignature(jarFileName);
>
>         } finally {
>             Files.deleteIfExists(Paths.get(jarFileName));
>         }
>     }
>    
>     static void testSignJarWithIndex(String jarFileName) throws Throwable {
>         try {
>             tool("jar", "cvfm", jarFileName,
>                     ManifestFileName, refClassFilename, testClassFilename)
>                 .shouldHaveExitValue(0);
>             tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
>             verifyJarSignature(jarFileName);
>
>         } finally {
>             Files.deleteIfExists(Paths.get(jarFileName));
>         }
>     }
>
>     static void testSignJarAddIndex(String jarFileName) throws Throwable {
>         try {
>             tool("jar", "cvfm", jarFileName,
>                     ManifestFileName, refClassFilename, testClassFilename)
>                 .shouldHaveExitValue(0);
>             tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>                     "-storepass", "changeit", "-debug", jarFileName, alias)
>                 .shouldHaveExitValue(0);
>             tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
>             verifyJarSignature(jarFileName);
>
>         } finally {
>             Files.deleteIfExists(Paths.get(jarFileName));
>         }
>     }
>    
>     static Map<String, String> jarsignerResources = Arrays.stream(new Resources().getContents()).
>             collect(Collectors.toMap(e -> (String) e[0], e -> (String) e[1]));
>            
>     static void verifyJarSignature(String jarFileName) throws Throwable {
>         // actually sign the jar
>         tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>                 "-storepass", "changeit", "-debug", jarFileName, alias)
>                 .shouldHaveExitValue(0);
>        
>         verifyClassNameLineBroken(jarFileName);
>        
>         // check for no unsigned entries
>         tool("jarsigner", "-verify", "-keystore", keystoreFileName, "-storetype", "JKS",
>                 "-storepass", "changeit", "-debug", jarFileName, alias)
>                 .shouldHaveExitValue(0)
>                 .shouldNotContain(jarsignerResources.get(
>                         "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));
>        
>         // check that both classes with and without multi-byte unicode characters in their names are signed
>         tool("jarsigner", "-verify", "-verbose", "-keystore", keystoreFileName, "-storetype", "JKS",
>                 "-storepass", "changeit", "-debug", jarFileName, alias)
>                 .shouldHaveExitValue(0)
>                 .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + refClassFilename.replaceAll("\\$", "\\\\\\$") + "$")
>                 .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + testClassFilename.replaceAll("\\$", "\\\\\\$") + "$");
>        
>         // check that both classes with and without multi-byte unicode characters in their names have signing certificates
>         tool("jarsigner", "-verify", "-verbose", "-certs", "-keystore", keystoreFileName,
>                 "-storetype", "JKS", "-storepass", "changeit", "-debug", jarFileName, alias)
>                 .shouldHaveExitValue(0)
>                 .shouldMatch(refClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)")
>                 .shouldMatch(testClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)");
>     }
>    
>     /**
>      * it would be too easy to miss the actual test case by just renaming an identifier so that
>      * the multi-byte encoded character would not any longer be broken across a line break.
>      *
>      * this test verifies that the actual test case is tested based on the manifest
>      * and not based on the signature file because at the moment, the signature file
>      * does not even contain the desired entry at all.
>      *
>      * this relies on the Manifest breaking lines unaware of bytes that belong to the same
>      * multi-ybte utf characters.
>      */
>     static void verifyClassNameLineBroken(String jarFileName) throws IOException {
>         byte[] eAcute = "é".getBytes(UTF_8);
>         byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
>        
>         try (
>             JarFile jar = new JarFile(jarFileName);
>         ) {
>             if (jar.getManifest().getAttributes(testClassFilename) == null) {
>                 throw new AssertionError(testClassFilename + " not found in manifest");
>             }
>            
>             JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
>             try (
>                 InputStream manifestIs = jar.getInputStream(manifestEntry);
>             ) {
>                 int bytesMatched = 0;
>                 for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
>                     if ((byte) b == eAcuteBroken[bytesMatched]) {
>                         bytesMatched++;
>                         if (bytesMatched == eAcuteBroken.length) {
>                             break;
>                         }
>                     } else {
>                         bytesMatched = 0;
>                     }
>                 }
>                 if (bytesMatched < eAcuteBroken.length) {
>                     throw new AssertionError("self-test failed: "
>                             + "multi-byte utf-8 character not broken across lines");
>                 }
>             }
>         }
>     }
>    
>     static OutputAnalyzer tool(String tool, String... args)
>             throws Throwable {
>         JDKToolLauncher l = JDKToolLauncher.createUsingTestJDK(tool);
>         for (String arg : args) {
>             if (arg .startsWith("-J")) {
>                 l.addVMArg(arg.substring(2));
>             } else {
>                 l.addToolArg(arg);
>             }
>         }
>         return ProcessTools.executeCommand(l.getCommand());
>     }
>    
> }
> END /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
>
>
> There are three e with acutes, on lines 84, 89, and 227, just in case the mail suffers bad encoding which might not be absolutely unconceivable regarding the bug's subject.
>
> In contrast to the bug description it uses a nested class with a two-byte UTF-8 character rather than one in its own file. I chose to do it that way because then the complete test goes into one file and therefore I assume the overview is kept easier. The test still demonstrates exactly JDK-6695402's problem.
>
> It may not have been necessary to also test jar files with indexes but i consider it necessary to have signing unsigned as well as partially signed jars tested, so why not have one more case tested, too?
>
> There might also be a more elegant approach to get class files into a jar file as to get their contents through the class loader. I chose to use real class files rather than dummy contents in files named *.class or for instance plain text files in the jar the signing of which to be tested in order to stay as close to the original bug problem as possible even though I don't have any notion that it would make a difference. The amount of code used to copy the class file contents around is comparatively small with respect to the whole test case amount of code.
>  
> For the demonstration that the multi-byte character actually makes the alleged difference, I concluded that it was necessary to have another class name not previously affected by the bug in order to compare the effects of just different names. Otherwise the signing could fail to any other reason undetectably.
>
> In order to express a condition to tell successful from failed test runs the best approach I found was to analyze the jarsigner tool's output which is in my opinion not the most desirable option. I'd have preferred output from a clearer structured api but then I guess the output format will not change too often in the foreseeable future. For instance the check for the s, m, and k flags is more pragmatical approach than obviously perfectly failsafe when operating with regular expressions to match it with the output. Other parts such as 'X.509' and 'CN=' are not even jarsigner tool resources. I put at least a reference to sun.security.tools.jarsigner.Resources.
>
> Finally, I afforded kind of a self-test that protects the test from undetected failing because renaming the main class which would move the two-byte unicode character to a position not suitable for the test. It may not be absolutely necessary.
>
>
>
> BEGIN PATH
> diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
> --- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
> +++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Wed Sep 20 00:56:15 2017 +0200
> @@ -28,6 +28,7 @@
>  import java.security.*;
>  import java.util.HashMap;
>  import java.io.ByteArrayOutputStream;
> +import static java.nio.charset.StandardCharsets.UTF_8;
>  
>  /**
>   * This class is used to compute digests on sections of the Manifest.
> @@ -112,7 +113,7 @@
>          rawBytes = bytes;
>          entries = new HashMap<>();
>  
> -        ByteArrayOutputStream baos = new ByteArrayOutputStream();
> +        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
>  
>          Position pos = new Position();
>  
> @@ -131,50 +132,40 @@
>  
>              if (len > 6) {
>                  if (isNameAttr(bytes, start)) {
> -                    StringBuilder nameBuf = new StringBuilder(sectionLen);
> +                    nameBuf.reset();
> +                    nameBuf.write(bytes, start+6, len-6);
>  
> -                    try {
> -                        nameBuf.append(
> -                            new String(bytes, start+6, len-6, "UTF8"));
> +                    int i = start + len;
> +                    if ((i-start) < sectionLen) {
> +                        if (bytes[i] == '\r') {
> +                            i += 2;
> +                        } else {
> +                            i += 1;
> +                        }
> +                    }
>  
> -                        int i = start + len;
> -                        if ((i-start) < sectionLen) {
> -                            if (bytes[i] == '\r') {
> -                                i += 2;
> -                            } else {
> -                                i += 1;
> -                            }
> +                    while ((i-start) < sectionLen) {
> +                        if (bytes[i++] == ' ') {
> +                            // name is wrapped
> +                            int wrapStart = i;
> +                            while (((i-start) < sectionLen)
> +                                    && (bytes[i++] != '\n'));
> +                            if (bytes[i-1] != '\n')
> +                                return; // XXX: exception?
> +                            int wrapLen;
> +                            if (bytes[i-2] == '\r')
> +                                wrapLen = i-wrapStart-2;
> +                            else
> +                                wrapLen = i-wrapStart-1;
> +
> +                            nameBuf.write(bytes, wrapStart, wrapLen);
> +                        } else {
> +                            break;
>                          }
> +                    }
>  
> -                        while ((i-start) < sectionLen) {
> -                            if (bytes[i++] == ' ') {
> -                                // name is wrapped
> -                                int wrapStart = i;
> -                                while (((i-start) < sectionLen)
> -                                        && (bytes[i++] != '\n'));
> -                                    if (bytes[i-1] != '\n')
> -                                        return; // XXX: exception?
> -                                    int wrapLen;
> -                                    if (bytes[i-2] == '\r')
> -                                        wrapLen = i-wrapStart-2;
> -                                    else
> -                                        wrapLen = i-wrapStart-1;
> -
> -                            nameBuf.append(new String(bytes, wrapStart,
> -                                                      wrapLen, "UTF8"));
> -                            } else {
> -                                break;
> -                            }
> -                        }
> -
> -                        entries.put(nameBuf.toString(),
> -                            new Entry(start, sectionLen, sectionLenWithBlank,
> -                                rawBytes));
> -
> -                    } catch (java.io.UnsupportedEncodingException uee) {
> -                        throw new IllegalStateException(
> -                            "UTF8 not available on platform");
> -                    }
> +                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
> +                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
>                  }
>              }
>              start = pos.startOfNext;
> END PATCH
>
>
> The patch is mostly so big because indentation has changed in many lines.
>
> There was baos there before without apparent use. The patch renames it and puts it into use. It came in very handy because if it would not have been there already, I would have had to defined one of my own.
>
> The main point of the patch is that manifest section names that are broken across lines at possibly arbitrary bytes are no longer converted into strings for each manifest line and then joined. Parts of names broken across lines are now joined first when they are still byte sequences and only when the complete byte sequence is available after processing all manifest lines that contain the same manifest section name decoded into a unicode string.
>
> For decoding the bytes into a string I chose a different string constructor than was used before that does not any longer declare UnsupportedEncodingException rendering the try/catch redundant. It couldn't have ever occurred anyway taking into consideration that UTF-8 is mandatory for every Java platform, StandardCharsets says. The difference according to the documentation is that the previously     used String constructor returned undefined strings for invalid byte sequences whereas the one used in the patch will replace unparseable portions with valid 'unknown' characters.
>
> One question I cannot still answer yet is how the ManifestDigester can be changed at all without complete test coverage or risking to break existing signatures. I already started on that but it's quite a piece of work to almost formally prove that all manifests will continue to produce identical digests, except of course for the ones now fixed.
>
> Regards,
> Philipp
>
>
> On 17.09.2017 21:25, Philipp Kunz wrote:
>> Hello Vincent
>>
>> I narrowed the error down so far and suspect now that it is an effect of sun.security.util.ManifestDigester's constructor, lines 134 and 163-164, in combination with java.util.jar.Manifest.make72Safe. Manifest breaks multi-byte characters in manifest section names across lines and ManifestDigester fails to restore them correctly, which both sounds wrong but ManifestDigester sure is.
>>
>> Just fixing it would be too tempting but then I would not know (I mean not know for sure with evidence from tests) if any existing jar-signature would still be valid and I don't want to break existing signatures. When I had a look at the tests specific for Manifest and ManifestDigester I found very little. There are many more different situations and corner cases and combinations thereof to cover with tests than it looks like at first glance so that writing tests that cover all relevant cases looks to me like quite an effort. I have the impression that test coverage was not a priority when the subjected code was developed or at least the tests might not have been contributed to the open JDK project. Hence, while I'm continuing to complete these tests I miss, if       someone has an idea how to simplify that so that I can still convince at least myself that no existing signature will break or where possibly more testcases are that I haven't discovered so far, please let me know.
>>
>> I'm also wondering how much performance should be taken into consideration. There are a few hints such as reusing byte arrays that suggest that it is an issue. Will a patch be rejected if it slows down some tests beyond a certain limit for so little added value or what might be the acceptance criteria here? I don't expect insuperable difficulties with performance but would still appreciate some general idea.
>>
>> My desired or currently preferred approach to fix JDK-6695402 would be to let ManifestDigester any kind of use or extend Manifest in order to let Manifest identify the manifest sections thereby preventing the parsing duplicated that identifies the manifest sections.
>>
>> As a new contributor I could use and would appreciate some guidance particularly about what kind and completeness of test coverage and performance tuning applies or is suggested in the context of the given bug. Otherwise I fear I might contribute too many tests which would have to be reviewed and maintained or quite some work would be for nothing if a patch would not satisfy performance requirements.
>>
>> Regards,
>> Philipp
>>
>>
>>
>> On 01.09.2017 15:20, Vincent Ryan wrote:
>>> That all sounds fine. Let me know when your patch is ready to submit.
>>> Thanks.
>>>
>>>
>>>> On 1 Sep 2017, at 13:15, Philipp Kunz <[hidden email]> wrote:
>>>>
>>>> Hello Vincent
>>>>
>>>> Thank you for sponsoring!
>>>> So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here: http://www.oracle.com/technetwork/community/oca-486395.html#p -> Paratix GmbH
>>>> Therefore I think I have followed the steps in http://openjdk.java.net/contribute/ at least the ones before actual patch submission.
>>>>
>>>> Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
>>>> make run-test-tier1
>>>> make run-test TEST="jdk/test"
>>>> they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
>>>> make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
>>>> then it reports zero errors and failures which may be a good starting point.
>>>> Do you think that sounds reasonable or do you have another suggestion how to run the tests?
>>>>
>>>> Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java. This way, I try to demonstrate the improvement.
>>>>
>>>> I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);
>>>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
>>>> So I'll try to remove it first including the whole four line if block.
>>>>
>>>> Philipp
>>>>
>>>>
>>>> On 01.09.2017 10:00, Vincent Ryan wrote:
>>>>> Hello Philipp,
>>>>>
>>>>> I’m happy to sponsor your fix for JDK 10. Have you followed these steps: http://openjdk.java.net/contribute/ ?
>>>>>
>>>>> Thanks.
>>>>>
>>>>>
>>>>>> On 1 Sep 2017, at 08:58, Vincent Ryan <[hidden email]> wrote:
>>>>>>
>>>>>> Moved to security-dev
>>>>>>
>>>>>>
>>>>>>> On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]> wrote:
>>>>>>>
>>>>>>> Hello everyone
>>>>>>>
>>>>>>> I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may not be too big a piece for a first contribution.
>>>>>>>
>>>>>>> I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:
>>>>>>>
>>>>>>> May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?
>>>>>>>
>>>>>>> Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.
>>>>>>>
>>>>>>> One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.
>>>>>>>
>>>>>>> I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.
>>>>>>>
>>>>>>> Philipp Kunz
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>>
>>
>>
>> <Mail Attachment.png>
>>
>> Paratix GmbH
>> St Peterhofstatt 11
>> 8001 Zürich
>>
>> +41 (0)76 397 79 35
>> [hidden email]
>

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Philipp Kunz
Hi Max and Vincent

Thank you for your suggestions. It sure looks better now. I hope this time I got the patch added in the right format.

diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
--- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
+++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Sun Sep 24 10:34:00 2017 +0200
@@ -28,6 +28,7 @@
 import java.security.*;
 import java.util.HashMap;
 import java.io.ByteArrayOutputStream;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 /**
  * This class is used to compute digests on sections of the Manifest.
@@ -112,7 +113,7 @@
         rawBytes = bytes;
         entries = new HashMap<>();
 
-        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
 
         Position pos = new Position();
 
@@ -131,50 +132,40 @@
 
             if (len > 6) {
                 if (isNameAttr(bytes, start)) {
-                    StringBuilder nameBuf = new StringBuilder(sectionLen);
+                    nameBuf.reset();
+                    nameBuf.write(bytes, start+6, len-6);
 
-                    try {
-                        nameBuf.append(
-                            new String(bytes, start+6, len-6, "UTF8"));
+                    int i = start + len;
+                    if ((i-start) < sectionLen) {
+                        if (bytes[i] == '\r') {
+                            i += 2;
+                        } else {
+                            i += 1;
+                        }
+                    }
 
-                        int i = start + len;
-                        if ((i-start) < sectionLen) {
-                            if (bytes[i] == '\r') {
-                                i += 2;
-                            } else {
-                                i += 1;
-                            }
+                    while ((i-start) < sectionLen) {
+                        if (bytes[i++] == ' ') {
+                            // name is wrapped
+                            int wrapStart = i;
+                            while (((i-start) < sectionLen)
+                                    && (bytes[i++] != '\n'));
+                            if (bytes[i-1] != '\n')
+                                return; // XXX: exception?
+                            int wrapLen;
+                            if (bytes[i-2] == '\r')
+                                wrapLen = i-wrapStart-2;
+                            else
+                                wrapLen = i-wrapStart-1;
+
+                            nameBuf.write(bytes, wrapStart, wrapLen);
+                        } else {
+                            break;
                         }
+                    }
 
-                        while ((i-start) < sectionLen) {
-                            if (bytes[i++] == ' ') {
-                                // name is wrapped
-                                int wrapStart = i;
-                                while (((i-start) < sectionLen)
-                                        && (bytes[i++] != '\n'));
-                                    if (bytes[i-1] != '\n')
-                                        return; // XXX: exception?
-                                    int wrapLen;
-                                    if (bytes[i-2] == '\r')
-                                        wrapLen = i-wrapStart-2;
-                                    else
-                                        wrapLen = i-wrapStart-1;
-
-                            nameBuf.append(new String(bytes, wrapStart,
-                                                      wrapLen, "UTF8"));
-                            } else {
-                                break;
-                            }
-                        }
-
-                        entries.put(nameBuf.toString(),
-                            new Entry(start, sectionLen, sectionLenWithBlank,
-                                rawBytes));
-
-                    } catch (java.io.UnsupportedEncodingException uee) {
-                        throw new IllegalStateException(
-                            "UTF8 not available on platform");
-                    }
+                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
+                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
                 }
             }
             start = pos.startOfNext;
diff -r ddc25f646c2e test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
--- /dev/null    Thu Jan 01 00:00:00 1970 +0000
+++ b/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java    Sun Sep 24 10:34:00 2017 +0200
@@ -0,0 +1,209 @@
+/*
+ * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug JDK-6695402
+ * @summary verify signatures of jars containing classes with names with multi-
+ *          byte unicode characters broken across lines
+ * @library /test/lib
+ * @modules jdk.jartool/sun.tools.jar
+ *          jdk.jartool/sun.security.tools.jarsigner
+ * @run main MultibyteUnicodeName
+ */
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.jar.JarFile;
+import java.util.jar.Manifest;
+import java.util.Arrays;
+import java.util.Map;
+import java.util.jar.Attributes.Name;
+import java.util.jar.JarEntry;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import static jdk.test.lib.SecurityTools.jarsigner;
+import static jdk.test.lib.SecurityTools.keytool;
+import static jdk.test.lib.util.JarUtils.createJar;
+import static jdk.test.lib.util.JarUtils.updateJar;
+
+public class MultibyteUnicodeName {
+
+    /**
+     * this class's name will break across lines in the jar manifest at the middle of
+     * a two-byte utf encoded character due to its e acute letter at its exact position.
+     *
+     * @see also eAcute in {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}
+     */
+    static class A1234567890B1234567890C1234567890D12345678éxyz { }
+    static class A1234567890B1234567890C1234567890D12345678exyz { }
+
+    static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() +
+            "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
+    static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() +
+            "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
+   
+    static final String alias = "a";
+    static final String keystoreFileName = "test.jks";
+    static final String ManifestFileName = "MANIFEST.MF";
+   
+    public static void main(String[] args) throws Exception {
+        try {
+            prepare();
+           
+            testSignJar("test.jar");
+            testSignJarNoManifest("test-no-manifest.jar");
+            testSignJarUpdate("test-update.jar", "test-updated.jar");
+           
+        } finally {
+            Files.deleteIfExists(Paths.get(keystoreFileName));
+            Files.deleteIfExists(Paths.get(ManifestFileName));
+            Files.deleteIfExists(Paths.get(refClassFilename));
+            Files.deleteIfExists(Paths.get(testClassFilename));
+        }
+    }
+   
+    static void prepare() throws Exception {
+        keytool("-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
+                "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
+                "-validity", "366").shouldHaveExitValue(0);
+       
+        Files.write(Paths.get(ManifestFileName),
+                (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
+        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(refClassFilename),
+                Paths.get(refClassFilename));
+        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(testClassFilename),
+                Paths.get(testClassFilename));
+    }
+   
+    static void testSignJar(String jarFileName) throws Exception {
+        try {
+            createJar(jarFileName, ManifestFileName, refClassFilename, testClassFilename);
+            verifyJarSignature(jarFileName);
+
+        } finally {
+            Files.deleteIfExists(Paths.get(jarFileName));
+        }
+    }
+
+    static void testSignJarNoManifest(String jarFileName) throws Exception {
+        try {
+            createJar(jarFileName, refClassFilename, testClassFilename);
+            verifyJarSignature(jarFileName);
+
+        } finally {
+            Files.deleteIfExists(Paths.get(jarFileName));
+        }
+    }
+   
+    static void testSignJarUpdate(String initialFileName, String updatedFileName) throws Exception {
+        try {
+            createJar(initialFileName, ManifestFileName, refClassFilename);
+            jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
+                    "-storepass", "changeit", "-debug", initialFileName, alias)
+                .shouldHaveExitValue(0);
+            updateJar(initialFileName, updatedFileName, testClassFilename);
+            verifyJarSignature(updatedFileName);
+
+        } finally {
+            Files.deleteIfExists(Paths.get(initialFileName));
+            Files.deleteIfExists(Paths.get(updatedFileName));
+        }
+    }
+   
+    static void verifyJarSignature(String jarFileName) throws Exception {
+        // actually sign the jar
+        jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
+                "-storepass", "changeit", "-debug", jarFileName, alias)
+                .shouldHaveExitValue(0);
+
+        try (
+            JarFile jar = new JarFile(jarFileName);
+        ) {
+            verifyClassNameLineBroken(jar, testClassFilename);
+            verifyCodeSigners(jar, jar.getJarEntry(testClassFilename));
+            verifyCodeSigners(jar, jar.getJarEntry(refClassFilename));
+        }
+    }
+   
+    /**
+     * it would be too easy to miss the actual test case by just renaming an identifier so that
+     * the multi-byte encoded character would not any longer be broken across a line break.
+     *
+     * this test verifies that the actual test case is tested based on the manifest
+     * and not based on the signature file because at the moment, the signature file
+     * does not even contain the desired entry at all.
+     *
+     * this relies on the Manifest breaking lines unaware of bytes that belong to the same
+     * multi-ybte utf characters.
+     */
+    static void verifyClassNameLineBroken(JarFile jar, String className) throws IOException {
+        byte[] eAcute = "é".getBytes(UTF_8);
+        byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
+       
+        if (jar.getManifest().getAttributes(className) == null) {
+            throw new AssertionError(className + " not found in manifest");
+        }
+       
+        JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
+        try (
+            InputStream manifestIs = jar.getInputStream(manifestEntry);
+        ) {
+            int bytesMatched = 0;
+            for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
+                if ((byte) b == eAcuteBroken[bytesMatched]) {
+                    bytesMatched++;
+                    if (bytesMatched == eAcuteBroken.length) {
+                        break;
+                    }
+                } else {
+                    bytesMatched = 0;
+                }
+            }
+            if (bytesMatched < eAcuteBroken.length) {
+                throw new AssertionError("self-test failed: "
+                        + "multi-byte utf-8 character not broken across lines");
+            }
+        }
+    }
+   
+    static void verifyCodeSigners(JarFile jar, JarEntry jarEntry) throws IOException {
+        // codeSigners is initialized only after the entry has been read
+        try (
+            InputStream inputStream = jar.getInputStream(jarEntry);
+        ) {
+            while (inputStream.read() > -1);
+           
+            if (jarEntry.getCodeSigners() == null || jarEntry.getCodeSigners().length == 0) {
+                throw new AssertionError("no signing certificate found for " + jarEntry.getName());
+            }
+        }
+       
+        // a check for the presence of code signers is sufficient to check bug JDK-6695402.
+        // no need to also verify the actual code signers attributes here.
+    }
+   
+}

Regards,
Philipp



On 20.09.2017 03:41, Weijun Wang wrote:
Hi Philipp

The change mostly looks fine. You might want to put everything into a patch file so Vincent can recreate a webrev and post it to cr.openjdk.java.net.

One thing I would suggest for the test is that instead of using jarsigner -verify and check the text in output you can open it as a JarFile, consume the content of an entry, and look into its getCertificates().

Also, you might want to reuse test/lib/jdk/test/lib/SecurityTools.java, and there is also JarUtils.java you can use. Maybe Files.copy(InputStream,Path) can be used in copyClassToFile().

Thanks
Max

On Sep 20, 2017, at 7:41 AM, Philipp Kunz [hidden email] wrote:

Hello Vincent

Here may be the fix for JDK-6695402. First a test.

BEGIN /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
/*
 * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/*
 * @test
 * @bug JDK-6695402
 * @summary verify signatures of jars containing classes with names with multi-
 *          byte unicode characters broken across lines
 * @library /test/lib
 * @modules jdk.jartool/sun.tools.jar
 *          jdk.jartool/sun.security.tools.jarsigner
 * @build jdk.test.lib.JDKToolLauncher
 *        jdk.test.lib.process.*
 * @run main MultibyteUnicodeName
 */

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.jar.JarFile;
import java.util.jar.Manifest;
import java.util.stream.Collectors;
import java.util.Arrays;
import java.util.Map;
import java.util.jar.Attributes.Name;
import java.util.jar.JarEntry;

import static java.nio.charset.StandardCharsets.UTF_8;

import sun.security.tools.jarsigner.Resources;

import jdk.test.lib.JDKToolLauncher;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.process.ProcessTools;

public class MultibyteUnicodeName {
    
    public static void main(String[] args) throws Throwable {
        try {
            prepare();
            
            testSignJar("test.jar");
            testSignJarNoManifest("test-no-manifest.jar");
            testSignJarUpdate("test-update.jar");
            testSignJarWithIndex("test-index.jar");
            testSignJarAddIndex("test-add-index.jar");
            
        } finally {
            Files.deleteIfExists(Paths.get(keystoreFileName));
            Files.deleteIfExists(Paths.get(ManifestFileName));
            Files.deleteIfExists(Paths.get(refClassFilename));
            Files.deleteIfExists(Paths.get(testClassFilename));
        }
    }
    
    static final String alias = "a";
    static final String keystoreFileName = "test.jks";
    static final String ManifestFileName = "MANIFEST.MF";
    
    static class A1234567890B1234567890C1234567890D12345678exyz { }
    static class A1234567890B1234567890C1234567890D12345678éxyz { }
    
    static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() + 
            "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
    static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() + 
            "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
    
    static void prepare() throws Throwable {
        tool("keytool", "-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
                "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
                "-validity", "366")
                .shouldHaveExitValue(0);
        
        Files.write(Paths.get(ManifestFileName),
                (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
        copyClassToFile(refClassFilename);
        copyClassToFile(testClassFilename);
    }
    
    static void copyClassToFile(String classFilename) throws IOException {
        try (
            InputStream asStream = MultibyteUnicodeName.class.getResourceAsStream(classFilename);
            ByteArrayOutputStream buf = new ByteArrayOutputStream();
        ) {
            int b;
            while ((b = asStream.read()) != -1) buf.write(b);
            Files.write(Paths.get(classFilename), buf.toByteArray());
        }
    }

    static void testSignJar(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvfm", jarFileName, 
                    ManifestFileName, refClassFilename, testClassFilename)
                    .shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }

    static void testSignJarNoManifest(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvf", jarFileName, refClassFilename, testClassFilename)
                .shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }
    
    static void testSignJarUpdate(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvfm", jarFileName, ManifestFileName, refClassFilename)
                .shouldHaveExitValue(0);
            tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
                    "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0);
            tool("jar", "uvf", jarFileName, testClassFilename)
                .shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }
    
    static void testSignJarWithIndex(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvfm", jarFileName, 
                    ManifestFileName, refClassFilename, testClassFilename)
                .shouldHaveExitValue(0);
            tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }

    static void testSignJarAddIndex(String jarFileName) throws Throwable {
        try {
            tool("jar", "cvfm", jarFileName, 
                    ManifestFileName, refClassFilename, testClassFilename)
                .shouldHaveExitValue(0);
            tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
                    "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0);
            tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
            verifyJarSignature(jarFileName);

        } finally {
            Files.deleteIfExists(Paths.get(jarFileName));
        }
    }
    
    static Map<String, String> jarsignerResources = Arrays.stream(new Resources().getContents()).
            collect(Collectors.toMap(e -> (String) e[0], e -> (String) e[1]));
            
    static void verifyJarSignature(String jarFileName) throws Throwable {
        // actually sign the jar
        tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
                "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0);
        
        verifyClassNameLineBroken(jarFileName);
        
        // check for no unsigned entries
        tool("jarsigner", "-verify", "-keystore", keystoreFileName, "-storetype", "JKS",
                "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0)
                .shouldNotContain(jarsignerResources.get(
                        "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));
        
        // check that both classes with and without multi-byte unicode characters in their names are signed
        tool("jarsigner", "-verify", "-verbose", "-keystore", keystoreFileName, "-storetype", "JKS",
                "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0)
                .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + refClassFilename.replaceAll("\\$", "\\\\\\$") + "$")
                .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + testClassFilename.replaceAll("\\$", "\\\\\\$") + "$");
        
        // check that both classes with and without multi-byte unicode characters in their names have signing certificates
        tool("jarsigner", "-verify", "-verbose", "-certs", "-keystore", keystoreFileName, 
                "-storetype", "JKS", "-storepass", "changeit", "-debug", jarFileName, alias)
                .shouldHaveExitValue(0)
                .shouldMatch(refClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)")
                .shouldMatch(testClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)");
    }
    
    /**
     * it would be too easy to miss the actual test case by just renaming an identifier so that
     * the multi-byte encoded character would not any longer be broken across a line break.
     *
     * this test verifies that the actual test case is tested based on the manifest
     * and not based on the signature file because at the moment, the signature file
     * does not even contain the desired entry at all.
     *
     * this relies on the Manifest breaking lines unaware of bytes that belong to the same
     * multi-ybte utf characters.
     */
    static void verifyClassNameLineBroken(String jarFileName) throws IOException {
        byte[] eAcute = "é".getBytes(UTF_8);
        byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
        
        try (
            JarFile jar = new JarFile(jarFileName);
        ) {
            if (jar.getManifest().getAttributes(testClassFilename) == null) {
                throw new AssertionError(testClassFilename + " not found in manifest");
            }
            
            JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
            try (
                InputStream manifestIs = jar.getInputStream(manifestEntry);
            ) {
                int bytesMatched = 0;
                for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
                    if ((byte) b == eAcuteBroken[bytesMatched]) {
                        bytesMatched++;
                        if (bytesMatched == eAcuteBroken.length) {
                            break;
                        }
                    } else {
                        bytesMatched = 0;
                    }
                }
                if (bytesMatched < eAcuteBroken.length) {
                    throw new AssertionError("self-test failed: "
                            + "multi-byte utf-8 character not broken across lines");
                }
            }
        }
    }
    
    static OutputAnalyzer tool(String tool, String... args)
            throws Throwable {
        JDKToolLauncher l = JDKToolLauncher.createUsingTestJDK(tool);
        for (String arg : args) {
            if (arg .startsWith("-J")) {
                l.addVMArg(arg.substring(2));
            } else {
                l.addToolArg(arg);
            }
        }
        return ProcessTools.executeCommand(l.getCommand());
    }
    
}
END /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java


There are three e with acutes, on lines 84, 89, and 227, just in case the mail suffers bad encoding which might not be absolutely unconceivable regarding the bug's subject.

In contrast to the bug description it uses a nested class with a two-byte UTF-8 character rather than one in its own file. I chose to do it that way because then the complete test goes into one file and therefore I assume the overview is kept easier. The test still demonstrates exactly JDK-6695402's problem.

It may not have been necessary to also test jar files with indexes but i consider it necessary to have signing unsigned as well as partially signed jars tested, so why not have one more case tested, too?

There might also be a more elegant approach to get class files into a jar file as to get their contents through the class loader. I chose to use real class files rather than dummy contents in files named *.class or for instance plain text files in the jar the signing of which to be tested in order to stay as close to the original bug problem as possible even though I don't have any notion that it would make a difference. The amount of code used to copy the class file contents around is comparatively small with respect to the whole test case amount of code.
  
For the demonstration that the multi-byte character actually makes the alleged difference, I concluded that it was necessary to have another class name not previously affected by the bug in order to compare the effects of just different names. Otherwise the signing could fail to any other reason undetectably.

In order to express a condition to tell successful from failed test runs the best approach I found was to analyze the jarsigner tool's output which is in my opinion not the most desirable option. I'd have preferred output from a clearer structured api but then I guess the output format will not change too often in the foreseeable future. For instance the check for the s, m, and k flags is more pragmatical approach than obviously perfectly failsafe when operating with regular expressions to match it with the output. Other parts such as 'X.509' and 'CN=' are not even jarsigner tool resources. I put at least a reference to sun.security.tools.jarsigner.Resources.

Finally, I afforded kind of a self-test that protects the test from undetected failing because renaming the main class which would move the two-byte unicode character to a position not suitable for the test. It may not be absolutely necessary.



BEGIN PATH
diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
--- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
+++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Wed Sep 20 00:56:15 2017 +0200
@@ -28,6 +28,7 @@
 import java.security.*;
 import java.util.HashMap;
 import java.io.ByteArrayOutputStream;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 /**
  * This class is used to compute digests on sections of the Manifest.
@@ -112,7 +113,7 @@
         rawBytes = bytes;
         entries = new HashMap<>();
 
-        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
 
         Position pos = new Position();
 
@@ -131,50 +132,40 @@
 
             if (len > 6) {
                 if (isNameAttr(bytes, start)) {
-                    StringBuilder nameBuf = new StringBuilder(sectionLen);
+                    nameBuf.reset();
+                    nameBuf.write(bytes, start+6, len-6);
 
-                    try {
-                        nameBuf.append(
-                            new String(bytes, start+6, len-6, "UTF8"));
+                    int i = start + len;
+                    if ((i-start) < sectionLen) {
+                        if (bytes[i] == '\r') {
+                            i += 2;
+                        } else {
+                            i += 1;
+                        }
+                    }
 
-                        int i = start + len;
-                        if ((i-start) < sectionLen) {
-                            if (bytes[i] == '\r') {
-                                i += 2;
-                            } else {
-                                i += 1;
-                            }
+                    while ((i-start) < sectionLen) {
+                        if (bytes[i++] == ' ') {
+                            // name is wrapped
+                            int wrapStart = i;
+                            while (((i-start) < sectionLen)
+                                    && (bytes[i++] != '\n'));
+                            if (bytes[i-1] != '\n')
+                                return; // XXX: exception?
+                            int wrapLen;
+                            if (bytes[i-2] == '\r')
+                                wrapLen = i-wrapStart-2;
+                            else
+                                wrapLen = i-wrapStart-1;
+
+                            nameBuf.write(bytes, wrapStart, wrapLen);
+                        } else {
+                            break;
                         }
+                    }
 
-                        while ((i-start) < sectionLen) {
-                            if (bytes[i++] == ' ') {
-                                // name is wrapped
-                                int wrapStart = i;
-                                while (((i-start) < sectionLen)
-                                        && (bytes[i++] != '\n'));
-                                    if (bytes[i-1] != '\n')
-                                        return; // XXX: exception?
-                                    int wrapLen;
-                                    if (bytes[i-2] == '\r')
-                                        wrapLen = i-wrapStart-2;
-                                    else
-                                        wrapLen = i-wrapStart-1;
-
-                            nameBuf.append(new String(bytes, wrapStart,
-                                                      wrapLen, "UTF8"));
-                            } else {
-                                break;
-                            }
-                        }
-
-                        entries.put(nameBuf.toString(),
-                            new Entry(start, sectionLen, sectionLenWithBlank,
-                                rawBytes));
-
-                    } catch (java.io.UnsupportedEncodingException uee) {
-                        throw new IllegalStateException(
-                            "UTF8 not available on platform");
-                    }
+                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
+                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
                 }
             }
             start = pos.startOfNext;
END PATCH


The patch is mostly so big because indentation has changed in many lines.

There was baos there before without apparent use. The patch renames it and puts it into use. It came in very handy because if it would not have been there already, I would have had to defined one of my own.

The main point of the patch is that manifest section names that are broken across lines at possibly arbitrary bytes are no longer converted into strings for each manifest line and then joined. Parts of names broken across lines are now joined first when they are still byte sequences and only when the complete byte sequence is available after processing all manifest lines that contain the same manifest section name decoded into a unicode string.

For decoding the bytes into a string I chose a different string constructor than was used before that does not any longer declare UnsupportedEncodingException rendering the try/catch redundant. It couldn't have ever occurred anyway taking into consideration that UTF-8 is mandatory for every Java platform, StandardCharsets says. The difference according to the documentation is that the previously     used String constructor returned undefined strings for invalid byte sequences whereas the one used in the patch will replace unparseable portions with valid 'unknown' characters.

One question I cannot still answer yet is how the ManifestDigester can be changed at all without complete test coverage or risking to break existing signatures. I already started on that but it's quite a piece of work to almost formally prove that all manifests will continue to produce identical digests, except of course for the ones now fixed.

Regards,
Philipp


On 17.09.2017 21:25, Philipp Kunz wrote:
Hello Vincent

I narrowed the error down so far and suspect now that it is an effect of sun.security.util.ManifestDigester's constructor, lines 134 and 163-164, in combination with java.util.jar.Manifest.make72Safe. Manifest breaks multi-byte characters in manifest section names across lines and ManifestDigester fails to restore them correctly, which both sounds wrong but ManifestDigester sure is.

Just fixing it would be too tempting but then I would not know (I mean not know for sure with evidence from tests) if any existing jar-signature would still be valid and I don't want to break existing signatures. When I had a look at the tests specific for Manifest and ManifestDigester I found very little. There are many more different situations and corner cases and combinations thereof to cover with tests than it looks like at first glance so that writing tests that cover all relevant cases looks to me like quite an effort. I have the impression that test coverage was not a priority when the subjected code was developed or at least the tests might not have been contributed to the open JDK project. Hence, while I'm continuing to complete these tests I miss, if       someone has an idea how to simplify that so that I can still convince at least myself that no existing signature will break or where possibly more testcases are that I haven't discovered so far, please let me know.

I'm also wondering how much performance should be taken into consideration. There are a few hints such as reusing byte arrays that suggest that it is an issue. Will a patch be rejected if it slows down some tests beyond a certain limit for so little added value or what might be the acceptance criteria here? I don't expect insuperable difficulties with performance but would still appreciate some general idea.

My desired or currently preferred approach to fix JDK-6695402 would be to let ManifestDigester any kind of use or extend Manifest in order to let Manifest identify the manifest sections thereby preventing the parsing duplicated that identifies the manifest sections.

As a new contributor I could use and would appreciate some guidance particularly about what kind and completeness of test coverage and performance tuning applies or is suggested in the context of the given bug. Otherwise I fear I might contribute too many tests which would have to be reviewed and maintained or quite some work would be for nothing if a patch would not satisfy performance requirements.

Regards,
Philipp



On 01.09.2017 15:20, Vincent Ryan wrote:
That all sounds fine. Let me know when your patch is ready to submit.
Thanks.


On 1 Sep 2017, at 13:15, Philipp Kunz [hidden email] wrote:

Hello Vincent

Thank you for sponsoring!
So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here: http://www.oracle.com/technetwork/community/oca-486395.html#p -> Paratix GmbH
Therefore I think I have followed the steps in http://openjdk.java.net/contribute/ at least the ones before actual patch submission.

Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
make run-test-tier1
make run-test TEST="jdk/test"
they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
then it reports zero errors and failures which may be a good starting point.
Do you think that sounds reasonable or do you have another suggestion how to run the tests?

Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java. This way, I try to demonstrate the improvement.

I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);
http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
So I'll try to remove it first including the whole four line if block.

Philipp


On 01.09.2017 10:00, Vincent Ryan wrote:
Hello Philipp,

I’m happy to sponsor your fix for JDK 10. Have you followed these steps: http://openjdk.java.net/contribute/ ?

Thanks.


On 1 Sep 2017, at 08:58, Vincent Ryan [hidden email] wrote:

Moved to security-dev


On 1 Sep 2017, at 08:28, Philipp Kunz [hidden email] wrote:

Hello everyone

I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: https://bugs.openjdk.java.net/browse/JDK-6695402. This also looks like it may not be too big a piece for a first contribution.

I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:

May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?

Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.

One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.

I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.

Philipp Kunz

                

              

            

          




<Mail Attachment.png>

Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]

      

    

--


Gruss Philipp







Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang
Great.

Some suggestions, just my own habit, you are free to decide yourself:

1. In ManifestDigester.java, I'd rather define nameBuf inside the if (isNameAttr(bytes, start)) block.

2. I normally don't use "import static" unless I can save a lot of keystrokes and still do not confuse anyone. Both in the test and in ManifestDigester.java.

3. In the test, there is no need to write "@run main MultibyteUnicodeName" if it is the only action (i.e. no other build/compile) and there is no modifier on main (i.e. no othervm/timeout etc).

Several tiny problems:

1. No need for @modules in the test. Maybe you used some internal classes in your previous version?

2. In the new consolidated repo, the test should be inside test/jdk/sun/..., please note the "jdk" after "test".

3. Please update the copyright years.

4. In the test, there should be no "JDK-" in the @test tag.

Thanks
Max

> On Sep 24, 2017, at 7:51 PM, Philipp Kunz <[hidden email]> wrote:
>
> Hi Max and Vincent
>
> Thank you for your suggestions. It sure looks better now. I hope this time I got the patch added in the right format.
>
> diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
> --- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
> +++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Sun Sep 24 10:34:00 2017 +0200
> @@ -28,6 +28,7 @@
>  import java.security.*;
>  import java.util.HashMap;
>  import java.io.ByteArrayOutputStream;
> +import static java.nio.charset.StandardCharsets.UTF_8;
>  
>  /**
>   * This class is used to compute digests on sections of the Manifest.
> @@ -112,7 +113,7 @@
>          rawBytes = bytes;
>          entries = new HashMap<>();
>  
> -        ByteArrayOutputStream baos = new ByteArrayOutputStream();
> +        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
>  
>          Position pos = new Position();
>  
> @@ -131,50 +132,40 @@
>  
>              if (len > 6) {
>                  if (isNameAttr(bytes, start)) {
> -                    StringBuilder nameBuf = new StringBuilder(sectionLen);
> +                    nameBuf.reset();
> +                    nameBuf.write(bytes, start+6, len-6);
>  
> -                    try {
> -                        nameBuf.append(
> -                            new String(bytes, start+6, len-6, "UTF8"));
> +                    int i = start + len;
> +                    if ((i-start) < sectionLen) {
> +                        if (bytes[i] == '\r') {
> +                            i += 2;
> +                        } else {
> +                            i += 1;
> +                        }
> +                    }
>  
> -                        int i = start + len;
> -                        if ((i-start) < sectionLen) {
> -                            if (bytes[i] == '\r') {
> -                                i += 2;
> -                            } else {
> -                                i += 1;
> -                            }
> +                    while ((i-start) < sectionLen) {
> +                        if (bytes[i++] == ' ') {
> +                            // name is wrapped
> +                            int wrapStart = i;
> +                            while (((i-start) < sectionLen)
> +                                    && (bytes[i++] != '\n'));
> +                            if (bytes[i-1] != '\n')
> +                                return; // XXX: exception?
> +                            int wrapLen;
> +                            if (bytes[i-2] == '\r')
> +                                wrapLen = i-wrapStart-2;
> +                            else
> +                                wrapLen = i-wrapStart-1;
> +
> +                            nameBuf.write(bytes, wrapStart, wrapLen);
> +                        } else {
> +                            break;
>                          }
> +                    }
>  
> -                        while ((i-start) < sectionLen) {
> -                            if (bytes[i++] == ' ') {
> -                                // name is wrapped
> -                                int wrapStart = i;
> -                                while (((i-start) < sectionLen)
> -                                        && (bytes[i++] != '\n'));
> -                                    if (bytes[i-1] != '\n')
> -                                        return; // XXX: exception?
> -                                    int wrapLen;
> -                                    if (bytes[i-2] == '\r')
> -                                        wrapLen = i-wrapStart-2;
> -                                    else
> -                                        wrapLen = i-wrapStart-1;
> -
> -                            nameBuf.append(new String(bytes, wrapStart,
> -                                                      wrapLen, "UTF8"));
> -                            } else {
> -                                break;
> -                            }
> -                        }
> -
> -                        entries.put(nameBuf.toString(),
> -                            new Entry(start, sectionLen, sectionLenWithBlank,
> -                                rawBytes));
> -
> -                    } catch (java.io.UnsupportedEncodingException uee) {
> -                        throw new IllegalStateException(
> -                            "UTF8 not available on platform");
> -                    }
> +                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
> +                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
>                  }
>              }
>              start = pos.startOfNext;
> diff -r ddc25f646c2e test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
> +++ b/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java    Sun Sep 24 10:34:00 2017 +0200
> @@ -0,0 +1,209 @@
> +/*
> + * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
> + *
> + * This code is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 only, as
> + * published by the Free Software Foundation.
> + *
> + * This code is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> + * version 2 for more details (a copy is included in the LICENSE file that
> + * accompanied this code).
> + *
> + * You should have received a copy of the GNU General Public License version
> + * 2 along with this work; if not, write to the Free Software Foundation,
> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
> + * or visit www.oracle.com if you need additional information or have any
> + * questions.
> + */
> +
> +/*
> + * @test
> + * @bug JDK-6695402
> + * @summary verify signatures of jars containing classes with names with multi-
> + *          byte unicode characters broken across lines
> + * @library /test/lib
> + * @modules jdk.jartool/sun.tools.jar
> + *          jdk.jartool/sun.security.tools.jarsigner
> + * @run main MultibyteUnicodeName
> + */
> +
> +import java.io.IOException;
> +import java.io.InputStream;
> +import java.nio.file.Files;
> +import java.nio.file.Paths;
> +import java.util.jar.JarFile;
> +import java.util.jar.Manifest;
> +import java.util.Arrays;
> +import java.util.Map;
> +import java.util.jar.Attributes.Name;
> +import java.util.jar.JarEntry;
> +
> +import static java.nio.charset.StandardCharsets.UTF_8;
> +
> +import static jdk.test.lib.SecurityTools.jarsigner;
> +import static jdk.test.lib.SecurityTools.keytool;
> +import static jdk.test.lib.util.JarUtils.createJar;
> +import static jdk.test.lib.util.JarUtils.updateJar;
> +
> +public class MultibyteUnicodeName {
> +
> +    /**
> +     * this class's name will break across lines in the jar manifest at the middle of
> +     * a two-byte utf encoded character due to its e acute letter at its exact position.
> +     *
> +     * @see also eAcute in {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}
> +     */
> +    static class A1234567890B1234567890C1234567890D12345678éxyz { }
> +    static class A1234567890B1234567890C1234567890D12345678exyz { }
> +
> +    static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() +
> +            "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
> +    static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() +
> +            "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
> +    
> +    static final String alias = "a";
> +    static final String keystoreFileName = "test.jks";
> +    static final String ManifestFileName = "MANIFEST.MF";
> +    
> +    public static void main(String[] args) throws Exception {
> +        try {
> +            prepare();
> +            
> +            testSignJar("test.jar");
> +            testSignJarNoManifest("test-no-manifest.jar");
> +            testSignJarUpdate("test-update.jar", "test-updated.jar");
> +            
> +        } finally {
> +            Files.deleteIfExists(Paths.get(keystoreFileName));
> +            Files.deleteIfExists(Paths.get(ManifestFileName));
> +            Files.deleteIfExists(Paths.get(refClassFilename));
> +            Files.deleteIfExists(Paths.get(testClassFilename));
> +        }
> +    }
> +    
> +    static void prepare() throws Exception {
> +        keytool("-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
> +                "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
> +                "-validity", "366").shouldHaveExitValue(0);
> +        
> +        Files.write(Paths.get(ManifestFileName),
> +                (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
> +        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(refClassFilename),
> +                Paths.get(refClassFilename));
> +        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(testClassFilename),
> +                Paths.get(testClassFilename));
> +    }
> +    
> +    static void testSignJar(String jarFileName) throws Exception {
> +        try {
> +            createJar(jarFileName, ManifestFileName, refClassFilename, testClassFilename);
> +            verifyJarSignature(jarFileName);
> +
> +        } finally {
> +            Files.deleteIfExists(Paths.get(jarFileName));
> +        }
> +    }
> +
> +    static void testSignJarNoManifest(String jarFileName) throws Exception {
> +        try {
> +            createJar(jarFileName, refClassFilename, testClassFilename);
> +            verifyJarSignature(jarFileName);
> +
> +        } finally {
> +            Files.deleteIfExists(Paths.get(jarFileName));
> +        }
> +    }
> +    
> +    static void testSignJarUpdate(String initialFileName, String updatedFileName) throws Exception {
> +        try {
> +            createJar(initialFileName, ManifestFileName, refClassFilename);
> +            jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
> +                    "-storepass", "changeit", "-debug", initialFileName, alias)
> +                .shouldHaveExitValue(0);
> +            updateJar(initialFileName, updatedFileName, testClassFilename);
> +            verifyJarSignature(updatedFileName);
> +
> +        } finally {
> +            Files.deleteIfExists(Paths.get(initialFileName));
> +            Files.deleteIfExists(Paths.get(updatedFileName));
> +        }
> +    }
> +    
> +    static void verifyJarSignature(String jarFileName) throws Exception {
> +        // actually sign the jar
> +        jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
> +                "-storepass", "changeit", "-debug", jarFileName, alias)
> +                .shouldHaveExitValue(0);
> +
> +        try (
> +            JarFile jar = new JarFile(jarFileName);
> +        ) {
> +            verifyClassNameLineBroken(jar, testClassFilename);
> +            verifyCodeSigners(jar, jar.getJarEntry(testClassFilename));
> +            verifyCodeSigners(jar, jar.getJarEntry(refClassFilename));
> +        }
> +    }
> +    
> +    /**
> +     * it would be too easy to miss the actual test case by just renaming an identifier so that
> +     * the multi-byte encoded character would not any longer be broken across a line break.
> +     *
> +     * this test verifies that the actual test case is tested based on the manifest
> +     * and not based on the signature file because at the moment, the signature file
> +     * does not even contain the desired entry at all.
> +     *
> +     * this relies on the Manifest breaking lines unaware of bytes that belong to the same
> +     * multi-ybte utf characters.
> +     */
> +    static void verifyClassNameLineBroken(JarFile jar, String className) throws IOException {
> +        byte[] eAcute = "é".getBytes(UTF_8);
> +        byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
> +        
> +        if (jar.getManifest().getAttributes(className) == null) {
> +            throw new AssertionError(className + " not found in manifest");
> +        }
> +        
> +        JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
> +        try (
> +            InputStream manifestIs = jar.getInputStream(manifestEntry);
> +        ) {
> +            int bytesMatched = 0;
> +            for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
> +                if ((byte) b == eAcuteBroken[bytesMatched]) {
> +                    bytesMatched++;
> +                    if (bytesMatched == eAcuteBroken.length) {
> +                        break;
> +                    }
> +                } else {
> +                    bytesMatched = 0;
> +                }
> +            }
> +            if (bytesMatched < eAcuteBroken.length) {
> +                throw new AssertionError("self-test failed: "
> +                        + "multi-byte utf-8 character not broken across lines");
> +            }
> +        }
> +    }
> +    
> +    static void verifyCodeSigners(JarFile jar, JarEntry jarEntry) throws IOException {
> +        // codeSigners is initialized only after the entry has been read
> +        try (
> +            InputStream inputStream = jar.getInputStream(jarEntry);
> +        ) {
> +            while (inputStream.read() > -1);
> +            
> +            if (jarEntry.getCodeSigners() == null || jarEntry.getCodeSigners().length == 0) {
> +                throw new AssertionError("no signing certificate found for " + jarEntry.getName());
> +            }
> +        }
> +        
> +        // a check for the presence of code signers is sufficient to check bug JDK-6695402.
> +        // no need to also verify the actual code signers attributes here.
> +    }
> +    
> +}
>
> Regards,
> Philipp
>
>
>
> On 20.09.2017 03:41, Weijun Wang wrote:
>> Hi Philipp
>>
>> The change mostly looks fine. You might want to put everything into a patch file so Vincent can recreate a webrev and post it to cr.openjdk.java.net.
>>
>> One thing I would suggest for the test is that instead of using jarsigner -verify and check the text in output you can open it as a JarFile, consume the content of an entry, and look into its getCertificates().
>>
>> Also, you might want to reuse test/lib/jdk/test/lib/SecurityTools.java, and there is also JarUtils.java you can use. Maybe Files.copy(InputStream,Path) can be used in copyClassToFile().
>>
>> Thanks
>> Max
>>
>>
>>> On Sep 20, 2017, at 7:41 AM, Philipp Kunz <[hidden email]>
>>>  wrote:
>>>
>>> Hello Vincent
>>>
>>> Here may be the fix for JDK-6695402. First a test.
>>>
>>> BEGIN /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
>>> /*
>>>  * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
>>>  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>  *
>>>  * This code is free software; you can redistribute it and/or modify it
>>>  * under the terms of the GNU General Public License version 2 only, as
>>>  * published by the Free Software Foundation.
>>>  *
>>>  * This code is distributed in the hope that it will be useful, but WITHOUT
>>>  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>>>  * version 2 for more details (a copy is included in the LICENSE file that
>>>  * accompanied this code).
>>>  *
>>>  * You should have received a copy of the GNU General Public License version
>>>  * 2 along with this work; if not, write to the Free Software Foundation,
>>>  * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>>  *
>>>  * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>>>  * or visit
>>> www.oracle.com
>>>  if you need additional information or have any
>>>  * questions.
>>>  */
>>>
>>> /*
>>>  * @test
>>>  * @bug JDK-6695402
>>>  * @summary verify signatures of jars containing classes with names with multi-
>>>  *          byte unicode characters broken across lines
>>>  * @library /test/lib
>>>  * @modules jdk.jartool/sun.tools.jar
>>>  *          jdk.jartool/sun.security.tools.jarsigner
>>>  * @build jdk.test.lib.JDKToolLauncher
>>>  *        jdk.test.lib.process.*
>>>  * @run main MultibyteUnicodeName
>>>  */
>>>
>>> import java.io.ByteArrayOutputStream;
>>> import java.io.IOException;
>>> import java.io.InputStream;
>>> import java.io.UnsupportedEncodingException;
>>> import java.nio.file.Files;
>>> import java.nio.file.Paths;
>>> import java.util.jar.JarFile;
>>> import java.util.jar.Manifest;
>>> import java.util.stream.Collectors;
>>> import java.util.Arrays;
>>> import java.util.Map;
>>> import java.util.jar.Attributes.Name;
>>> import java.util.jar.JarEntry;
>>>
>>> import static java.nio.charset.StandardCharsets.UTF_8;
>>>
>>> import sun.security.tools.jarsigner.Resources;
>>>
>>> import jdk.test.lib.JDKToolLauncher;
>>> import jdk.test.lib.process.OutputAnalyzer;
>>> import jdk.test.lib.process.ProcessTools;
>>>
>>> public class MultibyteUnicodeName {
>>>    
>>>     public static void main(String[] args) throws Throwable {
>>>         try {
>>>             prepare();
>>>            
>>>             testSignJar("test.jar");
>>>             testSignJarNoManifest("test-no-manifest.jar");
>>>             testSignJarUpdate("test-update.jar");
>>>             testSignJarWithIndex("test-index.jar");
>>>             testSignJarAddIndex("test-add-index.jar");
>>>            
>>>         } finally {
>>>             Files.deleteIfExists(Paths.get(keystoreFileName));
>>>             Files.deleteIfExists(Paths.get(ManifestFileName));
>>>             Files.deleteIfExists(Paths.get(refClassFilename));
>>>             Files.deleteIfExists(Paths.get(testClassFilename));
>>>         }
>>>     }
>>>    
>>>     static final String alias = "a";
>>>     static final String keystoreFileName = "test.jks";
>>>     static final String ManifestFileName = "MANIFEST.MF";
>>>    
>>>     static class A1234567890B1234567890C1234567890D12345678exyz { }
>>>     static class A1234567890B1234567890C1234567890D12345678éxyz { }
>>>    
>>>     static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() +
>>>             "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
>>>     static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() +
>>>             "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
>>>    
>>>     static void prepare() throws Throwable {
>>>         tool("keytool", "-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
>>>                 "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
>>>                 "-validity", "366")
>>>                 .shouldHaveExitValue(0);
>>>        
>>>         Files.write(Paths.get(ManifestFileName),
>>>                 (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
>>>         copyClassToFile(refClassFilename);
>>>         copyClassToFile(testClassFilename);
>>>     }
>>>    
>>>     static void copyClassToFile(String classFilename) throws IOException {
>>>         try (
>>>             InputStream asStream = MultibyteUnicodeName.class.getResourceAsStream(classFilename);
>>>             ByteArrayOutputStream buf = new ByteArrayOutputStream();
>>>         ) {
>>>             int b;
>>>             while ((b = asStream.read()) != -1) buf.write(b);
>>>             Files.write(Paths.get(classFilename), buf.toByteArray());
>>>         }
>>>     }
>>>
>>>     static void testSignJar(String jarFileName) throws Throwable {
>>>         try {
>>>             tool("jar", "cvfm", jarFileName,
>>>                     ManifestFileName, refClassFilename, testClassFilename)
>>>                     .shouldHaveExitValue(0);
>>>             verifyJarSignature(jarFileName);
>>>
>>>         } finally {
>>>             Files.deleteIfExists(Paths.get(jarFileName));
>>>         }
>>>     }
>>>
>>>     static void testSignJarNoManifest(String jarFileName) throws Throwable {
>>>         try {
>>>             tool("jar", "cvf", jarFileName, refClassFilename, testClassFilename)
>>>                 .shouldHaveExitValue(0);
>>>             verifyJarSignature(jarFileName);
>>>
>>>         } finally {
>>>             Files.deleteIfExists(Paths.get(jarFileName));
>>>         }
>>>     }
>>>    
>>>     static void testSignJarUpdate(String jarFileName) throws Throwable {
>>>         try {
>>>             tool("jar", "cvfm", jarFileName, ManifestFileName, refClassFilename)
>>>                 .shouldHaveExitValue(0);
>>>             tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>                     "-storepass", "changeit", "-debug", jarFileName, alias)
>>>                 .shouldHaveExitValue(0);
>>>             tool("jar", "uvf", jarFileName, testClassFilename)
>>>                 .shouldHaveExitValue(0);
>>>             verifyJarSignature(jarFileName);
>>>
>>>         } finally {
>>>             Files.deleteIfExists(Paths.get(jarFileName));
>>>         }
>>>     }
>>>    
>>>     static void testSignJarWithIndex(String jarFileName) throws Throwable {
>>>         try {
>>>             tool("jar", "cvfm", jarFileName,
>>>                     ManifestFileName, refClassFilename, testClassFilename)
>>>                 .shouldHaveExitValue(0);
>>>             tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
>>>             verifyJarSignature(jarFileName);
>>>
>>>         } finally {
>>>             Files.deleteIfExists(Paths.get(jarFileName));
>>>         }
>>>     }
>>>
>>>     static void testSignJarAddIndex(String jarFileName) throws Throwable {
>>>         try {
>>>             tool("jar", "cvfm", jarFileName,
>>>                     ManifestFileName, refClassFilename, testClassFilename)
>>>                 .shouldHaveExitValue(0);
>>>             tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>                     "-storepass", "changeit", "-debug", jarFileName, alias)
>>>                 .shouldHaveExitValue(0);
>>>             tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
>>>             verifyJarSignature(jarFileName);
>>>
>>>         } finally {
>>>             Files.deleteIfExists(Paths.get(jarFileName));
>>>         }
>>>     }
>>>    
>>>     static Map<String, String> jarsignerResources = Arrays.stream(new Resources().getContents()).
>>>             collect(Collectors.toMap(e -> (String) e[0], e -> (String) e[1]));
>>>            
>>>     static void verifyJarSignature(String jarFileName) throws Throwable {
>>>         // actually sign the jar
>>>         tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>                 "-storepass", "changeit", "-debug", jarFileName, alias)
>>>                 .shouldHaveExitValue(0);
>>>        
>>>         verifyClassNameLineBroken(jarFileName);
>>>        
>>>         // check for no unsigned entries
>>>         tool("jarsigner", "-verify", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>                 "-storepass", "changeit", "-debug", jarFileName, alias)
>>>                 .shouldHaveExitValue(0)
>>>                 .shouldNotContain(jarsignerResources.get(
>>>                         "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));
>>>        
>>>         // check that both classes with and without multi-byte unicode characters in their names are signed
>>>         tool("jarsigner", "-verify", "-verbose", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>                 "-storepass", "changeit", "-debug", jarFileName, alias)
>>>                 .shouldHaveExitValue(0)
>>>                 .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + refClassFilename.replaceAll("\\$", "\\\\\\$") + "$")
>>>                 .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + testClassFilename.replaceAll("\\$", "\\\\\\$") + "$");
>>>        
>>>         // check that both classes with and without multi-byte unicode characters in their names have signing certificates
>>>         tool("jarsigner", "-verify", "-verbose", "-certs", "-keystore", keystoreFileName,
>>>                 "-storetype", "JKS", "-storepass", "changeit", "-debug", jarFileName, alias)
>>>                 .shouldHaveExitValue(0)
>>>                 .shouldMatch(refClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)")
>>>                 .shouldMatch(testClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)");
>>>     }
>>>    
>>>     /**
>>>      * it would be too easy to miss the actual test case by just renaming an identifier so that
>>>      * the multi-byte encoded character would not any longer be broken across a line break.
>>>      *
>>>      * this test verifies that the actual test case is tested based on the manifest
>>>      * and not based on the signature file because at the moment, the signature file
>>>      * does not even contain the desired entry at all.
>>>      *
>>>      * this relies on the Manifest breaking lines unaware of bytes that belong to the same
>>>      * multi-ybte utf characters.
>>>      */
>>>     static void verifyClassNameLineBroken(String jarFileName) throws IOException {
>>>         byte[] eAcute = "é".getBytes(UTF_8);
>>>         byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
>>>        
>>>         try (
>>>             JarFile jar = new JarFile(jarFileName);
>>>         ) {
>>>             if (jar.getManifest().getAttributes(testClassFilename) == null) {
>>>                 throw new AssertionError(testClassFilename + " not found in manifest");
>>>             }
>>>            
>>>             JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
>>>             try (
>>>                 InputStream manifestIs = jar.getInputStream(manifestEntry);
>>>             ) {
>>>                 int bytesMatched = 0;
>>>                 for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
>>>                     if ((byte) b == eAcuteBroken[bytesMatched]) {
>>>                         bytesMatched++;
>>>                         if (bytesMatched == eAcuteBroken.length) {
>>>                             break;
>>>                         }
>>>                     } else {
>>>                         bytesMatched = 0;
>>>                     }
>>>                 }
>>>                 if (bytesMatched < eAcuteBroken.length) {
>>>                     throw new AssertionError("self-test failed: "
>>>                             + "multi-byte utf-8 character not broken across lines");
>>>                 }
>>>             }
>>>         }
>>>     }
>>>    
>>>     static OutputAnalyzer tool(String tool, String... args)
>>>             throws Throwable {
>>>         JDKToolLauncher l = JDKToolLauncher.createUsingTestJDK(tool);
>>>         for (String arg : args) {
>>>             if (arg .startsWith("-J")) {
>>>                 l.addVMArg(arg.substring(2));
>>>             } else {
>>>                 l.addToolArg(arg);
>>>             }
>>>         }
>>>         return ProcessTools.executeCommand(l.getCommand());
>>>     }
>>>    
>>> }
>>> END /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
>>>
>>>
>>> There are three e with acutes, on lines 84, 89, and 227, just in case the mail suffers bad encoding which might not be absolutely unconceivable regarding the bug's subject.
>>>
>>> In contrast to the bug description it uses a nested class with a two-byte UTF-8 character rather than one in its own file. I chose to do it that way because then the complete test goes into one file and therefore I assume the overview is kept easier. The test still demonstrates exactly JDK-6695402's problem.
>>>
>>> It may not have been necessary to also test jar files with indexes but i consider it necessary to have signing unsigned as well as partially signed jars tested, so why not have one more case tested, too?
>>>
>>> There might also be a more elegant approach to get class files into a jar file as to get their contents through the class loader. I chose to use real class files rather than dummy contents in files named *.class or for instance plain text files in the jar the signing of which to be tested in order to stay as close to the original bug problem as possible even though I don't have any notion that it would make a difference. The amount of code used to copy the class file contents around is comparatively small with respect to the whole test case amount of code.
>>>  
>>> For the demonstration that the multi-byte character actually makes the alleged difference, I concluded that it was necessary to have another class name not previously affected by the bug in order to compare the effects of just different names. Otherwise the signing could fail to any other reason undetectably.
>>>
>>> In order to express a condition to tell successful from failed test runs the best approach I found was to analyze the jarsigner tool's output which is in my opinion not the most desirable option. I'd have preferred output from a clearer structured api but then I guess the output format will not change too often in the foreseeable future. For instance the check for the s, m, and k flags is more pragmatical approach than obviously perfectly failsafe when operating with regular expressions to match it with the output. Other parts such as 'X.509' and 'CN=' are not even jarsigner tool resources. I put at least a reference to sun.security.tools.jarsigner.Resources.
>>>
>>> Finally, I afforded kind of a self-test that protects the test from undetected failing because renaming the main class which would move the two-byte unicode character to a position not suitable for the test. It may not be absolutely necessary.
>>>
>>>
>>>
>>> BEGIN PATH
>>> diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
>>> --- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
>>> +++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Wed Sep 20 00:56:15 2017 +0200
>>> @@ -28,6 +28,7 @@
>>>  import java.security.*;
>>>  import java.util.HashMap;
>>>  import java.io.ByteArrayOutputStream;
>>> +import static java.nio.charset.StandardCharsets.UTF_8;
>>>  
>>>  /**
>>>   * This class is used to compute digests on sections of the Manifest.
>>> @@ -112,7 +113,7 @@
>>>          rawBytes = bytes;
>>>          entries = new HashMap<>();
>>>  
>>> -        ByteArrayOutputStream baos = new ByteArrayOutputStream();
>>> +        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
>>>  
>>>          Position pos = new Position();
>>>  
>>> @@ -131,50 +132,40 @@
>>>  
>>>              if (len > 6) {
>>>                  if (isNameAttr(bytes, start)) {
>>> -                    StringBuilder nameBuf = new StringBuilder(sectionLen);
>>> +                    nameBuf.reset();
>>> +                    nameBuf.write(bytes, start+6, len-6);
>>>  
>>> -                    try {
>>> -                        nameBuf.append(
>>> -                            new String(bytes, start+6, len-6, "UTF8"));
>>> +                    int i = start + len;
>>> +                    if ((i-start) < sectionLen) {
>>> +                        if (bytes[i] == '\r') {
>>> +                            i += 2;
>>> +                        } else {
>>> +                            i += 1;
>>> +                        }
>>> +                    }
>>>  
>>> -                        int i = start + len;
>>> -                        if ((i-start) < sectionLen) {
>>> -                            if (bytes[i] == '\r') {
>>> -                                i += 2;
>>> -                            } else {
>>> -                                i += 1;
>>> -                            }
>>> +                    while ((i-start) < sectionLen) {
>>> +                        if (bytes[i++] == ' ') {
>>> +                            // name is wrapped
>>> +                            int wrapStart = i;
>>> +                            while (((i-start) < sectionLen)
>>> +                                    && (bytes[i++] != '\n'));
>>> +                            if (bytes[i-1] != '\n')
>>> +                                return; // XXX: exception?
>>> +                            int wrapLen;
>>> +                            if (bytes[i-2] == '\r')
>>> +                                wrapLen = i-wrapStart-2;
>>> +                            else
>>> +                                wrapLen = i-wrapStart-1;
>>> +
>>> +                            nameBuf.write(bytes, wrapStart, wrapLen);
>>> +                        } else {
>>> +                            break;
>>>                          }
>>> +                    }
>>>  
>>> -                        while ((i-start) < sectionLen) {
>>> -                            if (bytes[i++] == ' ') {
>>> -                                // name is wrapped
>>> -                                int wrapStart = i;
>>> -                                while (((i-start) < sectionLen)
>>> -                                        && (bytes[i++] != '\n'));
>>> -                                    if (bytes[i-1] != '\n')
>>> -                                        return; // XXX: exception?
>>> -                                    int wrapLen;
>>> -                                    if (bytes[i-2] == '\r')
>>> -                                        wrapLen = i-wrapStart-2;
>>> -                                    else
>>> -                                        wrapLen = i-wrapStart-1;
>>> -
>>> -                            nameBuf.append(new String(bytes, wrapStart,
>>> -                                                      wrapLen, "UTF8"));
>>> -                            } else {
>>> -                                break;
>>> -                            }
>>> -                        }
>>> -
>>> -                        entries.put(nameBuf.toString(),
>>> -                            new Entry(start, sectionLen, sectionLenWithBlank,
>>> -                                rawBytes));
>>> -
>>> -                    } catch (java.io.UnsupportedEncodingException uee) {
>>> -                        throw new IllegalStateException(
>>> -                            "UTF8 not available on platform");
>>> -                    }
>>> +                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
>>> +                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
>>>                  }
>>>              }
>>>              start = pos.startOfNext;
>>> END PATCH
>>>
>>>
>>> The patch is mostly so big because indentation has changed in many lines.
>>>
>>> There was baos there before without apparent use. The patch renames it and puts it into use. It came in very handy because if it would not have been there already, I would have had to defined one of my own.
>>>
>>> The main point of the patch is that manifest section names that are broken across lines at possibly arbitrary bytes are no longer converted into strings for each manifest line and then joined. Parts of names broken across lines are now joined first when they are still byte sequences and only when the complete byte sequence is available after processing all manifest lines that contain the same manifest section name decoded into a unicode string.
>>>
>>> For decoding the bytes into a string I chose a different string constructor than was used before that does not any longer declare UnsupportedEncodingException rendering the try/catch redundant. It couldn't have ever occurred anyway taking into consideration that UTF-8 is mandatory for every Java platform, StandardCharsets says. The difference according to the documentation is that the previously     used String constructor returned undefined strings for invalid byte sequences whereas the one used in the patch will replace unparseable portions with valid 'unknown' characters.
>>>
>>> One question I cannot still answer yet is how the ManifestDigester can be changed at all without complete test coverage or risking to break existing signatures. I already started on that but it's quite a piece of work to almost formally prove that all manifests will continue to produce identical digests, except of course for the ones now fixed.
>>>
>>> Regards,
>>> Philipp
>>>
>>>
>>> On 17.09.2017 21:25, Philipp Kunz wrote:
>>>
>>>> Hello Vincent
>>>>
>>>> I narrowed the error down so far and suspect now that it is an effect of sun.security.util.ManifestDigester's constructor, lines 134 and 163-164, in combination with java.util.jar.Manifest.make72Safe. Manifest breaks multi-byte characters in manifest section names across lines and ManifestDigester fails to restore them correctly, which both sounds wrong but ManifestDigester sure is.
>>>>
>>>> Just fixing it would be too tempting but then I would not know (I mean not know for sure with evidence from tests) if any existing jar-signature would still be valid and I don't want to break existing signatures. When I had a look at the tests specific for Manifest and ManifestDigester I found very little. There are many more different situations and corner cases and combinations thereof to cover with tests than it looks like at first glance so that writing tests that cover all relevant cases looks to me like quite an effort. I have the impression that test coverage was not a priority when the subjected code was developed or at least the tests might not have been contributed to the open JDK project. Hence, while I'm continuing to complete these tests I miss, if       someone has an idea how to simplify that so that I can still convince at least myself that no existing signature will break or where possibly more testcases are that I haven't discovered so far, please let me know.
>>>>
>>>> I'm also wondering how much performance should be taken into consideration. There are a few hints such as reusing byte arrays that suggest that it is an issue. Will a patch be rejected if it slows down some tests beyond a certain limit for so little added value or what might be the acceptance criteria here? I don't expect insuperable difficulties with performance but would still appreciate some general idea.
>>>>
>>>> My desired or currently preferred approach to fix JDK-6695402 would be to let ManifestDigester any kind of use or extend Manifest in order to let Manifest identify the manifest sections thereby preventing the parsing duplicated that identifies the manifest sections.
>>>>
>>>> As a new contributor I could use and would appreciate some guidance particularly about what kind and completeness of test coverage and performance tuning applies or is suggested in the context of the given bug. Otherwise I fear I might contribute too many tests which would have to be reviewed and maintained or quite some work would be for nothing if a patch would not satisfy performance requirements.
>>>>
>>>> Regards,
>>>> Philipp
>>>>
>>>>
>>>>
>>>> On 01.09.2017 15:20, Vincent Ryan wrote:
>>>>
>>>>> That all sounds fine. Let me know when your patch is ready to submit.
>>>>> Thanks.
>>>>>
>>>>>
>>>>>
>>>>>> On 1 Sep 2017, at 13:15, Philipp Kunz <[hidden email]>
>>>>>>  wrote:
>>>>>>
>>>>>> Hello Vincent
>>>>>>
>>>>>> Thank you for sponsoring!
>>>>>> So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here:
>>>>>> http://www.oracle.com/technetwork/community/oca-486395.html#p
>>>>>>  -> Paratix GmbH
>>>>>> Therefore I think I have followed the steps in
>>>>>> http://openjdk.java.net/contribute/
>>>>>>  at least the ones before actual patch submission.
>>>>>>
>>>>>> Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
>>>>>> make run-test-tier1
>>>>>> make run-test TEST="jdk/test"
>>>>>> they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
>>>>>> make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
>>>>>> then it reports zero errors and failures which may be a good starting point.
>>>>>> Do you think that sounds reasonable or do you have another suggestion how to run the tests?
>>>>>>
>>>>>> Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of
>>>>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java
>>>>>> . This way, I try to demonstrate the improvement.
>>>>>>
>>>>>> I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
>>>>>>
>>>>>> So I'll try to remove it first including the whole four line if block.
>>>>>>
>>>>>> Philipp
>>>>>>
>>>>>>
>>>>>> On 01.09.2017 10:00, Vincent Ryan wrote:
>>>>>>
>>>>>>> Hello Philipp,
>>>>>>>
>>>>>>> I’m happy to sponsor your fix for JDK 10. Have you followed these steps:
>>>>>>> http://openjdk.java.net/contribute/
>>>>>>>  ?
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> On 1 Sep 2017, at 08:58, Vincent Ryan <[hidden email]>
>>>>>>>>  wrote:
>>>>>>>>
>>>>>>>> Moved to security-dev
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]>
>>>>>>>>>  wrote:
>>>>>>>>>
>>>>>>>>> Hello everyone
>>>>>>>>>
>>>>>>>>> I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it:
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6695402
>>>>>>>>> . This also looks like it may not be too big a piece for a first contribution.
>>>>>>>>>
>>>>>>>>> I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:
>>>>>>>>>
>>>>>>>>> May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?
>>>>>>>>>
>>>>>>>>> Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.
>>>>>>>>>
>>>>>>>>> One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.
>>>>>>>>>
>>>>>>>>> I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.
>>>>>>>>>
>>>>>>>>> Philipp Kunz
>>>>>>>>>
>>>>
>>>>
>>>>
>>>>
>>>> <Mail Attachment.png>
>>>>
>>>> Paratix GmbH
>>>> St Peterhofstatt 11
>>>> 8001 Zürich
>>>>
>>>> +41 (0)76 397 79 35
>>>>
>>>> [hidden email]
>
> --
>
>
> Gruss Philipp
>
>
>
>
> <Mail Attachment.png>
>
> Paratix GmbH
> St Peterhofstatt 11
> 8001 Zürich
>
> +41 (0)76 397 79 35
> [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang
Some more suggestions on the test:

1. You can use readAllBytes() to ... err ... read all bytes.

2. I normally don’t remove intermediate files. Jtreg will do an automatic cleanup, and it can be configured to retain those files when the test fails. They will be very helpful in debugging.

Thanks
Max

> 在 2017年9月24日,23:12,Weijun Wang <[hidden email]> 写道:
>
> Great.
>
> Some suggestions, just my own habit, you are free to decide yourself:
>
> 1. In ManifestDigester.java, I'd rather define nameBuf inside the if (isNameAttr(bytes, start)) block.
>
> 2. I normally don't use "import static" unless I can save a lot of keystrokes and still do not confuse anyone. Both in the test and in ManifestDigester.java.
>
> 3. In the test, there is no need to write "@run main MultibyteUnicodeName" if it is the only action (i.e. no other build/compile) and there is no modifier on main (i.e. no othervm/timeout etc).
>
> Several tiny problems:
>
> 1. No need for @modules in the test. Maybe you used some internal classes in your previous version?
>
> 2. In the new consolidated repo, the test should be inside test/jdk/sun/..., please note the "jdk" after "test".
>
> 3. Please update the copyright years.
>
> 4. In the test, there should be no "JDK-" in the @test tag.
>
> Thanks
> Max
>
>> On Sep 24, 2017, at 7:51 PM, Philipp Kunz <[hidden email]> wrote:
>>
>> Hi Max and Vincent
>>
>> Thank you for your suggestions. It sure looks better now. I hope this time I got the patch added in the right format.
>>
>> diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
>> --- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
>> +++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Sun Sep 24 10:34:00 2017 +0200
>> @@ -28,6 +28,7 @@
>> import java.security.*;
>> import java.util.HashMap;
>> import java.io.ByteArrayOutputStream;
>> +import static java.nio.charset.StandardCharsets.UTF_8;
>>
>> /**
>> * This class is used to compute digests on sections of the Manifest.
>> @@ -112,7 +113,7 @@
>>        rawBytes = bytes;
>>        entries = new HashMap<>();
>>
>> -        ByteArrayOutputStream baos = new ByteArrayOutputStream();
>> +        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
>>
>>        Position pos = new Position();
>>
>> @@ -131,50 +132,40 @@
>>
>>            if (len > 6) {
>>                if (isNameAttr(bytes, start)) {
>> -                    StringBuilder nameBuf = new StringBuilder(sectionLen);
>> +                    nameBuf.reset();
>> +                    nameBuf.write(bytes, start+6, len-6);
>>
>> -                    try {
>> -                        nameBuf.append(
>> -                            new String(bytes, start+6, len-6, "UTF8"));
>> +                    int i = start + len;
>> +                    if ((i-start) < sectionLen) {
>> +                        if (bytes[i] == '\r') {
>> +                            i += 2;
>> +                        } else {
>> +                            i += 1;
>> +                        }
>> +                    }
>>
>> -                        int i = start + len;
>> -                        if ((i-start) < sectionLen) {
>> -                            if (bytes[i] == '\r') {
>> -                                i += 2;
>> -                            } else {
>> -                                i += 1;
>> -                            }
>> +                    while ((i-start) < sectionLen) {
>> +                        if (bytes[i++] == ' ') {
>> +                            // name is wrapped
>> +                            int wrapStart = i;
>> +                            while (((i-start) < sectionLen)
>> +                                    && (bytes[i++] != '\n'));
>> +                            if (bytes[i-1] != '\n')
>> +                                return; // XXX: exception?
>> +                            int wrapLen;
>> +                            if (bytes[i-2] == '\r')
>> +                                wrapLen = i-wrapStart-2;
>> +                            else
>> +                                wrapLen = i-wrapStart-1;
>> +
>> +                            nameBuf.write(bytes, wrapStart, wrapLen);
>> +                        } else {
>> +                            break;
>>                        }
>> +                    }
>>
>> -                        while ((i-start) < sectionLen) {
>> -                            if (bytes[i++] == ' ') {
>> -                                // name is wrapped
>> -                                int wrapStart = i;
>> -                                while (((i-start) < sectionLen)
>> -                                        && (bytes[i++] != '\n'));
>> -                                    if (bytes[i-1] != '\n')
>> -                                        return; // XXX: exception?
>> -                                    int wrapLen;
>> -                                    if (bytes[i-2] == '\r')
>> -                                        wrapLen = i-wrapStart-2;
>> -                                    else
>> -                                        wrapLen = i-wrapStart-1;
>> -
>> -                            nameBuf.append(new String(bytes, wrapStart,
>> -                                                      wrapLen, "UTF8"));
>> -                            } else {
>> -                                break;
>> -                            }
>> -                        }
>> -
>> -                        entries.put(nameBuf.toString(),
>> -                            new Entry(start, sectionLen, sectionLenWithBlank,
>> -                                rawBytes));
>> -
>> -                    } catch (java.io.UnsupportedEncodingException uee) {
>> -                        throw new IllegalStateException(
>> -                            "UTF8 not available on platform");
>> -                    }
>> +                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
>> +                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
>>                }
>>            }
>>            start = pos.startOfNext;
>> diff -r ddc25f646c2e test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
>> --- /dev/null    Thu Jan 01 00:00:00 1970 +0000
>> +++ b/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java    Sun Sep 24 10:34:00 2017 +0200
>> @@ -0,0 +1,209 @@
>> +/*
>> + * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
>> + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>> + *
>> + * This code is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 only, as
>> + * published by the Free Software Foundation.
>> + *
>> + * This code is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>> + * version 2 for more details (a copy is included in the LICENSE file that
>> + * accompanied this code).
>> + *
>> + * You should have received a copy of the GNU General Public License version
>> + * 2 along with this work; if not, write to the Free Software Foundation,
>> + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>> + *
>> + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>> + * or visit www.oracle.com if you need additional information or have any
>> + * questions.
>> + */
>> +
>> +/*
>> + * @test
>> + * @bug JDK-6695402
>> + * @summary verify signatures of jars containing classes with names with multi-
>> + *          byte unicode characters broken across lines
>> + * @library /test/lib
>> + * @modules jdk.jartool/sun.tools.jar
>> + *          jdk.jartool/sun.security.tools.jarsigner
>> + * @run main MultibyteUnicodeName
>> + */
>> +
>> +import java.io.IOException;
>> +import java.io.InputStream;
>> +import java.nio.file.Files;
>> +import java.nio.file.Paths;
>> +import java.util.jar.JarFile;
>> +import java.util.jar.Manifest;
>> +import java.util.Arrays;
>> +import java.util.Map;
>> +import java.util.jar.Attributes.Name;
>> +import java.util.jar.JarEntry;
>> +
>> +import static java.nio.charset.StandardCharsets.UTF_8;
>> +
>> +import static jdk.test.lib.SecurityTools.jarsigner;
>> +import static jdk.test.lib.SecurityTools.keytool;
>> +import static jdk.test.lib.util.JarUtils.createJar;
>> +import static jdk.test.lib.util.JarUtils.updateJar;
>> +
>> +public class MultibyteUnicodeName {
>> +
>> +    /**
>> +     * this class's name will break across lines in the jar manifest at the middle of
>> +     * a two-byte utf encoded character due to its e acute letter at its exact position.
>> +     *
>> +     * @see also eAcute in {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}
>> +     */
>> +    static class A1234567890B1234567890C1234567890D12345678éxyz { }
>> +    static class A1234567890B1234567890C1234567890D12345678exyz { }
>> +
>> +    static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() +
>> +            "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
>> +    static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() +
>> +            "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
>> +    
>> +    static final String alias = "a";
>> +    static final String keystoreFileName = "test.jks";
>> +    static final String ManifestFileName = "MANIFEST.MF";
>> +    
>> +    public static void main(String[] args) throws Exception {
>> +        try {
>> +            prepare();
>> +            
>> +            testSignJar("test.jar");
>> +            testSignJarNoManifest("test-no-manifest.jar");
>> +            testSignJarUpdate("test-update.jar", "test-updated.jar");
>> +            
>> +        } finally {
>> +            Files.deleteIfExists(Paths.get(keystoreFileName));
>> +            Files.deleteIfExists(Paths.get(ManifestFileName));
>> +            Files.deleteIfExists(Paths.get(refClassFilename));
>> +            Files.deleteIfExists(Paths.get(testClassFilename));
>> +        }
>> +    }
>> +    
>> +    static void prepare() throws Exception {
>> +        keytool("-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
>> +                "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
>> +                "-validity", "366").shouldHaveExitValue(0);
>> +        
>> +        Files.write(Paths.get(ManifestFileName),
>> +                (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
>> +        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(refClassFilename),
>> +                Paths.get(refClassFilename));
>> +        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(testClassFilename),
>> +                Paths.get(testClassFilename));
>> +    }
>> +    
>> +    static void testSignJar(String jarFileName) throws Exception {
>> +        try {
>> +            createJar(jarFileName, ManifestFileName, refClassFilename, testClassFilename);
>> +            verifyJarSignature(jarFileName);
>> +
>> +        } finally {
>> +            Files.deleteIfExists(Paths.get(jarFileName));
>> +        }
>> +    }
>> +
>> +    static void testSignJarNoManifest(String jarFileName) throws Exception {
>> +        try {
>> +            createJar(jarFileName, refClassFilename, testClassFilename);
>> +            verifyJarSignature(jarFileName);
>> +
>> +        } finally {
>> +            Files.deleteIfExists(Paths.get(jarFileName));
>> +        }
>> +    }
>> +    
>> +    static void testSignJarUpdate(String initialFileName, String updatedFileName) throws Exception {
>> +        try {
>> +            createJar(initialFileName, ManifestFileName, refClassFilename);
>> +            jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
>> +                    "-storepass", "changeit", "-debug", initialFileName, alias)
>> +                .shouldHaveExitValue(0);
>> +            updateJar(initialFileName, updatedFileName, testClassFilename);
>> +            verifyJarSignature(updatedFileName);
>> +
>> +        } finally {
>> +            Files.deleteIfExists(Paths.get(initialFileName));
>> +            Files.deleteIfExists(Paths.get(updatedFileName));
>> +        }
>> +    }
>> +    
>> +    static void verifyJarSignature(String jarFileName) throws Exception {
>> +        // actually sign the jar
>> +        jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
>> +                "-storepass", "changeit", "-debug", jarFileName, alias)
>> +                .shouldHaveExitValue(0);
>> +
>> +        try (
>> +            JarFile jar = new JarFile(jarFileName);
>> +        ) {
>> +            verifyClassNameLineBroken(jar, testClassFilename);
>> +            verifyCodeSigners(jar, jar.getJarEntry(testClassFilename));
>> +            verifyCodeSigners(jar, jar.getJarEntry(refClassFilename));
>> +        }
>> +    }
>> +    
>> +    /**
>> +     * it would be too easy to miss the actual test case by just renaming an identifier so that
>> +     * the multi-byte encoded character would not any longer be broken across a line break.
>> +     *
>> +     * this test verifies that the actual test case is tested based on the manifest
>> +     * and not based on the signature file because at the moment, the signature file
>> +     * does not even contain the desired entry at all.
>> +     *
>> +     * this relies on the Manifest breaking lines unaware of bytes that belong to the same
>> +     * multi-ybte utf characters.
>> +     */
>> +    static void verifyClassNameLineBroken(JarFile jar, String className) throws IOException {
>> +        byte[] eAcute = "é".getBytes(UTF_8);
>> +        byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
>> +        
>> +        if (jar.getManifest().getAttributes(className) == null) {
>> +            throw new AssertionError(className + " not found in manifest");
>> +        }
>> +        
>> +        JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
>> +        try (
>> +            InputStream manifestIs = jar.getInputStream(manifestEntry);
>> +        ) {
>> +            int bytesMatched = 0;
>> +            for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
>> +                if ((byte) b == eAcuteBroken[bytesMatched]) {
>> +                    bytesMatched++;
>> +                    if (bytesMatched == eAcuteBroken.length) {
>> +                        break;
>> +                    }
>> +                } else {
>> +                    bytesMatched = 0;
>> +                }
>> +            }
>> +            if (bytesMatched < eAcuteBroken.length) {
>> +                throw new AssertionError("self-test failed: "
>> +                        + "multi-byte utf-8 character not broken across lines");
>> +            }
>> +        }
>> +    }
>> +    
>> +    static void verifyCodeSigners(JarFile jar, JarEntry jarEntry) throws IOException {
>> +        // codeSigners is initialized only after the entry has been read
>> +        try (
>> +            InputStream inputStream = jar.getInputStream(jarEntry);
>> +        ) {
>> +            while (inputStream.read() > -1);
>> +            
>> +            if (jarEntry.getCodeSigners() == null || jarEntry.getCodeSigners().length == 0) {
>> +                throw new AssertionError("no signing certificate found for " + jarEntry.getName());
>> +            }
>> +        }
>> +        
>> +        // a check for the presence of code signers is sufficient to check bug JDK-6695402.
>> +        // no need to also verify the actual code signers attributes here.
>> +    }
>> +    
>> +}
>>
>> Regards,
>> Philipp
>>
>>
>>
>>> On 20.09.2017 03:41, Weijun Wang wrote:
>>> Hi Philipp
>>>
>>> The change mostly looks fine. You might want to put everything into a patch file so Vincent can recreate a webrev and post it to cr.openjdk.java.net.
>>>
>>> One thing I would suggest for the test is that instead of using jarsigner -verify and check the text in output you can open it as a JarFile, consume the content of an entry, and look into its getCertificates().
>>>
>>> Also, you might want to reuse test/lib/jdk/test/lib/SecurityTools.java, and there is also JarUtils.java you can use. Maybe Files.copy(InputStream,Path) can be used in copyClassToFile().
>>>
>>> Thanks
>>> Max
>>>
>>>
>>>> On Sep 20, 2017, at 7:41 AM, Philipp Kunz <[hidden email]>
>>>> wrote:
>>>>
>>>> Hello Vincent
>>>>
>>>> Here may be the fix for JDK-6695402. First a test.
>>>>
>>>> BEGIN /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
>>>> /*
>>>> * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
>>>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>>>> *
>>>> * This code is free software; you can redistribute it and/or modify it
>>>> * under the terms of the GNU General Public License version 2 only, as
>>>> * published by the Free Software Foundation.
>>>> *
>>>> * This code is distributed in the hope that it will be useful, but WITHOUT
>>>> * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>>> * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>>>> * version 2 for more details (a copy is included in the LICENSE file that
>>>> * accompanied this code).
>>>> *
>>>> * You should have received a copy of the GNU General Public License version
>>>> * 2 along with this work; if not, write to the Free Software Foundation,
>>>> * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
>>>> *
>>>> * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
>>>> * or visit
>>>> www.oracle.com
>>>> if you need additional information or have any
>>>> * questions.
>>>> */
>>>>
>>>> /*
>>>> * @test
>>>> * @bug JDK-6695402
>>>> * @summary verify signatures of jars containing classes with names with multi-
>>>> *          byte unicode characters broken across lines
>>>> * @library /test/lib
>>>> * @modules jdk.jartool/sun.tools.jar
>>>> *          jdk.jartool/sun.security.tools.jarsigner
>>>> * @build jdk.test.lib.JDKToolLauncher
>>>> *        jdk.test.lib.process.*
>>>> * @run main MultibyteUnicodeName
>>>> */
>>>>
>>>> import java.io.ByteArrayOutputStream;
>>>> import java.io.IOException;
>>>> import java.io.InputStream;
>>>> import java.io.UnsupportedEncodingException;
>>>> import java.nio.file.Files;
>>>> import java.nio.file.Paths;
>>>> import java.util.jar.JarFile;
>>>> import java.util.jar.Manifest;
>>>> import java.util.stream.Collectors;
>>>> import java.util.Arrays;
>>>> import java.util.Map;
>>>> import java.util.jar.Attributes.Name;
>>>> import java.util.jar.JarEntry;
>>>>
>>>> import static java.nio.charset.StandardCharsets.UTF_8;
>>>>
>>>> import sun.security.tools.jarsigner.Resources;
>>>>
>>>> import jdk.test.lib.JDKToolLauncher;
>>>> import jdk.test.lib.process.OutputAnalyzer;
>>>> import jdk.test.lib.process.ProcessTools;
>>>>
>>>> public class MultibyteUnicodeName {
>>>>
>>>>   public static void main(String[] args) throws Throwable {
>>>>       try {
>>>>           prepare();
>>>>
>>>>           testSignJar("test.jar");
>>>>           testSignJarNoManifest("test-no-manifest.jar");
>>>>           testSignJarUpdate("test-update.jar");
>>>>           testSignJarWithIndex("test-index.jar");
>>>>           testSignJarAddIndex("test-add-index.jar");
>>>>
>>>>       } finally {
>>>>           Files.deleteIfExists(Paths.get(keystoreFileName));
>>>>           Files.deleteIfExists(Paths.get(ManifestFileName));
>>>>           Files.deleteIfExists(Paths.get(refClassFilename));
>>>>           Files.deleteIfExists(Paths.get(testClassFilename));
>>>>       }
>>>>   }
>>>>
>>>>   static final String alias = "a";
>>>>   static final String keystoreFileName = "test.jks";
>>>>   static final String ManifestFileName = "MANIFEST.MF";
>>>>
>>>>   static class A1234567890B1234567890C1234567890D12345678exyz { }
>>>>   static class A1234567890B1234567890C1234567890D12345678éxyz { }
>>>>
>>>>   static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() +
>>>>           "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
>>>>   static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() +
>>>>           "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
>>>>
>>>>   static void prepare() throws Throwable {
>>>>       tool("keytool", "-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
>>>>               "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
>>>>               "-validity", "366")
>>>>               .shouldHaveExitValue(0);
>>>>
>>>>       Files.write(Paths.get(ManifestFileName),
>>>>               (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
>>>>       copyClassToFile(refClassFilename);
>>>>       copyClassToFile(testClassFilename);
>>>>   }
>>>>
>>>>   static void copyClassToFile(String classFilename) throws IOException {
>>>>       try (
>>>>           InputStream asStream = MultibyteUnicodeName.class.getResourceAsStream(classFilename);
>>>>           ByteArrayOutputStream buf = new ByteArrayOutputStream();
>>>>       ) {
>>>>           int b;
>>>>           while ((b = asStream.read()) != -1) buf.write(b);
>>>>           Files.write(Paths.get(classFilename), buf.toByteArray());
>>>>       }
>>>>   }
>>>>
>>>>   static void testSignJar(String jarFileName) throws Throwable {
>>>>       try {
>>>>           tool("jar", "cvfm", jarFileName,
>>>>                   ManifestFileName, refClassFilename, testClassFilename)
>>>>                   .shouldHaveExitValue(0);
>>>>           verifyJarSignature(jarFileName);
>>>>
>>>>       } finally {
>>>>           Files.deleteIfExists(Paths.get(jarFileName));
>>>>       }
>>>>   }
>>>>
>>>>   static void testSignJarNoManifest(String jarFileName) throws Throwable {
>>>>       try {
>>>>           tool("jar", "cvf", jarFileName, refClassFilename, testClassFilename)
>>>>               .shouldHaveExitValue(0);
>>>>           verifyJarSignature(jarFileName);
>>>>
>>>>       } finally {
>>>>           Files.deleteIfExists(Paths.get(jarFileName));
>>>>       }
>>>>   }
>>>>
>>>>   static void testSignJarUpdate(String jarFileName) throws Throwable {
>>>>       try {
>>>>           tool("jar", "cvfm", jarFileName, ManifestFileName, refClassFilename)
>>>>               .shouldHaveExitValue(0);
>>>>           tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>>                   "-storepass", "changeit", "-debug", jarFileName, alias)
>>>>               .shouldHaveExitValue(0);
>>>>           tool("jar", "uvf", jarFileName, testClassFilename)
>>>>               .shouldHaveExitValue(0);
>>>>           verifyJarSignature(jarFileName);
>>>>
>>>>       } finally {
>>>>           Files.deleteIfExists(Paths.get(jarFileName));
>>>>       }
>>>>   }
>>>>
>>>>   static void testSignJarWithIndex(String jarFileName) throws Throwable {
>>>>       try {
>>>>           tool("jar", "cvfm", jarFileName,
>>>>                   ManifestFileName, refClassFilename, testClassFilename)
>>>>               .shouldHaveExitValue(0);
>>>>           tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
>>>>           verifyJarSignature(jarFileName);
>>>>
>>>>       } finally {
>>>>           Files.deleteIfExists(Paths.get(jarFileName));
>>>>       }
>>>>   }
>>>>
>>>>   static void testSignJarAddIndex(String jarFileName) throws Throwable {
>>>>       try {
>>>>           tool("jar", "cvfm", jarFileName,
>>>>                   ManifestFileName, refClassFilename, testClassFilename)
>>>>               .shouldHaveExitValue(0);
>>>>           tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>>                   "-storepass", "changeit", "-debug", jarFileName, alias)
>>>>               .shouldHaveExitValue(0);
>>>>           tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
>>>>           verifyJarSignature(jarFileName);
>>>>
>>>>       } finally {
>>>>           Files.deleteIfExists(Paths.get(jarFileName));
>>>>       }
>>>>   }
>>>>
>>>>   static Map<String, String> jarsignerResources = Arrays.stream(new Resources().getContents()).
>>>>           collect(Collectors.toMap(e -> (String) e[0], e -> (String) e[1]));
>>>>
>>>>   static void verifyJarSignature(String jarFileName) throws Throwable {
>>>>       // actually sign the jar
>>>>       tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>>               "-storepass", "changeit", "-debug", jarFileName, alias)
>>>>               .shouldHaveExitValue(0);
>>>>
>>>>       verifyClassNameLineBroken(jarFileName);
>>>>
>>>>       // check for no unsigned entries
>>>>       tool("jarsigner", "-verify", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>>               "-storepass", "changeit", "-debug", jarFileName, alias)
>>>>               .shouldHaveExitValue(0)
>>>>               .shouldNotContain(jarsignerResources.get(
>>>>                       "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));
>>>>
>>>>       // check that both classes with and without multi-byte unicode characters in their names are signed
>>>>       tool("jarsigner", "-verify", "-verbose", "-keystore", keystoreFileName, "-storetype", "JKS",
>>>>               "-storepass", "changeit", "-debug", jarFileName, alias)
>>>>               .shouldHaveExitValue(0)
>>>>               .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + refClassFilename.replaceAll("\\$", "\\\\\\$") + "$")
>>>>               .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + testClassFilename.replaceAll("\\$", "\\\\\\$") + "$");
>>>>
>>>>       // check that both classes with and without multi-byte unicode characters in their names have signing certificates
>>>>       tool("jarsigner", "-verify", "-verbose", "-certs", "-keystore", keystoreFileName,
>>>>               "-storetype", "JKS", "-storepass", "changeit", "-debug", jarFileName, alias)
>>>>               .shouldHaveExitValue(0)
>>>>               .shouldMatch(refClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)")
>>>>               .shouldMatch(testClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)");
>>>>   }
>>>>
>>>>   /**
>>>>    * it would be too easy to miss the actual test case by just renaming an identifier so that
>>>>    * the multi-byte encoded character would not any longer be broken across a line break.
>>>>    *
>>>>    * this test verifies that the actual test case is tested based on the manifest
>>>>    * and not based on the signature file because at the moment, the signature file
>>>>    * does not even contain the desired entry at all.
>>>>    *
>>>>    * this relies on the Manifest breaking lines unaware of bytes that belong to the same
>>>>    * multi-ybte utf characters.
>>>>    */
>>>>   static void verifyClassNameLineBroken(String jarFileName) throws IOException {
>>>>       byte[] eAcute = "é".getBytes(UTF_8);
>>>>       byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
>>>>
>>>>       try (
>>>>           JarFile jar = new JarFile(jarFileName);
>>>>       ) {
>>>>           if (jar.getManifest().getAttributes(testClassFilename) == null) {
>>>>               throw new AssertionError(testClassFilename + " not found in manifest");
>>>>           }
>>>>
>>>>           JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
>>>>           try (
>>>>               InputStream manifestIs = jar.getInputStream(manifestEntry);
>>>>           ) {
>>>>               int bytesMatched = 0;
>>>>               for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
>>>>                   if ((byte) b == eAcuteBroken[bytesMatched]) {
>>>>                       bytesMatched++;
>>>>                       if (bytesMatched == eAcuteBroken.length) {
>>>>                           break;
>>>>                       }
>>>>                   } else {
>>>>                       bytesMatched = 0;
>>>>                   }
>>>>               }
>>>>               if (bytesMatched < eAcuteBroken.length) {
>>>>                   throw new AssertionError("self-test failed: "
>>>>                           + "multi-byte utf-8 character not broken across lines");
>>>>               }
>>>>           }
>>>>       }
>>>>   }
>>>>
>>>>   static OutputAnalyzer tool(String tool, String... args)
>>>>           throws Throwable {
>>>>       JDKToolLauncher l = JDKToolLauncher.createUsingTestJDK(tool);
>>>>       for (String arg : args) {
>>>>           if (arg .startsWith("-J")) {
>>>>               l.addVMArg(arg.substring(2));
>>>>           } else {
>>>>               l.addToolArg(arg);
>>>>           }
>>>>       }
>>>>       return ProcessTools.executeCommand(l.getCommand());
>>>>   }
>>>>
>>>> }
>>>> END /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
>>>>
>>>>
>>>> There are three e with acutes, on lines 84, 89, and 227, just in case the mail suffers bad encoding which might not be absolutely unconceivable regarding the bug's subject.
>>>>
>>>> In contrast to the bug description it uses a nested class with a two-byte UTF-8 character rather than one in its own file. I chose to do it that way because then the complete test goes into one file and therefore I assume the overview is kept easier. The test still demonstrates exactly JDK-6695402's problem.
>>>>
>>>> It may not have been necessary to also test jar files with indexes but i consider it necessary to have signing unsigned as well as partially signed jars tested, so why not have one more case tested, too?
>>>>
>>>> There might also be a more elegant approach to get class files into a jar file as to get their contents through the class loader. I chose to use real class files rather than dummy contents in files named *.class or for instance plain text files in the jar the signing of which to be tested in order to stay as close to the original bug problem as possible even though I don't have any notion that it would make a difference. The amount of code used to copy the class file contents around is comparatively small with respect to the whole test case amount of code.
>>>>
>>>> For the demonstration that the multi-byte character actually makes the alleged difference, I concluded that it was necessary to have another class name not previously affected by the bug in order to compare the effects of just different names. Otherwise the signing could fail to any other reason undetectably.
>>>>
>>>> In order to express a condition to tell successful from failed test runs the best approach I found was to analyze the jarsigner tool's output which is in my opinion not the most desirable option. I'd have preferred output from a clearer structured api but then I guess the output format will not change too often in the foreseeable future. For instance the check for the s, m, and k flags is more pragmatical approach than obviously perfectly failsafe when operating with regular expressions to match it with the output. Other parts such as 'X.509' and 'CN=' are not even jarsigner tool resources. I put at least a reference to sun.security.tools.jarsigner.Resources.
>>>>
>>>> Finally, I afforded kind of a self-test that protects the test from undetected failing because renaming the main class which would move the two-byte unicode character to a position not suitable for the test. It may not be absolutely necessary.
>>>>
>>>>
>>>>
>>>> BEGIN PATH
>>>> diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
>>>> --- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
>>>> +++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Wed Sep 20 00:56:15 2017 +0200
>>>> @@ -28,6 +28,7 @@
>>>> import java.security.*;
>>>> import java.util.HashMap;
>>>> import java.io.ByteArrayOutputStream;
>>>> +import static java.nio.charset.StandardCharsets.UTF_8;
>>>>
>>>> /**
>>>> * This class is used to compute digests on sections of the Manifest.
>>>> @@ -112,7 +113,7 @@
>>>>        rawBytes = bytes;
>>>>        entries = new HashMap<>();
>>>>
>>>> -        ByteArrayOutputStream baos = new ByteArrayOutputStream();
>>>> +        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();
>>>>
>>>>        Position pos = new Position();
>>>>
>>>> @@ -131,50 +132,40 @@
>>>>
>>>>            if (len > 6) {
>>>>                if (isNameAttr(bytes, start)) {
>>>> -                    StringBuilder nameBuf = new StringBuilder(sectionLen);
>>>> +                    nameBuf.reset();
>>>> +                    nameBuf.write(bytes, start+6, len-6);
>>>>
>>>> -                    try {
>>>> -                        nameBuf.append(
>>>> -                            new String(bytes, start+6, len-6, "UTF8"));
>>>> +                    int i = start + len;
>>>> +                    if ((i-start) < sectionLen) {
>>>> +                        if (bytes[i] == '\r') {
>>>> +                            i += 2;
>>>> +                        } else {
>>>> +                            i += 1;
>>>> +                        }
>>>> +                    }
>>>>
>>>> -                        int i = start + len;
>>>> -                        if ((i-start) < sectionLen) {
>>>> -                            if (bytes[i] == '\r') {
>>>> -                                i += 2;
>>>> -                            } else {
>>>> -                                i += 1;
>>>> -                            }
>>>> +                    while ((i-start) < sectionLen) {
>>>> +                        if (bytes[i++] == ' ') {
>>>> +                            // name is wrapped
>>>> +                            int wrapStart = i;
>>>> +                            while (((i-start) < sectionLen)
>>>> +                                    && (bytes[i++] != '\n'));
>>>> +                            if (bytes[i-1] != '\n')
>>>> +                                return; // XXX: exception?
>>>> +                            int wrapLen;
>>>> +                            if (bytes[i-2] == '\r')
>>>> +                                wrapLen = i-wrapStart-2;
>>>> +                            else
>>>> +                                wrapLen = i-wrapStart-1;
>>>> +
>>>> +                            nameBuf.write(bytes, wrapStart, wrapLen);
>>>> +                        } else {
>>>> +                            break;
>>>>                        }
>>>> +                    }
>>>>
>>>> -                        while ((i-start) < sectionLen) {
>>>> -                            if (bytes[i++] == ' ') {
>>>> -                                // name is wrapped
>>>> -                                int wrapStart = i;
>>>> -                                while (((i-start) < sectionLen)
>>>> -                                        && (bytes[i++] != '\n'));
>>>> -                                    if (bytes[i-1] != '\n')
>>>> -                                        return; // XXX: exception?
>>>> -                                    int wrapLen;
>>>> -                                    if (bytes[i-2] == '\r')
>>>> -                                        wrapLen = i-wrapStart-2;
>>>> -                                    else
>>>> -                                        wrapLen = i-wrapStart-1;
>>>> -
>>>> -                            nameBuf.append(new String(bytes, wrapStart,
>>>> -                                                      wrapLen, "UTF8"));
>>>> -                            } else {
>>>> -                                break;
>>>> -                            }
>>>> -                        }
>>>> -
>>>> -                        entries.put(nameBuf.toString(),
>>>> -                            new Entry(start, sectionLen, sectionLenWithBlank,
>>>> -                                rawBytes));
>>>> -
>>>> -                    } catch (java.io.UnsupportedEncodingException uee) {
>>>> -                        throw new IllegalStateException(
>>>> -                            "UTF8 not available on platform");
>>>> -                    }
>>>> +                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
>>>> +                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
>>>>                }
>>>>            }
>>>>            start = pos.startOfNext;
>>>> END PATCH
>>>>
>>>>
>>>> The patch is mostly so big because indentation has changed in many lines.
>>>>
>>>> There was baos there before without apparent use. The patch renames it and puts it into use. It came in very handy because if it would not have been there already, I would have had to defined one of my own.
>>>>
>>>> The main point of the patch is that manifest section names that are broken across lines at possibly arbitrary bytes are no longer converted into strings for each manifest line and then joined. Parts of names broken across lines are now joined first when they are still byte sequences and only when the complete byte sequence is available after processing all manifest lines that contain the same manifest section name decoded into a unicode string.
>>>>
>>>> For decoding the bytes into a string I chose a different string constructor than was used before that does not any longer declare UnsupportedEncodingException rendering the try/catch redundant. It couldn't have ever occurred anyway taking into consideration that UTF-8 is mandatory for every Java platform, StandardCharsets says. The difference according to the documentation is that the previously     used String constructor returned undefined strings for invalid byte sequences whereas the one used in the patch will replace unparseable portions with valid 'unknown' characters.
>>>>
>>>> One question I cannot still answer yet is how the ManifestDigester can be changed at all without complete test coverage or risking to break existing signatures. I already started on that but it's quite a piece of work to almost formally prove that all manifests will continue to produce identical digests, except of course for the ones now fixed.
>>>>
>>>> Regards,
>>>> Philipp
>>>>
>>>>
>>>>> On 17.09.2017 21:25, Philipp Kunz wrote:
>>>>>
>>>>> Hello Vincent
>>>>>
>>>>> I narrowed the error down so far and suspect now that it is an effect of sun.security.util.ManifestDigester's constructor, lines 134 and 163-164, in combination with java.util.jar.Manifest.make72Safe. Manifest breaks multi-byte characters in manifest section names across lines and ManifestDigester fails to restore them correctly, which both sounds wrong but ManifestDigester sure is.
>>>>>
>>>>> Just fixing it would be too tempting but then I would not know (I mean not know for sure with evidence from tests) if any existing jar-signature would still be valid and I don't want to break existing signatures. When I had a look at the tests specific for Manifest and ManifestDigester I found very little. There are many more different situations and corner cases and combinations thereof to cover with tests than it looks like at first glance so that writing tests that cover all relevant cases looks to me like quite an effort. I have the impression that test coverage was not a priority when the subjected code was developed or at least the tests might not have been contributed to the open JDK project. Hence, while I'm continuing to complete these tests I miss, if       someone has an idea how to simplify that so that I can still convince at least myself that no existing signature will break or where possibly more testcases are that I haven't discovered so far, please let me know.
>>>>>
>>>>> I'm also wondering how much performance should be taken into consideration. There are a few hints such as reusing byte arrays that suggest that it is an issue. Will a patch be rejected if it slows down some tests beyond a certain limit for so little added value or what might be the acceptance criteria here? I don't expect insuperable difficulties with performance but would still appreciate some general idea.
>>>>>
>>>>> My desired or currently preferred approach to fix JDK-6695402 would be to let ManifestDigester any kind of use or extend Manifest in order to let Manifest identify the manifest sections thereby preventing the parsing duplicated that identifies the manifest sections.
>>>>>
>>>>> As a new contributor I could use and would appreciate some guidance particularly about what kind and completeness of test coverage and performance tuning applies or is suggested in the context of the given bug. Otherwise I fear I might contribute too many tests which would have to be reviewed and maintained or quite some work would be for nothing if a patch would not satisfy performance requirements.
>>>>>
>>>>> Regards,
>>>>> Philipp
>>>>>
>>>>>
>>>>>
>>>>>> On 01.09.2017 15:20, Vincent Ryan wrote:
>>>>>>
>>>>>> That all sounds fine. Let me know when your patch is ready to submit.
>>>>>> Thanks.
>>>>>>
>>>>>>
>>>>>>
>>>>>>> On 1 Sep 2017, at 13:15, Philipp Kunz <[hidden email]>
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hello Vincent
>>>>>>>
>>>>>>> Thank you for sponsoring!
>>>>>>> So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here:
>>>>>>> http://www.oracle.com/technetwork/community/oca-486395.html#p
>>>>>>> -> Paratix GmbH
>>>>>>> Therefore I think I have followed the steps in
>>>>>>> http://openjdk.java.net/contribute/
>>>>>>> at least the ones before actual patch submission.
>>>>>>>
>>>>>>> Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
>>>>>>> make run-test-tier1
>>>>>>> make run-test TEST="jdk/test"
>>>>>>> they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
>>>>>>> make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
>>>>>>> then it reports zero errors and failures which may be a good starting point.
>>>>>>> Do you think that sounds reasonable or do you have another suggestion how to run the tests?
>>>>>>>
>>>>>>> Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of
>>>>>>> http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java
>>>>>>> . This way, I try to demonstrate the improvement.
>>>>>>>
>>>>>>> I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);
>>>>>>>
>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157
>>>>>>>
>>>>>>> So I'll try to remove it first including the whole four line if block.
>>>>>>>
>>>>>>> Philipp
>>>>>>>
>>>>>>>
>>>>>>>> On 01.09.2017 10:00, Vincent Ryan wrote:
>>>>>>>>
>>>>>>>> Hello Philipp,
>>>>>>>>
>>>>>>>> I’m happy to sponsor your fix for JDK 10. Have you followed these steps:
>>>>>>>> http://openjdk.java.net/contribute/
>>>>>>>> ?
>>>>>>>>
>>>>>>>> Thanks.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>> On 1 Sep 2017, at 08:58, Vincent Ryan <[hidden email]>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> Moved to security-dev
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> On 1 Sep 2017, at 08:28, Philipp Kunz <[hidden email]>
>>>>>>>>>> wrote:
>>>>>>>>>>
>>>>>>>>>> Hello everyone
>>>>>>>>>>
>>>>>>>>>> I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-6695402
>>>>>>>>>> . This also looks like it may not be too big a piece for a first contribution.
>>>>>>>>>>
>>>>>>>>>> I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:
>>>>>>>>>>
>>>>>>>>>> May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?
>>>>>>>>>>
>>>>>>>>>> Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.
>>>>>>>>>>
>>>>>>>>>> One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.
>>>>>>>>>>
>>>>>>>>>> I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.
>>>>>>>>>>
>>>>>>>>>> Philipp Kunz
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> <Mail Attachment.png>
>>>>>
>>>>> Paratix GmbH
>>>>> St Peterhofstatt 11
>>>>> 8001 Zürich
>>>>>
>>>>> +41 (0)76 397 79 35
>>>>>
>>>>> [hidden email]
>>
>> --
>>
>>
>> Gruss Philipp
>>
>>
>>
>>
>> <Mail Attachment.png>
>>
>> Paratix GmbH
>> St Peterhofstatt 11
>> 8001 Zürich
>>
>> +41 (0)76 397 79 35
>> [hidden email]
>

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Philipp Kunz
Hi Max

Thank you for the detailed assistance and I really hope it doesn't annoy you too much with so many beginner's mistakes. Every little point of yours seems to be absolutely justified in my point of view.

I did not understand where I could apply the readAllBytes. In case it still applies, would you give me another hint? I'd rather expect some potential for butifying verifyClassNameLineBroken but then I haven't found any really.

About where to place the declaration of nameBuf I was a little confused probably/obviously when there was a ByteArrayOutputStream before which, however, actually was not used into assuming that reusing the same buffer for all sections would result in better performance or so but that's certainly not a valid assumption now as I consider it again.

About the import static you convinced me but for statically importing java.nio.charset.StandardCharsets.UTF_8 which I prefer this way still. After you wrote I should decide myself, I removed other static imports. If there was kind of an unwritten rule not to use static imports generally that would be a compelling reason but I didn't bother to search for import static through the jdk sources.

The @modules was a left-over from previous versions.

I'm not sure where you meant I should put the test to exactly. In http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/test/jdk I cant find the parent folder sun you mentioned. In case there is some reorganization in progress not having arrived yet at the tip, could the new test be moved together with it? Or did you mean I should create the sun folder but then the existing tests would not be next to the new one? Did I look in the wrong place?

When updating the copyright years I was not sure if I should let the existing one, 2011, there in ManifestDigester or replace it with the current year which I did. When looking into other class files I saw none with more than two years but "," looks a little odd to me for indicating a range.

I assumed that I remove the JDK- prefix from the @bug tag and not the @test tag unless @bug is considered part of the @test.

Is it correct, that a maximum line width of 80 characters is the convention? In case I added some more breaks for that. I just wonder what other style guides I should have respected.

Just in case you think it would be more efficient I'd not object that a reviewer or some else would just change these little things to get over with it. On the other hand this way I learn it. If there is a suitable documentation that much detailed, I'd be glad if you could point me at it. That might save a few cycles.

Regards,
Philipp


---------------------------
diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
--- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
+++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Mon Sep 25 19:09:07 2017 +0200
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2011, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,6 +28,7 @@
 import java.security.*;
 import java.util.HashMap;
 import java.io.ByteArrayOutputStream;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 /**
  * This class is used to compute digests on sections of the Manifest.
@@ -112,8 +113,6 @@
         rawBytes = bytes;
         entries = new HashMap<>();
 
-        ByteArrayOutputStream baos = new ByteArrayOutputStream();
-
         Position pos = new Position();
 
         if (!findSection(0, pos))
@@ -131,50 +130,42 @@
 
             if (len > 6) {
                 if (isNameAttr(bytes, start)) {
-                    StringBuilder nameBuf = new StringBuilder(sectionLen);
+                    ByteArrayOutputStream nameBuf =
+                            new ByteArrayOutputStream();
+                    nameBuf.write(bytes, start+6, len-6);
 
-                    try {
-                        nameBuf.append(
-                            new String(bytes, start+6, len-6, "UTF8"));
+                    int i = start + len;
+                    if ((i-start) < sectionLen) {
+                        if (bytes[i] == '\r') {
+                            i += 2;
+                        } else {
+                            i += 1;
+                        }
+                    }
 
-                        int i = start + len;
-                        if ((i-start) < sectionLen) {
-                            if (bytes[i] == '\r') {
-                                i += 2;
-                            } else {
-                                i += 1;
-                            }
+                    while ((i-start) < sectionLen) {
+                        if (bytes[i++] == ' ') {
+                            // name is wrapped
+                            int wrapStart = i;
+                            while (((i-start) < sectionLen)
+                                    && (bytes[i++] != '\n'));
+                            if (bytes[i-1] != '\n')
+                                return; // XXX: exception?
+                            int wrapLen;
+                            if (bytes[i-2] == '\r')
+                                wrapLen = i-wrapStart-2;
+                            else
+                                wrapLen = i-wrapStart-1;
+
+                            nameBuf.write(bytes, wrapStart, wrapLen);
+                        } else {
+                            break;
                         }
+                    }
 
-                        while ((i-start) < sectionLen) {
-                            if (bytes[i++] == ' ') {
-                                // name is wrapped
-                                int wrapStart = i;
-                                while (((i-start) < sectionLen)
-                                        && (bytes[i++] != '\n'));
-                                    if (bytes[i-1] != '\n')
-                                        return; // XXX: exception?
-                                    int wrapLen;
-                                    if (bytes[i-2] == '\r')
-                                        wrapLen = i-wrapStart-2;
-                                    else
-                                        wrapLen = i-wrapStart-1;
-
-                            nameBuf.append(new String(bytes, wrapStart,
-                                                      wrapLen, "UTF8"));
-                            } else {
-                                break;
-                            }
-                        }
-
-                        entries.put(nameBuf.toString(),
-                            new Entry(start, sectionLen, sectionLenWithBlank,
-                                rawBytes));
-
-                    } catch (java.io.UnsupportedEncodingException uee) {
-                        throw new IllegalStateException(
-                            "UTF8 not available on platform");
-                    }
+                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
+                        new Entry(start, sectionLen, sectionLenWithBlank,
+                                rawBytes));
                 }
             }
             start = pos.startOfNext;
diff -r ddc25f646c2e test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
--- /dev/null    Thu Jan 01 00:00:00 1970 +0000
+++ b/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java    Mon Sep 25 19:09:07 2017 +0200
@@ -0,0 +1,196 @@
+/*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 6695402
+ * @summary verify signatures of jars containing classes with names
+ * with multi-byte unicode characters broken across lines
+ * @library /test/lib
+ */
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.jar.JarFile;
+import java.util.jar.Attributes.Name;
+import java.util.jar.JarEntry;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import jdk.test.lib.SecurityTools;
+import jdk.test.lib.util.JarUtils;
+
+public class MultibyteUnicodeName {
+
+    /**
+     * this class's name will break across lines in the jar manifest at the
+     * middle of a two-byte utf encoded character due to its e acute letter
+     * at its exact position.
+     *
+     * @see also eAcute in
+     * {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}
+     */
+    static class A1234567890B1234567890C1234567890D12345678éxyz { }
+    static class A1234567890B1234567890C1234567890D12345678exyz { }
+
+    static final String refClassFilename = MultibyteUnicodeName.class
+            .getSimpleName() + "$" +
+            A1234567890B1234567890C1234567890D12345678exyz.class
+            .getSimpleName() + ".class";
+    static final String testClassFilename = MultibyteUnicodeName.class
+            .getSimpleName() + "$" +
+            A1234567890B1234567890C1234567890D12345678éxyz.class
+            .getSimpleName() + ".class";
+   
+    static final String alias = "a";
+    static final String keystoreFileName = "test.jks";
+    static final String ManifestFileName = "MANIFEST.MF";
+   
+    public static void main(String[] args) throws Exception {
+        prepare();
+       
+        testSignJar("test.jar");
+        testSignJarNoManifest("test-no-manifest.jar");
+        testSignJarUpdate("test-update.jar", "test-updated.jar");
+    }
+   
+    static void prepare() throws Exception {
+        SecurityTools.keytool("-keystore", keystoreFileName, "-genkeypair",
+                "-storepass", "changeit", "-keypass", "changeit", "-storetype",
+                "JKS", "-alias", alias, "-dname", "CN=X", "-validity", "366")
+            .shouldHaveExitValue(0);
+       
+        Files.write(Paths.get(ManifestFileName), (Name.
+                MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8));
+        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(
+                refClassFilename), Paths.get(refClassFilename));
+        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(
+                testClassFilename), Paths.get(testClassFilename));
+    }
+   
+    static void testSignJar(String jarFileName) throws Exception {
+        JarUtils.createJar(jarFileName, ManifestFileName, refClassFilename,
+                testClassFilename);
+        verifyJarSignature(jarFileName);
+    }
+
+    static void testSignJarNoManifest(String jarFileName) throws Exception {
+        JarUtils.createJar(jarFileName, refClassFilename, testClassFilename);
+        verifyJarSignature(jarFileName);
+    }
+   
+    static void testSignJarUpdate(
+            String initialFileName, String updatedFileName
+    ) throws Exception {
+        JarUtils.createJar(initialFileName, ManifestFileName,
+                refClassFilename);
+        SecurityTools.jarsigner("-keystore", keystoreFileName, "-storetype",
+                "JKS", "-storepass", "changeit", "-debug", initialFileName,
+                alias)
+            .shouldHaveExitValue(0);
+        JarUtils.updateJar(initialFileName, updatedFileName,
+                testClassFilename);
+        verifyJarSignature(updatedFileName);
+    }
+   
+    static void verifyJarSignature(String jarFileName) throws Exception {
+        // actually sign the jar
+        SecurityTools.jarsigner("-keystore", keystoreFileName, "-storetype",
+                "JKS", "-storepass", "changeit", "-debug", jarFileName, alias)
+            .shouldHaveExitValue(0);
+
+        try (
+            JarFile jar = new JarFile(jarFileName);
+        ) {
+            verifyClassNameLineBroken(jar, testClassFilename);
+            verifyCodeSigners(jar, jar.getJarEntry(testClassFilename));
+            verifyCodeSigners(jar, jar.getJarEntry(refClassFilename));
+        }
+    }
+   
+    /**
+     * it would be too easy to miss the actual test case by just renaming an
+     * identifier so that the multi-byte encoded character would not any longer
+     * be broken across a line break.
+     *
+     * this test verifies that the actual test case is tested based on the
+     * manifest and not based on the signature file because at the moment, the
+     * signature file does not even contain the desired entry at all.
+     *
+     * this relies on the Manifest breaking lines unaware of bytes that belong
+     * to the same multi-ybte utf characters.
+     */
+    static void verifyClassNameLineBroken(JarFile jar, String className)
+    throws IOException {
+        byte[] eAcute = "é".getBytes(UTF_8); // U+00E9
+        byte[] eAcuteBroken =
+                new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
+       
+        if (jar.getManifest().getAttributes(className) == null) {
+            throw new AssertionError(className + " not found in manifest");
+        }
+       
+        JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
+        try (
+            InputStream manifestIs = jar.getInputStream(manifestEntry);
+        ) {
+            int bytesMatched = 0;
+            for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
+                if ((byte) b == eAcuteBroken[bytesMatched]) {
+                    bytesMatched++;
+                    if (bytesMatched == eAcuteBroken.length) {
+                        break;
+                    }
+                } else {
+                    bytesMatched = 0;
+                }
+            }
+            if (bytesMatched < eAcuteBroken.length) {
+                throw new AssertionError("self-test failed: multi-byte "
+                        + "utf-8 character not broken across lines");
+            }
+        }
+    }
+   
+    static void verifyCodeSigners(JarFile jar, JarEntry jarEntry)
+    throws IOException {
+        // codeSigners is initialized only after the entry has been read
+        try (
+            InputStream inputStream = jar.getInputStream(jarEntry);
+        ) {
+            while (inputStream.read() > -1);
+        }
+       
+        if (jarEntry.getCodeSigners() == null
+                || jarEntry.getCodeSigners().length == 0) {
+            throw new AssertionError(
+                    "no signing certificate found for " + jarEntry.getName());
+        }
+        // a check for the presence of code signers is sufficient to check
+        // bug JDK-6695402. no need to also verify the actual code signers
+        // attributes here.
+    }
+   
+}
---------------------------


On 24.09.2017 17:41, Wang Weijun wrote:
Some more suggestions on the test:

1. You can use readAllBytes() to ... err ... read all bytes. 

2. I normally don’t remove intermediate files. Jtreg will do an automatic cleanup, and it can be configured to retain those files when the test fails. They will be very helpful in debugging. 

Thanks
Max 

在 2017年9月24日,23:12,Weijun Wang [hidden email] 写道:

Great.

Some suggestions, just my own habit, you are free to decide yourself:

1. In ManifestDigester.java, I'd rather define nameBuf inside the if (isNameAttr(bytes, start)) block.

2. I normally don't use "import static" unless I can save a lot of keystrokes and still do not confuse anyone. Both in the test and in ManifestDigester.java.

3. In the test, there is no need to write "@run main MultibyteUnicodeName" if it is the only action (i.e. no other build/compile) and there is no modifier on main (i.e. no othervm/timeout etc).

Several tiny problems:

1. No need for @modules in the test. Maybe you used some internal classes in your previous version?

2. In the new consolidated repo, the test should be inside test/jdk/sun/..., please note the "jdk" after "test".

3. Please update the copyright years.

4. In the test, there should be no "JDK-" in the @test tag.

Thanks
Max

On Sep 24, 2017, at 7:51 PM, Philipp Kunz [hidden email] wrote:

Hi Max and Vincent

Thank you for your suggestions. It sure looks better now. I hope this time I got the patch added in the right format.

diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
--- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
+++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Sun Sep 24 10:34:00 2017 +0200
@@ -28,6 +28,7 @@
import java.security.*;
import java.util.HashMap;
import java.io.ByteArrayOutputStream;
+import static java.nio.charset.StandardCharsets.UTF_8;

/**
* This class is used to compute digests on sections of the Manifest.
@@ -112,7 +113,7 @@
       rawBytes = bytes;
       entries = new HashMap<>();

-        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();

       Position pos = new Position();

@@ -131,50 +132,40 @@

           if (len > 6) {
               if (isNameAttr(bytes, start)) {
-                    StringBuilder nameBuf = new StringBuilder(sectionLen);
+                    nameBuf.reset();
+                    nameBuf.write(bytes, start+6, len-6);

-                    try {
-                        nameBuf.append(
-                            new String(bytes, start+6, len-6, "UTF8"));
+                    int i = start + len;
+                    if ((i-start) < sectionLen) {
+                        if (bytes[i] == '\r') {
+                            i += 2;
+                        } else {
+                            i += 1;
+                        }
+                    }

-                        int i = start + len;
-                        if ((i-start) < sectionLen) {
-                            if (bytes[i] == '\r') {
-                                i += 2;
-                            } else {
-                                i += 1;
-                            }
+                    while ((i-start) < sectionLen) {
+                        if (bytes[i++] == ' ') {
+                            // name is wrapped
+                            int wrapStart = i;
+                            while (((i-start) < sectionLen)
+                                    && (bytes[i++] != '\n'));
+                            if (bytes[i-1] != '\n')
+                                return; // XXX: exception?
+                            int wrapLen;
+                            if (bytes[i-2] == '\r')
+                                wrapLen = i-wrapStart-2;
+                            else
+                                wrapLen = i-wrapStart-1;
+
+                            nameBuf.write(bytes, wrapStart, wrapLen);
+                        } else {
+                            break;
                       }
+                    }

-                        while ((i-start) < sectionLen) {
-                            if (bytes[i++] == ' ') {
-                                // name is wrapped
-                                int wrapStart = i;
-                                while (((i-start) < sectionLen)
-                                        && (bytes[i++] != '\n'));
-                                    if (bytes[i-1] != '\n')
-                                        return; // XXX: exception?
-                                    int wrapLen;
-                                    if (bytes[i-2] == '\r')
-                                        wrapLen = i-wrapStart-2;
-                                    else
-                                        wrapLen = i-wrapStart-1;
-
-                            nameBuf.append(new String(bytes, wrapStart,
-                                                      wrapLen, "UTF8"));
-                            } else {
-                                break;
-                            }
-                        }
-
-                        entries.put(nameBuf.toString(),
-                            new Entry(start, sectionLen, sectionLenWithBlank,
-                                rawBytes));
-
-                    } catch (java.io.UnsupportedEncodingException uee) {
-                        throw new IllegalStateException(
-                            "UTF8 not available on platform");
-                    }
+                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
+                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
               }
           }
           start = pos.startOfNext;
diff -r ddc25f646c2e test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
--- /dev/null    Thu Jan 01 00:00:00 1970 +0000
+++ b/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java    Sun Sep 24 10:34:00 2017 +0200
@@ -0,0 +1,209 @@
+/*
+ * Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug JDK-6695402
+ * @summary verify signatures of jars containing classes with names with multi-
+ *          byte unicode characters broken across lines
+ * @library /test/lib
+ * @modules jdk.jartool/sun.tools.jar
+ *          jdk.jartool/sun.security.tools.jarsigner
+ * @run main MultibyteUnicodeName
+ */
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.jar.JarFile;
+import java.util.jar.Manifest;
+import java.util.Arrays;
+import java.util.Map;
+import java.util.jar.Attributes.Name;
+import java.util.jar.JarEntry;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import static jdk.test.lib.SecurityTools.jarsigner;
+import static jdk.test.lib.SecurityTools.keytool;
+import static jdk.test.lib.util.JarUtils.createJar;
+import static jdk.test.lib.util.JarUtils.updateJar;
+
+public class MultibyteUnicodeName {
+
+    /**
+     * this class's name will break across lines in the jar manifest at the middle of
+     * a two-byte utf encoded character due to its e acute letter at its exact position.
+     * 
+     * @see also eAcute in {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}
+     */
+    static class A1234567890B1234567890C1234567890D12345678éxyz { }
+    static class A1234567890B1234567890C1234567890D12345678exyz { }
+
+    static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() + 
+            "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
+    static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() + 
+            "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";
+    
+    static final String alias = "a";
+    static final String keystoreFileName = "test.jks";
+    static final String ManifestFileName = "MANIFEST.MF";
+    
+    public static void main(String[] args) throws Exception {
+        try {
+            prepare();
+            
+            testSignJar("test.jar");
+            testSignJarNoManifest("test-no-manifest.jar");
+            testSignJarUpdate("test-update.jar", "test-updated.jar");
+            
+        } finally {
+            Files.deleteIfExists(Paths.get(keystoreFileName));
+            Files.deleteIfExists(Paths.get(ManifestFileName));
+            Files.deleteIfExists(Paths.get(refClassFilename));
+            Files.deleteIfExists(Paths.get(testClassFilename));
+        }
+    }
+    
+    static void prepare() throws Exception {
+        keytool("-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
+                "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
+                "-validity", "366").shouldHaveExitValue(0);
+        
+        Files.write(Paths.get(ManifestFileName),
+                (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
+        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(refClassFilename), 
+                Paths.get(refClassFilename));
+        Files.copy(MultibyteUnicodeName.class.getResourceAsStream(testClassFilename), 
+                Paths.get(testClassFilename));
+    }
+    
+    static void testSignJar(String jarFileName) throws Exception {
+        try {
+            createJar(jarFileName, ManifestFileName, refClassFilename, testClassFilename);
+            verifyJarSignature(jarFileName);
+
+        } finally {
+            Files.deleteIfExists(Paths.get(jarFileName));
+        }
+    }
+
+    static void testSignJarNoManifest(String jarFileName) throws Exception {
+        try {
+            createJar(jarFileName, refClassFilename, testClassFilename);
+            verifyJarSignature(jarFileName);
+
+        } finally {
+            Files.deleteIfExists(Paths.get(jarFileName));
+        }
+    }
+    
+    static void testSignJarUpdate(String initialFileName, String updatedFileName) throws Exception {
+        try {
+            createJar(initialFileName, ManifestFileName, refClassFilename);
+            jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
+                    "-storepass", "changeit", "-debug", initialFileName, alias)
+                .shouldHaveExitValue(0);
+            updateJar(initialFileName, updatedFileName, testClassFilename);
+            verifyJarSignature(updatedFileName);
+
+        } finally {
+            Files.deleteIfExists(Paths.get(initialFileName));
+            Files.deleteIfExists(Paths.get(updatedFileName));
+        }
+    }
+    
+    static void verifyJarSignature(String jarFileName) throws Exception {
+        // actually sign the jar
+        jarsigner("-keystore", keystoreFileName, "-storetype", "JKS",
+                "-storepass", "changeit", "-debug", jarFileName, alias)
+                .shouldHaveExitValue(0);
+
+        try (
+            JarFile jar = new JarFile(jarFileName);
+        ) {
+            verifyClassNameLineBroken(jar, testClassFilename);
+            verifyCodeSigners(jar, jar.getJarEntry(testClassFilename));
+            verifyCodeSigners(jar, jar.getJarEntry(refClassFilename));
+        }
+    }
+    
+    /**
+     * it would be too easy to miss the actual test case by just renaming an identifier so that
+     * the multi-byte encoded character would not any longer be broken across a line break.
+     *
+     * this test verifies that the actual test case is tested based on the manifest
+     * and not based on the signature file because at the moment, the signature file
+     * does not even contain the desired entry at all.
+     *
+     * this relies on the Manifest breaking lines unaware of bytes that belong to the same
+     * multi-ybte utf characters.
+     */
+    static void verifyClassNameLineBroken(JarFile jar, String className) throws IOException {
+        byte[] eAcute = "é".getBytes(UTF_8);
+        byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};
+        
+        if (jar.getManifest().getAttributes(className) == null) {
+            throw new AssertionError(className + " not found in manifest");
+        }
+        
+        JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
+        try (
+            InputStream manifestIs = jar.getInputStream(manifestEntry);
+        ) {
+            int bytesMatched = 0;
+            for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
+                if ((byte) b == eAcuteBroken[bytesMatched]) {
+                    bytesMatched++;
+                    if (bytesMatched == eAcuteBroken.length) {
+                        break;
+                    }
+                } else {
+                    bytesMatched = 0;
+                }
+            }
+            if (bytesMatched < eAcuteBroken.length) {
+                throw new AssertionError("self-test failed: "
+                        + "multi-byte utf-8 character not broken across lines");
+            }
+        }
+    }
+    
+    static void verifyCodeSigners(JarFile jar, JarEntry jarEntry) throws IOException {
+        // codeSigners is initialized only after the entry has been read
+        try (
+            InputStream inputStream = jar.getInputStream(jarEntry);
+        ) {
+            while (inputStream.read() > -1);
+            
+            if (jarEntry.getCodeSigners() == null || jarEntry.getCodeSigners().length == 0) {
+                throw new AssertionError("no signing certificate found for " + jarEntry.getName());
+            }
+        }
+        
+        // a check for the presence of code signers is sufficient to check bug JDK-6695402.
+        // no need to also verify the actual code signers attributes here.
+    }
+    
+}

Regards,
Philipp



On 20.09.2017 03:41, Weijun Wang wrote:
Hi Philipp

The change mostly looks fine. You might want to put everything into a patch file so Vincent can recreate a webrev and post it to cr.openjdk.java.net.

One thing I would suggest for the test is that instead of using jarsigner -verify and check the text in output you can open it as a JarFile, consume the content of an entry, and look into its getCertificates().

Also, you might want to reuse test/lib/jdk/test/lib/SecurityTools.java, and there is also JarUtils.java you can use. Maybe Files.copy(InputStream,Path) can be used in copyClassToFile().

Thanks
Max


On Sep 20, 2017, at 7:41 AM, Philipp Kunz [hidden email]
wrote:

Hello Vincent

Here may be the fix for JDK-6695402. First a test.

BEGIN /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java
/*
* Copyright (c) 1999, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit 
www.oracle.com
if you need additional information or have any
* questions.
*/

/*
* @test
* @bug JDK-6695402
* @summary verify signatures of jars containing classes with names with multi-
*          byte unicode characters broken across lines
* @library /test/lib
* @modules jdk.jartool/sun.tools.jar
*          jdk.jartool/sun.security.tools.jarsigner
* @build jdk.test.lib.JDKToolLauncher
*        jdk.test.lib.process.*
* @run main MultibyteUnicodeName
*/

import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException;
import java.nio.file.Files;
import java.nio.file.Paths;
import java.util.jar.JarFile;
import java.util.jar.Manifest;
import java.util.stream.Collectors;
import java.util.Arrays;
import java.util.Map;
import java.util.jar.Attributes.Name;
import java.util.jar.JarEntry;

import static java.nio.charset.StandardCharsets.UTF_8;

import sun.security.tools.jarsigner.Resources;

import jdk.test.lib.JDKToolLauncher;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.process.ProcessTools;

public class MultibyteUnicodeName {

  public static void main(String[] args) throws Throwable {
      try {
          prepare();

          testSignJar("test.jar");
          testSignJarNoManifest("test-no-manifest.jar");
          testSignJarUpdate("test-update.jar");
          testSignJarWithIndex("test-index.jar");
          testSignJarAddIndex("test-add-index.jar");

      } finally {
          Files.deleteIfExists(Paths.get(keystoreFileName));
          Files.deleteIfExists(Paths.get(ManifestFileName));
          Files.deleteIfExists(Paths.get(refClassFilename));
          Files.deleteIfExists(Paths.get(testClassFilename));
      }
  }

  static final String alias = "a";
  static final String keystoreFileName = "test.jks";
  static final String ManifestFileName = "MANIFEST.MF";

  static class A1234567890B1234567890C1234567890D12345678exyz { }
  static class A1234567890B1234567890C1234567890D12345678éxyz { }

  static final String refClassFilename = MultibyteUnicodeName.class.getSimpleName() + 
          "$" + A1234567890B1234567890C1234567890D12345678exyz.class.getSimpleName() + ".class";
  static final String testClassFilename = MultibyteUnicodeName.class.getSimpleName() + 
          "$" + A1234567890B1234567890C1234567890D12345678éxyz.class.getSimpleName() + ".class";

  static void prepare() throws Throwable {
      tool("keytool", "-keystore", keystoreFileName, "-genkeypair", "-storepass", "changeit",
              "-keypass", "changeit", "-storetype", "JKS", "-alias", alias, "-dname", "CN=X",
              "-validity", "366")
              .shouldHaveExitValue(0);

      Files.write(Paths.get(ManifestFileName),
              (Name.MANIFEST_VERSION.toString() + ": 1.0\r\n").getBytes(UTF_8.name()));
      copyClassToFile(refClassFilename);
      copyClassToFile(testClassFilename);
  }

  static void copyClassToFile(String classFilename) throws IOException {
      try (
          InputStream asStream = MultibyteUnicodeName.class.getResourceAsStream(classFilename);
          ByteArrayOutputStream buf = new ByteArrayOutputStream();
      ) {
          int b;
          while ((b = asStream.read()) != -1) buf.write(b);
          Files.write(Paths.get(classFilename), buf.toByteArray());
      }
  }

  static void testSignJar(String jarFileName) throws Throwable {
      try {
          tool("jar", "cvfm", jarFileName, 
                  ManifestFileName, refClassFilename, testClassFilename)
                  .shouldHaveExitValue(0);
          verifyJarSignature(jarFileName);

      } finally {
          Files.deleteIfExists(Paths.get(jarFileName));
      }
  }

  static void testSignJarNoManifest(String jarFileName) throws Throwable {
      try {
          tool("jar", "cvf", jarFileName, refClassFilename, testClassFilename)
              .shouldHaveExitValue(0);
          verifyJarSignature(jarFileName);

      } finally {
          Files.deleteIfExists(Paths.get(jarFileName));
      }
  }

  static void testSignJarUpdate(String jarFileName) throws Throwable {
      try {
          tool("jar", "cvfm", jarFileName, ManifestFileName, refClassFilename)
              .shouldHaveExitValue(0);
          tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
                  "-storepass", "changeit", "-debug", jarFileName, alias)
              .shouldHaveExitValue(0);
          tool("jar", "uvf", jarFileName, testClassFilename)
              .shouldHaveExitValue(0);
          verifyJarSignature(jarFileName);

      } finally {
          Files.deleteIfExists(Paths.get(jarFileName));
      }
  }

  static void testSignJarWithIndex(String jarFileName) throws Throwable {
      try {
          tool("jar", "cvfm", jarFileName, 
                  ManifestFileName, refClassFilename, testClassFilename)
              .shouldHaveExitValue(0);
          tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
          verifyJarSignature(jarFileName);

      } finally {
          Files.deleteIfExists(Paths.get(jarFileName));
      }
  }

  static void testSignJarAddIndex(String jarFileName) throws Throwable {
      try {
          tool("jar", "cvfm", jarFileName, 
                  ManifestFileName, refClassFilename, testClassFilename)
              .shouldHaveExitValue(0);
          tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
                  "-storepass", "changeit", "-debug", jarFileName, alias)
              .shouldHaveExitValue(0);
          tool("jar", "iv", jarFileName).shouldHaveExitValue(0);
          verifyJarSignature(jarFileName);

      } finally {
          Files.deleteIfExists(Paths.get(jarFileName));
      }
  }

  static Map<String, String> jarsignerResources = Arrays.stream(new Resources().getContents()).
          collect(Collectors.toMap(e -> (String) e[0], e -> (String) e[1]));

  static void verifyJarSignature(String jarFileName) throws Throwable {
      // actually sign the jar
      tool("jarsigner", "-keystore", keystoreFileName, "-storetype", "JKS",
              "-storepass", "changeit", "-debug", jarFileName, alias)
              .shouldHaveExitValue(0);

      verifyClassNameLineBroken(jarFileName);

      // check for no unsigned entries
      tool("jarsigner", "-verify", "-keystore", keystoreFileName, "-storetype", "JKS",
              "-storepass", "changeit", "-debug", jarFileName, alias)
              .shouldHaveExitValue(0)
              .shouldNotContain(jarsignerResources.get(
                      "This.jar.contains.unsigned.entries.which.have.not.been.integrity.checked."));

      // check that both classes with and without multi-byte unicode characters in their names are signed
      tool("jarsigner", "-verify", "-verbose", "-keystore", keystoreFileName, "-storetype", "JKS",
              "-storepass", "changeit", "-debug", jarFileName, alias)
              .shouldHaveExitValue(0)
              .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + refClassFilename.replaceAll("\\$", "\\\\\\$") + "$")
              .shouldMatch("^" + jarsignerResources.get("s") + jarsignerResources.get("m") + jarsignerResources.get("k") + ".+" + testClassFilename.replaceAll("\\$", "\\\\\\$") + "$");

      // check that both classes with and without multi-byte unicode characters in their names have signing certificates
      tool("jarsigner", "-verify", "-verbose", "-certs", "-keystore", keystoreFileName, 
              "-storetype", "JKS", "-storepass", "changeit", "-debug", jarFileName, alias)
              .shouldHaveExitValue(0)
              .shouldMatch(refClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)")
              .shouldMatch(testClassFilename.replaceAll("\\$", "\\\\\\$") + "\\s*\\R*\\s*(\\[" + jarsignerResources.get("entry.was.signed.on") + " .*\\]\\R*)?\\s*X.509, CN=X \\(" + alias + "\\)");
  }

  /**
   * it would be too easy to miss the actual test case by just renaming an identifier so that
   * the multi-byte encoded character would not any longer be broken across a line break.
   *
   * this test verifies that the actual test case is tested based on the manifest
   * and not based on the signature file because at the moment, the signature file
   * does not even contain the desired entry at all.
   *
   * this relies on the Manifest breaking lines unaware of bytes that belong to the same
   * multi-ybte utf characters.
   */
  static void verifyClassNameLineBroken(String jarFileName) throws IOException {
      byte[] eAcute = "é".getBytes(UTF_8);
      byte[] eAcuteBroken = new byte[] {eAcute[0], '\r', '\n', ' ', eAcute[1]};

      try (
          JarFile jar = new JarFile(jarFileName);
      ) {
          if (jar.getManifest().getAttributes(testClassFilename) == null) {
              throw new AssertionError(testClassFilename + " not found in manifest");
          }

          JarEntry manifestEntry = jar.getJarEntry(JarFile.MANIFEST_NAME);
          try (
              InputStream manifestIs = jar.getInputStream(manifestEntry);
          ) {
              int bytesMatched = 0;
              for (int b = manifestIs.read(); b > -1; b = manifestIs.read()) {
                  if ((byte) b == eAcuteBroken[bytesMatched]) {
                      bytesMatched++;
                      if (bytesMatched == eAcuteBroken.length) {
                          break;
                      }
                  } else {
                      bytesMatched = 0;
                  }
              }
              if (bytesMatched < eAcuteBroken.length) {
                  throw new AssertionError("self-test failed: "
                          + "multi-byte utf-8 character not broken across lines");
              }
          }
      }
  }

  static OutputAnalyzer tool(String tool, String... args)
          throws Throwable {
      JDKToolLauncher l = JDKToolLauncher.createUsingTestJDK(tool);
      for (String arg : args) {
          if (arg .startsWith("-J")) {
              l.addVMArg(arg.substring(2));
          } else {
              l.addToolArg(arg);
          }
      }
      return ProcessTools.executeCommand(l.getCommand());
  }

}
END /jdk10/jdk/test/sun/security/tools/jarsigner/MultibyteUnicodeName.java


There are three e with acutes, on lines 84, 89, and 227, just in case the mail suffers bad encoding which might not be absolutely unconceivable regarding the bug's subject.

In contrast to the bug description it uses a nested class with a two-byte UTF-8 character rather than one in its own file. I chose to do it that way because then the complete test goes into one file and therefore I assume the overview is kept easier. The test still demonstrates exactly JDK-6695402's problem.

It may not have been necessary to also test jar files with indexes but i consider it necessary to have signing unsigned as well as partially signed jars tested, so why not have one more case tested, too?

There might also be a more elegant approach to get class files into a jar file as to get their contents through the class loader. I chose to use real class files rather than dummy contents in files named *.class or for instance plain text files in the jar the signing of which to be tested in order to stay as close to the original bug problem as possible even though I don't have any notion that it would make a difference. The amount of code used to copy the class file contents around is comparatively small with respect to the whole test case amount of code.

For the demonstration that the multi-byte character actually makes the alleged difference, I concluded that it was necessary to have another class name not previously affected by the bug in order to compare the effects of just different names. Otherwise the signing could fail to any other reason undetectably.

In order to express a condition to tell successful from failed test runs the best approach I found was to analyze the jarsigner tool's output which is in my opinion not the most desirable option. I'd have preferred output from a clearer structured api but then I guess the output format will not change too often in the foreseeable future. For instance the check for the s, m, and k flags is more pragmatical approach than obviously perfectly failsafe when operating with regular expressions to match it with the output. Other parts such as 'X.509' and 'CN=' are not even jarsigner tool resources. I put at least a reference to sun.security.tools.jarsigner.Resources.

Finally, I afforded kind of a self-test that protects the test from undetected failing because renaming the main class which would move the two-byte unicode character to a position not suitable for the test. It may not be absolutely necessary.



BEGIN PATH
diff -r ddc25f646c2e src/java.base/share/classes/sun/security/util/ManifestDigester.java
--- a/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Thu Aug 31 22:21:20 2017 -0700
+++ b/src/java.base/share/classes/sun/security/util/ManifestDigester.java    Wed Sep 20 00:56:15 2017 +0200
@@ -28,6 +28,7 @@
import java.security.*;
import java.util.HashMap;
import java.io.ByteArrayOutputStream;
+import static java.nio.charset.StandardCharsets.UTF_8;

/**
* This class is used to compute digests on sections of the Manifest.
@@ -112,7 +113,7 @@
       rawBytes = bytes;
       entries = new HashMap<>();

-        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        ByteArrayOutputStream nameBuf = new ByteArrayOutputStream();

       Position pos = new Position();

@@ -131,50 +132,40 @@

           if (len > 6) {
               if (isNameAttr(bytes, start)) {
-                    StringBuilder nameBuf = new StringBuilder(sectionLen);
+                    nameBuf.reset();
+                    nameBuf.write(bytes, start+6, len-6);

-                    try {
-                        nameBuf.append(
-                            new String(bytes, start+6, len-6, "UTF8"));
+                    int i = start + len;
+                    if ((i-start) < sectionLen) {
+                        if (bytes[i] == '\r') {
+                            i += 2;
+                        } else {
+                            i += 1;
+                        }
+                    }

-                        int i = start + len;
-                        if ((i-start) < sectionLen) {
-                            if (bytes[i] == '\r') {
-                                i += 2;
-                            } else {
-                                i += 1;
-                            }
+                    while ((i-start) < sectionLen) {
+                        if (bytes[i++] == ' ') {
+                            // name is wrapped
+                            int wrapStart = i;
+                            while (((i-start) < sectionLen)
+                                    && (bytes[i++] != '\n'));
+                            if (bytes[i-1] != '\n')
+                                return; // XXX: exception?
+                            int wrapLen;
+                            if (bytes[i-2] == '\r')
+                                wrapLen = i-wrapStart-2;
+                            else
+                                wrapLen = i-wrapStart-1;
+
+                            nameBuf.write(bytes, wrapStart, wrapLen);
+                        } else {
+                            break;
                       }
+                    }

-                        while ((i-start) < sectionLen) {
-                            if (bytes[i++] == ' ') {
-                                // name is wrapped
-                                int wrapStart = i;
-                                while (((i-start) < sectionLen)
-                                        && (bytes[i++] != '\n'));
-                                    if (bytes[i-1] != '\n')
-                                        return; // XXX: exception?
-                                    int wrapLen;
-                                    if (bytes[i-2] == '\r')
-                                        wrapLen = i-wrapStart-2;
-                                    else
-                                        wrapLen = i-wrapStart-1;
-
-                            nameBuf.append(new String(bytes, wrapStart,
-                                                      wrapLen, "UTF8"));
-                            } else {
-                                break;
-                            }
-                        }
-
-                        entries.put(nameBuf.toString(),
-                            new Entry(start, sectionLen, sectionLenWithBlank,
-                                rawBytes));
-
-                    } catch (java.io.UnsupportedEncodingException uee) {
-                        throw new IllegalStateException(
-                            "UTF8 not available on platform");
-                    }
+                    entries.put(new String(nameBuf.toByteArray(), UTF_8),
+                        new Entry(start, sectionLen, sectionLenWithBlank, rawBytes));
               }
           }
           start = pos.startOfNext;
END PATCH


The patch is mostly so big because indentation has changed in many lines.

There was baos there before without apparent use. The patch renames it and puts it into use. It came in very handy because if it would not have been there already, I would have had to defined one of my own.

The main point of the patch is that manifest section names that are broken across lines at possibly arbitrary bytes are no longer converted into strings for each manifest line and then joined. Parts of names broken across lines are now joined first when they are still byte sequences and only when the complete byte sequence is available after processing all manifest lines that contain the same manifest section name decoded into a unicode string.

For decoding the bytes into a string I chose a different string constructor than was used before that does not any longer declare UnsupportedEncodingException rendering the try/catch redundant. It couldn't have ever occurred anyway taking into consideration that UTF-8 is mandatory for every Java platform, StandardCharsets says. The difference according to the documentation is that the previously     used String constructor returned undefined strings for invalid byte sequences whereas the one used in the patch will replace unparseable portions with valid 'unknown' characters.

One question I cannot still answer yet is how the ManifestDigester can be changed at all without complete test coverage or risking to break existing signatures. I already started on that but it's quite a piece of work to almost formally prove that all manifests will continue to produce identical digests, except of course for the ones now fixed.

Regards,
Philipp


On 17.09.2017 21:25, Philipp Kunz wrote:

Hello Vincent

I narrowed the error down so far and suspect now that it is an effect of sun.security.util.ManifestDigester's constructor, lines 134 and 163-164, in combination with java.util.jar.Manifest.make72Safe. Manifest breaks multi-byte characters in manifest section names across lines and ManifestDigester fails to restore them correctly, which both sounds wrong but ManifestDigester sure is.

Just fixing it would be too tempting but then I would not know (I mean not know for sure with evidence from tests) if any existing jar-signature would still be valid and I don't want to break existing signatures. When I had a look at the tests specific for Manifest and ManifestDigester I found very little. There are many more different situations and corner cases and combinations thereof to cover with tests than it looks like at first glance so that writing tests that cover all relevant cases looks to me like quite an effort. I have the impression that test coverage was not a priority when the subjected code was developed or at least the tests might not have been contributed to the open JDK project. Hence, while I'm continuing to complete these tests I miss, if       someone has an idea how to simplify that so that I can still convince at least myself that no existing signature will break or where possibly more testcases are that I haven't discovered so far, please let me know.

I'm also wondering how much performance should be taken into consideration. There are a few hints such as reusing byte arrays that suggest that it is an issue. Will a patch be rejected if it slows down some tests beyond a certain limit for so little added value or what might be the acceptance criteria here? I don't expect insuperable difficulties with performance but would still appreciate some general idea.

My desired or currently preferred approach to fix JDK-6695402 would be to let ManifestDigester any kind of use or extend Manifest in order to let Manifest identify the manifest sections thereby preventing the parsing duplicated that identifies the manifest sections.

As a new contributor I could use and would appreciate some guidance particularly about what kind and completeness of test coverage and performance tuning applies or is suggested in the context of the given bug. Otherwise I fear I might contribute too many tests which would have to be reviewed and maintained or quite some work would be for nothing if a patch would not satisfy performance requirements.

Regards,
Philipp



On 01.09.2017 15:20, Vincent Ryan wrote:

That all sounds fine. Let me know when your patch is ready to submit.
Thanks.



On 1 Sep 2017, at 13:15, Philipp Kunz [hidden email]
wrote:

Hello Vincent

Thank you for sponsoring!
So far, I have become a contributor by signing the OCA which has been accepted. Dalibor Topic wrote that he can confirm and it's also here: 
http://www.oracle.com/technetwork/community/oca-486395.html#p
-> Paratix GmbH
Therefore I think I have followed the steps in 
http://openjdk.java.net/contribute/
at least the ones before actual patch submission.

Currently, I'm working on getting the existing test cases running locally. Unfortunately, I started with jdk 9 and switched to 10 now. I figured these commands run at least the relevant tests with 9 and hope this also applies to 10:
make run-test-tier1
make run-test TEST="jdk/test"
they report errors and failures but it hasn't been completed and released which might explain it. If I run the tests I assume relevant for my patch
make run-test TEST="jtreg:jdk/test/sun/security/tools/jarsigner jtreg:jdk/test/java/util/jar"
then it reports zero errors and failures which may be a good starting point.
Do you think that sounds reasonable or do you have another suggestion how to run the tests?

Next, I will add a test for JDK-6695402 before actually fixing it. As an example, I'll try something in the style of 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/65464a307408/test/java/util/jar/Manifest/CreateManifest.java
. This way, I try to demonstrate the improvement.

I guess I have identified the following line as the cause: value = new String(vb, 0, 0, vb.length);

http://hg.openjdk.java.net/jdk9/jdk9/jdk/file/51f5d60713b5/src/java.base/share/classes/java/util/jar/Manifest.java#l157

So I'll try to remove it first including the whole four line if block.

Philipp


On 01.09.2017 10:00, Vincent Ryan wrote:

Hello Philipp,

I’m happy to sponsor your fix for JDK 10. Have you followed these steps: 
http://openjdk.java.net/contribute/
?

Thanks.



On 1 Sep 2017, at 08:58, Vincent Ryan [hidden email]
wrote:

Moved to security-dev



On 1 Sep 2017, at 08:28, Philipp Kunz [hidden email]
wrote:

Hello everyone

I have been developing with Java for around 17 years now and when I encountered some bug I decided to attempt to fix it: 
https://bugs.openjdk.java.net/browse/JDK-6695402
. This also looks like it may not be too big a piece for a first contribution.

I read through quite some guides and all kinds of documents but could not yet help myself with the following questions:

May I login to jira to add comments to bugs? If so, how would I request or receive credentials? Or are mailing lists preferred?

Another question is whether I should apply it to jdk9, but it may be too late now, or to jdk10, and backporting can be considered later. Probably it wouldn't even make much a difference for the patch itself.

One more question I have is how or where to find the sources from before migration to mercurial. Because some lines of code I intend to change go back farther and in the history I find only 'initial commit'. With such a history I might be able better to understand why it's there and prevent to make the same mistake again.

I guess the appropriate mailing list for above mentioned bug is security-dev. Is it correct that I can send a patch there and just hope for some sponsor to pick it up? Of course I'd be glad if some sponsor would contact me and maybe provide some assistance or if someone would confirm that sending a patch to the mailing list is the right way to find a sponsor.

Philipp Kunz



<Mail Attachment.png>

Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35

[hidden email]
-- 


Gruss Philipp




<Mail Attachment.png>

Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]

      

    

--


Gruss Philipp







Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang

> On Sep 26, 2017, at 1:11 AM, Philipp Kunz <[hidden email]> wrote:
>
> Hi Max
>
> Thank you for the detailed assistance and I really hope it doesn't annoy you too much with so many beginner's mistakes. Every little point of yours seems to be absolutely justified in my point of view.

I'm flattered.

>
> I did not understand where I could apply the readAllBytes. In case it still applies, would you give me another hint? I'd rather expect some potential for butifying verifyClassNameLineBroken but then I haven't found any really.

I meant you can change "while (inputStream.read() > -1);" to "inputStream.readAllBytes()". Yours is fine. Some people do not like the return value wasted and the auto-increment mechanism in the method is unnecessary here.

>
> About where to place the declaration of nameBuf I was a little confused probably/obviously when there was a ByteArrayOutputStream before which, however, actually was not used into assuming that reusing the same buffer for all sections would result in better performance or so but that's certainly not a valid assumption now as I consider it again.
>
> About the import static you convinced me but for statically importing java.nio.charset.StandardCharsets.UTF_8 which I prefer this way still. After you wrote I should decide myself, I removed other static imports. If there was kind of an unwritten rule not to use static imports generally that would be a compelling reason but I didn't bother to search for import static through the jdk sources.

I don't know if there is a rule. Yours is OK. I said it was just my habit.

>
> The @modules was a left-over from previous versions.
>
> I'm not sure where you meant I should put the test to exactly. In http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/test/jdk I cant find the parent folder sun you mentioned. In case there is some reorganization in progress not having arrived yet at the tip, could the new test be moved together with it? Or did you mean I should create the sun folder but then the existing tests would not be next to the new one? Did I look in the wrong place?

Oh sorry, we've just recently change it to http://hg.openjdk.java.net/jdk10/master/. You don't really need to rework this time, but if you want to continue hacking on OpenJDK, this is the new repo.

>
> When updating the copyright years I was not sure if I should let the existing one, 2011, there in ManifestDigester or replace it with the current year which I did. When looking into other class files I saw none with more than two years but "," looks a little odd to me for indicating a range.

It is a little uncommon, but that does mean a range.

>
> I assumed that I remove the JDK- prefix from the @bug tag and not the @test tag unless @bug is considered part of the @test.

Correct. @test must be there otherwise jtreg will not treat it as a test. @bug might be used by some reporting tools.

>
> Is it correct, that a maximum line width of 80 characters is the convention? In case I added some more breaks for that. I just wonder what other style guides I should have respected.

Yes that is the convention. Sometimes a line can be longer if wrapping it is very ugly. In general, don't make a Sdiffs page in a webrev non-readable. A Sdiffs page example is here

  http://cr.openjdk.java.net/~bpb/8186766/webrev.00/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c.sdiff.html

>
> Just in case you think it would be more efficient I'd not object that a reviewer or some else would just change these little things to get over with it. On the other hand this way I learn it. If there is a suitable documentation that much detailed, I'd be glad if you could point me at it. That might save a few cycles.

I don't know if there is a more suitable doc.

I have some small suggestions:

1. In fact I don't know exactly if a non-ASCII is allowed in source code now. For safety, please change the é char to \u1234 style.

2. Maybe it's better to capitalize "b" in the test name?

3. The @see tag has its format, you can just use "See...". The method name in @link has a wrong signature.

   * @see also eAcute in
   * {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}

4. Several "throws Exception" should be indented by 8 chars.

5. There are several trailing spaces in your patch. Maybe because of copy-and-paste.

Please send me the new patch as an attachment. That will be more precise and more formal. Please send me what the exact "name <email>" you want to see in the Contributed-by field of the changeset.

Thanks
Max

>
> Regards,
> Philipp

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Philipp Kunz
Hi Max

This time I got it with readAllBytes. Thank you for the hint.

Apparently, UTF characters are allowed in source code, particularly in identifiers here, which also has caused the bug. Even if only for sending patches around I changed it and was surprised to see escaping working not only in strings but also in identifiers.

When I had another look at the test I came to the conclusion that it does not need what has been named refClassFileName before. The purpose of the test is only to check a signature of a class with a two byte character in its name and not at the same time to verify that if that test failed it is specifically because of the name. If it fails there is a problem no matter why. In the beginning it was handy to see the difference but I don't think it should be kept and maintained so I removed it. For the update signature case a second file to sign is still required though.

I considered multi-byte a one word before but now I also prefer it with a capital b. Anyway, this name might not be the best choice and I changed it to LineBrokenMultiByteCharacter.

See attached patch.

Regards,
Philipp


On 26.09.2017 06:19, Weijun Wang wrote:

      
On Sep 26, 2017, at 1:11 AM, Philipp Kunz [hidden email] wrote:

Hi Max

Thank you for the detailed assistance and I really hope it doesn't annoy you too much with so many beginner's mistakes. Every little point of yours seems to be absolutely justified in my point of view.
I'm flattered.

I did not understand where I could apply the readAllBytes. In case it still applies, would you give me another hint? I'd rather expect some potential for butifying verifyClassNameLineBroken but then I haven't found any really.
I meant you can change "while (inputStream.read() > -1);" to "inputStream.readAllBytes()". Yours is fine. Some people do not like the return value wasted and the auto-increment mechanism in the method is unnecessary here. 

About where to place the declaration of nameBuf I was a little confused probably/obviously when there was a ByteArrayOutputStream before which, however, actually was not used into assuming that reusing the same buffer for all sections would result in better performance or so but that's certainly not a valid assumption now as I consider it again.

About the import static you convinced me but for statically importing java.nio.charset.StandardCharsets.UTF_8 which I prefer this way still. After you wrote I should decide myself, I removed other static imports. If there was kind of an unwritten rule not to use static imports generally that would be a compelling reason but I didn't bother to search for import static through the jdk sources.
I don't know if there is a rule. Yours is OK. I said it was just my habit.

The @modules was a left-over from previous versions.

I'm not sure where you meant I should put the test to exactly. In http://hg.openjdk.java.net/jdk10/jdk10/jdk/file/777356696811/test/jdk I cant find the parent folder sun you mentioned. In case there is some reorganization in progress not having arrived yet at the tip, could the new test be moved together with it? Or did you mean I should create the sun folder but then the existing tests would not be next to the new one? Did I look in the wrong place?
Oh sorry, we've just recently change it to http://hg.openjdk.java.net/jdk10/master/. You don't really need to rework this time, but if you want to continue hacking on OpenJDK, this is the new repo.

When updating the copyright years I was not sure if I should let the existing one, 2011, there in ManifestDigester or replace it with the current year which I did. When looking into other class files I saw none with more than two years but "," looks a little odd to me for indicating a range.
It is a little uncommon, but that does mean a range.

I assumed that I remove the JDK- prefix from the @bug tag and not the @test tag unless @bug is considered part of the @test.
Correct. @test must be there otherwise jtreg will not treat it as a test. @bug might be used by some reporting tools.

Is it correct, that a maximum line width of 80 characters is the convention? In case I added some more breaks for that. I just wonder what other style guides I should have respected.
Yes that is the convention. Sometimes a line can be longer if wrapping it is very ugly. In general, don't make a Sdiffs page in a webrev non-readable. A Sdiffs page example is here

  http://cr.openjdk.java.net/~bpb/8186766/webrev.00/src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c.sdiff.html

Just in case you think it would be more efficient I'd not object that a reviewer or some else would just change these little things to get over with it. On the other hand this way I learn it. If there is a suitable documentation that much detailed, I'd be glad if you could point me at it. That might save a few cycles.
I don't know if there is a more suitable doc.

I have some small suggestions:

1. In fact I don't know exactly if a non-ASCII is allowed in source code now. For safety, please change the é char to \u1234 style.

2. Maybe it's better to capitalize "b" in the test name?

3. The @see tag has its format, you can just use "See...". The method name in @link has a wrong signature.

   * @see also eAcute in
   * {@link MultibyteUnicodeName#verifyClassNameLineBroken(String)}

4. Several "throws Exception" should be indented by 8 chars.

5. There are several trailing spaces in your patch. Maybe because of copy-and-paste.

Please send me the new patch as an attachment. That will be more precise and more formal. Please send me what the exact "name <email>" you want to see in the Contributed-by field of the changeset.

Thanks
Max

Regards,
Philipp

    

--


Gruss Philipp







Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]

JDK-6695402.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang

> On Sep 26, 2017, at 1:37 PM, Philipp Kunz <[hidden email]> wrote:
>
> Hi Max
>
> This time I got it with readAllBytes. Thank you for the hint.
>
> Apparently, UTF characters are allowed in source code, particularly in identifiers here, which also has caused the bug. Even if only for sending patches around I changed it and was surprised to see escaping working not only in strings but also in identifiers.

See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2

I've submitted your change to our testing server. Once it's OK, I'll push the changeset.

I assume "Contributed-by: Philipp Kunz <[hidden email]>" is good.

BTW, there are several TAB chars and trailing spaces in your patch. I've removed them.

Thanks for your contribution.

--Max

>
> When I had another look at the test I came to the conclusion that it does not need what has been named refClassFileName before. The purpose of the test is only to check a signature of a class with a     two byte character in its name and not at the same time to verify that if that test failed it is specifically because of the name. If it fails there is a problem no matter why. In the beginning it was handy to see the difference but I don't think it should be kept and maintained so I removed it. For the update signature case a second file to sign is still required though.
>
> I considered multi-byte a one word before but now I also prefer it with a capital b. Anyway, this name might not be the best choice and I changed it to LineBrokenMultiByteCharacter.
>
> See attached patch.
>
> Regards,
> Philipp
>

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang
Oops, the new test fails on Linux and Solaris.

/scratch/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java:54: error: error while writing A1234567890B1234567890C123456789D1?xyz: bad filename RelativeFile[LineBrokenMultiByteCharacter$A1234567890B1234567890C123456789D1?xyz.class]
    static class A1234567890B1234567890C123456789D1\u00E9xyz { }
           ^
1 error

I'll ask the compiler team.

--Max

> On Sep 26, 2017, at 3:51 PM, Weijun Wang <[hidden email]> wrote:
>
>
>> On Sep 26, 2017, at 1:37 PM, Philipp Kunz <[hidden email]> wrote:
>>
>> Hi Max
>>
>> This time I got it with readAllBytes. Thank you for the hint.
>>
>> Apparently, UTF characters are allowed in source code, particularly in identifiers here, which also has caused the bug. Even if only for sending patches around I changed it and was surprised to see escaping working not only in strings but also in identifiers.
>
> See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2
>
> I've submitted your change to our testing server. Once it's OK, I'll push the changeset.
>
> I assume "Contributed-by: Philipp Kunz <[hidden email]>" is good.
>
> BTW, there are several TAB chars and trailing spaces in your patch. I've removed them.
>
> Thanks for your contribution.
>
> --Max
>
>>
>> When I had another look at the test I came to the conclusion that it does not need what has been named refClassFileName before. The purpose of the test is only to check a signature of a class with a     two byte character in its name and not at the same time to verify that if that test failed it is specifically because of the name. If it fails there is a problem no matter why. In the beginning it was handy to see the difference but I don't think it should be kept and maintained so I removed it. For the update signature case a second file to sign is still required though.
>>
>> I considered multi-byte a one word before but now I also prefer it with a capital b. Anyway, this name might not be the best choice and I changed it to LineBrokenMultiByteCharacter.
>>
>> See attached patch.
>>
>> Regards,
>> Philipp
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang
It might be a jtreg issue, but I'll have to get it resolved before pushing your changeset.

--Max

> On Sep 26, 2017, at 7:30 PM, Weijun Wang <[hidden email]> wrote:
>
> Oops, the new test fails on Linux and Solaris.
>
> /scratch/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java:54: error: error while writing A1234567890B1234567890C123456789D1?xyz: bad filename RelativeFile[LineBrokenMultiByteCharacter$A1234567890B1234567890C123456789D1?xyz.class]
>    static class A1234567890B1234567890C123456789D1\u00E9xyz { }
>           ^
> 1 error
>
> I'll ask the compiler team.
>
> --Max
>
>> On Sep 26, 2017, at 3:51 PM, Weijun Wang <[hidden email]> wrote:
>>
>>
>>> On Sep 26, 2017, at 1:37 PM, Philipp Kunz <[hidden email]> wrote:
>>>
>>> Hi Max
>>>
>>> This time I got it with readAllBytes. Thank you for the hint.
>>>
>>> Apparently, UTF characters are allowed in source code, particularly in identifiers here, which also has caused the bug. Even if only for sending patches around I changed it and was surprised to see escaping working not only in strings but also in identifiers.
>>
>> See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2
>>
>> I've submitted your change to our testing server. Once it's OK, I'll push the changeset.
>>
>> I assume "Contributed-by: Philipp Kunz <[hidden email]>" is good.
>>
>> BTW, there are several TAB chars and trailing spaces in your patch. I've removed them.
>>
>> Thanks for your contribution.
>>
>> --Max
>>
>>>
>>> When I had another look at the test I came to the conclusion that it does not need what has been named refClassFileName before. The purpose of the test is only to check a signature of a class with a     two byte character in its name and not at the same time to verify that if that test failed it is specifically because of the name. If it fails there is a problem no matter why. In the beginning it was handy to see the difference but I don't think it should be kept and maintained so I removed it. For the update signature case a second file to sign is still required though.
>>>
>>> I considered multi-byte a one word before but now I also prefer it with a capital b. Anyway, this name might not be the best choice and I changed it to LineBrokenMultiByteCharacter.
>>>
>>> See attached patch.
>>>
>>> Regards,
>>> Philipp
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang
Hi Philipp

The problem is that when launching by jtreg javac has difficulties writing the class file with the é char into the file system on several OSes.

I've updated the test a little. Now they are not written to files. Fortunately JarUtils can add non-existing entries. The test now passes on all our testing platforms.

   http://cr.openjdk.java.net/~weijun/6695402/webrev.01

If you are OK with the new version I'll push them.

Thanks
Max

> On Sep 26, 2017, at 10:54 PM, Weijun Wang <[hidden email]> wrote:
>
> It might be a jtreg issue, but I'll have to get it resolved before pushing your changeset.
>
> --Max
>
>> On Sep 26, 2017, at 7:30 PM, Weijun Wang <[hidden email]> wrote:
>>
>> Oops, the new test fails on Linux and Solaris.
>>
>> /scratch/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java:54: error: error while writing A1234567890B1234567890C123456789D1?xyz: bad filename RelativeFile[LineBrokenMultiByteCharacter$A1234567890B1234567890C123456789D1?xyz.class]
>>   static class A1234567890B1234567890C123456789D1\u00E9xyz { }
>>          ^
>> 1 error
>>
>> I'll ask the compiler team.
>>
>> --Max
>>
>>> On Sep 26, 2017, at 3:51 PM, Weijun Wang <[hidden email]> wrote:
>>>
>>>
>>>> On Sep 26, 2017, at 1:37 PM, Philipp Kunz <[hidden email]> wrote:
>>>>
>>>> Hi Max
>>>>
>>>> This time I got it with readAllBytes. Thank you for the hint.
>>>>
>>>> Apparently, UTF characters are allowed in source code, particularly in identifiers here, which also has caused the bug. Even if only for sending patches around I changed it and was surprised to see escaping working not only in strings but also in identifiers.
>>>
>>> See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2
>>>
>>> I've submitted your change to our testing server. Once it's OK, I'll push the changeset.
>>>
>>> I assume "Contributed-by: Philipp Kunz <[hidden email]>" is good.
>>>
>>> BTW, there are several TAB chars and trailing spaces in your patch. I've removed them.
>>>
>>> Thanks for your contribution.
>>>
>>> --Max
>>>
>>>>
>>>> When I had another look at the test I came to the conclusion that it does not need what has been named refClassFileName before. The purpose of the test is only to check a signature of a class with a     two byte character in its name and not at the same time to verify that if that test failed it is specifically because of the name. If it fails there is a problem no matter why. In the beginning it was handy to see the difference but I don't think it should be kept and maintained so I removed it. For the update signature case a second file to sign is still required though.
>>>>
>>>> I considered multi-byte a one word before but now I also prefer it with a capital b. Anyway, this name might not be the best choice and I changed it to LineBrokenMultiByteCharacter.
>>>>
>>>> See attached patch.
>>>>
>>>> Regards,
>>>> Philipp
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Philipp Kunz
Hi Max

Thank you for your update and its associated effort.

I suggest that at least a comment should be added about why and how non-existing files can be added and the test still serves it's purpose. In fact I was quite a bit surprised when I found out that JarUtils.createJar adds the file name as contents if the file cannot be found. Ideally, we would also add some note about why this was relevant, about the test not compiling on certain oses with jtreg together with the acute but it doesn't apply any longer now. Altogether the test has become even simpler now.

One more small thing that I just noticed now is that I would prefer ManifestFileName not to start with a capital letter like the other nearby variables. I thought testName was too ambiguous a name and changed it to testClassName. I also reviewed and slightly changed a few comments again. Of course it will never become perfect but now it should do. See attached patch.

The contributed by is fine. I'd be glad to share credits with you and please accept more flattering for your collaboration.

Regards,
Philipp



On 27.09.2017 03:20, Weijun Wang wrote:
Hi Philipp

The problem is that when launching by jtreg javac has difficulties writing the class file with the é char into the file system on several OSes.

I've updated the test a little. Now they are not written to files. Fortunately JarUtils can add non-existing entries. The test now passes on all our testing platforms.

   http://cr.openjdk.java.net/~weijun/6695402/webrev.01

If you are OK with the new version I'll push them.

Thanks
Max

On Sep 26, 2017, at 10:54 PM, Weijun Wang [hidden email] wrote:

It might be a jtreg issue, but I'll have to get it resolved before pushing your changeset.

--Max

On Sep 26, 2017, at 7:30 PM, Weijun Wang [hidden email] wrote:

Oops, the new test fails on Linux and Solaris.

/scratch/test/jdk/sun/security/tools/jarsigner/LineBrokenMultiByteCharacter.java:54: error: error while writing A1234567890B1234567890C123456789D1?xyz: bad filename RelativeFile[LineBrokenMultiByteCharacter$A1234567890B1234567890C123456789D1?xyz.class]
  static class A1234567890B1234567890C123456789D1\u00E9xyz { }
         ^
1 error

I'll ask the compiler team.

--Max

On Sep 26, 2017, at 3:51 PM, Weijun Wang [hidden email] wrote:


On Sep 26, 2017, at 1:37 PM, Philipp Kunz [hidden email] wrote:

Hi Max

This time I got it with readAllBytes. Thank you for the hint.

Apparently, UTF characters are allowed in source code, particularly in identifiers here, which also has caused the bug. Even if only for sending patches around I changed it and was surprised to see escaping working not only in strings but also in identifiers.
See https://docs.oracle.com/javase/specs/jls/se8/html/jls-3.html#jls-3.2

I've submitted your change to our testing server. Once it's OK, I'll push the changeset.

I assume "Contributed-by: Philipp Kunz [hidden email]" is good.

BTW, there are several TAB chars and trailing spaces in your patch. I've removed them.

Thanks for your contribution.

--Max

When I had another look at the test I came to the conclusion that it does not need what has been named refClassFileName before. The purpose of the test is only to check a signature of a class with a     two byte character in its name and not at the same time to verify that if that test failed it is specifically because of the name. If it fails there is a problem no matter why. In the beginning it was handy to see the difference but I don't think it should be kept and maintained so I removed it. For the update signature case a second file to sign is still required though.

I considered multi-byte a one word before but now I also prefer it with a capital b. Anyway, this name might not be the best choice and I changed it to LineBrokenMultiByteCharacter.

See attached patch.

Regards,
Philipp


          

        

      

    

--


Gruss Philipp







Paratix GmbH
St Peterhofstatt 11
8001 Zürich

+41 (0)76 397 79 35
[hidden email]

JDK-6695402.patch (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Patch for JDK-6695402: Jarsigner with multi-byte characters in class names

Weijun Wang
Hi Philipp

The change is now at http://hg.openjdk.java.net/jdk10/master/rev/f60a42d4b8cd.

I only moved the test to its new directory. Everything else unchanged.

Thanks again for your contribution, looking forward for more!

--Max

> On Sep 27, 2017, at 1:49 PM, Philipp Kunz <[hidden email]> wrote:
>
> Hi Max
>
> Thank you for your update and its associated effort.
>
> I suggest that at least a comment should be added about why and how non-existing files can be added and the test still serves it's purpose. In fact I was quite a bit surprised when I found out that JarUtils.createJar adds the file name as contents if the file cannot be found. Ideally, we would also add some note about why this was relevant, about the test not compiling on certain oses with jtreg together with the acute but it doesn't apply any longer now. Altogether the test has become even simpler now.
>
> One more small thing that I just noticed now is that I would prefer ManifestFileName not to start with a capital letter like the other nearby variables. I thought testName was too ambiguous a name and changed it to testClassName. I also reviewed and slightly changed a few comments again. Of course it will never become perfect but now it should do. See attached patch.
>
> The contributed by is fine. I'd be glad to share credits with you and please accept more flattering for your collaboration.
>
> Regards,
> Philipp
>