Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8242416

Code sample in javax.net.ssl API is incorrect

XMLWordPrintable

      ADDITIONAL SYSTEM INFORMATION :
      This problem is platform independent.

      A DESCRIPTION OF THE PROBLEM :
      The following code sample can be found in the JavaDoc for class SSLEngine:

      SSLEngineResult r = engine.unwrap(src, dst);
         switch (r.getStatus()) {
         BUFFER_OVERFLOW:
             // Could attempt to drain the dst buffer of any already obtained
             // data, but we'll just increase it to the size needed.
             int appSize = engine.getSession().getApplicationBufferSize();
             ByteBuffer b = ByteBuffer.allocate(appSize + dst.position());
             dst.flip();
             b.put(dst);
             dst = b;
             // retry the operation.
             break;
         BUFFER_UNDERFLOW:
             int netSize = engine.getSession().getPacketBufferSize();
             // Resize buffer if needed.
             if (netSize > src.capacity()) {
                 ByteBuffer b = ByteBuffer.allocate(netSize);
                 src.flip();
                 b.put(src);
                 src = b;
             }
             // Obtain more inbound network data for src,
             // then retry the operation.
             break;
         // other cases: CLOSED, OK.
         }

      I believe that the buffer reallocation handling in the BUFFER_UNDERFLOW case discards data.

      For engine.unwrap(src, dst) to work, src would have to contain data between the offsets "position" and "limit" that could be consumed by unwrap().
      Let's assume position = 15 and limit = 35, so there would be 20 bytes of data available.
      In the case where unwrap() returns with BUFFER_UNDERFLOW and the packet buffer size has changed as well, a new buffer is first allocated:
      ByteBuffer b = ByteBuffer.allocate(netSize);
      Then, the src buffer is flipped:
      src.flip();

      Documentation of ByteBuffer.flip() states:
      public ByteBuffer flip()
      [...] The limit is set to the current position and then the position is set to zero.

      Therefore, the result of this operation will be
      pos = 0, limit = 15

      The next statement is supposed to copy existing data from src to the newly allocated ByteBuffer b, which is supposed to replace src afterwards:
      b.put(src);

      The documentation for put(ByteBuffer src) states:
      public ByteBuffer put (ByteBuffer src)
      [...] this method copies n = src.remaining() bytes from the given buffer into this buffer, starting at each buffer's current position. The positions of both buffers are then incremented by n.

      This will result in copying 15 bytes of already consumed data to the newly allocated buffer, and will set the position of the newly allocated buffer to 15 as well, thereby discarding the data originally pending for consumption.

      What's even more bothering is that this code was last changed in 2018, when some contributor figured out that the condition that checks the buffer sizes had also been wrong for at least 10 years, but obviously noone bothered to actually review this sample code, despite the fact that it is supposed to instruct readers of the documentation on how to implement security-critical software.

      STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
      Hard to trigger this bug, because packet buffer size changes during runtime are rare, and during initialization the buffers are empty.


      EXPECTED VERSUS ACTUAL BEHAVIOR :
      EXPECTED -
      Existing data is copied from one buffer to an enlarged new buffer, and the contents of the new buffer after readjustment of the buffer size are logically identical to the old buffer
      ACTUAL -
      New buffer is empty (if old buffer position = 0) or contains already consumed data (if old buffer position >= 1), and logical contents of the new buffer is empty space behind the (incorrect) data that was copied. Data pending consumption is discarded.

      FREQUENCY : always


            ascarpino Anthony Scarpino
            webbuggrp Webbug Group
            Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

              Created:
              Updated: