New detector for MET03-J (#2447)

* Added the messages.xml and findbugs.xml components for my first checker. * Added the messages.xml and findbugs.xml components for my first checker. Added Checker as well. * Added the messages.xml and findbugs.xml components for my first checker. Added Checker as well. Added test cases calsses. * Added the messages.xml and findbugs.xml components for my first checker. Added Checker as well. Added test cases calsses. Added test class (JUnit) as well. * Added the messages.xml and findbugs.xml components for my first checker. Added Checker as well. Added test cases calsses. Added test class (JUnit) as well. Edited the Changelog.md. * Added the messages.xml and findbugs.xml components for my first checker. Added Checker as well. Added test cases calsses. Added test class (JUnit) as well. Edited the Changelog.md. Fixed the formatting issues as well. * Apply 1 suggestion(s) to 1 file(s) * Apply 1 suggestion(s) to 1 file(s) * Resolved many comments on merge request. * Added the license and Javadoc for checker class. * Don't report for stream methods called on Random If one of the stream-returning methods (doubles, ints, or longs) is called on Random or SecureRandom, don't report that the Random was only used once Fixes #2370 * fix(deps): update dependency checkstyle to v7.8.2 (#2373) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.mockito:mockito-core to v5.2.0 (#2376) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.apache.groovy:groovy-all to v4.0.10 (#2377) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.slf4j:slf4j-api to v2.0.7 (#2381) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency net.sf.saxon:saxon-he to v12.1 (#2385) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update plugin com.github.spotbugs to v5.0.14 (#2386) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.springframework:spring-core to v6.0.7 (#2383) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update plugin com.gradle.enterprise to v3.12.5 (#2382) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency joda-time:joda-time to v2.12.4 (#2387) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update plugin com.gradle.enterprise to v3.12.6 (#2390) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update asm to v9.5 (#2391) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency joda-time:joda-time to v2.12.5 (#2393) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.apache.groovy:groovy-all to v4.0.11 (#2394) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Issue 543 (#2395) * Issue-543 Store Java class annotation names to PackageMemberAnnotations * Issue-543 Store Java class annotation names to PackageMemberAnnotations * Issue-543 Add AnnotationMatcher for filtering (with test cases) * Issue-543 Extend SAX handler for AnnotationMatcher and classjas attr * Issue-543 Fix whitespaces * Issue-543 Add CHANGELOG entry * Issue-543 Make compatible with Java 1.8 * Issue-543 Extract code to addJavaAnnotationNames method * Issue-543 Update filter file xml schema * Issue-543 Run spotlessApply * Issue-543 Fix AnnotationMatcherTest * Issue-543 Fix file header comments * Issue-543 Use MethodHandles for logger * Issue 543 Fix documentation * Issue 543 Combine expressions when getting annotation type * Issue 543 Rename attribute classjas to classAnnotationNames * Issue-543 Fix whitespaces * Issue-543: Fix typo --------- Co-authored-by: Philipp Sadler <philipp.sadler@gebit.de> * fix(deps): update dependency jacoco to v0.8.9 (#2398) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.checkerframework:checker-qual to v3.33.0 (#2399) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.mockito:mockito-core to v5.3.0 (#2403) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.springframework:spring-core to v6.0.8 (#2405) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update plugin com.gradle.enterprise to v3.13 (#2404) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * minor: make private class final with default constructor (#2407) * chore(deps): update plugin org.ajoberstar.grgit to v5.1.0 (#2409) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * fix(deps): update dependency org.mockito:mockito-core to v5.3.1 (#2408) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update plugin org.ajoberstar.grgit to v5.2.0 (#2411) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Changed the implementation of testing classes entirely. * Added Javadoc for testing files. * Made changes for second time review comments. * Fixed the CHANGELOG.md and added the solution in the bug reporting message. * Fixed the extra boolean parameter in helping function for testing. * Separated the class addClass and addMethod in the bug reporting. * Fixed the package naming convention. * Added comments for detector thought process, improved it's efficiency. * Tried to fix white space issues in findbugs.xml * Fixed some white space issues causing the pipeline to fail. * Added the exact source line number in the detector. * White space correction. * Fixed new lines in finbugs.xml Improved commenting in the sawOpCode() method and removed the sourceline remoting as it yeilds wrong results for large results. * Removed always true evident null check in sawOpCode(). * Fixed source line annotation. Added another class "COmmandMap". This class is actually one of th etest cases of large repo. I added it to verify the correction of reported exact source line number. * Made the bug type local variable in JUnit testing class method `createBugInstanceMatcher`. * Fixed the CHANGELOG.md formatting errors. * Implemented a null check on variable `met` in another detector `ReflectionIncreaseAccessibility` in `sawOpCode()` method. * deleted the temporary test case file. Refactored teh xMethod Variable. * DOn't see it. * Implemented the null check on `met` variable in another checker `ReflectionIncreaseAccessibility.java` class. * Added many good and bad test cases. * Improved and added new assertions in the Junit testing. * Improved the implementation of checker. It has changed totally. Now it is very efficient. * Changed the message reported in consistent with the new implementation of the checker. * Fixing white spaces. * Removed teh commented out lines. * Done! * Done! * Implemented new test cases in `FindVulnerableSecurityCheckMethodsTest.java` * Implemented new test cases in `GoodVulnerableSecurityCheckMethodsTest.java` * Implemented user defined `SecurityManager` required for some test cases. * Implemented Junit tests for the newly added test cases. * Fixed the comments. Fixed bug reporting message. * I don't know where these changes came form automatically. * Removed the commented out lines. Fixed the private methods with 1 usage only. * Revert "I don't know where these changes came form automatically." This reverts commit e8f89d4. * Reverted the Null Pointer Exception. * Refactored the name of the testing class as per conventions. * Refactored the bug type in all the related files. * Refactored the bug type in all the `CHANGELOG.md` --------- Co-authored-by: Judit Knoll <judit.knoll@sigmatechnology.com> Co-authored-by: Mike Dillon <mike@appropriate.io> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Carsten Pfeiffer <cpfeiffer@users.noreply.github.com> Co-authored-by: Philipp Sadler <philipp.sadler@gebit.de> Co-authored-by: Kevin222004 <97679350+Kevin222004@users.noreply.github.com>
spotbugs · Jun 19, 2023 · 073d870 · 073d870
1 parent 8f41e5d
commit 073d870
Show file tree

Hide file tree

Showing 8 changed files with 841 additions and 0 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -27,6 +27,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 * New detector `FindAssertionsWithSideEffects` detecting bug `ASSERTION_WITH_SIDE_EFFECT` and `ASSERTION_WITH_SIDE_EFFECT_METHOD` in case of assertions which may have side effects (See [EXP06-J. Expressions used in assertions must not produce side effects](https://wiki.sei.cmu.edu/confluence/display/java/EXP06-J.+Expressions+used+in+assertions+must+not+produce+side+effects))
 * New rule set `PA_PUBLIC_PRIMITIVE_ATTRIBUTE`, `PA_PUBLIC_ARRAY_ATTRIBUTE` and `PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE` to warn for public attributes which are written by the methods of the class. This rule is loosely based on the SEI CERT rule *OBJ01-J Limit accessibility of fields*. ([#OBJ01-J](https://wiki.sei.cmu.edu/confluence/display/java/OBJ01-J.+Limit+accessibility+of+fields))
 * Extend `SerializableIdiom` detector with new bug type: `SE_PREVENT_EXT_OBJ_OVERWRITE`. It's reported in case of the `readExternal()` method allows any caller to reset any value of an object
+* New Detector `FindVulnerableSecurityCheckMethods` for new bug type `VSC_VULNERABLE_SECURITY_CHECK_METHODS`. This bug is reported whenever a non-final and non-private method of a non-final class performs a security check using the `java.lang.SecurityManager`. (See [SEI CERT MET03-J] (https://wiki.sei.cmu.edu/confluence/display/java/MET03-J.+Methods+that+perform+a+security+check+must+be+declared+private+or+final))
 
 ### Security
 - Disable access to external entities when processing XML ([#2217](https://github.com/spotbugs/spotbugs/pull/2217))

diff --git a/...ests/src/test/java/edu/umd/cs/findbugs/detect/FindVulnerableSecurityCheckMethodsTest.java b/...ests/src/test/java/edu/umd/cs/findbugs/detect/FindVulnerableSecurityCheckMethodsTest.java
@@ -0,0 +1,180 @@
+package edu.umd.cs.findbugs.detect;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasItem;
+
+import org.junit.Test;
+import edu.umd.cs.findbugs.AbstractIntegrationTest;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+
+public class FindVulnerableSecurityCheckMethodsTest extends AbstractIntegrationTest {
+    static String bugType = "VSC_VULNERABLE_SECURITY_CHECK_METHODS";
+
+    @Test
+    public void testingBadCases() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+
+        BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType(bugType)
+                .build();
+        assertThat(getBugCollection(), containsExactly(7, bugInstanceMatcher));
+    }
+
+    @Test
+    public void testingBadCase1() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertVSCBug("badFindVulnerableSecurityCheckMethodsCheck", 23);
+    }
+
+    @Test
+    public void testingBadCase2() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertVSCBug("badFindVulnerableSecurityCheckMethodsCheck2", 37);
+    }
+
+    @Test
+    public void testingBadCase3() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertVSCBug("badFindVulnerableSecurityCheckMethodsCheck3", 53);
+    }
+
+    @Test
+    public void testingBadCase4() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertVSCBug("badFindVulnerableSecurityCheckMethodsCheck4", 94);
+    }
+
+    @Test
+    public void testingBadCase5() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertVSCBug("badFindVulnerableSecurityCheckMethodsCheck6", 128);
+    }
+
+
+    @Test
+    public void testingGoodCase1() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodvulnerablesecuritycheckmethodstestCheck");
+    }
+
+    @Test
+    public void testingGoodCase2() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck2");
+    }
+
+    @Test
+    public void testingGoodCase3() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck4");
+    }
+
+    @Test
+    public void testingGoodCase4() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck5");
+    }
+
+    @Test
+    public void testingGoodCase5() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck");
+    }
+
+    @Test
+    public void testingGoodCase6() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck2");
+    }
+
+    @Test
+    public void testingGoodCase7() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck3");
+    }
+
+    @Test
+    public void testingGoodCase8() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck4");
+    }
+
+    @Test
+    public void testingGoodCase9() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck5");
+    }
+
+    @Test
+    public void testingGoodCase10() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("badCalled");
+    }
+
+    @Test
+    public void testingGoodCase11() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck6");
+    }
+
+    @Test
+    public void testingGoodCase12() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck7");
+    }
+
+    @Test
+    public void testingGoodCase13() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodFindVulnerableSecurityCheckMethodsCheck5");
+    }
+
+    @Test
+    public void testingGoodCase14() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck6");
+    }
+
+    @Test
+    public void testingGoodCase15() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck7");
+    }
+
+    @Test
+    public void testingGoodCase16() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck6");
+    }
+
+    @Test
+    public void testingGoodCase17() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/FindVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck7");
+    }
+
+    @Test
+    public void testingGoodCase18() {
+        performAnalysis("vulnerablesecuritycheckmethodstest/GoodVulnerableSecurityCheckMethodsTest.class");
+        assertNoVSCBug("goodVulnerableSecurityCheckMethodsTestCheck8");
+    }
+
+    private void assertNoVSCBug(String methodName) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType(bugType)
+                .inMethod(methodName)
+                .build();
+        assertThat(getBugCollection(), containsExactly(0, bugTypeMatcher));
+    }
+
+    private void assertVSCBug(String methodName, int lineNumber) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType(bugType)
+                .inMethod(methodName)
+                .atLine(lineNumber)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugTypeMatcher));
+    }
+}
diff --git a/spotbugs/etc/findbugs.xml b/spotbugs/etc/findbugs.xml
@@ -673,6 +673,8 @@
                     reports="ASE_ASSERTION_WITH_SIDE_EFFECT,ASE_ASSERTION_WITH_SIDE_EFFECT_METHOD"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindPublicAttributes" speed="fast"
                     reports="PA_PUBLIC_PRIMITIVE_ATTRIBUTE,PA_PUBLIC_ARRAY_ATTRIBUTE,PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE"/>
