[RFC][icedtea-web] extend reproducers engine for signed applications
Omair Majid
omajid at redhat.com
Wed Sep 21 09:55:51 PDT 2011
On 09/21/2011 06:19 AM, Jiri Vanek wrote:
> On 09/20/2011 09:13 PM, Omair Majid wrote:
>> On 09/20/2011 10:42 AM, Jiri Vanek wrote:
>>> Hi!
> ...
>>> "accepted" to icedtea-web.
>>> I was afraid, tahat -XtrustAll could be misused in way described in
>>> ReadPropertiesBySignedHack (included in *xtrustAllHack*) - that
>>> xtrustall can be setted on by reflection from signed code (this is
>>> possible) and then unsigned content can be loaded but it is not
>>> (luckily) possible, as SecurityMAnager is compalining about different
>>> signature (which is correct behaviour)
>>
>> I am not sure if there is much that we can do. After all, trusted code
>> can download and run arbitrary code on your machine already. We can
>> try to make it harder for developers to make mistakes, but there is
>> not much we can do to completely stop them.
>>
>>> I'm not sure weather to include this patch into icedtea-web.
>>>
>>
>> I dont have strong feelings either way, but I am leaning towards
>> leaving it out.
>>
>
> The xtrustAllHack was more or less intended as brainstorming whether
> -XtrustAll can be misused or not. For now, I believe that it can not be.
> Whether to include it or not,I'm not sure. It is an manual how not to do
> things, but also some guide "this hack is not working, try another one".
> The only reason i would like to have it inside is, that maybe, in some
> future, SecurityManager in icedtea-web will be replaced or security
> mechanisms in (jdk7,jdk8...15..) will change and this test will suddenly
> fail.
> Everything else (Unless you have some more concepts how to misuse
> xtrusall) is speaking against including this test.
> Current patches do not include this hack (so much I'm confused about
> including it).
>
It's really up to you. If you want to add it, I will be happy to review it.
>>
>>> @@ -635,6 +665,20 @@
>>> ln -sf $(JAR) $(BOOT_DIR)/bin/jar
>>> ln -sf $(abs_top_builddir)/javac $(BOOT_DIR)/bin/javac
>>> ln -sf $(JAVADOC) $(BOOT_DIR)/bin/javadoc
>>> + if [ -e "$(KEYTOOL)" ] ; then \
>>> + ln -sf $(KEYTOOL) $(BOOT_DIR)/bin/keytool ;\
>>> + else \
>>> + echo "#! /bin/sh"> $(BOOT_DIR)/bin/keytool ;\
>>> + echo "echo \"keytool not exist on your system\"">>
>>> $(BOOT_DIR)/bin/keytool ;\
>>> + chmod 777 $(BOOT_DIR)/bin/keytool ;\
>>> + fi
>>> + if [ -e "$(JARSIGNER)" ] ; then \
>>> + ln -sf $(JARSIGNER) $(BOOT_DIR)/bin/jarsigner ;\
>>> + else \
>>> + echo "#! /bin/sh"> $(BOOT_DIR)/bin/jarsigner ;\
>>> + echo "echo \"jarsigner not exist on your system\"">>
>>> $(BOOT_DIR)/bin/jarsigner ;\
>>> + chmod 777 $(BOOT_DIR)/bin/jarsigner ;\
>>> + fi
>>
>> :)
>>
>
> Smiling are you, yeah? :D That is good thing :) I was afraid what you
> will tell on this.. 'bootstrap tools'.... I was really thinking hard how
> to deal with your sentence "What happens if JARSIGNER and/or KEYTOOL is
> not found? "
> (http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2011-June/014919.html).
>
> Are you ok with it?
>
It might be better to use autoconf for this. See jrunscript for an
example. It's okay to do that as a separate patch though. This is fine
for now.
> I was also thinking about --with-jarsigner(keytool)=no. (same source).
> At the end i did not implement it, as I think failing tests are enough
> as reaction (and no added to much lines)
>
>>> mkdir -p $(BOOT_DIR)/jre/lib&& \
>>> ln -s $(SYSTEM_JRE_DIR)/lib/rt.jar $(BOOT_DIR)/jre/lib&& \
>>> if [ -e $(SYSTEM_JRE_DIR)/lib/jsse.jar ] ; then \
>>> diff -r e9a9792ee189 acinclude.m4
>>> --- a/acinclude.m4 Thu Sep 15 15:27:40 2011 +0200
>>> +++ b/acinclude.m4 Tue Sep 20 15:35:19 2011 +0200
>>> @@ -655,6 +655,56 @@
>>> AC_SUBST(JAVA)
>>> ])
>>>
>>> +AC_DEFUN_ONCE([IT_FIND_KEYTOOL],
>>> +[
>>> + AC_REQUIRE([IT_CHECK_FOR_JDK])
>>> + AC_MSG_CHECKING([for keytool])
>>> + AC_ARG_WITH([keytool],
>>> + [AS_HELP_STRING(--with-keytool,specify location of keytool for
>>> signed part of run-netx-dist)],
>>> + [
>>> + KEYTOOL="${withval}"
>>
>> AFAIK, this will break if you run "./configure --with-keytool",
>> because ${withval} is "yes" in that case.
>
> IMHO it did not break it. the lines:
>
> if ! test -f "${KEYTOOL}"; then
> + AC_PATH_PROG(KEYTOOL, keytool)
> + fi
>
> cough it (unless yes file exists in working directory), nut yes the path
> to key tool was then different (eg /bin/keytool), so I have added check
> for "yes" case.
>
Sorry, my mistake. But thanks for making it more obvious.
>>> diff -r e9a9792ee189 tests/jnlp_tests/README
>>> --- a/tests/jnlp_tests/README Thu Sep 15 15:27:40 2011 +0200
>>> ...
>>> Files in advanced directory have to care about themselves, but even
>>> those can have some parts inside simple directory, so some parts of
>>> them are processed automatically. There are three reproducers –
>>> simpletest1, simpletest2 and deadlocktest, which tests test’s suite
>>> itself and serve as examples of behaviour.
>>>
>>
>> Any chance this could be line-wrapped?
>
> Although I think that line-wrapping is a way *for* view, but if you
> wish :) - refactored.
> diff -r e9a9792ee189 tests/jnlp_tests/README
> --- a/tests/jnlp_tests/README Thu Sep 15 15:27:40 2011 +0200
> +++ b/tests/jnlp_tests/README Wed Sep 21 11:03:01 2011 +0200
> @@ -1,2 +1,5 @@
> -Each file in directory simple must follows naming convention and is compiled/jared automatically into server's working directory and content of resources likewise. The name of jnlp is independent, and there can be even more jnlps for each future jar. Directories should be honored in srcs and in resources, but noty in testcases.
> -Files in advanced directory have to care about themselves, but even those can have some parts inside simple directory, so some parts of them are processed automatically. There are three reproducers – simpletest1, simpletest2 and deadlocktest, which tests test’s suite itself and serve as examples of behaviour.
> +Each file in directory simple must follows hierarchy conventions and is compiled/jared automatically into server's working directory and content of resources likewise.
> + The name of jnlp is independent, and there can be even more jnlps for each future jar. Directories are honored in srcs and in resources, but noty in testcases.
> +Directories in signed hande their content in similar way as simle's content is handled, but in addition final jars are signed with simple testkey.
> +Files in advanced directory have to care about themselves, but even those can have some parts inside simple directory, so some parts of them are processed automatically.
> + There are three reproducers – simpletest1, simpletest2 and deadlocktest, which tests test’s suite itself and serve as examples of behaviour.
80 (or something else that is somewhat reasonable) columns please.
> +public class ReadPropertiesSigned {
> +
> +/**
> +*some system property is expected as arg[0], eg user.name or user.home
> +*/
Indentation is a little off.
> + public static void main(String[] args) {
> + System.out.println(System.getProperty(args[0]));
> + }
> +}
> diff -r e9a9792ee189 tests/jnlp_tests/signed/ReadPropertiesSigned/testcases/ReadPropertiesSignedTest.java
> --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> +++ b/tests/jnlp_tests/signed/ReadPropertiesSigned/testcases/ReadPropertiesSignedTest.java Wed Sep 21 11:03:01 2011 +0200
> +public class ReadPropertiesSignedTest {
> +
> + private static ServerAccess server = new ServerAccess();
> + private final List<String> l=Collections.unmodifiableList(Arrays.asList(new String[] {"-Xtrustall"}));
> +
> +
> + public ReadPropertiesSignedTest() {
> +
> + }
> +
Perhaps this can be removed?
> +
> + @Test
> + public void ReadSignedPropertiesWithoutPermissionsWithXtrustAll() throws Exception {
> + //no request for permissions
> + System.out.println("connecting ReadPropertiesSigned1 request");
> + System.err.println("connecting ReadPropertiesSigned1 request");
> + ServerAccess.ProcessResult pr=server.executeJavawsHeadless(l,"/ReadPropertiesSigned1.jnlp");
> + System.out.println(pr.stdout);
> + System.err.println(pr.stderr);
> + String s="java.security.AccessControlException: access denied (java.util.PropertyPermission user.name read)";
> + Assert.assertTrue("Stderr should contains "+s+" but did not",pr.stderr.contains(s));
> + String ss="ClassNotFoundException";
> + Assert.assertFalse("Stderr should not contains "+ss+" but did",pr.stderr.contains(ss));
> + Assert.assertTrue("stdout lenght should be<2 but was "+pr.stdout.length(),pr.stdout.length()<2); // /home/user or /root or eanything else :(
> + Assert.assertFalse("should not be terminated but was",pr.wasTerminated);
> + Assert.assertEquals((Integer)0, pr.returnValue);
> + }
> +
> + @Test
> + public void ReadSignedPropertiesWithPermissionsWithXtrustAll() throws Exception {
> + //request for allpermissions
> + System.out.println("connecting ReadPropertiesSigned2 request");
> + System.err.println("connecting ReadPropertiesSigned2 request");
> + ServerAccess.ProcessResult pr=server.executeJavawsHeadless(l,"/ReadPropertiesSigned2.jnlp");
> + System.out.println(pr.stdout);
> + System.err.println(pr.stderr);
> + String s="java.security.AccessControlException: access denied (java.util.PropertyPermission user.name read)";
> + Assert.assertFalse("Stderr should NOT contains "+s+" but did",pr.stderr.contains(s));
> + String ss="ClassNotFoundException";
> + Assert.assertFalse("Stderr should not contains "+ss+" but did",pr.stderr.contains(ss));
> + Assert.assertTrue("stdout lenght should be>= but was "+pr.stdout.length(),pr.stdout.length()>=4); // /home/user or /root or eanything else :(
> + Assert.assertFalse("should not be terminated but was",pr.wasTerminated);
> + Assert.assertEquals((Integer)0, pr.returnValue);
> + }
> +
> + @Test
Formatting issue?
> + public void EnsureXtrustallNotAffectingUnsignedBehaviour() throws Exception {
> + System.err.println("connecting ReadPropertiesSigned3 request");
> + System.out.println("connecting ReadPropertiesSigned3 request");
> + ServerAccess.ProcessResult pr=server.executeJavawsHeadless(l,"/ReadProperties1.jnlp");
> + System.out.println(pr.stdout);
> + System.err.println(pr.stderr);
> + String s="java.security.AccessControlException: access denied (java.util.PropertyPermission user.name read)";
> + Assert.assertTrue(pr.stderr.contains(s));
> + String ss="ClassNotFoundException";
> + Assert.assertFalse("Stderr should not contains "+ss+" but did",pr.stderr.contains(ss));
> + Assert.assertFalse("stdout lenght should not be>2 but was "+pr.stdout.length(),pr.stdout.length()>2);
> + Assert.assertFalse("should not be terminated but was",pr.wasTerminated);
> + Assert.assertEquals((Integer)0, pr.returnValue);
> + ServerAccess.ProcessResult pr2=server.executeJavawsHeadless(null,"/ReadProperties1.jnlp");
> + Assert.assertEquals(pr.stderr, pr2.stderr);
> + Assert.assertEquals(pr.stdout, pr2.stdout);
> +
> + }
> + }
Cheers,
Omair
More information about the distro-pkg-dev
mailing list