Triplea: Swing JPasswordField `#getPassword` uses `String` internally

[DanVanAtta]

Unless I've missed something, JPasswordField uses String internally. If our goal of scrubbing char[] array is to remove all memory references to a password value, we cannot achieve with that implementation.

JPasswordField is used in LobbyGamePanel.java

JPasswordField-source:

 252:   /**
 253:    * Returns the text contained in this TextComponent. If the underlying 
 254:    * document is null, will give a NullPointerException. 
 255:    * For stronger security, it is recommended that the returned character 
 256:    * array be cleared after use by setting each character to zero.
 257:    * 
 258:    * @return char[]
 259:    */
 260:   public char[] getPassword()
 261:   {
 262:     return getText().toCharArray();
 263:   }
 264: 

 212:   /**
 213:    * Returns the text contained in this TextComponent. If the 
 214:    * underlying document is null, will give a NullPointerException.
 215:    * 
 216:    * @return String
 217:    * 
 218:    * @deprecated
 219:    */
 220:   public String getText()
 221:   {
 222:     try
 223:       {
 224:         return getDocument().getText(0, getDocument().getLength());
 225:       }
 226:     catch (BadLocationException ble)
 227:       {
 228:         // This should never happen.
 229:         throw new AssertionError(ble);
 230:       }
 231:   }

---

As far as I can see we have roughly two options:

1. Anywhere we use char[] for a password, convert to a String for simpler APIs.
2. JPasswordField is just an instance of JTextField, we could presumably provide our own implementation with the same API to consistently handle passwords with a char[]. We'd need to be sure that JTextField itself used char[] always, and further up the chain to root out all String usages.

A secondary decision to be made:

• We may still want our own JPasswordField implementation that will return a hashed value. The benefit here is that we stop dealing with plain-text passwords in the clear as soon as we read the password value from the UI.

[RoiEXLab]

@DanVanAtta
On the first decision to be made: I'd probably go with option 1, I realised that especially for the PbF/PbEM we keep passwords for the forum and email relatively long in memory. Of course I'll try to use the "protected" version when refactoring the whole system, but I don't think it's worth creating our own swing component just for this.

About decision 2: Hashing will only work for lobby login, not for the PbF and PbEM for obvious reasons. Of course if we created a JPasswordField that protects the password internally that would be a viable option, but then again probably too much work especially when looking at the JavaFX API that doesn't provide any special handling for passwords. Everything is a String.

[RoiEXLab]

As an addition:
The OpenJDK (at least the latest master branch) uses a different way of obtaining the content that doesn't involve string:
http://hg.openjdk.java.net/jdk/jdk/file/0c637249d934/src/java.desktop/share/classes/javax/swing/JPasswordField.java#l293

[DanVanAtta]

To clarify the idea on internal protection of password by a custom component that only returns a hashed version - in effect the password becomes the hashed version. This does not protect against 'replay' attacks where an attacker gets the hashed version, but if the user shares their password between sites then the plain-text version will not be compromised unless the hash is cracked.

I can go either way on this one TBH. TripleA is fragile, a heavy handed approach will be more sure to solve this, at the same time arguably unnecessary code moves us away from the goal of simplifying the code base.

[RoiEXLab]

@DanVanAtta I got that, I just wanted to point out that this wouldn't work for things other than the lobby, because other sites require us to send the "plain" password.

[DanVanAtta]

@RoiEXLab @ssoloff , suggesting some options below, which way, if any, are you leaning?

1. confirm the latest JDK versions that can be used with TripleA (or at least commonly used) would not have this problem, continue using char[] for passwords
2. use a custom swing component, use char[] for passwords
3. Focus only on minimizing variable scope and use String for passwords

In either case of char[] or String IMO we want to have nice scrubbing libraries that can 'blank' out the stored value to protect us from ourselves if we go on to 'toString()' the value. I was trying to double check if you really could guarantee an overwrite of a char[] value, given GC etc, writing over a value may not guarantee the value is destroyed in memory, and there is no guarantee a GC that reclaims memory will happen anytime 'soon' or even ever. I ran across this: https://security.stackexchange.com/questions/74718/is-it-more-secure-to-overwrite-the-value-char-in-a-string, which speaks to the topic and suggests to only focus on controlling variable scope and use String.

The end goal is to form a strategy of how we handle sensitive info, notably passwords. I'm also curious if we have done a survey of everywhere in the system to find any other places where we may be leaking passwords.

[ssoloff]

Focus only on minimizing variable scope and use String for passwords

How about a slight variant of (3), in which a custom value type is used instead of String?

I still think the biggest problem with String is accidental disclosure to something like a log file. While we would probably catch something like log.severe("Failed to login: " + password) in a code review, it would be much harder to catch something like this transitively. For example, assume you have a User type that has a String password field and is annotated with Lombok @ToString. Then log.severe("Failed to login: " + user) may not raise a red flag during a code review, but the password would be printed due to the hidden Lombok toString() implementation.

We could introduce a value type (e.g. Secret, SecureString, whatever) that is simply a wrapper around a String but with additional constraints:

1. toString() is not overridden or is overridden in such a way to not disclose the wrapped string.
2. Access to the wrapped string is performed using the _Execute Around_ pattern to make it as hard as possible to accidentally leak the wrapped string outside the container. For example:

public final class Secret {
  private final String value;

  // ... other stuff ...

  public void use(final Consumer<String> action) {
    action.accept(value);
  }

  public <R> R useAndReturn(final Function<String, R> action) {
    return action.apply(value);
  }
}

Presumably, during a PR, a code reviewer would confirm value is not being leaked further outside the Consumer and Function implementations, except for certain cases, such as sending the value to PasswordField#setText().

Bottom line is we're using a custom type to simply give a heads-up to the original author and code reviewers that the target instance should have its scope minimized rather than just relying on a variable name and context to convey that warning.

[DanVanAtta]

@ssoloff thanks for weighing in, sounds good

[RoiEXLab]

@ssoloff I didn't really follow the conversation here, but I'd like to see your review on #4454 (when it's ready, hopefully in 2018 when I'm not messing something up) before trying to implement something, because there are some places in the code, where having a string is inevitable.

[ssoloff]

@RoiEXLab No problem, I wasn't planning on doing anything in this area any time soon (tagging @DanVanAtta just so he's aware your in-flight PR might be affected by this in case he is going to take it on soon).

I agree that there will be places where String is required. Worst case, secret.useAndReturn(Function.identity()) would be used to extract the String value. It's simply serving as a speed-bump that says, "I know what I'm doing." But hopefully, the hand-off can occur within the Consumer<String>, if at all possible.

I was waiting to review #4454 when the <WIP> label was removed. I apologize if you wanted a review now. If so, please request a review from me--I should have time to go through it tomorrow.