JNI: always call wolfSSL_get1_session() inside native JNI getSession(), callers expect to always free returned pointer

Author: cconlon

.github/workflows/main.yml

@@ -101,6 +101,25 @@ jobs:
       jdk_version: ${{ matrix.jdk_version }}
       wolfssl_configure: ${{ matrix.wolfssl_configure }}
 
+  # -------------------- session cache variant sanity checks -----------------------
+  # Only check one Linux and Mac JDK version as a sanity check.
+  # Using Zulu, but this can be expanded if needed.
+  linux-zulu-sesscache:
+    strategy:
+      matrix:
+        os: [ 'ubuntu-latest', 'macos-latest' ]
+        jdk_version: [ '11' ]
+        wolfssl_configure: [
+            '--enable-jni CFLAGS=-DNO_SESSION_CACHE_REF',
+        ]
+    name: ${{ matrix.os }} (Zulu JDK ${{ matrix.jdk_version }}, ${{ matrix.wolfssl_configure}})
+    uses: ./.github/workflows/linux-common.yml
+    with:
+      os: ${{ matrix.os }}
+      jdk_distro: "zulu"
+      jdk_version: ${{ matrix.jdk_version }}
+      wolfssl_configure: ${{ matrix.wolfssl_configure }}
+
   # ------------------ Facebook Infer static analysis -------------------
   # Run Facebook infer over PR code, only running on Linux with one
   # JDK/version for now.

native/com_wolfssl_WolfSSLSession.c

@@ -1644,23 +1644,18 @@ JNIEXPORT jlong JNICALL Java_com_wolfssl_WolfSSLSession_get1Session
                 }
             }
         } while (err == SSL_ERROR_WANT_READ);
-
-        sess = wolfSSL_get_session(ssl);
-        hasTicket = wolfSSL_SESSION_has_ticket((const WOLFSSL_SESSION*)sess);
     }
 
