Name: js151677 Date: 04/21/2004


URL OF FAULTY DOCUMENTATION :
http://java.sun.com/j2se/1.4.2/docs/guide/security/jce/JCERefGuide.html#PBEEx

A DESCRIPTION OF THE PROBLEM :
See also http://forum.java.sun.com/thread.jsp?forum=9&thread=490728&tstart=0

You offer the following code as an example of how to read chars off a stream securely (see http://java.sun.com/j2se/1.4.2/docs/guide/security/jce/JCERefGuide.html#PBEEx):

    /**
     * Reads user password from given input stream.
     */
    public char[] readPasswd(InputStream in) throws IOException {
        char[] lineBuffer;
        char[] buf;
        int i;
        buf = lineBuffer = new char[128];
        int room = buf.length;
        int offset = 0;
        int c;

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

              case '\r':
                int c2 = in.read();
                if ((c2 != '\n') && (c2 != -1)) {
                    if (!(in instanceof PushbackInputStream)) {
                        in = new PushbackInputStream(in);
                    }
                    ((PushbackInputStream)in).unread(c2);
                } else
                    break loop;

              default:
                if (--room < 0) {
                    buf = new char[offset + 128];
                    room = buf.length - offset - 1;
                    System.arraycopy(lineBuffer, 0, buf, 0, offset);
                    Arrays.fill(lineBuffer, ' ');
                    lineBuffer = buf;
                }
                buf[offset++] = (char) c;
                break;
            }
        }

        if (offset == 0) {
            return null;
        }

        char[] ret = new char[offset];
        System.arraycopy(buf, 0, ret, 0, offset);
        Arrays.fill(buf, ' ');

        return ret;
    }

Since this code is undoubtably used by tons of people after the peruse your documentation, it should be pristine clean, clear, and free of all known bugs.

I claim that this code suffers from at least 2 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) falls thru to the default case, and so will be part of the result

bug #2: fails to guarantee that all char[]s get blanked out (need a finally clause to guarantee this in the case of exceptions)

In addition, albeit much less critical, are some poor programming practices:
--can return null (better is to return a zero-length array, so that the caller need never do null checks)
--too terse of names
--variable declaration not sufficiently localized (best practice is to always declare them at as low a level as possible, not all of them at the beginning of method like C requires)
--lack of modularity (fine grained methods that do one simple function are best both for code clarity as well as reuse), .


EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
Here is code I suggest as a replacement which fixes all the above bugs and bad features:

/**
* 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, ' ');
}
(Incident Review ID: 239212)
======================================================================