Passphrase storing

[dtitov]

Hi! Very nice library, but we've discovered a potential security issue: passphrase for the private key is stored as String, which is a bit insecure.

I would suggest not just replacing the type of passphrase field in the Subkey class from String to char[], but rather not storing passphrase at all: it should be possible to store built PBESecretKeyDecryptor instead. Or even not store anything at all and just ask for a char[] each time upon decryption.

What do you think? Would it be possible to implement such a change?

[justinludwig]

Thank you for raising this issue! -- it's a very good point, and your suggestions make a lot of sense to me.

I'll play around with it a bit over the next few weeks, and see if I can put together an update that:

1. allows passphrases to be supplied directly as a char[]
2. stores and reuses the generated PBESecretKeyDecryptor where possible (or maybe even the extracted PGPPrivateKey itself)
3. adds a method to the Subkey class that zeroes-out the passphrase (and any other secret data the Subkey has stored)
4. documents a pattern for library consumers to use when they want to zero-out the supplied passphrase after each use -- I'm envisioning something like this:

Key key = new Key(new File("path/to/alice.asc"));
Decryptor decryptor = new Decryptor(key);
char[] passphrase = ... // load this as a char[] from some source
try {
    key.setPassphrase(passphrase);
    decryptor.decrypt(
        new File("path/to/ciphertext.txt.gpg"),
        new File("path/to/plaintext.txt")
    );
} finally {
    key.clearSecrets(); // new method to zero-out passphrase
}

[dtitov]

Thanks for the quick response. Sounds like a plan!

Ping me if you need any assistance, I would gladly help.

[justinludwig]

I coded up some changes as PR #20, trying to avoid breaking the existing use of passphrases as Strings, but enabling passphrases as char[]s to be used without too much additional awkwardness -- please take a look when you have some time, and let me known what you think. These changes:

1. Allow passphrases to be supplied directly as a char[] and stored via a new passphraseChars property on the Subkey class (and via convenience methods on the Key class); and also allows the PGPPrivateKey object to be extracted without storing the passphrase at all, via a new unlock() method. If the passphrase is supplied as a char[] via the passphraseChars property, no copies are made -- so zeroing-out the original char[] wipes the passphrase from memory.
2. Store and reuse the extracted PGPPrivateKey in the Subkey class -- which should at least be an improvement over the current behavior, where a separate copy of the PGPPrivateKey is created every time a separate message is signed or decrypted.
3. Add a clearSecrets() method to the Subkey class (and all the other principal classes), zeroing-out the stored passphrase char[], and releasing the stored PGPPrivateKey for garbage collection.
4. Also allow the passphrase for symmetric encryption to be supplied as a char[] to the Encryptor and Decryptor classes, via new symmetricPassphraseChars properties (analogous to the passphraseChars property on a Subkey).

I also wrote up some changes to the wiki pages, including updating the section on setting passphrases to document how to supply the passphrase as a char[], and how to clean up the passphrases in memory once decryption/signing is done. I put these changes in the passphrase-storing branch of the wiki -- Github doesn't seem to have a web UI for viewing wiki branches, but you can at least see the diff online here:

https://github.com/justinludwig/jpgpj/wiki/_compare/5ea327f7c9601cc159d0afa085bd00dfc2eb1e28...0cb8d85ccd7b62ddb665a919efd75f76eed6dbb9

[justinludwig]

Thanks again for your help @dtitov! -- I merged #20 into master, and released v0.5 with the change. I also merged those wiki updates in -- the main section for documenting how best to use the new API methods is here: https://github.com/justinludwig/jpgpj/wiki/KeyRings#cleaning-up-memory

[dtitov]

Thanks! :)