From 094525796113e8a38dde003e39dae1419d7f248a Mon Sep 17 00:00:00 2001
From: Stefan Bodewig <bodewig@apache.org>
Date: Tue, 20 Mar 2018 12:13:57 +0100
Subject: [PATCH] verifyjar must use -storepass or jarsigner will not work

https://bz.apache.org/bugzilla/show_bug.cgi?id=62194
---
 WHATSNEW                                      |  9 ++++
 manual/Tasks/signjar.html                     |  4 +-
 manual/Tasks/verifyjar.html                   |  8 +++-
 .../apache/tools/ant/taskdefs/VerifyJar.java  | 46 +++++++++++++++++++
 src/tests/antunit/taskdefs/signjar-test.xml   |  9 ++++
 5 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/WHATSNEW b/WHATSNEW
index 6dd00fe29..1011201f8 100644
--- a/WHATSNEW
+++ b/WHATSNEW
@@ -16,6 +16,15 @@ Fixed bugs:
  * Fixed NullPointerException when a mappedresource is used in pathconvert
    Bugzilla Report 62076
 
+ * Added a workaround for a bug in the jarsigner tool to <verifyjar>
+   which requires the -storepass command line argument when verifying
+   signatures using -strict together with a PKCS12 keystore. Unlike
+   when signing the jar it will not prompt for the keystore's password
+   and read it from standard input.
+   This means Ant will now pass the keystore's password on the command
+   line when using <verifyjar>, which poses a security risk you should
+   be aware of.
+   Bugzilla Report 62194
 
 Other changes:
 --------------
diff --git a/manual/Tasks/signjar.html b/manual/Tasks/signjar.html
index 0f9d77846..32315c158 100644
--- a/manual/Tasks/signjar.html
+++ b/manual/Tasks/signjar.html
@@ -66,7 +66,9 @@ and <tt>lazy</tt> is false, the JAR is signed.</li>
   </tr>
   <tr>
     <td valign="top">storepass</td>
-    <td valign="top">password for keystore integrity.</td>
+    <td valign="top">password for keystore integrity. Ant will not use
+    the <code>-storepass</code> command line argument but send the
+    password to jarsigner when it prompts for it.</td>
     <td valign="top" align="center">Yes.</td>
   </tr>
   <tr>
diff --git a/manual/Tasks/verifyjar.html b/manual/Tasks/verifyjar.html
index 4be278811..886075a79 100644
--- a/manual/Tasks/verifyjar.html
+++ b/manual/Tasks/verifyjar.html
@@ -52,8 +52,12 @@ supported
   </tr>
   <tr>
     <td valign="top">storepass</td>
-    <td valign="top">password for keystore integrity.</td>
-    <td valign="top" align="center">Yes.</td>
+    <td valign="top">password for keystore integrity.
+    Note that
+    jarsigner does not read the password from stdin during
+    verification, so the password must be send via a command line
+    interface and may be visible to other users of the system.</td>
+    <td valign="top" align="center">No.</td>
   </tr>
   <tr>
     <td valign="top">keystore</td>
diff --git a/src/main/org/apache/tools/ant/taskdefs/VerifyJar.java b/src/main/org/apache/tools/ant/taskdefs/VerifyJar.java
index a0003c771..4cbbe2a31 100644
--- a/src/main/org/apache/tools/ant/taskdefs/VerifyJar.java
+++ b/src/main/org/apache/tools/ant/taskdefs/VerifyJar.java
@@ -58,6 +58,8 @@ public class VerifyJar extends AbstractJarSignerTask {
     /** Error output if there is a failure to verify the jar. */
     public static final String ERROR_NO_VERIFY = "Failed to verify ";
 
+    private String savedStorePass = null;
+
     /**
      * Ask for certificate information to be printed
      * @param certificates if true print certificates.
@@ -99,6 +101,42 @@ public class VerifyJar extends AbstractJarSignerTask {
 
     }
 
+    /**
+     * @since 1.9.11
+     */
+    @Override
+    protected void beginExecution() {
+        // when using a PKCS12 keystore jarsigner -verify will not
+        // prompt for the keystore password but will only properly
+        // verify the jar with -strict enabled if the -storepass
+        // parameter is used. Note that the documentation of jarsigner
+        // says -storepass was never required with -verify - this is
+        // wrong.
+        //
+        // See https://bz.apache.org/bugzilla/show_bug.cgi?id=62194
+        //
+        // So if strict is true then we hide storepass from the base
+        // implementation and instead add the -storepass command line
+        // argument
+        if (mustHideStorePass()) {
+            savedStorePass = storepass;
+            setStorepass(null);
+        }
+        super.beginExecution();
+    }
+
+    /**
+     * @since 1.9.11
+     */
+    @Override
+    protected void endExecution() {
+        if (savedStorePass != null) {
+            setStorepass(savedStorePass);
+            savedStorePass = null;
+        }
+        super.endExecution();
+    }
+
     /**
      * verify a JAR.
      * @param jar the jar to verify.
@@ -112,6 +150,10 @@ public class VerifyJar extends AbstractJarSignerTask {
 
         setCommonOptions(cmd);
         bindToKeystore(cmd);
+        if (savedStorePass != null) {
+            addValue(cmd, "-storepass");
+            addValue(cmd, savedStorePass);
+        }
 
         //verify special operations
         addValue(cmd, "-verify");
@@ -151,6 +193,10 @@ public class VerifyJar extends AbstractJarSignerTask {
         }
     }
 
+    private boolean mustHideStorePass() {
+        return strict && storepass != null;
+    }
+
     /**
      * we are not thread safe here. Do not use on multiple threads at the same time.
      */
diff --git a/src/tests/antunit/taskdefs/signjar-test.xml b/src/tests/antunit/taskdefs/signjar-test.xml
index 4d998fb69..30671cfb2 100644
--- a/src/tests/antunit/taskdefs/signjar-test.xml
+++ b/src/tests/antunit/taskdefs/signjar-test.xml
@@ -282,5 +282,14 @@
     </au:expectfailure>
   </target>
 
+  <target name="testVerifyJarStrict" depends="basic">
+    <verify-base jar="${signtest.jar}" strict="true"/>
+  </target>
+
+  <target name="testVerifyJarStrictPKCS12" depends="basic-pkcs12"
+          description="https://bz.apache.org/bugzilla/show_bug.cgi?id=62194">
+    <verify-base-pkcs12 jar="${signtest.jar}" strict="true"/>
+  </target>
+
 </project>