SHIRO-349 Security: Byte arrays (and other memory) holding sensitive …

core/src/main/java/org/apache/shiro/mgt/AbstractRememberMeManager.java

@@ -22,20 +22,22 @@
 import org.apache.shiro.authc.AuthenticationInfo;
 import org.apache.shiro.authc.AuthenticationToken;
 import org.apache.shiro.authc.RememberMeAuthenticationToken;
+import org.apache.shiro.crypto.cipher.ByteSourceBroker;
 import org.apache.shiro.crypto.cipher.AesCipherService;
 import org.apache.shiro.crypto.cipher.CipherService;
 import org.apache.shiro.lang.io.DefaultSerializer;
 import org.apache.shiro.lang.io.Serializer;
+import org.apache.shiro.lang.util.ByteSource;
 import org.apache.shiro.subject.PrincipalCollection;
 import org.apache.shiro.subject.Subject;
 import org.apache.shiro.subject.SubjectContext;
-import org.apache.shiro.lang.util.ByteSource;
+import org.apache.shiro.util.ByteUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /**
  * Abstract implementation of the {@code RememberMeManager} interface that handles
- * {@link #setSerializer(org.apache.shiro.lang.io.Serializer) serialization} and
+ * {@link #setSerializer(Serializer)  serialization} and
  * {@link #setCipherService encryption} of the remembered user identity.
  * <p/>
  * The remembered identity storage location and details are left to subclasses.
@@ -378,14 +380,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 {
+            ByteUtils.wipe(bytes);
         }
 
         return principals;
@@ -478,8 +483,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;
     }

crypto/cipher/src/main/java/org/apache/shiro/crypto/cipher/ByteSourceBroker.java

@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.cipher;
+
+/**
+ * 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();
+}

crypto/cipher/src/main/java/org/apache/shiro/crypto/cipher/ByteSourceUser.java

@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.cipher;
+
+/**
+ * {@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);
+}

crypto/cipher/src/main/java/org/apache/shiro/crypto/cipher/CipherService.java

@@ -96,7 +96,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

crypto/cipher/src/main/java/org/apache/shiro/crypto/cipher/JcaCipherService.java

@@ -345,7 +345,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;
 
@@ -380,10 +384,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));

crypto/cipher/src/main/java/org/apache/shiro/crypto/cipher/SimpleByteSourceBroker.java

@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.shiro.crypto.cipher;
+
+import org.apache.shiro.lang.util.ByteSource;
+import org.apache.shiro.lang.util.Destroyable;
+import org.apache.shiro.util.ByteSourceWrapper;
+import org.apache.shiro.util.ByteUtils;
+
+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;
+                ByteUtils.wipe(ciphertext);
+                ciphertext = null;
+                ByteUtils.wipe(key);
+                key = null;
+            }
+        }
+    }
+}

crypto/cipher/src/test/groovy/org/apache/shiro/crypto/cipher/AesCipherServiceTest.groovy

@@ -19,15 +19,16 @@
 package org.apache.shiro.crypto.cipher
 
 
+import org.apache.shiro.lang.codec.CodecSupport
+import org.apache.shiro.lang.util.ByteSource
+import org.apache.shiro.lang.util.Destroyable
+import org.apache.shiro.util.ByteUtils
 import org.bouncycastle.jce.provider.BouncyCastleProvider
+import org.junit.Test
 
 import java.security.Security
 
-import static org.junit.Assert.*;
-
-import org.apache.shiro.lang.codec.CodecSupport
-import org.apache.shiro.lang.util.ByteSource
-import org.junit.Test
+import static org.junit.Assert.assertTrue
 
 /**
  * Test class for the AesCipherService class.
@@ -51,12 +52,60 @@ class AesCipherServiceTest {
         assertBlock(cipher)
     }
 
+    @Test
+    void testBlockOperations_ByteSource() {
+        AesCipherService aes = new AesCipherService();
+
+        byte[] key = aes.generateNewKey().getEncoded();
+
+        for (String plain : PLAINTEXTS) {
+            byte[] plaintext = CodecSupport.toBytes(plain);
+            ByteSource ciphertext = aes.encrypt(plaintext, key);
+            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();
+            }
+        }
+    }
+
     @Test
     void testStreamingOperations() {
         AesCipherService cipher = new AesCipherService()
         assertStreaming(cipher)
     }
 
+    @Test
+    void testStreamingOperations_ByteSource() {
+
+        AesCipherService cipher = new AesCipherService();
+        byte[] key = cipher.generateNewKey().getEncoded();
+
+        for (String plain : PLAINTEXTS) {
+            byte[] plaintext = CodecSupport.toBytes(plain);
+            InputStream plainIn = new ByteArrayInputStream(plaintext);
+            ByteArrayOutputStream cipherOut = new ByteArrayOutputStream();
+            cipher.encrypt(plainIn, cipherOut, key);
+
+            byte[] ciphertext = cipherOut.toByteArray();
+            InputStream cipherIn = new ByteArrayInputStream(ciphertext);
+            ByteArrayOutputStream plainOut = new ByteArrayOutputStream();
+            cipher.decrypt(cipherIn, plainOut, key);
+
+            byte[] decrypted = plainOut.toByteArray();
+            try {
+                assertTrue(Arrays.equals(plaintext, decrypted));
+            } finally {
+                ByteUtils.wipe(decrypted);
+            }
+        }
+    }
+
     @Test
     void testAesGcm() {
         assertBlock(OperationMode.GCM)
@@ -155,8 +204,8 @@ class AesCipherServiceTest {
         for (String plain : PLAINTEXTS) {
             byte[] plaintext = CodecSupport.toBytes(plain)
             ByteSource ciphertext = cipher.encrypt(plaintext, key)
-            ByteSource decrypted = cipher.decrypt(ciphertext.getBytes(), key)
-            assertTrue(Arrays.equals(plaintext, decrypted.getBytes()))
+            ByteSourceBroker decrypted = cipher.decrypt(ciphertext.getBytes(), key)
+            assertTrue(Arrays.equals(plaintext, decrypted.getClonedBytes()))
         }
     }
 

crypto/cipher/src/test/groovy/org/apache/shiro/crypto/cipher/BlowfishCipherServiceTest.groovy

@@ -18,8 +18,10 @@
  */
 package org.apache.shiro.crypto.cipher
 
+
 import org.apache.shiro.lang.codec.CodecSupport
 import org.apache.shiro.lang.util.ByteSource
+import org.apache.shiro.util.ByteUtils
 import org.junit.Test
 
 import static org.junit.Assert.assertTrue
@@ -45,8 +47,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 {
+                ByteUtils.wipe(decrypted);
+            }
         }
     }
 
@@ -68,7 +75,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 {
+                ByteUtils.wipe(decrypted);
+            }
         }
 
     }