Community
Participate
Working Groups
Currently, all our login dialogs have Text widget in it for password field. Per Java security best practices, passwords should always be stored as char[] rather than String, so we use getTextChars() API to get password as char array and then zero it out for safety: char[] password = pwText.getTextChars(); //..login with password Arrays.fill(password, '\0'); //zero out the password. But we found we can still see password array in memory dump even we zero out the password array get from getTextChars(). This is because getTextChar() itself creates a copy of input text, and it will and there is no way to clear it explictly. So the password will stay in memory until it is collected by GC. Can we improve the implementation of getTextChars to further reduce the chances of a password being compromised? On Windows: public char[] getTextChars () { checkWidget (); int length = OS.GetWindowTextLength (handle); if (length == 0) return new char[0]; TCHAR buffer = new TCHAR (getCodePage (), length + 1); OS.GetWindowText (handle, buffer, length + 1); if (segments != null) buffer = deprocessText (buffer, 0, -1, false); char [] chars = new char [length]; System.arraycopy (buffer.chars, 0, chars, 0, length); + Arrays.fill(buffer.chars, '\0'); return chars; }
Created attachment 231299 [details] patch The patch clears the text and handles Unicode and non-Unicode text.
The patch looks good, but in talking with SSQ we realized that yesterday's suggestion to handle the non-unicode case was not appropriate since it is currently not being considered. So I've cut a few of the lines and pushed the change to ensure that it is in for today's build. Closing report as Fixed > 20130522, commit: http://git.eclipse.org/c/platform/eclipse.platform.swt.git/commit/?id=a1e0ef96d65e6ef2f4c5b16924d15cef55932534 .
I assume we also assume that the caller of Text#getTextChars() doesn't add a SegmentListener? Like we already assume that the caller doesn't use a VerifyListener (discussed around bug 297412 comment 6, but never documented, AFAICS). Note that Text#computeSize(..) is another innocent-looking method that creates a TCHAR buffer, fills it via OS.GetWindowText(..), and then leaves it up to the GC.
Can we also add fix for Linux/Mac?
Lakshmi, please open a separate bug for the remaining issues (comment#3 and comment#4). I believe it is too late to add this for 4.3. The only thing we could consider adding in 4.3 is documentation warning that there is no way to protected the password if verify/segments listeners are installed. I had a quick look on Mac and GTK. Mac getTextChars() does not have any local memory to clean. GTK getTextChars() has a byte array to clean. We should probably add two helper methods as follow and call where appropriate. Anywhere there is local temporary buffers with the contents of the field. void clear(byte[] buffer) { if ((style & PASSWORD) != 0) { fill with zeros } }
(In reply to Silenio Quarti from comment #5) Forgot to update here before > Lakshmi, please open a separate bug for the remaining issues (comment#3 and > comment#4). I believe it is too late to add this for 4.3. The only thing we > could consider adding in 4.3 is documentation warning that there is no way > to protected the password if verify/segments listeners are installed. Done. (Bug 408957) >I had a quick look on Mac and GTK.GTK getTextChars() has a byte array to clean. Opened Bug 408953
