You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2019/03/09 20:47:12 UTC

[tomcat] branch 8.5.x updated: Avoid overflow with OpenSSL engine

This is an automated email from the ASF dual-hosted git repository.

remm pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/8.5.x by this push:
     new ef099ae  Avoid overflow with OpenSSL engine
ef099ae is described below

commit ef099aeb3a3d6e1c6664ada940e3092ed7348606
Author: remm <re...@apache.org>
AuthorDate: Sat Mar 9 21:37:11 2019 +0100

    Avoid overflow with OpenSSL engine
    
    Avoid many overflow situations with OpenSSL engine, overflow will now
    only occur if the destination buffers are all full when unwrap is
    called. Since it has an internal buffer (the bio in the native code), it
    doesn't have to write all the decrypted data available and can hold them
    until the next unwrap call. Also improve the overflow handling in the
    NIO2 code to give the right bytes produced and avoid some useless
    processing.
    Rebase on the read(ByteBuffer[]) code from 9.
    # Conflicts:
    #	java/org/apache/tomcat/util/net/SecureNio2Channel.java
---
 .../apache/tomcat/util/net/SecureNio2Channel.java  | 41 ++++++++++++++++++++--
 .../tomcat/util/net/openssl/OpenSSLEngine.java     |  2 +-
 webapps/docs/changelog.xml                         |  4 +++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/java/org/apache/tomcat/util/net/SecureNio2Channel.java b/java/org/apache/tomcat/util/net/SecureNio2Channel.java
index 4ae2509..6eae95d 100644
--- a/java/org/apache/tomcat/util/net/SecureNio2Channel.java
+++ b/java/org/apache/tomcat/util/net/SecureNio2Channel.java
@@ -1008,16 +1008,28 @@ public class SecureNio2Channel extends Nio2Channel  {
                         long read = 0;
                         //the SSL engine result
                         SSLEngineResult unwrap;
+                        ByteBuffer[] dsts2 = dsts;
+                        int length2 = length;
+                        boolean processOverflow = false;
                         do {
+                            boolean useOverflow = false;
+                            if (processOverflow) {
+                                useOverflow = true;
+                            }
+                            processOverflow = false;
                             //prepare the buffer
                             netInBuffer.flip();
                             //unwrap the data
-                            unwrap = sslEngine.unwrap(netInBuffer, dsts, offset, length);
+                            unwrap = sslEngine.unwrap(netInBuffer, dsts2, offset, length2);
                             //compact the buffer
                             netInBuffer.compact();
                             if (unwrap.getStatus() == Status.OK || unwrap.getStatus() == Status.BUFFER_UNDERFLOW) {
                                 //we did receive some data, add it to our total
                                 read += unwrap.bytesProduced();
+                                if (useOverflow) {
+                                    // Remove the data read into the overflow buffer
+                                    read -= dsts2[dsts.length].position();
+                                }
                                 //perform any tasks if needed
                                 if (unwrap.getHandshakeStatus() == HandshakeStatus.NEED_TASK)
                                     tasks();
@@ -1034,13 +1046,36 @@ public class SecureNio2Channel extends Nio2Channel  {
                                 //buffer overflow can happen, if we have read data, then
                                 //empty out the dst buffer before we do another read
                                 break;
-                            } else {
+                            } else if (unwrap.getStatus() == Status.BUFFER_OVERFLOW) {
                                 //here we should trap BUFFER_OVERFLOW and call expand on the buffer
                                 //for now, throw an exception, as we initialized the buffers
                                 //in the constructor
+                                ByteBuffer readBuffer = getBufHandler().getReadBuffer();
+                                boolean found = false;
+                                for (ByteBuffer buffer : dsts2) {
+                                    if (buffer == readBuffer) {
+                                        found = true;
+                                    }
+                                }
+                                if (found) {
+                                    throw new IOException(sm.getString("channel.nio.ssl.unwrapFail", unwrap.getStatus()));
+                                } else {
+                                    // Add the main read buffer in the destinations and try again
+                                    dsts2 = new ByteBuffer[dsts.length + 1];
+                                    for (int i = 0; i < dsts.length; i++) {
+                                        dsts2[i] = dsts[i];
+                                    }
+                                    dsts2[dsts.length] = readBuffer;
+                                    length2 = length + 1;
+                                    getBufHandler().configureReadBufferForWrite();
+                                    processOverflow = true;
+                                }
+                            } else if (unwrap.getStatus() == Status.CLOSED) {
+                                break;
+                            } else {
                                 throw new IOException(sm.getString("channel.nio.ssl.unwrapFail", unwrap.getStatus()));
                             }
-                        } while (netInBuffer.position() != 0); //continue to unwrapping as long as the input buffer has stuff
+                        } while ((netInBuffer.position() != 0) || processOverflow); //continue to unwrapping as long as the input buffer has stuff
                         int capacity = 0;
                         final int endOffset = offset + length;
                         for (int i = offset; i < endOffset; i++) {
diff --git a/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java b/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java
index 5c085fb..241a35d 100644
--- a/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java
+++ b/java/org/apache/tomcat/util/net/openssl/OpenSSLEngine.java
@@ -572,7 +572,7 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn
         int bytesProduced = 0;
         int idx = offset;
         // Do we have enough room in dsts to write decrypted data?
-        if (capacity < pendingApp) {
+        if (capacity == 0) {
             return new SSLEngineResult(SSLEngineResult.Status.BUFFER_OVERFLOW, getHandshakeStatus(), written, 0);
         }
 
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 3b2b00d..39df098 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -183,6 +183,10 @@
         Verify HTTP/2 stream is still writable before assuming a timeout
         occurred. (remm)
       </fix>
+      <fix>
+        Avoid some overflow cases with OpenSSL to improve efficiency, as the
+        OpenSSL engine has an internal buffer. (remm)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="WebSocket">


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org