Skip to content
This repository was archived by the owner on Sep 2, 2022. It is now read-only.

8268775: Password is being converted to String in AccessibleJPasswordField #127

Closed
azuev-java wants to merge 2 commits into from
Closed

8268775: Password is being converted to String in AccessibleJPasswordField #127

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
372202b
8268775: Password is being converted to String in AccessibleJPassword…
azuev-java Jun 23, 2021
6c57265
Use getDocumnt().getLength() instead of requesting password as an arr…
azuev-java Jun 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions src/java.desktop/share/classes/javax/swing/JPasswordField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,20 +510,19 @@ private String getEchoString(String str) {
* @since 1.6
*/
public String getAtIndex(int part, int index) {
String str = null;
if (part == AccessibleText.CHARACTER) {
str = super.getAtIndex(part, index);
return getEchoString(super.getAtIndex(part, index));
Copy link
Contributor

@prrace prrace Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how removing the local variable changes anything. Explanation ??

Copy link
Member Author

@azuev-java azuev-java Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@Vest Vest Jun 23, 2021 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@azuev-java azuev-java Jun 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
// Treat the text displayed in the JPasswordField
// as one word and sentence.
char[] password = getPassword();
if (password == null ||
index < 0 || index >= password.length) {
int length = getDocument().getLength();
if (index < 0 || index >= length) {
return null;
}
str = new String(password);
char[] password = new char[length];
Arrays.fill(password, getEchoChar());
return new String(password);
}
return getEchoString(str);
}

/**
Expand All @@ -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));
Copy link
Contributor

@prrace prrace Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how removing the local variable changes anything. Explanation ??

Copy link
Member Author

@azuev-java azuev-java Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@prrace prrace Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@azuev-java azuev-java Jun 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
// There is no word or sentence after the text
// displayed in the JPasswordField.
Expand All @@ -571,8 +569,7 @@ public String getAfterIndex(int part, int index) {
*/
public String getBeforeIndex(int part, int index) {
if (part == AccessibleText.CHARACTER) {
String str = super.getBeforeIndex(part, index);
return getEchoString(str);
return getEchoString(super.getBeforeIndex(part, index));
} else {
// There is no word or sentence before the text
// displayed in the JPasswordField.
Expand Down Expand Up @@ -627,14 +624,14 @@ public AccessibleTextSequence getTextSequenceAt(int part, int index) {
} else {
// Treat the text displayed in the JPasswordField
// as one word, sentence, line and attribute run
char[] password = getPassword();
if (password == null ||
index < 0 || index >= password.length) {
int length = getDocument().getLength();
if (index < 0 || index >= length) {
return null;
}
char[] password = new char[length];
Arrays.fill(password, getEchoChar());
String text = new String(password);
return new AccessibleTextSequence(0, password.length - 1,
getEchoString(text));
return new AccessibleTextSequence(0, password.length - 1, text);
Copy link
Contributor

@prrace prrace Jun 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@azuev-java azuev-java Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mrserb mrserb Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the "getDocument().getLength()"?

Copy link
Member Author

@azuev-java azuev-java Jun 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the "getDocument().getLength()"?

We can surely do that. Fixed.

}
}

Expand Down