8208583: Better management of internal KeyStore buffers

8207775: Better management of CipherCore buffers
Reviewed-by: weijun, ascarpino

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

 /*
- * Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -673,7 +673,12 @@ final class CipherCore {
             if (len == output.length) {
                 return output;
             } else {
-                return Arrays.copyOf(output, len);
+                byte[] copy = Arrays.copyOf(output, len);
+                if (decrypting) {
+                    // Zero out internal buffer which is no longer required
+                    Arrays.fill(output, (byte) 0x00);
+                }
+                return copy;
             }
         } catch (ShortBufferException e) {
             // should never happen
@@ -767,11 +772,14 @@ final class CipherCore {
                         inputLen -= temp;
                         buffered = Math.addExact(buffered, temp);
                     }
-                    // process 'buffer'
+                    // process 'buffer'. When finished we can null out 'buffer'
+                    // Only necessary to null out if buffer holds data for encryption
                     if (decrypting) {
                          outLen = cipher.decrypt(buffer, 0, buffered, output, outputOffset);
                     } else {
                          outLen = cipher.encrypt(buffer, 0, buffered, output, outputOffset);
+                         //encrypt mode. Zero out internal (input) buffer
+                         Arrays.fill(buffer, (byte) 0x00);
                     }
                     outputOffset = Math.addExact(outputOffset, outLen);
                     buffered = 0;
@@ -846,7 +854,12 @@ final class CipherCore {
             output = new byte[getOutputSizeByOperation(inputLen, true)];
             int len = doFinal(input, inputOffset, inputLen, output, 0);
             if (len < output.length) {
-                return Arrays.copyOf(output, len);
+                byte[] copy = Arrays.copyOf(output, len);
+                if (decrypting) {
+                    // Zero out internal (ouput) array
+                    Arrays.fill(output, (byte) 0x00);
+                }
+                return copy;
             } else {
                 return output;
             }
@@ -959,6 +972,11 @@ final class CipherCore {
             finalOffset = 0;
             if (buffered != 0) {
                 System.arraycopy(buffer, 0, finalBuf, 0, buffered);
+                if (!decrypting) {
+                    // done with input buffer. We should zero out the
+                    // data if we're in encrypt mode.
+                    Arrays.fill(buffer, (byte) 0x00);
+                }
             }
             if (inputLen != 0) {
                 System.arraycopy(input, inputOffset, finalBuf,
@@ -1005,6 +1023,8 @@ final class CipherCore {
             }
             // copy the result into user-supplied output buffer
             System.arraycopy(outWithPadding, 0, output, outputOffset, outLen);
+            // decrypt mode. Zero out output data that's not required
+            Arrays.fill(outWithPadding, (byte) 0x00);
         } else { // encrypting
             try {
                 outLen = finalNoPadding(finalBuf, finalOffset, output,
@@ -1012,6 +1032,10 @@ final class CipherCore {
             } finally {
                 // reset after doFinal() for GCM encryption
                 requireReinit = (cipherMode == GCM_MODE);
+                if (finalBuf != input) {
+                    // done with internal finalBuf array. Copied to output
+                    Arrays.fill(finalBuf, (byte) 0x00);
+                }
             }
         }
 

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

@@ -37,12 +37,15 @@ import java.security.UnrecoverableKeyException;
 import java.security.AlgorithmParameters;
 import java.security.spec.InvalidParameterSpecException;
 import java.security.spec.PKCS8EncodedKeySpec;
+import java.util.Arrays;
 
 import javax.crypto.Cipher;
 import javax.crypto.CipherSpi;
 import javax.crypto.SecretKey;
 import javax.crypto.SealedObject;
 import javax.crypto.spec.*;
+import javax.security.auth.DestroyFailedException;
+
 import sun.security.x509.AlgorithmId;
 import sun.security.util.ObjectIdentifier;
 
@@ -103,15 +106,20 @@ final class KeyProtector {
 
         // create PBE key from password
         PBEKeySpec pbeKeySpec = new PBEKeySpec(this.password);
-        SecretKey sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
-        pbeKeySpec.clearPassword();
-
-        // encrypt private key
+        SecretKey sKey = null;
         PBEWithMD5AndTripleDESCipher cipher;
+        try {
+            sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
+            // encrypt private key
             cipher = new PBEWithMD5AndTripleDESCipher();
             cipher.engineInit(Cipher.ENCRYPT_MODE, sKey, pbeSpec, null);
+        } finally {
+            pbeKeySpec.clearPassword();
+            if (sKey != null) sKey.destroy();
+        }
         byte[] plain = key.getEncoded();
         byte[] encrKey = cipher.engineDoFinal(plain, 0, plain.length);
+        Arrays.fill(plain, (byte) 0x00);
 
         // wrap encrypted private key in EncryptedPrivateKeyInfo
         // (as defined in PKCS#8)
@@ -131,8 +139,8 @@ final class KeyProtector {
     Key recover(EncryptedPrivateKeyInfo encrInfo)
         throws UnrecoverableKeyException, NoSuchAlgorithmException
     {
-        byte[] plain;
-
+        byte[] plain = null;
+        SecretKey sKey = null;
         try {
             String encrAlg = encrInfo.getAlgorithm().getOID().toString();
             if (!encrAlg.equals(PBE_WITH_MD5_AND_DES3_CBC_OID)
@@ -160,8 +168,7 @@ final class KeyProtector {
 
                 // create PBE key from password
                 PBEKeySpec pbeKeySpec = new PBEKeySpec(this.password);
-                SecretKey sKey =
-                    new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
+                sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
                 pbeKeySpec.clearPassword();
 
                 // decrypt private key
@@ -178,7 +185,6 @@ final class KeyProtector {
                 (new PrivateKeyInfo(plain).getAlgorithm().getOID()).getName();
             KeyFactory kFac = KeyFactory.getInstance(oidName);
             return kFac.generatePrivate(new PKCS8EncodedKeySpec(plain));
-
         } catch (NoSuchAlgorithmException ex) {
             // Note: this catch needed to be here because of the
             // later catch of GeneralSecurityException
@@ -187,6 +193,15 @@ final class KeyProtector {
             throw new UnrecoverableKeyException(ioe.getMessage());
         } catch (GeneralSecurityException gse) {
             throw new UnrecoverableKeyException(gse.getMessage());
+        } finally {
+            if (plain != null) Arrays.fill(plain, (byte) 0x00);
+            if (sKey != null) {
+                try {
+                    sKey.destroy();
+                } catch (DestroyFailedException e) {
+                    //shouldn't happen
+                }
+            }
         }
     }
 
@@ -262,7 +277,7 @@ final class KeyProtector {
         // of <code>protectedKey</code>. If the two digest values are
         // different, throw an exception.
         md.update(passwdBytes);
-        java.util.Arrays.fill(passwdBytes, (byte)0x00);
+        Arrays.fill(passwdBytes, (byte)0x00);
         passwdBytes = null;
         md.update(plainKey);
         digest = md.digest();
@@ -291,17 +306,21 @@ final class KeyProtector {
 
         // create PBE key from password
         PBEKeySpec pbeKeySpec = new PBEKeySpec(this.password);
-        SecretKey sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
+        SecretKey sKey = null;
+        Cipher cipher;
+        try {
+            sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
             pbeKeySpec.clearPassword();
 
             // seal key
-        Cipher cipher;
-
             PBEWithMD5AndTripleDESCipher cipherSpi;
             cipherSpi = new PBEWithMD5AndTripleDESCipher();
             cipher = new CipherForKeyProtector(cipherSpi, SunJCE.getInstance(),
                                            "PBEWithMD5AndTripleDES");
             cipher.init(Cipher.ENCRYPT_MODE, sKey, pbeSpec);
+        } finally {
+            if (sKey != null) sKey.destroy();
+        }
         return new SealedObjectForKeyProtector(key, cipher);
     }
 
@@ -309,12 +328,12 @@ final class KeyProtector {
      * Unseals the sealed key.
      */
     Key unseal(SealedObject so)
