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
diff --git
a/core/src/main/java/org/apache/shiro/mgt/AbstractRememberMeManager.java
b/core/src/main/java/org/apache/shiro/mgt/AbstractRememberMeManager.java
index c857ef9..63354bd 100644
--- a/core/src/main/java/org/apache/shiro/mgt/AbstractRememberMeManager.java
+++ b/core/src/main/java/org/apache/shiro/mgt/AbstractRememberMeManager.java
@@ -24,6 +24,7 @@ import org.apache.shiro.authc.AuthenticationToken;
import org.apache.shiro.authc.RememberMeAuthenticationToken;
import org.apache.shiro.codec.Base64;
import org.apache.shiro.crypto.AesCipherService;
+import org.apache.shiro.crypto.ByteSourceBroker;
import org.apache.shiro.crypto.CipherService;
import org.apache.shiro.io.DefaultSerializer;
import org.apache.shiro.io.Serializer;
@@ -31,6 +32,7 @@ import org.apache.shiro.subject.PrincipalCollection;
import org.apache.shiro.subject.Subject;
import org.apache.shiro.subject.SubjectContext;
import org.apache.shiro.util.ByteSource;
+import org.apache.shiro.util.CollectionUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -379,14 +381,17 @@ public abstract class AbstractRememberMeManager
implements RememberMeManager {
*/
public PrincipalCollection getRememberedPrincipals(SubjectContext
subjectContext) {
PrincipalCollection principals = null;
+ byte[] bytes = null;
try {
- byte[] bytes = getRememberedSerializedIdentity(subjectContext);
+ bytes = getRememberedSerializedIdentity(subjectContext);
//SHIRO-138 - only call convertBytesToPrincipals if bytes exist:
if (bytes != null && bytes.length > 0) {
principals = convertBytesToPrincipals(bytes, subjectContext);
}
} catch (RuntimeException re) {
principals = onRememberedPrincipalFailure(re, subjectContext);
+ } finally {
+ CollectionUtils.wipe(bytes);
}
return principals;
@@ -476,8 +481,8 @@ public abstract class AbstractRememberMeManager implements
RememberMeManager {
byte[] serialized = encrypted;
CipherService cipherService = getCipherService();
if (cipherService != null) {
- ByteSource byteSource = cipherService.decrypt(encrypted,
getDecryptionCipherKey());
- serialized = byteSource.getBytes();
+ ByteSourceBroker broker = cipherService.decrypt(encrypted,
getDecryptionCipherKey());
+ serialized = broker.getClonedBytes();
}
return serialized;
}
diff --git
a/core/src/main/java/org/apache/shiro/subject/SimplePrincipalCollection.java
b/core/src/main/java/org/apache/shiro/subject/SimplePrincipalCollection.java
index 9b17f2a..b15bb1e 100644
--- a/core/src/main/java/org/apache/shiro/subject/SimplePrincipalCollection.java
+++ b/core/src/main/java/org/apache/shiro/subject/SimplePrincipalCollection.java
@@ -284,7 +284,7 @@ public class SimplePrincipalCollection implements
MutablePrincipalCollection {
* input stream.
* <p/>
* NOTE: Don't forget to change the serialVersionUID constant at the top
of this class
- * if you make any backwards-incompatible serializatoin changes!!!
+ * if you make any backwards-incompatible serialization changes!!!
* (use the JDK 'serialver' program for this)
*
* @param in input stream provided by
diff --git
a/crypto/cipher/src/main/java/org/apache/shiro/crypto/ByteSourceBroker.java
b/crypto/cipher/src/main/java/org/apache/shiro/crypto/ByteSourceBroker.java
new file mode 100644
index 0000000..1b356d3
--- /dev/null
+++ b/crypto/cipher/src/main/java/org/apache/shiro/crypto/ByteSourceBroker.java
@@ -0,0 +1,29 @@
+package org.apache.shiro.crypto;
+
+/**
+ * ByteSourceBroker holds an encrypted value to decrypt it on demand.
+ * <br/>
+ * {@link #useBytes(ByteSourceUser)} method is designed for dictating
+ * developers to use the byte source in a special way, to prevent its
prevalence
+ * and difficulty of managing & zeroing that critical information at end of
use.
+ * <br/>
+ * For exceptional cases we allow developers to use the other method,
+ * {@link #getClonedBytes()}, but it's not advised.
+ */
+public interface ByteSourceBroker {
+ /**
+ * This method accepts an implementation of ByteSourceUser functional
interface.
+ * <br/>
+ * To limit the decrypted value's existence, developers should maintain the
+ * implementation part as short as possible.
+ *
+ * @param user Implements a use-case for the decrypted value.
+ */
+ void useBytes(ByteSourceUser user);
+
+ /**
+ * As the name implies, this returns a cloned byte array
+ * and caller has a responsibility to wipe it out at end of use.
+ */
+ byte[] getClonedBytes();
+}
diff --git
a/crypto/cipher/src/main/java/org/apache/shiro/crypto/ByteSourceUser.java
b/crypto/cipher/src/main/java/org/apache/shiro/crypto/ByteSourceUser.java
new file mode 100644
index 0000000..12195d6
--- /dev/null
+++ b/crypto/cipher/src/main/java/org/apache/shiro/crypto/ByteSourceUser.java
@@ -0,0 +1,12 @@
+package org.apache.shiro.crypto;
+
+/**
+ * {@link ByteSourceBroker#useBytes(ByteSourceUser)} method requires
ByteSourceUser argument,
+ * and developers should implement how we use the byte arrays in our code-base.
+ * <br/>
+ * The byte array "bytes" could be a decrypted password in plaintext format,
or other
+ * sensitive information that needs to be erased at end of use.
+ */
+public interface ByteSourceUser {
+ void use(byte[] bytes);
+}
diff --git
a/crypto/cipher/src/main/java/org/apache/shiro/crypto/CipherService.java
b/crypto/cipher/src/main/java/org/apache/shiro/crypto/CipherService.java
index 8dda1ac..d5405a5 100644
--- a/crypto/cipher/src/main/java/org/apache/shiro/crypto/CipherService.java
+++ b/crypto/cipher/src/main/java/org/apache/shiro/crypto/CipherService.java
@@ -95,7 +95,7 @@ public interface CipherService {
* @return a byte source representing the original form of the specified
encrypted data.
* @throws CryptoException if there is an error during decryption
*/
- ByteSource decrypt(byte[] encrypted, byte[] decryptionKey) throws
CryptoException;
+ ByteSourceBroker decrypt(byte[] encrypted, byte[] decryptionKey) throws
CryptoException;
/**
* Receives encrypted data from the given {@code InputStream}, decrypts
it, and sends the resulting decrypted data
diff --git
a/crypto/cipher/src/main/java/org/apache/shiro/crypto/JcaCipherService.java
b/crypto/cipher/src/main/java/org/apache/shiro/crypto/JcaCipherService.java
index bb21556..825069e 100644
--- a/crypto/cipher/src/main/java/org/apache/shiro/crypto/JcaCipherService.java
+++ b/crypto/cipher/src/main/java/org/apache/shiro/crypto/JcaCipherService.java
@@ -344,7 +344,11 @@ public abstract class JcaCipherService implements
CipherService {
return ByteSource.Util.bytes(output);
}
- public ByteSource decrypt(byte[] ciphertext, byte[] key) throws
CryptoException {
+ public ByteSourceBroker decrypt(byte[] ciphertext, byte[] key) throws
CryptoException {
+ return new SimpleByteSourceBroker(this, ciphertext, key);
+ }
+
+ ByteSource decryptInternal(byte[] ciphertext, byte[] key) throws
CryptoException {
byte[] encrypted = ciphertext;
@@ -379,10 +383,10 @@ public abstract class JcaCipherService implements
CipherService {
}
}
- return decrypt(encrypted, key, iv);
+ return decryptInternal(encrypted, key, iv);
}
- private ByteSource decrypt(byte[] ciphertext, byte[] key, byte[] iv)
throws CryptoException {
+ private ByteSource decryptInternal(byte[] ciphertext, byte[] key, byte[]
iv) throws CryptoException {
if (log.isTraceEnabled()) {
log.trace("Attempting to decrypt incoming byte array of length " +
(ciphertext != null ? ciphertext.length : 0));
diff --git
a/crypto/cipher/src/main/java/org/apache/shiro/crypto/SimpleByteSourceBroker.java
b/crypto/cipher/src/main/java/org/apache/shiro/crypto/SimpleByteSourceBroker.java
new file mode 100644
index 0000000..cc4390e
--- /dev/null
+++
b/crypto/cipher/src/main/java/org/apache/shiro/crypto/SimpleByteSourceBroker.java
@@ -0,0 +1,57 @@
+package org.apache.shiro.crypto;
+
+import org.apache.shiro.util.ByteSource;
+import org.apache.shiro.util.ByteSourceWrapper;
+import org.apache.shiro.util.CollectionUtils;
+import org.apache.shiro.util.Destroyable;
+
+import java.io.IOException;
+
+/**
+ * A simple implementation that maintains cipher service, ciphertext and key
for decrypting it later.
+ * {@link #useBytes(ByteSourceUser)} guarantees the sensitive data in byte
array will be erased at end of use.
+ */
+public class SimpleByteSourceBroker implements ByteSourceBroker, Destroyable {
+ private JcaCipherService cipherService;
+ private byte[] ciphertext;
+ private byte[] key;
+ private boolean destroyed = false;
+
+ public SimpleByteSourceBroker(JcaCipherService cipherService, byte[]
ciphertext, byte[] key) {
+ this.cipherService = cipherService;
+ this.ciphertext = ciphertext.clone();
+ this.key = key.clone();
+ }
+
+ public synchronized void useBytes(ByteSourceUser user) {
+ if (destroyed || user == null) {
+ return;
+ }
+ ByteSource byteSource = cipherService.decryptInternal(ciphertext, key);
+
+ try (ByteSourceWrapper temp =
ByteSourceWrapper.wrap(byteSource.getBytes())) {
+ user.use(temp.getBytes());
+ } catch (IOException e) {
+ // ignore
+ }
+
+ }
+
+ public byte[] getClonedBytes() {
+ ByteSource byteSource = cipherService.decryptInternal(ciphertext, key);
+ return byteSource.getBytes(); // this's a newly created byte array
+ }
+
+ public void destroy() throws Exception {
+ if (!destroyed) {
+ synchronized (this) {
+ destroyed = true;
+ cipherService = null;
+ CollectionUtils.wipe(ciphertext);
+ ciphertext = null;
+ CollectionUtils.wipe(key);
+ key = null;
+ }
+ }
+ }
+}
diff --git
a/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/AesCipherServiceTest.groovy
b/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/AesCipherServiceTest.groovy
index f35751e..b95fddd 100644
---
a/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/AesCipherServiceTest.groovy
+++
b/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/AesCipherServiceTest.groovy
@@ -20,6 +20,8 @@ package org.apache.shiro.crypto
import org.apache.shiro.codec.CodecSupport
import org.apache.shiro.util.ByteSource
+import org.apache.shiro.util.CollectionUtils
+import org.apache.shiro.util.Destroyable
import org.junit.Test
import static junit.framework.Assert.*
@@ -45,8 +47,16 @@ public class AesCipherServiceTest {
for (String plain : PLAINTEXTS) {
byte[] plaintext = CodecSupport.toBytes(plain);
ByteSource ciphertext = aes.encrypt(plaintext, key);
- ByteSource decrypted = aes.decrypt(ciphertext.getBytes(), key);
- assertTrue(Arrays.equals(plaintext, decrypted.getBytes()));
+ 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();
+ }
}
}
@@ -68,7 +78,11 @@ public class AesCipherServiceTest {
cipher.decrypt(cipherIn, plainOut, key);
byte[] decrypted = plainOut.toByteArray();
- assertTrue(Arrays.equals(plaintext, decrypted));
+ try {
+ assertTrue(Arrays.equals(plaintext, decrypted));
+ } finally {
+ CollectionUtils.wipe(decrypted);
+ }
}
}
}
diff --git
a/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/BlowfishCipherServiceTest.groovy
b/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/BlowfishCipherServiceTest.groovy
index eaadf55..5fbc478 100644
---
a/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/BlowfishCipherServiceTest.groovy
+++
b/crypto/cipher/src/test/groovy/org/apache/shiro/crypto/BlowfishCipherServiceTest.groovy
@@ -20,6 +20,7 @@ package org.apache.shiro.crypto
import org.apache.shiro.codec.CodecSupport
import org.apache.shiro.util.ByteSource
+import org.apache.shiro.util.CollectionUtils
import org.junit.Test
import static junit.framework.Assert.assertTrue
@@ -45,8 +46,13 @@ public class BlowfishCipherServiceTest {
for (String plain : PLAINTEXTS) {
byte[] plaintext = CodecSupport.toBytes(plain);
ByteSource ciphertext = blowfish.encrypt(plaintext, key);
- ByteSource decrypted = blowfish.decrypt(ciphertext.getBytes(),
key);
- assertTrue(Arrays.equals(plaintext, decrypted.getBytes()));
+ ByteSourceBroker broker = blowfish.decrypt(ciphertext.getBytes(),
key);
+ byte[] decrypted = broker.getClonedBytes()
+ try {
+ assertTrue(Arrays.equals(plaintext, decrypted));
+ } finally {
+ CollectionUtils.wipe(decrypted);
+ }
}
}
@@ -68,7 +74,11 @@ public class BlowfishCipherServiceTest {
cipher.decrypt(cipherIn, plainOut, key);
byte[] decrypted = plainOut.toByteArray();
- assertTrue(Arrays.equals(plaintext, decrypted));
+ try {
+ assertTrue(Arrays.equals(plaintext, decrypted));
+ } finally {
+ CollectionUtils.wipe(decrypted);
+ }
}
}
diff --git a/lang/src/main/java/org/apache/shiro/util/ByteSourceWrapper.java
b/lang/src/main/java/org/apache/shiro/util/ByteSourceWrapper.java
new file mode 100644
index 0000000..2933343
--- /dev/null
+++ b/lang/src/main/java/org/apache/shiro/util/ByteSourceWrapper.java
@@ -0,0 +1,38 @@
+package org.apache.shiro.util;
+
+import java.io.Closeable;
+import java.io.IOException;
+
+/**
+ * To use try-with-resources idiom, this class supports wrapping existing
ByteSource
+ * object or byte array. At end of try block, it gets zeroed out automatically.
+ */
+public class ByteSourceWrapper implements Closeable {
+ private byte[] bytes;
+
+ private ByteSourceWrapper(byte[] bytes) {
+ this.bytes = bytes;
+ }
+
+ /**
+ * This method generically accepts byte array or ByteSource instance.
+ */
+ public static ByteSourceWrapper wrap(Object value) {
+ if (value instanceof byte[]) {
+ byte[] bytes = (byte[]) value;
+ return new ByteSourceWrapper(bytes);
+ } else if (value instanceof ByteSource) {
+ byte[] bytes = ((ByteSource) value).getBytes();
+ return new ByteSourceWrapper(bytes);
+ }
+ throw new IllegalArgumentException();
+ }
+
+ public byte[] getBytes() {
+ return bytes;
+ }
+
+ public void close() throws IOException {
+ CollectionUtils.wipe(bytes);
+ }
+}
diff --git a/lang/src/main/java/org/apache/shiro/util/CollectionUtils.java
b/lang/src/main/java/org/apache/shiro/util/CollectionUtils.java
index 67e9901..1fe446f 100644
--- a/lang/src/main/java/org/apache/shiro/util/CollectionUtils.java
+++ b/lang/src/main/java/org/apache/shiro/util/CollectionUtils.java
@@ -107,6 +107,20 @@ public class CollectionUtils {
return Arrays.asList(elements);
}
+ /**
+ * For security, sensitive information in array should be zeroed-out at
end of use (SHIRO-349).
+ * @param value An array holding sensitive data
+ */
+ public static void wipe(Object value) {
+ if (value instanceof byte[]) {
+ byte[] array = (byte[]) value;
+ Arrays.fill(array, (byte) 0);
+ } else if (value instanceof char[]) {
+ char[] array = (char[]) value;
+ Arrays.fill(array, '\u0000');
+ }
+ }
+
/*public static <E> Deque<E> asDeque(E... elements) {
if (elements == null || elements.length == 0) {
return new ArrayDeque<E>();
