From f626da75fd59da82b14dee7b8cc46ad51eefdbe5 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Thu, 21 Jan 2016 11:56:27 +0000 Subject: [PATCH] When using the new sessionAttributeValueClassNameFilter, apply the filter earlier rather than loading the class and then deciding to filter it out. When a SecurityManager is used, enable filtering by default. git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1725914 13f79535-47bb-0310-9956-ffa450edef68 --- .../apache/catalina/session/ManagerBase.java | 16 +++- .../catalina/session/StandardManager.java | 8 +- .../apache/catalina/session/StoreBase.java | 15 +++- .../util/CustomObjectInputStream.java | 89 ++++++++++++++++++- .../catalina/util/LocalStrings.properties | 2 + webapps/docs/changelog.xml | 3 +- webapps/docs/config/cluster-manager.xml | 14 ++- webapps/docs/config/manager.xml | 14 ++- 8 files changed, 146 insertions(+), 15 deletions(-) diff --git a/java/org/apache/catalina/session/ManagerBase.java b/java/org/apache/catalina/session/ManagerBase.java index afd5101ac34b..d191064d86f5 100644 --- a/java/org/apache/catalina/session/ManagerBase.java +++ b/java/org/apache/catalina/session/ManagerBase.java @@ -37,6 +37,7 @@ import org.apache.catalina.Container; import org.apache.catalina.Context; import org.apache.catalina.Engine; +import org.apache.catalina.Globals; import org.apache.catalina.Lifecycle; import org.apache.catalina.LifecycleException; import org.apache.catalina.LifecycleState; @@ -193,7 +194,20 @@ public abstract class ManagerBase extends LifecycleMBeanBase implements Manager private boolean warnOnSessionAttributeFilterFailure; - // ------------------------------------------------------------- Properties + // ------------------------------------------------------------ Constructors + + public ManagerBase() { + if (Globals.IS_SECURITY_ENABLED) { + // Minimum set required for default distribution/persistence to work + // plus String + setSessionAttributeValueClassNameFilter( + "java\\.lang\\.(?:Boolean|Integer|Long|Number|String)"); + setWarnOnSessionAttributeFilterFailure(true); + } + } + + + // -------------------------------------------------------------- Properties /** * Obtain the regular expression used to filter session attribute based on diff --git a/java/org/apache/catalina/session/StandardManager.java b/java/org/apache/catalina/session/StandardManager.java index 1decb5a8526c..2195886009b9 100644 --- a/java/org/apache/catalina/session/StandardManager.java +++ b/java/org/apache/catalina/session/StandardManager.java @@ -191,10 +191,12 @@ protected void doLoad() throws ClassNotFoundException, IOException { } Loader loader = null; ClassLoader classLoader = null; + Log logger = null; try (FileInputStream fis = new FileInputStream(file.getAbsolutePath()); - BufferedInputStream bis = new BufferedInputStream(fis);) { + BufferedInputStream bis = new BufferedInputStream(fis)) { Context c = getContext(); loader = c.getLoader(); + logger = c.getLogger(); if (loader != null) { classLoader = loader.getClassLoader(); } @@ -204,7 +206,9 @@ protected void doLoad() throws ClassNotFoundException, IOException { // Load the previously unloaded active sessions synchronized (sessions) { - try (ObjectInputStream ois = new CustomObjectInputStream(bis, classLoader)) { + try (ObjectInputStream ois = new CustomObjectInputStream(bis, classLoader, logger, + getSessionAttributeValueClassNamePattern(), + getWarnOnSessionAttributeFilterFailure())) { Integer count = (Integer) ois.readObject(); int n = count.intValue(); if (log.isDebugEnabled()) diff --git a/java/org/apache/catalina/session/StoreBase.java b/java/org/apache/catalina/session/StoreBase.java index 679ac983be43..09c88e01db00 100644 --- a/java/org/apache/catalina/session/StoreBase.java +++ b/java/org/apache/catalina/session/StoreBase.java @@ -215,7 +215,20 @@ public void processExpires() { */ protected ObjectInputStream getObjectInputStream(InputStream is) throws IOException { BufferedInputStream bis = new BufferedInputStream(is); - return new CustomObjectInputStream(bis, Thread.currentThread().getContextClassLoader()); + + CustomObjectInputStream ois; + ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); + + if (manager instanceof ManagerBase) { + ManagerBase managerBase = (ManagerBase) manager; + ois = new CustomObjectInputStream(bis, classLoader, manager.getContext().getLogger(), + managerBase.getSessionAttributeValueClassNamePattern(), + managerBase.getWarnOnSessionAttributeFilterFailure()); + } else { + ois = new CustomObjectInputStream(bis, classLoader); + } + + return ois; } diff --git a/java/org/apache/catalina/util/CustomObjectInputStream.java b/java/org/apache/catalina/util/CustomObjectInputStream.java index b6ec5f88adc5..911d499addb2 100644 --- a/java/org/apache/catalina/util/CustomObjectInputStream.java +++ b/java/org/apache/catalina/util/CustomObjectInputStream.java @@ -18,9 +18,18 @@ import java.io.IOException; import java.io.InputStream; +import java.io.InvalidClassException; import java.io.ObjectInputStream; import java.io.ObjectStreamClass; import java.lang.reflect.Proxy; +import java.util.Collections; +import java.util.Set; +import java.util.WeakHashMap; +import java.util.concurrent.ConcurrentHashMap; +import java.util.regex.Pattern; + +import org.apache.juli.logging.Log; +import org.apache.tomcat.util.res.StringManager; /** * Custom subclass of ObjectInputStream that loads from the @@ -32,14 +41,26 @@ */ public final class CustomObjectInputStream extends ObjectInputStream { + private static final StringManager sm = StringManager.getManager(CustomObjectInputStream.class); + + private static final WeakHashMap> reportedClassCache = + new WeakHashMap<>(); + /** * The class loader we will use to resolve classes. */ private final ClassLoader classLoader; + private final Set reportedClasses; + private final Log log; + + private final Pattern allowedClassNamePattern; + private final String allowedClassNameFilter; + private final boolean warnOnFailure; /** - * Construct a new instance of CustomObjectInputStream + * Construct a new instance of CustomObjectInputStream without any filtering + * of deserialized classes. * * @param stream The input stream we will read from * @param classLoader The class loader used to instantiate objects @@ -47,8 +68,56 @@ public final class CustomObjectInputStream extends ObjectInputStream { * @exception IOException if an input/output error occurs */ public CustomObjectInputStream(InputStream stream, ClassLoader classLoader) throws IOException { + this(stream, classLoader, null, null, false); + } + + + /** + * Construct a new instance of CustomObjectInputStream with filtering of + * deserialized classes. + * + * @param stream The input stream we will read from + * @param classLoader The class loader used to instantiate objects + * @param log The logger to use to report any issues. It may only be null if + * the filterMode does not require logging + * @param allowedClassNamePattern The regular expression to use to filter + * deserialized classes. The fully qualified + * class name must match this pattern for + * deserialization to be allowed if filtering + * is enabled. + * @param warnOnFailure Should any failures be logged? + * + * @exception IOException if an input/output error occurs + */ + public CustomObjectInputStream(InputStream stream, ClassLoader classLoader, + Log log, Pattern allowedClassNamePattern, boolean warnOnFailure) + throws IOException { super(stream); + if (log == null && allowedClassNamePattern != null && warnOnFailure) { + throw new IllegalArgumentException( + sm.getString("customObjectInputStream.logRequired")); + } this.classLoader = classLoader; + this.log = log; + this.allowedClassNamePattern = allowedClassNamePattern; + if (allowedClassNamePattern == null) { + this.allowedClassNameFilter = null; + } else { + this.allowedClassNameFilter = allowedClassNamePattern.toString(); + } + this.warnOnFailure = warnOnFailure; + + Set reportedClasses = reportedClassCache.get(classLoader); + if (reportedClasses == null) { + reportedClasses = Collections.newSetFromMap(new ConcurrentHashMap<>()); + Set original = reportedClassCache.putIfAbsent(classLoader, reportedClasses); + if (original != null) { + // Concurrent attempts to create the new Set. Make sure all + // threads use the first successfully added Set. + reportedClasses = original; + } + } + this.reportedClasses = reportedClasses; } @@ -64,8 +133,24 @@ public CustomObjectInputStream(InputStream stream, ClassLoader classLoader) thro @Override public Class resolveClass(ObjectStreamClass classDesc) throws ClassNotFoundException, IOException { + + String name = classDesc.getName(); + if (allowedClassNamePattern != null) { + boolean allowed = allowedClassNamePattern.matcher(name).matches(); + if (!allowed) { + boolean doLog = warnOnFailure && reportedClasses.add(name); + String msg = sm.getString("customObjectInputStream.nomatch", name, allowedClassNameFilter); + if (doLog) { + log.warn(msg); + } else if (log.isDebugEnabled()) { + log.debug(msg); + } + throw new InvalidClassException(msg); + } + } + try { - return Class.forName(classDesc.getName(), false, classLoader); + return Class.forName(name, false, classLoader); } catch (ClassNotFoundException e) { try { // Try also the superclass because of primitive types diff --git a/java/org/apache/catalina/util/LocalStrings.properties b/java/org/apache/catalina/util/LocalStrings.properties index e64b741d6dcb..7dbb399ebf02 100644 --- a/java/org/apache/catalina/util/LocalStrings.properties +++ b/java/org/apache/catalina/util/LocalStrings.properties @@ -17,6 +17,8 @@ parameterMap.locked=No modifications are allowed to a locked ParameterMap resourceSet.locked=No modifications are allowed to a locked ResourceSet hexUtil.bad=Bad hexadecimal digit hexUtil.odd=Odd number of hexadecimal digits +customObjectInputStream.logRequired=A valid logger is required for class name filtering with logging +customObjectInputStream.nomatch=The class [{0}] did not match the regular expression [{1}] for classes allowed to be deserialized #Default Messages Utilized by the ExtensionValidator extensionValidator.web-application-manifest=Web Application Manifest extensionValidator.extension-not-found-error=ExtensionValidator[{0}][{1}]: Required extension [{2}] not found. diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index ba29da2ba864..982193450a0c 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -212,7 +212,8 @@ based on the implementation class of the value and optional WARN level logging if an attribute is filtered. These options are avaialble for all of the Manager implementations that ship - with Tomcat. (markt) + with Tomcat. When a SecurityManager is used filtering will + be enabled by default. (markt) Remove distributable and maxInactiveInterval diff --git a/webapps/docs/config/cluster-manager.xml b/webapps/docs/config/cluster-manager.xml index 055c65f6f38e..cbcc24a713fa 100644 --- a/webapps/docs/config/cluster-manager.xml +++ b/webapps/docs/config/cluster-manager.xml @@ -180,7 +180,9 @@ length or null, all attributes are eligible for replication. The pattern is anchored so the fully qualified class name must fully match the pattern. If not specified, the default value of - null will be used.

