Fix memory-safety, zeroization, and error-handling findings

apps/wolfsshd/auth.c

@@ -423,6 +423,7 @@ static int CheckPasswordHashUnix(const char* input, char* stored)
                         (const byte*)stored, storedSz) != 0) {
                 ret = WSSHD_AUTH_FAILURE;
             }
+            WS_FORCEZERO(hashedInput, hashedInputSz);
         }
     }
 

apps/wolfsshd/configuration.c

@@ -151,7 +151,7 @@ static long GetConfigInt(const char* in, int inSz, int isTime, void* heap)
             WMEMCPY(num, in, sz);
             num[sz] = '\0';
             ret = atol(num);
-            if (ret == 0 && WSTRCMP(in, "0") != 0) {
+            if (ret == 0 && WSTRCMP(num, "0") != 0) {
                 ret = WS_BAD_ARGUMENT;
             }
             else if (ret > 0) {

apps/wolfsshd/test/test_configuration.c

@@ -262,6 +262,10 @@ static int test_ParseConfigLine(void)
         {"Invalid login grace time", "LoginGraceTime wolfsshd", 1},
         {"Bare multiplier m (no digit)", "LoginGraceTime m", 1},
         {"Bare multiplier h (no digit)", "LoginGraceTime h", 1},
+        {"Valid login grace time zero", "LoginGraceTime 0", 0},
+        {"Valid login grace time zero minutes", "LoginGraceTime 0m", 0},
+        {"Valid login grace time zero hours", "LoginGraceTime 0h", 0},
+        {"Invalid zero padded login grace time", "LoginGraceTime 00", 1},
 
         /* Permit empty password tests. */
         {"Permit empty password no", "PermitEmptyPasswords no", 0},

examples/client/common.c

@@ -52,6 +52,7 @@
 
 static byte userPublicKeyBuf[512];
 static byte* userPublicKey = userPublicKeyBuf;
+static byte userPublicKeyAlloc = 0;
 static const byte* userPublicKeyType = NULL;
 static byte userPassword[256];
 static const byte* userPrivateKeyType = NULL;
@@ -284,7 +285,7 @@ static int load_der_file(const char* filename, byte** out, word32* outSz,
     }
     WREWIND(NULL, file);
 
-    in = (byte*)WMALLOC(inSz, heap, 0);
+    in = (byte*)WMALLOC(inSz, heap, DYNTYPE_PRIVKEY);
     if (in == NULL) {
         WFCLOSE(NULL, file);
         return -1;
@@ -293,7 +294,7 @@ static int load_der_file(const char* filename, byte** out, word32* outSz,
     ret = (int)WFREAD(NULL, in, 1, inSz, file);
     if (ret <= 0 || ret != inSz) {
         ret = -1;
-        WFREE(in, heap, 0);
+        WFREE(in, heap, DYNTYPE_PRIVKEY);
         in = 0;
         inSz = 0;
     }
@@ -757,6 +758,15 @@ int ClientUseCert(const char* certName, void* heap)
             userPublicKeyType = publicKeyType;
             userPublicKeyTypeSz = (word32)WSTRLEN((const char*)publicKeyType);
             pubKeyLoaded = 1;
+            userPublicKeyAlloc = 1;
+        }
+        else {
+            /* Defensive: load_der_file() clears its output pointer on the
+             * short-read failure, so put the static buffer back. */
+            userPublicKey = userPublicKeyBuf;
+            userPublicKeySz = 0;
+            userPublicKeyType = NULL;
+            userPublicKeyAlloc = 0;
         }
     #else
         (void)heap;
@@ -860,7 +870,7 @@ static int readKeyBlob(const char* filename, WOLFTPM2_KEYBLOB* key)
 }
 
 static int wolfSSH_TPM_InitKey(WOLFTPM2_DEV* dev, const char* name,
-    WOLFTPM2_KEY* pTpmKey, const char* tpmKeyAuth)
+    WOLFTPM2_KEY* pTpmKey, const char* tpmKeyAuth, void* heap)
 {
     int rc = 0;
     WOLFTPM2_KEY endorse;
@@ -943,11 +953,14 @@ static int wolfSSH_TPM_InitKey(WOLFTPM2_DEV* dev, const char* name,
 
     /* Read public key from buffer and convert key to OpenSSH format */
     if (rc == 0) {
+        /* Allocate from the caller's heap, ClientFreeBuffers() frees it
+         * with the same one. */
         rc = wolfSSH_ReadPublicKey_buffer(userPublicKey, userPublicKeySz,
             WOLFSSH_FORMAT_ASN1, &p, &userPublicKeySz, &userPublicKeyType,
-            &userPublicKeyTypeSz, NULL);
+            &userPublicKeyTypeSz, heap);
         if (rc == 0) {
             userPublicKey = p;
+            userPublicKeyAlloc = 1;
         } else {
             WLOG(WS_LOG_DEBUG, "Reading public key failed, rc: %d", rc);
         }
@@ -1030,7 +1043,8 @@ int ClientSetPrivateKey(const char* privKeyName, int userEcc,
          */
         WMEMSET(&tpmDev, 0, sizeof(tpmDev));
         WMEMSET(&tpmKey, 0, sizeof(tpmKey));
-        ret = wolfSSH_TPM_InitKey(&tpmDev, privKeyName, &tpmKey, tpmKeyAuth);
+        ret = wolfSSH_TPM_InitKey(&tpmDev, privKeyName, &tpmKey, tpmKeyAuth,
+                heap);
     #elif !defined(NO_FILESYSTEM)
         userPrivateKey = NULL; /* create new buffer based on parsed input */
         userPrivateKeyAlloc = 1;
@@ -1089,6 +1103,13 @@ int ClientUsePubKey(const char* pubKeyName, int userEcc, void* heap)
     #endif /* NO_FILESYSTEM */
         if (ret == 0) {
             pubKeyLoaded = 1;
+            userPublicKeyAlloc = 1;
+        }
+        else {
+            userPublicKey = userPublicKeyBuf;
+            userPublicKeySz = 0;
+            userPublicKeyType = NULL;
+            userPublicKeyAlloc = 0;
         }
     }
 
@@ -1112,7 +1133,7 @@ int ClientLoadCA(WOLFSSH_CTX* ctx, const char* caCert)
                 fprintf(stderr, "Couldn't parse in CA certificate.");
                 ret = WS_PARSE_E;
             }
-            WFREE(der, NULL, 0);
+            WFREE(der, ctx->heap, DYNTYPE_PRIVKEY);
         }
     #else
         (void)ctx;
@@ -1132,11 +1153,16 @@ void ClientFreeBuffers(const char* pubKeyName, const char* privKeyName,
 #ifdef WOLFSSH_TPM
     wolfSSH_TPM_Cleanup(&tpmDev, &tpmKey);
 #endif
-    if (pubKeyName != NULL && userPublicKey != NULL &&
-        userPublicKey != userPublicKeyBuf) {
+    /* The public key buffer is tracked by userPublicKeyAlloc, not by
+     * pubKeyName: ClientUseCert() allocates one without a public key file
+     * name being given. */
+    (void)pubKeyName;
+
+    if (userPublicKeyAlloc && userPublicKey != NULL) {
         WFREE(userPublicKey, heap, DYNTYPE_PRIVKEY);
         userPublicKey = userPublicKeyBuf;
         userPublicKeySz = 0;
+        userPublicKeyAlloc = 0;
     }
 
     if (privKeyName != NULL && userPrivateKey != NULL) {

examples/echoserver/echoserver.c

@@ -2352,41 +2352,68 @@ static int LoadPubKeyList(StrList* strList, int format, PwMapList* mapList)
             fileName++;
 
             load_file(fileName, NULL, &bufSz);
-            buf = (byte*)WMALLOC(bufSz, NULL, 0);
-            bufSz = load_file(fileName, buf, &bufSz);
-            if (bufSz > 0) {
+            if (bufSz == 0) {
+                fprintf(stderr, "File error: %s\n", fileName);
+            }
+            else if ((buf = (byte*)WMALLOC(bufSz, NULL, 0)) == NULL) {
+                fprintf(stderr, "Memory error: %s\n", fileName);
+            }
+            else if ((bufSz = load_file(fileName, buf, &bufSz)) == 0) {
+                fprintf(stderr, "File error: %s\n", fileName);
+            }
+            else {
+                byte* out = NULL;
+                word32 outSz = 0;
+                int ok = 1;
+
                 if (format == WOLFSSH_FORMAT_SSH) {
                     const byte* type = NULL;
-                    byte* out = NULL;
-                    word32 typeSz, outSz;
+                    word32 typeSz = 0;
 
-                    wolfSSH_ReadKey_buffer(buf, bufSz, WOLFSSH_FORMAT_SSH,
-                            &out, &outSz, &type, &typeSz, NULL);
+                    if (wolfSSH_ReadKey_buffer(buf, bufSz, WOLFSSH_FORMAT_SSH,
+                                &out, &outSz, &type, &typeSz, NULL)
+                            != WS_SUCCESS || out == NULL) {
+                        fprintf(stderr, "ReadKey error: %s\n", fileName);
+                        ok = 0;
+                    }
 
                     (void)type;
                     (void)typeSz;
-
-                    WFREE(buf, NULL, 0);
-                    buf = out;
-                    bufSz = outSz;
                 }
                 else if (format == WOLFSSH_FORMAT_PEM) {
-                    byte* out = NULL;
-                    word32 outSz;
-
                     out = (byte*)WMALLOC(bufSz, NULL, 0);
-                    outSz = wc_CertPemToDer(buf, bufSz, out, bufSz, CERT_TYPE);
+                    if (out == NULL) {
+                        fprintf(stderr, "Memory error: %s\n", fileName);
+                        ok = 0;
+                    }
+                    else {
+                        int rc = wc_CertPemToDer(buf, bufSz, out, bufSz,
+                                CERT_TYPE);
 
-                    WFREE(buf, NULL, 0);
-                    buf = out;
-                    bufSz = outSz;
+                        if (rc <= 0) {
+                            fprintf(stderr, "PEM error: %s\n", fileName);
+                            ok = 0;
+                        }
+                        else {
+                            outSz = (word32)rc;
+                        }
+                    }
                 }
 
-                PwMapNew(mapList, WOLFSSH_USERAUTH_PUBLICKEY,
-                        (byte*)names, (word32)WSTRLEN(names), buf, bufSz);
-            }
-            else {
-                fprintf(stderr, "File error: %s\n", names);
+                if (ok) {
+                    /* Converted key replaces the raw file contents. */
+                    if (out != NULL) {
+                        WFREE(buf, NULL, 0);
+                        buf = out;
+                        bufSz = outSz;
+                        out = NULL;
+                    }
+
+                    PwMapNew(mapList, WOLFSSH_USERAUTH_PUBLICKEY,
+                            (byte*)names, (word32)WSTRLEN(names), buf, bufSz);
+                }
+
+                WFREE(out, NULL, 0);
             }
         }
         else {

examples/portfwd/portfwd.c

@@ -690,25 +690,34 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args)
         rxFds = templateFds;
         errFds = templateFds;
 
+        /* A readable appFd with a full buffer would spin select(). Mask the
+         * read set only, so errors are still noticed. */
+        if (appFdSet && appBufferUsed == appBufferSz)
+            FD_CLR(appFd, &rxFds);
+
         to.tv_sec = 1;
         to.tv_usec = 0;
         ret = select(nFds, &rxFds, NULL, &errFds, &to);
+        if (ret < 0)
+            err_sys("select failed\n");
         if (ret == 0) {
+            /* Fall through rather than continue, a short send leaves a tail
+             * owed another attempt. select() cleared the sets. */
             ret = wolfSSH_SendIgnore(ssh, NULL, 0);
             if (ret != WS_SUCCESS)
                 err_sys("Couldn't send an ignore message.");
-            continue;
         }
-        else if (ret < 0)
-            err_sys("select failed\n");
 
         if ((appFdSet && FD_ISSET(appFd, &errFds)) ||
             FD_ISSET(sshFd, &errFds) ||
             (!reverse && FD_ISSET(listenFd, &errFds))) {
 
                 err_sys("some socket had an error");
             }
-        if (appFdSet && FD_ISSET(appFd, &rxFds)) {
+        /* Skip the read when the buffer is full, a zero length recv() returns
+         * 0 and would be mistaken for the peer closing. */
+        if (appFdSet && appBufferUsed < appBufferSz &&
+                FD_ISSET(appFd, &rxFds)) {
             int rxd;
             rxd = (int)recv(appFd,
                     appBuffer + appBufferUsed, appBufferSz - appBufferUsed, 0);
@@ -789,8 +798,12 @@ THREAD_RETURN WOLFSSH_THREAD portfwd_worker(void* args)
         }
         if (appBufferUsed > 0) {
             ret = wolfSSH_ChannelSend(fwdChannel, appBuffer, appBufferUsed);
-            if (ret > 0)
+            if (ret > 0) {
+                if ((word32)ret != appBufferUsed) {
+                    WMEMMOVE(appBuffer, appBuffer + ret, appBufferUsed - ret);
+                }
                 appBufferUsed -= ret;
+            }
             else if (ret == WS_CHANNEL_NOT_CONF || ret == WS_CHAN_RXD) {
             #ifdef SHELL_DEBUG
                 printf("Waiting for channel open confirmation.\n");

src/agent.c

@@ -1701,6 +1701,11 @@ int wolfSSH_AGENT_worker(WOLFSSH* ssh)
     if (ssh == NULL)
         ret = WS_SSH_NULL_E;
 
+    if (ret == WS_SUCCESS) {
+        if (ssh->agent == NULL)
+            ret = WS_AGENT_NULL_E;
+    }
+
     if (ret == WS_SUCCESS) {
         SendSuccess(NULL);
         DoMessage(NULL, NULL, 0, NULL);

src/certman.c

@@ -93,6 +93,7 @@ static WOLFSSH_CERTMAN* _CertMan_init(WOLFSSH_CERTMAN* cm, void* heap)
     ret = cm;
     if (ret != NULL) {
         WMEMSET(ret, 0, sizeof(WOLFSSH_CERTMAN));
+        ret->heap = heap;
         ret->cm = wolfSSL_CertManagerNew_ex(heap);
         if (ret->cm == NULL) {
             ret = NULL;
@@ -507,20 +508,24 @@ static int CheckProfile(DecodedCert* cert, int profile)
         const byte* date;
         int         dateSz;
         byte        dateFormat;
-        struct tm t;
+        struct tm t = { 0 };
 
         dateFormat = cert->afterDate[0]; /* i.e ASN_UTC_TIME */
         dateSz     = cert->afterDate[1];
         date       = &cert->afterDate[2];
 
-        wc_GetDateAsCalendarTime(date, dateSz, dateFormat, &t);
-        if (t.tm_year <= 149 && dateFormat != ASN_UTC_TIME) {
+        if (wc_GetDateAsCalendarTime(date, dateSz, dateFormat, &t) != 0) {
+            WLOG(WS_LOG_CERTMAN, "unable to get date");
+            valid = 0;
+        }
+
+        if (valid && t.tm_year <= 149 && dateFormat != ASN_UTC_TIME) {
             WLOG(WS_LOG_CERTMAN, "date format was not utc for year %d",
             t.tm_year);
             valid = 0;
         }
 
-        if (t.tm_year > 149 && dateFormat != ASN_GENERALIZED_TIME) {
+        if (valid && t.tm_year > 149 && dateFormat != ASN_GENERALIZED_TIME) {
             WLOG(WS_LOG_CERTMAN, "date format was not general for year %d",
             t.tm_year);
             valid = 0;

src/internal.c

@@ -1650,7 +1650,9 @@ void SshResourceFree(WOLFSSH* ssh, void* heap)
 
     ShrinkBuffer(&ssh->inputBuffer, 1);
     ShrinkBuffer(&ssh->outputBuffer, 1);
-    WS_FORCEZERO(ssh->k, ssh->kSz);
+    /* Use the sizeof k, as kSz can change. */
+    WS_FORCEZERO(ssh->k, sizeof(ssh->k));
+    ssh->kSz = 0;
     HandshakeInfoFree(ssh->handshake, heap);
     WS_FORCEZERO(&ssh->keys, sizeof(Keys));
     WS_FORCEZERO(&ssh->peerKeys, sizeof(Keys));
@@ -11985,14 +11987,20 @@ static INLINE int VerifyMac(WOLFSSH* ssh, const byte* in, word32 inSz,
 
 
 #ifndef WOLFSSH_NO_AEAD
+/* Increment the explicit portion of the AEAD nonce. The carry runs the full
+ * width with no early exit, so the timing doesn't depend on the counter,
+ * which starts out as KDF output. */
 static INLINE void AeadIncrementExpIv(byte* iv)
 {
     int i;
+    unsigned int carry = 1;
 
     iv += AEAD_IMP_IV_SZ;
 
     for (i = AEAD_EXP_IV_SZ-1; i >= 0; i--) {
-        if (++iv[i]) return;
+        unsigned int sum = (unsigned int)iv[i] + carry;
+        iv[i] = (byte)sum;
+        carry = (sum >> 8) & 1u;
     }
 }