Merge pull request #363 from cconlon/scanIssues

Security and correctness fixes from internal scan

Author: rlm2002

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

@@ -145,7 +145,8 @@ private void initKeyManager(KeyManager[] in)
                 if (managers[i] instanceof X509KeyManager) {
                     km = (X509KeyManager)managers[i];
                     WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
-                        () -> "located X509KeyManager instance: " + km);
+                        () -> "located X509KeyManager instance: " +
+                        km.getClass().getName());
                     break;
                 }
             }
@@ -185,7 +186,8 @@ private void initTrustManager(TrustManager[] in)
                 if (managers[i] instanceof X509TrustManager) {
                     tm = (X509TrustManager)managers[i];
                     WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
-                        () -> "located X509TrustManager instance: " + tm);
+                        () -> "located X509TrustManager instance: " +
+                        tm.getClass().getName());
                     break;
                 }
             }
@@ -201,8 +203,9 @@ private void initSecureRandom(SecureRandom random) {
 
         if (random == null) {
             sr = new SecureRandom();
+        } else {
+            sr = random;
         }
-        sr = random;
     }
 
     /**
@@ -616,6 +619,7 @@ protected int addSession(WolfSSLImplementSSLSession session) {
 
         String toHash;
         final int hashCode;
+        final boolean haveKey;
 
         /* Don't store session if invalid (or not complete with sesPtr
          * if on client side, or not resumable). Server-side still needs to
@@ -639,6 +643,7 @@ protected int addSession(WolfSSLImplementSSLSession session) {
             toHash = session.getPeerHost().concat(Integer.toString(
                      session.getPeerPort()));
             hashCode = toHash.hashCode();
+            haveKey = true;
         }
         else {
             /* If no peer host is available then create hash key from
@@ -647,15 +652,18 @@ protected int addSession(WolfSSLImplementSSLSession session) {
             if (sessionId != null && sessionId.length > 0 &&
                 (idAllZeros(sessionId) == false)) {
                 hashCode = Arrays.toString(session.getId()).hashCode();
+                haveKey = true;
             } else {
                 hashCode = 0;
+                haveKey = false;
             }
         }
 
-        /* Always try to store session into cache table, as long as we
-         * have a hashCode. If session already exists for hashCode, it
-         * will be overwritten with new/refreshed version */
-        if (hashCode != 0) {
+        /* Only store session into cache if we have a usable key. If session
+         * already exists for hashCode, it will be overwritten with the new
+         * version. Note that hashCode == 0 is a legitimate hash value, so
+         * we use haveKey rather than hashCode == 0 to gate caching. */
+        if (haveKey) {
             WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
                 () -> "stored session in cache table (host: " +
                 session.getPeerHost() + ", port: " +
@@ -669,8 +677,12 @@ protected int addSession(WolfSSLImplementSSLSession session) {
             }
 
             printSessionStoreStatus();
+            return WolfSSL.SSL_SUCCESS;
         }
 
+        WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
+            () -> "Session not cached: no peer host and no usable session ID");
+
         return WolfSSL.SSL_SUCCESS;
     }
 
@@ -746,11 +758,13 @@ protected void updateTimeouts(int in, int side) {
                     diff = (now - current.creation.getTime()) / 1000;
 
                     if (diff < 0) {
-                    /* session is from the future ... */ /* TODO */
-
+                        /* Session creation time in the future. Invalidate so
+                         * it cannot persist indefinitely past the timeout. */
+                        WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
+                            () -> "Invalidating session with future creation");
+                        current.invalidate();
                     }
-
-                    if (in > 0 && diff >= in) {
+                    else if (in > 0 && diff >= in) {
                         current.invalidate();
                     }
                     try {

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

@@ -26,7 +26,6 @@
 import java.security.cert.CertificateEncodingException;
 import java.security.cert.X509Certificate;
 import javax.security.auth.x500.X500Principal;
-import java.util.Arrays;
 
 import javax.net.ssl.KeyManager;
 import javax.net.ssl.SSLContextSpi;
@@ -310,7 +309,7 @@ private void LoadTrustedRootCerts() {
         }
 
         WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
-            () -> "Using X509TrustManager: " + tm.toString());
+            () -> "Using X509TrustManager: " + tm.getClass().getName());
 
         /* We only need to load trusted CA certificates into our native
          * WOLFSSL_CTX if we are doing internal verification with wolfSSL
@@ -364,10 +363,12 @@ private void LoadTrustedRootCerts() {
                 /* skip loading if encoding error is encountered */
                 WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
                     () -> "skipped loading CA, encoding error");
