Passwords should be wiped from memory partial implementation

[giusvale-dev]

Updated Password Handling: Transitioning to byte[] for Enhanced Security

I have introduced changes to facilitate password handling using byte[] arrays in place of String objects.

Key Changes:

Entry Interface and AbstractEntry Class Update:

• The Entry interface and the AbstractEntry class have been thoughtfully adjusted to accommodate the migration towards byte[] arrays for password management.
• I have introduced new methods within these components to provide direct support for byte[] arrays.
• All classes inherited from AbstractEntry now include implementations of the getPropertyAsBytes method.

Work in Progress:

Prior to proceeding with further code implementation, I believe it's essential to gather your valuable feedback. The following steps are on the horizon:

1. Implementing an Obfuscation Strategy:
   Before progressing, I intend to establish a robust obfuscation strategy to bolster security.

2. Assessing Source Code Updates:
   Subsequently, we can evaluate the integration of these changes into the source code, particularly within the serialization and deserialization processes.

3. Security Considerations:

For example, when examining serialization/deserialization methods, we allocate the protected value as plaintext in memory, and when using setter methods, we add this value to other objects allocated in the heap. Consequently, the protected information is stored in clear text within the heap, whether obfuscated or not.

I have provided a simple code snippet to illustrate this point, and I've added comments to my source code to further clarify this behavior.

Your input and insights are pivotal at this stage as we strive to fortify security and maintain the codebase's integrity.

ORIGINAL CODE

public JaxbSerializableDatabase load(InputStream inputStream) {
        try {
            JAXBContext jc = JAXBContext.newInstance(KeePassFile.class, ValueBinding.class);
            Unmarshaller u = jc.createUnmarshaller();
            u.setListener(new Unmarshaller.Listener() {
                @Override
                public void afterUnmarshal(Object target, Object parent) {
                    if (target instanceof StringField.Value) {
                        StringField.Value value = (StringField.Value) target;
                        if (value.getProtected() != null && value.getProtected()) {
                            byte[] encrypted = Base64.decodeBase64(value.getValue().getBytes());
                            String decrypted = new String(encryption.decrypt(encrypted), StandardCharsets.UTF_8);
                            value.setValue(decrypted);
                            value.setProtected(null);
                            value.protectOnOutput=true;
                        }
                    }
                    if (target instanceof JaxbGroupBinding && (parent instanceof JaxbGroupBinding)) {
                        ((JaxbGroupBinding) target).parent = ((JaxbGroupBinding) parent);
                    }
                    if (target instanceof JaxbEntryBinding && (parent instanceof JaxbGroupBinding)) {
                        ((JaxbEntryBinding) target).parent = ((JaxbGroupBinding) parent);
                    }
                }
            });
            keePassFile = (KeePassFile) u.unmarshal(inputStream);
            return this;
        } catch (JAXBException e) {
            throw new IllegalStateException(e);
        }
    }

POSSIBLE UPDATE CODE

public JaxbSerializableDatabase load(InputStream inputStream) {
        try {
            JAXBContext jc = JAXBContext.newInstance(KeePassFile.class, ValueBinding.class);
            Unmarshaller u = jc.createUnmarshaller();
            u.setListener(new Unmarshaller.Listener() {
                @Override
                public void afterUnmarshal(Object target, Object parent) {
                    if (target instanceof StringField.Value) {
                        StringField.Value value = (StringField.Value) target;
                        if (value.getProtected() != null && value.getProtected()) {
                            byte[] encrypted = Base64.decodeBase64(value.getValue().getBytes());
                            byte[] decrypted = encryption.decrypt(encrypted);
                            value.setValue(new String(decrypted)); //HERE I STORE INTO MEMORY THE PLAINTEXT
                            Arrays.fill(decrypted, 0, decrypted.length, (byte) 0x00); // CLEAR FROM MEMORY THE DECRYPTED (HAS SENSE?)
                            value.setProtected(null);
                            value.protectOnOutput=true;
                        }
                    }
                    if (target instanceof JaxbGroupBinding && (parent instanceof JaxbGroupBinding)) {
                        ((JaxbGroupBinding) target).parent = ((JaxbGroupBinding) parent);
                    }
                    if (target instanceof JaxbEntryBinding && (parent instanceof JaxbGroupBinding)) {
                        ((JaxbEntryBinding) target).parent = ((JaxbGroupBinding) parent);
                    }
                }
            });
            keePassFile = (KeePassFile) u.unmarshal(inputStream);
            return this;
        } catch (JAXBException e) {
            throw new IllegalStateException(e);
        }
    }

[jorabin]

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.

1. On the one hand, offering passwords as strings through the API is wrong, and
2. 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]

Hi Jo,
thanks for your reply.

Don't worry about rejecting this PR, we will make it together after further analysis (we will continue our discussion in #28).

Indeed there are many points to clarify (as you have seen in my PR comment), and I agree with your last comment.

I meant to do a few PRs to solve this hard task, but is better to clarify before all the points.

Giuseppe