[rfc][icedtea-web] Reproducer & unit test for unsigned content in META-INF/

[Jiri Vanek]

On 07/05/2012 05:27 PM, Adam Domurad wrote:
> Note, bugfix already in HEAD. Reproducer sanity checked by backing out
> -r429 in HEAD.
>
> Reproducer functions by copying and modifying ReadPropertiesSigned.jar
> to have unsigned content in META-INF/ directory. If it is felt that it
> should sign and create its own jar let me know.
>
> Unit test changelog:
> 2012-07-05  Adam Domurad<adomurad at redhat.com>
>
> 	Unit test for method in JCV, isMetaInfFile()
> 	* netx/net/sourceforge/jnlp/tools/JarCertVerifier.java:
> 	Made isMetaInfFile package-private for testing purposes.
> 	* tests/netx/unit/net/sourceforge/jnlp/tools/JarCertVerifierTest.java:
> 	New, tests isMetaInfFile()

Unittest seams ok. As it is modifying also a code a bit, do you mind head + 1.3?

>
> Reproducer changelog:
> 2012-07-05  Adam Domurad<adomurad at redhat.com>
>
> 	Reproducer for allowing unsigned content in META-INF/ folder.
> 	Derives from ReadPropertiesSigned test's signed jar.
> 	*
> tests/reproducers/custom/UnsignedContentInMETAINF/resources/UnsignedContentInMETAINF.jnlp:
> 	New, runs a modified version of ReadPropertiesSigned.jar
> (UnsignedContentInMETAINF.jar)
> 	*
> tests/reproducers/custom/UnsignedContentInMETAINF/srcs/META-INF/unsigned_file_in_metainf:
> 	New, placed into a modified version of ReadPropertiesSigned.jar
> 	(UnsignedContentInMETAINF.jar) so that there is unsigned content in the
> 	META-INF/ folder.
> 	* tests/reproducers/custom/UnsignedContentInMETAINF/srcs/Makefile:
> 	New, creates a modified version of ReadPropertiesSigned.jar, named
> 	UnsignedContentInMETAINF.jar, and places unsigned content inside its
> 	META-INF/ folder
> 	*
> tests/reproducers/custom/UnsignedContentInMETAINF/testcases/UnsignedContentInMETAINF.java:
> 	Test driver for jnlp file
>
> Thanks,
> Adam
>


Thanx for reproducer.  Overall looks good, one clarification below however needed.


>
> unsigned-metainf-content-reproducer.patch
>

...snip...

> +</jnlp>
> diff --git a/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/META-INF/unsigned_file_in_metainf b/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/META-INF/unsigned_file_in_metainf
> new file mode 100644

Purpose of this is to create new file (I have not seen it used in reproducer) or empty directory in 
src (which I have not seen used too)?