+                continue;
             } catch (WolfSSLJNIException we) {
                 /* skip loading if wolfSSL fails to load der encoding */
                 WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
                     () -> "skipped loading CA, JNI exception");
+                continue;
             }
 
             final String sigAltName = caList[i].getSigAlgName();
@@ -385,6 +386,24 @@ private void LoadTrustedRootCerts() {
         }
     }
 
+    /* Format an array of objects as "[Class1, Class2, ...]" for debug
+     * logging without invoking caller-supplied toString() on user-provided
+     * KeyManager/TrustManager objects. */
+    private static String classNames(Object[] arr) {
+        if (arr == null) {
+            return "null";
+        }
+        StringBuilder sb = new StringBuilder("[");
+        for (int i = 0; i < arr.length; i++) {
+            if (i > 0) {
+                sb.append(", ");
+            }
+            sb.append(arr[i] == null ? "null" : arr[i].getClass().getName());
+        }
+        sb.append("]");
+        return sb.toString();
+    }
+
     /**
      * Initializes a SSLContext.
      *
@@ -406,8 +425,9 @@ protected void engineInit(KeyManager[] km, TrustManager[] tm,
         SecureRandom sr) throws KeyManagementException {
 
         WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
-            () -> "entered engineInit(km=" + Arrays.toString(km) + ", tm=" +
-            Arrays.toString(tm) + ", sr=" + sr +")");
+            () -> "entered engineInit(km=" + classNames(km) + ", tm=" +
+            classNames(tm) + ", sr=" +
+            (sr == null ? "null" : sr.getClass().getName()) + ")");
 
         try {
             authStore = new WolfSSLAuthStore(km, tm, sr, currentVersion,
@@ -620,7 +640,7 @@ protected void finalize() throws Throwable {
      * @return String array of enabled SSL/TLS protocols for this
      *         WolfSSLContext object
      */
