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'.
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.
(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?
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.
(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!
An update patch can be found here: http://developer.classpath.org/pipermail/classpath-patches/attachments/20060629/37ada768/Crypto-PBEKeySpec.bin
(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?
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?
(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.
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?
... > 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?
Created attachment 11849 [details] Fix all outstanding issues of this PR
* 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!