On Sep 20, 2017, at 7:41 AM, Philipp Kunz <philipp.k...@paratix.ch> 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 <philipp.k...@paratix.ch> 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 <vincent.x.r...@oracle.com> wrote:
Moved to security-dev
On 1 Sep 2017, at 08:28, Philipp Kunz <philipp.k...@paratix.ch> 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
philipp.k...@paratix.ch
