288338 - Intermittent "ERROR: Password was garbage collected before it was cleared." messages in "test digesting"

Status: RESOLVED FIXED

(JSS Graveyard :: Library, defect)

Description

[Wan-Teh Chang]

I'm working on the JSS 3.6 release.

I built it on Red Hat Enterprise Linux 3 against
JDK 1.4.2.

When I ran all.pl on the debug build, I sometimes
get the following error message during "test digesting":

============= test digesting
39 bytes to be digested
main: jss library loaded
Sun and Mozilla give same SHA-1 hash
Sun and Mozilla give same MD5 hash
ERROR: Password was garbage collected before it was cleared.
digest output size is 20

I found that this error message comes from Password.java:
http://lxr.mozilla.org/security/source/security/jss/org/mozilla/jss/util/Password.java#191

191     /**
192      * The finalizer clears the sensitive information before releasing
193      * it to the garbage collector, but it should have been cleared manually
194      * before this point anyway.
195      */
196     protected void finalize() throws Throwable {
197         if(Debug.DEBUG && !cleared) {
198             System.err.println("ERROR: Password was garbage collected before"+
199                 " it was cleared.");
200         }
201         clear();
202     }

I don't know what this error message means.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed fix 1

I found that the Password object that is not cleared
before being garbage collected is the temporary Password
object passed to the constructor of PBEKeyGenParams.

There are two ways to fix this bug because PBEKeyGenParams
can take the password as either a Password object or a
char array.

(PBEKeyGenParams contains a Password object.  That
embedded Password object also needs to be cleared
before being garbage collected.  So I think we need
to call pbe.clear() and my patches indicate where
we might call pbe.clear(). But for some reason, without
calling pbe.clear(), the embedded Password object
is cleared before being garbage collected.)

This is the first fix.	We still pass a Password object
to the PBEKeyGenParams constructor.  We use a local variable
to reference this Password object so we can invoke its
clear method.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed fix 2

In the second fix, we pass the password as a char array
to the PBEKeyGenParams constructor.

I like the first fix better because it demonstrates the
need to call the clear method on a Password object,
even though it creates a Password object unnecessarily.

Comment 3

[Wan-Teh Chang]

I found out why the embedded Password object in
PBEKeyGenParams doesn't generate the error message.
This is because PBEKeyGenParams.finalize calls
the clear method on the embedded Password object.

We should still call pbe.clear() as soon as pbe
is no longer needed, but it's not obvious to me
when that is.  I think it is after we have generated
symkey:
         SymmetricKey symkey = kg.generate();
+        pbe.clear();

Comment 4

[Sandeep Konchady]

Hi Wan-Teh,

  I have some time at hand and was going over open JSS bugs hoping to scrub some
of them.  I am trying to reproduce this bug under JSS4 on RHEL 3 using both JDK
1.4.2_08 and 1.5.0_04.  I am unable to reproduce it using the original source. 
Do you think it is still a good idea to apply your changes.  If so should I
apply these changes to JSS 4.1?

uname -a
Linux nssamdrhel3 2.4.21-19.ELsmp #1 SMP Thu Aug 12 23:22:47 EDT 2004 x86_64
x86_64 x86_64 GNU/Linux

java -version
java version "1.4.2_08"
Java(TM) 2 Runtime Environment, Standard Edition (build 1.4.2_08-b03)
Java HotSpot(TM) Client VM (build 1.4.2_08-b03, mixed mode)

JSSTEST_CASE 5 (Digesting): PASS
============= test signing
main: jss library loaded
Available tokens:
 token : Internal Crypto Services Token
 token : Internal Key Storage Token
 token : Builtin Object Token
***FilePasswordCallback returns netscape
Created a signing context
initialized the signing operation
updated signature with data
Successfully signed!
initialized verification
updated verification with data
Signature Verified Successfully!
SigTest passed.

Sandeep

Comment 5

[glen beasley]

It is still a good idea to apply these changes. 

I believe the best approach is Option 1 with the clear after kg.generate:

         SymmetricKey symkey = kg.generate();
+        pbe.clear();

This error is very intermittent, and I have seen it on 
occasion. 

Comment 6

[Sandeep Konchady]

Attachment: Based on suggestion #1 clearing PBEKeyGenParams after SymmetricKey generation

Added pbe.clear() after SymmetricKey.generate() call.  This should clear pbe
and make it available for GC.

Comment 7

[Sandeep Konchady]

Comment on attachment 194486 [details] [diff] [review]
Based on suggestion #1 clearing PBEKeyGenParams after SymmetricKey generation

Added Password.clear() and PBEKeyGenParams.clear() so that the objects are
released when GC tries to collect them.

Checking in DigestTest.java;
/cvsroot/mozilla/security/jss/org/mozilla/jss/tests/DigestTest.java,v  <-- 
DigestTest.java
new revision: 1.9; previous revision: 1.8
done

Comment 8

[Sandeep Konchady]

Reopening bug until we are sure that this has eliminated the ERROR issue.  Also
need to verify if this only reduces the chance of being garbage collected prior
to clearing or does it for sure release the handle and GC can do its work.

Comment 9

[Wan-Teh Chang]

The purpose of the patch is to clear the password buffer
immediately after use.  The purpose is not to release the
handle to the object so GC can do its work.

Comment 10

[Sandeep Konchady]

Sorry Wan-Teh.  This bug was reopened because of some confusion about the intent
of the fix.  This has been addressed and is very clear from the bug.  I am
closing the bug as FIXED now.