SecureString Thread-locale cache

[shathor]

Looking at the SecureString implementation, wouldn't it be better to put a synchronized block on the chars-array around the Arrays.fill call?

E.g.

public void clear(){
    synchronized(chars) {
        Arrays.fill(chars, '0');
    }
}

=> This would give assurance, that the JVM doesn't optimize anything around fill and prevent thread-local caching (as per this thread)

Actually you could make this class thread-safe while at it, with read-locks for all other methods and write-lock for the clear().

Additionally it's not clear, that the class creates a copy of the input char-array. It's likely users forget to clear their input "manually" after creating an instance.

If you want I can make a PR.

[c-a-m]

I'm torn. While I do want bullet proof code, the intent of this class is that it be mostly immutable - except for when we are done using it. When we are done analyzing it, passwords should be cleared out and no longer in memory. At that point passfault should be done using an instance of this object - so thread safety shouldn't be a concern. So I'd hate to slow down access to SecureString by making it thread-safe.

On copying the array, that's a great point. In fact I bet I do that same thing in the servlet code. Yep, here it is: https://github.com/OWASP/passfault/blob/master/jsonService/src/main/java/org/owasp/passfault/web/PassfaultServlet.java Nice find!

[shathor]

If you use read-write locks, the access penalty shouldn't be noticable. Maybe you find a better way to prevent thread-local caching and optimization, which is the actual goal (wolatile?).

Btw. I was also told, that filling the array with '0' character is not exactly clearing the array. Though technically ok I guess (unless someone uses 0's as a password ;))
Better clear the arrays with Character.MIN_VALUE or '\0' or (char) 0?
Using System.arrayCopy should be even faster.

[Marcono1234]

Only synchronizing clear() is likely not enough. There would be no guarantee that the other methods see the updated value. Using volatile will not work either because it only makes guarantees about the array reference, but not the elements of the array, see for example https://jeremymanson.blogspot.com/2009/06/volatile-arrays-in-java.html.

---

Using System.arrayCopy should be even faster.

But that would require allocating a new array, or you have an array constant containing only \0 for that (and repeat copying if that array is shorter than the password).

---

Side note: There is also the javax.security.auth.Destroyable interface, though implementing it is probably now not worth it anymore.