In the previous article, I was describing the creation of a new FindBugs detector for hard-coded passwords and cryptographic keys. I also mentioned some imperfections and I have decided to learn more about FindBugs and improve the detection.

Java virtual machine has a stack architecture – operands must be pushed on the stack before method is invoked, given number of stack values is consumed during invocation and produced return value (if any) is pushed subsequently. My detector class extends OpcodeStackDetector, which implements abstract interpretation technique to collect approximated information about values at the operand stack for each code location. These pieces of information (usually called facts) are kept only for those locations, where a derived value of the fact does not depend on the preceding control flow (for example, the value is the same for each possible branch executed in earlier conditional statements).

One of the facts available for stack values is the actual value of a number or String (related to constant propagation performed by compilers during optimization). We can use this to detect hard-coded values – known constant means hard-coded value. However, we also need to track other other data types besides Strings (numbers can be ignored) to detect passwords in a char array and hard-coded keys. In addition, there is one more issue with this approach…

Tracking concrete values is unnecessarily complicated and the value often becomes unknown – we only need to know whether the value is constant, not which constant is on the stack. Consider a piece of code like this:

private Connection getConnection(String user) {
    String password;
    if ("root".equals(user)) {
        password = "superSecurePassword";
    } else {
        password = "differentPassword";
    }
    return DriverManager.getConnection(DB_URL, user, password);
}

The constant value in the password variable is known inside both branches, but these values are forgotten after the conditional statement, since the values differ. For this reason, weakness like this was not reported by the previous version of the detector nor by the original FindBugs detector for constant database passwords. Even if there is only one possible constant, analysis can fail because of null values, see this code (looking a bit unreal, but demonstrating the problem):

String pwd = null;
if (shouldConnect()) {
    pwd = "hardcoded";
}
if (pwd != null) {
    Connection connection = DriverManager.getConnection(url, user, pwd);
    // some code working with database
}

We can easily see that the password variable has always value “hardcoded”, but the performed analysis is linear and the fact is forgotten right after the first conditional statement. Second condition cannot return the forgotten constant back and weakness is not detected again.

Fortunately, these issues can be solved by setting and reading user value fact, which OpcodeStackDetector allows (if annotation CustomUserValue is added to the extending class). Our fact has only one value for hard-coded stack items or it is null to indicate unknown state (default). We can periodically check, whether a value on the stack is a known constant or null and set the user value for it if it is, propagation is done automatically. If then analysis merges facts from different control flow branches with different constants (or null), user value is the same and not reset to default. Custom user value is also used to mark hard-coded passwords and keys with the other password and key data types, detection of those objects remains similar as in the previous version of the detector. Weakness is reported if sink parameter has non-null user value and stack value is not null (null passwords are not considered to be hard-coded).

The detector is using proper flow analysis after this improvement; however, it is restricted to a method scope and hard-coded values in fields are reported only if used in the same class. Inter-method and inter-class analysis is a future challenge, but I have kept reporting of hard-coded fields with suspicious names and unknown sink not to miss important positives. In contrast to the previous version, these fields are reported with lower priority and only if not reported before using proper flow analysis technique to prevent duplicate warning. Moreover, all fields are reported in a single warning for a class to make possible false positives less distracting.

Another improvement is the possibility to configure more method parameters as password or key sinks. If more than one parameter is hard-coded, only single warning is produced and the parameters are mentioned in the detailed message. The last important change is that hard-coded cryptographic keys are reported in a separated bug pattern since both hard-coded passwords and keys have a different CWE identifier (259 and 321) and are equally important. Decision between reported warnings is done automatically based on the data types of hard-coded parameters.

I have tested the detector with Juliet Test Suite and using proper analysis it can reveal both types of weakness in 17 flow variants (out of 37) and all sink variants with no false positives. Original FindBugs detector reveals weaknesses in 10 flow variants and only for database passwords, other password methods and hard-coded keys are not detected in any variant.

You can see the detector class on GitHub. Happy coding with no hard-coded passwords!

Comments