From 979801a05dd0d1b4f9bc5da022b80735ac77a335 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 30 Nov 2023 13:12:33 -0800 Subject: [PATCH 1/2] Known Hosts Update 1. Move setting a nul termination on the knownHosts data until after checking the size is reasonable. 2. A temporary keySz variable was getting used to get the length of the key type value, but it wasn't used to copy the value. Deleted it and used the other sz value. 3. Fix the leaking of the known hosts filename. --- apps/wolfssh/common.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index 881deb29..28376cf7 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -283,11 +283,6 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) sz = 0; ret = load_der_file(knownHostsName, (byte**)&knownHosts, &sz); - /* load_der_file() loads exactly what's in the file. Since it is - * NL terminated lines of known host data, and the last line ends - * in a NL, overwrite that with a nul to terminate the new string. */ - knownHosts[sz - 1] = 0; - if (ret == 0) { if (sz < sizeof(word32)) { /* This file is too small. There must be at least a word32 @@ -297,6 +292,11 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) } if (ret == 0) { + /* load_der_file() loads exactly what's in the file. Since it is + * NL terminated lines of known host data, and the last line ends + * in a NL, overwrite that with a nul to terminate the new string. */ + knownHosts[sz - 1] = 0; + encodedKey = (char*)WMALLOC(WOLFSSH_CLIENT_ENCKEY_SIZE_ESTIMATE + WOLFSSH_CLIENT_PUBKEYTYPE_SIZE_ESTIMATE + WOLFSSH_CLIENT_FINGERPRINT_SIZE_ESTIMATE, NULL, 0); @@ -306,8 +306,6 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) } if (ret == 0) { - word32 keySz; - pubKeyType = encodedKey + WOLFSSH_CLIENT_ENCKEY_SIZE_ESTIMATE; fp = pubKeyType + WOLFSSH_CLIENT_PUBKEYTYPE_SIZE_ESTIMATE; @@ -316,8 +314,9 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) fp[0] = 0; /* Get the key type out of the key. */ - ato32(pubKey, &keySz); - if (keySz > sz - sizeof(word32)) { + ato32(pubKey, &sz); + if ((sz > pubKeySz - sizeof(word32)) + || (sz > WOLFSSH_CLIENT_PUBKEYTYPE_SIZE_ESTIMATE - 1)) { ret = -1; } } @@ -479,6 +478,8 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) WFREE(encodedKey, NULL, 0); if (knownHosts) WFREE(knownHosts, NULL, 0); + if (knownHostsName) + WFREE(knownHostsName, NULL, 0); return ret; } From d004536aa73bf25c777d09128950139bb9832530 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 30 Nov 2023 16:53:09 -0800 Subject: [PATCH 2/2] Known Hosts Fix 1. Didn't take into account of getenv() returning NULL. Fixed. --- apps/wolfssh/common.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index 28376cf7..f83d135f 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -273,16 +273,23 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) char *env; env = getenv("HOME"); - sz = (word32)(WSTRLEN(env) + WSTRLEN(defaultName) + 1); - knownHostsName = (char*)WMALLOC(sz, NULL, 0); - if (knownHostsName != NULL) { - WSTRCPY(knownHostsName, env); - WSTRCAT(knownHostsName, defaultName); + if (env != NULL) { + sz = (word32)(WSTRLEN(env) + WSTRLEN(defaultName) + 1); + knownHostsName = (char*)WMALLOC(sz, NULL, 0); + if (knownHostsName != NULL) { + WSTRCPY(knownHostsName, env); + WSTRCAT(knownHostsName, defaultName); + } } + else + ret = -1; + } + + if (ret == 0) { + sz = 0; + ret = load_der_file(knownHostsName, (byte**)&knownHosts, &sz); } - sz = 0; - ret = load_der_file(knownHostsName, (byte**)&knownHosts, &sz); if (ret == 0) { if (sz < sizeof(word32)) { /* This file is too small. There must be at least a word32