[rfc][icedtea-web] Fix for PR1198, JSObject passed incorrectly to Javascript
Jiri Vanek
jvanek at redhat.com
Mon Jan 7 08:44:10 PST 2013
On 01/07/2013 04:47 PM, Adam Domurad wrote:
> On 01/07/2013 09:53 AM, Jiri Vanek wrote:
>>> [.. rest snipped..]
>> Isn't missing updated junit patch here?
>
> Sorry, was awaiting your advice -- if you did not like this security-manager-in-constructor change
> than the unit test would not be valid, a quite different approach would have to be used.
>
>>
>> Thanx for looking into it.
>> J.
>>
>
>
> patch attached,
> -Adam
>
> jsfix-unittest2.patch
>
>
> diff --git a/plugin/icedteanp/java/netscape/javascript/JSObjectUnboxPermission.java b/plugin/icedteanp/java/netscape/javascript/JSObjectUnboxPermission.java
> new file mode 100644
> --- /dev/null
> +++ b/plugin/icedteanp/java/netscape/javascript/JSObjectUnboxPermission.java
> @@ -0,0 +1,49 @@
> +/* JSObjectUnboxPermission.java
> + Copyright (C) 2012 Red Hat
> +
> +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; either version 2, or (at your option)
> +any later version.
> +
> +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. */
> +
> +package netscape.javascript;
> +
> +import java.security.BasicPermission;
> +
> +/**
> + * Permission to access internal reference of JSObject
> + */
> +public class JSObjectUnboxPermission extends BasicPermission {
> + public JSObjectUnboxPermission() {
> + super("JSObjectUnbox");
> + }
> +}
> diff --git a/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java b/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> --- a/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginAppletSecurityContext.java
> @@ -53,7 +53,6 @@ import java.security.Permissions;
> import java.security.PrivilegedAction;
> import java.security.ProtectionDomain;
> import java.util.ArrayList;
> -import java.util.Arrays;
> import java.util.Hashtable;
> import java.util.List;
> import java.util.Map;
> @@ -241,16 +240,6 @@ public class PluginAppletSecurityContext
> public PluginAppletSecurityContext(int identifier) {
> this.identifier = identifier;
>
> - // We need a security manager.. and since there is a good chance that
> - // an applet will be loaded at some point, we should make it the SM
> - // that JNLPRuntime will try to install
> - if (System.getSecurityManager() == null) {
> - JNLPRuntime.initialize(/* isApplication */false);
> - JNLPRuntime.setDefaultLaunchHandler(new DefaultLaunchHandler(System.err));
> - }
> -
> - JNLPRuntime.disableExit();
> -
> URL u = null;
> try {
> u = new URL("file://");
> diff --git a/plugin/icedteanp/java/sun/applet/PluginMain.java b/plugin/icedteanp/java/sun/applet/PluginMain.java
> --- a/plugin/icedteanp/java/sun/applet/PluginMain.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginMain.java
> @@ -73,6 +73,7 @@ import java.net.ProxySelector;
> import java.util.Enumeration;
> import java.util.Properties;
>
> +import net.sourceforge.jnlp.DefaultLaunchHandler;
> import net.sourceforge.jnlp.config.DeploymentConfiguration;
> import net.sourceforge.jnlp.runtime.JNLPRuntime;
> import net.sourceforge.jnlp.security.JNLPAuthenticator;
> @@ -106,6 +107,15 @@ public class PluginMain {
> // must be called before JNLPRuntime.initialize()
> JNLPRuntime.setRedirectStreams(redirectStreams);
>
> + // We need a security manager for PluginAppletSecurityContext,
> + // so we ensure it is initialized
> + if (System.getSecurityManager() == null) {
> + JNLPRuntime.initialize(/* isApplication */false);
> + JNLPRuntime.setDefaultLaunchHandler(new DefaultLaunchHandler(System.err));
> + }
> +
> + JNLPRuntime.disableExit();
> +
> PluginAppletSecurityContext sc = new PluginAppletSecurityContext(0);
> sc.prePopulateLCClasses();
Well, I'm still not comfortable with this change.
I'm not sure which security parts can be affected:-/ I really would like to know Jana's opinion here.
Do you mind to move it to separate method?
> PluginAppletSecurityContext.setStreamhandler(streamHandler);
> diff --git a/tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java b/tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java
> new file mode 100644
> --- /dev/null
> +++ b/tests/netx/unit/sun/applet/PluginAppletSecurityContextTest.java
> @@ -0,0 +1,181 @@
> +package sun.applet;
> +
> +import static org.junit.Assert.assertTrue;
> +import static org.junit.Assert.assertFalse;
> +import static org.junit.Assert.assertEquals;
> +
> +import netscape.javascript.JSObject;
> +
> +import org.junit.Test;
> +
> +public class PluginAppletSecurityContextTest {
> +
> + @Test
> + public void toIDStringNullTest() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> + assertEquals("literalreturn null",
> + pasc.toObjectIDString(null, Object.class, false));
> + }
> +
> + @Test
> + public void toIDStringVoidTest() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> + assertEquals("literalreturn void",
> + pasc.toObjectIDString(null, Void.TYPE, false));
> +
> + assertFalse("literalreturn void".equals(pasc.toObjectIDString(null,
> + Void.class, false)));
> + }
> +
> + @Test
> + public void toIDStringIntegralTest() {
> + // NB: the special .TYPE classes here represent primitives
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + // Test both unboxing allowed and not allowed to be sure it doesn't
> + // alter result
> + // although it really shouldn't
> + for (boolean unboxPrimitives : new Boolean[] { false, true }) {
> + assertEquals("literalreturn true", pasc.toObjectIDString(
> + new Boolean(true), Boolean.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(new Byte(
> + (byte) 1), Byte.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(
> + new Character((char) 1), Character.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(new Short(
> + (short) 1), Short.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(
> + new Integer(1), Integer.TYPE, unboxPrimitives));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(new Long(1),
> + Long.TYPE, unboxPrimitives));
> + }
> + }
> +
> + @Test
> + public void toIDStringBoxedIntegralNoUnboxingTest() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + assertFalse("literalreturn true".equals(pasc.toObjectIDString(
> + new Boolean(true), Boolean.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(new Byte(
> + (byte) 1), Byte.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(
> + new Character((char) 1), Character.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(new Short(
> + (short) 1), Short.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(new Integer(
> + 1), Integer.class, false)));
> +
> + assertFalse("literalreturn 1".equals(pasc.toObjectIDString(new Long(1),
> + Long.class, false)));
> + }
> +
> + @Test
> + public void toIDStringBoxedIntegralWithUnboxingTest() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + assertEquals("literalreturn true",
> + pasc.toObjectIDString(new Boolean(true), Boolean.class, true));
> +
> + assertEquals("literalreturn 1",
> + pasc.toObjectIDString(new Byte((byte) 1), Byte.class, true));
> +
> + assertEquals("literalreturn 1", pasc.toObjectIDString(new Character(
> + (char) 1), Character.class, true));
> +
> + assertEquals("literalreturn 1",
> + pasc.toObjectIDString(new Short((short) 1), Short.class, true));
> +
> + assertEquals("literalreturn 1",
> + pasc.toObjectIDString(new Integer(1), Integer.class, true));
> +
> + assertEquals("literalreturn 1",
> + pasc.toObjectIDString(new Long(1), Long.class, true));
> + }
> +
> + @Test
> + public void toIDStringFloatingPoint() {
> + final int prefixLength = "literalreturn ".length();
> +
> + // NB: the special .TYPE classes here represent primitives
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + // Test both unboxing allowed and not allowed to be sure it doesn't
> + // alter result
> + // although it really shouldn't
> + for (boolean unboxPrimitives : new Boolean[] { false, true }) {
> + {
> + final float testFloat = 3.141592f;
> + String idString = pasc.toObjectIDString(new Float(testFloat),
> + Float.TYPE, unboxPrimitives);
> + String floatRepr = idString.substring(prefixLength);
> + assertTrue(testFloat == Float.parseFloat(floatRepr));
> + }
> + {
> + final double testDouble = 3.141592;
> + String idString = pasc.toObjectIDString(new Double(testDouble),
> + Double.TYPE, unboxPrimitives);
> + String doubleRepr = idString.substring(prefixLength);
> + assertTrue(testDouble == Double.parseDouble(doubleRepr));
> + }
> +
> + }
> + {
> + final float testFloat = 3.141592f;
> + String idString = pasc.toObjectIDString(new Float(testFloat),
> + Float.class, true);
> + String floatRepr = idString.substring(prefixLength);
> + assertTrue(testFloat == Float.parseFloat(floatRepr));
> + }
> + {
> + final double testDouble = 3.141592;
> + String idString = pasc.toObjectIDString(new Double(testDouble),
> + Double.class, true);
> + String doubleRepr = idString.substring(prefixLength);
> + assertTrue(testDouble == Double.parseDouble(doubleRepr));
> + }
> + {
> + final float testFloat = 3.141592f;
> + String idString = pasc.toObjectIDString(new Float(testFloat),
> + Float.class, false);
> + assertFalse(idString.startsWith("literalreturn "));
> + }
> + {
> + final double testDouble = 3.141592;
> + String idString = pasc.toObjectIDString(new Double(testDouble),
> + Double.class, false);
> + assertFalse(idString.startsWith("literalreturn "));
> + }
> + }
> +
> +// FIXME: How can we get the permissions to do this?
> +// @Test
> +// public void toIDStringJSObject() {
> +// PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +//
> +// long testReference = 1;
> +// assertEquals("literalreturn 1", pasc.toObjectIDString(new JSObject(
> +// testReference), JSObject.class, false));
> +// }
Otherwise I'm of for test to go, even with this one commented out. I will try to revisit this once
it will be inside.
> +
> + @Test
> + public void toIDStringArbitraryObject() {
> + PluginAppletSecurityContext pasc = new PluginAppletSecurityContext(0);
> +
> + final Object testObject = new Object();
> + String idString = pasc.toObjectIDString(testObject,
> + testObject.getClass(), false);
> +
> + assertFalse(idString.startsWith("literalreturn"));
> + assertFalse(idString.startsWith("jsobject"));
> + }
> +}
>
More information about the distro-pkg-dev
mailing list