-        throws NoSuchAlgorithmException, UnrecoverableKeyException
-    {
+        throws NoSuchAlgorithmException, UnrecoverableKeyException {
+        SecretKey sKey = null;
         try {
             // create PBE key from password
             PBEKeySpec pbeKeySpec = new PBEKeySpec(this.password);
-            SecretKey skey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
+            sKey = new PBEKey(pbeKeySpec, "PBEWithMD5AndTripleDES");
             pbeKeySpec.clearPassword();
 
             SealedObjectForKeyProtector soForKeyProtector = null;
@@ -342,7 +361,7 @@ final class KeyProtector {
             Cipher cipher = new CipherForKeyProtector(cipherSpi,
                                                       SunJCE.getInstance(),
                                                       "PBEWithMD5AndTripleDES");
-            cipher.init(Cipher.DECRYPT_MODE, skey, params);
+            cipher.init(Cipher.DECRYPT_MODE, sKey, params);
             return soForKeyProtector.getKey(cipher);
         } catch (NoSuchAlgorithmException ex) {
             // Note: this catch needed to be here because of the
@@ -354,6 +373,14 @@ final class KeyProtector {
             throw new UnrecoverableKeyException(cnfe.getMessage());
         } catch (GeneralSecurityException gse) {
             throw new UnrecoverableKeyException(gse.getMessage());
+        } finally {
+            if (sKey != null) {
+                try {
+                    sKey.destroy();
+                } catch (DestroyFailedException e) {
+                    //shouldn't happen
+                }
+            }
         }
     }
 }

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

 /*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,6 +28,7 @@ package com.sun.crypto.provider;
 import java.security.MessageDigest;
 import java.security.KeyRep;
 import java.security.spec.InvalidKeySpecException;
+import java.util.Arrays;
 import java.util.Locale;
 import javax.crypto.SecretKey;
 import javax.crypto.spec.PBEKeySpec;
@@ -68,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);
-        java.util.Arrays.fill(passwd, ' ');
+        Arrays.fill(passwd, ' ');
         type = keytype;
     }
 
@@ -110,10 +111,22 @@ final class PBEKey implements SecretKey {
 
         byte[] thatEncoded = that.getEncoded();
         boolean ret = MessageDigest.isEqual(this.key, thatEncoded);
-        java.util.Arrays.fill(thatEncoded, (byte)0x00);
+        Arrays.fill(thatEncoded, (byte)0x00);
         return ret;
     }
 
+    /**
+     * Clears the internal copy of the key.
+     *
+     */
+    @Override
+    public void destroy() {
+        if (key != null) {
+            Arrays.fill(key, (byte) 0x00);
+            key = null;
+        }
+    }
+
     /**
      * readObject is called to restore the state of this key from
      * a stream.

src/share/classes/javax/crypto/spec/PBEKeySpec.java

 /*
- * Copyright (c) 1997, 2011, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,6 +26,7 @@
 package javax.crypto.spec;
 
 import java.security.spec.KeySpec;
+import java.util.Arrays;
 
 /**
  * A user-chosen password that can be used with password-based encryption
@@ -174,9 +175,7 @@ public class PBEKeySpec implements KeySpec {
      */
     public final void clearPassword() {
         if (password != null) {
-            for (int i = 0; i < password.length; i++) {
-                password[i] = ' ';
-            }
+            Arrays.fill(password, ' ');
             password = null;
         }
     }

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

 /*
- * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -132,18 +132,20 @@ abstract class JavaKeyStore extends KeyStoreSpi {
             throw new UnrecoverableKeyException("Password must not be null");
         }
 
-        KeyProtector keyProtector = new KeyProtector(password);
+        byte[] passwordBytes = convertToBytes(password);
+        KeyProtector keyProtector = new KeyProtector(passwordBytes);
         byte[] encrBytes = ((KeyEntry)entry).protectedPrivKey;
         EncryptedPrivateKeyInfo encrInfo;
-        byte[] plain;
         try {
             encrInfo = new EncryptedPrivateKeyInfo(encrBytes);
+            return keyProtector.recover(encrInfo);
         } catch (IOException ioe) {
             throw new UnrecoverableKeyException("Private key not stored as "
                                                 + "PKCS #8 "
                                                 + "EncryptedPrivateKeyInfo");
+        } finally {
+            Arrays.fill(passwordBytes, (byte) 0x00);
         }
-        return keyProtector.recover(encrInfo);
     }
 
     /**
@@ -252,7 +254,8 @@ abstract class JavaKeyStore extends KeyStoreSpi {
                                   Certificate[] chain)
         throws KeyStoreException
     {
-        KeyProtector keyProtector = null;
+        KeyProtector keyProtector;
+        byte[] passwordBytes = null;
 
         if (!(key instanceof java.security.PrivateKey)) {
             throw new KeyStoreException("Cannot store non-PrivateKeys");
@@ -263,7 +266,8 @@ abstract class JavaKeyStore extends KeyStoreSpi {
                 entry.date = new Date();
 
                 // Protect the encoding of the key
-                keyProtector = new KeyProtector(password);
+                passwordBytes = convertToBytes(password);
+                keyProtector = new KeyProtector(passwordBytes);
                 entry.protectedPrivKey = keyProtector.protect(key);
 
                 // clone the chain
@@ -279,7 +283,8 @@ abstract class JavaKeyStore extends KeyStoreSpi {
         } catch (NoSuchAlgorithmException nsae) {
             throw new KeyStoreException("Key protection algorithm not found");
         } finally {
-            keyProtector = null;
+            if (passwordBytes != null)
+                Arrays.fill(passwordBytes, (byte) 0x00);
         }
     }
 
@@ -793,18 +798,26 @@ abstract class JavaKeyStore extends KeyStoreSpi {
     private MessageDigest getPreKeyedHash(char[] password)
         throws NoSuchAlgorithmException, UnsupportedEncodingException
     {
-        int i, j;
 
         MessageDigest md = MessageDigest.getInstance("SHA");
+        byte[] passwdBytes = convertToBytes(password);
+        md.update(passwdBytes);
+        Arrays.fill(passwdBytes, (byte) 0x00);
+        md.update("Mighty Aphrodite".getBytes("UTF8"));
+        return md;
+    }
+
+    /**
+     * Helper method to convert char[] to byte[]
+     */
+
+    private byte[] convertToBytes(char[] password) {
+        int i, j;
         byte[] passwdBytes = new byte[password.length * 2];
         for (i=0, j=0; i<password.length; i++) {
             passwdBytes[j++] = (byte)(password[i] >> 8);
             passwdBytes[j++] = (byte)password[i];
         }
-        md.update(passwdBytes);
-        for (i=0; i<passwdBytes.length; i++)
-            passwdBytes[i] = 0;
-        md.update("Mighty Aphrodite".getBytes("UTF8"));
-        return md;
+        return passwdBytes;
     }
 }

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

 /*
- * Copyright (c) 1997, 2006, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -119,28 +119,15 @@ final class KeyProtector {
     /**
      * Creates an instance of this class, and initializes it with the given
      * password.
-     *
-     * <p>The password is expected to be in printable ASCII.
-     * Normal rules for good password selection apply: at least
-     * seven characters, mixed case, with punctuation encouraged.
-     * Phrases or words which are easily guessed, for example by
-     * being found in dictionaries, are bad.
      */
-    public KeyProtector(char[] password)
+    public KeyProtector(byte[] passwordBytes)
         throws NoSuchAlgorithmException
     {
-        int i, j;
-
-        if (password == null) {
+        if (passwordBytes == null) {
            throw new IllegalArgumentException("password can't be null");
         }
         md = MessageDigest.getInstance(DIGEST_ALG);
-        // Convert password to byte array, so that it can be digested
-        passwdBytes = new byte[password.length * 2];
-        for (i=0, j=0; i<password.length; i++) {
-            passwdBytes[j++] = (byte)(password[i] >> 8);
-            passwdBytes[j++] = (byte)password[i];
-        }
+        this.passwdBytes = passwordBytes;
     }
 
     /**