Add new rule REFL_REFLECTION_INCREASES_ACCESSIBILITY_OF_CLASS #1541

baloghadamsoftware · 2021-05-07T13:31:45Z

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.

Change-Id: I1881502512848f8a04b7ee6be95539f5fe88c6b3

Make sure these boxes are checked before submitting your PR -- thank you!

Added an entry into CHANGELOG.md if you have changed SpotBugs code

ThrawnCA · 2021-05-09T22:56:25Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ReflectionIncreaseAccessibility.java

+                    securityCheck = true;
+                }
+            } catch (ClassNotFoundException e) {
+            }


There probably should be at least a comment here explaining why we take no action.

ThrawnCA · 2021-05-09T22:57:27Z

spotbugs/etc/messages.xml

+        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


The security manager is being deprecated and will be removed (see JEP 411).

ThrawnCA · 2021-05-11T00:18:26Z

spotbugs/etc/messages.xml

-        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.
+        be avoided by either making the method non-public or by checking for package access permission on the package.
+        A third possiblity is to use the java.beans.Beans.instantiate() method instead of java.lang.Class.newInstance()


Misspelled "possibility"

baloghadamsoftware · 2021-06-02T12:56:08Z

Sorry for force pushing but there was a git mess which I had to sort out.

baloghadamsoftware · 2021-06-08T13:00:31Z

We tested this on some open-source projects and found some true positives. (I mean that they are violations of the SEI CERT rule.)

MATSim
NanoHTTPD

On the rest we did not found anything. Thus no false positives at all and no crashes:
SpotBugs itself
Jenkins
Bt
Ttorrent

ThrawnCA · 2021-06-30T03:42:54Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ReflectionIncreaseAccessibility.java

