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!

[giusvale-dev]

1. I think the worst case for us should be: "A paranoic user would be marked as protected all fields", then we need to use the char[] instead ofString
   For that reason I think is needed the migration from String to char[] for all properties.
   Other KeePass projects store the sensible deserialized data in a protected memory, which we can't do in Java. Maybe, we can think of an encrypted byte[] where we can serialize/deserialize all sensible information retrieved from the database. The problem is, what key should be used to encrypt and decrypt this data? With these points, I think we have the same security as the other KeePass projects: that would be great!
2. Ok
3. I think the String should kick off in the protected fields then we don't allow the getter methods of String for these fields. Why do you want to maintain that?
4. I think we need to align with the other KeePass projects: at the moment we should have a memory dump vulnerability. If we have other contributors they are welcome in the discussion!

[jorabin]

1. There is a performance penalty associated with protecting fields and so protecting all fields should be optional. There are a potentially unlimited number of fields, not just the 5 standard ones.
2. I don't recall there being an api call for setting the "protect by default" fields. So that needs adding too.
3. Initially for backward compatibility.
4. I agree that protected fields should actually be protected in some way.

[giusvale-dev]

I think the aim should be data protection because this is a security API, the performance is a secondary goal (not needed). My point of view.
Then, I think the right way is the protection of all fields marked as "protected" by the user.

[jorabin]

it's not for me or you to say what the trade off is between performance and security. In fact, if the implementor can choose between protection "none" and protection "takes a million years to decrypt" then that is a choice that we can offer by creating an interface that can be implemented to and by default we can offer a couple of options.

[giusvale-dev]

Yeah! That sounds good!

[jorabin]

Noting that I turned #28 into this discussion.

[jorabin]

Noting PR #52 from @giusvale-dev which I turned down as follows:

Hi again Giuseppe

Thanks for this PR which came as a bit of a surprise, especially since it comes so soon after your last contribution!

There are two different aspects to this.

On the one hand, offering passwords as strings through the API is wrong, and
On the other hand the worse problem is decrypting the inner encrypted stream and storing unencrypted is wrong, never mind that the values are stored as Strings, which makes it worse. The fact that they are stored that way, though, means that offering passwords as Strings through the API isn't making anything worse than it already is.
I very much appreciate your wanting to address this, and hurrying to do so. However, as the Romans said "festina lente". Not as slowly as I have been hurrying, admittedly, but let's think carefully about making things better. And do it once.

I am going to hold off on merging your PR for now, till we have thought about this a bit more, and in particular, thought about:

How will we store the passwords in the database, not as strings, but as what?
How will the API offer password values, not as Strings, but we have assumed as byte [], as that is what was suggested in the original issue. However, if you review the stuff out there on "don't do passwords in Strings", they say "do them as char []". That makes more sense than byte [] as it offers the same advantages as well as allowing the characters of the password to be properly distinguished.
Let's continue this discussion under Issue #28 and decide with the help of anyone who cares to contribute, what we should do next.

A couple of other things:

Bumping the release to 2.2.3 isn't necessary as we didn't release 2.2.2 yet. If you think that whatever changes get made warrant going to 2.3 that would be a different question, I think.
I think that we need to provide non-String setters as well as non-String getters.
Thank you for your contribution, I really appreciate your activity! I regret not adopting your PR, but hope you'll see why?

Jo

[giusvale-dev]

Maybe I have a solution that can help us in the following lines the possible fix:

1. Method getPassword will return a char[]
2. Method setPassword will protect (see later) the input params (char[]) and then call the standard method setProperyAsString
3. New class ProtectedMemory to manage the random password to protect the data in memory with the KeyStore
4. The class ProtectedMemory offers the methods to manage the keys in KeyStore (Delete, Create, Renew, etc.)
5. We will use the ChaCha20 algorithm Doc to encrypt the fields marked as "Protected" and/or password.
6. We need to renew the key into Keystore after the protected field is read (value.getText() called)
7. During serialization, we need to decrypt the protected data from the memory (and delete the relative key from memory and KeyStore) before sending the data to the database.
8. During the deserialization, we need to create a new random key and store it in KeyStore, then cipher the field (marked as "Protected") and/or password with this ChaCha20 into RAM.
9. Every time a field is decrypted from memory, we need to generate a new key to encrypt and store the data in the memory.
10. We should have all sensitive data in memory for the slow time.

Please give me your feedback, I'd like to develop these features.
Giuseppe

[jorabin]

Hi Giuseppe

I have been thinking about all this (alongside day job, etc.) and think I have reached a few conclusions:

1. It's going to be hard and a lot of work to integrate secure storage with existing "format native" serialisers and deserialisers. If we want this feature on all kdbx implementations we need to do it 4 times (5 including kdb).
2. We know Simple is dead, Dom won't go away but no one realistically will use it for anything. Jaxb is not the sharpest technology in the block and really there is no significant user benefit to having those technologies.
3. Creating a simple-ish memory model for properties and accessors independent of serialization model is a fairly straightforward task. And allows protected storage, simply. Using same interfaces can also provide Database or Keystore backed storage.
4. Separating the (de)serialisation process from the storage process for any particular file format (at the moment we only have kdbx and kdb read-only storage formats) needs be done only once for each storage format. Although this probably needs doing "by hand" rather than "automagically" via annotations, it is not complex, even if it might be a bit laborious (and kdb is done by hand anyway).
5. The current approach to generics e.g. Entry <D extends ..., G extends ..., E extends ..., I> is overly complex and is unnecessary with the above approach.
6. The underlying interfaces (Group, Entry) could do with review and this would be a good opportunity e.g. to provide for CharString (as well as char [] and byte []) accessors for values, and possibly drop String accessors (though if values are not stored as strings it's a nuisance comparing them for searches).

I have been working in that overall direction, with a view to producing KeePassJava2 Version 3. Not sure the numbering makes sense but those are the lines along which I am thinking. Once I have somthing that compiles, I wil push to a branch, am looking for comments and contributions.

If this was the direction then KeepassJava2 next release (2.2.2) would be the Jackson implementation. Then no more implementations other than bug fixes.

In summary:
a. Simplified and more secure API for Database, Group and Entry
b. Storage model no longer determined by serialisation tool, or output format. An in memory model to start with, could be database and other implementations too.
c. Significant reduction in code.

What do you think?
Jo

PS you really did open a can of worms 😉

[giusvale-dev]

Can you open 2 new branches for future PRs?

• one for bugfix only
• second for version 3

It would be great if you could organize the new issue with labels or kanban board indicating the release version (2.2.2 or 3).

I try to answer your points:

1. If you decide to maintain all engines to serialize and deserialize the database, yeah, we need to implement the protection strategies for all engines.

2. I think we need only one engine to serialize and deserialize the database. For many reasons, in particular, to be as possible independently from others' libraries. As an end user, I'm not interested in how KeePassJava2 serialize and deserialize the data, I'm interested in the result of this operation. I agree with your analysis.

3. I don't understand your vision at this point. In the previous message, I intended to store the temporary key in KeyStore to protect the KeePass fields (protected) and the KeePass password that are in the Heap Space.

4. I agree that we should have only one process to serialize and deserialize the data, but I don't understand why this should be done by hands. Maybe I don't know the kdb file format, and this cause confusion.

5. If you consider using only one engine to serialize and deserialize the database, yeah, the implementation is very smart.

6. I agree

I think you need to choose only one engine to serialize and deserialize the data. It's the first architectural thing.

Then, we need to open the issue related to the activity to develop with the label and, if possible, a kanban.

So I'm happy we have a lot of work to do! We need to improve this project!

Giuseppe