New detector for MET07-J #2467

NazirMuhammadZafarIqbal · 2023-07-09T18:58:33Z

I have implemented a checker named "FindHidingSubClass" for SEI CERT MET07-J

The checker proved to be effective in identifying potential vulnerabilities in Java code. By enforcing the declaration of methods that can be hidden as non-static or private. This checker helps reduce the risk of method hiding to avoid unexpected and misleading outcomes, especially when programmers expect it to be method overriding.

I have both White Box and Black Box Testing. For White Box Testing, I have created 11 compliant and 12 non-compliant test cases. My checker passes all of the test cases. For Black Box Testing, I tested the checker on the SpotBugs projects. I got 10 true positives and 0 false positives.

Results for spotbugs Project:
13.0_SBTestRepoLogicChanges.txt

Detections by MET03-J in SpotBugs:
13.0_SBTestRepoLogicChanges_filtered.txt

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

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

…ker.

Fixed typo - renamed GoodFindHidingSubClassTest class and file

Fixed the commented out lines. Fixed the package naming convention in test cases and Junit test repo. Added the correct argument checking for detecting entry point of the class. Improved the checker logic to detect not only equality but also subSignature case.

…me but failed so commented out the code.

Fixed messages.xml and findbugs.xml added lines as per standards. Fixed whitespace issues.

Added comments in test cases. Improved the messages.xml description by adding the suggested solutions to resolve the error. Renamed one of the test cases. "GoodFindHidingSubClassTest2"

… the detector on the TestRepo.

Implemented a few more test cases.

Implemented some new test cases.

…o track down the .class$(String) detections in the output of large test case repo: SpotBUgs

… signature for hiding. Dropped the idea of subSignature.

…ding the `OpcodeStackDetector` rather implements a more basic interface `Detector`. SO, now detector works at more low-level i.e. increasing efficiency.

JuditKnoll · 2023-11-23T13:30:24Z

@gtoison, @ThrawnCA you had some change request for this PR. I think all of them had been addressed. Can you please look at this PR once more, when you have time?

gtoison · 2023-11-23T14:27:28Z

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

