Add new rule set PA_PUBLIC_PRIMITIVE_ATTRIBUTE, PA_PUBLIC_ARRAY_ATTRIBUTE and PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE

CHANGELOG.md

@@ -25,6 +25,7 @@ Currently the versioning policy of this project follows [Semantic Versioning v2.
 * New simple name-based AnnotationMatcher for exclude files (now bug annotations store the class java annotations in an attribute called `classAnnotationNames`). For example, use like <Match><Annotation name="org.immutables.value.Generated" /></Match> in an excludeFilter.xml to ignore classes generated by the Immutable framework. This ignores all class, method or field bugs in classes with that annotation.
 * Added the Common Weakness Enumeration (CWE) taxonomy to the Static Analysis Results Interchange Format (SARIF) report. The short and long description for the CWEs are retrived from a JSON file which is a slimmed down version of the official comprehensive CWE XML from MITRE. The JSON contains information about all CWEs. ([#2410](https://github.com/spotbugs/spotbugs/pull/2410)).
 * 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))
 
 ### Security
 - Disable access to external entities when processing XML ([#2217](https://github.com/spotbugs/spotbugs/pull/2217))

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

@@ -0,0 +1,58 @@
+package edu.umd.cs.findbugs.detect;
+
+import edu.umd.cs.findbugs.*;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcher;
+import edu.umd.cs.findbugs.test.matcher.BugInstanceMatcherBuilder;
+import org.junit.Test;
+
+import static edu.umd.cs.findbugs.test.CountMatcher.containsExactly;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasItem;
+
+public class FindPublicAttributesTest extends AbstractIntegrationTest {
+    private static final String PRIMITIVE_PUBLIC = "PA_PUBLIC_PRIMITIVE_ATTRIBUTE";
+    private static final String MUTABLE_PUBLIC = "PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE";
+    private static final String ARRAY_PUBLIC = "PA_PUBLIC_ARRAY_ATTRIBUTE";
+
+    @Test
+    public void testPublicAttributesChecks() {
+        performAnalysis("PublicAttributesTest.class");
+
+        assertNumOfBugs(PRIMITIVE_PUBLIC, 3);
+        assertNumOfBugs(ARRAY_PUBLIC, 4);
+        assertNumOfBugs(MUTABLE_PUBLIC, 1);
+
+        assertBugTypeAtField(PRIMITIVE_PUBLIC, "attr1", 11);
+        assertBugTypeAtField(PRIMITIVE_PUBLIC, "attr2", 42);
+        assertBugTypeAtField(PRIMITIVE_PUBLIC, "sattr1", 15);
+        assertBugTypeAtField(MUTABLE_PUBLIC, "hm", 20);
+        assertBugTypeAtField(ARRAY_PUBLIC, "items", 28);
+        assertBugTypeAtField(ARRAY_PUBLIC, "nitems", 48);
+        assertBugTypeAtField(ARRAY_PUBLIC, "sitems", 32);
+        assertBugTypeAtField(ARRAY_PUBLIC, "SFITEMS", 34);
+    }
+
+    @Test
+    public void testGoodPublicAttributesChecks() {
+        performAnalysis("PublicAttributesNegativeTest.class");
+        assertNumOfBugs(PRIMITIVE_PUBLIC, 0);
+        assertNumOfBugs(ARRAY_PUBLIC, 0);
+        assertNumOfBugs(MUTABLE_PUBLIC, 0);
+    }
+
+    private void assertNumOfBugs(String bugtype, int num) {
+        final BugInstanceMatcher bugTypeMatcher = new BugInstanceMatcherBuilder()
+                .bugType(bugtype).build();
+        assertThat(getBugCollection(), containsExactly(num, bugTypeMatcher));
+    }
+
+    private void assertBugTypeAtField(String bugtype, String field, int line) {
+        final BugInstanceMatcher bugInstanceMatcher = new BugInstanceMatcherBuilder()
+                .bugType(bugtype)
+                .inClass("PublicAttributesTest")
+                .atField(field)
+                .atLine(line)
+                .build();
+        assertThat(getBugCollection(), hasItem(bugInstanceMatcher));
+    }
+}

spotbugs/etc/findbugs.xml

@@ -671,7 +671,9 @@
                     reports="USC_POTENTIAL_SECURITY_CHECK_BASED_ON_UNTRUSTED_SOURCE"/>
           <Detector class="edu.umd.cs.findbugs.detect.FindAssertionsWithSideEffects" speed="fast"
                     reports="ASE_ASSERTION_WITH_SIDE_EFFECT,ASE_ASSERTION_WITH_SIDE_EFFECT_METHOD"/>
-            <!-- Bug Categories -->
+          <Detector class="edu.umd.cs.findbugs.detect.FindPublicAttributes" speed="fast"
+                    reports="PA_PUBLIC_PRIMITIVE_ATTRIBUTE,PA_PUBLIC_ARRAY_ATTRIBUTE,PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE"/>
+    <!-- Bug Categories -->
            <BugCategory category="NOISE" hidden="true"/>
 
           <!-- Bug Codes -->
@@ -1291,4 +1293,7 @@
                     category="MALICIOUS_CODE"/>
           <BugPattern abbrev="ASE" type="ASE_ASSERTION_WITH_SIDE_EFFECT" category="SECURITY" />
           <BugPattern abbrev="ASE" type="ASE_ASSERTION_WITH_SIDE_EFFECT_METHOD" category="SECURITY" />
+          <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"/>
 </FindbugsPlugin>

spotbugs/etc/messages.xml

@@ -1779,11 +1779,19 @@ factory pattern to create these objects.</p>
       ]]>
     </Details>
   </Detector>
+  <Detector class="edu.umd.cs.findbugs.detect.FindPublicAttributes">
+    <Details>
+      <![CDATA[
+            <p>This detector looks for public attributes that are also written by the methods of the class.</p>
+      ]]>
+    </Details>
+  </Detector>
   <!--
   **********************************************************************
   BugPatterns
   **********************************************************************
    -->
+
   <BugPattern type="JUA_DONT_ASSERT_INSTANCEOF_IN_TESTS">
     <ShortDescription> Asserting value of instanceof in tests is not recommended. </ShortDescription>
     <LongDescription> Assertion of type {0} in {2} at {3} may hide useful information about why a cast may have failed.</LongDescription>
@@ -8820,6 +8828,69 @@ Using floating-point variables should not be used as loop counters, as they are
             ]]>
     </Details>
   </BugPattern>
