Merge pull request #275 from cconlon/jniLockFixes

Fixes to avoid use-after-free issues in WolfSSLSocket

Author: JacobBarthelmeh

native/com_wolfssl_WolfSSLSession.c

@@ -1673,6 +1673,8 @@ JNIEXPORT void JNICALL Java_com_wolfssl_WolfSSLSession_freeSSL
     jobject* g_cachedVerifyCb;
     SSLAppData* appData;
     WOLFSSL* ssl = (WOLFSSL*)(uintptr_t)sslPtr;
+    wolfSSL_Mutex* sessLock = NULL;
+    byte lockAvailable = 1;
     (void)jcl;
 #if defined(HAVE_PK_CALLBACKS) && (defined(HAVE_ECC) || !defined(NO_RSA))
     internCtx* pkCtx = NULL;
@@ -1684,9 +1686,41 @@ JNIEXPORT void JNICALL Java_com_wolfssl_WolfSSLSession_freeSSL
         return;
     }
 
-    /* free session mutex lock */
+    /* Get and set WOLFSSL app data back to NULL. Try to lock on jniSessLock
+     * to prevent race conditions between multiple threads getting
+     * and using/freeing app data. */
     appData = (SSLAppData*)wolfSSL_get_app_data(ssl);
     if (appData != NULL) {
+        if (appData->jniSessLock != NULL) {
+            sessLock = appData->jniSessLock;
+            if (wc_LockMutex(sessLock) != 0) {
+                lockAvailable = 0;
+            }
+            /* Re-check getting appData after we have mutex */
+            appData = (SSLAppData*)wolfSSL_get_app_data(ssl);
+            if (appData != NULL) {
+                /* Set SSLAppData back to NULL inside WOLFSSL struct, if other
+                 * threads are trying to use it they will fail with error
+                 * instead of use after free faults. */
+                wolfSSL_set_app_data(ssl, NULL);
+            }
+            if (lockAvailable) {
+                /* Unlock on saved mutex, in case appData is now NULL */
+                wc_UnLockMutex(sessLock);
+                sessLock = NULL;
+            }
+        } else {
+            /* If no jniSessLock available, fall back to just
+             * setting app data to NULL in WOLFSSL. We should always have
+             * appData->jniSessLock though. */
+            wolfSSL_set_app_data(ssl, NULL);
+        }
+    }
+
+    /* appData may have been refreshed/NULL retrieved above inside
+     * mutex lock, check again against NULL before continuing freeing. */
+    if (appData != NULL) {
+        /* free session mutex lock */
         if (appData->jniSessLock != NULL) {
             wc_FreeMutex(appData->jniSessLock);
             XFREE(appData->jniSessLock, NULL, DYNAMIC_TYPE_TMP_BUFFER);

src/java/com/wolfssl/provider/jsse/WolfSSLSocket.java

@@ -1967,6 +1967,30 @@ public synchronized boolean sessionResumed() throws SSLException {
         return false;
     }
 
+    /**
+     * Internal private method to check if WolfSSLInputStream
+     * and WolfSSLOutputStream are closed.
+     *
+     * @return true if both streams are closed (or were not created/null),
+     * otherwise false if they were created and are still active.
+     */
+    private boolean ioStreamsAreClosed() {
+
+        if (this.inStream == null && this.outStream == null) {
+            return true;
+        }
+
+        if (this.inStream != null && !this.inStream.isClosed()) {
+            return false;
+        }
+
+        if (this.outStream != null && !this.outStream.isClosed()) {
+            return false;
+        }
+
+        return true;
+    }
+
     /**
      * Closes this SSLSocket.
      *
@@ -2132,13 +2156,22 @@ public synchronized void close() throws IOException {
 
                     /* Connection is closed, free native WOLFSSL session
                      * to release native memory earlier than garbage
-                     * collector might with finalize(), if we don't
-                     * have any threads still waiting in poll/select. */
-                    if (this.ssl.getThreadsBlockedInPoll() == 0) {
+                     * collector might with finalize(), Don't free if we
+                     * have threads still waiting in poll/select or if
+                     * our WolfSSLInputStream or WolfSSLOutputStream are
+                     * still open. */
+                    if (this.ssl.getThreadsBlockedInPoll() == 0 &&
+                        ioStreamsAreClosed()) {
                         WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
                             () -> "calling this.ssl.freeSSL()");
                         this.ssl.freeSSL();
                         this.ssl = null;
+                    } else {
+                        WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
+                            () -> "deferring freeing this.ssl, threads " +
+                            "blocked in poll: " +
+                            this.ssl.getThreadsBlockedInPoll() +
+                            ", or streams not closed");
                     }
 
                     /* Reset internal WolfSSLEngineHelper to null */
@@ -2517,7 +2550,7 @@ class WolfSSLInputStream extends InputStream {
 
         private WolfSSLSession ssl;
         private WolfSSLSocket  socket;
-        private boolean isClosed = true;
+        private volatile boolean isClosed = true;
 
         /* Atomic boolean to indicate if this InputStream has started to
          * close. Protects against deadlock between two threads calling
@@ -2530,6 +2563,18 @@ public WolfSSLInputStream(WolfSSLSession ssl, WolfSSLSocket socket) {
             this.isClosed = false;
         }
 
+        /**
+         * Non standard method to check if this InputStream has been
+         * closed. This is used by WolfSSLSocket to check if the associated
+         * WolfSSLInputStream has been closed before calling freeSSL()
+         * internally.
+         *
+         * @return true if this InputStream has been closed, otherwise false
+         */
+        public boolean isClosed() {
+            return this.isClosed;
+        }
+
         /**
          * Close InputStream, but gives caller option to close underlying
          * Socket or not.
@@ -2741,7 +2786,7 @@ class WolfSSLOutputStream extends OutputStream {
 
         private WolfSSLSession ssl;
         private WolfSSLSocket  socket;
-        private boolean isClosed = true;
+        private volatile boolean isClosed = true;
 
         /* Atomic boolean to indicate if this InputStream has started to
          * close. Protects against deadlock between two threads calling
@@ -2754,6 +2799,18 @@ public WolfSSLOutputStream(WolfSSLSession ssl, WolfSSLSocket socket) {
             this.isClosed = false;
         }
 
+        /**
+         * Non standard method to check if this OutputStream has been
+         * closed. This is used by WolfSSLSocket to check if the associated
+         * WolfSSLOutputStream has been closed before calling freeSSL()
+         * internally.
+         *
+         * @return true if this OutputStream has been closed, otherwise false
+         */
+        public boolean isClosed() {
+            return this.isClosed;
+        }
+
         /**
          * Close OutputStream, but gives caller option to close underlying
          * Socket or not.