-    public String[] getProtocolsMask(long noOpt) {
+    private String[] getProtocolsMask(long noOpt) {
         if (ctx != null) {
             ctx.setOptions(noOpt);
         }

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

@@ -134,6 +134,14 @@ public class WolfSSLEngine extends SSLEngine {
      * inside SendAppData, of size SSLSession.getApplicationBufferSize() */
     private ByteBuffer staticAppDataBuf = null;
 
+    /* Largest plaintext byte count written into staticAppDataBuf by the most
+     * recent SendAppData() call. Used to bound the post-write zeroing range
+     * so residual plaintext from a prior larger write is wiped even when the
+     * current write is smaller. Bytes past max(sendSz, lastSendSz) never need
+     * zeroing here: ByteBuffer.allocateDirect() zero-initializes the buffer,
+     * and SendAppData() is the only writer of plaintext into it. */
+    private int lastSendSz = 0;
+
     /* Stashed decrypted data when output buffer too small.
      * Served on next unwrap() without calling ssl_read(). */
     private byte[] pendingAppData = null;
@@ -780,14 +788,31 @@ private synchronized int SendAppData(ByteBuffer[] in, int ofst, int len)
         WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
             () -> "calling ssl.write() with size: " + tmpSendSz);
 
-        synchronized (ioLock) {
-            ret = this.ssl.write(dataArr, sendSz);
-        }
-        if (ret <= 0) {
-            /* error, reset in[] positions for next call */
-            for (i = ofst; i < ofst + len; i++) {
-                in[i].position(pos[i - ofst]);
+        try {
+            synchronized (ioLock) {
+                ret = this.ssl.write(dataArr, sendSz);
             }
+            if (ret <= 0) {
+                /* error, reset in[] positions for next call */
+                for (i = ofst; i < ofst + len; i++) {
+                    in[i].position(pos[i - ofst]);
+                }
+            }
+        } finally {
+            /* Zero plaintext from heap and direct buffers even on exception.
+             * Zero up to max(sendSz, lastSendSz) bytes so bytes from a prior
+             * larger write are wiped if needed. */
+            Arrays.fill(dataArr, (byte)0);
+            int zeroSz = Math.max(sendSz, this.lastSendSz);
+            /* clear() resets position/limit only, does not zero bytes */
+            this.staticAppDataBuf.clear();
+            /* Bulk put faster than a per-byte loop on DirectByteBuffer */
+            this.staticAppDataBuf.put(dataArr, 0, sendSz);
+            /* Loop wipes [sendSz, lastSendSz) when previous write was larger */
+            for (int j = sendSz; j < zeroSz; j++) {
+                this.staticAppDataBuf.put((byte)0);
+            }
+            this.lastSendSz = sendSz;
         }
 
         final int tmpRet = ret;
@@ -1077,17 +1102,19 @@ else if (produced == 0) {
      * @return number of available/remaining bytes in array of ByteBuffers
      * @throws IllegalArgumentException if readonly buffer found
      */
-    private static synchronized int getTotalOutputSize(ByteBuffer[] out,
-                                          int ofst, int length) {
+    private static int getTotalOutputSize(ByteBuffer[] out,
+        int ofst, int length) {
+
         int i = 0;
         int maxOutSz = 0;
 
         for (i = 0; i < length; i++) {
-            if (out[i + ofst] == null || out[i + ofst].isReadOnly()) {
+            ByteBuffer buf = out[i + ofst];
+            if (buf == null || buf.isReadOnly()) {
                 throw new IllegalArgumentException(
                     "null or readonly out buffer found");
             }
-            maxOutSz += out[i + ofst].remaining();
+            maxOutSz += buf.remaining();
         }
 
         return maxOutSz;
@@ -2301,7 +2328,9 @@ else if (!this.needInit && !this.handshakeFinished) {
             this.handshakeStartedExplicitly = true;
 
         } catch (SocketTimeoutException e) {
-            e.printStackTrace();
+            WolfSSLDebug.log(getClass(), WolfSSLDebug.ERROR,
+                () -> "doHandshake() timed out in beginHandshake(): " +
+                e.getMessage());
             throw new SSLException(e);
 
         } finally {
@@ -2813,11 +2842,16 @@ protected synchronized void finalize() throws Throwable {
             this.ssl.freeSSL();
             this.ssl = null;
         }
-        /* Clear our reference to static application direct ByteBuffer */
+        /* Zero only the byte range that held plaintext and drop our reference
+         * to the buffer. */
         if (this.staticAppDataBuf != null) {
             this.staticAppDataBuf.clear();
+            for (int j = 0; j < this.lastSendSz; j++) {
+                this.staticAppDataBuf.put((byte)0);
+            }
             this.staticAppDataBuf = null;
         }
+        this.lastSendSz = 0;
         super.finalize();
     }
 }

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

@@ -516,7 +516,7 @@ protected synchronized Exception getLastVerifyException() {
      *
      * @return String array of all supported cipher suites
      */
-    protected static synchronized String[] getAllCiphers() {
+    protected static String[] getAllCiphers() {
         return WolfSSLUtil.sanitizeSuites(WolfSSL.getCiphersIana(), false);
     }
 
@@ -638,7 +638,7 @@ protected synchronized String[] getProtocols() {
      *
      * @return String array of supported protocols
      */
-    protected static synchronized String[] getAllProtocols() {
+    protected static String[] getAllProtocols() {
         return WolfSSLUtil.sanitizeProtocols(
             WolfSSL.getProtocols(), WolfSSL.TLS_VERSION.INVALID);
     }
@@ -1696,7 +1696,7 @@ protected synchronized int doHandshake(int isSSLEngine, int timeout)
                 /* send CloseNotify */
                 /* TODO: SunJSSE sends a Handshake Failure alert instead here */
                 this.ssl.shutdownSSL();
-            } catch (SocketException e) {
+            } catch (SocketException | SocketTimeoutException e) {
                 throw new SSLException(e);
             }
 
@@ -1712,6 +1712,13 @@ protected synchronized int doHandshake(int isSSLEngine, int timeout)
                 WolfSSLDebug.log(getClass(), WolfSSLDebug.INFO,
                     () -> "session creation not allowed");
 
+                try {
+                    /* send CloseNotify so peer is not left hanging */
+                    this.ssl.shutdownSSL();
+                } catch (SocketException | SocketTimeoutException e) {
+                    throw new SSLException(e);
+                }
+
                 return WolfSSL.SSL_HANDSHAKE_FAILURE;
             }