@@ -0,0 +1,82 @@
+/*
+ * FindBugs - Find bugs in Java programs


New files should use a SpotBugs copyright notice, not FindBugs.

CHANGELOG.md

commit 54e0306 Author: Ádám Balogh <adam.balogh@ericsson.com> Date: Wed Jun 30 20:04:00 2021 +0200 Comment header updated from FindBugs to SpotBugs commit 34430ab Author: Ádám Balogh <adam.balogh@ericsson.com> Date: Fri May 14 11:18:04 2021 +0200 Nullness check for returned JavaClass instance added commit 0b10681 Author: Ádám Balogh <adam.balogh@ericsson.com> Date: Fri May 14 11:03:56 2021 +0200 Fixed typo in messages.xml commit d9ae166 Author: Ádám Balogh <adam.balogh@ericsson.com> Date: Mon May 10 13:07:45 2021 +0200 Added report for missing class and long description rephrased commit 2c5aa45 Author: Ádám Balogh <adam.balogh@ericsson.com> Date: Fri May 7 15:25:20 2021 +0200 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.

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.

baloghadamsoftware · 2021-07-07T14:46:13Z

@KengoTODA Rebasing done, also moved line in CHANGELOG.md to ###Unreleased.

baloghadamsoftware · 2021-09-08T13:54:25Z

@KengoTODA It seems that you approved this one. Are there any obstacles in merging it?

KengoTODA · 2021-09-13T08:46:18Z

@baloghadamsoftware thanks for reminding me. Could you merge origin/master again and move your CHANGELOG entry to ## Unreleased?

baloghadamsoftware · 2021-10-12T15:39:34Z

@KengoTODA?

KengoTODA

Current implementation catches just limited cases and will trigger false positives (expected reflection access). We need a more complicated analysis to make this detector productive.

And please merge the latest master branch, to resolve conflict in the CHANGELOG.md.

KengoTODA · 2021-10-25T10:03:59Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ReflectionIncreaseAccessibility.java

+
+    @Override
+    public void sawOpcode(int seen) {
+        if (seen == Const.INVOKEVIRTUAL) {


Better to exit method to reduce nest, so I suggest moving securityCheck confirmation to here like below:

if (securityCheck || seen != Const.INVOKEVIRTUAL) { return; }

KengoTODA · 2021-10-25T10:21:13Z

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ReflectionIncreaseAccessibility.java

+                if (!securityCheck && obj.isInitialParameter() && "java.lang.Class".equals(cls.getClassName()) &&
+                        "newInstance".equals(met.getName()) && "()Ljava/lang/Object;".equals(met.getSignature()) &&
+                        getMethod().isPublic()) {


I feel these conditions are not so suitable to detect the target defect. What we need to avoid is increasing accessibility, but this method does not check the accessibility of the constructor and the target class.

… condition to check whether the class is public as well.

baloghadamsoftware · 2021-10-25T11:14:24Z

Current implementation catches just limited cases and will trigger false positives (expected reflection access). We need a more complicated analysis to make this detector productive.

And please merge the latest master branch, to resolve conflict in the CHANGELOG.md.

Thank you for taking you time to do a second review on this PR. I merged master into this PR and also fixed the CHANGELOG. Furthermore, I made the change in the condition check you requested and added an additional condition to work only on public classes.

You are completely right, this detector is only the first step, it only catches the second non-compliant example described in the SEI-CERT rule. I originally planned to add the rest in a subsequent PR. Should I do it now, in this particular one?

I tested this on several open-source projects, as you can see above and could not find any false positives, Could you suggest me an example project where we get the false positive you mentioned?

What more complex analysis do you suggest? Actually, a public method in a public class can be called by anyone, even outside the package. If such a method takes a class as its parameter and instantiates it without any security check then we can call it a vulnerability because anyone can pass it a class that is not meant to be instantiated by any other class, just some special classes. We cannot check the actual "increase" because we must be able to detect the vulnerability itself, without the attacker code being present. Thus at the time of the analysis we are not aware of the actual parameter passed to the method. Maybe we should change the error message to contain the "potential" word?

baloghadamsoftware · 2021-10-25T13:11:35Z

Hmm, maybe REFL_REFLECTION_MAY_INCREASE_ACCESSIBILITY_OF_CLASS?

Because of field summaries some fields may also be initial parameters if they are initialized from constructor parameters. Let us exclude them!

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.

baloghadamsoftware · 2021-10-27T15:25:45Z

We tested this on some open-source projects and found some true positives. (I mean that they are violations of the SEI CERT rule.)

MATSim
NanoHTTPD

One out of the three findings on MATSim-Libs and one out of the two findings on NanoHTTPD were actually false positives beause isInitialParameter on OpcodeStack.Item returned true on fields which where initialized by a constructor. Now they are excluded so we only have two true positives on MATSim-Libs and one true positive on NanoHTTPD. No true or false positives on the rest of the projects.

KengoTODA · 2021-10-31T04:51:21Z

spotbugs/etc/messages.xml

@@ -8676,4 +8717,5 @@ after the call to initLogging, the logger configuration is lost
  <BugCode abbrev="DL">Unintended contention or possible deadlock due to locking on shared objects</BugCode>
  <BugCode abbrev="JUA">Problems in JUnit Assertions</BugCode>
  <BugCode abbrev="EOS">Bad End of Stream check</BugCode>
+  <BugCode abbrev="REFL">Reflection increasing accessibility of classes, methods and fields</BugCode>


Better to add another BugCode element even in the findbugs.xml

Do you mean that I should use two different bug codes for the two kinds of bugs? I changed them now to REFLC and REFLF.

Do you mean that I should separate the bug codes? I did it, now we have REFLC and REFLF.

I mean you need to add <BugCode> element in another file. Both messages.xml and findbugs.xml have <BugCode> element.

spotbugs/src/main/java/edu/umd/cs/findbugs/detect/ReflectionIncreaseAccessibility.java

KengoTODA

LGTM, thanks for your contribution! I will keep this PR open for a while, to get more feedback from the team.

KengoTODA · 2021-11-04T06:13:42Z

@baloghadamsoftware Please resolve conflict, probably around CHANGELOG.md.

baloghadamsoftware · 2021-11-05T10:35:55Z

@baloghadamsoftware Please resolve conflict, probably around CHANGELOG.md.

Done.

ThrawnCA requested changes May 9, 2021

View reviewed changes

ThrawnCA reviewed May 11, 2021

View reviewed changes

baloghadamsoftware requested a review from ThrawnCA May 14, 2021 12:47

dkrupp mentioned this pull request May 28, 2021

New SEI-Cert Detectors spotbugs/discuss#110

Open

baloghadamsoftware force-pushed the Reflection branch from c2ff2bb to 34430ab Compare June 2, 2021 11:50

ThrawnCA requested changes Jun 30, 2021

View reviewed changes

KengoTODA requested a review from ThrawnCA July 1, 2021 22:57

ThrawnCA previously approved these changes Jul 2, 2021

View reviewed changes

KengoTODA requested changes Jul 2, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

baloghadamsoftware dismissed ThrawnCA’s stale review via 604d132 July 6, 2021 10:33

baloghadamsoftware force-pushed the Reflection branch from 54e0306 to 604d132 Compare July 6, 2021 10:33

KengoTODA previously approved these changes Jul 23, 2021

View reviewed changes

baloghadamsoftware dismissed KengoTODA’s stale review via c71df83 September 13, 2021 10:40

Ádám Balogh added 2 commits September 13, 2021 12:40

Merge branch 'master' into Reflection

9dfdd20

CHANGELOG.md updated

c71df83

baloghadamsoftware requested a review from KengoTODA September 14, 2021 08:24

KengoTODA requested changes Oct 25, 2021

View reviewed changes

KengoTODA reviewed Oct 25, 2021

View reviewed changes

Ádám Balogh added 3 commits October 25, 2021 12:39

Merge branch 'master' into Reflection

3843d2b

Updated according to a comment from @KengoTODA. Also added additional…

5cd8117

… condition to check whether the class is public as well.

CHANGELOG fixed

bee2b4e

baloghadamsoftware requested a review from KengoTODA October 25, 2021 11:25

Ádám Balogh added 3 commits October 27, 2021 16:09

Exclude fields from the objects considered as class-type arguments

45ddecb

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

f709cf1

Add the other part of the SEI CERT Rule: Fields

f2228dd

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.

ThrawnCA previously approved these changes Oct 31, 2021

View reviewed changes

KengoTODA requested changes Nov 1, 2021

View reviewed changes

Bug codes separated and annotation added to the detector

162ca59

baloghadamsoftware dismissed ThrawnCA’s stale review via 162ca59 November 2, 2021 10:35

KengoTODA previously approved these changes Nov 3, 2021

View reviewed changes

ThrawnCA previously approved these changes Nov 3, 2021

View reviewed changes

KengoTODA mentioned this pull request Nov 4, 2021

Release 4.5.0 #1794

Merged

Merge branch 'master' into Reflection

6b1c02a

baloghadamsoftware dismissed stale reviews from ThrawnCA and KengoTODA via 6b1c02a November 5, 2021 10:35

KengoTODA approved these changes Nov 7, 2021

View reviewed changes

KengoTODA merged commit 085339f into spotbugs:master Nov 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new rule REFL_REFLECTION_INCREASES_ACCESSIBILITY_OF_CLASS #1541

Add new rule REFL_REFLECTION_INCREASES_ACCESSIBILITY_OF_CLASS #1541

baloghadamsoftware commented May 7, 2021 •

edited

ThrawnCA May 9, 2021

ThrawnCA May 9, 2021

ThrawnCA May 11, 2021

baloghadamsoftware commented Jun 2, 2021

baloghadamsoftware commented Jun 8, 2021

ThrawnCA Jun 30, 2021

baloghadamsoftware commented Jul 7, 2021

baloghadamsoftware commented Sep 8, 2021

KengoTODA commented Sep 13, 2021

baloghadamsoftware commented Oct 12, 2021

KengoTODA left a comment •

edited

KengoTODA Oct 25, 2021

KengoTODA Oct 25, 2021

baloghadamsoftware commented Oct 25, 2021

baloghadamsoftware commented Oct 25, 2021

baloghadamsoftware commented Oct 27, 2021 •

edited

KengoTODA Oct 31, 2021

baloghadamsoftware Nov 2, 2021

baloghadamsoftware Nov 2, 2021

KengoTODA Nov 4, 2021

KengoTODA left a comment

KengoTODA commented Nov 4, 2021

baloghadamsoftware commented Nov 5, 2021

Add new rule REFL_REFLECTION_INCREASES_ACCESSIBILITY_OF_CLASS #1541

Add new rule REFL_REFLECTION_INCREASES_ACCESSIBILITY_OF_CLASS #1541

Conversation

baloghadamsoftware commented May 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloghadamsoftware commented Jun 2, 2021

baloghadamsoftware commented Jun 8, 2021

Choose a reason for hiding this comment

baloghadamsoftware commented Jul 7, 2021

baloghadamsoftware commented Sep 8, 2021

KengoTODA commented Sep 13, 2021

baloghadamsoftware commented Oct 12, 2021

KengoTODA left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloghadamsoftware commented Oct 25, 2021

baloghadamsoftware commented Oct 25, 2021

baloghadamsoftware commented Oct 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KengoTODA left a comment

Choose a reason for hiding this comment

KengoTODA commented Nov 4, 2021

baloghadamsoftware commented Nov 5, 2021

baloghadamsoftware commented May 7, 2021 •

edited

KengoTODA left a comment •

edited

baloghadamsoftware commented Oct 27, 2021 •

edited