You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shiro.apache.org by Eric Cho <he...@gmail.com> on 2016/08/15 06:54:34 UTC
An idea on SHIRO-349
Hi All,
I was looking into the Shiro's code-base and Jira issues. One issue that
caught my eye was SHIRO-349
<https://issues.apache.org/jira/browse/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
Re: An idea on SHIRO-349
Posted by Eric Cho <he...@gmail.com>.
OK, I made a PR as you said in the repo. Thanks!
Eric
On Mon, Aug 15, 2016 at 10:44 PM, Brian Demers <br...@gmail.com>
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 <he...@gmail.com> 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
>
Re: An idea on SHIRO-349
Posted by Brian Demers <br...@gmail.com>.
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 <he...@gmail.com> 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