You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/04/18 18:36:06 UTC
[tomcat] branch 8.5.x updated: Security hardening. Avoid loading
user specified classes.
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push:
new 2e23cb4 Security hardening. Avoid loading user specified classes.
2e23cb4 is described below
commit 2e23cb4221e828f1f462a6b4a58f96762fd4a5e7
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Apr 18 19:32:57 2019 +0100
Security hardening. Avoid loading user specified classes.
The user is trusted so loading a user specified class should be safe but
given it isn't necessary to load the class, avoid doing so. Reported by
Coverity scan.
---
.../apache/catalina/manager/ManagerServlet.java | 56 ++++++-----
.../org/apache/tomcat/util/IntrospectionUtils.java | 22 ++++
.../apache/tomcat/util/TestIntrospectionUtils.java | 112 +++++++++++++++++++++
webapps/docs/changelog.xml | 4 +
4 files changed, 169 insertions(+), 25 deletions(-)
diff --git a/java/org/apache/catalina/manager/ManagerServlet.java b/java/org/apache/catalina/manager/ManagerServlet.java
index dc22ae5..3174cb5 100644
--- a/java/org/apache/catalina/manager/ManagerServlet.java
+++ b/java/org/apache/catalina/manager/ManagerServlet.java
@@ -66,6 +66,7 @@ import org.apache.coyote.ProtocolHandler;
import org.apache.coyote.http11.AbstractHttp11Protocol;
import org.apache.tomcat.util.Diagnostics;
import org.apache.tomcat.util.ExceptionUtils;
+import org.apache.tomcat.util.IntrospectionUtils;
import org.apache.tomcat.util.modeler.Registry;
import org.apache.tomcat.util.net.SSLContext;
import org.apache.tomcat.util.net.SSLHostConfig;
@@ -1131,21 +1132,31 @@ public class ManagerServlet extends HttpServlet implements ContainerServlet {
writer.println(smClient.getString("managerServlet.resourcesAll"));
}
- Class<?> clazz = null;
- try {
- if (type != null) {
- clazz = Class.forName(type);
- }
- } catch (Throwable t) {
- ExceptionUtils.handleThrowable(t);
- log("ManagerServlet.resources[" + type + "]", t);
- writer.println(smClient.getString("managerServlet.exception",
- t.toString()));
- return;
- }
+ printResources(writer, "", global, type, smClient);
+
+ }
- printResources(writer, "", global, type, clazz, smClient);
+ /**
+ * List the resources of the given context.
+ *
+ * @param writer Writer to render to
+ * @param prefix Path for recursion
+ * @param namingContext The naming context for lookups
+ * @param type Fully qualified class name of the resource type of interest,
+ * or <code>null</code> to list resources of all types
+ * @param clazz Unused
+ * @param smClient i18n support for current client's locale
+ *
+ * @deprecated Use {@link #printResources(PrintWriter, String,
+ * javax.naming.Context, String, StringManager)}
+ * This method will be removed in Tomcat 10.x onwards
+ */
+ @Deprecated
+ protected void printResources(PrintWriter writer, String prefix,
+ javax.naming.Context namingContext,
+ String type, Class<?> clazz, StringManager smClient) {
+ printResources(writer, prefix, namingContext, type, smClient);
}
@@ -1156,27 +1167,23 @@ public class ManagerServlet extends HttpServlet implements ContainerServlet {
* @param namingContext The naming context for lookups
* @param type Fully qualified class name of the resource type of interest,
* or <code>null</code> to list resources of all types
- * @param clazz The resource class or <code>null</code> to list
- * resources of all types
* @param smClient i18n support for current client's locale
*/
protected void printResources(PrintWriter writer, String prefix,
javax.naming.Context namingContext,
- String type, Class<?> clazz,
+ String type,
StringManager smClient) {
-
try {
NamingEnumeration<Binding> items = namingContext.listBindings("");
while (items.hasMore()) {
Binding item = items.next();
- if (item.getObject() instanceof javax.naming.Context) {
- printResources
- (writer, prefix + item.getName() + "/",
- (javax.naming.Context) item.getObject(), type, clazz,
- smClient);
+ Object obj = item.getObject();
+ if (obj instanceof javax.naming.Context) {
+ printResources(writer, prefix + item.getName() + "/",
+ (javax.naming.Context) obj, type, smClient);
} else {
- if ((clazz != null) &&
- (!(clazz.isInstance(item.getObject())))) {
+ if (type != null && (obj == null ||
+ !IntrospectionUtils.isInstance(obj.getClass(), type))) {
continue;
}
writer.print(prefix + item.getName());
@@ -1192,7 +1199,6 @@ public class ManagerServlet extends HttpServlet implements ContainerServlet {
writer.println(smClient.getString("managerServlet.exception",
t.toString()));
}
-
}
diff --git a/java/org/apache/tomcat/util/IntrospectionUtils.java b/java/org/apache/tomcat/util/IntrospectionUtils.java
index aa96446..e3d6c76 100644
--- a/java/org/apache/tomcat/util/IntrospectionUtils.java
+++ b/java/org/apache/tomcat/util/IntrospectionUtils.java
@@ -452,6 +452,28 @@ public final class IntrospectionUtils {
return result;
}
+
+ public static boolean isInstance(Class<?> clazz, String type) {
+ if (type.equals(clazz.getName())) {
+ return true;
+ }
+
+ Class<?>[] ifaces = clazz.getInterfaces();
+ for (Class<?> iface : ifaces) {
+ if (isInstance(iface, type)) {
+ return true;
+ }
+ }
+
+ Class<?> superClazz = clazz.getSuperclass();
+ if (superClazz == null) {
+ return false;
+ } else {
+ return isInstance(superClazz, type);
+ }
+ }
+
+
// -------------------- Get property --------------------
// This provides a layer of abstraction
diff --git a/test/org/apache/tomcat/util/TestIntrospectionUtils.java b/test/org/apache/tomcat/util/TestIntrospectionUtils.java
new file mode 100644
index 0000000..64864f6
--- /dev/null
+++ b/test/org/apache/tomcat/util/TestIntrospectionUtils.java
@@ -0,0 +1,112 @@
+/*
+ * 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.tomcat.util;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+import org.apache.catalina.core.StandardContext;
+
+public class TestIntrospectionUtils {
+
+ // Test for all the classes and interfaces in StandardContext's type hierarchy
+
+ @Test
+ public void testIsInstanceStandardContext01() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "org.apache.catalina.core.StandardContext"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext02() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "org.apache.catalina.util.LifecycleMBeanBase"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext03() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "org.apache.catalina.util.LifecycleBase"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext04() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "java.lang.Object"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext05() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "org.apache.catalina.Lifecycle"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext06() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "org.apache.catalina.JmxEnabled"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext07() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "javax.management.MBeanRegistration"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext08() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "org.apache.catalina.Container"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext09() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "org.apache.tomcat.ContextBind"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext10() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "javax.management.NotificationEmitter"));
+ }
+
+
+ @Test
+ public void testIsInstanceStandardContext11() {
+ Assert.assertTrue(IntrospectionUtils.isInstance(
+ StandardContext.class, "javax.management.NotificationBroadcaster"));
+ }
+
+ // And one to check that non-matches return false
+
+
+ @Test
+ public void testIsInstanceStandardContext12() {
+ Assert.assertFalse(IntrospectionUtils.isInstance(
+ StandardContext.class, "com.example.Other"));
+ }
+}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 3354ad8..b8a0685 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -75,6 +75,10 @@
Fix a potential resource leak when a JNDI lookup returns an object of an
in compatible class. Identified by Coverity scan. (markt)
</fix>
+ <scode>
+ Refactor <code>ManagerServlet</code> to avoid loading classes when
+ filtering JNDI resources for resources of a specified type. (markt)
+ </scode>
</changelog>
</subsection>
</section>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org