Skip to content

Commit ca912b0

Browse files
JacobBarthelmehJacobBarthelmeh
committed
fixes for brace style, null deref, ecc curve detection, unused variables
fix for memory leak and windows build issues sanity check on input and refactor parse cert store spec function
1 parent 4ddac11 commit ca912b0

File tree

11 files changed

+304
-212
lines changed

11 files changed

+304
-212
lines changed

‎.github/workflows/windows-cert-store-test.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ name: Windows Certificate Store Test
99

1010
on:
1111
push:
12-
branches: [ '*' ]
12+
branches: [ 'master', 'main', 'release/**' ]
1313
pull_request:
1414
branches: [ '*' ]
1515

‎apps/wolfsshd/configuration.c‎

Lines changed: 49 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -87,9 +87,11 @@ struct WOLFSSHD_CONFIG {
8787
char* authKeysFile;
8888
char* forceCmd;
8989
char* pidFile;
90+
#ifdef USE_WINDOWS_API
9091
char* winUserStores;
9192
char* winUserDwFlags;
9293
char* winUserPvPara;
94+
#endif /* USE_WINDOWS_API */
9395
WOLFSSHD_CONFIG* next; /* next config in list */
9496
long loginTimer;
9597
word16 port;
@@ -331,9 +333,11 @@ void wolfSSHD_ConfigFree(WOLFSSHD_CONFIG* conf)
331333
#endif /* WOLFSSH_CERTS */
332334
#endif /* USE_WINDOWS_API */
333335
FreeString(&current->pidFile, heap);
336+
#ifdef USE_WINDOWS_API
334337
FreeString(&current->winUserStores, heap);
335338
FreeString(&current->winUserDwFlags, heap);
336339
FreeString(&current->winUserPvPara, heap);
340+
#endif /* USE_WINDOWS_API */
337341

338342
WFREE(current, heap, DYNTYPE_SSHD);
339343
current = next;
@@ -381,13 +385,16 @@ enum {
381385
OPT_BANNER = 23,
382386
OPT_TRUSTED_SYSTEM_CA_KEYS = 24,
383387
OPT_TRUSTED_USER_CA_STORE = 25,
388+
#ifdef USE_WINDOWS_API
384389
OPT_WIN_USER_STORES = 26,
385390
OPT_WIN_USER_DW_FLAGS = 27,
386-
OPT_WIN_USER_PV_PARA = 28
391+
OPT_WIN_USER_PV_PARA = 28,
392+
#endif /* USE_WINDOWS_API */
387393
};
388394
enum {
389-
NUM_OPTIONS = 29
395+
NUM_OPTIONS = 26
390396
#ifdef USE_WINDOWS_API
397+
+ 3
391398
#ifdef WOLFSSH_CERTS
392399
+ 3
393400
#endif /* WOLFSSH_CERTS */
@@ -432,9 +439,11 @@ static const CONFIG_OPTION options[NUM_OPTIONS] = {
432439
{OPT_BANNER, "Banner"},
433440
{OPT_TRUSTED_SYSTEM_CA_KEYS, "wolfSSH_TrustedSystemCAKeys"},
434441
{OPT_TRUSTED_USER_CA_STORE, "wolfSSH_TrustedUserCaStore"},
442+
#ifdef USE_WINDOWS_API
435443
{OPT_WIN_USER_STORES, "wolfSSH_WinUserStores"},
436444
{OPT_WIN_USER_DW_FLAGS, "wolfSSH_WinUserDwFlags"},
437-
{OPT_WIN_USER_PV_PARA, "wolfSSH_WinUserPvPara"}
445+
{OPT_WIN_USER_PV_PARA, "wolfSSH_WinUserPvPara"},
446+
#endif /* USE_WINDOWS_API */
438447
};
439448

440449
/* returns WS_SUCCESS on success */
@@ -1088,6 +1097,7 @@ static int HandleConfigOption(WOLFSSHD_CONFIG** conf, int opt,
10881097
case OPT_TRUSTED_USER_CA_STORE:
10891098
ret = wolfSSHD_ConfigSetUserCAStore(*conf, value);
10901099
break;
1100+
#ifdef USE_WINDOWS_API
10911101
case OPT_WIN_USER_STORES:
10921102
ret = wolfSSHD_ConfigSetWinUserStores(*conf, value);
10931103
break;
@@ -1097,6 +1107,7 @@ static int HandleConfigOption(WOLFSSHD_CONFIG** conf, int opt,
10971107
case OPT_WIN_USER_PV_PARA:
10981108
ret = wolfSSHD_ConfigSetWinUserPvPara(*conf, value);
10991109
break;
1110+
#endif /* USE_WINDOWS_API */
11001111
#ifdef USE_WINDOWS_API
11011112
#ifdef WOLFSSH_CERTS
11021113
case OPT_HOST_KEY_STORE:
@@ -1474,11 +1485,13 @@ int wolfSSHD_ConfigSetUserCAStore(WOLFSSHD_CONFIG* conf, const char* value)
14741485
return ret;
14751486
}
14761487

1477-
char* wolfSSHD_ConfigGetWinUserStores(WOLFSSHD_CONFIG* conf) {
1488+
#ifdef USE_WINDOWS_API
1489+
char* wolfSSHD_ConfigGetWinUserStores(WOLFSSHD_CONFIG* conf)
1490+
{
14781491
if (conf != NULL) {
14791492
if (conf->winUserStores == NULL) {
14801493
/* If no value was specified, default to CERT_STORE_PROV_SYSTEM */
1481-
CreateString(&conf->winUserStores, "CERT_STORE_PROV_SYSTEM",
1494+
CreateString(&conf->winUserStores, "CERT_STORE_PROV_SYSTEM",
14821495
(int)WSTRLEN("CERT_STORE_PROV_SYSTEM"), conf->heap);
14831496
}
14841497

@@ -1488,24 +1501,32 @@ char* wolfSSHD_ConfigGetWinUserStores(WOLFSSHD_CONFIG* conf) {
14881501
return NULL;
14891502
}
14901503

1491-
int wolfSSHD_ConfigSetWinUserStores(WOLFSSHD_CONFIG* conf, const char* value) {
1504+
int wolfSSHD_ConfigSetWinUserStores(WOLFSSHD_CONFIG* conf, const char* value)
1505+
{
14921506
int ret = WS_SUCCESS;
14931507

14941508
if (conf == NULL) {
14951509
ret = WS_BAD_ARGUMENT;
14961510
}
14971511

1498-
ret = CreateString(&conf->winUserStores, value, (int)WSTRLEN(value), conf->heap);
1512+
if (ret == WS_SUCCESS) {
1513+
ret = CreateString(&conf->winUserStores, value,
1514+
(int)WSTRLEN(value), conf->heap);
1515+
}
14991516

15001517
return ret;
15011518
}
15021519

1503-
char* wolfSSHD_ConfigGetWinUserDwFlags(WOLFSSHD_CONFIG* conf) {
1520+
char* wolfSSHD_ConfigGetWinUserDwFlags(WOLFSSHD_CONFIG* conf)
1521+
{
15041522
if (conf != NULL) {
15051523
if (conf->winUserDwFlags == NULL) {
1506-
/* If no value was specified, default to CERT_SYSTEM_STORE_CURRENT_USER */
1507-
CreateString(&conf->winUserDwFlags, "CERT_SYSTEM_STORE_CURRENT_USER",
1508-
(int)WSTRLEN("CERT_SYSTEM_STORE_CURRENT_USER"), conf->heap);
1524+
/* If no value was specified, default to
1525+
* CERT_SYSTEM_STORE_CURRENT_USER */
1526+
CreateString(&conf->winUserDwFlags,
1527+
"CERT_SYSTEM_STORE_CURRENT_USER",
1528+
(int)WSTRLEN("CERT_SYSTEM_STORE_CURRENT_USER"),
1529+
conf->heap);
15091530
}
15101531

15111532
return conf->winUserDwFlags;
@@ -1514,23 +1535,29 @@ char* wolfSSHD_ConfigGetWinUserDwFlags(WOLFSSHD_CONFIG* conf) {
15141535
return NULL;
15151536
}
15161537

1517-
int wolfSSHD_ConfigSetWinUserDwFlags(WOLFSSHD_CONFIG* conf, const char* value) {
1538+
int wolfSSHD_ConfigSetWinUserDwFlags(WOLFSSHD_CONFIG* conf, const char* value)
1539+
{
15181540
int ret = WS_SUCCESS;
15191541

15201542
if (conf == NULL) {
15211543
ret = WS_BAD_ARGUMENT;
15221544
}
15231545

1524-
ret = CreateString(&conf->winUserDwFlags, value, (int)WSTRLEN(value), conf->heap);
1546+
if (ret == WS_SUCCESS) {
1547+
ret = CreateString(&conf->winUserDwFlags, value,
1548+
(int)WSTRLEN(value), conf->heap);
1549+
}
15251550

15261551
return ret;
15271552
}
15281553

1529-
char* wolfSSHD_ConfigGetWinUserPvPara(WOLFSSHD_CONFIG* conf) {
1554+
char* wolfSSHD_ConfigGetWinUserPvPara(WOLFSSHD_CONFIG* conf)
1555+
{
15301556
if (conf != NULL) {
15311557
if (conf->winUserPvPara == NULL) {
15321558
/* If no value was specified, default to MY */
1533-
CreateString(&conf->winUserPvPara, "MY", (int)WSTRLEN("MY"), conf->heap);
1559+
CreateString(&conf->winUserPvPara, "MY",
1560+
(int)WSTRLEN("MY"), conf->heap);
15341561
}
15351562

15361563
return conf->winUserPvPara;
@@ -1539,17 +1566,22 @@ char* wolfSSHD_ConfigGetWinUserPvPara(WOLFSSHD_CONFIG* conf) {
15391566
return NULL;
15401567
}
15411568

1542-
int wolfSSHD_ConfigSetWinUserPvPara(WOLFSSHD_CONFIG* conf, const char* value) {
1569+
int wolfSSHD_ConfigSetWinUserPvPara(WOLFSSHD_CONFIG* conf, const char* value)
1570+
{
15431571
int ret = WS_SUCCESS;
15441572

15451573
if (conf == NULL) {
15461574
ret = WS_BAD_ARGUMENT;
15471575
}
15481576

1549-
ret = CreateString(&conf->winUserPvPara, value, (int)WSTRLEN(value), conf->heap);
1577+
if (ret == WS_SUCCESS) {
1578+
ret = CreateString(&conf->winUserPvPara, value,
1579+
(int)WSTRLEN(value), conf->heap);
1580+
}
15501581

15511582
return ret;
15521583
}
1584+
#endif /* USE_WINDOWS_API */
15531585

15541586
char* wolfSSHD_ConfigGetUserCAKeysFile(const WOLFSSHD_CONFIG* conf)
15551587
{

‎apps/wolfsshd/configuration.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,12 +53,14 @@ int wolfSSHD_ConfigSetSystemCA(WOLFSSHD_CONFIG* conf, const char* value);
5353
int wolfSSHD_ConfigGetSystemCA(const WOLFSSHD_CONFIG* conf);
5454
int wolfSSHD_ConfigSetUserCAStore(WOLFSSHD_CONFIG* conf, const char* value);
5555
int wolfSSHD_ConfigGetUserCAStore(const WOLFSSHD_CONFIG* conf);
56+
#ifdef USE_WINDOWS_API
5657
char* wolfSSHD_ConfigGetWinUserStores(WOLFSSHD_CONFIG* conf);
5758
int wolfSSHD_ConfigSetWinUserStores(WOLFSSHD_CONFIG* conf, const char* value);
5859
char* wolfSSHD_ConfigGetWinUserDwFlags(WOLFSSHD_CONFIG* conf);
5960
int wolfSSHD_ConfigSetWinUserDwFlags(WOLFSSHD_CONFIG* conf, const char* value);
6061
char* wolfSSHD_ConfigGetWinUserPvPara(WOLFSSHD_CONFIG* conf);
6162
int wolfSSHD_ConfigSetWinUserPvPara(WOLFSSHD_CONFIG* conf, const char* value);
63+
#endif /* USE_WINDOWS_API */
6264
int wolfSSHD_ConfigSetUserCAKeysFile(WOLFSSHD_CONFIG* conf, const char* file);
6365
word16 wolfSSHD_ConfigGetPort(const WOLFSSHD_CONFIG* conf);
6466
char* wolfSSHD_ConfigGetAuthKeysFile(const WOLFSSHD_CONFIG* conf);

‎apps/wolfsshd/wolfsshd.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -538,7 +538,7 @@ static int SetupCTX(WOLFSSHD_CONFIG* conf, WOLFSSH_CTX** ctx,
538538

539539
if (ret == WS_SUCCESS) {
540540
if (wolfSSHD_ConfigGetUserCAStore(conf)) {
541-
#if defined(_WIN32)
541+
#ifdef USE_WINDOWS_API
542542
if (wolfSSL_CTX_load_windows_user_CA_certs(sslCtx,
543543
wolfSSHD_ConfigGetWinUserStores(conf),
544544
wolfSSHD_ConfigGetWinUserDwFlags(conf),
@@ -550,7 +550,7 @@ static int SetupCTX(WOLFSSHD_CONFIG* conf, WOLFSSH_CTX** ctx,
550550
wolfSSH_Log(WS_LOG_INFO,
551551
"[SSHD] User CA store is only supported on Windows");
552552
ret = WS_BAD_ARGUMENT;
553-
#endif /* _WIN32 */
553+
#endif /* USE_WINDOWS_API */
554554
}
555555
}
556556

‎examples/echoserver/echoserver.c‎

Lines changed: 12 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,14 @@
4141
#include <wolfssh/internal.h>
4242
#include <wolfssh/wolfsftp.h>
4343
#include <wolfssh/agent.h>
44+
#include <wolfssh/certman.h>
4445
#include <wolfssh/port.h>
4546
#include <wolfssh/test.h>
4647
#include <wolfssl/wolfcrypt/ecc.h>
4748
#include <wolfssl/wolfcrypt/logging.h>
4849

4950
#include "examples/echoserver/echoserver.h"
51+
#include "examples/client/common.h"
5052

5153
#if defined(WOLFSSL_PTHREADS) && defined(WOLFSSL_TEST_GLOBAL_REQ)
5254
#include <pthread.h>
@@ -2958,63 +2960,22 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
29582960

29592961
#if defined(USE_WINDOWS_API) && defined(WOLFSSH_CERTS)
29602962
if (certStoreSpec != NULL) {
2961-
/* Load host key from Windows certificate store: store:subject:flags */
2962-
char* specCopy = NULL;
2963-
char* storeName = NULL;
2964-
char* subjectName = NULL;
2965-
char* flagsStr = NULL;
2963+
/* Load host key from Windows certificate store */
29662964
wchar_t* wStoreName = NULL;
29672965
wchar_t* wSubjectName = NULL;
2968-
DWORD dwFlags = CERT_SYSTEM_STORE_CURRENT_USER;
2966+
DWORD dwFlags = 0;
29692967
int ret;
2970-
size_t specLen = WSTRLEN(certStoreSpec) + 1;
2971-
2972-
specCopy = (char*)WMALLOC(specLen, NULL, 0);
2973-
if (specCopy == NULL) {
2974-
ES_ERROR("Memory allocation failed for cert store spec\n");
2975-
}
2976-
WSTRNCPY(specCopy, certStoreSpec, specLen);
2977-
2978-
storeName = specCopy;
2979-
subjectName = WSTRCHR(storeName, ':');
2980-
if (subjectName != NULL) {
2981-
*subjectName++ = '\0';
2982-
flagsStr = WSTRCHR(subjectName, ':');
2983-
if (flagsStr != NULL) {
2984-
*flagsStr++ = '\0';
2985-
if (WSTRCMP(flagsStr, "CURRENT_USER") == 0) {
2986-
dwFlags = CERT_SYSTEM_STORE_CURRENT_USER;
2987-
} else if (WSTRCMP(flagsStr, "LOCAL_MACHINE") == 0) {
2988-
dwFlags = CERT_SYSTEM_STORE_LOCAL_MACHINE;
2989-
} else {
2990-
dwFlags = (DWORD)atoi(flagsStr);
2991-
}
2992-
}
2993-
}
2994-
if (storeName == NULL || subjectName == NULL) {
2995-
WFREE(specCopy, NULL, 0);
2996-
ES_ERROR("Invalid cert store spec. Use: store:subject:flags\n");
2997-
}
29982968

2999-
{
3000-
int wStoreNameLen = MultiByteToWideChar(CP_UTF8, 0, storeName, -1, NULL, 0);
3001-
int wSubjectNameLen = MultiByteToWideChar(CP_UTF8, 0, subjectName, -1, NULL, 0);
3002-
wStoreName = (wchar_t*)WMALLOC(wStoreNameLen * sizeof(wchar_t), NULL, 0);
3003-
wSubjectName = (wchar_t*)WMALLOC(wSubjectNameLen * sizeof(wchar_t), NULL, 0);
3004-
if (wStoreName == NULL || wSubjectName == NULL) {
3005-
if (wStoreName != NULL) WFREE(wStoreName, NULL, 0);
3006-
if (wSubjectName != NULL) WFREE(wSubjectName, NULL, 0);
3007-
WFREE(specCopy, NULL, 0);
3008-
ES_ERROR("Memory allocation failed for cert store wide strings\n");
3009-
}
3010-
MultiByteToWideChar(CP_UTF8, 0, storeName, -1, wStoreName, wStoreNameLen);
3011-
MultiByteToWideChar(CP_UTF8, 0, subjectName, -1, wSubjectName, wSubjectNameLen);
2969+
ret = wolfSSH_ParseCertStoreSpec(certStoreSpec, &wStoreName,
2970+
&wSubjectName, &dwFlags, NULL);
2971+
if (ret != WS_SUCCESS) {
2972+
ES_ERROR("Invalid cert store spec. Use: store:subject:flags\n");
30122973
}
30132974

3014-
ret = wolfSSH_CTX_UsePrivateKey_fromStore(ctx, wStoreName, dwFlags, wSubjectName);
3015-
WFREE(specCopy, NULL, 0);
3016-
WFREE(wStoreName, NULL, 0);
3017-
WFREE(wSubjectName, NULL, 0);
2975+
ret = wolfSSH_CTX_UsePrivateKey_fromStore(ctx, wStoreName,
2976+
dwFlags, wSubjectName);
2977+
WFREE(wStoreName, NULL, DYNTYPE_TEMP);
2978+
WFREE(wSubjectName, NULL, DYNTYPE_TEMP);
30182979
if (ret != WS_SUCCESS) {
30192980
ES_ERROR("Couldn't load host key from certificate store.\n");
30202981
}

0 commit comments

Comments
 (0)