+          <Detector class="edu.umd.cs.findbugs.detect.FindVulnerableSecurityCheckMethods" speed="fast"
+                    reports="VSC_VULNERABLE_SECURITY_CHECK_METHODS"/>
     <!-- Bug Categories -->
            <BugCategory category="NOISE" hidden="true"/>
 
@@ -702,6 +704,7 @@
           <BugCode abbrev="AT"/>
           <BugCode abbrev="JUA"/>
           <BugCode abbrev="ASE"/>
+          <BugCode abbrev="VSC"/>
           <!-- Bug patterns -->
           <BugPattern abbrev="JUA" type="JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS" category="BAD_PRACTICE" experimental="true" />
           <BugPattern abbrev="CN" type="OVERRIDING_METHODS_MUST_INVOKE_SUPER" category="CORRECTNESS" />
@@ -1297,4 +1300,6 @@
           <BugPattern abbrev="PA" type="PA_PUBLIC_PRIMITIVE_ATTRIBUTE" category="BAD_PRACTICE"/>
           <BugPattern abbrev="PA" type="PA_PUBLIC_ARRAY_ATTRIBUTE" category="BAD_PRACTICE"/>
           <BugPattern abbrev="PA" type="PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE" category="BAD_PRACTICE"/>
+          <BugPattern abbrev="VSC" type="VSC_VULNERABLE_SECURITY_CHECK_METHODS"
+                    category="MALICIOUS_CODE"/>
 </FindbugsPlugin>
