From e2bde8de4b4aa82ddbaacbab88200ddc1e9a3248 Mon Sep 17 00:00:00 2001 From: Taehun Cho Date: Tue, 16 Aug 2016 10:37:18 +0900 Subject: [PATCH] SHIRO-349 Security: Byte arrays (and other memory) holding sensitive data (even temporarily) should be zerod-out --- .../shiro/mgt/AbstractRememberMeManager.java | 11 +++- .../subject/SimplePrincipalCollection.java | 2 +- .../apache/shiro/crypto/ByteSourceBroker.java | 29 ++++++++++ .../apache/shiro/crypto/ByteSourceUser.java | 12 ++++ .../apache/shiro/crypto/CipherService.java | 2 +- .../apache/shiro/crypto/JcaCipherService.java | 10 +++- .../shiro/crypto/SimpleByteSourceBroker.java | 57 +++++++++++++++++++ .../shiro/crypto/AesCipherServiceTest.groovy | 20 ++++++- .../crypto/BlowfishCipherServiceTest.groovy | 16 +++++- .../apache/shiro/util/ByteSourceWrapper.java | 38 +++++++++++++ .../apache/shiro/util/CollectionUtils.java | 14 +++++ 11 files changed, 197 insertions(+), 14 deletions(-) create mode 100644 crypto/cipher/src/main/java/org/apache/shiro/crypto/ByteSourceBroker.java create mode 100644 crypto/cipher/src/main/java/org/apache/shiro/crypto/ByteSourceUser.java create mode 100644 crypto/cipher/src/main/java/org/apache/shiro/crypto/SimpleByteSourceBroker.java create mode 100644 lang/src/main/java/org/apache/shiro/util/ByteSourceWrapper.java 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 c857ef915a..63354bdbe7 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.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.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 @@ protected byte[] convertPrincipalsToBytes(PrincipalCollection principals) { */ 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 @@ protected byte[] decrypt(byte[] encrypted) { 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 9b17f2aa14..b15bb1ef1b 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 @@ private void writeObject(ObjectOutputStream out) throws IOException { * input stream. *

* 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 0000000000..1b356d3428 --- /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. + *
+ * {@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. + *
+ * 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. + *
+ * 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 0000000000..12195d64e3 --- /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. + *
+ * 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 8dda1acfa7..d5405a5e9a 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 bb215568e3..825069ec22 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 @@ private ByteSource encrypt(byte[] plaintext, byte[] key, byte[] iv, boolean prep 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 ByteSource decrypt(byte[] ciphertext, byte[] key) throws CryptoException } } - 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 0000000000..cc4390e635 --- /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 f35751e72d..b95fddd992 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 eaadf55553..5fbc4788fb 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 0000000000..2933343858 --- /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 67e990139d..1fe446f8b3 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 static List asList(E... elements) { 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 Deque asDeque(E... elements) { if (elements == null || elements.length == 0) { return new ArrayDeque();