The java.security.AccessController
class is part of Java's security mechanism; it is responsible for enforcing the applicable security policy. This class's static doPrivileged()
method executes a code block with a relaxed security policy. The doPrivileged()
method stops permissions from being checked further down the call chain.
Consequently, any method that invokes doPrivileged()
must assume responsibility for enforcing its own security on the code block supplied to doPrivileged()
. Likewise, code in the doPrivileged()
method must not leak sensitive information or capabilities.
For example, suppose that a web application must maintain a sensitive password file for a web service and also must run untrusted code. The application could then enforce a security policy preventing the majority of its own code—as well as all untrusted code—from accessing the sensitive file. Because it must also provide mechanisms for adding and changing passwords, it can call the doPrivileged()
method to temporarily allow untrusted code to access the sensitive file. In this case, any privileged block must prevent all information about passwords from being accessible to untrusted code.
Noncompliant Code Example
In this noncompliant code example, the doPrivileged()
method is called from the openPasswordFile()
method. The openPasswordFile()
method is privileged and returns a FileInputStream
for the sensitive password file. Because the method is public, it could be invoked by an untrusted caller.
public class PasswordManager { public static void changePassword() throws FileNotFoundException { FileInputStream fin = openPasswordFile(); // Test old password with password in file contents; change password, // then close the password file } public static FileInputStream openPasswordFile() throws FileNotFoundException { final String password_file = "password"; FileInputStream fin = null; try { fin = AccessController.doPrivileged( new PrivilegedExceptionAction<FileInputStream>() { public FileInputStream run() throws FileNotFoundException { // Sensitive action; can't be done outside privileged block FileInputStream in = new FileInputStream(password_file); return in; } }); } catch (PrivilegedActionException x) { Exception cause = x.getException(); if (cause instanceof FileNotFoundException) { throw (FileNotFoundException) cause; } else { throw new Error("Unexpected exception type", cause); } } return fin; } }
Compliant Solution
In general, when any method containing a privileged block exposes a field (such as an object reference) beyond its own boundary, it becomes trivial for untrusted callers to exploit the program. This compliant solution mitigates the vulnerability by declaring openPasswordFile()
to be private. Consequently, an untrusted caller can call changePassword()
but cannot directly invoke the openPasswordFile()
method.
public class PasswordManager { public static void changePassword() throws FileNotFoundException { // ... } private static FileInputStream openPasswordFile() throws FileNotFoundException { // ... } }
Compliant Solution (Hiding Exceptions)
The previous noncompliant code example and the previous compliant solution throw a FileNotFoundException
when the password file is missing. If the existence of the password file is itself considered sensitive information, this exception also must be prevented from leaking outside the trusted code.
This compliant solution suppresses the exception, leaving the array to contain a single null value to indicate that the file does not exist. It uses the simpler PrivilegedAction
class rather than PrivilegedExceptionAction
to prevent exceptions from propagating out of the doPrivileged()
block. The Void
return type is recommended for privileged actions that do not return any value.
class PasswordManager { public static void changePassword() { FileInputStream fin = openPasswordFile(); if (fin == null) { // No password file; handle error } // Test old password with password in file contents; change password } private static FileInputStream openPasswordFile() { final String password_file = "password"; final FileInputStream fin[] = { null }; AccessController.doPrivileged(new PrivilegedAction<Void>() { public Void run() { try { // Sensitive action; can't be done outside // doPrivileged() block fin[0] = new FileInputStream(password_file); } catch (FileNotFoundException x) { // Report to handler } return null; } }); return fin[0]; } }
Risk Assessment
Returning references to sensitive resources from within a doPrivileged()
block can break encapsulation and confinement and can leak capabilities. Any caller who can invoke the privileged code directly and obtain a reference to a sensitive resource or field can maliciously modify its elements.
Rule | Severity | Likelihood | Remediation Cost | Priority | Level |
---|---|---|---|---|---|
SEC00-J | Medium | Likely | High | P6 | L2 |
Automated Detection
Identifying sensitive information requires assistance from the programmer; fully automated identification of sensitive information is beyond the current state of the art.
Assuming user-provided tagging of sensitive information, escape analysis could be performed on the doPrivileged()
blocks to prove that nothing sensitive leaks out from them. Methods similar to those used in thread-role analysis could be used to identify the methods that must, or must not, be called from doPrivileged()
blocks.
Related Guidelines
CWE-266, Incorrect Privilege Assignment | |
Guideline 9-3 / ACCESS-3: Safely invoke |
Android Implementation Details
The java.security
package exists on Android for compatibility purposes only, and it should not be used.
Bibliography
[API 2014] | |
Section 6.4, " |
14 Comments
Thomas Hawtin
Using the return value from doPrivileged return value is better than using a hack to return a value. The examples swallow exceptions, but a form of doPrivileged allows exception to be wrapped.
Dhruv Mohindra
The return value hack may be removed, it was just there to demonstrate how one can return things from doPrivileged when the return value is not a single statement/method call.
For exception wrapping, I could not compile the above code. I had to add a throws to
public InputStream run() throws FileNotFoundException
and comment out});
. Is this desired or am I missing something? I suspect this would again swallow exceptions.On a sidenote, starting and ending tags {code} can be used to denote source code...
Thomas Hawtin
Sorry, yes the run method does need to declare any exceptions within the method. It could be just declared as throws Exception, but that isn't very helpful. In PrivilegedExceptionAction it is declared as
.
Dhruv Mohindra
I added a CS as per your suggestion, though with one modification - only throw the exception(s) when no sensitive information (such as file path) can be revealed in the process.
Dean Sutherland
Fred Long and I observe that the text of this guideline discusses avoiding leaks of sensitive information out of doPrivileged blocks. But the title of the guideline talks about guarding against untrusted invocation -- a totally different question. Should we move the text to a different guideline, or should we change the title?
Robert Seacord
I was just having similar thoughts. My take on this guideline is that it is really about "capabilities" and that it should be titled "SEC02-J. Guard doPrivileged blocks against untrusted invocation". The example used to illustrate the risks of untrusted invocation is leaking of sensitive information, but the way the rule is currently worded it does come across too strongly as if this were the reason for the rule.
Dhruv Mohindra
For doPrivileged code, untrusted invocations are very much fine but the code having the particular permissions (and exercising doPrivileged) should ensure that any untrusted code is unable to directly manipulate it (this includes returning handles etc). Some public wrapper (changePassword()) should be used which can achieve the desired functionality such as changing the password as an "independent task". The leaking of sensitive information or direct manipulation of sensitive files is an example and should not be in the title.
I think the second CS is just informative. It just demonstrates the use of PrivilegedExceptionAction instead of a PrivilegedAction. It can be summed up in a line without having to go into gory details.
Robert Seacord (Manager)
I don't see any logging in the logging exceptions compliant solution, so I'm not at all sure what is going on there.
There are a lot of other disconnects in this rule, to the point where I'm thinking it should be discarded. The intro paragraph lacks normative text except in the title. The "example" in the intro talks about an apparently different problem.
David Svoboda
Bumped back up to review2. The intro is clearer, and the code now supports the intro. (previously it was trying to show off PrivilegedAction and PrivilegedExceptionAction, which is informative, but unnecessary to the actual rule.)
David Svoboda
Thomas Hawtin says:
David Svoboda
For production code, I agree: returning the FileInputStream is a fine alternative to filling a final array...that coding practice is showcased in an earlier code sample. When I reviewed this rule several months ago, I decided to leave the 2nd CS filling the final array instead, to show that it is a perfectly fine (though distinct) coding practice.
Wrt generics, we do have a rule against relying on raw types. I've modified the CS to use
Void
, as suggested.Masaki Kubo
In the second CCE, where is the "Void" (not void) defined in the code?
David Svoboda
The
Void
datatype is part of the Java standard library, and it was created specifically to address this problem, see:http://download.oracle.com/javase/6/docs/api/java/lang/Void.html
Masaki Kubo
Thanks for the info, David! Domo.