Extend the FindOverridableMethodCall detector to handle SER09-J

CHANGELOG.md

@@ -37,6 +37,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
   - `SING_SINGLETON_IMPLEMENTS_SERIALIZABLE` is reported when a class directly or indirectly implements the `Serializable` interface and
   - `SING_SINGLETON_GETTER_NOT_SYNCHRONIZED` is reported when the instance-getter method of the singleton class is not synchronized.
     (See [SEI CERT MSC07-J](https://wiki.sei.cmu.edu/confluence/display/java/MSC07-J.+Prevent+multiple+instantiations+of+singleton+objects))
+- Extend `FindOverridableMethodCall` detector with new bug type: `MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT`. It's reported when an overridable method is called from `readObject()`, according to SEI CERT rule [SER09-J. Do not invoke overridable methods from the readObject() method](https://wiki.sei.cmu.edu/confluence/display/java/SER09-J.+Do+not+invoke+overridable+methods+from+the+readObject%28%29+method).
 
 ### Changed
 - Minor cleanup in connection with slashed and dotted names ([#2805](https://github.com/spotbugs/spotbugs/pull/2805))

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

@@ -126,6 +126,101 @@ void testFinalClassInheritedMethodReference() {
         testPass("FinalClassInheritedMethodReference");
     }
 
+    @Test
+    void testDirectReadObject() {
+        testReadObject("DirectReadObject", 8);
+    }
+
+    @Test
+    void testDirectReadObjectStreamMethods() {
+        testPass("DirectReadObjectStreamMethods");
+    }
+
+    @Test
+    void testDirectReadObjectStreamMethods2() {
+        testReadObject("DirectReadObjectStreamMethods2", 11);
+    }
+
+    @Test
+    void testIndirectReadObject1() {
+        testReadObject("IndirectReadObject1", 11);
+    }
+
+    @Test
+    void testIndirectReadObject2() {
+        testReadObject("IndirectReadObject2", 7);
+    }
+
+    @Test
+    void testIndirectStreamMethods1() {
+        testPass("IndirectStreamMethods1");
+    }
+
+    @Test
+    void testIndirectStreamMethods2() {
+        testPass("IndirectStreamMethods2");
+    }
+
+    @Test
+    void DoubleIndirectReadObjectCase1() {
+        testReadObject("DoubleIndirectReadObjectCase1", 18);
+    }
+
+    @Test
+    void DoubleIndirectReadObjectCase2() {
+        testReadObject("DoubleIndirectReadObjectCase2", 18);
+    }
+
+    @Test
+    void DoubleIndirectReadObjectCase3() {
+        testReadObject("DoubleIndirectReadObjectCase3", 13);
+    }
+
+    @Test
+    void DoubleIndirectReadObjectCase4() {
+        testReadObject("DoubleIndirectReadObjectCase4", 12);
+    }
+
+    @Test
+    void DoubleIndirectReadObjectCase5() {
+        testReadObject("DoubleIndirectReadObjectCase5", 7);
+    }
+
+    @Test
+    void DoubleIndirectReadObjectCase6() {
+        testReadObject("DoubleIndirectReadObjectCase6", 7);
+    }
+
+    @Test
+    void MethodReferenceReadObject() {
+        testReadObject("MethodReferenceReadObject", 12);
+    }
+
+    @Test
+    void MethodReferenceReadObjectIndirect1() {
+        testReadObject("MethodReferenceReadObjectIndirect1", 16);
+    }
+
+    @Test
+    void MethodReferenceReadObjectIndirect2() {
+        testReadObject("MethodReferenceReadObjectIndirect2", 24);
+    }
+
+    @Test
+    void MethodReferenceReadObjectIndirect3() {
+        testReadObject("MethodReferenceReadObjectIndirect3", 24);
+    }
+
+    @Test
+    void testFinalClassDirectReadObject() {
+        testPass("FinalClassDirectReadObject");
+    }
+
+    @Test
+    void testFinalClassIndirectReadObject() {
+        testPass("FinalClassIndirectReadObject");
+    }
+
     void testCase(String className, int constructorLine, int cloneLine) {
         performAnalysis("overridableMethodCall/" + className + ".class");
 
@@ -134,6 +229,22 @@ void testCase(String className, int constructorLine, int cloneLine) {
         checkOverridableMethodCallInClone(className, cloneLine);
     }
 
+    void testReadObject(String className, int warningLine) {
+        performAnalysis("overridableMethodCall/" + className + ".class");
+
+        BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT").build();
+        assertThat(getBugCollection(), containsExactly(1, bugTypeMatcher));
+
+        final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType("MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT")
+                .inClass(className)
+                .inMethod("readObject")
+                .atLine(warningLine)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+
     void testPass(String className) {
         performAnalysis("overridableMethodCall/" + className + ".class");
 
@@ -158,6 +269,10 @@ void checkNoBug() {
         bugTypeMatcher = new BugInstanceMatcherBuilder()
                 .bugType("MC_OVERRIDABLE_METHOD_CALL_IN_CLONE").build();
         assertThat(getBugCollection(), containsExactly(0, bugTypeMatcher));
+
+        bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType("MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT").build();
+        assertThat(getBugCollection(), containsExactly(0, bugTypeMatcher));
     }
 
     void checkOverridableMethodCallInConstructor(String className, int line) {

spotbugs/etc/findbugs.xml

@@ -658,7 +658,7 @@
           <Detector class="edu.umd.cs.findbugs.detect.ReflectionIncreaseAccessibility"
                     reports="REFLC_REFLECTION_MAY_INCREASE_ACCESSIBILITY_OF_CLASS,REFLF_REFLECTION_MAY_INCREASE_ACCESSIBILITY_OF_FIELD"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindOverridableMethodCall"
-                    reports="MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR,MC_OVERRIDABLE_METHOD_CALL_IN_CLONE"/>
+                    reports="MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR,MC_OVERRIDABLE_METHOD_CALL_IN_CLONE,MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT"/>
           <Detector class="edu.umd.cs.findbugs.detect.ConstructorThrow" speed="fast"
                     reports="CT_CONSTRUCTOR_THROW"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindInstanceLockOnSharedStaticData" speed="fast"
@@ -1304,6 +1304,7 @@
           <BugPattern abbrev="REFLF" type="REFLF_REFLECTION_MAY_INCREASE_ACCESSIBILITY_OF_FIELD" category="MALICIOUS_CODE" />
           <BugPattern abbrev="MC" type="MC_OVERRIDABLE_METHOD_CALL_IN_CONSTRUCTOR" category="MALICIOUS_CODE" />
           <BugPattern abbrev="MC" type="MC_OVERRIDABLE_METHOD_CALL_IN_CLONE" category="MALICIOUS_CODE" />
+          <BugPattern abbrev="MC" type="MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT" category="MALICIOUS_CODE" />
           <BugPattern abbrev="SSD" type="SSD_DO_NOT_USE_INSTANCE_LOCK_ON_SHARED_STATIC_DATA" category="MT_CORRECTNESS" />
           <BugPattern abbrev="FL" type="FL_FLOATS_AS_LOOP_COUNTERS" category="CORRECTNESS" />
           <BugPattern abbrev="THROWS" type="THROWS_METHOD_THROWS_RUNTIMEEXCEPTION" category="BAD_PRACTICE" />

spotbugs/etc/messages.xml

@@ -1738,11 +1738,14 @@ factory pattern to create these objects.</p>
   <Detector class="edu.umd.cs.findbugs.detect.FindOverridableMethodCall">
     <Details>
       <![CDATA[
-            <p>Detector for patterns where a constructor or a clone() method calls an overridable
-               method.</p>
+            <p>
+               Detector for patterns where a constructor, a clone(), or a readObject() method
+               calls an overridable method.
+            </p>
             <p>
                Calling an overridable method from a constructor may result in the use of
-               uninitialized data. Calling such method from a clone() method is insecure.
+               uninitialized data. Calling such method from a clone(), or readObject() method
+               is insecure.
             </p>
       ]]>
     </Details>
@@ -8794,6 +8797,22 @@ object explicitly.</p>
       </p>]]>
     </Details>
   </BugPattern>
+  <BugPattern type="MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT">
+    <ShortDescription>An overridable method is called from the readObject method.</ShortDescription>
+    <LongDescription>Overridable method {2} is called from readObject.</LongDescription>
+    <Details>
+      <![CDATA[<p>
+      The readObject() method must not call any overridable methods. Invoking overridable methods from the readObject()
+      method can provide the overriding method with access to the object's state before it is fully initialized. This
+      premature access is possible because, in deserialization, readObject plays the role of object constructor and
+      therefore object initialization is not complete until readObject exits.</p>
+      <p>
+      <br/>
+      See SEI CERT rule <a href="https://wiki.sei.cmu.edu/confluence/display/java/SER09-J.+Do+not+invoke+overridable+methods+from+the+readObject%28%29+method">
+      SER09-J. Do not invoke overridable methods from the readObject() method</a>.
+      </p>]]>
+    </Details>
+  </BugPattern>
   <BugPattern type="SING_SINGLETON_IMPLEMENTS_CLONEABLE">
     <ShortDescription>Class using singleton design pattern directly implements Cloneable interface.</ShortDescription>
     <LongDescription>Class ({0}) using singleton design pattern directly implements Cloneable interface.</LongDescription>

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

@@ -61,13 +61,15 @@ private static class CallerInfo {
     // For methods called using the standard way
     private final Map<XMethod, CallerInfo> callerConstructors = new HashMap<>();
     private final Map<XMethod, CallerInfo> callerClones = new HashMap<>();
+    private final Map<XMethod, CallerInfo> callerReadObjects = new HashMap<>();
     private final Map<XMethod, XMethod> callsToOverridable = new HashMap<>();
     private final MultiMap<XMethod, XMethod> callerToCalleeMap = new MultiMap<>(ArrayList.class);
     private final MultiMap<XMethod, XMethod> calleeToCallerMap = new MultiMap<>(ArrayList.class);
 
     // For methods called using method references
     private final Map<Integer, CallerInfo> refCallerConstructors = new HashMap<>();
     private final Map<Integer, CallerInfo> refCallerClones = new HashMap<>();
+    private final Map<Integer, CallerInfo> refCallerReadObjects = new HashMap<>();
     private final MultiMap<Integer, XMethod> refCalleeToCallerMap = new MultiMap<>(ArrayList.class);
 
 
@@ -82,11 +84,13 @@ public void visit(JavaClass obj) {
         super.visit(obj);
         callerConstructors.clear();
         callerClones.clear();
+        callerReadObjects.clear();
         callsToOverridable.clear();
         callerToCalleeMap.clear();
         calleeToCallerMap.clear();
         refCallerConstructors.clear();
         refCallerClones.clear();
+        refCallerReadObjects.clear();
         refCalleeToCallerMap.clear();
     }
 
@@ -98,8 +102,9 @@ public void visitBootstrapMethods(BootstrapMethods obj) {
         for (int i = 0; i < obj.getBootstrapMethods().length; ++i) {
             CallerInfo ctor = refCallerConstructors.get(i);
             CallerInfo clone = refCallerClones.get(i);
+            CallerInfo readObject = refCallerReadObjects.get(i);
             Collection<XMethod> callers = refCalleeToCallerMap.get(i);
-            if (ctor == null && clone == null && (callers == null || callers.isEmpty())) {
+            if (ctor == null && clone == null && readObject == null && (callers == null || callers.isEmpty())) {
                 continue;
             }
             Optional<Method> method = BootstrapMethodsUtil.getMethodFromBootstrap(obj, i, getConstantPool(),
@@ -117,6 +122,9 @@ public void visitBootstrapMethods(BootstrapMethods obj) {
                     NORMAL_PRIORITY, clone.sourceLine)) {
                 checkAndRecordCallFromClone(clone.method, xMethod, clone.sourceLine);
             }
+            if (readObject != null && reportIfOverridableCallInReadObject(readObject.method, xMethod, readObject.sourceLine)) {
+                checkAndRecordCallFromReadObject(readObject.method, xMethod, readObject.sourceLine);
+            }
             if (callers != null) {
                 for (XMethod caller : callers) {
                     if (xMethod.isPrivate() || xMethod.isFinal()) {
@@ -156,6 +164,9 @@ public void sawOpcode(int seen) {
                     && item.getReturnValueOf().equals(superClone(getXClass()))) {
                 refCallerClones.put(constDyn.getBootstrapMethodAttrIndex(),
                         new CallerInfo(getXMethod(), SourceLineAnnotation.fromVisitedInstruction(this)));
+            } else if (isCurrentMethodReadObject()) {
+                refCallerReadObjects.put(constDyn.getBootstrapMethodAttrIndex(),
+                        new CallerInfo(getXMethod(), SourceLineAnnotation.fromVisitedInstruction(this)));
             } else {
                 refCalleeToCallerMap.add(constDyn.getBootstrapMethodAttrIndex(), getXMethod());
             }
@@ -183,7 +194,12 @@ public void sawOpcode(int seen) {
                     checkAndRecordCallFromClone(getXMethod(), method,
                             SourceLineAnnotation.fromVisitedInstruction(this));
                 }
-
+            } else if (isCurrentMethodReadObject()) {
+                if (reportIfOverridableCallInReadObject(getXMethod(), method,
+                        SourceLineAnnotation.fromVisitedInstruction(this))) {
+                    checkAndRecordCallFromReadObject(getXMethod(), method,
+                            SourceLineAnnotation.fromVisitedInstruction(this));
+                }
             } else if (item.getRegisterNumber() == 0
                     && (getXMethod().isPrivate() || getXMethod().isFinal())) {
                 if (method.isPrivate() || method.isFinal()) {
@@ -195,6 +211,10 @@ public void sawOpcode(int seen) {
         }
     }
 
+    private boolean isCurrentMethodReadObject() {
+        return "readObject".equals(getMethodName()) && "(Ljava/io/ObjectInputStream;)V".equals(getMethodSig());
+    }
+
     private XMethod superClone(XClass clazz) {
         ClassDescriptor superD = clazz.getSuperclassDescriptor();
         XClass xSuper;
@@ -221,6 +241,27 @@ boolean checkDirectCase(XMethod caller, XMethod method, String message, int prio
         return true;
     }
 
+    private boolean shouldIgnoreCallInReadObject(XMethod method) {
+        // SER09-J-EX0: The readObject() method may invoke the overridable methods defaultReadObject() and readFields()
+        // in class java.io.ObjectInputStream
+        boolean inOIS = "java.io.ObjectInputStream".equals(method.getClassName());
+        String methodName = method.getName();
+        return inOIS && ("defaultReadObject".equals(methodName) || "readFields".equals(methodName));
+    }
+
+    private boolean reportIfOverridableCallInReadObject(XMethod caller, XMethod method, SourceLineAnnotation sourceLine) {
+        if (!shouldIgnoreCallInReadObject(method) && !method.isPrivate() && !method.isFinal()) {
+            bugAccumulator.accumulateBug(
+                    new BugInstance(this, "MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT", NORMAL_PRIORITY)
+                            .addClass(this)
+                            .addMethod(caller)
+                            .addString(method.getName()), sourceLine);
+            return false;
+        }
+
+        return true;
+    }
+
     private boolean checkAndRecordCallFromConstructor(XMethod constructor, XMethod callee,
             SourceLineAnnotation sourceLine) {
         XMethod overridable = getIndirectlyCalledOverridable(callee);
@@ -247,6 +288,18 @@ private boolean checkAndRecordCallFromClone(XMethod clone, XMethod callee,
         return true;
     }
 
+    private void checkAndRecordCallFromReadObject(XMethod readObject, XMethod callee,
+            SourceLineAnnotation sourceLine) {
+        XMethod overridable = getIndirectlyCalledOverridable(callee);
+        if (overridable != null && !shouldIgnoreCallInReadObject(overridable)) {
+            bugAccumulator.accumulateBug(new BugInstance(this,
+                    "MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT", NORMAL_PRIORITY)
+                    .addClass(this).addMethod(readObject).addString(overridable.getName()), sourceLine);
+            return;
+        }
+        callerReadObjects.put(callee, new CallerInfo(readObject, sourceLine));
+    }
+
     private boolean checkAndRecordCallToOverridable(XMethod caller, XMethod overridable) {
         CallerInfo constructor = getIndirectCallerConstructor(caller);
         if (constructor != null) {
@@ -263,7 +316,14 @@ private boolean checkAndRecordCallToOverridable(XMethod caller, XMethod overrida
                     .addClassAndMethod(clone.method).addString(overridable.getName()), clone.sourceLine);
         }
 
-        if (constructor != null || clone != null) {
+        CallerInfo readObject = getIndirectCallerReadObject(caller);
+        if (readObject != null) {
+            bugAccumulator.accumulateBug(new BugInstance(this,
+                    "MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT", NORMAL_PRIORITY)
+                    .addClassAndMethod(readObject.method).addString(overridable.getName()), readObject.sourceLine);
+        }
+
+        if (constructor != null || clone != null || readObject != null) {
             return false;
         }
 
@@ -274,8 +334,9 @@ private boolean checkAndRecordCallToOverridable(XMethod caller, XMethod overrida
     private boolean checkAndRecordCallBetweenNonOverridableMethods(XMethod caller, XMethod callee) {
         CallerInfo constructor = getIndirectCallerConstructor(caller);
         CallerInfo clone = getIndirectCallerClone(caller);
+        CallerInfo readObject = getIndirectCallerReadObject(caller);
 
-        if (constructor != null || clone != null) {
+        if (constructor != null || clone != null || readObject != null) {
             XMethod overridable = getIndirectlyCalledOverridable(callee);
             if (overridable != null) {
                 if (constructor != null) {
@@ -293,6 +354,13 @@ private boolean checkAndRecordCallBetweenNonOverridableMethods(XMethod caller, X
                             clone.sourceLine);
                 }
 
+                if (readObject != null) {
+                    bugAccumulator.accumulateBug(new BugInstance(this,
+                            "MC_OVERRIDABLE_METHOD_CALL_IN_READ_OBJECT", NORMAL_PRIORITY)
+                            .addClassAndMethod(readObject.method).addString(overridable.getName()),
+                            readObject.sourceLine);
+                }
+
                 return false;
             }
         }
@@ -333,6 +401,10 @@ private CallerInfo getIndirectCallerClone(XMethod callee) {
         return getIndirectCallerSpecial(callee, callerClones);
     }
 
+    private CallerInfo getIndirectCallerReadObject(XMethod callee) {
+        return getIndirectCallerSpecial(callee, callerReadObjects);
+    }
+
     private CallerInfo getIndirectCallerSpecial(XMethod callee, Map<XMethod, CallerInfo> map) {
         return getIndirectCallerSpecial(callee, map, new HashSet<>());
     }