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:34:11 UTC
[tomcat] branch master 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 master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push:
new 3032e08 Security hardening. Avoid loading user specified classes.
3032e08 is described below
commit 3032e08baec63874fec292daf288c319719dfeac
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 d261873..1e3c7b4 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;
@@ -1163,21 +1164,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(sm.getString("managerServlet.error.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);
}
@@ -1188,27 +1199,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());
@@ -1224,7 +1231,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 82752e4..bb954fb 100644
--- a/java/org/apache/tomcat/util/IntrospectionUtils.java
+++ b/java/org/apache/tomcat/util/IntrospectionUtils.java
@@ -433,6 +433,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 1a9f224..80748b3 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -80,6 +80,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>
<subsection name="Coyote">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch master updated: Security hardening. Avoid loading
user specified classes.
Posted by Mark Thomas <ma...@apache.org>.
On 23/04/2019 14:44, Christopher Schultz wrote:
> Mark,
>
> On 4/18/19 14:34, markt@apache.org wrote:
>> This is an automated email from the ASF dual-hosted git
>> repository.
>
>> markt pushed a commit to branch master in repository
>> https://gitbox.apache.org/repos/asf/tomcat.git
>
>
>> The following commit(s) were added to refs/heads/master by this
>> push: new 3032e08 Security hardening. Avoid loading user specified
>> classes. 3032e08 is described below
>
>> commit 3032e08baec63874fec292daf288c319719dfeac 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.
>
> [...]
>
>> diff --git a/java/org/apache/tomcat/util/IntrospectionUtils.java
>> b/java/org/apache/tomcat/util/IntrospectionUtils.java index
>> 82752e4..bb954fb 100644 ---
>> a/java/org/apache/tomcat/util/IntrospectionUtils.java +++
>> b/java/org/apache/tomcat/util/IntrospectionUtils.java @@ -433,6
>> +433,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); + } + } +
>
> This method is basically Class.isAssignableFrom except that it works
> on a String argument and never instantiates the Class object it
> represents, right?
Correct.
> If so, I'll add a little Javadoc because when I has read the code
> (without apparently reading the "Detail" text of this patch), I
> thought "why not just call Class.isAssignableFrom instead of all this?
Thanks.
> Is there ever any problem with primitive values, or is that never an
> issue?
In this instance I think any primitive type will be boxed to the
corresponding object. More generally, I guess this might get used with
primitives at some point. I don't think I tested that.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [tomcat] branch master updated: Security hardening. Avoid loading
user specified classes.
Posted by Christopher Schultz <ch...@christopherschultz.net>.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
Mark,
On 4/18/19 14:34, markt@apache.org wrote:
> This is an automated email from the ASF dual-hosted git
> repository.
>
> markt pushed a commit to branch master in repository
> https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/master by this
> push: new 3032e08 Security hardening. Avoid loading user specified
> classes. 3032e08 is described below
>
> commit 3032e08baec63874fec292daf288c319719dfeac 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.
[...]
> diff --git a/java/org/apache/tomcat/util/IntrospectionUtils.java
> b/java/org/apache/tomcat/util/IntrospectionUtils.java index
> 82752e4..bb954fb 100644 ---
> a/java/org/apache/tomcat/util/IntrospectionUtils.java +++
> b/java/org/apache/tomcat/util/IntrospectionUtils.java @@ -433,6
> +433,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); + } + } +
This method is basically Class.isAssignableFrom except that it works
on a String argument and never instantiates the Class object it
represents, right?
If so, I'll add a little Javadoc because when I has read the code
(without apparently reading the "Detail" text of this patch), I
thought "why not just call Class.isAssignableFrom instead of all this?
Is there ever any problem with primitive values, or is that never an
issue?
- -chris
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with Thunderbird - https://www.enigmail.net/
iQIzBAEBCAAdFiEEMmKgYcQvxMe7tcJcHPApP6U8pFgFAly/FtIACgkQHPApP6U8
pFjJYw/+Lw6As1tmjIhNI8o6F26RILrg6j3hFK6BfPC+Sq2vSuDXpeAwg74d641e
rpf/RzSpt08AYDU7cEFaALGJX1+FoC/jOD5/kfQvt3blaKn+fteFXAl5w4vX6Ato
y30jy4H0drdAATcHjFyvIlVD9U1YRra345kmN8ARHLkb3O8aSGYrBvWfPbe7vzLv
iEkGXBp/CJj/H1Vq8KWszJ1kNtbue8HQx5NhF9UvLIWKwZQzpDHrYtBr9uyaai46
twD535AqhhjyStLqgG1+Dgq4jMjHOyABD2NY2bH0KQBbbzzJhZTLJjQuKR8HweeK
U+DoypqKh606vAMmTVdc5X/6IjyxRFoXTlhGheYpQjeESLBFDKfPW1ZnV6k0BhFC
kPXEJJAYZRRONaEocumWjmQ1d7nTqStfRvzLUTWwkdf7UnGheSgc7rhxLS2pFRPe
il3cG1JRVxECa7GzagnTu1o+0QecaUxZKW0KNUFd9zo9l2ohvuHRwG5D96+3/CDM
uHAD8T5Xpk7zkSRZ/UbympZAGjXz/trh6Srxo6Upi9HtOtN4YrsKBtWPQvWitPBg
fba7r6aRGrtussjoe9FG+Gkt4vG1Mm/uhRd5gPBzUuqBK/FXIJ0jHOC/Jl8IqBq0
Rx0ptB0BQLn8BvQZTEkSPGoCFVWsAnxXdhJptPNUJTV3BPI5RBs=
=alEg
-----END PGP SIGNATURE-----
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org