8209129: Further improvements to cipher buffer management

Reviewed-by: weijun, igerasim

src/share/classes/com/sun/crypto/provider/HmacPKCS12PBESHA1.java

@@ -73,18 +73,22 @@ public final class HmacPKCS12PBESHA1 extends HmacCore {
             salt = pbeKey.getSalt(); // maybe null if unspecified
             iCount = pbeKey.getIterationCount(); // maybe 0 if unspecified
         } else if (key instanceof SecretKey) {
-            byte[] passwdBytes = key.getEncoded();
-            if ((passwdBytes == null) ||
-                !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3))) {
+            byte[] passwdBytes;
+            if (!(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3)) ||
+                    (passwdBytes = key.getEncoded()) == null) {
                 throw new InvalidKeyException("Missing password");
             }
             passwdChars = new char[passwdBytes.length];
             for (int i=0; i<passwdChars.length; i++) {
                 passwdChars[i] = (char) (passwdBytes[i] & 0x7f);
             }
+            Arrays.fill(passwdBytes, (byte)0x00);
         } else {
             throw new InvalidKeyException("SecretKey of PBE type required");
         }
+
+        byte[] derivedKey;
+        try {
             if (params == null) {
                 // should not auto-generate default values since current
                 // javax.crypto.Mac api does not have any method for caller to
@@ -127,8 +131,11 @@ public final class HmacPKCS12PBESHA1 extends HmacCore {
                 throw new InvalidAlgorithmParameterException
                         ("IterationCount must be a positive number");
             }
-        byte[] derivedKey = PKCS12PBECipherCore.derive(passwdChars, salt,
+            derivedKey = PKCS12PBECipherCore.derive(passwdChars, salt,
                     iCount, engineGetMacLength(), PKCS12PBECipherCore.MAC_KEY);
+        } finally {
+            Arrays.fill(passwdChars, '\0');
+        }
         SecretKey cipherKey = new SecretKeySpec(derivedKey, "HmacSHA1");
         super.engineInit(cipherKey, null);
     }

src/share/classes/com/sun/crypto/provider/KeyProtector.java

@@ -119,7 +119,7 @@ final class KeyProtector {
         }
         byte[] plain = key.getEncoded();
         byte[] encrKey = cipher.engineDoFinal(plain, 0, plain.length);
-        Arrays.fill(plain, (byte) 0x00);
+        Arrays.fill(plain, (byte)0x00);
 
         // wrap encrypted private key in EncryptedPrivateKeyInfo
         // (as defined in PKCS#8)
@@ -194,7 +194,7 @@ final class KeyProtector {
         } catch (GeneralSecurityException gse) {
             throw new UnrecoverableKeyException(gse.getMessage());
         } finally {
-            if (plain != null) Arrays.fill(plain, (byte) 0x00);
+            if (plain != null) Arrays.fill(plain, (byte)0x00);
             if (sKey != null) {
                 try {
                     sKey.destroy();

src/share/classes/com/sun/crypto/provider/PBEKey.java

@@ -69,7 +69,7 @@ final class PBEKey implements SecretKey {
         this.key = new byte[passwd.length];
         for (int i=0; i<passwd.length; i++)
             this.key[i] = (byte) (passwd[i] & 0x7f);
-        Arrays.fill(passwd, ' ');
+        Arrays.fill(passwd, '\0');
         type = keytype;
     }
 
@@ -122,7 +122,7 @@ final class PBEKey implements SecretKey {
     @Override
     public void destroy() {
         if (key != null) {
-            Arrays.fill(key, (byte) 0x00);
+            Arrays.fill(key, (byte)0x00);
             key = null;
         }
     }

src/share/classes/com/sun/crypto/provider/PBES1Core.java

@@ -27,6 +27,7 @@ package com.sun.crypto.provider;
 
 import java.security.*;
 import java.security.spec.*;
+import java.util.Arrays;
 import javax.crypto.*;
 import javax.crypto.spec.*;
 
@@ -213,8 +214,14 @@ final class PBES1Core {
             throw new InvalidAlgorithmParameterException("Parameters "
                                                          + "missing");
         }
-        if ((key == null) ||
-            (key.getEncoded() == null) ||
+        if (key == null) {
+            throw new InvalidKeyException("Null key");
+        }
+
+        byte[] derivedKey;
+        byte[] passwdBytes = key.getEncoded();
+        try {
+            if ((passwdBytes == null) ||
                     !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3))) {
                 throw new InvalidKeyException("Missing password");
             }
@@ -240,8 +247,10 @@ final class PBES1Core {
                             ("IterationCount must be a positive number");
                 }
             }
-
-        byte[] derivedKey = deriveCipherKey(key);
+            derivedKey = deriveCipherKey(passwdBytes);
+        } finally {
+            if (passwdBytes != null) Arrays.fill(passwdBytes, (byte) 0x00);
+        }
         // use all but the last 8 bytes as the key value
         SecretKeySpec cipherKey = new SecretKeySpec(derivedKey, 0,
                                                     derivedKey.length-8, algo);
@@ -253,16 +262,14 @@ final class PBES1Core {
         cipher.init(opmode, cipherKey, ivSpec, random);
     }
 
-    private byte[] deriveCipherKey(Key key) {
+    private byte[] deriveCipherKey(byte[] passwdBytes) {
 
         byte[] result = null;
-        byte[] passwdBytes = key.getEncoded();
 
         if (algo.equals("DES")) {
             // P || S (password concatenated with salt)
             byte[] concat = new byte[Math.addExact(passwdBytes.length, salt.length)];
             System.arraycopy(passwdBytes, 0, concat, 0, passwdBytes.length);
-            java.util.Arrays.fill(passwdBytes, (byte)0x00);
             System.arraycopy(salt, 0, concat, passwdBytes.length, salt.length);
 
             // digest P || S with c iterations
@@ -271,7 +278,7 @@ final class PBES1Core {
                 md.update(toBeHashed);
                 toBeHashed = md.digest(); // this resets the digest
             }
-            java.util.Arrays.fill(concat, (byte)0x00);
+            Arrays.fill(concat, (byte)0x00);
             result = toBeHashed;
         } else if (algo.equals("DESede")) {
             // if the 2 salt halves are the same, invert one of them
@@ -294,8 +301,6 @@ final class PBES1Core {
             // Concatenate the output from each digest round with the
             // password, and use the result as the input to the next digest
             // operation.
-            byte[] kBytes = null;
-            IvParameterSpec iv = null;
             byte[] toBeHashed = null;
             result = new byte[DESedeKeySpec.DES_EDE_KEY_LEN +
                               DESConstants.DES_BLOCK_SIZE];
@@ -306,12 +311,14 @@ final class PBES1Core {
                 for (int j=0; j < iCount; j++) {
                     md.update(toBeHashed);
                     md.update(passwdBytes);
-                    toBeHashed = md.digest(); // this resets the digest
+                    toBeHashed = md.digest();
                 }
                 System.arraycopy(toBeHashed, 0, result, i*16,
                                  toBeHashed.length);
             }
         }
+        // clear data used in message
+        md.reset();
         return result;
     }
 
@@ -478,9 +485,9 @@ final class PBES1Core {
     byte[] wrap(Key key)
         throws IllegalBlockSizeException, InvalidKeyException {
         byte[] result = null;
-
+        byte[] encodedKey = null;
         try {
-            byte[] encodedKey = key.getEncoded();
+            encodedKey = key.getEncoded();
             if ((encodedKey == null) || (encodedKey.length == 0)) {
                 throw new InvalidKeyException("Cannot get an encoding of " +
                                               "the key to be wrapped");
@@ -489,6 +496,8 @@ final class PBES1Core {
             result = doFinal(encodedKey, 0, encodedKey.length);
         } catch (BadPaddingException e) {
             // Should never happen
+        } finally {
+            if (encodedKey != null) Arrays.fill(encodedKey, (byte)0x00);
         }
 
         return result;

src/share/classes/com/sun/crypto/provider/PBES2Core.java

@@ -27,6 +27,7 @@ package com.sun.crypto.provider;
 
 import java.security.*;
 import java.security.spec.*;
+import java.util.Arrays;
 import javax.crypto.*;
 import javax.crypto.spec.*;
 
@@ -173,8 +174,15 @@ abstract class PBES2Core extends CipherSpi {
                               SecureRandom random)
         throws InvalidKeyException, InvalidAlgorithmParameterException {
 
-        if ((key == null) ||
-            (key.getEncoded() == null) ||
+        if (key == null) {
+            throw new InvalidKeyException("Null key");
+        }
+
+        byte[] passwdBytes = key.getEncoded();
+        char[] passwdChars = null;
+        PBEKeySpec pbeSpec;
+        try {
+            if ((passwdBytes == null) ||
                     !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3))) {
                 throw new InvalidKeyException("Missing password");
             }
@@ -254,20 +262,17 @@ abstract class PBES2Core extends CipherSpi {
                 }
             }
 
-        SecretKeySpec cipherKey = null;
-        byte[] derivedKey = null;
-        byte[] passwdBytes = key.getEncoded();
-        char[] passwdChars = new char[passwdBytes.length];
-
-        for (int i=0; i<passwdChars.length; i++)
+            passwdChars = new char[passwdBytes.length];
+            for (int i = 0; i < passwdChars.length; i++)
                 passwdChars[i] = (char) (passwdBytes[i] & 0x7f);
 
-        PBEKeySpec pbeSpec =
-            new PBEKeySpec(passwdChars, salt, iCount, keyLength);
+            pbeSpec = new PBEKeySpec(passwdChars, salt, iCount, keyLength);
             // password char[] was cloned in PBEKeySpec constructor,
             // so we can zero it out here
-        java.util.Arrays.fill(passwdChars, ' ');
-        java.util.Arrays.fill(passwdBytes, (byte)0x00);
+        } finally {
+            if (passwdChars != null) Arrays.fill(passwdChars, '\0');
+            if (passwdBytes != null) Arrays.fill(passwdBytes, (byte)0x00);
+        }
 
         SecretKey s = null;
 
@@ -280,8 +285,8 @@ abstract class PBES2Core extends CipherSpi {
             ike.initCause(ikse);
             throw ike;
         }
-        derivedKey = s.getEncoded();
-        cipherKey = new SecretKeySpec(derivedKey, cipherAlgo);
+        byte[] derivedKey = s.getEncoded();
+        SecretKeySpec cipherKey = new SecretKeySpec(derivedKey, cipherAlgo);
 
         // initialize the underlying cipher
         cipher.init(opmode, cipherKey, ivSpec, random);

src/share/classes/com/sun/crypto/provider/PBKDF2KeyImpl.java

@@ -89,6 +89,8 @@ final class PBKDF2KeyImpl implements javax.crypto.interfaces.PBEKey {
         }
         // Convert the password from char[] to byte[]
         byte[] passwdBytes = getPasswordBytes(this.passwd);
+        // remove local copy
+        if (passwd != null) Arrays.fill(passwd, '\0');
 
         this.salt = keySpec.getSalt();
         if (salt == null) {
@@ -108,13 +110,15 @@ final class PBKDF2KeyImpl implements javax.crypto.interfaces.PBEKey {
         }
         try {
             this.prf = Mac.getInstance(prfAlgo, SunJCE.getInstance());
+            this.key = deriveKey(prf, passwdBytes, salt, iterCount, keyLength);
         } catch (NoSuchAlgorithmException nsae) {
             // not gonna happen; re-throw just in case
             InvalidKeySpecException ike = new InvalidKeySpecException();
             ike.initCause(nsae);
             throw ike;
+        } finally {
+            Arrays.fill(passwdBytes, (byte)0x00);
         }
-        this.key = deriveKey(prf, passwdBytes, salt, iterCount, keyLength);
     }
 
     private static byte[] deriveKey(final Mac prf, final byte[] password,
@@ -240,8 +244,8 @@ final class PBKDF2KeyImpl implements javax.crypto.interfaces.PBEKey {
         if (!(that.getFormat().equalsIgnoreCase("RAW")))
             return false;
         byte[] thatEncoded = that.getEncoded();
-        boolean ret = MessageDigest.isEqual(key, that.getEncoded());
-        java.util.Arrays.fill(thatEncoded, (byte)0x00);
+        boolean ret = MessageDigest.isEqual(key, thatEncoded);
+        Arrays.fill(thatEncoded, (byte)0x00);
         return ret;
     }
 
@@ -266,7 +270,7 @@ final class PBKDF2KeyImpl implements javax.crypto.interfaces.PBEKey {
         try {
             synchronized (this) {
                 if (this.passwd != null) {
-                    java.util.Arrays.fill(this.passwd, '0');
+                    java.util.Arrays.fill(this.passwd, '\0');
                     this.passwd = null;
                 }
                 if (this.key != null) {

src/share/classes/com/sun/crypto/provider/PBMAC1Core.java

@@ -108,18 +108,22 @@ abstract class PBMAC1Core extends HmacCore {
             salt = pbeKey.getSalt(); // maybe null if unspecified
             iCount = pbeKey.getIterationCount(); // maybe 0 if unspecified
         } else if (key instanceof SecretKey) {
-            byte[] passwdBytes = key.getEncoded();
-            if ((passwdBytes == null) ||
-                !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3))) {
+            byte[] passwdBytes;
+            if (!(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3)) ||
+                    (passwdBytes = key.getEncoded()) == null) {
                 throw new InvalidKeyException("Missing password");
             }
             passwdChars = new char[passwdBytes.length];
             for (int i=0; i<passwdChars.length; i++) {
                 passwdChars[i] = (char) (passwdBytes[i] & 0x7f);
             }
+            Arrays.fill(passwdBytes, (byte)0x00);
         } else {
             throw new InvalidKeyException("SecretKey of PBE type required");
         }
+
+        PBEKeySpec pbeSpec;
+        try {
             if (params == null) {
                 // should not auto-generate default values since current
                 // javax.crypto.Mac api does not have any method for caller to
@@ -163,17 +167,17 @@ abstract class PBMAC1Core extends HmacCore {
                         ("IterationCount must be a positive number");
             }
 
-        PBEKeySpec pbeSpec =
-            new PBEKeySpec(passwdChars, salt, iCount, blockLength);
+            pbeSpec = new PBEKeySpec(passwdChars, salt, iCount, blockLength);
             // password char[] was cloned in PBEKeySpec constructor,
             // so we can zero it out here
-        java.util.Arrays.fill(passwdChars, ' ');
+        } finally {
+            Arrays.fill(passwdChars, '\0');
+        }
 
-        SecretKey s = null;
+        SecretKey s;
         PBKDF2Core kdf = getKDFImpl(kdfAlgo);
         try {
             s = kdf.engineGenerateSecret(pbeSpec);
-
         } catch (InvalidKeySpecException ikse) {
             InvalidKeyException ike =
                 new InvalidKeyException("Cannot construct PBE key");

src/share/classes/com/sun/crypto/provider/PKCS12PBECipherCore.java

@@ -103,6 +103,7 @@ final class PKCS12PBECipherCore {
             Arrays.fill(D, (byte)type);
             concat(salt, I, 0, s);
             concat(passwd, I, s, p);
+            Arrays.fill(passwd, (byte)0x00);
 
             byte[] Ai;
             byte[] B = new byte[v];
@@ -265,19 +266,21 @@ final class PKCS12PBECipherCore {
             salt = pbeKey.getSalt(); // maybe null if unspecified
             iCount = pbeKey.getIterationCount(); // maybe 0 if unspecified
         } else if (key instanceof SecretKey) {
-            byte[] passwdBytes = key.getEncoded();
-            if ((passwdBytes == null) ||
-                !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3))) {
+            byte[] passwdBytes;
+            if (!(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3)) ||
+                    (passwdBytes = key.getEncoded()) == null) {
                 throw new InvalidKeyException("Missing password");
             }
             passwdChars = new char[passwdBytes.length];
             for (int i=0; i<passwdChars.length; i++) {
                 passwdChars[i] = (char) (passwdBytes[i] & 0x7f);
             }
+            Arrays.fill(passwdBytes, (byte)0x00);
         } else {
             throw new InvalidKeyException("SecretKey of PBE type required");
         }
 
+        try {
             if (((opmode == Cipher.DECRYPT_MODE) ||
                     (opmode == Cipher.UNWRAP_MODE)) &&
                     ((params == null) && ((salt == null) || (iCount == 0)))) {
@@ -347,6 +350,9 @@ final class PKCS12PBECipherCore {
                 // initialize the underlying cipher
                 cipher.init(opmode, cipherKey, ivSpec, random);
             }
+        } finally {
+           Arrays.fill(passwdChars, '\0');
+        }
     }
 
     void implInit(int opmode, Key key, AlgorithmParameters params,

src/share/classes/sun/security/provider/DigestBase.java

@@ -28,6 +28,7 @@ package sun.security.provider;
 import java.security.MessageDigestSpi;
 import java.security.DigestException;
 import java.security.ProviderException;
+import java.util.Arrays;
 
 /**
  * Common base message digest implementation for the Sun provider.
@@ -151,6 +152,7 @@ abstract class DigestBase extends MessageDigestSpi implements Cloneable {
         implReset();
         bufOfs = 0;
         bytesProcessed = 0;
+        Arrays.fill(buffer, (byte) 0x00);
     }
 
     // return the digest. See JCA doc.

src/share/classes/sun/security/provider/MD4.java

@@ -26,6 +26,7 @@
 package sun.security.provider;
 
 import java.security.*;
+import java.util.Arrays;
 
 import static sun.security.provider.ByteArrayAccess.*;
 
@@ -90,7 +91,7 @@ public final class MD4 extends DigestBase {
         super("MD4", 16, 64);
         state = new int[4];
         x = new int[16];
-        implReset();
+        resetHashes();
     }
 
     // clone this object
@@ -106,6 +107,12 @@ public final class MD4 extends DigestBase {
      */
     void implReset() {
         // Load magic initialization constants.
+        resetHashes();
+        // clear out old data
+        Arrays.fill(x, 0);
+    }
+
+    private void resetHashes() {
         state[0] = 0x67452301;
         state[1] = 0xefcdab89;
         state[2] = 0x98badcfe;

src/share/classes/sun/security/provider/MD5.java

@@ -25,6 +25,8 @@
 
 package sun.security.provider;
 
+import java.util.Arrays;
+
 import static sun.security.provider.ByteArrayAccess.*;
 
 /**
@@ -66,7 +68,7 @@ public final class MD5 extends DigestBase {
         super("MD5", 16, 64);
         state = new int[4];
         x = new int[16];
-        implReset();
+        resetHashes();
     }
 
     // clone this object
@@ -82,6 +84,12 @@ public final class MD5 extends DigestBase {
      */
     void implReset() {
         // Load magic initialization constants.
+        resetHashes();
+        // clear out old data
+        Arrays.fill(x, 0);
+    }
+
+    private void resetHashes() {
         state[0] = 0x67452301;
         state[1] = 0xefcdab89;
         state[2] = 0x98badcfe;

src/share/classes/sun/security/provider/SHA.java

@@ -25,6 +25,8 @@
 
 package sun.security.provider;
 
+import java.util.Arrays;
+
 import static sun.security.provider.ByteArrayAccess.*;
 
 /**
@@ -59,7 +61,7 @@ public final class SHA extends DigestBase {
         super("SHA-1", 20, 64);
         state = new int[5];
         W = new int[80];
-        implReset();
+        resetHashes();
     }
 
     /*
@@ -76,6 +78,13 @@ public final class SHA extends DigestBase {
      * Resets the buffers and hash value to start a new hash.
      */
     void implReset() {
+        // Load magic initialization constants.
+        resetHashes();
+        // clear out old data
+        Arrays.fill(W, 0);
+    }
+
+    private void resetHashes() {
         state[0] = 0x67452301;
         state[1] = 0xefcdab89;
         state[2] = 0x98badcfe;

src/share/classes/sun/security/provider/SHA2.java

@@ -25,6 +25,8 @@
 
 package sun.security.provider;
 
+import java.util.Arrays;
+
 import static sun.security.provider.ByteArrayAccess.*;
 
 /**
@@ -80,13 +82,18 @@ abstract class SHA2 extends DigestBase {
         this.initialHashes = initialHashes;
         state = new int[8];
         W = new int[64];
-        implReset();
+        resetHashes();
     }
 
     /**
      * Resets the buffers and hash value to start a new hash.
      */
     void implReset() {
+        resetHashes();
+        Arrays.fill(W, 0);
+    }
+
+    private void resetHashes() {
         System.arraycopy(initialHashes, 0, state, 0, state.length);
     }
 

src/share/classes/sun/security/provider/SHA5.java

@@ -25,8 +25,7 @@
 
 package sun.security.provider;
 
-import java.security.*;
-import java.math.BigInteger;
+import java.util.Arrays;
 
 import static sun.security.provider.ByteArrayAccess.*;
 
@@ -98,10 +97,15 @@ abstract class SHA5 extends DigestBase {
         this.initialHashes = initialHashes;
         state = new long[8];
         W = new long[80];
-        implReset();
+        resetHashes();
     }
 
     final void implReset() {
+        resetHashes();
+        Arrays.fill(W, 0L);
+    }
+
+    private void resetHashes() {
         System.arraycopy(initialHashes, 0, state, 0, state.length);
     }
 

test/com/sun/crypto/provider/Cipher/PBE/PKCS12Cipher.java

@@ -105,7 +105,7 @@ class MyPBEKey implements PBEKey {
         this.salt = salt;
         this.iCount = iCount;
     }
-    public char[] getPassword() { return passwd; }
+    public char[] getPassword() { return passwd.clone(); }
     public byte[] getSalt() { return salt; }
     public int getIterationCount() { return iCount; }
     public String getAlgorithm() { return "PBE"; }