Nonfinal member methods that perform security checks can be compromised when a malicious subclass overrides the methods and omits the checks. Consequently, such methods must be declared private or final to prevent overriding.

Noncompliant Code Example

This noncompliant code example allows a subclass to override the readSensitiveFile() method and omit the required security check:

public void readSensitiveFile() {
  try {
    SecurityManager sm = System.getSecurityManager();
    if (sm != null) {  // Check for permission to read file
      sm.checkRead("/temp/tempFile");
    }
    // Access the file
  } catch (SecurityException se) {
    // Log exception
  }
}

Compliant Solution

This compliant solution prevents overriding of the readSensitiveFile() method by declaring it final:

public final void readSensitiveFile() {
  try {
    SecurityManager sm = System.getSecurityManager();
    if (sm != null) {  // Check for permission to read file
      sm.checkRead("/temp/tempFile");
    }
    // Access the file
  } catch (SecurityException se) {
    // Log exception
  }
}

Compliant Solution

This compliant solution prevents overriding of the readSensitiveFile() method by declaring it private:

private void readSensitiveFile() {
  try {
    SecurityManager sm = System.getSecurityManager();
    if (sm != null) {  // Check for permission to read file
      sm.checkRead("/temp/tempFile");
    }
    // Access the file
  } catch (SecurityException se) {
    // Log exception
  }
}

Exceptions

MET03-J-EX0: Classes that are declared final are exempt from this rule because their member methods cannot be overridden.

Risk Assessment

Failure to declare a class's method private or final affords the opportunity for a malicious subclass to bypass the security checks performed in the method.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

MET03-J

Medium

Probable

Medium

P8

L2

Android Implementation Details

On Android, System.getSecurityManager() is not used, and the use of a security manager is not exercised. However, an Android developer can implement security-sensitive methods, so the principle may be applicable on Android.

Bibliography

[Ware 2008]

IH.2.b.b. Declare methods that enforce SecurityManager checks final—especially in non-final classes

 


7 Comments

  1. The first sentence says "...nonfinal classes", but I think the intention is "nonfinal methods". I revised this sentence.

  2. I don't understand this rule. Any method that calls this method will also indirectly be doing a security check, so should that also be private or final?

    More reasonably, methods like java.lang.Thread.checkAccess are there to be called as a guard but don't perform any action. Clearly they should not be overridable, if a subclass can do anything useful.

    1. Now I'm confused. Thread.checkAccess() certainly does something; it calls SecurityManager.checkAccess(). For that reason it is correctly marked 'final', so an attacker cannot override it with a method that does no check.

      Obviously we don't want to require all methods to be private or final that might eventually call a method that performs a security check, the question being how far 'up the chain' the private/final requirement goes? Prob the best answer is that private/final is required for any method that provides a security guarantee.

      IOW if your method invokes Thread.checkAccess() and makes guarantees as to its success (eg it returns true iff success), it needs private/final. OTOH if your method invokes Thread.checkAccess(), and does something sensitive if it succeeds, but returns nothing indicating if it succeeded, your method may be overridden. Its overridable b/c the child method might achieve the same ends by other means, and legitimately bypass Thread.checkAccess() entirely.

      1. David -- agreed. I would only add that the rule as stated is checkable, whereas the rule your sketching is subjective, and therefore not possible to check (or even properly call a "rule"). It might be an idea to mention the generalization as a "guideline" or somesuch.

      2. If a method is defined to do anything useful at all, it is going to have some kind contract (I guess that's a tautology). So a maliciously overridden version can break the contract and do something "surprising". Doesn't need to be about the specific SecurityManager checking.

        The important thing for client code is not to trust overridable methods on objects that may be malicious. And that's kind of transitive.

        1. I think your argument suggests that this rule is a specific instance of this more general rule:

          OBJ00-J. Limit extensibility of classes and methods with invariants to trusted subclasses only