From 64584fd604e31b12d549e96edac182c76e9f2ad7 Mon Sep 17 00:00:00 2001 From: Chris Conlon Date: Wed, 27 Nov 2024 15:29:40 -0700 Subject: [PATCH] JNI: always call wolfSSL_get1_session() inside native JNI getSession(), callers expect to always free returned pointer --- .github/workflows/main.yml | 19 +++++++ native/com_wolfssl_WolfSSLSession.c | 25 ++++---- src/java/com/wolfssl/WolfSSLSession.java | 72 ++++++++++++++++-------- 3 files changed, 78 insertions(+), 38 deletions(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 90c900d3..64236be0 100644 --- a/.github/workflows/main.yml +++ b/.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. diff --git a/native/com_wolfssl_WolfSSLSession.c b/native/com_wolfssl_WolfSSLSession.c index a697ad3e..ed9d909e 100644 --- a/native/com_wolfssl_WolfSSLSession.c +++ b/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()"); diff --git a/src/java/com/wolfssl/WolfSSLSession.java b/src/java/com/wolfssl/WolfSSLSession.java index d8010c4a..fdeed8a9 100644 --- a/src/java/com/wolfssl/WolfSSLSession.java +++ b/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 shutdownSSL() - * with your session object, an application should save the session ID - * from the object with a call to getSession(), which returns - * a pointer to the session. Later, the application should create a new - * SSL session object and assign the saved session with - * setSession(). At this point, the application may call - * connect() 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. + *

+ * To do session resumption, before calling shutdownSSL() + * with your WolfSSLSession object, save the internal session state by + * calling getSession(), 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 setSession(long session) method. This should be + * done before the handshake is started for the second/resumed time. After + * calling setSession(long session), the application may call + * connect() 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. + *

+ * To do session resumption, before calling shutdownSSL() + * with your WolfSSLSession object, save the internal session state by + * calling getSession(), 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 setSession(long session) method. This should be + * done before the handshake is started for the second/resumed time. After + * calling setSession(long session), the application may call + * connect() and wolfSSL will try to resume the session. If + * the session cannot be resumed, a new fresh handshake will be + * established. + *

+ * IMPORTANT: *

- * For session resumption, before calling shutdownSSL() - * with your session object, an application should save the session ID - * from the object with a call to getSession(), which returns - * a pointer to the session. Later, the application should create a new - * SSL object and assign the saved session with setSession. - * At this point, the application may call connect() 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 - * freeSession(long). This will release the underlying - * native memory associated with this WOLFSSL_SESSION. + * when the application is finished with it by calling + * freeSession(long session). 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. * null if ssl is null, * 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,