RH672262, CVE-2011-0025: IcedTea jarfile signature verification bypass

Fixes JAR signature handling so that multiply/partially signed jars
are correctly handled.

7 files changed, 109 insertions, 40 deletions

diff --git a/ChangeLog b/ChangeLog
index 390b52e..20f51eb 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,33 @@
+2011-01-24 Deepak Bhole <[email protected]>
+
+	RH672262, CVE-2011-0025: IcedTea jarfile signature verification bypass
+	* rt/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
+	(initializeResources): Prompt user only if there is a single certificate
+	that signs all jars in the jnlp file, otherwise treat as unsigned.
+	* rt/net/sourceforge/jnlp/security/CertVerifier.java: Rename getCerts to
+	getCertPath and make it return a CertPath.
+	* rt/net/sourceforge/jnlp/security/CertsInfoPane.java: Rename certs
+	variable to certPath and change its type to CertPath.
+	(buildTree): Use new certPath variable.
+	(populateTable): Same.
+	* rt/net/sourceforge/jnlp/security/HttpsCertVerifier.java: Rename getCerts
+	to getCertPath and make it return a CertPath.
+	* rt/net/sourceforge/jnlp/tools/JarSigner.java: Change type for certs
+	variable to be a hashmap that stores certs and the number of entries they
+	have signed.
+	(totalSignableEntries): New variable to track how many signable entries
+	have been encountered.
+	(getCerts): Updated method to return certs from new hashmap.
+	(isFullySignedByASingleCert): New method. Returns if there is a single
+	cert that signs all the entries in the jars specified in the jnlp file.
+	(verifyJars): Move verifiedJars and unverifiedJars out of the for loop so
+	that the data is not lost when the next jar is processed. After verifying
+	each jar, see if there is a single signer, and prompt the user if there is
+	such an untrusted signer.
+	(verifyJar): Increment totalSignableEntries for each signable entry
+	encountered and the count for each cert when it signs an entry. Move
+	checkTrustedCerts() out of the function into verifyJars(). 
+
 2011-01-28  Omair Majid  <[email protected]>
 
 	* Makefile.am: Move ICEDTEA_REV, ICEDTEA_PKG to acinclude.m4. Use
diff --git a/NEWS b/NEWS
index ce1b8f6..b4cc07f 100644
--- a/NEWS
+++ b/NEWS
@@ -17,6 +17,7 @@ New in release 1.0 (2010-XX-XX):
 * Initial release of IcedTea-Web
 * Security updates
   - RH645843, CVE-2010-3860: IcedTea System property information leak via public static
+  - RH672262, CVE-2011-0025: IcedTea jarfile signature verification bypass
 * Plugin
   - PR542: Plugin fails with NPE on http://www.openprocessing.org/visuals/iframe.php?visualID=2615
   - PR552: Support for FreeBSD's pthread implementation
diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
index ebea041..acadde0 100644
--- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
+++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
@@ -430,7 +430,7 @@ public class JNLPClassLoader extends URLClassLoader {
             }
 
             //Case when at least one jar has some signing
