After looking into this issue more, PBEKeySpec needs to also create a
copy of the salt, and some of the PBEKeySpec implementation is not
according to spec.
Please find attached a patch for PBEKeySpec(Crypto-PBEKeySpec.patch).
This patch now only stores clones of the password and salt arguments, as
well as checking arguments being passed for validity. The javadoc has
also been updated to reflect that only copies of the passed parameters
are being stored.
I am also attaching a mauve testlet for this class
(TestOfPBEKeySpec.java). This test should go into
gnu/testlet/javax/crypto/spec (and as this directory does not exist yet
in cvs, I could not just create a nice patch)
Please review and comment. I do not have classpath commit permissions
nor mauve commit permissions, so if this is deemed acceptable, could
someone please submit them for me.
Thanks,
Matt Wringe
On Thu, 2006-06-29 at 17:25 -0700, Casey Marshall wrote:
> On Jun 29, 2006, at 4:33 PM, Matthew Wringe wrote:
>
> > On Thu, 2006-06-29 at 15:36 -0700, Casey Marshall wrote:
> >> On Jun 29, 2006, at 3:24 PM, Matthew Wringe wrote:
> >>
> >>> Hi,
> >>>
> >>> I have attached a very small patch that fixes PR28204 : PBEKeySpec
> >>> incorrectly deletes the originally passed password array
> >>> (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28204)
> >>>
> >>> Instead of taking a reference to the passed password, it now
> >>> creates a
> >>> copy of it.
> >>>
> >>
> >> This looks fine, except for this space here at the end:
> >>
> >>> + System.arraycopy(password, 0, this.password, 0,
> >>> password.length );
> >>
> >> And you can accomplish the same thing with `clone()'.
> >>
> >> The JavaDoc should also be updated to explain that a copy of the
> >> argument is made (the JDK documentation says this, and it is an
> >> important API detail).
> >
> > The attached patch now uses clone() instead of System.arraycopy and
> > the
> > javadoc has been updated to reflect that it only stores a copy.
> >
> > Out of curiosity, what is the real big difference between clone() and
> > arraycopy? and under what situation should one be used over another?
> >
>
> I think clone() can be faster for arrays, because it can combine the
> allocation and the copy into a single call. But really, I don't have
> any idea if it's actually faster on any VM (it seems like a JIT could
> inline both the `new' call and the array copy).
>
> The only other advantage is that it's much more concise.
Index: PBEKeySpec.java
===================================================================
RCS file: /sources/classpath/classpath/javax/crypto/spec/PBEKeySpec.java,v
retrieving revision 1.2
diff -u -r1.2 PBEKeySpec.java
--- PBEKeySpec.java 2 Jul 2005 20:32:45 -0000 1.2
+++ PBEKeySpec.java 5 Jul 2006 19:09:26 -0000
@@ -76,54 +76,166 @@
/** The salt. */
private byte[] salt;
+ /** The password state */
+ private boolean passwordValid = true;
+
// Constructors.
// ------------------------------------------------------------------------
/**
- * Create a new PBE key spec with just a password.
+ * Create a new PBE key spec with just a password.<p>
+ * <p>
+ * A copy of the password argument is stored instead of the argument
+ * itself. <p>
*
* @param password The password char array.
*/
public PBEKeySpec(char[] password)
{
- this(password, null, 0, 0);
+ if (password != null)
+ {
+ this.password = password.clone();
+ }
+ else
+ {
+ this.password = new char[0];
+ }
+
+ // load the default values for unspecified variables.
+ salt = null;
+ iterationCount = 0;
+ keyLength = 0;
+
+ // set the password state as being true.
+ passwordValid = true;
}
/**
- * Create a PBE key spec with a password, salt, and iteration count.
+ * Create a PBE key spec with a password, salt, and iteration count.<p>
+ * <p>
+ * A copy of the password and salt arguments are stored instead of the
+ * arguments themselves.<p>
*
* @param password The password char array.
* @param salt The salt bytes.
* @param iterationCount The iteration count.
+ *
+ * @throws NullPointerException If salt is null
+ * @throws IllegalArgumentException If salt is an empty array, or iterationCount is negative
*/
public PBEKeySpec(char[] password, byte[] salt, int iterationCount)
{
- this(password, salt, iterationCount, 0);
+ if (password != null)
+ {
+ this.password = password.clone();
+ }
+ else
+ {
+ this.password = new char[0];
+ }
+
+ if (salt == null)
+ {
+ throw new NullPointerException("Salt cannot be null");
+ }
+ else if (salt.length == 0)
+ {
+ throw new IllegalArgumentException("Salt cannot be an empty byte array");
+ }
+ else
+ {
+ this.salt = salt.clone();
+ }
+
+ if (iterationCount < 0)
+ {
+ throw new IllegalArgumentException("iterationCount must be positive");
+ }
+ else
+ {
+ this.iterationCount = iterationCount;
+ }
+
+ // load default values into unspecified variables.
+ keyLength = 0;
+
+ //set the state of the password as being valid.
+ passwordValid = true;
}
/**
* Create a PBE key spec with a password, salt, iteration count, and
- * key length.
+ * key length.<p>
+ * <p>
+ * A copy of the password and salt arguments are stored instead of
+ * the arguments themselves.<p>
*
* @param password The password char array.
* @param salt The salt bytes.
* @param iterationCount The iteration count.
* @param keyLength The generated key length.
+ *
+ * @throws NullPointerException If salt is null
+ * @throws IllegalArgumentException If salt is an empty array, if iterationCount or keyLength is negative
*/
public PBEKeySpec(char[] password, byte[] salt, int iterationCount,
int keyLength)
{
- this.password = password;
- this.salt = salt;
- this.iterationCount = iterationCount;
- this.keyLength = keyLength;
+
+ if (password != null)
+ {
+ this.password = password.clone();
+ }
+ else
+ {
+ this.password = new char[0];
+ }
+
+ if (salt == null)
+ {
+ throw new NullPointerException("Salt cannot be null");
+ }
+ else if (salt.length == 0)
+ {
+ throw new IllegalArgumentException("Salt cannot be an empty byte array");
+ }
+ else
+ {
+ this.salt = salt.clone();
+ }
+
+ if (iterationCount < 0)
+ {
+ throw new IllegalArgumentException("iterationCount must be positive");
+ }
+ else
+ {
+ this.iterationCount = iterationCount;
+ }
+
+ if (keyLength < 0)
+ {
+ throw new IllegalArgumentException("keyLength must be positive");
+ }
+ else
+ {
+ this.keyLength = keyLength;
+ }
+
+ //set the state of the password as being valid
+ passwordValid = true;
+
}
// Instance methods.
// ------------------------------------------------------------------------
/**
- * Clear the password array by filling it with null characters.
+ * Clear the password array by filling it with null characters.<p>
+ * <p>
+ * This clears the stored copy of the password, not the original
+ * char array used to create the password.<p>
+ *
*/
public final void clearPassword()
{
@@ -132,6 +244,9 @@
{
password[i] = '\u0000';
}
+
+ // since the password is cleared, it is no longer valid
+ passwordValid = false;
}
/**
@@ -155,17 +270,25 @@
}
/**
- * Get the password character array.
+ * Get the password character array copy.<p>
+ * <p>
+ * This returns the stored copy of the password, not the original
+ * char array used to create the password.<p>
*
* @return The password.
*/
public final char[] getPassword()
{
+ if (!passwordValid)
+ throw new IllegalStateException ("The clearPassword method has been called, the password is no longer valid.");
return password;
}
/**
- * Get the salt bytes.
+ * Get the salt bytes array copy.<p>
+ * <p>
+ * This returns the stored copy of the salt, not the original
+ * byte array used to create the salt.<p>
*
* @return The salt.
*/
/* TestOfPBEKeySpec.java -- Test for PBEKeySpec
Copyright (C) 2006 Free Software Foundation, Inc.
This file is part of Mauve.
Mauve is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2, or (at your option)
any later version.
Mauve is distributed in the hope that it will be useful, but
WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
General Public License for more details.
You should have received a copy of the GNU General Public License
along with Mauve; see the file COPYING. If not, write to the
Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
02110-1301 USA.
*/
// Tags: FIXME
package gnu.testlet.javax.crypto.spec;
import java.util.Arrays;
import javax.crypto.spec.PBEKeySpec;
import gnu.testlet.TestHarness;
import gnu.testlet.Testlet;
public class TestOfPBEKeySpec
implements Testlet
{
// default password used in the tests
char[] password = "HelloWorld".toCharArray();
// default salt used in the tests
byte[] salt = new byte[] { 0, 1, 1, 2, 3, 5, 8 };
// default iterationCount used in the tests
int iterationCount = 102;
// default keyLength used in the tests
int keyLength = 4;
/*
* (non-Javadoc)
*
* @see gnu.testlet.Testlet#test(gnu.testlet.TestHarness)
*/
public void test(TestHarness harness)
{
testConstructorP(harness);
testConstructorPSI(harness);
testConstructorPSIK(harness);
testPassword(harness);
testSalt(harness);
testIterationCount(harness);
testKeyLength(harness);
}
// test the constructor PBEKeySpec(char[] password)
private void testConstructorP(TestHarness harness)
{
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password);
harness.check(pbeKeySpec.getSalt() == null,
"salt should have a default value of null");
harness.check(pbeKeySpec.getIterationCount() == 0,
"iterationCount should have a default value of 0");
harness.check(pbeKeySpec.getKeyLength() == 0,
"keyLength should have a default value of 0");
}
catch (Exception x)
{
harness.debug(x);
harness.fail("PBEKeySpec with valid password : " + String.valueOf(x));
}
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(null);
char[] pbePassword = pbeKeySpec.getPassword();
harness.check(pbePassword.length == 0,
"a null password should produce an empty char array");
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
}
// test the constructor
// PBEKeySpec(char[] password, byte[] salt, int iterationCount)
private void testConstructorPSI(TestHarness harness)
{
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, salt, iterationCount);
harness.check(pbeKeySpec.getKeyLength() == 0,
"keyLength should have a default value of 0");
}
catch (Exception x)
{
harness.debug(x);
harness.fail("PBEKeySpec with valid password, salt and iterationCount :"
+ String.valueOf(x));
}
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(null, salt, iterationCount);
char[] pbePassword = pbeKeySpec.getPassword();
harness.check(pbePassword.length == 0,
"a null password should produce an empty char array");
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, null, iterationCount);
harness.fail("PBEKeySpec MUST throw a NullPointerException if the salt is null");
}
catch (NullPointerException npe)
{
// expected behaviour for a null salt
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
try
{
byte[] emptySalt = new byte[0];
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, emptySalt,
iterationCount);
harness.fail("PBEKeySpec MUST throw an IllegalArgumentException if the salt is an empty array");
}
catch (IllegalArgumentException iae)
{
// expected behavior for an empty salt
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, salt, - 1);
harness.fail("PBEKeySpec must throw an IllegalArgumentException of the iterationCount is negative");
}
catch (IllegalArgumentException iae)
{
// expected behavior for a negative iteration count
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
}
// test the constructor
// PBEKeySpec(char[] password, byte[] salt, int iterationCount, int keyLength)
private void testConstructorPSIK(TestHarness harness)
{
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, salt, iterationCount,
keyLength);
}
catch (Exception x)
{
harness.debug(x);
harness.fail("PBEKeySpec with valid password, salt, iterationCount and keyLength :"
+ String.valueOf(x));
}
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(null, salt, iterationCount,
keyLength);
char[] pbePassword = pbeKeySpec.getPassword();
harness.check(pbePassword.length == 0,
"a null password should produce an empty char array");
}
catch (Exception x)
{
harness.debug(x);
harness.fail("PBEKeySpec(password, salt, iterationcount, keyLength) with a null password : "
+ String.valueOf(x));
}
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, null, iterationCount,
keyLength);
harness.fail("PBEKeySpec MUST throw a NullPointerException if the salt is null");
}
catch (NullPointerException npe)
{
// expected behaviour for a null salt
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
try
{
byte[] emptySalt = new byte[0];
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, emptySalt,
iterationCount, keyLength);
harness.fail("PBEKeySpec MUST throw an IllegalArgumentException if the salt is an empty array");
}
catch (IllegalArgumentException iae)
{
// expected behavior for an empty salt
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, salt, - 1, keyLength);
harness.fail("PBEKeySpec must throw an IllegalArgumentException if the iterationCount is negative");
}
catch (IllegalArgumentException iae)
{
// expected behavior for a negative iteration count
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, salt, iterationCount,
- 1);
harness.fail("PBEKeySpec must throw an IllegalArgumentException if the keyLength is negative");
}
catch (IllegalArgumentException iae)
{
// expected behavior for a negative keyLength
}
catch (Exception x)
{
harness.debug(x);
harness.fail(String.valueOf(x));
}
}
// test that PBEKeySpec is indeed storing a copy of the password and not the
// password itself
private void testPassword(TestHarness harness)
{
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password);
// check to make sure PBEKeySpec password copy matches
harness.check(Arrays.equals(pbeKeySpec.getPassword(), password),
"The PBEKeySpec copy of the password MUST equal the actual password.");
// this should clear just the copy of the password, not the password it
// self
pbeKeySpec.clearPassword();
harness.check(
Arrays.equals(password, "HelloWorld".toCharArray()),
"clearPassword() cleared the actual password. PBEKeySpec should only store a COPY of the password");
try {
pbeKeySpec.getPassword();
harness.fail ("Calling getPassword method after calling clearPassword MUST throw an illegalStateException");
} catch (IllegalStateException ise){
// expected behaviour
}
}
catch (Exception x)
{
harness.debug(x);
harness.fail("testPassword failed : " + String.valueOf(x));
}
}
// test that PBEKeySpec is indeed storing a copy of the salt and not the salt
// itself
private void testSalt(TestHarness harness)
{
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, salt, iterationCount);
// check to make sure the PBEKeySpec password copy matches
harness.check(Arrays.equals(pbeKeySpec.getSalt(), salt),
"The PBEKeySpec copy of the salt MUST equal the actual salt.");
// check that PBEKeySpec is indeed storing only a copy of the salt
byte[] saltCopy = pbeKeySpec.getSalt();
for (int i = 0; i < saltCopy.length; i++)
{
saltCopy[i] = (byte) i;
}
byte[] originalSalt = new byte[] { 0, 1, 1, 2, 3, 5, 8 };
harness.check(
Arrays.equals(salt, originalSalt),
"changing the stored salt changed the actual salt. PBEKeySpec should only store a COPY of the salt");
}
catch (Exception x)
{
harness.debug(x);
harness.fail("testSalt failed : " + String.valueOf(x));
}
}
// quick tests to verify PBEKeySpec is storing the proper IterationCount
private void testIterationCount(TestHarness harness)
{
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, salt, iterationCount);
harness.check(pbeKeySpec.getIterationCount() == iterationCount,
"PBEKeySpec.getIterationCount() does not match iterationCount");
}
catch (Exception x)
{
harness.debug(x);
harness.fail("testIterationCount failed : " + String.valueOf(x));
}
}
// quick tests to verify PBEKeySpec is storing the proper KeyLength
private void testKeyLength(TestHarness harness)
{
try
{
PBEKeySpec pbeKeySpec = new PBEKeySpec(password, salt, iterationCount,
keyLength);
harness.check(pbeKeySpec.getKeyLength() == keyLength,
"PBEKeySpec.getKeyLength() does not match keyLength");
}
catch (Exception x)
{
harness.debug(x);
harness.fail("testIterationCount failed : " + String.valueOf(x));
}
}
}
