/hg/release/icedtea-web-1.0: RH672262, CVE-2011-0025: IcedTea ja...
Dr Andrew John Hughes
ahughes at redhat.com
Tue Feb 1 09:49:05 PST 2011
On 16:00 Tue 01 Feb , dbhole at icedtea.classpath.org wrote:
> changeset 3bd328e4b515 in /hg/release/icedtea-web-1.0
> details: http://icedtea.classpath.org/hg/release/icedtea-web-1.0?cmd=changeset;node=3bd328e4b515
> author: Deepak Bhole <dbhole at redhat.com>
> date: Tue Feb 01 10:53:44 2011 -0500
>
> RH672262, CVE-2011-0025: IcedTea jarfile signature verification
> bypass
>
> Fixes JAR signature handling so that multiply/partially signed jars
> are correctly handled.
>
>
> diffstat:
>
> 7 files changed, 109 insertions(+), 40 deletions(-)
> ChangeLog | 30 ++++
> NEWS | 1
> netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java | 2
> netx/net/sourceforge/jnlp/security/CertVerifier.java | 3
> netx/net/sourceforge/jnlp/security/CertsInfoPane.java | 19 +-
> netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java | 4
> netx/net/sourceforge/jnlp/tools/JarSigner.java | 90 +++++++++----
>
> diffs (309 lines):
>
> diff -r e84432bca9a3 -r 3bd328e4b515 ChangeLog
> --- a/ChangeLog Fri Jan 28 15:51:03 2011 -0500
> +++ b/ChangeLog Tue Feb 01 10:53:44 2011 -0500
> @@ -1,3 +1,33 @@ 2011-01-28 Deepak Bhole <dbhole at redhat.
> +2011-01-24 Deepak Bhole <dbhole at 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-28 Deepak Bhole <dbhole at redhat.com>
>
> * Makefile.am
> diff -r e84432bca9a3 -r 3bd328e4b515 NEWS
> --- a/NEWS Fri Jan 28 15:51:03 2011 -0500
> +++ b/NEWS Tue Feb 01 10:53:44 2011 -0500
> @@ -14,6 +14,7 @@ New in release 1.0 (2011-XX-XX):
> * Security updates
> - RH645843, CVE-2010-3860: IcedTea System property information leak via public static
> - RH663680, CVE-2010-4351: IcedTea JNLP SecurityManager bypass
> + - RH672262, CVE-2011-0025: IcedTea jarfile signature verification bypass
> * New Features
> - IcedTea-Web now uses a deployment.properties file to specify configuration
> - System-level as well as user-level deployment.properties files with locked configuration are supported
> diff -r e84432bca9a3 -r 3bd328e4b515 netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Fri Jan 28 15:51:03 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java Tue Feb 01 10:53:44 2011 -0500
> @@ -430,7 +430,7 @@ public class JNLPClassLoader extends URL
> }
>
> //Case when at least one jar has some signing
> - if (js.anyJarsSigned()) {
> + if (js.anyJarsSigned() && js.isFullySignedByASingleCert()) {
> signing = true;
>
> if (!js.allJarsSigned() &&
> diff -r e84432bca9a3 -r 3bd328e4b515 netx/net/sourceforge/jnlp/security/CertVerifier.java
> --- a/netx/net/sourceforge/jnlp/security/CertVerifier.java Fri Jan 28 15:51:03 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/security/CertVerifier.java Tue Feb 01 10:53:44 2011 -0500
> @@ -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 -r e84432bca9a3 -r 3bd328e4b515 netx/net/sourceforge/jnlp/security/CertsInfoPane.java
> --- a/netx/net/sourceforge/jnlp/security/CertsInfoPane.java Fri Jan 28 15:51:03 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/security/CertsInfoPane.java Tue Feb 01 10:53:44 2011 -0500
> @@ -67,7 +67,7 @@ import net.sourceforge.jnlp.tools.*;
> */
> public class CertsInfoPane extends SecurityDialogPanel {
>
> - private ArrayList<CertPath> certs;
> + private CertPath certPath;
> private JList list;
> protected JTree tree;
> private JTable table;
> @@ -87,12 +87,9 @@ public class CertsInfoPane extends Secur
> * 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 =
> @@ -104,9 +101,9 @@ public class CertsInfoPane extends Secur
>
> //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 =
> @@ -125,12 +122,12 @@ public class CertsInfoPane extends Secur
> * 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 -r e84432bca9a3 -r 3bd328e4b515 netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java
> --- a/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java Fri Jan 28 15:51:03 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/security/HttpsCertVerifier.java Tue Feb 01 10:53:44 2011 -0500
> @@ -83,7 +83,7 @@ public class HttpsCertVerifier implement
> 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 implement
> // carry on
> }
>
> - return certPaths;
> + return certPaths.get(0);
> }
>
> public ArrayList<String> getDetails() {
> diff -r e84432bca9a3 -r 3bd328e4b515 netx/net/sourceforge/jnlp/tools/JarSigner.java
> --- a/netx/net/sourceforge/jnlp/tools/JarSigner.java Fri Jan 28 15:51:03 2011 -0500
> +++ b/netx/net/sourceforge/jnlp/tools/JarSigner.java Tue Feb 01 10:53:44 2011 -0500
> @@ -138,10 +138,12 @@ public class JarSigner implements CertVe
> 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()
> @@ -191,18 +193,41 @@ public class JarSigner implements CertVe
> * @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 {
>
> @@ -231,16 +256,28 @@ public class JarSigner implements CertVe
> 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 {
> boolean anySigned = false;
> 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);
> @@ -280,21 +317,22 @@ public class JarSigner implements CertVe
> 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);
> @@ -314,7 +352,12 @@ public class JarSigner implements CertVe
> }
> }
> } //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) {
> @@ -354,9 +397,6 @@ public class JarSigner implements CertVe
> 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
Are we having the planned release today? You don't seem to be reachable
on IRC.
I would like some time to test your recent patches didn't break anything
but I can do that shortly.
--
Andrew :)
Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)
Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: 94EFD9D8 (http://subkeys.pgp.net)
Fingerprint = F8EF F1EA 401E 2E60 15FA 7927 142C 2591 94EF D9D8
More information about the distro-pkg-dev
mailing list