Skip to end of metadata
Go to start of metadata

Java input classes such as Scanner and BufferedInputStream facilitate fast, nonblocking I/O by buffering an underlying input stream. Programs can create multiple wrappers on an InputStream. Programs that use multiple wrappers around a single input stream, however, can behave unpredictably depending on whether the wrappers allow look-ahead. An attacker can exploit this difference in behavior, for example, by redirecting System.in (from a file) or by using the System.setIn() method to redirect System.in. In general, any input stream that supports nonblocking buffered I/O is susceptible to this form of misuse.

An input stream must not have more than one buffered wrapper. Instead, create and use only one wrapper per input stream, either by passing it as an argument to the methods that need it or by declaring it as a class variable.

Likewise, an output stream must not have more than one buffered wrapper because multiple wrappers can cause multiple output strings to be output in an unexpected order. For example, the javax.servlet.ServletResponse allows for the creation of a PrintWriter or an OutputStream to hold the response generated by a web servlet. But only one or the other should be used, not both.

Noncompliant Code Example

This noncompliant code example creates multiple BufferedInputStream wrappers on System.in, even though there is only one declaration of a BufferedInputStream. The getChar() method creates a new BufferedInputStream each time it is called. Data that is read from the underlying stream and placed in the buffer during execution of one call cannot be replaced in the underlying stream so that a second call has access to it. Consequently, data that remains in the buffer at the end of a particular execution of getChar() is lost. Although this noncompliant code example uses a BufferedInputStream, any buffered wrapper is unsafe; this condition is also exploitable when using a Scanner, for example.

public final class InputLibrary {
  public static char getChar() throws EOFException, IOException {
    BufferedInputStream in = new BufferedInputStream(System.in); // Wrapper
    int input = in.read();
    if (input == -1) {
      throw new EOFException();
    }
    // Down casting is permitted because InputStream guarantees read() in range
    // 0..255 if it is not -1
    return (char) input;
  }

  public static void main(String[] args) {
    try {
      // Either redirect input from the console or use
      // System.setIn(new FileInputStream("input.dat"));
      System.out.print("Enter first initial: ");
      char first = getChar();
      System.out.println("Your first initial is " + first);
      System.out.print("Enter last initial: ");
      char last = getChar();
      System.out.println("Your last initial is " + last);
    } catch (EOFException e) {
      System.err.println("ERROR");
      // Forward to handler
    } catch (IOException e) {
      System.err.println("ERROR");
      // Forward to handler
    }
  }
}
Implementation Details (POSIX)

When compiled under Java 1.6.0 and run from the command line, this program successfully takes two characters as input and prints them out. However, when run with a file redirected to standard input, the program throws EOFException because the second call to getChar() finds no characters to read upon encountering the end of the stream.

It may appear that the mark() and reset() methods of BufferedInputStream could be used to replace the read bytes. However, these methods provide look-ahead by operating on the internal buffers of the BufferedInputStream rather than by operating directly on the underlying stream. Because the example code creates a new BufferedInputStream on each call to getchar(), the internal buffers of the previous BufferedInputStream are lost.

Compliant Solution (Class Variable)

Create and use only a single BufferedInputStream on System.in. This compliant solution ensures that all methods can access the BufferedInputStream by declaring it as a class variable:

public final class InputLibrary {
  private static BufferedInputStream in =
      new BufferedInputStream(System.in);

  public static char getChar() throws EOFException, IOException {
    int input = in.read();
    if (input == -1) {
      throw new EOFException();
    }
    in.skip(1); // This statement is to advance to the next line.
                // The noncompliant code example deceptively
                // appeared to work without it (in some cases).
    return (char) input;
  }

  public static void main(String[] args) {
    try {
      System.out.print("Enter first initial: ");
      char first = getChar();
      System.out.println("Your first initial is " + first);
      System.out.print("Enter last initial: ");
      char last = getChar();
      System.out.println("Your last initial is " + last);
    } catch (EOFException e) {
      System.err.println("ERROR");
      // Forward to handler
    } catch (IOException e) {
       System.err.println("ERROR");
       // Forward to handler
    }
  }
}
Implementation Details (POSIX)

When compiled under Java 1.6.0 and run from the command line, this program successfully takes two characters as input and prints them out. Unlike the noncompliant code example, this program also produces correct output when run with a file redirected to standard input.

Compliant Solution (Accessible Class Variable)

This compliant solution uses both System.in and the InputLibrary class, which creates a buffered wrapper around System.in. Because the InputLibrary class and the remainder of the program must share a single buffered wrapper, the InputLibrary class must export a reference to that wrapper. Code outside the InputLibrary class must use the exported wrapper rather than create and use its own additional buffered wrapper around System.in.

public final class InputLibrary {
  private static BufferedInputStream in =
     new BufferedInputStream(System.in);

  static BufferedInputStream getBufferedWrapper() {
    return in;
  }

  // ... Other methods
}


// Some code that requires user input from System.in
class AppCode {
  private static BufferedInputStream in;

  AppCode() {
    in = InputLibrary.getBufferedWrapper();
  }

  // ... Other methods
}

Note that reading from a stream is not a thread-safe operation by default; consequently, this compliant solution may be inappropriate in multithreaded environments. In such cases, explicit synchronization is required.

Risk Assessment

Creating multiple buffered wrappers around an InputStream can cause unexpected program behavior when the InputStream is redirected.