diff --git a/spotbugs/etc/messages.xml b/spotbugs/etc/messages.xml
@@ -1786,6 +1786,16 @@ factory pattern to create these objects.</p>
       ]]>
     </Details>
   </Detector>
+  <Detector class="edu.umd.cs.findbugs.detect.FindVulnerableSecurityCheckMethods">
+    <Details>
+      <![CDATA[
+        <p>
+        Methods that perform security checks should be prevented from being overridden, so they must be declared as
+        private or final. Otherwise, these methods can be compromised when a malicious subclass overrides them
+        and omits the checks.
+        </p>]]>
+    </Details>
+  </Detector>
   <!--
   **********************************************************************
   BugPatterns
@@ -8902,6 +8912,19 @@ Using floating-point variables should not be used as loop counters, as they are
     ]]>
     </Details>
   </BugPattern>
+  <BugPattern type="VSC_VULNERABLE_SECURITY_CHECK_METHODS">
+    <ShortDescription>Non-Private and non-final security check methods are vulnerable</ShortDescription>
+    <LongDescription>The method '{1}' performs security check by using '{2}' method of Security Manager Class, but is overrideable. Declare the method final or private in order to resolve the issue. </LongDescription>
+    <Details>
+      <![CDATA[<p>
+        Methods that perform security checks should be prevented from being overridden, so they must be declared as
+        private or final. Otherwise, these methods can be compromised when a malicious subclass overrides them
+        and omits the checks.
+        <p>
+        See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/MET03-J.+Methods+that+perform+a+security+check+must+be+declared+private+or+final">MET03-J. Methods that perform a security check must be declared private or final</a>.
+        </p>]]>
+    </Details>
+  </BugPattern>
   <!--
   **********************************************************************
    BugCodes
