Name: gm110360 Date: 02/20/2004


A DESCRIPTION OF THE PROBLEM :
In addressing the question of how to mask keyboard input of passwords for console applications, you provide the following code (see http://java.sun.com/features/2002/09/pword_mask.html):

Code Sample 1: MaskingThread.java
/**
* This class attempts to erase characters echoed to the console.
*/
class MaskingThread extends Thread {
private boolean stop = false;
private int index;
private String prompt;


/**
*@param prompt The prompt displayed to the user
*/
public MaskingThread(String prompt) {
this.prompt = prompt;
}


/**
* Begin masking until asked to stop.
*/
public void run() {
while(!stop) {
try {
// attempt masking at this rate
this.sleep(1);
}catch (InterruptedException iex) {
iex.printStackTrace();
}
if (!stop) {
System.out.print("\r" + prompt + " \r" + prompt);
}
System.out.flush();
}
}


/**
* Instruct the thread to stop masking.
*/
public void stopMasking() {
this.stop = true;
}
}

Code Sample 2: PasswordField.java

import java.io.*;

/**
* This class prompts the user for a password and attempts to mask input with ""
*/

public class PasswordField {

/**
*@param prompt The prompt to display to the user.
*@return The password as entered by the user.
*/

String getPassword(String prompt) throws IOException {
// password holder
String password = "";
MaskingThread maskingthread = new MaskingThread(prompt);
Thread thread = new Thread(maskingthread);
thread.start();
// block until enter is pressed
while (true) {
char c = (char)System.in.read();
// assume enter pressed, stop masking
maskingthread.stopMasking();

if (c == '\r') {
c = (char)System.in.read();
if (c == '\n') {
break;
} else {
continue;
}
} else if (c == '\n') {
break;
} else {
// store the password
password += c;
}
}
return password;
}
}


I claim that this code suffers from at least 3 bugs:

bug #1: fails to handle Mac streams (which use a simple '\r' as line ends); in the code above, a '\r' (that is not followed by '\n') will end up part of the result; indeed, since it will advance to a new line, masking should fail altogether

bug #2: the stop field (in MaskingThread) is not volatile, which you really need in order to guarantee visibility across threads (ESPECIALLY ON MULTI CPU BOXES)

bug #3: it assumes that only 1 char has been typed beyond the prompt since the last mask; it is possibile, however, that several could have been

bug #4: there is a nasty timing/thread communication bug that happens on occaision is--the timing sometimes is such that the user has pressed enter, which causes the console to advance to the next line, but the unbuffering of its input and unblocking of the main (non-masking) thread does not occur soon enough to prevent a final mask, which now occurs on the next line. (See about 70% of the way thru the first post of http://forum.java.sun.com/thread.jsp?forum=9&thread=490728&tstart=0&trange=15 for more details--this bug HAS been observed in reality.)

In addition, it has some other poor programming practices:
--ignores interrupts, always throwing away the InterruptedException (extremely bad practice)
--fails to boost priority of the masking thread during the time that it masks, which is critical in helping ensure that masking always takes place when system under heavy load
--the run method, altho it flushes out, never checks for errors; turns out that you can very conveniently do both operations with a simple call to checkError
--the main password reading thread fails to use join to ensure that the masking thread is dead before continuing; this is probably a good idea

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
Here is code that I suggest as a replacement for the above:

/**
* Reads and returns some sensitive piece of information (e.g. a password)
* from the console (i.e. System.in and System.out) in a secure manner.
* <p>
* For top security, all console input is masked out while the user types in the password.
* Once the user presses enter, the password is read via a call to {@link #readLineSecure readLineSecure(in)},
* using a PushbackInputStream that wraps System.in.
* <p>
* This method never returns null.
* <p>
* @throws IOException if an I/O problem occurs
* @throws InterruptedException if the calling thread is interrupted while it is waiting at some point
* @see <a href="http://java.sun.com/features/2002/09/pword_mask.html">Password masking in console</a>
*/
public static final char[] readConsoleSecure(String prompt) throws IOException, InterruptedException {
// start a separate thread which will mask out all chars typed on System.in by overwriting them using System.out:
StreamMasker masker = new StreamMasker(System.out, prompt);
Thread threadMasking = new Thread(masker);
threadMasking.start();

// Goal: block this current thread (allowing masker to mask all user input)
// while the user is in the middle of typing the password.
// This may be achieved by trying to read just the first byte from System.in,
// since reading from System.in blocks until it detects that an enter has been pressed.
// Wrap System.in with a PushbackInputStream because this byte will be unread below.
PushbackInputStream pin = new PushbackInputStream(System.in);
int b = pin.read();

// When current thread gets here, the block on reading System.in is over (e.g. the user pressed enter, or some error occurred?)

// signal threadMasking to stop and wait till it is dead:
masker.stop();
threadMasking.join();

// check for stream errors:
if (b == -1) throw new IOException("end-of-file was detected in System.in without any data being read");
if (System.out.checkError()) throw new IOException("an I/O problem was detected in System.out");

// pushback the first byte and supply the now unaltered stream to readLineSecure which will return the complete password:
pin.unread(b);
return readLineSecure(pin);
}


/**
* Reads chars from in until an end-of-line sequence (EOL) or end-of-file (EOF) is encountered,
* and then returns the data as a char[].
* <p>
* The EOL sequence may be any of the standard formats: '\n' (unix), '\r' (mac), "\r\n" (dos).
* The EOL sequence is always completely read off the stream but is never included in the result.
* <i>Note:</i> this means that the result will never contain the chars '\n' or '\r'.
* In order to guarantee reading thru but not beyond the EOL sequence for all formats (unix, mac, dos),
* this method requires that a PushbackInputStream and not a more general InputStream be supplied.
* <p>
* The code is secure: no Strings are used, only char arrays,
* and all such arrays other than the result are guaranteed to be blanked out after last use to ensure privacy.
* Thus, this method is suitable for reading in sensitive information such as passwords.
* <p>
* This method never returns null; if no data before the EOL or EOF is read, a zero-length char[] is returned.
* <p>
* @throws IllegalArgumentException if in == null
* @throws IOException if an I/O problem occurs
* @see <a href="http://java.sun.com/j2se/1.4.2/docs/guide/security/jce/JCERefGuide.html#PBEEx">Password based encryption code examples from JCE documentation</a>
*/
public static final char[] readLineSecure(PushbackInputStream in) throws IllegalArgumentException, IOException {
if (in == null) throw new IllegalArgumentException("in == null");

char[] buffer = null;
try {
buffer = new char[128];
int offset = 0;

loop: while (true) {
int c = in.read();
switch (c) {
case -1:
case '\n':
break loop;

case '\r':
int c2 = in.read();
if ((c2 != '\n') && (c2 != -1)) in.unread(c2); // guarantees that mac & dos line end sequences are completely read thru but not beyond
break loop;

default:
buffer = checkBuffer(buffer, offset);
buffer[offset++] = (char) c;
break;
}
}

char[] result = new char[offset];
System.arraycopy(buffer, 0, result, 0, offset);
return result;
}
finally {
eraseChars(buffer);
}
}


/**
* Checks if buffer is sufficiently large to store an element at an index == offset.
* If it is, then buffer is simply returned.
* If it is not, then a new char[] of more than sufficient size is created and initialized with buffer's current elements and returned;
* the original supplied buffer is guaranteed to be blanked out upon method return in this case.
* <p>
* @throws IllegalArgumentException if buffer == null; offset < 0
*/
public static final char[] checkBuffer(char[] buffer, int offset) throws IllegalArgumentException {
if (buffer == null) throw new IllegalArgumentException("buffer == null");
if (offset < 0) throw new IllegalArgumentException("offset = " + offset + " is < 0");

if (offset < buffer.length)
return buffer;
else {
try {
char[] bufferNew = new char[offset + 128];
System.arraycopy(buffer, 0, bufferNew, 0, buffer.length);
return bufferNew;
}
finally {
eraseChars(buffer);
}
}
}


/** If buffer is not null, fills buffer with space (' ') chars. */
public static final void eraseChars(char[] buffer) {
if (buffer != null) Arrays.fill(buffer, ' ');
}


/**
* Masks an InputStream by overwriting blank chars to the PrintStream corresponding to its output.
* A typical application is for password input masking.
* <p>
* @see <a href="http://forum.java.sun.com/thread.jsp?forum=9&thread=490728&tstart=0&trange=15">My forum posting on password entry</a>
* @see <a href="http://java.sun.com/features/2002/09/pword_mask.html">Password masking in console</a>
*/
public static class StreamMasker implements Runnable {

private static final String TEN_BLANKS = StringUtil.repeatChars(' ', 10);
private final PrintStream out;
private final String promptOverwrite;
private volatile boolean doMasking; // MUST be volatile to ensure update by one thread is instantly visible to other threads

/**
* Constructor.
* <p>
* @throws IllegalArgumentException if out == null; prompt == null; prompt contains the char '\r' or '\n'
*/
public StreamMasker(PrintStream out, String prompt) throws IllegalArgumentException {
if (out == null) throw new IllegalArgumentException("out == null");
if (prompt == null) throw new IllegalArgumentException("prompt == null");
if (prompt.indexOf('\r') != -1) throw new IllegalArgumentException("prompt contains the char '\\r'");
if (prompt.indexOf('\n') != -1) throw new IllegalArgumentException("prompt contains the char '\\n'");

this.out = out;
String setCursorToStart = StringUtil.repeatChars('\b', prompt.length() + TEN_BLANKS.length());
this.promptOverwrite =
setCursorToStart + // sets cursor back to beginning of line:
prompt + // writes prompt (critical: this reduces visual flicker in the prompt text that otherwise occurs if simply write blanks here)
TEN_BLANKS + // writes 10 blanks beyond the prompt to mask out any input; go 10, not 1, spaces beyond end of prompt to handle the (hopefully rare) case that input occurred at a rapid rate
setCursorToStart + // sets cursor back to beginning of line:
prompt; // writes prompt again; the cursor will now be positioned immediately after prompt (critical: overwriting only works if all input text starts here)
}


/**
* Repeatedly overwrites the current line of out with prompt followed by blanks.
* This effectively masks any chars coming on out, as long as the masking occurs fast enough.
* <p>
* To help ensure that masking occurs when system is in heavy use, the calling thread will have its priority
* boosted to the max for the duration of the call (with its original priority restored upon return).
* Interrupting the calling thread will eventually result in an exit from this method,
* and the interrupted status of the calling thread will be set to true.
* <p>
* @throws RuntimeException if an error in the masking process is detected
*/
public void run() throws RuntimeException {
int priorityOriginal = Thread.currentThread().getPriority();
Thread.currentThread().setPriority(Thread.MAX_PRIORITY);
try {
doMasking = true; // do this assignment here and NOT at variable declaration line to allow this instance to be restarted if desired
while (doMasking) {
out.print(promptOverwrite);
// call checkError, which first flushes out, and then lets us confirm that everything was written correctly:
if (out.checkError()) throw new RuntimeException("an I/O problem was detected in out"); // should be an IOException, but that would break method contract

// limit the masking rate to fairly share the cpu; interruption breaks the loop
try {
Thread.currentThread().sleep(1); // have experimentally found that sometimes see chars for a brief bit unless set this to its min value
}
catch (InterruptedException ie) {
Thread.currentThread().interrupt(); // resets the interrupted status, which is typically lost when an InterruptedException is thrown, as per our method contract; see Lea, "Concurrent Programming in Java Second Edition", p. 171
return; // return, NOT break, since now want to skip the lines below where write bunch of blanks since typically the user will not have pressed enter yet
}
}
// erase any prompt that may have been spuriously written on the NEXT line after the user pressed enter (see reply 2 of http://forum.java.sun.com/thread.jsp?forum=9&thread=490728&tstart=0&trange=15)
out.print('\r');
for (int i = 0; i < promptOverwrite.length(); i++) out.print(' ');
out.print('\r');
}
finally {
Thread.currentThread().setPriority(priorityOriginal);
}
}

/** Signals any thread executing run to stop masking and exit run. */
public void stop() { doMasking = false; }

}

My gosh, Sun, WAKE UP: the average programmer should NOT have to jump thru such hoops and know so much simply to ask the user for a password in a console app--would you PLEASE finally add decent console support to Java? (And don't tell people to use a gui: many server side apps are headless.)



URL OF FAULTY DOCUMENTATION :
http://java.sun.com/features/2002/09/pword_mask.html
(Incident Review ID: 239221)
======================================================================