-            if (js.anyJarsSigned()) {
+            if (js.anyJarsSigned() && js.isFullySignedByASingleCert()) {
                 signing = true;
 
                 if (!js.allJarsSigned() &&
diff --git a/netx/net/sourceforge/jnlp/security/CertVerifier.java b/netx/net/sourceforge/jnlp/security/CertVerifier.java
index cef69c3..842a865 100644
--- a/netx/net/sourceforge/jnlp/security/CertVerifier.java
+++ b/netx/net/sourceforge/jnlp/security/CertVerifier.java
@@ -76,7 +76,7 @@ public interface CertVerifier {
      * Return a valid certificate path to this certificate(s) being verified
      * @return The CertPath
      */
-    public ArrayList<CertPath> getCerts();
+    public CertPath getCertPath();
 
     /**
      * Returns the application's publisher's certificate.
@@ -89,4 +89,5 @@ public interface CertVerifier {
      * the event that the application is self signed.
      */
     public abstract Certificate getRoot();
+
 }
diff --git a/netx/net/sourceforge/jnlp/security/CertsInfoPane.java b/netx/net/sourceforge/jnlp/security/CertsInfoPane.java
index 4571b4e..ebf8b3f 100644
--- a/netx/net/sourceforge/jnlp/security/CertsInfoPane.java
+++ b/netx/net/sourceforge/jnlp/security/CertsInfoPane.java
@@ -64,7 +64,7 @@ import javax.swing.tree.TreeSelectionModel;
  */
 public class CertsInfoPane extends SecurityDialogPanel {
 
-    private ArrayList<CertPath> certs;
+    private CertPath certPath;
     private JList list;
     protected JTree tree;
     private JTable table;
@@ -84,12 +84,9 @@ public class CertsInfoPane extends SecurityDialogPanel {
      * Builds the JTree out of CertPaths.
      */
     void buildTree() {
-        certs = parent.getJarSigner().getCerts();
-        //for now, we're only going to display the first signer, even though
-        //jars can be signed by multiple people.
-        CertPath firstPath = certs.get(0);
+        certPath = parent.getJarSigner().getCertPath();
         X509Certificate firstCert =
-                        ((X509Certificate) firstPath.getCertificates().get(0));
+                        ((X509Certificate) certPath.getCertificates().get(0));
         String subjectString =
                         SecurityUtil.getCN(firstCert.getSubjectX500Principal().getName());
         String issuerString =
@@ -101,9 +98,9 @@ public class CertsInfoPane extends SecurityDialogPanel {
 
         //not self signed
         if (!firstCert.getSubjectDN().equals(firstCert.getIssuerDN())
-                        && (firstPath.getCertificates().size() > 1)) {
+                        && (certPath.getCertificates().size() > 1)) {
             X509Certificate secondCert =
-                                ((X509Certificate) firstPath.getCertificates().get(1));
+                                ((X509Certificate) certPath.getCertificates().get(1));
             subjectString =
                                 SecurityUtil.getCN(secondCert.getSubjectX500Principal().getName());
             issuerString =
@@ -122,12 +119,12 @@ public class CertsInfoPane extends SecurityDialogPanel {
      * Fills in certsNames, certsData with data from the certificates.
      */
     protected void populateTable() {
-        certNames = new String[certs.get(0).getCertificates().size()];
+        certNames = new String[certPath.getCertificates().size()];
         certsData = new ArrayList<String[][]>();
 
-        for (int i = 0; i < certs.get(0).getCertificates().size(); i++) {
+        for (int i = 0; i < certPath.getCertificates().size(); i++) {
 
-            X509Certificate c = (X509Certificate) certs.get(0).getCertificates().get(i);
+            X509Certificate c = (X509Certificate) certPath.getCertificates().get(i);
             certsData.add(parseCert(c));
             certNames[i] = SecurityUtil.getCN(c.getSubjectX500Principal().getName())
                                 + " (" + SecurityUtil.getCN(c.getIssuerX500Principal().getName()) + ")";
diff --git a/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java b/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java
index 3593291..a7787d2 100644
--- a/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java
+++ b/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java
@@ -83,7 +83,7 @@ public class HttpsCertVerifier implements CertVerifier {
         return isTrusted;
     }
 
-    public ArrayList<CertPath> getCerts() {
+    public CertPath getCertPath() {
 
         ArrayList<X509Certificate> list = new ArrayList<X509Certificate>();
         for (int i = 0; i < chain.length; i++)
@@ -99,7 +99,7 @@ public class HttpsCertVerifier implements CertVerifier {
             // carry on
         }
 
-        return certPaths;
+        return certPaths.get(0);
     }
 
     public ArrayList<String> getDetails() {
diff --git a/netx/net/sourceforge/jnlp/tools/JarSigner.java b/netx/net/sourceforge/jnlp/tools/JarSigner.java
index 06b962f..4e246f7 100644
--- a/netx/net/sourceforge/jnlp/tools/JarSigner.java
+++ b/netx/net/sourceforge/jnlp/tools/JarSigner.java
@@ -96,11 +96,13 @@ public class JarSigner implements CertVerifier {
     private ArrayList<String> unverifiedJars = null;
 
     /** the certificates used for jar verification */
-    private ArrayList<CertPath> certs = null;
+    private HashMap<CertPath, Integer> certs = new HashMap<CertPath, Integer>();
 
     /** details of this signing */
     private ArrayList<String> details = new ArrayList<String>();
 
+    private int totalSignableEntries = 0;
+
     /* (non-Javadoc)
      * @see net.sourceforge.jnlp.tools.CertVerifier2#getAlreadyTrustPublisher()
      */
@@ -149,18 +151,41 @@ public class JarSigner implements CertVerifier {
      * @see net.sourceforge.jnlp.tools.CertVerifier2#getCerts()
      */
     public ArrayList<CertPath> getCerts() {
-        return certs;
+        return new ArrayList<CertPath>(certs.keySet());
+    }
+
+    /**
+     * Returns whether or not all entries have a common signer.
+     *  
+     * It is possible to create jars where only some entries are signed. In 
+     * such cases, we should not prompt the user to accept anything, as the whole 
+     * application must be treated as unsigned. This method should be called by a 
+     * caller before it is about to ask the user to accept a cert and determine 
+     * whether the application is trusted or not.
+     *  
+     * @return Whether or not all entries have a common signer
+     */
+    public boolean isFullySignedByASingleCert() {
+
+        for (CertPath cPath : certs.keySet()) {
+            // If this cert has signed everything, return true
+            if (certs.get(cPath) == totalSignableEntries)
+                return true;
+        }
+
+        // No cert found that signed all entries. Return false.
+        return false;
     }
 
     public void verifyJars(List<JARDesc> jars, ResourceTracker tracker)
             throws Exception {
 
-        certs = new ArrayList<CertPath>();
+        verifiedJars = new ArrayList<String>();
+        unverifiedJars = new ArrayList<String>();
+
         for (int i = 0; i < jars.size(); i++) {
 
             JARDesc jar = jars.get(i);
-            verifiedJars = new ArrayList<String>();
-            unverifiedJars = new ArrayList<String>();
 
             try {
 
@@ -189,6 +214,22 @@ public class JarSigner implements CertVerifier {
                 throw e;
             }
         }
+
+        //we really only want the first certPath
+        for (CertPath cPath : certs.keySet()) {
+
+            if (certs.get(cPath) != totalSignableEntries)
+                continue;
+            else
+                certPath = cPath;
+
+            // check if the certs added above are in the trusted path
+            checkTrustedCerts();
+
+            if (alreadyTrustPublisher || rootInCacerts)
+                break;
+        }
+
     }
 
     public verifyResult verifyJar(String jarName) throws Exception {
@@ -196,10 +237,6 @@ public class JarSigner implements CertVerifier {
         boolean hasUnsignedEntry = false;
         JarFile jarFile = null;
 
-        // certs could be uninitialized if one calls this method directly
-        if (certs == null)
-            certs = new ArrayList<CertPath>();
-
         try {
             jarFile = new JarFile(jarName, true);
             Vector<JarEntry> entriesVec = new Vector<JarEntry>();
@@ -238,21 +275,22 @@ public class JarSigner implements CertVerifier {
                     CodeSigner[] signers = je.getCodeSigners();
                     boolean isSigned = (signers != null);
                     anySigned |= isSigned;
-                    hasUnsignedEntry |= !je.isDirectory() && !isSigned
-                                        && !signatureRelated(name);
+
+                    boolean shouldHaveSignature = !je.isDirectory()
+                                                && !signatureRelated(name);
+
+                    hasUnsignedEntry |= shouldHaveSignature &&  !isSigned;
+
+                    if (shouldHaveSignature)
+                        totalSignableEntries++;
+
                     if (isSigned) {
-                        // TODO: Perhaps we should check here that
-                        // signers.length is only of size 1, and throw an
-                        // exception if it's not?
                         for (int i = 0; i < signers.length; i++) {
                             CertPath certPath = signers[i].getSignerCertPath();
-                            if (!certs.contains(certPath))
-                                certs.add(certPath);
-
-                            //we really only want the first certPath
-                            if (!certPath.equals(this.certPath)) {
-                                this.certPath = certPath;
-                            }
+                            if (!certs.containsKey(certPath))
+                                certs.put(certPath, 1);
+                            else
+                                certs.put(certPath, certs.get(certPath) + 1);
 
                             Certificate cert = signers[i].getSignerCertPath()
                                     .getCertificates().get(0);
@@ -272,7 +310,12 @@ public class JarSigner implements CertVerifier {
                         }
                     }
                 } //while e has more elements
-            } //if man not null
+            } else { //if man not null
+
+                // Else increment totalEntries by 1 so that unsigned jars with 
+                // no manifests can't sneak in
+                totalSignableEntries++;
+            }
 
             //Alert the user if any of the following are true.
             if (!anySigned) {
@@ -313,9 +356,6 @@ public class JarSigner implements CertVerifier {
             }
         }
 
-        // check if the certs added above are in the trusted path
-        checkTrustedCerts();
-
         //anySigned does not guarantee that all files were signed.
         return (anySigned && !(hasUnsignedEntry || hasExpiredCert
                               || badKeyUsage || badExtendedKeyUsage || badNetscapeCertType || notYetValidCert)) ? verifyResult.SIGNED_OK : verifyResult.SIGNED_NOT_OK;