MINA
  1. MINA
  2. DIRMINA-637

SSLEngine output buffer seems to be too small

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.1.1, 1.1.7
    • Fix Version/s: 1.1.8
    • Component/s: Filter
    • Labels:
      None

      Description

      the code below is in SSLHandler.java. it makes the assumption that the size of the output will never be larger than 2x the size of the input. that assumption appears to not hold up. It looks like this code has been fixed in trunk, but not in 1.1.7. we only see an error for VERY specific content, i.e. almost never.

      public void encrypt(ByteBuffer src) throws SSLException {
      if (!initialHandshakeComplete)

      { throw new IllegalStateException(); }

      // The data buffer is (must be) empty, we can reuse the entire
      // buffer.
      outNetBuffer.clear();

      // Loop until there is no more data in src
      while (src.hasRemaining()) {

      if (src.remaining() > ((outNetBuffer.capacity() - outNetBuffer
      .position()) / 2)) {
      // We have to expand outNetBuffer
      // Note: there is no way to know the exact size required, but enrypted data
      // shouln't need to be larger than twice the source data size?
      outNetBuffer = SSLByteBufferPool.expandBuffer(outNetBuffer, src
      .capacity() * 2);
      if (SessionLog.isDebugEnabled(session))

      { SessionLog.debug(session, " expanded outNetBuffer:" + outNetBuffer); }

      }

      SSLEngineResult result = sslEngine.wrap(src, outNetBuffer);
      if (SessionLog.isDebugEnabled(session))

      { SessionLog.debug(session, " Wrap res:" + result); }

      if (result.getStatus() == SSLEngineResult.Status.OK) {
      if (result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_TASK)

      { doTasks(); }

      } else

      { throw new SSLException("SSLEngine error during encrypt: " + result.getStatus() + " src: " + src + "outNetBuffer: " + outNetBuffer); }

      }

      outNetBuffer.flip();
      }

        Activity

        Diego Belfer made changes -
        Attachment ssl-filter.patch [ 12506474 ]
        Hide
        Diego Belfer added a comment -

        We had this issue in production and we fixed it using the patch attached here. It expands the buffer up to 3 times if the wrap method returns a overflow status.

        Show
        Diego Belfer added a comment - We had this issue in production and we fixed it using the patch attached here. It expands the buffer up to 3 times if the wrap method returns a overflow status.
        Hide
        Dmitry Shohov added a comment -

        fix provided by Dan Mihai Dumitriu in comment is not working. It expands buffer every time when encrypt() method is called. This leads to OOM errors. Must be improved before applying in 1.1.8

        Show
        Dmitry Shohov added a comment - fix provided by Dan Mihai Dumitriu in comment is not working. It expands buffer every time when encrypt() method is called. This leads to OOM errors. Must be improved before applying in 1.1.8
        Hide
        Emmanuel Lecharny added a comment -

        Acn you do a svn diff, and attach the result in this JIRA ? It will be easier to inject in trunk.

        Thanks !

        Show
        Emmanuel Lecharny added a comment - Acn you do a svn diff, and attach the result in this JIRA ? It will be easier to inject in trunk. Thanks !
        Hide
        Dan Mihai Dumitriu added a comment -

        To fix our immediate problem, I adapted the code from 2.0.0, as shown below. Maybe it would be good to make the initial capacity larger than just src.remaining(), so that it doesn't overflow everytime.

        public void encrypt(ByteBuffer src) throws SSLException {
        if (!handshakeComplete)

        { throw new IllegalStateException(); }

        // The data buffer is (must be) empty, we can reuse the entire
        // buffer.
        outNetBuffer.clear();

        int capacity = Math.max(
        src.remaining(),
        sslEngine.getSession().getPacketBufferSize());

        outNetBuffer = SSLByteBufferPool.expandBuffer(outNetBuffer, capacity);

        // Loop until there is no more data in src
        while (src.hasRemaining()) {

        SSLEngineResult result = sslEngine.wrap(src, outNetBuffer);

        if (result.getStatus() == SSLEngineResult.Status.OK) {
        if (result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_TASK)

        { doTasks(); }

        } else if (result.getStatus() == SSLEngineResult.Status.BUFFER_OVERFLOW)

        { outNetBuffer = SSLByteBufferPool.expandBuffer(outNetBuffer, outNetBuffer.capacity() * 2); }

        else

        { throw new SSLException("SSLEngine error during encrypt: " + result.getStatus() + " src: " + src + "outNetBuffer: " + outNetBuffer); }

        }

        outNetBuffer.flip();
        }

        Show
        Dan Mihai Dumitriu added a comment - To fix our immediate problem, I adapted the code from 2.0.0, as shown below. Maybe it would be good to make the initial capacity larger than just src.remaining(), so that it doesn't overflow everytime. public void encrypt(ByteBuffer src) throws SSLException { if (!handshakeComplete) { throw new IllegalStateException(); } // The data buffer is (must be) empty, we can reuse the entire // buffer. outNetBuffer.clear(); int capacity = Math.max( src.remaining(), sslEngine.getSession().getPacketBufferSize()); outNetBuffer = SSLByteBufferPool.expandBuffer(outNetBuffer, capacity); // Loop until there is no more data in src while (src.hasRemaining()) { SSLEngineResult result = sslEngine.wrap(src, outNetBuffer); if (result.getStatus() == SSLEngineResult.Status.OK) { if (result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NEED_TASK) { doTasks(); } } else if (result.getStatus() == SSLEngineResult.Status.BUFFER_OVERFLOW) { outNetBuffer = SSLByteBufferPool.expandBuffer(outNetBuffer, outNetBuffer.capacity() * 2); } else { throw new SSLException("SSLEngine error during encrypt: " + result.getStatus() + " src: " + src + "outNetBuffer: " + outNetBuffer); } } outNetBuffer.flip(); }
        Hide
        Emmanuel Lecharny added a comment -

        Ok, so we are safe in 2.0 That's a good news !

        Show
        Emmanuel Lecharny added a comment - Ok, so we are safe in 2.0 That's a good news !
        Hide
        Dan Mihai Dumitriu added a comment -

        I don't believe that it does. The 2.0.0 code doubles the output buffer and tries again if there is overflow.

        Show
        Dan Mihai Dumitriu added a comment - I don't believe that it does. The 2.0.0 code doubles the output buffer and tries again if there is overflow.
        Emmanuel Lecharny made changes -
        Fix Version/s 1.1.8 [ 12313117 ]
        Hide
        Emmanuel Lecharny added a comment -

        We should check if it also affects 2.0.0

        Show
        Emmanuel Lecharny added a comment - We should check if it also affects 2.0.0
        Emmanuel Lecharny made changes -
        Field Original Value New Value
        Assignee Emmanuel Lecharny [ elecharny ]
        Dan Mihai Dumitriu created issue -

          People

          • Assignee:
            Emmanuel Lecharny
            Reporter:
            Dan Mihai Dumitriu
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:

              Development