+  <BugPattern type="PA_PUBLIC_PRIMITIVE_ATTRIBUTE">
+    <ShortDescription>Primitive field is public</ShortDescription>
+    <LongDescription>Primitive field {1} is public and set from inside the class, which makes it too exposed. Consider making it private to limit external accessibility.</LongDescription>
+    <Details>
+      <![CDATA[
+    <p>
+    <a href="https://wiki.sei.cmu.edu/confluence/display/java/OBJ01-J.+Limit+accessibility+of+fields">SEI CERT rule OBJ01-J</a> requires that accessibility to fields must be limited.
+    Otherwise, the values of the fields may be manipulated from outside the class, which may be unexpected or
+    undesired behaviour.
+    In general, requiring that no fields are allowed to be public is overkill and unrealistic. Even
+    the rule mentions that final fields may be public. Besides final fields, there may be other
+    usages for public fields: some public fields may serve as "flags" that affect the behavior of
+    the class. Such flag fields are expected to be read by the current instance (or the current
+    class, in case of static fields), but written by others. If a field is both written by the
+    methods of the current instance (or the current class, in case of static fields) and from the
+    outside, the code is suspicious. Consider making these fields private and provide appropriate
+    setters, if necessary. Please note that constructors, initializers and finalizers are
+    exceptions, if only they write the field inside the class, the field is not considered as
+    written by the class itself.</p>
+    ]]>
+    </Details>
+  </BugPattern>
+  <BugPattern type="PA_PUBLIC_ARRAY_ATTRIBUTE">
+    <ShortDescription>Array-type field is public</ShortDescription>
+    <LongDescription>Array-type field {1} is public, which makes it too exposed. Consider making it private to limit external accessibility.</LongDescription>
+    <Details>
+      <![CDATA[
+    <p>
+    <a href="https://wiki.sei.cmu.edu/confluence/display/java/OBJ01-J.+Limit+accessibility+of+fields">SEI CERT rule OBJ01-J</a> requires that accessibility of fields must be limited.
+    Making an array-type field final does not prevent other classes from modifying the contents of
+    the array. However, in general, requiring that no fields are allowed to be public is overkill
+    and unrealistic. There may be usages for public fields: some public fields may serve as "flags"
+    that affect the behavior of the class. Such flag fields are expected to be read by the current
+    instance (or the current class, in case of static fields), but written by others. If a field is
+    both written by the methods of the current instance (or the current class, in case of static
+    fields) and from the outside, the code is suspicious. Consider making these fields private and
+    provide appropriate setters, if necessary. Please note that constructors, initializers and
+    finalizers are exceptions, if only they write the field inside the class, the field is not
+    considered as written by the class itself.</p>
+    ]]>
+    </Details>
+  </BugPattern>
+  <BugPattern type="PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE">
+    <ShortDescription>Mutable object-type field is public</ShortDescription>
+    <LongDescription>Mutable object-type field {1} is public, which makes it too exposed. Consider making it private to limit external accessibility.</LongDescription>
+    <Details>
+      <![CDATA[
+    <p>
+    <a href="https://wiki.sei.cmu.edu/confluence/display/java/OBJ01-J.+Limit+accessibility+of+fields">SEI CERT rule OBJ01-J</a> requires that accessibility of fields must be limited.
+    Making a mutable object-type field final does not prevent other classes from modifying the
+    contents of the object. However, in general, requiring that no fields are allowed to be public
+    is overkill and unrealistic. There may be usages for public fields: some public fields may
+    serve as "flags" that affect the behavior of the class. Such flag fields are expected to be
+    read by the current instance (or the current class, in case of static fields), but written by
+    others. If a field is both written by the methods of the current instance (or the current
+    class, in case of static fields) and from the outside, the code is suspicious. Consider making
+    these fields private and provide appropriate setters, if necessary. Please note that
+    constructors, initializers and finalizers are exceptions, if only they write the field inside
+    the class, the field is not considered as written by the class itself. In case of object-type
+    fields "writing" means calling methods whose name suggest modification.</p>
+    ]]>
+    </Details>
+  </BugPattern>
   <!--
   **********************************************************************
    BugCodes
