2

I have a Spring Boot application that uses a CredentialsService class to store credentials as GuardedStrings and return them when requested by other classes.

Where the problem arises is in the fact that we use Checkmarx to scan our code and catch potential issues. Where storage of the usernames/passwords are not a problem anymore, I still have to use a String variable to return the plain text credentials. Checkmarx doesn't like that - especially for passwords.

This is the abbreviated view of the CredentialsService:

@Component
public class CredentialsService {
    
    final ExecutorService executor = Executors.newSingleThreadExecutor();

    private GuardedString customerApiPassword;
    . . . 
    private StringBuilder clearCustomerApiPassword;

    public CredentialsService( . . .
            @Value("${customerapi.pwd}") String customerApiPassword,. . .) {
        
        setCustomerApiPassword(customerApiPassword);
        . . .
    }

    private void setCustomerApiPassword(String customerApiPasswordString) {
        this.customerApiPassword = new GuardedString(customerApiPasswordString.toCharArray());
        this.customerApiPassword.makeReadOnly();        
    }
    
    public String getCustomerApiPasswordNo() {
        
        clearCustomerApiPassword = new StringBuilder();
        customerApiPassword.access(new GuardedString.Accessor() {
            @Override
            public void access(final char[] clearChars) {
                clearCustomerApiPassword.append(clearChars);
            }
        });
        customerApiPassword.dispose();
        System.out.println("DGC: clearCustomerApiPassword is " + clearCustomerApiPassword);
        Runnable clearFromMemory = () -> {
            clearCustomerApiPassword = null;
            System.out.println("DGC: clearCustomerApiPassword is " + clearCustomerApiPassword);
        };
        executor.execute(clearFromMemory);
        return clearCustomerApiPassword.toString();
    }

And then a requester accesses the values it needs with:

    IntegrationApiUtil.setBasicAuthKey(headers, credentialsService.getCustomerApiUsername(), credentialsService.getCustomerApiPassword());

However Checkmarx is still not happy. I use the same approach for storing the GuardedString usernames and passwords and the exact same approach to clearing the Strings that are returned. Checkmarx is fine with the usernames, but it still complains about the passwords:

    Method clearCustomerApiPassword; at line 24 of
src/main/java/com/.../service/CredentialsService.java
defines clearCustomerApiPassword, which is designated to contain user passwords. However, while plaintext
passwords are later assigned to clearCustomerApiPassword, this variable is never cleared from memory.

I have tried all sorts of things - a finalize method to destroy the service after it is last used, a disposeAll method to explicitly set all variables to null and call the garbage collector. With the code above I am creating a separate thread in each get method to set the 'clear' variables to null as I return the value to the requester. While I can confirm that this latest approach does provide the requester with the correct values and also sets the variables to null, nothing seems to satisfy Checkmarx.

Does anyone have any ideas?

Thanks in advance.

D

Roman Canlas
  • 2,045
  • 3
  • 14
  • 19
dgc1222
  • 21
  • 2
  • Really interested if there is an answer for this. Since Java is run in a runtime with a garbage collector, you can never know when a value will be removed from memory (unless you start implementing your own memory management with char arrays maybe?) – knittl Jun 29 '20 at 20:48

2 Answers2

3

Well, you are kinda throwing away all the value of storing passwords in GuardedString by returning/transporting them as regular String.

Don't know a lot about Checkmarx, but it's just a code scanning tool, so it's easy to fool. I suggest actually fixing the problems, instead of trying to sweep them under the rug.

  1. Notice that GuardedString constructor accepts char[], not a String. That's the first problem - you should carry your password from the source up to this point as a char[] - more about it here.
  2. Don't return String to your consumer - return the GuardedString or at least a char[].
  3. Depends on what consumers you are targeting with this class/library, but try to provide a way for them to access the actual passwords for as short time as possible, and clearning the char[] after usage (in a way that consumer will not have to do that himself, since he can forget)
Shadov
  • 5,061
  • 2
  • 16
  • 27
2

Once you put sensitive data into an immutable object like String, the data will remain in the memory for a long time. You can release the variable, but even without a physical reference the value will still sit in the memory. You can run GC, it will still be there. The only thing that would help would be a creation of another variable using the same memory space and overriding the value.

Long story short: As long as you put your password in a String, Checkmarx will complain.

You have two things you can do:

  • you either rely on char[] only and clear the array once used,
  • or use a String value if you are forced to and request a special exception for your case.
Marek Puchalski
  • 2,105
  • 1
  • 17
  • 27