Passwords should be wiped from memory

[danielgrahl]

Entry.getPassword() returns a String. Since Strings are pooled and never GC'ed, there are effectively accessible throughout the JVM and can be read from a heap dump. It's a common practise to have passwords in byte arrays, instead. These arrays can be wiped by filling them with zeros.

Cf. Secure Coding standard recommendation:
https://wiki.sei.cmu.edu/confluence/display/java/MSC59-J.+Limit+the+lifetime+of+sensitive+data

[jorabin]

Thanks for this. Sorry to take so long to reply. Yes, I agree and if there is a future version the String returning getPassword() should be deprecated in favour of one that returns a byte array.

[ng-23]

@danielgrahl Other fields like username, URL, etc. are also stored as Strings. Would it be better to instead store them as byte arrays too, even though they aren't as valuable as a password?

[jorabin]

I think the approach to the question as posed, should be:

1. Deprecate the String Entry.getPassword() method.
2. Add a byte[] Entry.getPropertyAsBytes(String propertyName) to complement the already existing String Entry.getProperty(String propertyName) method (this addresses @ng-23's point about getting any property as a byte[]).
3. Question would be whether to provide a byte[] Entry.GetPasswordAsBytes(String propertyName) method.

A related (and possibly more important) question is what to do about the storage of passwords (and other sensitive fields) in the underlying data storage. As things stand these fields are "inner stream encoded" in the XML meaning they are base64 representation of encrypted values in the XML stream (which is itself encrypted as overall encryption of the database). These fields are decrypted when the database is loaded and are stored in memory as, well, strings.

You can see what this looks like, a bit, by doing this:

database.save(new StreamFormat.None(), new Credentials.None(), System.out);

in the output from this, protected fields are "merely" base 64 encoded, they are not encrypted then base64 encoded when you use StreamFormat.None. The SAX Parse Example actually lists the passwords.

There are a few ways one could improve having strings in memory.

1. Leave them stream encrypted till they are needed (this would mean decrypting them in sequence each time they are used), which would also have the problem that adding encrypted fields would require a re-encryption of them all
2. Dominik discusses how he does it Process Memory Protection in KeePass itself.
3. KeePassXC discussion
4. Discussion of protecting strings in memory in Java, and a related solution

As Dominik points out in the reference above, this is all very well till it comes to actually using the passwords, when they will typically be needed as Strings, anyway.

I'd welcome discussion.

[giusvale-dev]

I'm not sure this solution is perfectly compliant to protect sensitive data, and I'm not convinced the change of the data structure to represent sensitive information is the right solution.

If I have access to the memory as an attacker, I can retrieve the heap and stack information from memory with corruption attacks.

In this hypothetical scenarios I can see the password stored in the memory system because I can read your byte array or string.

So if I change the data type to store the password, this doesn't add any level of security because the password is a plaintext as a string or as byte array.

I think the true effort could be to add the protection with the cryptographic system. For example, the getPassword() could be retrive the hash of the Password and setPassword should be store the password as the hashed string.

After that, we can override the sensitivities information contained in the string (or byte array), for me, it is the same (my opinion).

I think the right way isn't changing the data structure to protect the information, but it is the cryptographic approach.

Now, if I decrypt data and this info is stored in a security context (I store the plaintext in an encrypted heap), the clean password process can be useless and theoretically unnecessary to clean the sensitive data (this is a plus).

[jorabin]

The point about not using String is that once a password is in a String as plaintext it remains, can't be zeroed etc. whereas a byte array (or char array) can. So returning a String for getPassword is really, well, wrong. Except that if it is needed as a String to get used then it's going to end up as a String anyway.

As far as storing the the data as part of the database, well, as long as it's in memory then it's vulnerable if it is plaintext. So some level of obfuscation would be a good thing, probably.

In addition to the links above, needless to say there is something on StackOverflow with lots of opinions on this.

[giusvale-dev]

Ok, I understand this point of view, and yeah, we can edit the getter/setter methods for the password as a char array.

However, I think this link Guarded String can be util in this case adding a little level of protection

[giusvale-dev]

I've compiled all the relevant information in this post as I'm determined to address this vulnerability. It's a substantial challenge, but I'm committed to finding a solution.

Considerations

1. One critical step is transitioning from using a String to a char[] for passwords. In Java, since String objects are immutable, obfuscating plaintext stored in a String isn't effective due to the way references are stored in memory and new String objects are created.
2. We have an additional constraint of maintaining compatibility with other KeePass applications, which we need to take into account.
3. Other KeePass projects securely decrypt sensitive data within a protected portion of memory. However, in Java, it's not feasible to encrypt a specific segment of the heap.
4. A migration from String to char[] introduces a memory vulnerability, particularly during deserialization, where the plaintext resides in memory.

Integration of Secure String Project

I've experimented with the Secure String project and had success using the following code:

public class Test {

    static String readWholeBufferAsString(SecureCharBuffer buffer) {
        char[] chars = new char[buffer.length()];
        for (int i = 0; i < chars.length; i++) {
            chars[i] = buffer.get(i);
        }
        return new String(chars);
    }

    public static void main(String[] args) {
        SecureCharBuffer secureString = new SecureCharBuffer();
        secureString.append("Hello");
        final String test = readWholeBufferAsString(secureString);
        System.out.println(String.format("Before closing secure buffer: %s", test)); // Prints Hello
        secureString.close();
        System.out.println(String.format("After closing secure buffer: %s", test)); // Throws an exception
    }

}

Proposed Solution

1. Migration to Secure Char Arrays: Transition the fields like title, username, password, url, and notes from using String to char[]. This change ensures that sensitive data is not stored in immutable String objects, reducing vulnerability to memory attacks.

2. Serialization:

   • Store the plain data in a SecureCharBuffer object.
   • Retrieve the plain data from the SecureCharBuffer object.
   • Encrypt the data and convert it to base64.
   • Close the SecureCharBuffer instance and destroy the temporary plaintext char[] from memory.

3. Deserialization:

   • Read the base64-encoded string.
   • Decode the base64 string to obtain encrypted data.
   • Decrypt the data and initialize a SecureCharBuffer using SecureCharBuffer(encryption.decrypt(encryptedData)).
   • Retrieve the plaintext data from the SecureCharBuffer.
   • Close the SecureCharBuffer instance and destroy the plaintext data.

By adopting this approach, we significantly reduce the window of time during which sensitive data is present in plaintext in memory. While other projects rely on protected memory, our specific context might not allow for such implementation due to the uncertainty surrounding the possibility of encrypting portions of the Heap space.

Your feedback on this approach is highly valued!