@@ -9057,4 +9080,5 @@ Using floating-point variables should not be used as loop counters, as they are
   <BugCode abbrev="USC">Potential security check based on untrusted source</BugCode>
   <BugCode abbrev="ASE">Assertion with side effect</BugCode>
   <BugCode abbrev="PA">Public Attribute</BugCode>
+  <BugCode abbrev="VSC">Vulnerable security check performing methods</BugCode>
 </MessageCollection>
diff --git a/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindVulnerableSecurityCheckMethods.java b/spotbugs/src/main/java/edu/umd/cs/findbugs/detect/FindVulnerableSecurityCheckMethods.java
@@ -0,0 +1,142 @@
+/*
+ * 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 edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.SourceLineAnnotation;
+import edu.umd.cs.findbugs.ba.ClassContext;
+import edu.umd.cs.findbugs.ba.XMethod;
+import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
+
+import org.apache.bcel.Const;
+import org.apache.bcel.classfile.JavaClass;
+
+import java.util.HashSet;
+import java.util.Set;
+
+/***
+ * This detector finds all the vulnerable methods which uses Security Manager to perform
+ * some security check but are declared non-final and non-private in a non-final class.
+ * Please see @see <a href="https://wiki.sei.cmu.edu/confluence/display/java/MET03-J.+Methods+that+perform+a+security+check+must+be+declared+private+or+final">SEI CERT MET03-J</a>
+ * @author Nazir, Muhammad Zafar Iqbal
+ */
+public class FindVulnerableSecurityCheckMethods extends OpcodeStackDetector {
+    private final BugReporter bugReporter;
+    private static final Set<String> badMethodNames;
+
+    public FindVulnerableSecurityCheckMethods(BugReporter bugReporter) {
+        this.bugReporter = bugReporter;
+
+    }
+
+    static {
+        badMethodNames = new HashSet<String>() {
+            {
+                add("checkAccept");
+                add("checkAccess");
+                add("checkAwtEventQueueAccess");
+                add("checkConnect");
+                add("checkCreateClassLoader");
+                add("checkDelete");
+                add("checkExec");
+                add("checkExit");
+                add("checkLink");
+                add("checkListen");
+                add("checkMemberAccess");
+                add("checkMulticast");
+                add("checkPackageAccess");
+                add("checkPackageDefinition");
+                add("checkPermission");
+                add("checkPrintJobAccess");
+                add("checkPropertiesAccess");
+                add("checkRead");
+                add("checkSecurityAccess");
+                add("checkSetFactory");
+                add("checkSystemClipboardAccess");
+                add("checkTopLevelWindow");
+                add("checkWrite");
+
+                //Deprecated Method Call Support
+                add("classDepth");
+                add("classLoaderDepth");
+                add("currentClassLoader");
+                add("currentLoadedClass");
+
+                add("getInCheck");
+                add("inClass");
+                add("inClassLoader");
+            }
+        };
+    }
+
+    /**
+     * Only interested in non-final classes
+     */
+    @Override
+    public void visitClassContext(ClassContext classContext) {
+        JavaClass ctx = classContext.getJavaClass();
+        // Break out of analyzing this class if declared as final
+        if (ctx.isFinal()) {
+            return;
+        }
+        super.visitClassContext(classContext);
+    }
+
+    /**
+     * Returns true if the opcode is a method invocation false otherwise
+     */
+    private boolean isMethodCall(int seen) {
+        return seen == Const.INVOKESTATIC ||
+                seen == Const.INVOKEVIRTUAL ||
+                seen == Const.INVOKEINTERFACE ||
+                seen == Const.INVOKESPECIAL;
+    }
+
+    @Override
+    public void sawOpcode(int seen) {
+        //I first check if there is any method invocation.
+        if (isMethodCall(seen)) {
+            //If yes then I get the method in which this invocation happened
+            XMethod method = getXMethod();
+            if (method == null) {
+                return;
+            }
+            //We examine only non-final and non-private methods
+            if (!method.isFinal() && !method.isPrivate()) {
+                //Then we get the method which was invoked
+                XMethod calledMethod = this.getXMethodOperand();
+                if (calledMethod == null) {
+                    return;
+                }
+                //I used the documentation of SecurityManager class and compiled the "badMethodNames"
+                // list of method names which can really perform security check.
+                //If security check is really performed, I report the bug.
+                if ("java.lang.SecurityManager".equals(calledMethod.getClassName())
+                        && badMethodNames.contains(calledMethod.getName())) {
+                    bugReporter.reportBug(new BugInstance(this, "VSC_VULNERABLE_SECURITY_CHECK_METHODS", NORMAL_PRIORITY)
+                            .addClass(this.getClassContext().getJavaClass())
+                            .addMethod(method)
+                            .addMethod(calledMethod)
+                            .addSourceLine(SourceLineAnnotation.fromVisitedInstruction(this, getPC())));
+                }
+            }
+        }
+    }
+}