Bug 28204

Summary: PBEKeySpec incorrectly deletes the originally passed password array
Product: classpath Reporter: Matt Wringe <mwringe>
Component: cryptoAssignee: Casey Marshall <csm>
Status: RESOLVED FIXED    
Severity: normal CC: bug-classpath
Priority: P3    
Version: 0.92   
Target Milestone: ---   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2006-06-29 22:30:13
Attachments: Patch to copy the passed array instead of referencing the original array
Updated mauve testlet
Fix all outstanding issues of this PR

Description Matt Wringe 2006-06-29 21:10:33 UTC
Within javax.crypto.spec.PBEKeySpec, the password attribute references the passed password array. As such, when the clear password method is called in PBEKeySpec, the original password array is also cleared.

For Example:

char[] password = "foobar".toCharArray();
PBEKeySpec keySpec = new PBEKeySpec(password); 
keySpec.clearPassword();
System.out.println (password);

The char array printed will be empty, when it should still contain 'foobar'.
Comment 1 Matt Wringe 2006-06-29 21:16:11 UTC
Created attachment 11781 [details]
Patch to copy the passed array instead of referencing the original array

This patch makes the PBEKeySpec's password a copy of the passed password array instead of referencing it. Now when the PBEKeySpec's clearPassword method is invoked, only its stored copy will be cleared.
Comment 2 Matt Wringe 2006-06-29 22:07:51 UTC
(In reply to comment #0)

> For Example:
> 
> char[] password = "foobar".toCharArray();
> PBEKeySpec keySpec = new PBEKeySpec(password); 
> keySpec.clearPassword();
> System.out.println (password);
> 
> The char array printed will be empty, when it should still contain 'foobar'.

I would like to add a mauve test for this, but I am unclear as to where it should be added. Any ideas?

Comment 3 Casey Marshall 2006-06-29 22:30:13 UTC
Confirmed.

The patch looks OK, except it looks like there's an extra space before the closing paren. Also, you can just `clone()' the char array.
Comment 4 Raif S. Naffah 2006-06-30 09:53:54 UTC
(In reply to comment #2)
> (In reply to comment #0)
> 
> > For Example:
> > 
> > char[] password = "foobar".toCharArray();
> > PBEKeySpec keySpec = new PBEKeySpec(password); 
> > keySpec.clearPassword();
> > System.out.println (password);
> > 
> > The char array printed will be empty, when it should still contain 'foobar'.
> 
> I would like to add a mauve test for this, but I am unclear as to where it
> should be added. Any ideas?
> 

the convention is to:

a. name it TestOfPR28204,
b. place it in gnu.testlet.javax.crypto.spec --i.e. the package of the offending class under the gnu.testlet hierarchy.

if you need any help writing the testlet feel free to contact me off the list.

thanks for doing this btw!



Comment 6 Raif S. Naffah 2006-06-30 22:54:47 UTC
(In reply to comment #5)
> An update patch can be found here:
> 
> http://developer.classpath.org/pipermail/classpath-patches/attachments/20060629/37ada768/Crypto-PBEKeySpec.bin

looking at the javadoc of the RI, i feel that the [salt] suffers from the same defect as the [password].  i'm sure one can show that in a testlet and include in the fix cloning the salt's byte array if it's not null.

thoughts?

Comment 7 Matt Wringe 2006-07-05 19:29:10 UTC
After looking into the issue more, the salt also must be cloned.
There are also a lot of other issues with this class that need to be addressed. There are no checks for invalid parameters, and it allows for some invalid behavior.

An upated patch can be found here: http://developer.classpath.org/pipermail/classpath-patches/attachments/20060705/1f52b6af/Crypto-PBEKeySpec.bin

This patch should bring PBEKeySpec upto spec. The major changes in this patch include:
1) password and salt are now correctly stored as copies. The javadoc has been updated to reflect this
2) method arguments are now checked for correctness. Incorrect arguments include: null salts, empty salts, negative iterationCounts, and negative keyLengths
3) an illegalStateException is now properly thrown when calling getPassword after calling clearPassword()

I also have a mauve testlet that checks for the proper behaviour of PBEKeySpec, it can be found here: http://developer.classpath.org/pipermail/classpath-patches/attachments/20060705/1f52b6af/TestOfPBEKeySpec.java
It should go under gnu/testlet/javax/crypto/spec (this directory does not yet exist in cvs)

I do not have mauve or classpath commit access, so if these are deemed acceptable, could someone please commit them for me on my behalf.

Comments?
Comment 8 Raif S. Naffah 2006-07-05 21:37:54 UTC
(In reply to comment #7)
> After looking into the issue more...
> An upated patch can be found here:
> http://developer.classpath.org/pipermail/classpath-patches/attachments/20060705/1f52b6af/Crypto-PBEKeySpec.bin
> ...
> I also have a mauve testlet ... can be found here:
> http://developer.classpath.org/pipermail/classpath-patches/attachments/20060705/1f52b6af/TestOfPBEKeySpec.java
> ...
> Comments?

on the PBEKeySpec:

* i don't think cloning the password and salt at construction is enough.  the code for getPassword() and getSalt() should still return a clone of the arrays.  if this is the case, my patch/diff comparator did not show it.  the cloning is necessary to guard againt either of these entities being changed between consecutive invocations of those methods.

* minor nit: you probably can reduce code duplication by confining all the correctness checking code in one method; e.g. checkParams() or the like, which can be called in each constructor.


on the Test:

* the "Tags" line, near the beginning of the file, for the crypto classes is usually:

// Tags: GNU-CRYPTO JDK1.4


* because the test's methods are run sequentally, it's difficult to see if and how previous test methods may have affected the class fields; e.g. salt, etc.  i suggest you include the field initialisation in each methods.

* usually, we use harness.check(true, "message") instead of comments in the code to reflect an expected behaviour; i.e. catch NPE in testConstructorPSI(), etc.

Comment 9 Matt Wringe 2006-07-06 17:56:24 UTC
Created attachment 11845 [details]
Updated mauve testlet

Updated mauve testlet to take into consideration suggestions for Raif (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=28204#c8)

The tag is now properly set.
Variables are now initalized in every method.
Harness.check(true, msg) is now used instead of comments in the code

Any other changes needed?
Comment 10 Matt Wringe 2006-07-06 18:00:15 UTC
...
> on the PBEKeySpec:
> 
> * i don't think cloning the password and salt at construction is enough.  the
> code for getPassword() and getSalt() should still return a clone of the arrays.
>  if this is the case, my patch/diff comparator did not show it.  the cloning is
> necessary to guard againt either of these entities being changed between
> consecutive invocations of those methods.
> 
> * minor nit: you probably can reduce code duplication by confining all the
> correctness checking code in one method; e.g. checkParams() or the like, which
> can be called in each constructor.
> 
...

An updated patch can be found here: http://developer.classpath.org/pipermail/classpath-patches/attachments/20060706/22dc056c/Crypto-PBEKeySpec.bin

A copy of salt and password are now returned when its get method is called. It now follows the proper behaviour.

The code has also been cleaned up to reduce code duplication. Instead of checking for argument validity in each constructor, the constructors now call a private set method that does the checking and throws any errors if necessary.

Any other suggestions?

Comment 11 Raif S. Naffah 2006-07-07 12:34:24 UTC
Created attachment 11849 [details]
Fix all outstanding issues of this PR
Comment 12 Raif S. Naffah 2006-07-07 12:39:32 UTC
* patch #11849 fixes all reported issues with this class.
* TestOfPBEKeySpec was added to Mauve (in gnu/testlet/javax/crypto/spec).


thanks Matt for the fix and the Mauve test!