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
IH.2.b.b. Declare methods that enforce |
7 Comments
Yozo TODA
The first sentence says "...nonfinal classes", but I think the intention is "nonfinal methods". I revised this sentence.
David Svoboda
Thanks.
Thomas Hawtin
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.
David Svoboda
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.
Yitzhak Mandelbaum
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.
Thomas Hawtin
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.
David Svoboda
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