+        Method[] methods = subClass.getMethods();
+        //For each super class, I go through each method of the subclass
+        //and filter the non-private and static methods, as private methods can't be overridden.
+        for (JavaClass superClass : superClasses) {


I think it should be a bit faster to invert the superClass and the method loops.
Here we're evalutating the method.isStatic() && !method.isPrivate() && !isMainMethod(method) test (as well as the following test) for every super class and every method.
We could do this instead:

for (Method method : methods) { if (method.isStatic() && !method.isPrivate() && !isMainMethod(method)) { if (isConstructor(method) || isHidingInnerClass(method) || isAutoGeneratedMethod(method)) { continue; } } for (JavaClass superClass : superClasses) {

I got your idea for efficiency ;)

Your suggestion has the problem we need to invert the outer conditional and keep the inner conditional in parallel to the outer conditional.

I have fixed the code accordingly.

gtoison · 2023-11-23T14:28:33Z

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

+    }
+
+    /**
+     * This method checks either the argument is String array


either -> whether ?

ThrawnCA · 2023-11-24T00:41:55Z

spotbugs/etc/messages.xml

+      <![CDATA[
+            <p>
+              Hiding happens when a sub class defines a static method
+              with same header (signature plus return type) as any of its super classes.


"with same" -> "with the same"

ThrawnCA · 2023-11-24T00:42:20Z

spotbugs/etc/messages.xml

+            <p>
+              Hiding happens when a sub class defines a static method
+              with same header (signature plus return type) as any of its super classes.
+              In case of method hiding, the invoked method is determined based on the specific qualified name or


"In case of" -> "In the event of"

ThrawnCA · 2023-11-24T00:42:42Z

spotbugs/etc/messages.xml

+              method invocation expression used at the calling site.
+              The results are often unexpected, although the Java language provides unambiguous rules for method invocation for method hiding.
+              Moreover, method hiding and method overriding is often confused by programmers.
+              Consequently, programmers must avoid the method hiding.


I think this is a "should" rather than "must".

I wrote must because if the programmer has mistaken the method hiding as method overriding, then he is receiving wrong results. So he must fix it.

I still adopt your suggestion in case the programmer is aware of the difference and he is using the method of hiding carefully. Then he does not need to fix the code.

ThrawnCA · 2023-11-24T00:42:56Z

spotbugs/etc/messages.xml

+              The results are often unexpected, although the Java language provides unambiguous rules for method invocation for method hiding.
+              Moreover, method hiding and method overriding is often confused by programmers.
+              Consequently, programmers must avoid the method hiding.
+              Programmer should declare the respective methods non-static or restrict it as private to eradicate the problem.


"methods" -> "method"

ThrawnCA · 2023-11-24T00:45:35Z

spotbugsTestCases/src/java/findhidingsubclasstest/BadInEqualMultipleStaticMethod.java

@@ -0,0 +1,31 @@
+package findhidingsubclasstest;
+
+class SuperBadInEqualMultipleMethod {


I don't understand what this class name means.

It is the SUPERclass for a non-compliant (BAD) test case, with MULTIPLE METHODs declared in both super and sub classes. But the number methods is INEQUAL. So Super.

ThrawnCA · 2023-11-24T00:46:59Z

spotbugsTestCases/src/java/findhidingsubclasstest/GoodInEqualNonStaticMethods.java

+}
+
+/**
+ * This test case is a complaint test case with multiple methods declared in both super and sub classes.


"complaint" -> "compliant"?

ThrawnCA · 2023-11-24T00:47:08Z

spotbugsTestCases/src/java/findhidingsubclasstest/GoodMultiLevelInheritanceNonStatic.java

+}
+
+/**
+ * This test case is a complaint test case with multiple level inheritance.


"complaint" -> "compliant"?

ThrawnCA · 2023-11-24T00:47:15Z

spotbugsTestCases/src/java/findhidingsubclasstest/GoodMultiLevelInheritancePrivate.java

+}
+
+/**
+ * This test case is a complaint test case with multiple level inheritance.


"complaint" -> "compliant"?

ThrawnCA · 2023-11-24T00:47:20Z

spotbugsTestCases/src/java/findhidingsubclasstest/GoodMultipleNonStaticMethods.java

+}
+
+/**
+ * This test case is a complaint test case with multiple methods declared in both super and sub classes.


"complaint" -> "compliant"?

ThrawnCA · 2023-11-24T00:47:30Z

spotbugsTestCases/src/java/findhidingsubclasstest/GoodNonOrderedMultipleNonStaticMethods.java

+
+
+/**
+ * This test case is a complaint test case with multiple methods declared in both super and sub classes.


"complaint" -> "compliant"?

Midya-ELTE · 2023-11-24T08:56:36Z

I suggest the name of the checker be (FindHiddenMethod) instead of (FindHiddenSubClass).

Midya-ELTE · 2023-11-24T09:20:55Z

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

+ * Please see @see <a href="https://wiki.sei.cmu.edu/confluence/display/java/MET07-J.+Never+declare+a+class+method+that+hides+a+method+declared+in+a+superclass+or+superinterface">SEI CERT MET07-J</a>
+ * @author Nazir, Muhammad Zafar Iqbal
+ */
+public class FindHidingSubClass implements Detector {


I think it will be more descriptive if you name the class FindHiddenMethod instead.

Thank you for pointing out the miss-naming. I have renamed the checker now. I have also changed it at all the relevant places.

Midya-ELTE · 2023-11-24T09:25:27Z

spotbugs/etc/findbugs.xml

@@ -1320,4 +1324,5 @@
          <BugPattern abbrev="PI" type="PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_METHOD_NAMES" category="BAD_PRACTICE"/>
          <BugPattern abbrev="PI" type="PI_DO_NOT_REUSE_PUBLIC_IDENTIFIERS_LOCAL_VARIABLE_NAMES" category="BAD_PRACTICE"/>

+          <BugPattern abbrev="HSBC" type="HSBC_HIDING_SUB_CLASS" category="CORRECTNESS"/>


I think it will be more descriptive if you change the type to HSM_HIDING_METHOD, and the abbrev will be HSM in this case.

…itionals accordingly.

…abbreviation from `HSBC` to `HSM`.

…abbreviation from `HSBC` to `HSM`. In teh test module.

…as well.

gtoison · 2023-11-27T16:02:32Z

CHANGELOG.md

@@ -16,6 +16,9 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.

 - tbd

+### Added
+- New detector `FindHidingSubClass` for bug type `HSBC_HIDING_SUB_CLASS`. This bug is reported whenever a subclass method hides the static method of super class. (See [SEI CERT MET07-J] (https://wiki.sei.cmu.edu/confluence/display/java/MET07-J.+Never+declare+a+class+method+that+hides+a+method+declared+in+a+superclass+or+superinterface)).


I think you forgot to edit the changelog after changing the name of the detector, thank you for all the changes!

Yeah, it just slipped out of my mind. I have fixed it now. Thank you for your prompt review :))

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

spotbugs/etc/messages.xml

spotbugsTestCases/src/java/findhiddenmethodtest/BadMultiLevelInheritance.java

spotbugsTestCases/src/java/findhiddenmethodtest/GoodMultiLevelInheritanceNonStatic.java

spotbugsTestCases/src/java/findhiddenmethodtest/GoodMultiLevelInheritancePrivate.java

CHANGELOG.md

Merged the conditionals. Synchronized the comments. Fixed typos.

ThrawnCA · 2023-12-03T22:44:57Z

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

+
+            for (JavaClass superClass : superClasses) {
+                Method[] superMethods = superClass.getMethods();
+                //I check either the subclass method is hiding the superclass method. If yes I report the bug.


I'm having trouble following this grammar. "I check either the subclass method" doesn't make sense to me. Did you mean "I check if"?

I meant "whether".

CHANGELOG.md

spotbugsTestCases/src/java/findhiddenmethodtest/BadInEqualMultipleStaticMethod.java

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

spotbugs/etc/messages.xml

ThrawnCA · 2023-12-05T03:29:02Z

spotbugs/etc/messages.xml

@@ -9014,6 +9025,25 @@ Using floating-point variables should not be used as loop counters, as they are
        ]]>
    </Details>
  </BugPattern>
+  <BugPattern type="HSM_HIDING_METHOD">
+    <ShortDescription>Method hiding must be avoided.</ShortDescription>


"Should" rather than "must".

# Conflicts: # spotbugs/etc/findbugs.xml

JuditKnoll · 2024-01-30T08:39:59Z

spotbugs-tests/src/test/java/edu/umd/cs/findbugs/detect/FindHiddenMethodTest.java

+    private final String BUG_TYPE = "HSM_HIDING_METHOD";
+
+    @Test
+    void testBadFindHiddenMethodTest() {


For the "bad" testcases, there could be an assert for the number of the found buginstances as well.

User fixed issue, so this is resolved.

NazirMuhammadZafarIqbal and others added 30 commits April 2, 2023 16:48

Added the messages.xml and findbugs.xml components for my second chec…

19087fc

…ker.

Added test cases calsses.

28bf58c

Added test class (JUnit) as well.

4e461cf

Fixed typo - renamed GoodFindHidingSubClassTest class and file

Added Checker as well.

b0c19f8

Further Implemented Checker. Yet not complete.

5c7ddcb

Tried to get rid of Assertion Exception but couldn't succeed.

3953d68

Merge branch 'master' into MET07

b579fb7

Improved the implementation.

c850d23

Merge branch 'master' into MET07

3058c61

Fixed the problem regarding the testing. Now build is successful.

7fabca9

Trying out some methods to detect method call with fully qualified na…

991941e

…me but failed so commented out the code.

Cleared out the trash code when experimenting in previous commit.

001b4ed

Merge branch 'master' into MET07

777b8ff

Added changeLog.md.

df8a6ba

Fixed messages.xml and findbugs.xml added lines as per standards. Fixed whitespace issues.

Added comments for the main detector class.

9c86374

Added comments in test cases. Improved the messages.xml description by adding the suggested solutions to resolve the error. Renamed one of the test cases. "GoodFindHidingSubClassTest2"

Fixed a type in Javadoc.

b8d0a96

Applied a null check in ReflectionIncreaseAccessibility.java to run…

e623348

… the detector on the TestRepo.

Fixing whitespaces.

31f07c3

Improved checker implementation by main method detection.

84e0cef

Implemented a few more test cases.

Improved and tested logic of checker.

484a273

Implemented some new test cases.

Trying out minilist example of MailcapCommandMap and its superclass t…

3efa5de

…o track down the .class$(String) detections in the output of large test case repo: SpotBUgs

As discussed in meeting, I reverted back to the exact matching of the…

717b83d

… signature for hiding. Dropped the idea of subSignature.

Commented out lines to make build successful.

5ec2627

Merge branch 'master' into MET07

cc84241

Improved and Implemented many new test cases.

d64ab65

Implemented the Junit tests accordingly.

f2c0aa8

Improved and changed the implementation. Now the checker is not exten…

285120f

…ding the `OpcodeStackDetector` rather implements a more basic interface `Detector`. SO, now detector works at more low-level i.e. increasing efficiency.

Done!

f483f07

Done!

45aa398

JuditKnoll approved these changes Nov 17, 2023

View reviewed changes

gtoison reviewed Nov 23, 2023

View reviewed changes

ThrawnCA requested changes Nov 24, 2023

View reviewed changes

Midya-ELTE suggested changes Nov 24, 2023

View reviewed changes

NazirMuhammadZafarIqbal added 6 commits November 27, 2023 13:48

Merge branch 'master' into MET07

5308f6f

Inverted the loops to make the code efficient. Also adjusted the cond…

4e7e3f3

…itionals accordingly.

Fixed some English grammar issues in comments.

660a5ff

Renamed the Checker name to more appropriate name. ALso, changed the …

57544aa

…abbreviation from `HSBC` to `HSM`.

Renamed the Checker name to more appropriate name. ALso, changed the …

42843c0

…abbreviation from `HSBC` to `HSM`. In teh test module.

Forgot the change in bug reporter. And renamed the assertion methods …

58741f9

…as well.

gtoison previously requested changes Nov 27, 2023

View reviewed changes

Changed checker name in CHANGELOG.md

58422fa

JuditKnoll requested changes Nov 27, 2023

View reviewed changes

Midya-ELTE approved these changes Nov 30, 2023

View reviewed changes

Midya-ELTE approved these changes Dec 3, 2023

View reviewed changes

NazirMuhammadZafarIqbal added 4 commits December 3, 2023 15:56

Updated master branch.

81399df

Changed the description of detector.

d7c581b

Merged the conditionals. Synchronized the comments. Fixed typos.

Update by IDE.

c1095a0

Fixed spacing issues.

6cc4453

ThrawnCA reviewed Dec 3, 2023

View reviewed changes

JuditKnoll requested changes Dec 4, 2023

View reviewed changes

ThrawnCA reviewed Dec 5, 2023

View reviewed changes

NazirMuhammadZafarIqbal added 2 commits January 26, 2024 19:32

Merge branch 'master' into MET07

ee646c2

# Conflicts: # spotbugs/etc/findbugs.xml

Fixed misleading comment issues.

047d871

ThrawnCA approved these changes Jan 29, 2024

View reviewed changes

JuditKnoll approved these changes Jan 29, 2024

View reviewed changes

JuditKnoll requested changes Jan 30, 2024

View reviewed changes

		@@ -0,0 +1,31 @@
		package findhidingsubclasstest;

		class SuperBadInEqualMultipleMethod {



		/**
		* This test case is a complaint test case with multiple methods declared in both super and sub classes.

New detector for MET07-J #2467

Are you sure you want to change the base?

New detector for MET07-J #2467

Conversation

NazirMuhammadZafarIqbal commented Jul 9, 2023

JuditKnoll commented Nov 23, 2023

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

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

Choose a reason for hiding this comment

MuhammadZafarIqbal Nov 27, 2023 • 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

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

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

Choose a reason for hiding this comment

Midya-ELTE commented Nov 24, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuhammadZafarIqbal Nov 27, 2023 •

edited