There has been some more discussion about this issue on bugzilla
(http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28204). Raif made some good
suggestions and these have been implemented in my latest patch.
The major changes this patch does are listed below:
1) password and salt store a copy of the argument used to create them.
2) when get methods are called for password and salt, a copy of the
internal salt and password are returned.
3) methods now check if the value of their parameters are valid.
4) getPassword now checks if the current state of the password is valid
If someone could please review this patch it would be greatly
appreciated.
Thanks,
Matt Wringe
On Wed, 2006-07-05 at 15:20 -0400, Matthew Wringe wrote:
> 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 6 Jul 2006 15:39:08 -0000
@@ -76,54 +76,86 @@
/** 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);
+ setPassword(password);
+
+ // load the default values for unspecified variables.
+ salt = null;
+ iterationCount = 0;
+ keyLength = 0;
}
/**
- * 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);
+ setPassword(password);
+ setSalt(salt);
+ setIterationCount(iterationCount);
+
+ // load default values into unspecified variables.
+ keyLength = 0;
}
/**
* 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;
+ setPassword(password);
+ setSalt(salt);
+ setIterationCount(iterationCount);
+ setKeyLength(keyLength);
}
// 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 +164,9 @@
{
password[i] = '\u0000';
}
+
+ // since the password is cleared, it is no longer valid
+ passwordValid = false;
}
/**
@@ -155,22 +190,116 @@
}
/**
- * Get the password character array.
+ * Get the password character array copy.<p>
+ * <p>
+ * This returns a copy of the password, not the password itself.<p>
*
* @return The password.
+ * @throws IllegalStateException If clearPassword has already been called.
*/
public final char[] getPassword()
{
- return password;
+ if (!passwordValid)
+ throw new IllegalStateException ("The clearPassword method has been called, the password is no longer valid.");
+ return password.clone();
}
/**
- * Get the salt bytes.
+ * Get the salt bytes array copy.<p>
+ * <p>
+ * This returns a copy of the salt, not the salt itself.<p>
*
* @return The salt.
*/
public final byte[] getSalt()
{
- return salt;
+ if (salt != null)
+ {
+ return salt.clone();
+ }
+ else
+ {
+ return null;
+ }
+ }
+
+ /**
+ * Set the password char array.<p>
+ * <p>
+ * A copy of the password argument is stored instead of the argument itself.<p>
+ *
+ * @param password The password to be set
+ */
+ private void setPassword (char[] password){
+ if (password != null)
+ {
+ this.password = password.clone();
+ }
+ else
+ {
+ this.password = new char[0];
+ }
+
+ // set the password state as being valid.
+ passwordValid = true;
+ }
+
+ /**
+ * Set the salt byte array.<p>
+ * <p>
+ * A copy of the salt arguments is stored instead of the argument itself.<p>
+ *
+ * @param salt The salt to be set.
+ * @throws NullPointerException If the salt is null.
+ * @throws IllegalArgumentException If the salt is an empty array.
+ */
+ private void setSalt (byte[] salt){
+ 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();
+ }
+ }
+
+ /**
+ * Set the iterationCount.
+ *
+ * @param iterationCount The iteration count to be set.
+ * @throws IllegalArgumentException If the iterationCount is negative.
+ */
+ private void setIterationCount (int iterationCount){
+ if (iterationCount < 0)
+ {
+ throw new IllegalArgumentException("iterationCount must be positive");
+ }
+ else
+ {
+ this.iterationCount = iterationCount;
+ }
+ }
+
+ /**
+ * Set the keyLength.
+ *
+ * @param keyLength The keyLength to be set.
+ * @throws IllegalArguementException if the keyLength is negative.
+ */
+ private void setKeyLength (int keyLength){
+ if (keyLength < 0)
+ {
+ throw new IllegalArgumentException("keyLength must be positive");
+ }
+ else
+ {
+ this.keyLength = keyLength;
+ }
}
+
}
