-
Notifications
You must be signed in to change notification settings - Fork 578
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Add new rule REFL_REFLECTION_INCREASES_ACCESSIBILITY_OF_CLASS (#1541)
* Add new rule REFL_REFLECTION_INCREASES_ACCESSIBILITY_OF_CLASS SEI CERT rule SEC05-J forbids the use of reflection to increase accessibility of classes, methods or fields. If a class in a package provides a public method which takes an instance of java.lang.Class as its parameter and calls its newInstance() method then it increases accessibility of classes in the same package without public constructors. An attacker code may call this method and pass such class to create an instance of it. This should be avoided by either making the method non-public or by invoking SecurityManager.checkPackageAccess on the package. A third possiblity is to use the java.beans.Beans.instantiate() method instead of java.lang.Class.newInstance() which checks whether the Class object being received has any public constructors. This patch adds a new detector to check this rule. * CHANGELOG.md updated * Updated according to a comment from @KengoTODA. Also added additional condition to check whether the class is public as well. * CHANGELOG fixed * Exclude fields from the objects considered as class-type arguments Because of field summaries some fields may also be initial parameters if they are initialized from constructor parameters. Let us exclude them! * Error message changed to REFLECTION_MAY_INCREASE_ACCESSIBILITY_OF_CLASS * Add the other part of the SEI CERT Rule: Fields The SEI CERT rule also mentions two uncompliant code pieces where accessibility of fields are increased. This patch extends the detector to also report these cases: REFL_REFLECTION_MAY_INCREASE_ACCESSIBILITY_OF_FIELD. * Bug codes separated and annotation added to the detector
- Loading branch information
Balogh, Ádám
committed
Nov 7, 2021
1 parent
3088491
commit 085339f
Showing
6 changed files
with
251 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
111 changes: 111 additions & 0 deletions
111
spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ReflectionIncreaseAccessibility.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| /* | ||
| * SpotBugs - Find bugs in Java programs | ||
| * | ||
| * This library is free software; you can redistribute it and/or | ||
| * modify it under the terms of the GNU Lesser General Public | ||
| * License as published by the Free Software Foundation; either | ||
| * version 2.1 of the License, or (at your option) any later version. | ||
| * | ||
| * This library is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
| * Lesser General Public License for more details. | ||
| * | ||
| * You should have received a copy of the GNU Lesser General Public | ||
| * License along with this library; if not, write to the Free Software | ||
| * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA | ||
| */ | ||
|
|
||
| package edu.umd.cs.findbugs.detect; | ||
|
|
||
| import org.apache.bcel.Const; | ||
| import org.apache.bcel.classfile.JavaClass; | ||
| import org.apache.bcel.classfile.Method; | ||
|
|
||
|
|
||
| import edu.umd.cs.findbugs.BugAccumulator; | ||
| import edu.umd.cs.findbugs.BugInstance; | ||
| import edu.umd.cs.findbugs.BugReporter; | ||
| import edu.umd.cs.findbugs.OpcodeStack; | ||
| import edu.umd.cs.findbugs.ba.AnalysisContext; | ||
| import edu.umd.cs.findbugs.ba.XMethod; | ||
| import edu.umd.cs.findbugs.bcel.OpcodeStackDetector; | ||
|
|
||
| @OpcodeStack.CustomUserValue | ||
| public class ReflectionIncreaseAccessibility extends OpcodeStackDetector { | ||
| private boolean securityCheck = false; | ||
| private boolean fieldNameUnderGet = false; | ||
|
|
||
| private final BugAccumulator bugAccumulator; | ||
|
|
||
| public ReflectionIncreaseAccessibility(BugReporter bugReporter) { | ||
| this.bugAccumulator = new BugAccumulator(bugReporter); | ||
| } | ||
|
|
||
| @Override | ||
| public void visit(Method met) { | ||
| securityCheck = false; | ||
| super.visit(met); | ||
| } | ||
|
|
||
| @Override | ||
| public void visitAfter(JavaClass obj) { | ||
| bugAccumulator.reportAccumulatedBugs(); | ||
| } | ||
|
|
||
| @Override | ||
| public void sawOpcode(int seen) { | ||
| fieldNameUnderGet = false; | ||
|
|
||
| if (seen != Const.INVOKEVIRTUAL || !getThisClass().isPublic() || !getMethod().isPublic()) { | ||
| return; | ||
| } | ||
|
|
||
| OpcodeStack.Item obj = stack.getItemMethodInvokedOn(this); | ||
| try { | ||
| JavaClass cls = obj.getJavaClass(); | ||
| if (cls == null) { | ||
| return; | ||
| } | ||
| XMethod met = getXMethodOperand(); | ||
| if (!securityCheck && obj.isInitialParameter() && obj.getXField() == null && | ||
| "java.lang.Class".equals(cls.getClassName()) && | ||
| "newInstance".equals(met.getName()) && | ||
| "()Ljava/lang/Object;".equals(met.getSignature())) { | ||
| bugAccumulator.accumulateBug(new BugInstance(this, | ||
| "REFLC_REFLECTION_MAY_INCREASE_ACCESSIBILITY_OF_CLASS", NORMAL_PRIORITY) | ||
| .addClassAndMethod(this), this); | ||
| } else if ("java.lang.Class".equals(cls.getClassName()) && | ||
| "getDeclaredField".equals(met.getName()) && | ||
| "(Ljava/lang/String;)Ljava/lang/reflect/Field;".equals(met.getSignature())) { | ||
| OpcodeStack.Item param = stack.getStackItem(0); | ||
| fieldNameUnderGet = param.isInitialParameter() && param.getXField() == null; | ||
| } else if (!securityCheck && "java.lang.reflect.Field".equals(cls.getClassName()) && | ||
| met.getName().startsWith("set")) { | ||
| Boolean fieldIsFromParam = (Boolean) obj.getUserValue(); | ||
| if (fieldIsFromParam != null && fieldIsFromParam.booleanValue()) { | ||
| bugAccumulator.accumulateBug(new BugInstance(this, | ||
| "REFLF_REFLECTION_MAY_INCREASE_ACCESSIBILITY_OF_FIELD", NORMAL_PRIORITY) | ||
| .addClassAndMethod(this), this); | ||
| } | ||
| } else if ("java.lang.SecurityManager".equals(cls.getClassName()) && | ||
| "checkPackageAccess".equals(met.getName()) && | ||
| "(Ljava/lang/String;)V".equals(met.getSignature()) && | ||
| getPackageName().equals((String) stack.getStackItem(0).getConstant())) { | ||
| securityCheck = true; | ||
| } | ||
| } catch (ClassNotFoundException e) { | ||
| AnalysisContext.reportMissingClass(e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void afterOpcode(int seen) { | ||
| super.afterOpcode(seen); | ||
|
|
||
| if (fieldNameUnderGet) { | ||
| stack.getStackItem(0).setUserValue(true); | ||
| } | ||
|
|
||
| } | ||
| } |
50 changes: 50 additions & 0 deletions
50
spotbugsTestCases/src/java/ReflectionIncreaseAccessibilityNegativeTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import java.util.ArrayList; | ||
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| import edu.umd.cs.findbugs.annotations.NoWarning; | ||
|
|
||
| public class ReflectionIncreaseAccessibilityNegativeTest { | ||
| private Class cls; | ||
|
|
||
| ReflectionIncreaseAccessibilityNegativeTest(Class cls) { | ||
| this.cls = cls; | ||
| } | ||
|
|
||
| @NoWarning("REFLC") | ||
| public static <T> T create(Class<T> c) | ||
| throws InstantiationException, IllegalAccessException { | ||
| if (c.getConstructors().length == 0) { | ||
| SecurityManager sm = System.getSecurityManager(); | ||
| if (sm != null) { | ||
| sm.checkPackageAccess(""); | ||
| } | ||
| } | ||
| return c.newInstance(); | ||
| } | ||
|
|
||
| private static class C { | ||
| Class<?> cls; | ||
| public C(Class<?> cls) { | ||
| this.cls = cls; | ||
| } | ||
| } | ||
|
|
||
| @NoWarning("REFLC") | ||
| public void strangeFP() | ||
| throws InstantiationException, IllegalAccessException { | ||
| cls.newInstance(); | ||
| } | ||
|
|
||
| private C getC() { | ||
| return new C(Integer.class); | ||
| } | ||
|
|
||
| @NoWarning("REFLC") | ||
| public void strangeFP2() | ||
| throws InstantiationException, IllegalAccessException { | ||
| C c = getC(); | ||
| c.cls.newInstance(); | ||
| } | ||
| } |
42 changes: 42 additions & 0 deletions
42
spotbugsTestCases/src/java/ReflectionIncreaseAccessibilityTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import java.lang.reflect.Field; | ||
|
|
||
| import edu.umd.cs.findbugs.annotations.ExpectWarning; | ||
|
|
||
| public class ReflectionIncreaseAccessibilityTest { | ||
| ReflectionIncreaseAccessibilityTest() {} | ||
|
|
||
| @ExpectWarning("REFLC") | ||
| public static <T> T create(Class<T> c) | ||
| throws InstantiationException, IllegalAccessException { | ||
| return c.newInstance(); | ||
| } | ||
|
|
||
| private int field; | ||
|
|
||
| @ExpectWarning("REFLF") | ||
| public void setField(String fieldName, int value) { | ||
| try { | ||
| Field f = this.getClass().getDeclaredField(fieldName); | ||
| f.setInt(this, value); | ||
| } catch (NoSuchFieldException e) { | ||
| e.printStackTrace(); | ||
| } catch (IllegalAccessException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } | ||
|
|
||
| @ExpectWarning("REFLF") | ||
| public Field getAccessibleField(String fieldName) { | ||
| try { | ||
| Field f = this.getClass().getDeclaredField(fieldName); | ||
| f.setAccessible(true); | ||
| return f; | ||
| } catch (NoSuchFieldException e) { | ||
| e.printStackTrace(); | ||
| return null; | ||
| } catch (SecurityException e) { | ||
| e.printStackTrace(); | ||
| return null; | ||
| } | ||
| } | ||
| } |