Thanks Eric! I've been thinking a little bit about this too. #2 and #3 are defiantly something we should think about for 2.0, as they both are not backwards compatible.
#1 could potentially still break things in 1.x, so something like that would need some heavy test coverage. Can you send your diff as a github pull request? (an we can chat about it further there?) https://github.com/apache/shiro -Brian On Mon, Aug 15, 2016 at 2:54 AM, Eric Cho <[email protected]> wrote: > Hi All, > > I was looking into the Shiro's code-base and Jira issues. One issue that > caught my eye was SHIRO-349, for eliminating sensitive information in byte > array, such as password, key. It seems obvious that we should zero out the > memory as soon as we're done with it to not expose it to potential > attackers, e.g., memory dump. > For the issue, I came to writing a small piece to mitigate that and suggest > a new way of managing/zeroing out the variables. So far I found three > methods to achieve the goal as follows: > > > 1. Wiping out manually > > As soon as we're done with the byte array, we zero out the space by using > CollectionUtils#wipe method I made. The finally block will ensure the memory > clean up even if it throws any exceptions in the try block. > > byte[] decrypted = plainOut.toByteArray(); > try { > assertTrue(Arrays.equals(plaintext, decrypted)); > } finally { > CollectionUtils.wipe(decrypted); > } > > > 2. Try-with-resources idiom (Java 7+) > > Adding try-finally block on every usage of the sensitive byte array can be > too much overhead. We can bring the try-with-resources idiom which used for > releasing certain resources at end of the try block automatically. An > example would be: > > try (ByteWrapper temp = ByteWrapper.wrap(byteSource.getBytes())) { > user.use(temp.getBytes()); > } catch (IOException e) { > // ignore > } > > The byte array, temp.getBytes, should be deleted as soon as the try block is > completed. > > > 3. Limit access to the value by class design > > Advice developers to follow a certain rule like above isn't enough. > Sometimes we want to make more powerful guideline that no one can break. I > thought of ByteSourceBroker class which doesn't expose the byte array, > rather we need to deliver how we'll use the value in our scenario > (ByteSourceUser interface): > > ByteSourceBroker broker = aes.decrypt(ciphertext.getBytes(), key); > broker.useBytes(new ByteSourceUser() { > @Override > void use(byte[] bytes) { > assertTrue(Arrays.equals(plaintext, bytes)); > } > }); > if (broker instanceof Destroyable) { > ((Destroyable) broker).destroy(); > } > > * If we use Java 8's lambda: > broker.useBytes((bytes) -> assertTrue(Arrays.equals(plaintext, bytes))); > > > I hope the above code snippets are pretty self-explanatory, but in case you > need to see whole changes I made, please check the attachment. > > I'm looking forward to hearing your opinion on my approach for SHIRO-349, > what you'd think on my code changes to improve it. Any comments would be > appreciated. > > Thanks, > Eric