If there is need for empty file or (or dir) then touch (mkdir) in custom Makefile perhaps?  Although 
I agree that have those files in file tree is little bit clearer, mercurial do not like empty 
directories :(

Also  - it is in src so you expect to have it compiled (as you are overwriting embedded jar's work) 
or copied?  it is not clear  from makefile (but I can be dummy enough to miss it)


> diff --git a/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/Makefile b/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/Makefile
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/custom/UnsignedContentInMETAINF/srcs/Makefile
> @@ -0,0 +1,17 @@
> +TESTNAME=UnsignedContentInMETAINF
> +JAVAC_CLASSPATH=$(JNLP_TESTS_ENGINE_DIR):$(NETX_DIR)/lib/classes.jar
> +DEPLOY_DIR=$(JNLP_TESTS_SERVER_DEPLOYDIR)
> +JAVAC=$(BOOT_DIR)/bin/javac
> +JAR=$(BOOT_DIR)/bin/jar
> +
> +prepare-reproducer:
> +	echo PREPARING REPRODUCER $(TESTNAME)

> +	cp $(DEPLOY_DIR)/ReadPropertiesSigned.jar $(DEPLOY_DIR)/UnsignedContentInMETAINF.jar
> +	$(JAR) uf $(DEPLOY_DIR)/UnsignedContentInMETAINF.jar META-INF

those two lines above are what is  confusing me:
you  are coping ReadPropertiesSigned.jar and overwriting UnsignedContentInMETAINF.jar.
Then you are updating this copy for  META-INF (directory?)  which do not exists(?)
Although if it would exists, then it would be nice to have deirectory where this jar u is happening.

 From both above - probably wrongly placed META-INF and strange cp and jar u, I think  that 
reproducer is not testing what it should.

Or am I wrong and  pure $(JAR) uf $(DEPLOY_DIR)/UnsignedContentInMETAINF.jar META-INF, with 
non-existent   META-INF directory will cause timestamps in jar to change and so make signatures of 
metainf invalid? But then you do not need this 
tests/reproducers/custom/UnsignedContentInMETAINF/srcs/META-INF/unsigned_file_in_metainf misleading 
file.
If this is true, then perhaps more visible approach then just modifying the time-stamp can be used?


Rest of the reproducer  is nice work, thanks for it!

J.
> +	echo PREPARED REPRODUCER $(TESTNAME)s
> +
> +clean-reproducer:
> +	echo CLEANING REPRODUCER $(TESTNAME)
> +	rm -f UnsignedContentInMETAINF.jar
> +	echo CLEANED REPRODUCER $(TESTNAME)
> +
> diff --git a/tests/reproducers/custom/UnsignedContentInMETAINF/testcases/UnsignedContentInMETAINF.java b/tests/reproducers/custom/UnsignedContentInMETAINF/testcases/UnsignedContentInMETAINF.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/reproducers/custom/UnsignedContentInMETAINF/testcases/UnsignedContentInMETAINF.java
> @@ -0,0 +1,65 @@
> +/* UnsignedContentInMETAINF.java
> +Copyright (C) 2011 Red Hat, Inc.
> +
> +This file is part of IcedTea.
> +
> +IcedTea is free software; you can redistribute it and/or
> +modify it under the terms of the GNU General Public License as published by
> +the Free Software Foundation, version 2.
> +
> +IcedTea is distributed in the hope that it will be useful,
> +but WITHOUT ANY WARRANTY; without even the implied warranty of
> +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +General Public License for more details.
> +
> +You should have received a copy of the GNU General Public License
> +along with IcedTea; see the file COPYING.  If not, write to
> +the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> +02110-1301 USA.
> +
> +Linking this library statically or dynamically with other modules is
> +making a combined work based on this library.  Thus, the terms and
> +conditions of the GNU General Public License cover the whole
> +combination.
> +
> +As a special exception, the copyright holders of this library give you
> +permission to link this library with independent modules to produce an
> +executable, regardless of the license terms of these independent
> +modules, and to copy and distribute the resulting executable under
> +terms of your choice, provided that you also meet, for each linked
> +independent module, the terms and conditions of the license of that
> +module.  An independent module is a module which is not derived from
> +or based on this library.  If you modify this library, you may extend
> +this exception to your version of the library, but you are not
> +obligated to do so.  If you do not wish to do so, delete this
> +exception statement from your version.
> + */
> +
> +
> +import java.util.Arrays;
> +import java.util.Collections;
> +import java.util.List;
> +import net.sourceforge.jnlp.ServerAccess;
> +import org.junit.Assert;
> +import org.junit.Test;
> +import net.sourceforge.jnlp.ProcessResult;;
> +
> +public class UnsignedContentInMETAINF {
> +
> +    private static ServerAccess server = new ServerAccess();
> +    private final List<String> l=Collections.unmodifiableList(Arrays.asList(new String[] {"-Xtrustall"}));
> +
> +    String accessMatcher = "(?s).*java.security.AccessControlException.{0,5}access denied.{0,5}java.util.PropertyPermission.{0,5}" + "user.name.{0,5}read" + ".*";
> +
> +    @Test
> +    public void ReadSignedPropertiesWithUnsignedContentInMETAINF() throws Exception {
> +        //request for allpermissions
> +        ProcessResult pr=server.executeJavawsHeadless(l,"/UnsignedContentInMETAINF.jnlp");
> +        Assert.assertFalse("Stderr should NOT match "+accessMatcher+" but did",pr.stderr.matches(accessMatcher));
> +        String ss="ClassNotFoundException";
> +        Assert.assertFalse("Stderr should not contain "+ss+" but did",pr.stderr.contains(ss));
> +        Assert.assertTrue("Stdout length should be >= 4 but was "+pr.stdout.length(),pr.stdout.length()>=4); // /home/user or /root or anything else :(
> +        Assert.assertFalse("Should not be terminated but was",pr.wasTerminated);
> +        Assert.assertEquals((Integer)0, pr.returnValue);
> +    }
> +  }
>
>
> JCVIsMetaInfFile.patch
>
>
> 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
> @@ -441,7 +441,7 @@ public class JarCertVerifier implements
>        * . META-INF/*.DSA
>        * . META-INF/*.RSA
>        */
> -    static private boolean isMetaInfFile(String name) {
> +    static boolean isMetaInfFile(String name) {
>           String ucName = name.toUpperCase();
>           return ucName.startsWith(META_INF);
>       }
> diff --git a/tests/netx/unit/net/sourceforge/jnlp/tools/JarCertVerifierTest.java b/tests/netx/unit/net/sourceforge/jnlp/tools/JarCertVerifierTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/netx/unit/net/sourceforge/jnlp/tools/JarCertVerifierTest.java
> @@ -0,0 +1,17 @@
> +package net.sourceforge.jnlp.tools;
> +
> +import static org.junit.Assert.*;
> +
> +import org.junit.Test;
> +
> +public class JarCertVerifierTest {
> +
> +    @Test
> +    public void testIsMetaInfFile() {
> +        final String METAINF ="META-INF";
> +        assertFalse(JarCertVerifier.isMetaInfFile("some_dir/" + METAINF + "/filename"));
> +        assertFalse(JarCertVerifier.isMetaInfFile(METAINF + "filename"));
> +        assertTrue(JarCertVerifier.isMetaInfFile(METAINF + "/filename"));
> +    }
> +
> +}
>