Skip to content

Commit

Permalink
Create class to share logic between ErrorProne checkers
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 540100213
  • Loading branch information
java-team-github-bot authored and Error Prone Team committed Aug 1, 2023
1 parent c603793 commit aa8396e
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 38 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Copyright 2023 The Error Prone Authors.
*
* Licensed 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 com.google.errorprone.bugpatterns;

import com.google.errorprone.VisitorState;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.sun.source.tree.Tree;

/** An abstract class that detects use of the unsafe APIs. */
public abstract class AbstractBanUnsafeAPIChecker extends BugChecker {

protected <T extends Tree> Description matchHelper(
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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,17 @@
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
public final class BanClassLoader extends AbstractBanUnsafeAPIChecker
implements MethodInvocationTreeMatcher, NewClassTreeMatcher, ClassTreeMatcher {

private static final Matcher<ExpressionTree> METHOD_MATCHER =
private static final Matcher<MethodInvocationTree> METHOD_MATCHER =
anyOf(
anyMethod().onDescendantOf("java.lang.ClassLoader").named("defineClass"),
anyMethod().onDescendantOf("java.lang.invoke.MethodHandles.Lookup").named("defineClass"),
Expand All @@ -63,28 +62,18 @@ public final class BanClassLoader extends BugChecker
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);
return matchHelper(tree, state, METHOD_MATCHER);
}

@Override
public Description matchNewClass(NewClassTree tree, VisitorState state) {
return matchWith(tree, state, CONSTRUCTOR_MATCHER);
return matchHelper(tree, state, CONSTRUCTOR_MATCHER);
}

@Override
public Description matchClass(ClassTree tree, VisitorState state) {
return matchWith(tree, state, EXTEND_CLASS_MATCHER);
return matchHelper(tree, state, EXTEND_CLASS_MATCHER);
}
}
14 changes: 4 additions & 10 deletions core/src/main/java/com/google/errorprone/bugpatterns/BanJNDI.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;

/** A {@link BugChecker} that detects use of the unsafe JNDI API system. */
Expand All @@ -34,10 +33,11 @@
"Using JNDI may deserialize user input via the `Serializable` API which is extremely"
+ " dangerous",
severity = SeverityLevel.ERROR)
public final class BanJNDI extends BugChecker implements MethodInvocationTreeMatcher {
public final class BanJNDI extends AbstractBanUnsafeAPIChecker
implements MethodInvocationTreeMatcher {

/** Checks for direct or indirect calls to context.lookup() via the JDK */
private static final Matcher<ExpressionTree> MATCHER =
private static final Matcher<MethodInvocationTree> MATCHER =
anyOf(
anyMethod()
.onDescendantOf("javax.naming.directory.DirContext")
Expand Down Expand Up @@ -70,12 +70,6 @@ public final class BanJNDI extends BugChecker implements MethodInvocationTreeMat

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (state.errorProneOptions().isTestOnlyTarget() || !MATCHER.matches(tree, state)) {
return Description.NO_MATCH;
}

Description.Builder description = buildDescription(tree);

return description.build();
return this.matchHelper(tree, state, MATCHER);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,16 @@
import com.google.errorprone.bugpatterns.BugChecker.MethodInvocationTreeMatcher;
import com.google.errorprone.matchers.Description;
import com.google.errorprone.matchers.Matcher;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.MethodInvocationTree;

/** A {@link BugChecker} that detects use of the unsafe {@link java.io.Serializable} API. */
@BugPattern(
summary = "Deserializing user input via the `Serializable` API is extremely dangerous",
severity = SeverityLevel.ERROR)
public final class BanSerializableRead extends BugChecker implements MethodInvocationTreeMatcher {
public final class BanSerializableRead extends AbstractBanUnsafeAPIChecker
implements MethodInvocationTreeMatcher {

private static final Matcher<ExpressionTree> EXEMPT =
private static final Matcher<MethodInvocationTree> EXEMPT =
anyOf(
// This is called through ObjectInputStream; a call further up the callstack will have
// been exempt.
Expand All @@ -56,7 +56,7 @@ public final class BanSerializableRead extends BugChecker implements MethodInvoc
methodTree.getName().toString()))));

/** Checks for unsafe deserialization calls on an ObjectInputStream in an ExpressionTree. */
private static final Matcher<ExpressionTree> OBJECT_INPUT_STREAM_DESERIALIZE_MATCHER =
private static final Matcher<MethodInvocationTree> OBJECT_INPUT_STREAM_DESERIALIZE_MATCHER =
allOf(
anyOf(
// this matches calls to the ObjectInputStream to read some objects
Expand Down Expand Up @@ -91,16 +91,11 @@ public final class BanSerializableRead extends BugChecker implements MethodInvoc
not(EXEMPT));

/** Checks for unsafe uses of the Java deserialization API. */
private static final Matcher<ExpressionTree> MATCHER = OBJECT_INPUT_STREAM_DESERIALIZE_MATCHER;
private static final Matcher<MethodInvocationTree> MATCHER =
OBJECT_INPUT_STREAM_DESERIALIZE_MATCHER;

@Override
public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) {
if (state.errorProneOptions().isTestOnlyTarget() || !MATCHER.matches(tree, state)) {
return Description.NO_MATCH;
}

Description.Builder description = buildDescription(tree);

return description.build();
return this.matchHelper(tree, state, MATCHER);
}
}

0 comments on commit aa8396e

Please sign in to comment.