+ null will be used unless a SecurityManager is + enabled in which case the default will be + java\\.lang\\.(?:Boolean|Integer|Long|Number|String).

When this node sends a GET_ALL_SESSIONS message to other @@ -199,7 +201,8 @@ attribute, should this be logged at WARN level? If WARN level logging is disabled then it will be logged at DEBUG. The default value of this attribute is - false.

+ false unless a SecurityManager is enabled in + which case the default will be true.

 @@ -242,7 +245,9 @@ length or null, all attributes are eligible for replication. The pattern is anchored so the fully qualified class name must fully match the pattern. If not specified, the default value of - null will be used.

+ null will be used unless a SecurityManager is + enabled in which case the default will be + java\\.lang\\.(?:Boolean|Integer|Long|Number|String).

Set to true if you wish to terminate replication map when replication @@ -257,7 +262,8 @@ attribute, should this be logged at WARN level? If WARN level logging is disabled then it will be logged at DEBUG. The default value of this attribute is - false.

+ false unless a SecurityManager is enabled in + which case the default will be true.

 diff --git a/webapps/docs/config/manager.xml b/webapps/docs/config/manager.xml index 086c95ca71dc..0805561577b4 100644 --- a/webapps/docs/config/manager.xml +++ b/webapps/docs/config/manager.xml @@ -159,7 +159,9 @@ length or null, all attributes are eligible for distribution. The pattern is anchored so the fully qualified class name must fully match the pattern. If not specified, the default value of - null will be used.

+ null will be used unless a SecurityManager is + enabled in which case the default will be + java\\.lang\\.(?:Boolean|Integer|Long|Number|String).

@@ -168,7 +170,8 @@ attribute, should this be logged at WARN level? If WARN level logging is disabled then it will be logged at DEBUG. The default value of this attribute is - false.

+ false unless a SecurityManager is enabled in + which case the default will be true.

 @@ -279,7 +282,9 @@ length or null, all attributes are eligible for distribution. The pattern is anchored so the fully qualified class name must fully match the pattern. If not specified, the default value of - null will be used.

+ null will be used unless a SecurityManager is + enabled in which case the default will be + java\\.lang\\.(?:Boolean|Integer|Long|Number|String).

@@ -288,7 +293,8 @@ attribute, should this be logged at WARN level? If WARN level logging is disabled then it will be logged at DEBUG. The default value of this attribute is - false.

+ false unless a SecurityManager is enabled in + which case the default will be true.