Guard against unbounded grow of suppressed exceptions storage (#13351)

normanmaurer · web-flow · commit ba1ff7a3c6ba · 2023-04-23T16:07:31.000+02:00
Motivation: We should guard against unbounded grows of suppressed exceptions in our SSLEngine implementation to prevent possible OOME Modifications: - Check if a suppressed exception was already added for a specific error code before adding it again - Ensure we always null out pending exception field. Result: Fixes #13350
diff --git a/common/src/main/java/io/netty/util/internal/EmptyArrays.java b/common/src/main/java/io/netty/util/internal/EmptyArrays.java
@@ -37,5 +37,7 @@ public final class EmptyArrays {
     public static final X509Certificate[] EMPTY_X509_CERTIFICATES = {};
     public static final javax.security.cert.X509Certificate[] EMPTY_JAVAX_X509_CERTIFICATES = {};
 
+    public static final Throwable[] EMPTY_THROWABLES = {};
+
     private EmptyArrays() { }
 }
diff --git a/common/src/main/java/io/netty/util/internal/ThrowableUtil.java b/common/src/main/java/io/netty/util/internal/ThrowableUtil.java
@@ -76,4 +76,12 @@ public static void addSuppressed(Throwable target, List<Throwable> suppressed) {
             addSuppressed(target, t);
         }
     }
+
+    @SuppressJava6Requirement(reason = "Throwable getSuppressed is only available for >= 7. Has check for < 7.")
+    public static Throwable[] getSuppressed(Throwable source) {
+        if (!haveSuppressed()) {
+            return EmptyArrays.EMPTY_THROWABLES;
+        }
+        return source.getSuppressed();
+    }
 }
diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -1077,20 +1077,17 @@ private SSLException shutdownWithError(String operations, int sslError) {
     }
 
     private SSLException shutdownWithError(String operation, int sslError, int error) {
-        String errorString = SSL.getErrorString(error);
         if (logger.isDebugEnabled()) {
+            String errorString = SSL.getErrorString(error);
             logger.debug("{} failed with {}: OpenSSL error: {} {}",
                          operation, sslError, error, errorString);
         }
 
         // There was an internal error -- shutdown
         shutdown();
-        if (handshakeState == HandshakeState.FINISHED) {
-            return new SSLException(errorString);
-        }
 
-        SSLHandshakeException exception = new SSLHandshakeException(errorString);
-        // If we have a handshakeException stored already we should include it as well to help the user debug things.
+        SSLException exception = newSSLExceptionForError(error);
+        // If we have a pendingException stored already we should include it as well to help the user debug things.
         if (pendingException != null) {
             exception.initCause(pendingException);
             pendingException = null;
@@ -1360,15 +1357,12 @@ private boolean needWrapAgain(int stackError) {
         // This is needed so we ensure close_notify etc is correctly send to the remote peer.
         // See https://github.com/netty/netty/issues/3900
         if (SSL.bioLengthNonApplication(networkBIO) > 0) {
-            // we seems to have data left that needs to be transferred and so the user needs
+            // we seem to have data left that needs to be transferred and so the user needs
             // call wrap(...). Store the error so we can pick it up later.
-            String message = SSL.getErrorString(stackError);
-            SSLException exception = handshakeState == HandshakeState.FINISHED ?
-                    new SSLException(message) : new SSLHandshakeException(message);
             if (pendingException == null) {
-                pendingException = exception;
-            } else {
-                ThrowableUtil.addSuppressed(pendingException, exception);
+                pendingException = newSSLExceptionForError(stackError);
+            } else if (shouldAddSuppressed(pendingException, stackError)) {
+                ThrowableUtil.addSuppressed(pendingException, newSSLExceptionForError(stackError));
             }
             // We need to clear all errors so we not pick up anything that was left on the stack on the next
             // operation. Note that shutdownWithError(...) will cleanup the stack as well so its only needed here.
@@ -1378,6 +1372,23 @@ private boolean needWrapAgain(int stackError) {
         return false;
     }
 
+    private SSLException newSSLExceptionForError(int stackError) {
+        String message = SSL.getErrorString(stackError);
+        return handshakeState == HandshakeState.FINISHED ?
+                new OpenSslException(message, stackError) : new OpenSslHandshakeException(message, stackError);
+    }
+
+    private static boolean shouldAddSuppressed(Throwable target, int errorCode) {
+        for (Throwable suppressed: ThrowableUtil.getSuppressed(target)) {
+            if (suppressed instanceof NativeSslException &&
+                    ((NativeSslException) suppressed).errorCode() == errorCode) {
+                /// An exception with this errorCode was already added before.
+                return false;
+            }
+        }
+        return true;
+    }
+
     private SSLEngineResult sslReadErrorResult(int error, int stackError, int bytesConsumed, int bytesProduced)
             throws SSLException {
         if (needWrapAgain(stackError)) {
@@ -2661,4 +2672,36 @@ public String toString() {
                     '}';
         }
     }
+
+    private interface NativeSslException {
+        int errorCode();
+    }
+
+    private static final class OpenSslException extends SSLException implements NativeSslException {
+        private final int errorCode;
+
+        OpenSslException(String reason, int errorCode) {
+            super(reason);
+            this.errorCode = errorCode;
+        }
+
+        @Override
+        public int errorCode() {
+            return errorCode;
+        }
+    }
+
+    private static final class OpenSslHandshakeException extends SSLHandshakeException implements NativeSslException {
+        private final int errorCode;
+
+        OpenSslHandshakeException(String reason, int errorCode) {
+            super(reason);
+            this.errorCode = errorCode;
+        }
+
+        @Override
+        public int errorCode() {
+            return errorCode;
+        }
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -37,5 +37,7 @@ public final class EmptyArrays {`
`37`	`37`	`public static final X509Certificate[] EMPTY_X509_CERTIFICATES = {};`
`38`	`38`	`public static final javax.security.cert.X509Certificate[] EMPTY_JAVAX_X509_CERTIFICATES = {};`
`39`	`39`
	`40`	`+ public static final Throwable[] EMPTY_THROWABLES = {};`
	`41`	`+`
`40`	`42`	`private EmptyArrays() { }`
`41`	`43`	`}`
Original file line number	Diff line number	Diff line change
`@@ -76,4 +76,12 @@ public static void addSuppressed(Throwable target, List<Throwable> suppressed) {`
`76`	`76`	`addSuppressed(target, t);`
`77`	`77`	`}`
`78`	`78`	`}`
	`79`	`+`
	`80`	`+ @SuppressJava6Requirement(reason = "Throwable getSuppressed is only available for >= 7. Has check for < 7.")`
	`81`	`+ public static Throwable[] getSuppressed(Throwable source) {`
	`82`	`+ if (!haveSuppressed()) {`
	`83`	`+ return EmptyArrays.EMPTY_THROWABLES;`
	`84`	`+ }`
	`85`	`+ return source.getSuppressed();`
	`86`	`+ }`
`79`	`87`	`}`