[RFC][icedtea-web]: PR1049 fix - extension jnlp with empty jars
adomurad
adomurad at redhat.com
Tue Jul 17 12:07:38 PDT 2012
Hey thanks for tackling this, comments inline.
On Mon, 2012-07-09 at 12:54 -0400, Saad Mohammad wrote:
> Hi,
>
> The following patch fixes PR1049 and accepts extension loaders
> containing only empty jars (jars with no content or only META-INF/*).
> The handling of empty jars behaves much like the proprietary plugin, it
> will not pop up any security dialog even if the content of META-INF/* is
> signed. Changelog and reproducers are also attached.
>
> http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1049
>
> [More information]
> Presently, JarCertVerifier verifies empty jars with the result as
> verifyResult.SIGNED_OK and causes problems when
> JarCertVerifier.isFullySignedByASingleCert() is called because the list
> of certificates is empty (certificates arenot added from empty jars if
> found). This patch resolves this issue by keeping track
> ofJarCertVerifier with allempty jars.
>
> [Changelog]
>
> 2012-07-09 Saad Mohammad <smohammad at redhat.com>
>
> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
> (initializeResources): Removes the display of the security dialog for
> loaders with only empty jars.
> * netx/net/sourceforge/jnlp/tools/JarCertVerifier.java:
> (JarCertVerifier): Tracks whether all jars verified are empty jars.
> (hasAllEmptyJars): Returns true if all jars verified are empty jars.
> (verifyJars): Checks whether signable entries and certificates are
> found and
> decides if all jars are empty jars.
> (isFullySignedByASingleCert): If all jars are emptyJars, returns true.
> *
> tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJar.jnlp:
> Launching jnlp with the resource of a the main jar and an extension
> jnlp.
> *
> tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJarExtension.jnlp:
> Extension jnlp containing only an empty jar.
> *
> tests/reproducers/signed/EmptySignedJar/testcases/EmptySignedJarTest.java:
> Testcase that tests jnlp files with empty jars.
> *
> tests/reproducers/signed/SignedJarResource/resources/SignedJarResource.jnlp:
> Launches SignedJarResource class directly.
>
Reproducer patch:
> diff --git a/tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJar.jnlp b/tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJar.jnlp
> [snipped]
> +***********************************************************************
> +This file contains the main jar and an extension jnlp for its resources - the extension jnlp
> +contains a empty jar
> +***********************************************************************
> + -->
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0" href="EmptySignedJar.jnlp" codebase=".">
> + <information>
> + <title>EmptySignedJar</title>
> + <vendor>IcedTea</vendor>
> + <homepage href="http://icedtea.classpath.org/wiki/IcedTea-Web#Testing_IcedTea-Web"/>
> + <description>EmptySignedJar</description>
> + <offline/>
> + </information>
> +
> + <security>
> + <all-permissions/>
> + </security>
> +
> + <resources>
> + <j2se version="1.6+"/>
> + <jar href="SignedJarResource.jar"/>
> + <extension name="EmptySignedJarExtension" href="./EmptySignedJarExtension.jnlp"/>
> + </resources>
> +
> + <application-desc main-class="SignedJarResource">
> + </application-desc>
> +</jnlp>
> diff --git a/tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJarExtension.jnlp b/tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJarExtension.jnlp
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/signed/EmptySignedJar/resources/EmptySignedJarExtension.jnlp
> @@ -0,0 +1,58 @@
> +<!--
> + [snipped]
> +
> +***********************************************************************
> +This file is used as an extension jnlp for the launching jnlp's resource - contains
> +only an empty jar
> +***********************************************************************
> + -->
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0" href="EmptySignedJarExtension.jnlp" codebase=".">
> + <information>
> + <title>EmptySignedJarExtension</title>
> + <vendor>IcedTea</vendor>
> + <homepage href="http://icedtea.classpath.org/wiki/IcedTea-Web#Testing_IcedTea-Web"/>
> + <description>EmptySignedJarExtension</description>
> + <offline/>
> + </information>
> +
> + <resources>
> + <j2se version="1.6+"/>
> + <jar href="EmptySignedJar.jar"/>
> + </resources>
> +
> + <component-desc />
> +</jnlp>
> diff --git a/tests/reproducers/signed/EmptySignedJar/testcases/EmptySignedJarTest.java b/tests/reproducers/signed/EmptySignedJar/testcases/EmptySignedJarTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/signed/EmptySignedJar/testcases/EmptySignedJarTest.java
> @@ -0,0 +1,66 @@
> +/* EmptySignedJar.java
>
> +> [snipped]
> +
> +import java.util.Arrays;
> +import java.util.Collections;
> +import java.util.List;
> +import net.sourceforge.jnlp.ServerAccess;
> +import net.sourceforge.jnlp.annotations.Bug;
> +
> +import org.junit.Assert;
> +import org.junit.Test;
> +
> +public class EmptySignedJarTest {
> +
> + private static ServerAccess server = new ServerAccess();
> + private final List<String> l = Collections.unmodifiableList(Arrays.asList(new String[] { "-Xtrustall" }));
> + private final String jarOutput = "Running SignedJarResource..";
> +
> + @Test
> + public void checkingForRequiredResources() throws Exception {
> + String s = "Running SignedJarResource..";
> + ServerAccess.ProcessResult pr = server.executeJavawsHeadless(l, "/SignedJarResource.jnlp");
> + Assert.assertTrue("Could not locate SignedJarResource class within SignedJarResource jar", pr.stdout.contains(s));
> + }
> +
> + @Bug(id = "PR1049")
> + @Test
> + public void usingExtensionWithEmptyJar() throws Exception {
> + ServerAccess.ProcessResult pr = server.executeJavawsHeadless(l, "/EmptySignedJar.jnlp");
> + Assert.assertTrue("Stdout should contain " + jarOutput + " but did not", pr.stdout.contains(jarOutput));
> + }
> +}
This test 'passes' with HEAD ? It does bring up a pop up window, however
this is not a proper test if it claims a pass if you close the window...
> diff --git a/tests/reproducers/signed/SignedJarResource/resources/SignedJarResource.jnlp b/tests/reproducers/signed/SignedJarResource/resources/SignedJarResource.jnlp
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/signed/SignedJarResource/resources/SignedJarResource.jnlp
> @@ -0,0 +1,62 @@
> +[snipped]
> +***********************************************************************
> +Launches SignedJarResource directly
> +***********************************************************************
> + -->
> +<?xml version="1.0" encoding="utf-8"?>
> +<jnlp spec="1.0" href="SignedJarResource.jnlp" codebase=".">
> + <information>
> + <title>SignedJarResource</title>
> + <vendor>IcedTea</vendor>
> + <homepage href="http://icedtea.classpath.org/wiki/IcedTea-Web#Testing_IcedTea-Web"/>
> + <description>SignedJarResource</description>
> + <offline/>
> + </information>
> +
> + <security>
> + <all-permissions/>
> + </security>
> +
> + <resources>
> + <j2se version="1.6+"/>
> + <jar href="SignedJarResource.jar" main="true"/>
> + </resources>
> +
> + <application-desc main-class="SignedJarResource">
> + </application-desc>
> +</jnlp>
Patch itself:
> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
> @@ -600,7 +600,7 @@
> file.setSignedJNLPAsMissing();
>
> //user does not trust this publisher
> - if (!jcv.getAlreadyTrustPublisher()) {
> + if (!jcv.getAlreadyTrustPublisher() && !jcv.hasAllEmptyJars()) {
> checkTrustWithUser(jcv);
> } else {
> /**
> diff --git a/netx/net/sourceforge/jnlp/tools/JarCertVerifier.java b/netx/net/sourceforge/jnlp/tools/JarCertVerifier.java
> --- a/netx/net/sourceforge/jnlp/tools/JarCertVerifier.java
> +++ b/netx/net/sourceforge/jnlp/tools/JarCertVerifier.java
> @@ -103,6 +103,16 @@
>
> private int totalSignableEntries = 0;
>
> + /** Whether all jars are empty (jars with no content or only META-INF/*) */
> + private boolean allEmptyJars = false;
I think this isn't something JCV should concern itself with.
I think a better solution would be to check if there are non-META_INF/ entries, and only run JarCertVerifier then.
Or, as an alternative, you could remain this to 'triviallySigned', to align it more with the intent of JCV.
> +
> + /**
> + * Return true if all jars are empty jars
> + */
> + public boolean hasAllEmptyJars() {
> + return allEmptyJars;
> + }
> +
As above, if you're going to keep this, a clearer name will probably be 'isTriviallySigned'.
> /* (non-Javadoc)
> * @see net.sourceforge.jnlp.tools.CertVerifier2#getAlreadyTrustPublisher()
> */
> @@ -167,6 +177,9 @@
> */
> public boolean isFullySignedByASingleCert() {
>
> + if(allEmptyJars)
> + return true;
> +
Spacing issue, it helps to apply autoformat whenever you write code (it may mess up rest of file, so highlight just your code before doing so).
> for (CertPath cPath : certs.keySet()) {
> // If this cert has signed everything, return true
> if (certs.get(cPath) == totalSignableEntries)
> @@ -197,6 +210,7 @@
>
> String localFile = jarFile.getAbsolutePath();
> verifyResult result = verifyJar(localFile);
> + allEmptyJars = false;
>
> if (result == verifyResult.UNSIGNED) {
> unverifiedJars.add(localFile);
> @@ -205,6 +219,7 @@
> verifiedJars.add(localFile);
> } else if (result == verifyResult.SIGNED_OK) {
> verifiedJars.add(localFile);
> + allEmptyJars = totalSignableEntries <= 0 && certs.size() <= 0;
> }
> } catch (Exception e) {
> // We may catch exceptions from using verifyJar()
Overall patch looks OK... however behaviour of reproducers does not seem to change when it is applied ?
More information about the distro-pkg-dev
mailing list