From df7da6c29aace17c92fe47fe386ab14ece59b5d4 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 30 Mar 2022 12:37:26 +0100 Subject: [PATCH] Fix BZ 65736 replace forceString with a String setter lookup --- .../apache/naming/factory/BeanFactory.java | 77 ++++-------- .../naming/factory/LocalStrings.properties | 1 + .../naming/factory/TestBeanFactory.java | 67 +++++++++++ .../org/apache/naming/factory/TesterBean.java | 41 +++++++ webapps/docs/changelog.xml | 6 + webapps/docs/jndi-resources-howto.xml | 112 +++--------------- 6 files changed, 151 insertions(+), 153 deletions(-) create mode 100644 test/org/apache/naming/factory/TestBeanFactory.java create mode 100644 test/org/apache/naming/factory/TesterBean.java diff --git a/java/org/apache/naming/factory/BeanFactory.java b/java/org/apache/naming/factory/BeanFactory.java index 7a42991b0509..1f207cd712d7 100644 --- a/java/org/apache/naming/factory/BeanFactory.java +++ b/java/org/apache/naming/factory/BeanFactory.java @@ -19,13 +19,9 @@ import java.beans.BeanInfo; import java.beans.Introspector; import java.beans.PropertyDescriptor; -import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.util.Enumeration; -import java.util.HashMap; import java.util.Hashtable; -import java.util.Locale; -import java.util.Map; import javax.naming.Context; import javax.naming.Name; @@ -34,6 +30,8 @@ import javax.naming.Reference; import javax.naming.spi.ObjectFactory; +import org.apache.juli.logging.Log; +import org.apache.juli.logging.LogFactory; import org.apache.naming.ResourceRef; import org.apache.naming.StringManager; @@ -92,6 +90,8 @@ public class BeanFactory implements ObjectFactory { private static final StringManager sm = StringManager.getManager(BeanFactory.class); + private final Log log = LogFactory.getLog(BeanFactory.class); // Not static + /** * Create a new Bean instance. * @@ -125,44 +125,14 @@ public Object getObjectInstance(Object obj, Name name, Context nameCtx, Hashtabl Object bean = beanClass.getConstructor().newInstance(); - /* Look for properties with explicitly configured setter */ + // Look for the removed forceString option RefAddr ra = ref.get("forceString"); - Map forced = new HashMap<>(); - String value; - if (ra != null) { - value = (String)ra.getContent(); - Class paramTypes[] = new Class[1]; - paramTypes[0] = String.class; - String setterName; - int index; - - /* Items are given as comma separated list */ - for (String param: value.split(",")) { - param = param.trim(); - /* A single item can either be of the form name=method - * or just a property name (and we will use a standard - * setter) */ - index = param.indexOf('='); - if (index >= 0) { - setterName = param.substring(index + 1).trim(); - param = param.substring(0, index).trim(); - } else { - setterName = "set" + - param.substring(0, 1).toUpperCase(Locale.ENGLISH) + - param.substring(1); - } - try { - forced.put(param, beanClass.getMethod(setterName, paramTypes)); - } catch (NoSuchMethodException|SecurityException ex) { - throw new NamingException - ("Forced String setter " + setterName + - " not found for property " + param); - } - } + log.warn(sm.getString("beanFactory.noForceString")); } Enumeration e = ref.getAll(); + String value; while (e.hasMoreElements()) { @@ -180,28 +150,13 @@ public Object getObjectInstance(Object obj, Name name, Context nameCtx, Hashtabl Object[] valueArray = new Object[1]; - /* Shortcut for properties with explicitly configured setter */ - Method method = forced.get(propName); - if (method != null) { - valueArray[0] = value; - try { - method.invoke(bean, valueArray); - } catch (IllegalAccessException| - IllegalArgumentException| - InvocationTargetException ex) { - throw new NamingException - ("Forced String setter " + method.getName() + - " threw exception for property " + propName); - } - continue; - } - int i = 0; for (i = 0; i < pda.length; i++) { if (pda[i].getName().equals(propName)) { Class propType = pda[i].getPropertyType(); + Method setProp = pda[i].getWriteMethod(); if (propType.equals(String.class)) { valueArray[0] = value; @@ -221,12 +176,22 @@ public Object getObjectInstance(Object obj, Name name, Context nameCtx, Hashtabl valueArray[0] = Double.valueOf(value); } else if (propType.equals(Boolean.class) || propType.equals(boolean.class)) { valueArray[0] = Boolean.valueOf(value); + } else if (setProp != null) { + // This is a Tomcat specific extension and is not part of the + // Java Bean specification. + String setterName = setProp.getName(); + try { + setProp = bean.getClass().getMethod(setterName, String.class); + valueArray[0] = value; + } catch (NoSuchMethodException nsme) { + throw new NamingException(sm.getString( + "beanFactory.noStringConversion", propName, propType.getName())); + } } else { - throw new NamingException( - sm.getString("beanFactory.noStringConversion", propName, propType.getName())); + throw new NamingException(sm.getString( + "beanFactory.noStringConversion", propName, propType.getName())); } - Method setProp = pda[i].getWriteMethod(); if (setProp != null) { setProp.invoke(bean, valueArray); } else { diff --git a/java/org/apache/naming/factory/LocalStrings.properties b/java/org/apache/naming/factory/LocalStrings.properties index a4d1d570f743..cbd43355ee19 100644 --- a/java/org/apache/naming/factory/LocalStrings.properties +++ b/java/org/apache/naming/factory/LocalStrings.properties @@ -14,6 +14,7 @@ # limitations under the License. beanFactory.classNotFound=Class not found: [{0}] +beanFactory.noForceString=The forceString option has been removed as a security hardening measure. Instead, if the setter method doesn't use String, a primitive or a primitive wrapper, the factory will look for a method with the same name as the setter that accepts a String and use that if found. beanFactory.noSetMethod=No set method found for property [{0}] beanFactory.noStringConversion=String conversion for property [{0}] of type [{1}] not available beanFactory.readOnlyProperty=Write not allowed for property [{0}] diff --git a/test/org/apache/naming/factory/TestBeanFactory.java b/test/org/apache/naming/factory/TestBeanFactory.java new file mode 100644 index 000000000000..ec85ff613648 --- /dev/null +++ b/test/org/apache/naming/factory/TestBeanFactory.java @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.naming.factory; + +import javax.naming.StringRefAddr; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.naming.ResourceRef; + +public class TestBeanFactory { + + private static final String IP_ADDRESS = "127.0.0.1"; + + @Test + public void testForceStringAlternativeWithout() throws Exception { + doTestForceStringAlternatove(false); + } + + + @Test + public void testForceStringAlternativeWith() throws Exception { + doTestForceStringAlternatove(true); + } + + + private void doTestForceStringAlternatove(boolean useForceString) throws Exception { + + // Create the resource definition + ResourceRef resourceRef = new ResourceRef(TesterBean.class.getName(), null, null, null, false); + StringRefAddr server = new StringRefAddr("server", IP_ADDRESS); + resourceRef.add(server); + if (useForceString) { + StringRefAddr force = new StringRefAddr("forceString", "server"); + resourceRef.add(force); + } + + // Create the factory + BeanFactory factory = new BeanFactory(); + + // Use the factory to create the resource from the definition + Object obj = factory.getObjectInstance(resourceRef, null, null, null); + + // Check the correct type was created + Assert.assertNotNull(obj); + Assert.assertEquals(obj.getClass(), TesterBean.class); + // Check the server field was set + TesterBean result = (TesterBean) obj; + Assert.assertNotNull(result.getServer()); + Assert.assertEquals(IP_ADDRESS, result.getServer().getHostAddress()); + } +} diff --git a/test/org/apache/naming/factory/TesterBean.java b/test/org/apache/naming/factory/TesterBean.java new file mode 100644 index 000000000000..d2beef275c15 --- /dev/null +++ b/test/org/apache/naming/factory/TesterBean.java @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.naming.factory; + +import java.net.InetAddress; +import java.net.UnknownHostException; + +public class TesterBean { + + private InetAddress server; + + public InetAddress getServer() { + return server; + } + + public void setServer(InetAddress server) { + this.server = server; + } + + public void setServer(String server) { + try { + this.server = InetAddress.getByName(server); + } catch (UnknownHostException e) { + throw new IllegalArgumentException(e); + } + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 1e3ca3bdf3e3..7bed1a4432c5 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -107,6 +107,12 @@
+ + 65736: Disable the forceString option for the + JNDI BeanFactory and replace it with an automatic search + for an alternative setter with the same name that accepts a + String. This is a security hardening measure. (markt) + 65853: Refactor the CsrfPreventionFilter to make it easier for sub-classes to modify the nonce generation and storage. diff --git a/webapps/docs/jndi-resources-howto.xml b/webapps/docs/jndi-resources-howto.xml index 31b08f6159d1..2736fb3d428d 100644 --- a/webapps/docs/jndi-resources-howto.xml +++ b/webapps/docs/jndi-resources-howto.xml @@ -328,103 +328,21 @@ writer.println("foo = " + bean.getFoo() + ", bar = " + foo property (although we could have), the bean will contain whatever default value is set up by its constructor.

-

Some beans have properties with types that cannot automatically be - converted from a string value. Setting such properties using the Tomcat - BeanFactory will fail with a NamingException. In cases were those beans - provide methods to set the properties from a string value, the Tomcat - BeanFactory can be configured to use these methods. The configuration is - done with the forceString attribute.

- -

Assume our bean looks like this:

- - - -

The bean has two properties, both are of type InetAddress. - The first property local has an additional setter taking a - string argument. By default the Tomcat BeanFactory would try to use the - automatically detected setter with the same argument type as the property - type and then throw a NamingException, because it is not prepared to convert - the given string attribute value to InetAddress. - We can tell the Tomcat BeanFactory to use the other setter like that:

- - - ... - - ... -]]> - -

The bean property remote can also be set from a string, - but one has to use the non-standard method name host. - To set local and remote use the following - configuration:

- - - ... - - ... -]]> - -

Multiple property descriptions can be combined in - forceString by concatenation with comma as a separator. - Each property description consists of either only the property name - in which case the BeanFactory calls the setter method. Or it consist - of name=method in which case the property named - name is set by calling method method. - For properties of types String or of primitive type - or of their associated primitive wrapper classes using - forceString is not needed. The correct setter will be - automatically detected and argument conversion will be applied.

- +

If the bean property is of type String, the BeanFactory + will call the property setter using the provided property value. If the + bean property type is a primitive or a primitive wrapper, the the + BeanFactory will convert the value to the appropriate primitive or + primitive wrapper and then use that value when calling the setter. Some + beans have properties with types that cannot automatically be converted + from String. If the bean provides an alternative setter with + the same name that does take a String, the BeanFactory will + attempt to use that setter. If the BeanFactory cannot use the value or + perform an appropriate conversion, setting the property will fail with a + NamingException.

+ +

The forceString property available in earlier Tomcat + releases has been removed as a security hardening measure.

+