From c201297639729f4285244d347e485bdeaf8b000a Mon Sep 17 00:00:00 2001 From: Duncan Webb Date: Sun, 21 Jan 2024 15:24:02 +0100 Subject: [PATCH 1/2] Fix possible buffer overrun calls to PR_GetErrorText Removed call to NSS_NoDB_Init as certfile will be defined. Copy paste from upscli change to ssl_init. --- server/netssl.c | 72 ++++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/server/netssl.c b/server/netssl.c index 79c5d3175f..d669864a36 100644 --- a/server/netssl.c +++ b/server/netssl.c @@ -193,9 +193,15 @@ static char *nss_password_callback(PK11SlotInfo *slot, PRBool retry, static void nss_error(const char* text) { char buffer[SMALLBUF]; - PRInt32 length = PR_GetErrorText(buffer); - if (length > 0 && length < SMALLBUF) { - upsdebugx(1, "nss_error %ld in %s : %s", (long)PR_GetError(), text, buffer); + PRErrorCode err_num = PR_GetError(); + PRInt32 err_len = PR_GetErrorTextLength(); + if (err_len > 0) { + if (err_len < SMALLBUF) { + PR_GetErrorText(buffer); + upsdebugx(1, "nss_error %ld in %s : %s", (long)err_num, text, buffer); + }else{ + upsdebugx(1, "nss_error %ld in %s : Internal error buffer too small, needs %ld bytes", (long)err_num, text, (long)err_len); + } }else{ upsdebugx(1, "nss_error %ld in %s", (long)PR_GetError(), text); } @@ -204,17 +210,20 @@ static void nss_error(const char* text) static int ssl_error(PRFileDesc *ssl, ssize_t ret) { char buffer[256]; - PRInt32 length; - PRErrorCode e; NUT_UNUSED_VARIABLE(ssl); NUT_UNUSED_VARIABLE(ret); - - e = PR_GetError(); - length = PR_GetErrorText(buffer); - if (length > 0 && length < 256) { - upsdebugx(1, "ssl_error() ret=%d %*s", e, length, buffer); - } else { - upsdebugx(1, "ssl_error() ret=%d", e); + PRErrorCode err_num = PR_GetError(); + PRInt32 err_len = PR_GetErrorTextLength(); + PRInt32 length; + if (err_len > 0) { + if (err_len < SMALLBUF) { + length = PR_GetErrorText(buffer); + upsdebugx(1, "ssl_error %ld : %*s", (long)err_num, length, buffer); + }else{ + upsdebugx(1, "ssl_error %ld : Internal error buffer too small, needs %ld bytes", (long)err_num, (long)err_len); + } + }else{ + upsdebugx(1, "ssl_error %ld", (long)err_num); } return -1; @@ -506,24 +515,21 @@ void ssl_init(void) PK11_SetPasswordFunc(nss_password_callback); - if (certfile) - /* Note: this call can generate memory leaks not resolvable - * by any release function. - * Probably NSS key module object allocation and - * probably NSS key db object allocation too. */ - status = NSS_Init(certfile); - else - status = NSS_NoDB_Init(NULL); + /* Note: this call can generate memory leaks not resolvable + * by any release function. + * Probably NSS key module object allocation and + * probably NSS key db object allocation too. */ + status = NSS_Init(certfile); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not initialize SSL context"); - nss_error("upscli_init / NSS_[NoDB]_Init"); + nss_error("ssl_init / NSS_Init"); return; } status = NSS_SetDomesticPolicy(); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not initialize SSL policy"); - nss_error("upscli_init / NSS_SetDomesticPolicy"); + nss_error("ssl_init / NSS_SetDomesticPolicy"); return; } @@ -531,7 +537,7 @@ void ssl_init(void) status = SSL_ConfigServerSessionIDCache(0, 0, 0, NULL); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not initialize SSL server cache"); - nss_error("upscli_init / SSL_ConfigServerSessionIDCache"); + nss_error("ssl_init / SSL_ConfigServerSessionIDCache"); return; } @@ -539,13 +545,13 @@ void ssl_init(void) status = SSL_OptionSetDefault(SSL_ENABLE_SSL3, PR_TRUE); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not enable SSLv3"); - nss_error("upscli_init / SSL_OptionSetDefault(SSL_ENABLE_SSL3)"); + nss_error("ssl_init / SSL_OptionSetDefault(SSL_ENABLE_SSL3)"); return; } status = SSL_OptionSetDefault(SSL_ENABLE_TLS, PR_TRUE); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not enable TLSv1"); - nss_error("upscli_init / SSL_OptionSetDefault(SSL_ENABLE_TLS)"); + nss_error("ssl_init / SSL_OptionSetDefault(SSL_ENABLE_TLS)"); return; } } else { @@ -553,7 +559,7 @@ void ssl_init(void) status = SSL_VersionRangeGetSupported(ssl_variant_stream, &range); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not get versions supported"); - nss_error("upscli_init / SSL_VersionRangeGetSupported"); + nss_error("ssl_init / SSL_VersionRangeGetSupported"); return; } range.min = SSL_LIBRARY_VERSION_TLS_1_1; @@ -563,7 +569,7 @@ void ssl_init(void) status = SSL_VersionRangeSetDefault(ssl_variant_stream, &range); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not set versions supported"); - nss_error("upscli_init / SSL_VersionRangeSetDefault"); + nss_error("ssl_init / SSL_VersionRangeSetDefault"); return; } /* Disable old/weak ciphers */ @@ -575,13 +581,13 @@ void ssl_init(void) status = SSL_OptionSetDefault(SSL_ENABLE_SSL3, PR_FALSE); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not disable SSLv3"); - nss_error("upscli_init / SSL_OptionSetDefault(SSL_DISABLE_SSL3)"); + nss_error("ssl_init / SSL_OptionSetDefault(SSL_DISABLE_SSL3)"); return; } status = SSL_OptionSetDefault(SSL_ENABLE_TLS, PR_TRUE); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not enable TLSv1"); - nss_error("upscli_init / SSL_OptionSetDefault(SSL_ENABLE_TLS)"); + nss_error("ssl_init / SSL_OptionSetDefault(SSL_ENABLE_TLS)"); return; } #endif @@ -599,7 +605,7 @@ void ssl_init(void) status = SSL_OptionSetDefault(SSL_REQUEST_CERTIFICATE, PR_TRUE); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not enable certificate request"); - nss_error("upscli_init / SSL_OptionSetDefault(SSL_REQUEST_CERTIFICATE)"); + nss_error("ssl_init / SSL_OptionSetDefault(SSL_REQUEST_CERTIFICATE)"); return; } } @@ -608,7 +614,7 @@ void ssl_init(void) status = SSL_OptionSetDefault(SSL_REQUIRE_CERTIFICATE, PR_TRUE); if (status != SECSuccess) { upslogx(LOG_ERR, "Can not enable certificate requirement"); - nss_error("upscli_init / SSL_OptionSetDefault(SSL_REQUIRE_CERTIFICATE)"); + nss_error("ssl_init / SSL_OptionSetDefault(SSL_REQUIRE_CERTIFICATE)"); return; } } @@ -617,14 +623,14 @@ void ssl_init(void) cert = PK11_FindCertFromNickname(certname, NULL); if(cert==NULL) { upslogx(LOG_ERR, "Can not find server certificate"); - nss_error("upscli_init / PK11_FindCertFromNickname"); + nss_error("ssl_init / PK11_FindCertFromNickname"); return; } privKey = PK11_FindKeyByAnyCert(cert, NULL); if(privKey==NULL){ upslogx(LOG_ERR, "Can not find private key associate to server certificate"); - nss_error("upscli_init / PK11_FindKeyByAnyCert"); + nss_error("ssl_init / PK11_FindKeyByAnyCert"); return; } From e2e5223ec4e3b386df6c878c23c15c81f86604d8 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sun, 21 Jan 2024 23:56:04 +0100 Subject: [PATCH 2/2] server/netssl.c: fix decl-after-statement NUT_UNUSED_VARIABLE() implementation counts as a code statement, so should be after true declarations. Signed-off-by: Jim Klimov --- server/netssl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/netssl.c b/server/netssl.c index d669864a36..f6f4993029 100644 --- a/server/netssl.c +++ b/server/netssl.c @@ -195,6 +195,7 @@ static void nss_error(const char* text) char buffer[SMALLBUF]; PRErrorCode err_num = PR_GetError(); PRInt32 err_len = PR_GetErrorTextLength(); + if (err_len > 0) { if (err_len < SMALLBUF) { PR_GetErrorText(buffer); @@ -210,11 +211,12 @@ static void nss_error(const char* text) static int ssl_error(PRFileDesc *ssl, ssize_t ret) { char buffer[256]; - NUT_UNUSED_VARIABLE(ssl); - NUT_UNUSED_VARIABLE(ret); PRErrorCode err_num = PR_GetError(); PRInt32 err_len = PR_GetErrorTextLength(); PRInt32 length; + NUT_UNUSED_VARIABLE(ssl); + NUT_UNUSED_VARIABLE(ret); + if (err_len > 0) { if (err_len < SMALLBUF) { length = PR_GetErrorText(buffer);