diff --git a/apps/wolfsshd/auth.c b/apps/wolfsshd/auth.c index eadb0a027..fa515afcd 100644 --- a/apps/wolfsshd/auth.c +++ b/apps/wolfsshd/auth.c @@ -354,7 +354,7 @@ static int CheckAuthKeysLine(char* line, word32 lineSz, const byte* key, /* Constant-time compare to avoid leaking which prefix bytes of an * authorized key match a candidate offered by a remote peer. */ if (keyCandSz != keySz || - ConstantCompare(key, keyCand, keySz) != 0) { + wolfSSH_ConstantCompare(key, keyCand, keySz) != 0) { ret = WSSHD_AUTH_FAILURE; } } @@ -811,7 +811,7 @@ static int CheckPasswordHashUnix(const char* input, const char* stored) if (hashedInputSz == 0 || hashedInput[0] == '*' || hashedInputSz != storedSz || - ConstantCompare((const byte*)hashedInput, + wolfSSH_ConstantCompare((const byte*)hashedInput, (const byte*)stored, storedSz) != 0) { ret = WSSHD_AUTH_FAILURE; } diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index ca45977b0..dd3a90e06 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -2772,7 +2772,7 @@ static PwMap* PwMapNew(PwMapList* list, byte type, const byte* username, map->type = type; if (usernameSz >= sizeof(map->username)) usernameSz = sizeof(map->username) - 1; - WMEMCPY(map->username, username, usernameSz + 1); + WMEMCPY(map->username, username, usernameSz); map->username[usernameSz] = 0; map->usernameSz = usernameSz; @@ -3400,13 +3400,14 @@ static int wsUserAuthResult(byte res, static int userAuthWouldBlock = 0; -static int wsUserAuth(byte authType, - WS_UserAuthData* authData, - void* ctx) +/* authHash is WC_SHA256_DIGEST_SIZE bytes of scratch for the digest of the + * offered secret; the caller wipes it. */ +static int wsUserAuthCheck(byte authType, + WS_UserAuthData* authData, + void* ctx, byte* authHash) { PwMapList* list; PwMap* map; - byte authHash[WC_SHA256_DIGEST_SIZE] = {0}; int userFound = 0; if (ctx == NULL) { @@ -3503,6 +3504,8 @@ static int wsUserAuth(byte authType, WFREE(uuid, NULL, 0); } + wc_FreeDecodedCert(&cert); + /* failed to at least get UUID string */ if (ret != 0) { return WOLFSSH_USERAUTH_INVALID_PUBLICKEY; @@ -3529,14 +3532,16 @@ static int wsUserAuth(byte authType, if (authData->type == WOLFSSH_USERAUTH_PUBLICKEY) { userFound = 1; - if (WMEMCMP(map->p, authHash, WC_SHA256_DIGEST_SIZE) == 0) { + if (wolfSSH_ConstantCompare(map->p, authHash, + WC_SHA256_DIGEST_SIZE) == 0) { return WOLFSSH_USERAUTH_SUCCESS; } /* Hash mismatch: continue checking other registered keys * for this user (a user may have multiple public keys). */ } else if (authData->type == WOLFSSH_USERAUTH_PASSWORD) { - if (WMEMCMP(map->p, authHash, WC_SHA256_DIGEST_SIZE) == 0) { + if (wolfSSH_ConstantCompare(map->p, authHash, + WC_SHA256_DIGEST_SIZE) == 0) { return WOLFSSH_USERAUTH_SUCCESS; } else { @@ -3556,7 +3561,8 @@ static int wsUserAuth(byte authType, } /* do keyboard auth prompts */ - if (WMEMCMP(map->p, authHash, WC_SHA256_DIGEST_SIZE) == 0) { + if (wolfSSH_ConstantCompare(map->p, authHash, + WC_SHA256_DIGEST_SIZE) == 0) { return WOLFSSH_USERAUTH_SUCCESS; } else { @@ -3582,6 +3588,20 @@ static int wsUserAuth(byte authType, } +static int wsUserAuth(byte authType, + WS_UserAuthData* authData, + void* ctx) +{ + byte authHash[WC_SHA256_DIGEST_SIZE] = {0}; + int ret; + + ret = wsUserAuthCheck(authType, authData, ctx, authHash); + wc_ForceZero(authHash, sizeof(authHash)); + + return ret; +} + + #ifdef WOLFSSH_SFTP /* * Sets the WOLFSSH object's default SFTP path to the value provided by diff --git a/examples/tpmcertserver/tpmcertserver.c b/examples/tpmcertserver/tpmcertserver.c index ae78ea2be..2a3c13968 100644 --- a/examples/tpmcertserver/tpmcertserver.c +++ b/examples/tpmcertserver/tpmcertserver.c @@ -64,11 +64,12 @@ static int TpmCsUserAuth(byte authType, WS_UserAuthData* authData, void* ctx) WOLFSSH_UNUSED(ctx); if (authType == WOLFSSH_USERAUTH_PASSWORD) { - const char* pw = (const char*)authData->sf.password.password; + const byte* pw = authData->sf.password.password; word32 pwSz = authData->sf.password.passwordSz; if (pwSz == (word32)XSTRLEN(TPMCS_PASSWORD) - && XMEMCMP(pw, TPMCS_PASSWORD, pwSz) == 0) { + && wolfSSH_ConstantCompare(pw, + (const byte*)TPMCS_PASSWORD, pwSz) == 0) { ret = WOLFSSH_USERAUTH_SUCCESS; } } diff --git a/src/internal.c b/src/internal.c index 20e13e225..4d524a080 100644 --- a/src/internal.c +++ b/src/internal.c @@ -14865,7 +14865,7 @@ static INLINE int VerifyMac(WOLFSSH* ssh, const byte* in, word32 inSz, ret = wc_HmacUpdate(&hmac, in, inSz); if (ret == WS_SUCCESS) ret = wc_HmacFinal(&hmac, checkMac); - if (ret == WS_SUCCESS && ConstantCompare(checkMac, mac, ssh->peerMacSz) != 0) + if (ret == WS_SUCCESS && WS_CONSTANTCOMPARE(checkMac, mac, ssh->peerMacSz) != 0) ret = WS_VERIFY_MAC_E; break; #endif @@ -14880,7 +14880,7 @@ static INLINE int VerifyMac(WOLFSSH* ssh, const byte* in, word32 inSz, ret = wc_HmacUpdate(&hmac, in, inSz); if (ret == WS_SUCCESS) ret = wc_HmacFinal(&hmac, checkMac); - if (ret == WS_SUCCESS && ConstantCompare(checkMac, mac, ssh->peerMacSz) != 0) + if (ret == WS_SUCCESS && WS_CONSTANTCOMPARE(checkMac, mac, ssh->peerMacSz) != 0) ret = WS_VERIFY_MAC_E; break; #endif @@ -14895,7 +14895,7 @@ static INLINE int VerifyMac(WOLFSSH* ssh, const byte* in, word32 inSz, ret = wc_HmacUpdate(&hmac, in, inSz); if (ret == WS_SUCCESS) ret = wc_HmacFinal(&hmac, checkMac); - if (ret == WS_SUCCESS && ConstantCompare(checkMac, mac, ssh->peerMacSz) != 0) + if (ret == WS_SUCCESS && WS_CONSTANTCOMPARE(checkMac, mac, ssh->peerMacSz) != 0) ret = WS_VERIFY_MAC_E; break; #endif @@ -17196,7 +17196,7 @@ int wolfSSH_RsaVerify(const byte *sig, word32 sigSz, RSA_PUBLIC_ENCRYPT, key, NULL); if (rsaRet != 0 || checkBlockSz != keySz - || ConstantCompare(checkBlock, encBlock, keySz) != 0) { + || WS_CONSTANTCOMPARE(checkBlock, encBlock, keySz) != 0) { WLOG(WS_LOG_DEBUG, "%s: %s", loc, "Bad RSA Verify"); ret = WS_RSA_E; } diff --git a/src/misc.c b/src/misc.c index c22bc929e..c33004608 100644 --- a/src/misc.c +++ b/src/misc.c @@ -109,12 +109,14 @@ STATIC INLINE void wolfSSH_ForceZero(void* mem, size_t len) #endif +#ifdef WOLFSSH_NO_CONSTCOMPARE /* check all length bytes for equality, return 0 on success */ STATIC INLINE int ConstantCompare(const byte* a, const byte* b, word32 length) { word32 i; - word32 compareSum = 0; + /* volatile keeps the compiler from turning the loop into an early exit */ + volatile word32 compareSum = 0; for (i = 0; i < length; i++) { compareSum |= a[i] ^ b[i]; @@ -122,6 +124,7 @@ STATIC INLINE int ConstantCompare(const byte* a, const byte* b, return compareSum; } +#endif #undef STATIC diff --git a/src/ssh.c b/src/ssh.c index 167bc3719..712b9cc6a 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -56,6 +56,7 @@ #include #include #include +#include #ifdef WOLFSSH_WINDOWS_CERT_STORE #include @@ -4610,6 +4611,15 @@ void wolfSSH_GetStats(WOLFSSH* ssh, word32* txCount, word32* rxCount, } +int wolfSSH_ConstantCompare(const byte* a, const byte* b, word32 len) +{ + if (a == NULL || b == NULL || len > (word32)INT_MAX) + return WS_BAD_ARGUMENT; + + return WS_CONSTANTCOMPARE(a, b, len); +} + + int wolfSSH_KDF(byte hashId, byte keyId, byte* key, word32 keySz, const byte* k, word32 kSz, diff --git a/tests/api.c b/tests/api.c index 97b926d0c..f69ce5082 100644 --- a/tests/api.c +++ b/tests/api.c @@ -32,6 +32,7 @@ #include #include +#include #if ((defined(WOLFSSH_SFTP) && !defined(NO_WOLFSSH_CLIENT)) || \ (defined(WOLFSSH_SCP) && defined(WOLFSSH_HAVE_SYMLINK) && \ !defined(WOLFSSH_SCP_USER_CALLBACKS))) && \ @@ -8034,6 +8035,26 @@ static void test_wolfSSH_ConvertConsole(void) } +static void test_wolfSSH_ConstantCompare(void) +{ + const byte a[] = { 0x01, 0x02, 0x03, 0x04 }; + const byte b[] = { 0x01, 0x02, 0x03, 0x04 }; + const byte first[] = { 0x81, 0x02, 0x03, 0x04 }; + const byte last[] = { 0x01, 0x02, 0x03, 0x05 }; + + AssertIntEQ(0, wolfSSH_ConstantCompare(a, b, sizeof(a))); + AssertIntNE(0, wolfSSH_ConstantCompare(a, first, sizeof(a))); + AssertIntNE(0, wolfSSH_ConstantCompare(a, last, sizeof(a))); + /* Only len bytes take part. */ + AssertIntEQ(0, wolfSSH_ConstantCompare(a, last, sizeof(a) - 1)); + AssertIntEQ(0, wolfSSH_ConstantCompare(a, first, 0)); + AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_ConstantCompare(NULL, b, sizeof(b))); + AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_ConstantCompare(a, NULL, sizeof(a))); + AssertIntEQ(WS_BAD_ARGUMENT, + wolfSSH_ConstantCompare(a, b, (word32)INT_MAX + 1)); +} + + static void test_wstrcat(void) { #ifndef WSTRING_USER @@ -9286,6 +9307,7 @@ int wolfSSH_ApiTest(int argc, char** argv) #endif /* HAVE_FIPS */ test_wstrcat(); + test_wolfSSH_ConstantCompare(); test_wolfSSH_Log_sanitize(); test_wolfSSH_CTX_new(); test_server_wolfSSH_new(); diff --git a/wolfssh/misc.h b/wolfssh/misc.h index 3ec47b9dd..5b0f17d70 100644 --- a/wolfssh/misc.h +++ b/wolfssh/misc.h @@ -36,6 +36,7 @@ #define WOLFSSL_V5_8_4 0x05008004 +#define WOLFSSL_V5_9_4 0x05009004 #if (LIBWOLFSSL_VERSION_HEX < WOLFSSL_V5_8_4) || \ defined(WOLFSSL_NO_FORCE_ZERO) @@ -48,6 +49,18 @@ #define WS_FORCEZERO(mem, len) wc_ForceZero((mem), (len)) #endif +#if (LIBWOLFSSL_VERSION_HEX < WOLFSSL_V5_9_4) || \ + defined(WOLFSSL_NO_CONST_CMP) + #define WOLFSSH_NO_CONSTCOMPARE +#endif + +#ifdef WOLFSSH_NO_CONSTCOMPARE + #define WS_CONSTANTCOMPARE(a, b, len) ConstantCompare((a), (b), (len)) +#else + #define WS_CONSTANTCOMPARE(a, b, len) \ + wc_ConstantCompare((a), (b), (int)(len)) +#endif + #ifdef NO_INLINE @@ -58,7 +71,9 @@ WOLFSSH_LOCAL word32 min(word32 a, word32 b); WOLFSSH_LOCAL void ato32(const byte* c, word32* u32); WOLFSSH_LOCAL void c32toa(word32 u32, byte* c); +#ifdef WOLFSSH_NO_CONSTCOMPARE WOLFSSH_LOCAL int ConstantCompare(const byte* a, const byte* b, word32 length); +#endif #ifdef WOLFSSH_NO_FORCEZERO WOLFSSH_LOCAL void wolfSSH_ForceZero(void* mem, size_t len); #endif diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 57a4e253e..039c4297b 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -764,7 +764,9 @@ typedef struct WS_UserAuthData { * For WOLFSSH_USERAUTH_PUBLICKEY, the callback must check the offered * public key against the user's authorized keys; returning success on an * unchecked key authorizes an attacker-supplied key. The library verifies - * the signature, not the key's authorization. */ + * the signature, not the key's authorization. + * + * Compare secrets with wolfSSH_ConstantCompare(), not memcmp(). */ typedef int (*WS_CallbackUserAuth)(byte authType, WS_UserAuthData* authData, void* ctx); WOLFSSH_API void wolfSSH_SetUserAuth(WOLFSSH_CTX* ctx, WS_CallbackUserAuth cb); @@ -782,6 +784,15 @@ WOLFSSH_API void wolfSSH_SetUserAuthResultCtx(WOLFSSH* ssh, void* userAuthResultCtx); WOLFSSH_API void* wolfSSH_GetUserAuthResultCtx(WOLFSSH* ssh); +/* Compares len bytes of a and b in time that depends only on len, using + * wolfCrypt's wc_ConstantCompare() where the wolfSSL build provides it. + * Returns 0 when they match, nonzero when they differ, and WS_BAD_ARGUMENT + * for a NULL a or b or a len over INT_MAX. Intended for userauth callbacks + * comparing passwords, responses or digests; check lengths separately, as + * the compare cannot hide them. */ +WOLFSSH_API int wolfSSH_ConstantCompare(const byte* a, const byte* b, + word32 len); + /* Public Key Check Callback * * A client-side callback that decides whether to trust the server's host