Programs must comply with the principle of least privilege not only by providing privileged blocks with the minimum permissions required for correct operation (see SEC50-J. Avoid granting excess privileges) but also by ensuring that privileged code contains only those operations that require increased privileges. Superfluous code contained within a privileged block must operate with the privileges of that block, increasing the attack surface.
Noncompliant Code Example
This noncompliant code example contains a changePassword() method that attempts to open a password file within a doPrivileged block and performs operations using that file. The doPrivileged block also contains a superfluous System.loadLibrary() call that loads the authentication library.
public void changePassword(String currentPassword, String newPassword) {
final FileInputStream f[] = { null };
AccessController.doPrivileged(new PrivilegedAction() {
public Object run() {
try {
String passwordFile = System.getProperty("user.dir") + File.separator + "PasswordFileName";
f[0] = new FileInputStream(passwordFile);
// Check whether oldPassword matches the one in the file
// If not, throw an exception
System.loadLibrary("authentication");
} catch (FileNotFoundException cnf) {
// Forward to handler
}
return null;
}
}); // End of doPrivileged()
}
This example violates the principle of least privilege because an unprivileged caller could also cause the authentication library to be loaded. An unprivileged caller cannot invoke the System.loadLibrary() method directly because this could expose native methods to the unprivileged code [SCG 2010]. Furthermore, the System.loadLibrary() method checks only the privileges of its immediate caller, so it should be used only with great care. For more information, see SEC52-J. Do not expose methods that use reduced-security checks to untrusted code.
Compliant Solution
This compliant solution moves the call to System.loadLibrary() outside the doPrivileged() block. Doing so allows unprivileged code to perform preliminary password reset checks using the file but prevents it from loading the authentication library.
public void changePassword(String currentPassword, String newPassword) {
final FileInputStream f[] = { null };
AccessController.doPrivileged(new PrivilegedAction() {
public Object run() {
try {
String passwordFile = System.getProperty("user.dir") + File.separator + "PasswordFileName";
f[0] = new FileInputStream(passwordFile);
// Check whether oldPassword matches the one in the file
// If not, throw an exception
} catch (FileNotFoundException cnf) {
// Forward to handler
}
return null;
}
}); // End of doPrivileged()
System.loadLibrary("authentication");
}
The loadLibrary() invocation could also occur before preliminary password reset checks are performed; in this example, it is deferred for performance reasons.
Applicability
Minimizing privileged code reduces the attack surface of an application and simplifies the task of auditing privileged code.
Automated Detection
| Tool | Version | Checker | Description |
|---|---|---|---|
| Parasoft Jtest | 2023.1 | CERT.SEC51.PCL | Limit the number of lines in "privileged" code blocks |
Bibliography
9 Comments
David Svoboda
I wish the code samples didn't use this loadLibrary() function, as it is totally superfluous.
Robert Seacord
The
AccessController.doPrivilegedblock looks a lot different then the one used in SEC00-J. Do not allow privileged blocks to leak sensitive information across a trust boundary.Which version is correct?
David Svoboda
The difference I see is that SEC00's NCCE uses the PrivilegedExceptionAction class, while this rule uses PrivilegedAction. According to the javadocs for PrivilegedExceptionAction:
Robert Seacord
OK, thanks.
Robert Seacord
I'm trying to decide if the compliant solution violates SEC00-J. Do not allow privileged blocks to leak sensitive information across a trust boundary by leaking f[0] outside of the doPrivileged block. We had this same discussion in Mexico City. I sort of feel like this compliant solution might be considered "controversial".
David Svoboda
Clearly the CS intends to send the FileInputStream out of the doPrivileged block, so if this violates SEC00-J, it would be a design error rather than a coding error. I suppose we can argue that the FileInputStream is not sensitive wrt the trust boundary imposed by doPrivileged. Most likely we don't want the FileInputStream to escape a larger trust boundary (that is bigger than the code example), but it's ok to escape the doPrivileged one.
Robert Seacord
The problem with this example is that there are two competing drivers. One is to reduce the lines of code that need to be audited. The other is to not leak a file handle outside of a privileged block because the file handle provides access to a privileged resource, and this becomes much less apparent once the file handle is no longer in the privileged block. It now might not receive the same level of scrutiny as privileged code might get, but it should. It is also more likely that someone who is maintaining this code will lose track of the fact that this handle provides access to a restricted resource.
In the C/Unix model it seems safer to drop privileges as soon as you can. Java has these privileged blocks, that want to contain things.
Dhruv Mohindra
As discussed offline we cannot forbid leaking the handle outside the privileged block. The following statement may be eliminated
"The open
FileInputStream f[0]must not be allowed to escape out of the privileged block"I think we could also make it explicit that an expected caller of
changePassword()must have the necessary privileges to also load the library (that is required for the CS to work without a security exception). A second caller (possibly untrusted as well) lacking the privileges would not be able to load the library because the library is being loaded outside the doPrivileged block in CS.A better example would be - any code (including untrusted code) is allowed to load a password reset library by using the doPrivileged block but only code having the permissions to change the password is allowed to open the file. So we should move the code that opens the file outside the privileged block in CS and move the load library code in the doPrivileged - basically switch the two. Comments?
Dhruv Mohindra
I have edited the guideline and somewhat refuted the last paragraph of my comment above. Mostly because of SEC53-J which can also be cited instead of SCG 2010.