Versions Compared

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

It is difficult to control how data members declared public or protected are accessedInvariants cannot be enforced for public nonfinal fields or for final fields that reference a mutable object. A protected member of an exported (non-final) class represents a public commitment to an implementation detail. Attackers can manipulate such members in unexpected ways. Use wrapper accessor methods to expose class members beyond the package in which their class is declared. The use of wrapper methods enables appropriate monitoring and control of the modification of data members (for example, by defensive copying, validating input, and logging). The wrapper methods can preserve class invariantsfields to violate class invariants, or they may be corrupted by multiple threads accessing them concurrently [Bloch 2008]. As a result, fields must be declared private or package-private.

Noncompliant Code Example (Public Primitive Field)

In this noncompliant code example, the data member total keeps track of total field tracks the total number of elements as they are added to and removed from a container using the methods add() and remove(), respectively.

Code Block
bgColor#FFCCCC

public class Widget {
  public int total; // Number of elements

  void add() {
    if (total < Integer.MAX_VALUE) {      
      total++;
      // ...
    } else {
      throw new ArithmeticException("Overflow");
    }
  }

  void remove() {  
    if (total > 0) {      
      total--;
      // ...
    } else {
      throw new ArithmeticException("Overflow");
    }
  }
}

...

As a {{public}} data member, {{total}} can be altered by external code, independent of the {{add()}} and {{remove()}} methods. It is a bad practice to expose both mutable and immutable fields from a {{public}} class \[[Bloch 2008|AA. Bibliography#Bloch 08]\]public field, total can be altered by client code independently of the add() and remove() methods.

Compliant Solution (

...

Private Primitive Field)

Accessor methods provide controlled access to fields outside of the package in which their class is declared. This compliant solution declares total as private and provides a public accessor so that the required member can be accessed beyond the current package. The add() and remove() methods modify its value without violating any while preserving class invariants.Note that accessor functions should be careful about providing references to private mutable objects; see OBJ09-J. Defensively copy private mutable class members before returning their references for details. 

Code Block
bgColor#ccccff

public class Widget {
  private int total; // Declared private

  public int getTotal () {
    return total;
  }

  // definitionsDefinitions for add() and remove() remain the same
}

It is good practice to use methods such as add(), remove(), and getTotal(), to manipulate the private internal state, because the methods Accessor methods can perform additional functions, such as input validation and security manager checks, prior to before manipulating the state. 

Noncompliant Code Example (Public Mutable Field)

Programmers often incorrectly assume that declaring a field or variable final makes the referenced object immutable. Declaring variables that have a primitive type final does prevent changes to their values after initialization (by normal Java processing). However, when the field has a reference type, declaring the field final only makes the reference itself immutable. The final clause has no effect on the referenced object. According to The Java Language Specification§4.12.4, "final Variables" [JLS 2015],

If a final variable holds a reference to an object, then the state of the object may be changed by operations on the object, but the variable will always refer to the same object.

This noncompliant code example shows a static example declares a static final mutable hash map with public accessibility. :

Code Block
bgColor#FFCCCC

public static final HashMap<Integer, String> hm = new HashMap<Integer, String>();

Compliant Solution (

...

Private Mutable Fields)

Mutable data members that are static fields must be declared private.:

Code Block
bgColor#ccccff

private static final HashMap<Integer, String> hm = new HashMap<Integer, String>();

public static String getElement(int key) { 
  return hm.get(key);
}

Depending on the required functionality, wrapper methods may retrieve either a reference to the Hashmap, accessor methods may return a copy of the HashMap, or a value contained by the Hashmap HashMap. This compliant solution adds a wrapper method to return an accessor method that returns the value of an element given its index in the Hashmap.

Exceptions

key in the HashMap. Make sure that you do not return references to private mutable objects from accessor methods (see OBJ05-J. Do not return references to private mutable class members for details).

Noncompliant Code Example (Public Final Array)

A nonzero-length array is always mutable. Declaring a public final array is a potential security risk as the array elements may be modified by a client. 

Code Block
bgColor#ffcccc
public static final String[] items = {/* ... */};

Declaring the array reference final prevents modification of the reference but does not prevent clients from modifying the contents of the array.

Compliant Solution (Index Getter)

This compliant solution declares a private array and provides public methods to get individual items and array size:

Code Block
bgColor#ccccff
private static final String[] items = {/* ... */};

public static final String getItem(int index) {
  return items[index];
}

public static final int getItemCount() {
  return items.length;
}

Providing direct access to the array objects themselves is safe because String is immutable.

Compliant Solution (Clone the Array)

This compliant solution defines a private array and a public method that returns a copy of the array:

Code Block
bgColor#ccccff
private static final String[] items = {/* ... */};

public static final String[] getItems() {
  return items.clone();
}

Because a copy of the array is returned, the original array values (the references to the String objects) cannot be modified by a client. Note that a manual deep copy could be required when dealing with arrays of objects. This generally happens when the objects do not export a clone() method (see OBJ06-J. Defensively copy mutable inputs and mutable internal components for more information).

As before, this method provides direct access to the array objects themselves, but this is safe because String is immutable. If the array contained mutable objects, the getItems() method should return an array of cloned objects instead.

Compliant Solution (Unmodifiable Wrappers)

This compliant solution constructs a public immutable list from the private array. It is safe to share immutable objects without risk that the recipient can modify them [Mettler 2010]. This example is safe because String is immutable.

Code Block
bgColor#ccccff
private static final String[] items = { ... };

public static final List<String> itemsList =
  Collections.unmodifiableList(Arrays.asList(items));

Neither the original array values nor the public list can be modified by a client. For more details about unmodifiable wrappers, refer to OBJ56-J. Provide sensitive mutable classes with unmodifiable wrappers.

Exceptions

OBJ01-J-EX0: Fields with no associated behavior or invariants can be public. According to Sun's Code Conventions document [Conventions 2009]: Wiki Markup*OBJ00-EX0:* According to Sun's Code Conventions document \[[Conventions 2009|AA. Bibliography#Conventions 09]\]

One example of appropriate public instance variables is the case where the class is essentially a data structure, with no behavior. In other words, if you would have used a struct instead of a class (if Java supported struct), then it's appropriate to make the class's instance variables public.

Wiki Markup*OBJ00OBJ01-J-EX1:* "if a class is Fields in a package-private class or is a {{private}} nested class, there is nothing inherently wrong with exposing its data fields&#xE2;&euro;&rdquo;assuming they do an adequate job of describing the abstraction provided by the class. This approach generates less visual clutter than the accessor-method approach, both in the class definition and in the client code that uses it" \[[Bloch 2008|AA. Bibliography#Bloch 08]\]. This exception applies to both mutable and immutable fields.in a private nested class may be pubic or protected. There is nothing inherently wrong with declaring fields to be public or protected in these cases. Eliminating accessor methods generally improves the readability of the code both in the class definition and in the client [Bloch 2008].

OBJ01-JOBJ01-EX2: Static final fields that contain mathematical or reference immutable constants may be declared public or protected.

Risk Assessment

Failing to declare data members private can break encapsulation. limit field accessibility can defeat encapsulation, allow attackers to manipulate fields to violate class invariants, or allow these fields to be corrupted as the result of concurrent accesses from multiple threads.

Rule

Guideline

Severity

Likelihood

Remediation Cost

Priority

Level

OBJ01-J

medium

Medium

likely

Likely

medium

Medium

P12

L1

Automated Detection

Detection of public and protected data members fields is trivial; heuristic detection of the presence or absence of getter and setter wrapper accessor methods is straightforward. However, simply reporting all detected cases without suppressing those cases covered by the exceptions to this rule would produce many excessive false positives and appears unlikely to be satisfactory. Sound detection and application of the exceptions to this guideline appears to be rule is infeasible; however, heuristic techniques may be useful.

Related Vulnerabilities

...

ToolVersionCheckerDescription
SonarQube
Include Page
SonarQube_V
SonarQube_V
S2386

Mutable fields should not be "public static"

Implemented for public static array, Collection, Date, and awt.Point members.


Related Guidelines

...

...

Wiki Markup
\[[SCG 2007|AA. Bibliography#SCG 07]\] Guideline 3-2 Define wrapper methods around modifiable internal state

Bibliography

Wiki Markup
\[[Bloch 2008|AA. Bibliography#Bloch 08]\] Items 13: Minimize the accessibility of classes and members; 14: In public classes, use accessor methods, not public fields
\[[JLS 2006|AA. Bibliography#JLS 06]\] Section 6.6, Access Control
\[[Long 2005|AA. Bibliography#Long 05]\] Section 2.2, Public Fields

CWE-766, Critical Variable Declared Public

Secure Coding Guidelines for Java SE, Version 5.0

Guideline 6-8 / MUTABLE-8: Define wrapper methods around modifiable internal state

Bibliography

[Bloch 2008]

Item 13, "Minimize the Accessibility of Classes and Members"
Item 14, "In Public Classes, Use Accessor Methods, Not Public Fields"

[Conventions 2009]

[Core Java 2004]

Chapter 6, "Interfaces and Inner Classes"

[JLS 2015]

§4.12.4, "final Variables"
§6.6, "Access Control"

[Long 2005]

Section 2.2, "Public Fields"

[Mettler 2010]

Class Properties for Security Review in an Object-Capability Subset of Java


...

Image Added Image Added Image AddedOBJ00-J. Limit extensibility of classes and methods with invariants to trusted subclasses only      04. Object Orientation (OBJ)      OBJ02-J. Never conflate immutability of a reference with that of the referenced object