Fix BZ 65736 replace forceString with a String setter lookup

markt-asf · markt-asf · commit df7da6c29aac · 2022-05-09T19:23:29.000+01:00
diff --git 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<String, Method> 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<RefAddr> 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
@@ -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
@@ -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
@@ -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
@@ -107,6 +107,12 @@
 <section name="Tomcat 9.0.63 (remm)" rtext="in development">
   <subsection name="Catalina">
     <changelog>
+      <fix>
+        <bug>65736</bug>: Disable the <code>forceString</code> option for the
+        JNDI <code>BeanFactory</code> and replace it with an automatic search
+        for an alternative setter with the same name that accepts a
+        <code>String</code>. This is a security hardening measure. (markt)
+      </fix>
       <scode>
         <bug>65853</bug>: Refactor the <code>CsrfPreventionFilter</code> 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
@@ -328,103 +328,21 @@ writer.println("foo = " + bean.getFoo() + ", bar = " +
     <code>foo</code> property (although we could have), the bean will
     contain whatever default value is set up by its constructor.</p>
 
-    <p>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 <code>forceString</code> attribute.</p>
-
-    <p>Assume our bean looks like this:</p>
-
-<source><![CDATA[package com.mycompany;
-
-import java.net.InetAddress;
-import java.net.UnknownHostException;
-
-public class MyBean2 {
-
-  private InetAddress local = null;
-
-  public InetAddress getLocal() {
-    return local;
-  }
-
-  public void setLocal(InetAddress ip) {
-    local = ip;
-  }
-
-  public void setLocal(String localHost) {
-    try {
-      local = InetAddress.getByName(localHost);
-    } catch (UnknownHostException ex) {
-    }
-  }
-
-  private InetAddress remote = null;
-
-  public InetAddress getRemote() {
-    return remote;
-  }
-
-  public void setRemote(InetAddress ip) {
-    remote = ip;
-  }
-
-  public void host(String remoteHost) {
-    try {
-      remote = InetAddress.getByName(remoteHost);
-    } catch (UnknownHostException ex) {
-    }
-  }
-
-}]]></source>
-
-    <p>The bean has two properties, both are of type <code>InetAddress</code>.
-    The first property <code>local</code> 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 <code>InetAddress</code>.
-    We can tell the Tomcat BeanFactory to use the other setter like that:</p>
-
-<source><![CDATA[<Context ...>
-  ...
-  <Resource name="bean/MyBeanFactory" auth="Container"
-            type="com.mycompany.MyBean2"
-            factory="org.apache.naming.factory.BeanFactory"
-            forceString="local"
-            local="localhost"/>
-  ...
-</Context>]]></source>
-
-    <p>The bean property <code>remote</code> can also be set from a string,
-    but one has to use the non-standard method name <code>host</code>.
-    To set <code>local</code> and <code>remote</code> use the following
-    configuration:</p>
-
-<source><![CDATA[<Context ...>
-  ...
-  <Resource name="bean/MyBeanFactory" auth="Container"
-            type="com.mycompany.MyBean2"
-            factory="org.apache.naming.factory.BeanFactory"
-            forceString="local,remote=host"
-            local="localhost"
-            remote="tomcat.apache.org"/>
-  ...
-</Context>]]></source>
-
-    <p>Multiple property descriptions can be combined in
-    <code>forceString</code> 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 <code>name=method</code> in which case the property named
-    <code>name</code> is set by calling method <code>method</code>.
-    For properties of types <code>String</code> or of primitive type
-    or of their associated primitive wrapper classes using
-    <code>forceString</code> is not needed. The correct setter will be
-    automatically detected and argument conversion will be applied.</p>
-
+    <p>If the bean property is of type <code>String</code>, 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 <code>String</code>. If the bean provides an alternative setter with 
+    the same name that does take a <code>String</code>, 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.</p>
+    
+    <p>The <code>forceString</code> property available in earlier Tomcat
+    releases has been removed as a security hardening measure.</p>
+    
   </subsection>
 
 