Rule

Severity

Likelihood

Remediation Cost

Priority

Level

FIO06-J

Low

Unlikely

Medium

P2

L3

Automated Detection

Sound automated detection of this vulnerability is not feasible in the general case. Heuristic approaches may be useful.

Bibliography

 


17 Comments

  1. The formatting is fixed, and the examples are sound. But I'm still unsure why this is a security flaw...at present it sounds more like a bug report that the Java devs should fix. Or maybe the Eclipse devs. I'm not convinced this is not merely an Eclipse bug. Does any Java textbook or official document mention this behavior?

    Personal obvervation is useful (though not a complete source for a secure coding rule). Was this behavior your own observation? Mentioning that it happens on Eclipse is a start, but reproduceablity is important...what version of Eclipse? what version of Java? what platform is Eclipse running on?

    1. It's a problem that is not restricted to System.in. Many input stream/reader wrappers perform buffering (BufferedInputStream certainly does) for performance or to look ahead (if you were reading a stream of digits, how would you know where the end was without going at least one over?). Generally once wrapped/decorated you should ignore the underlying stream or explicitly hand over (closing can be a little awkward).

  2. This behavior occurs from the command line, so I don't think it's specific to Eclipse. I wasn't aware that the issue had to do with buffering specifically. Does the rule need to mention the connection with buffering? Should I expand it to ignoring underlying wrapped streams as suggested?

    1. Looks like you'll have to do some research, David, as I don't know precisely why this happens either (other than Thomas's comment blaming it on buffering).

      Ideally you'd find some authoritative document that says "don't use multiple scanners", and you could just cite that as a reference. Lacking that, perhaps you should do some experiments with BufferedInputStream...maybe wrapping System.in inside a BufferedInputStream will reproduce the problem. In which case you could cite the BufferedInputStream Javadoc API and explain why your code behaves the way it does. The good part of the research is it may help you generalize just what ou can and can't do with scanners, which will make your rule more precise and helpful to others.

  3. Good observations, David (Neville). A few questions for you :

    • How are you creating multiple wrappers in the noncompliant example (NCE)? Isn't the scope of BufferedInputStream limited to the method?
    • Can you be more specific in throwing exceptions (instead of IOException).
    • The modifiers of the class/methods are all public. (reduce scope as far as possible) (granted, it is an InputLibrary)
    • If you say that it produces surprising results upon redirecting a stream only, why not provide the appropriate example in the NCE? It appears that you want to say that this NCE works but if you modify it, it doesn't.
    • I spot a narrowing primitive conversion from int to char which may not be the ideal thing to do.
    • I think a related issue is, while reading bytes from a stream sometimes one doesn't account for the fact that multiple threads can access the same stream inconsistently. When you provide the compliant solution with a single stream, I think this point gains more relevance.
    1. David, good work. Not only does the rule explain what is going on, it is clearer. Remaining issues. My only question is this: So what happens when you run the NCCE? Does the programs' output vary between invocations? Does it print the Wrong Thing? Need an implementation details section with this info.

  4. Hey, I tried to address all of the issues you guys mentioned, sometimes in the code and sometimes in the paragraphs describing it. The only thing I didn't really add was the point about multiple threads - I wasn't sure how to modify the rule to account for them.

  5. Much clearer, David. Good work, I think this should suffice. I'll add a separate guideline about the threads issue at a later date. If possible, add a line about why one would need to redirect input from a file to System.in.

  6. I will say that the implementation details sections resolve my question, however they should go after the code that 'generates' them. For instance, the NCCE should have intro text, then the code, then the imp.details section describing what the code does.

    Your line about redirecting from System.in is probably necessary in the Java culture, you can just explain that redirecting input is common practice in the Unix culture. (smile)

  7. David, are there any workarounds/exceptions to this? What if someone uses: mark(), reset(), markSupported() of the class InputStream, to make data that is read available again? Also, you might want to check if any example violates INT04-J. Do not attempt to store signed values in the char integral type.

  8. The following criticism of the Compliant Solution is its preamble.

    "However, if a program were to use this library in conjunction with other input from a user that needs another buffered wrapper on System.in, the library must be modified so that all code uses the same buffered wrapper instead of additional ones that are created."

    Why not add the suggestion modification to the Compliant Solution and eliminate the sentence? BTW if I knew how to do it I would.

    1. I've added a separate compliant solution for this piece of advice. Thanks.

  9. the paragraph at the last CCE sounds bad for me.

    If a program intends to use System.in as well as this class ...

    how about the following?

    If another class intends to use System.in as well as our class InputLibrary, the class must use the same buffered wrapper which is used in InputLibrary, rather than creating and using its own additional buffered wrapper. Consequently, InputLibrary must make available a reference to the buffered wrapper for such classes.

    1. I adopted your paragraph, and tweaked it some more. Sigh. English is just not that good at expressing some concepts clearly. (sad)

    • i don't think we are closing the BufferedReader in in both the CSs.
    • The above gets sinister especially in the 2nd CS when client code cannot decide whether it should close the stream or not.
    1. In the general sense, this is a problem. But in these specific examples, the input stream is System.in which need not be closed.

    • True, I bet we gave it some thought back then
    • I think the general solution could be to keep streams out of bean classes. On Similar lines, the data access object code (DAO layer) should abstract streams related to data handling and should populate beans and send them back to the service layer.