-    /* Only duplicate / save session if not TLS 1.3 (will be using normal
-     * session IDs), or is TLS 1.3 and we have a session ticket */
-    if (version != TLS1_3_VERSION || hasTicket == 1) {
-
-        /* wolfSSL checks ssl for NULL, returns pointer to new WOLFSSL_SESSION,
-         * Returns new duplicated WOLFSSL_SESSION. Needs to be freed with
-         * wolfSSL_SESSION_free() when finished with pointer. */
-        if (sess != NULL) {
-            /* Guarantee that we own the WOLFSSL_SESSION, make a copy */
-            dup = wolfSSL_SESSION_dup(sess);
-        }
-    }
+    /* Call wolfSSL_get1_session() to increase the ref count of the internal
+     * WOLFSSL_SESSION struct. This is needed in all build option cases,
+     * since Java callers of this function expect to explicitly free this
+     * pointer when finished with use. In some build cases, for example
+     * NO_CLIENT_CACHE or NO_SESSION_CACHE_REF, the poiner returned by
+     * wolfSSL_get_session() will be a pointer into the WOLFSSL struct, which
+     * will be freed with wolfSSL_free(). This can cause issues if the Java
+     * app expects to hold a valid session pointer for resumption and free
+     * later on. */
+    dup = wolfSSL_get1_session(ssl);
 
     if (wc_UnLockMutex(jniSessLock) != 0) {
         printf("Failed to unlock jniSessLock in get1Session()");

src/java/com/wolfssl/WolfSSLSession.java

@@ -1358,15 +1358,25 @@ public int getError(int ret) throws IllegalStateException {
     }
 
     /**
-     * Sets the session to be used when the SSL object is used to create
-     * a SSL/TLS connection.
-     * For session resumption, before calling <code>shutdownSSL()</code>
-     * with your session object, an application should save the session ID
-     * from the object with a call to <code>getSession()</code>, which returns
-     * a pointer to the session. Later, the application should create a new
-     * SSL session object and assign the saved session with <code>
-     * setSession()</code>. At this point, the application may call <code>
-     * connect()</code> and wolfSSL will try to resume the session.
+     * Sets the session (native WOLFSSL_SESSION) to be used with this object
+     * for session resumption.
+     *
+     * The native WOLFSSL_SESSION pointed to contains all the necessary
+     * information required to perform a session resumption and reestablishment
+     * of the connection without a new handshake.
+     * <p>
+     * To do session resumption, before calling <code>shutdownSSL()</code>
+     * with your WolfSSLSession object, save the internal session state by
+     * calling <code>getSession()</code>, which returns a pointer to the
+     * native WOLFSSL_SESSION session structure. Later, when the application
+     * is ready to resume a session, it should create a new WolfSSLSession
+     * object and assign the previously-saved session pointer by passing it
+     * to the <code>setSession(long session)</code> method. This should be
+     * done before the handshake is started for the second/resumed time. After
+     * calling <code>setSession(long session)</code>, the application may call
+     * <code>connect()</code> and wolfSSL will try to resume the session. If
+     * the session cannot be resumed, a new fresh handshake will be
+     * established.
      *
      * @param session  pointer to the native WOLFSSL_SESSION structure used
      *                 to set the session for the SSL session object.
@@ -1411,25 +1421,35 @@ public int setSession(long session) throws IllegalStateException {
     }
 
     /**
-     * Returns a pointer to the current session used in the given SSL object.
+     * Returns a pointer to the current session (native WOLFSSL_SESSION)
+     * associated with this object, or null if not available.
+     *
      * The native WOLFSSL_SESSION pointed to contains all the necessary
      * information required to perform a session resumption and reestablishment
-     * the connection without a new handshake.
+     * of the connection without a new handshake.
+     * <p>
+     * To do session resumption, before calling <code>shutdownSSL()</code>
+     * with your WolfSSLSession object, save the internal session state by
+     * calling <code>getSession()</code>, which returns a pointer to the
+     * native WOLFSSL_SESSION session structure. Later, when the application
+     * is ready to resume a session, it should create a new WolfSSLSession
+     * object and assign the previously-saved session pointer by passing it
+     * to the <code>setSession(long session)</code> method. This should be
+     * done before the handshake is started for the second/resumed time. After
+     * calling <code>setSession(long session)</code>, the application may call
+     * <code>connect()</code> and wolfSSL will try to resume the session. If
+     * the session cannot be resumed, a new fresh handshake will be
+     * established.
+     * <p>
+     * <b>IMPORTANT:</b>
      * <p>
-     * For session resumption, before calling <code>shutdownSSL()</code>
-     * with your session object, an application should save the session ID
-     * from the object with a call to <code>getSession()</code>, which returns
-     * a pointer to the session. Later, the application should create a new
-     * SSL object and assign the saved session with <code>setSession</code>.
-     * At this point, the application may call <code>connect()</code> and
-     * wolfSSL will try to resume the session.
-     *
      * The pointer (WOLFSSL_SESSION) returned by this method needs to be freed
-     * when the application is finished with it, by calling
-     * <code>freeSession(long)</code>. This will release the underlying
-     * native memory associated with this WOLFSSL_SESSION.
+     * when the application is finished with it by calling
+     * <code>freeSession(long session)</code>. This will release the underlying
+     * native memory associated with this WOLFSSL_SESSION. Failing to free
+     * the session will result in a memory leak.
      *
-     * @throws IllegalStateException WolfSSLContext has been freed
+     * @throws IllegalStateException this WolfSSLSession has been freed
      * @return      a pointer to the current SSL session object on success.
      *              <code>null</code> if <b>ssl</b> is <code>null</code>,
      *              the SSL session cache is disabled, wolfSSL doesn't have
@@ -1446,6 +1466,12 @@ public long getSession() throws IllegalStateException {
             WolfSSLDebug.log(getClass(), WolfSSLDebug.Component.JNI,
                 WolfSSLDebug.INFO, this.sslPtr, "entered getSession()");
 
+            /* Calling get1Session() here as an indication that the native
+             * JNI level should always return a session pointer that needs
+             * to be freed by the application. This behavior can change in
+             * native wolfSSL depending on build options
+             * (ex: NO_SESSION_CACHE_REF), so JNI layer here will make that
+             * behavior consistent to the JNI/JSSE callers. */
             sessPtr = get1Session(this.sslPtr);
 
             WolfSSLDebug.log(getClass(), WolfSSLDebug.Component.JNI,