Hi Max and everyone,
With respect to the previous patch, parentheses moved from storeHash to
printCert, bug number added, and some comments added and clarified.
Regards,Philipp
On Tue, 2019-04-30 at 14:45 +0800, Weijun Wang wrote:
> > On Apr 30, 2019, at 1:56 PM, Philipp Kunz <[email protected]>
> > wrote:
> >
> > Hi Max,
> >
> > I agree that the parentheses are superfluous, however, I would
> > slightly prefer going small steps.
> >
> > Would the parentheses have to come from a resource bundle as well
> > like SPACE or COMMA when removed from storeHash values?
>
> No. It's hardcoded and not localized in printCert.
>
> > Now that with the patch, storeHash is not any longer useful to
> > check whether it contains ckaliases in inKeyStoreForOneSigner (but
> > currently still useful to save another call to
> > store.getCertificateAlias in printCert lateron), would it be a
> > worth a consideration to rearrange the code populating storeHash
> > closer to printCert in some way or even just completely remove
> > storeHash and replace it with store.getCertificateAlias in
> > printCert or anything similar?
>
> Probably not. I believe storeHash was introduced to avoid too many
> calls to store.getCertificateAlias when there are hundreds of classes
> in a signed jar.
>
> --Max
>
> > Regards,
> > Philipp
> >
> >
> >
> >
> >
> > On Sat, 2019-03-30 at 23:01 +0800, Weijun Wang wrote:
> > > Hi Philipp,
> > >
> > > Sorry I'm so late giving a response. I've filed
> > >
> > >
> > > https://bugs.openjdk.java.net/browse/JDK-8221719
> > >
> > > Jarsigner Fails To Verify Signed By Alias If Alias Given In
> > > Wrong Case
> > >
> > > The fix is good. Since we don't support identitydb anymore I
> > > think we can remove the parenthesis around the alias in the
> > > storeHash now.
> > >
> > > Thanks,
> > > Max
> > >
> > >
> > > > On Feb 20, 2019, at 8:41 PM, Philipp Kunz <
> > > > [email protected]
> > > > > wrote:
> > > >
> > > > Hi,
> > > >
> > > > When signing a jarfile the specified alias is converted by
> > > > JavaKeyStore (storetype JKS) to lower case.
> > > > When verifying the jarfile with the same alias, it is currently
> > > > not converted by JKS to lower case and jarsigner reports that
> > > > the jar is not signed by the alias even after having just
> > > > signed it with that same alias if not all characters are in
> > > > lower case.
> > > >
> > > > I found no statement in the documentation that an alias would
> > > > support only lower case or which characters could be used. It
> > > > could, however, be worth noting in the documentation of
> > > > JavaKeyStore or the keytool that the alias can be changed by
> > > > the keystore in certain circumstances.
> > > > In my opinion, it feels like a bug that aliases are treated
> > > > differently when signing and verifying with respect to their
> > > > upper and lower case.
> > > >
> > > > Find a patch attached. Some explanations, just in case not too
> > > > obvious anyway:
> > > >
> > > > - The jarsigner never changed the upper and lower cases of
> > > > aliases itself and neither does with the patch. That is now
> > > > delegated to the keystore implementation not only for signing
> > > > but in addition also for verifying. The fix is therefore not
> > > > JSK-specific.
> > > > - I assume it is correct that the if (store != null) check and
> > > > the try catch KeyStoreException can both be moved outside the
> > > > for loop over the certificates because before as well as now
> > > > with the patch the result was alway 0 if store was null and
> > > > KeyStoreException can happen only when loading the keystore,
> > > > which is certainly true for JKS.
> > > > - storeHash is used only by printCert and
> > > > inKeyStoreForOneSigner and contains the aliases as values
> > > > enclosed in parentheses "(" and ")" which is how it is printed
> > > > by printCert. It would have probably been easier to add the
> > > > parentheses only later in printCert but then I tried to change
> > > > as little as possible.
> > > > - Two pairs of "result |= " statements were duplicate.
> > > > Therefore there are the two methods in the test which show that
> > > > both actually failed with "ckaliases.contains" being the
> > > > offending piece of code. In the patch I refactored to recombine
> > > > them.
> > > > - I really wonder how "result |= SIGNED_BY_ALIAS;" should be
> > > > set for any certificate c and not only the one an alias in
> > > > ckaliases points at but I guess that is another story if one at
> > > > all and might have come from filling storeHash with all
> > > > certificates and not only the ones in ckaliases or so.
> > > > - At first glance it might look like a mistake that "alias" is
> > > > not used inside the loop over ckaliases but instead of
> > > > comparing "alias" in lower case to ckaliases with unspecified
> > > > case, the certificate c is now compared to the one gotten from
> > > > the keystore handling the alias case in its own way.
> > > >
> > > > Would someone sponsor this fix or can/should it be improved?
> > > >
> > > > Regards,
> > > > Philipp
> > > > <JavaKeyStoreAliasCaseInsensitive.patch>
> > > >
> > >
> > >
> > > diff -r 616618caad5e src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
--- a/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java Tue Apr 30 20:26:16 2019 -0700
+++ b/src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java Wed May 01 16:08:14 2019 +0200
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2019, 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
@@ -1300,7 +1300,7 @@
String alias = storeHash.get(c);
if (alias != null) {
- certStr.append(space).append(alias);
+ certStr.append(space).append("(").append(alias).append(")");
}
if (x509Cert != null) {
@@ -1425,37 +1425,43 @@
}
int result = 0;
- List<? extends Certificate> certs = signer.getSignerCertPath().getCertificates();
- for (Certificate c : certs) {
- String alias = storeHash.get(c);
- if (alias != null) {
- if (alias.startsWith("(")) {
- result |= IN_KEYSTORE;
- }
- if (ckaliases.contains(alias.substring(1, alias.length() - 1))) {
- result |= SIGNED_BY_ALIAS;
- }
- } else {
- if (store != null) {
- try {
+ if (store != null) {
+ try {
+ List<? extends Certificate> certs =
+ signer.getSignerCertPath().getCertificates();
+ for (Certificate c : certs) {
+ String alias = storeHash.get(c);
+ if (alias == null) {
alias = store.getCertificateAlias(c);
- } catch (KeyStoreException kse) {
- // never happens, because keystore has been loaded
+ if (alias != null) {
+ storeHash.put(c, alias);
+ }
}
if (alias != null) {
- storeHash.put(c, "(" + alias + ")");
result |= IN_KEYSTORE;
}
+ for (String ckalias : ckaliases) {
+ if (c.equals(store.getCertificate(ckalias))) {
+ result |= SIGNED_BY_ALIAS;
+ // must continue with next certificate c and cannot
+ // return or break outer loop because has to fill
+ // storeHash for printCert
+ break;
+ }
+ }
}
- if (ckaliases.contains(alias)) {
- result |= SIGNED_BY_ALIAS;
- }
+ } catch (KeyStoreException kse) {
+ // never happens, because keystore has been loaded
}
}
cacheForInKS.put(signer, result);
return result;
}
+ /**
+ * Maps certificates (as keys) to alias names associated in the keystore
+ * {@link #store} (as values).
+ */
Hashtable<Certificate, String> storeHash = new Hashtable<>();
int inKeyStore(CodeSigner[] signers) {
diff -r 616618caad5e test/jdk/sun/security/tools/jarsigner/JavaKeyStoreAliasCaseInsensitive.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/sun/security/tools/jarsigner/JavaKeyStoreAliasCaseInsensitive.java Wed May 01 16:08:14 2019 +0200
@@ -0,0 +1,140 @@
+/*
+ * Copyright (c) 2019, 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.
+ */
+
+import java.nio.file.Files;
+import java.nio.file.Paths;
+import java.util.jar.Manifest;
+import java.util.jar.Attributes.Name;
+import jdk.test.lib.util.JarUtils;
+import jdk.test.lib.SecurityTools;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.*;
+
+/**
+ * @test
+ * @bug 8221719
+ * @library /test/lib
+ * @run testng JavaKeyStoreAliasCaseInsensitive
+ * @summary Checks that jarsigner verifies a signed jar with the same alias as
+ * was specified for signing, particularly regarding upper and lower case and
+ * its conversion to lower case by JKS
+ * ({@link sun.security.provider.JavaKeyStore.JKS#convertAlias(String)}).
+ */
+public class JavaKeyStoreAliasCaseInsensitive {
+
+ /**
+ * Alias for certificates in the keystore with letters in different
+ * (upper and lower) cases.
+ */
+ static final String ALIAS = "AlIaS";
+
+ @Test
+ public void testAliasCase() throws Exception {
+ final String KEYSTORE_OPTIONS = "-storetype JKS -keystore "
+ + "test-alias-case.jks -storepass changeit";
+ SecurityTools.keytool(KEYSTORE_OPTIONS + " -genkeypair"
+ + " -keypass changeit -alias " + ALIAS + " -dname CN=" + ALIAS)
+ .shouldHaveExitValue(0);
+ String jarFilename = "test-alias-case.jar";
+ Manifest mf = new Manifest();
+ mf.getMainAttributes().put(Name.MANIFEST_VERSION, "1.0");
+ JarUtils.createJarFile(Paths.get(jarFilename), mf, Paths.get("."),
+ Files.write(Paths.get("aFile"), new byte[1]));
+
+ SecurityTools.jarsigner(KEYSTORE_OPTIONS + " -verbose -debug " +
+ jarFilename + " " + ALIAS).shouldHaveExitValue(0);
+
+ SecurityTools.jarsigner("-verify -strict " + KEYSTORE_OPTIONS +
+ " -debug -verbose " + jarFilename + " " + ALIAS)
+ .shouldHaveExitValue(0).shouldNotContain(
+ "This jar contains signed entries which are not "
+ + "signed by the specified alias(es).");
+ }
+
+ /**
+ * This test essentially covers compatibility with the previous version of
+ * {@link sun.security.tools.jarsigner.Main#inKeyStoreForOneSigner} in case
+ * a certificate and alias entry is already in
+ * {@link sun.security.tools.jarsigner.Main#storeHash} when
+ * {@link sun.security.tools.jarsigner.Main#inKeyStoreForOneSigner} is
+ * invoked from a previous invocation of it.
+ * It passed with the previous {@code jarsigner} version with a lowercase
+ * alias {@link #ALIAS} and basically covers the duplicated portions of
+ * code in {@link sun.security.tools.jarsigner.Main#inKeyStoreForOneSigner}
+ * near {@code IN_KEYSTORE} and {@code SIGNED_BY_ALIAS} before having
+ * refactored and re-unified them in order to demonstrate identical
+ * behavior.
+ */
+ @Test
+ public void testAliasCaseStoreHash() throws Exception {
+ // Create a keystore with a certificate associated with ALIAS + "2"
+ // signed by another certificate associated with ALIAS + "1".
+ final String KEYSTORE_OPTIONS = "-storetype JKS -keystore"
+ + " test-alias-storeHash-case.jks -storepass changeit";
+ SecurityTools.keytool(KEYSTORE_OPTIONS + " -genkeypair"
+ + " -keypass changeit -alias " + ALIAS + "1 -dname CN=" +
+ ALIAS + "1").shouldHaveExitValue(0);
+ SecurityTools.keytool(KEYSTORE_OPTIONS + " -genkeypair"
+ + " -keypass changeit -alias " + ALIAS + "2 -dname CN="
+ + ALIAS + "2").shouldHaveExitValue(0);
+ String certReq = SecurityTools.keytool(KEYSTORE_OPTIONS +
+ " -certreq -keypass changeit -alias " + ALIAS + "2")
+ .shouldHaveExitValue(0).getStdout();
+ SecurityTools.setResponse(certReq);
+ String cert = SecurityTools.keytool(KEYSTORE_OPTIONS +
+ " -gencert -rfc -keypass changeit -alias " + ALIAS + "1")
+ .shouldHaveExitValue(0).getOutput();
+ SecurityTools.setResponse(cert);
+ SecurityTools.keytool(KEYSTORE_OPTIONS +
+ " -importcert -keypass changeit -alias " + ALIAS + "2")
+ .shouldHaveExitValue(0);
+
+ // Create a jar file signed by ALIAS + "2" and then add another file to
+ // that same jar and sign it by ALIAS + "1", ALIAS + "1" being an alias
+ // for a certificate which is part of the certificate chain of ALIAS +
+ // "2" but not the certificate ALIAS + "2" points to directly.
+ String jarFilename = "test-alias-storeHash-case.jar";
+ Manifest mf = new Manifest();
+ mf.getMainAttributes().put(Name.MANIFEST_VERSION, "1.0");
+ JarUtils.createJarFile(Paths.get(jarFilename), mf, Paths.get("."));
+ SecurityTools.jarsigner(KEYSTORE_OPTIONS + " -verbose -debug " +
+ jarFilename + " " + ALIAS + "2").shouldHaveExitValue(0);
+ JarUtils.updateJarFile(Paths.get(jarFilename), Paths.get("."),
+ Files.write(Paths.get("added-file"), new byte[1]));
+ SecurityTools.jarsigner(KEYSTORE_OPTIONS + " -verbose -debug " +
+ jarFilename + " " + ALIAS + "1").shouldHaveExitValue(0);
+
+ // The later added file "added-file" is signed by the certificate
+ // associated with alias ALIAS + "1" directly while the other jarfile
+ // contents is signed by a certificate associated with alias ALIAS + "2"
+ // which includes the certificate associated with alias ALIAS + "1" in
+ // its certification path.
+ SecurityTools.jarsigner("-verify -strict " + KEYSTORE_OPTIONS +
+ " -debug -verbose " + jarFilename + " " + ALIAS + "1")
+ .shouldHaveExitValue(0).shouldNotContain(
+ "This jar contains signed entries which are not "
+ + "signed by the specified alias(es).");
+ }
+
+}
