MSC03-J. Never hard code sensitive information

We could possibly have another rule - "Do not store sensitive information in immutable objects", such as a password in a String object (as shown in this rule). The NCCE/CS will be the same as this rule so am unsure if simply replicating it will be a good idea.

Something wrong about the last Compliant Solution:

return DriverManager.getConnection(
      "jdbc:mysql://localhost/dbName",
      username.toString(), password.toString());
for (int i = username.length - 1; i >= 0; i--) { 
  username[i] = 0;
}
for (int i = password.length - 1; i >= 0; i--) { 
  password[i] = 0;
}

The last two for loops will never be executed after the return statement. Should we change it to use try-finally block?

It would be interesting to see the automated detection section include reference to Sonar rules

http://docs.codehaus.org/display/SONAR/Extending+Coding+Rules

I recently wrote a stand-alone batch app in Java.  I used Spring's framework to read an encrypted password and connect to the database.  I stored the password in an environment variable set at login for a "nameless id" (basically, an ID used only for that system) and set the file privs on the .profile file to only be read by the user (like chmod 700 or something like that).

That is as secure as I needed to get for that application.  The only person who could get the password is a person with the password for that nameless ID.  Even then, they'd have to be smart enough to look for the encrypted password in the environment variables.  Not impossible but not a likely place for it either.  Finally, they'd have to write code to decrypt it which wouldn't be that hard either if they had access to the decrypting source in the source code repository–probably something they WOULD have if they knew the password to the nameless ID (thinking a rogue developer or something).

Combining this technique with others described here would be pretty secure as long as you maintain tight control over who knows the faceless id.

Code looks like this:

spring config file:

-<bean class="xxx.lobsb.util.crypt.DecryptingDataSource" id="dataSource" destroy-method="close"><property value="oracle.jdbc.driver.OracleDriver" name="driverClassName"/>
<!--Restart IDEA after setting these OS level env vars:-->
<property value="${DB_URL}" name="url"/><property value="${DB_USER_ID}" name="username"/><property value="${LOBSB_DB_PWD}" name="password"/>
<!-- Set in OS env var -->
</bean>

DecryptingDataSource.java:

package xxx.lobsb.util.crypt;

import java.sql.Connection;
import java.sql.SQLException;

import org.apache.commons.dbcp.BasicDataSource;

public class DecryptingDataSource extends BasicDataSource {

public Connection getConnection() {
Connection conn = null;
try {
this.setPassword(ProtectedConfigFile.decrypt(password));
conn = getConnection(username, password);
}catch (Exception e) {
System.err.println("GeneralSecurityException thrown: " + e.getMessage());
}

return conn;
}

public Connection getConnection(String userID, String password) {
// String decryptedPassword = null;
Connection conn = null;

try {
if(password.endsWith("=")) {
this.setPassword(ProtectedConfigFile.decrypt(password));
}

conn = super.getConnection();
}catch (Exception e) {
System.err.println("GeneralSecurityException thrown: " + e.getMessage());
}

return conn;
}

public boolean isWrapperFor(Class<?> iface) throws SQLException {
throw new SQLException("Not implemented.");
}
public <T> T unwrap(Class<T> iface) throws SQLException {
throw new SQLException("Not implemented.");
}

}

I would have thought that this was more severe than MSC02-J, as it gives you system access directly.

Also severity on the index page for this item does not match: 49. Miscellaneous (MSC).

The stream handling in the first Compliant Solution is completely below par. No try with resources, closing streams, correct buffer handling, defined character encoding etc.

PMD (since v6.4.0) has included a security rule for identifying hard coded keys that are passed to javax.crypto.spec.SecretKeySpec. link

Suggestion: Add the following before or after the existing PMD 1.0 row in the "Automated Detection" table.

| PMD | 6.4.0 | HardCodedCryptoKey | Identifies if a hard coded key is passed to javax.crypto.spec.SecretKeySpec. Rule is not part of the default/quickstart rule set as of 6.21.0 |

"user name" (as two words) is listed three times in the document.

Should it be one word?

Suggestion:

Change title from "Never hard code sensitive information" to "Do not hard code sensitive information" to be consistent with the titles of other rules.

Ahmed,

Thank you for the feedback and recommendations (PMD, user name, and title).  Because these aren't critical errors, or significant updates to the primary normative text of the rules, we aren't going to update them for these suggestions at this time.  But, the updates are effectively now a part of the record of the rule in its comments.  When we do a mass update in the future, we will review the comments and make changes accordingly.  In particular, that might include updating the automated detection information.  We will also review our use of terms like username and user name throughout the standard, and renaming titles.  In the case of user name, "user name" is more common in the prose description through the standard, but the space needs to be removed to create a token in the code.  If you continue to make comments like these, that are more quality improvements, we may not make the changes immediately.  But, we'll consider them when we perform the next general update.

Cheers.