changeset 2028:1db6ba4a4593

RH672262, CVE-2011-0025: IcedTea jarfile signature verification bypass 2011-01-24 Deepak Bhole <dbhole@redhat.com> 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().
author Andrew John Hughes <ahughes@redhat.com>
date Tue, 25 Jan 2011 15:39:52 +0000
parents 500f06b81c78
children 626c4d1e1fa4
files ChangeLog NEWS rt/net/sourceforge/jnlp/runtime/JNLPClassLoader.java rt/net/sourceforge/jnlp/security/CertVerifier.java rt/net/sourceforge/jnlp/security/CertsInfoPane.java rt/net/sourceforge/jnlp/security/HttpsCertVerifier.java rt/net/sourceforge/jnlp/tools/JarSigner.java
diffstat 7 files changed, 111 insertions(+), 40 deletions(-) [+]
line wrap: on
line diff
--- a/ChangeLog	Thu Jan 20 11:11:20 2011 -0500
+++ b/ChangeLog	Tue Jan 25 15:39:52 2011 +0000
@@ -1,3 +1,33 @@
+2011-01-24  Deepak Bhole <dbhole@redhat.com>
+
+	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-20  Deepak Bhole <dbhole@redhat.com>
 
 	PR619: Improper finalization by the plugin can crash the browser
--- a/NEWS	Thu Jan 20 11:11:20 2011 -0500
+++ b/NEWS	Tue Jan 25 15:39:52 2011 +0000
@@ -10,6 +10,8 @@
 
 New in release 1.7.8 (2011-XX-XX):
 
+* Security updates
+  - RH672262, CVE-2011-0025: IcedTea jarfile signature verification bypass
 * Backports
   - S6687968: PNGImageReader leaks native memory through an Inflater
   - S6541476, RH665355: PNG imageio plugin incorrectly handles iTXt chunk
--- a/rt/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Thu Jan 20 11:11:20 2011 -0500
+++ b/rt/net/sourceforge/jnlp/runtime/JNLPClassLoader.java	Tue Jan 25 15:39:52 2011 +0000
@@ -399,7 +399,7 @@
 			}
 
 			//Case when at least one jar has some signing
-			if (js.anyJarsSigned()){
+			if (js.anyJarsSigned() && js.isFullySignedByASingleCert()){
 				signing = true;
 
 				if (!js.allJarsSigned() && 
--- a/rt/net/sourceforge/jnlp/security/CertVerifier.java	Thu Jan 20 11:11:20 2011 -0500
+++ b/rt/net/sourceforge/jnlp/security/CertVerifier.java	Tue Jan 25 15:39:52 2011 +0000
@@ -76,7 +76,7 @@
      * 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.
--- a/rt/net/sourceforge/jnlp/security/CertsInfoPane.java	Thu Jan 20 11:11:20 2011 -0500
+++ b/rt/net/sourceforge/jnlp/security/CertsInfoPane.java	Tue Jan 25 15:39:52 2011 +0000
@@ -65,7 +65,7 @@
  */
 public class CertsInfoPane extends SecurityDialogUI {
 	
-	private ArrayList<CertPath> certs;
+	private CertPath certPath;
     private JList list;
 	protected JTree tree;
     private JTable table;
@@ -84,12 +84,10 @@
 	 * Builds the JTree out of CertPaths.
 	 */
 	void buildTree() {
-		certs = ((SecurityWarningDialog)optionPane).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 = ((SecurityWarningDialog)optionPane).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 +99,9 @@
 
 		//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 +120,12 @@
 	 * 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()) + ")";
--- a/rt/net/sourceforge/jnlp/security/HttpsCertVerifier.java	Thu Jan 20 11:11:20 2011 -0500
+++ b/rt/net/sourceforge/jnlp/security/HttpsCertVerifier.java	Tue Jan 25 15:39:52 2011 +0000
@@ -82,7 +82,7 @@
         return isTrusted;
     }
 
-    public ArrayList<CertPath> getCerts() {
+    public CertPath getCertPath() {
         
         ArrayList<X509Certificate> list = new ArrayList<X509Certificate>();
         for (int i=0; i < chain.length; i++)
@@ -98,7 +98,7 @@
             // carry on
         }
 
-        return certPaths; 
+        return certPaths.get(0); 
     }
 
     public ArrayList<String> getDetails() {
--- a/rt/net/sourceforge/jnlp/tools/JarSigner.java	Thu Jan 20 11:11:20 2011 -0500
+++ b/rt/net/sourceforge/jnlp/tools/JarSigner.java	Tue Jan 25 15:39:52 2011 +0000
@@ -142,11 +142,13 @@
     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()
      */
@@ -195,18 +197,41 @@
      * @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 = (JARDesc) jars.get(i);
-            verifiedJars = new ArrayList<String>();
-            unverifiedJars = new ArrayList<String>();
 
             try {
                 
@@ -235,6 +260,22 @@
                 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 {
@@ -242,10 +283,6 @@
         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>();
@@ -283,21 +320,23 @@
                     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);
@@ -317,7 +356,12 @@
                         }
                     }
                 } //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) {
@@ -357,9 +401,6 @@
                 jarFile.close();
             }
         }
-
-        // 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