@@ -8974,4 +9045,5 @@ Using floating-point variables should not be used as loop counters, as they are
   <BugCode abbrev="PERM">Custom class loader does not call its superclass's getPermissions()</BugCode>
   <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>
 </MessageCollection>

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

@@ -0,0 +1,208 @@
+/*
+ * 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 java.util.Arrays;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import edu.umd.cs.findbugs.SourceLineAnnotation;
+import org.apache.bcel.Const;
+
+import edu.umd.cs.findbugs.BugInstance;
+import edu.umd.cs.findbugs.BugReporter;
+import edu.umd.cs.findbugs.ba.XField;
+import edu.umd.cs.findbugs.ba.XMethod;
+import edu.umd.cs.findbugs.bcel.OpcodeStackDetector;
+import edu.umd.cs.findbugs.util.MutableClasses;
+import org.apache.bcel.classfile.JavaClass;
+import org.apache.bcel.classfile.Method;
+
+public class FindPublicAttributes extends OpcodeStackDetector {
+
+    private static final Set<String> CONSTRUCTOR_LIKE_NAMES = new HashSet<String>(Arrays.asList(
+            Const.CONSTRUCTOR_NAME, Const.STATIC_INITIALIZER_NAME,
+            "clone", "init", "initialize", "dispose", "finalize", "this",
+            "_jspInit", "jspDestroy"));
+
+    private static final List<String> SETTER_LIKE_NAMES = Arrays.asList(
+            "set", "put", "add", "insert", "delete", "remove", "erase", "clear",
+            "push", "pop", "enqueue", "dequeue", "write", "append");
+
+    private final BugReporter bugReporter;
+
+    private final Set<XField> writtenFields = new HashSet<XField>();
+
+    private final Map<XField, SourceLineAnnotation> fieldDefLineMap = new HashMap<XField, SourceLineAnnotation>();
+
+    public FindPublicAttributes(BugReporter bugReporter) {
+        this.bugReporter = bugReporter;
+    }
+
+    @Override
+    public void sawField() {
+        XField field = getXFieldOperand();
+        if (field != null && !fieldDefLineMap.containsKey(field)) {
+            SourceLineAnnotation sla = SourceLineAnnotation.fromVisitedInstruction(this);
+            fieldDefLineMap.put(field, sla);
+        }
+    }
+
+    @Override
+    public void visit(JavaClass obj) {
+        for (Method m : obj.getMethods()) {
+            String methodName = m.getName();
+            // First visit the Constructor and the static initializer to collect the field initialization lines
+            if (Const.CONSTRUCTOR_NAME.equals(methodName) || Const.STATIC_INITIALIZER_NAME.equals(methodName)) {
+                doVisitMethod(m);
+            }
+        }
+    }
+
+    // Check for each statement which writes an own attribute of the class
+    // instance. If the attribute written is public then report a bug.
+    @Override
+    public void sawOpcode(int seen) {
+        // It is normal that classes used as simple data types have a
+        // constructor to make initialization easy.
+        if (isConstructorLikeMethod(getMethodName())) {
+            return;
+        }
+
+        // We do not care for enums and nested classes.
+        if (getThisClass().isEnum() || getClassName().indexOf('$') >= 0) {
+            return;
+        }
+
+        if (seen == Const.PUTFIELD || seen == Const.PUTSTATIC) {
+            XField field = getXFieldOperand();
+            // Do not report the same public field twice.
+            if (field == null || writtenFields.contains(field)) {
+                return;
+            }
+
+            // Only consider attributes of self.
+            if (!field.getClassDescriptor().equals(getClassDescriptor())) {
+                return;
+            }
+
+            if (!field.isPublic()) {
+                return;
+            }
+
+            SourceLineAnnotation sla = fieldDefLineMap.containsKey(field)
+                    ? fieldDefLineMap.get(field)
+                    : SourceLineAnnotation.fromVisitedInstruction(this);
+
+            bugReporter.reportBug(new BugInstance(this,
+                    "PA_PUBLIC_PRIMITIVE_ATTRIBUTE",
+                    NORMAL_PRIORITY)
+                            .addClass(this).addField(field).addSourceLine(sla));
+            writtenFields.add(field);
+        } else if (seen == Const.AASTORE) {
+            XField field = stack.getStackItem(2).getXField();
+            // Do not report the same public field twice.
+            if (field == null || writtenFields.contains(field)) {
+                return;
+            }
+
+            // Only consider attributes of self.
+            if (!field.getClassDescriptor().equals(getClassDescriptor())) {
+                return;
+            }
+
+            if (!field.isPublic()) {
+                return;
+            }
+
+            SourceLineAnnotation sla = fieldDefLineMap.containsKey(field)
+                    ? fieldDefLineMap.get(field)
+                    : SourceLineAnnotation.fromVisitedInstruction(this);
+
+            bugReporter.reportBug(new BugInstance(this,
+                    "PA_PUBLIC_ARRAY_ATTRIBUTE",
+                    NORMAL_PRIORITY)
+                            .addClass(this).addField(field).addSourceLine(sla));
+            writtenFields.add(field);
+        } else if (seen == Const.INVOKEINTERFACE || seen == Const.INVOKEVIRTUAL) {
+            XMethod xmo = getXMethodOperand();
+            if (xmo == null) {
+                return;
+            }
+
+            // Heuristics: suppose that that model has a proper name in English
+            // which roughly describes its behavior, beginning with the verb.
+            // If the verb does not hint that the method writes the object
+            // then we skip it.
+            if (!looksLikeASetter(xmo.getName())) {
+                return;
+            }
+
+            XField field = stack.getStackItem(xmo.getNumParams()).getXField();
+            // Do not report the same public field twice.
+            if (field == null || writtenFields.contains(field)) {
+                return;
+            }
+
+            // Mutable static fields are already detected by MS
+            if (field.isStatic()) {
+                return;
+            }
+
+            // Only consider attributes of self.
+            if (!field.getClassDescriptor().equals(getClassDescriptor())) {
+                return;
+            }
+
+            if (!field.isPublic() || !field.isReferenceType()) {
+                return;
+            }
+
+            if (!MutableClasses.mutableSignature(field.getSignature())) {
+                return;
+            }
+
+            SourceLineAnnotation sla = fieldDefLineMap.containsKey(field)
+                    ? fieldDefLineMap.get(field)
+                    : SourceLineAnnotation.fromVisitedInstruction(this);
+
+            bugReporter.reportBug(new BugInstance(this,
+                    "PA_PUBLIC_MUTABLE_OBJECT_ATTRIBUTE",
+                    NORMAL_PRIORITY)
+                            .addClass(this).addField(field).addSourceLine(sla));
+            writtenFields.add(field);
+        }
+    }
+
+    private static boolean isConstructorLikeMethod(String methodName) {
+        return CONSTRUCTOR_LIKE_NAMES.contains(methodName);
+    }
+
+    private static boolean looksLikeASetter(String methodName) {
+        for (String name : SETTER_LIKE_NAMES) {
+            if (methodName.startsWith(name)) {
+                return true;
+            }
+        }
+        return false;
+    }
+}