8268775: Password is being converted to String in AccessibleJPasswordField#127
Conversation
…Field Removed unneeded String internal variables in a number of methods.
👋 Welcome back kizune! A progress list of the required criteria for merging this PR into |
@azuev-java The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
openjdkbot added swing Webrevs
|
There's code but not a single word of description of the fix. |
| if (part == AccessibleText.CHARACTER) { | ||
| str = super.getAtIndex(part, index); | ||
| return getEchoString(super.getAtIndex(part, index)); |
There was a problem hiding this comment.
I don't see how removing the local variable changes anything. Explanation ??
There was a problem hiding this comment.
Here it is just a slight code cleanup. We do not need additional variable for passing value from one method to another. It serves no other purpose at all. It was used before on the second leg of the if but the usage was removed so it became useless.
There was a problem hiding this comment.
If this is about security, I don’t see how it might help. There is a chance that the heap dump might capture the content of the local variable. If you submit your heap dump to someone, whom you do not trust, I have bad news for you.
The probability of the heap dump to capture a local variable is more than zero. True, but less than probable.
I don’t know if calling same methods in a single line makes this control more secure (if we take the situation that the heap dump pauses an execution of the thread exactly at our „moment of time“). I am not a member of the project JDK, but I doubt that this PR solves something.
To me, an additional local variable adds better supportability (debugging) to this code. Otherwise everything should be put into a single fat method.
There was a problem hiding this comment.
I don’t know if calling same methods in a single line makes this control more secure
As i said above this exact change is not about making code more secure - it is just to eliminate additional variable that has no purpose after the second half of the method is changed. I would say that it would add to the supportability if we do anything with this information - but we don't.
| @@ -544,8 +543,7 @@ public String getAtIndex(int part, int index) { | |||
| */ | |||
| public String getAfterIndex(int part, int index) { | |||
| if (part == AccessibleText.CHARACTER) { | |||
| String str = super.getAfterIndex(part, index); | |||
| return getEchoString(str); | |||
| return getEchoString(super.getAfterIndex(part, index)); | |||
There was a problem hiding this comment.
I don't see how removing the local variable changes anything. Explanation ??
There was a problem hiding this comment.
Here it is just a slight code cleanup. We do not need additional variable for passing value from one method to another. It serves no other purpose at all.
There was a problem hiding this comment.
Let me ask it this way.
Does super.getAfterIndex(part, index) return a String with any of the password in clear text ?
It seems to me like it might.
There was a problem hiding this comment.
For CHARACTER it will return String with a single character in the corresponding position. There is a possibility that someone will iterate the entirety of the password text and get all the characters in the password as a separate strings but digging it from the memory dump is much more difficult than the singular string with the whole password.
For anything but character we do not use this method - we get password as an array of characters and - after the fix - immediately overriding them with the same number of echo characters.
The problem here is that if someone uses the accessibility methods on JPasswordField it will lead to unencrypted password being stored in the local String variable and that in turn can lead to it being recorded in, say, crash tump file where it can be found amongst the string literals. This is highly improbable scenario but it can be done so we better to not do it. And there are only two places where it happens - because in other methods we only serving the AccessibleText.CHARACTER retrieval which means that we are getting one password character at a time in a separate String variable which is Ok. These two places are at methods getAtIndex and getTextSequenceAt where we requesting something other than AccessibleText.CHARACTER. There we were converting password from the array of chars to the String only to pass this string to the method that generates string of echo characters of the same length. Instead i am doing conversion myself filling in the returned array with echo characters and returning constructed string. The rest of the changes is just a slight code cleanup - getting rid of the local variable that is used only to store some value before passing it to another method. |
| String text = new String(password); | ||
| return new AccessibleTextSequence(0, password.length - 1, | ||
| getEchoString(text)); | ||
| return new AccessibleTextSequence(0, password.length - 1, text); |
There was a problem hiding this comment.
So the accessible text is just the right number of "echo" chars.
And you are still calling getPassword() just so you can find out the length.
Then it is over-written. There's a really tiny window after getPassword() and before Arrays.fill() when the clear password is still there.
The number of "char"s isn't the same as the number of "characters" if there's a non-BMP code point in there .. perhaps these are not allowed by this class .. but it makes me wonder how much if having the exact number of echo chars as the actual password is important. I wonder how many text-to-speech readers can say "bullet" for a unicode bullet character ?
If it weren't for all of this (the class and the getPassword() method being non-final I'd suggest you look into a way to pull just the length rather than the actual chars.
There was a problem hiding this comment.
So the accessible text is just the right number of "echo" chars.
Yes.
And you are still calling getPassword() just so you can find out the length.
Then it is over-written. There's a really tiny window after getPassword() and before Arrays.fill() when the clear password is still there.
Yes. But not as a string - and the window of opportunity to get the characters before they are overwritten by the echo characters is minimal.
The number of "char"s isn't the same as the number of "characters" if there's a non-BMP code point in there ..
Since in order to enter non-BPM characters you have to have an input methods helper which should be disabled on password fields for obvious reason - it would pretty much disclose the typed password in the IM helper window - the only way to enter such symbols would b copy/paste and in this case i do not expect it to be edited within password field.
perhaps these are not allowed by this class .. but it makes me wonder how much if having the exact number of echo chars as the actual password is important. I wonder how many text-to-speech readers can say "bullet" for a unicode bullet character ?
Well, accessibility is not only about text to speech - it is also about easier navigation so having exact number of the bullets is preferable. There are limitations - like some non-BPM text can be pasted into the password field and then navigating within it might be broken but since there will be no IM engaged fixing it would be equally problematic.
If it weren't for all of this (the class and the getPassword() method being non-final I'd suggest you look into a way to pull just the length rather than the actual chars.
That would be preferable but under the current circumstances i would say that my fix makes things better without adding incompatible changes.
There was a problem hiding this comment.
Why not use the "getDocument().getLength()"?
There was a problem hiding this comment.
Why not use the "getDocument().getLength()"?
We can surely do that. Fixed.
| @@ -544,8 +543,7 @@ public String getAtIndex(int part, int index) { | |||
| */ | |||
| public String getAfterIndex(int part, int index) { | |||
| if (part == AccessibleText.CHARACTER) { | |||
| String str = super.getAfterIndex(part, index); | |||
| return getEchoString(str); | |||
| return getEchoString(super.getAfterIndex(part, index)); | |||
There was a problem hiding this comment.
Let me ask it this way.
Does super.getAfterIndex(part, index) return a String with any of the password in clear text ?
It seems to me like it might.
@mrserb please review |
…ay of chars and counting its length
@azuev-java This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 73 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit 97e0e9e.
Your commit was automatically rebased without conflicts. |
openjdkbot added integrated @azuev-java Pushed as commit 97e0e9e. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk17 pull/127/head:pull/127$ git checkout pull/127Update a local copy of the PR:
$ git checkout pull/127$ git pull https://git.openjdk.java.net/jdk17 pull/127/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 127View PR using the GUI difftool:
$ git pr show -t 127Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk17/pull/127.diff