OK, I made a PR as you said in the repo. Thanks! Eric On Mon, Aug 15, 2016 at 10:44 PM, Brian Demers <[email protected]> wrote:
> 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 >
