From a20059a09c56555f6c2006a7193de4c1676b477a Mon Sep 17 00:00:00 2001 From: =?utf8?q?Eivind=20N=C3=A6ss?= Date: Thu, 16 Mar 2023 16:13:25 -0700 Subject: [PATCH] Fix several issues uncovered by Coverity (#397) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * Fix for coverity issue 436265, we should cap copy to size of destination buffer Signed-off-by: Eivind Næss * Fix for coverity issue 436262, llv6_ntoa() returns a pointer to a buffer that can be up to 64 bytes long; likely not a problem, but this will quiet coverity Signed-off-by: Eivind Næss * Fix for coverity issue 436251, not freeing path in the normal flow of the code Signed-off-by: Eivind Næss * Fixing coverity issue #436258, Digest maybe uninitialized in some paths of this code Signed-off-by: Eivind Næss * Fix for coverity issue 436254, forgot to free 's' before returning from the function? Signed-off-by: Eivind Næss * Fixing coverity issue #436251, memory leak in put_string() function Signed-off-by: Eivind Næss * Fixing coverity issue 436215, should copy at most sizeof(devname) bytes Signed-off-by: Eivind Næss * Fixing coverity issue #436203, if no authentication (or no accounting) server was found, we still need to free the allocated local instance Signed-off-by: Eivind Næss * Fixing coverity issue #436171, use of uninitialized variable Signed-off-by: Eivind Næss * Use of signed vs unsigned variable in printf for MD4Update Signed-off-by: Eivind Næss * Fixing coverity issue #436182, fixing possible buffer overrun in handling of PW_CLASS attribute Signed-off-by: Eivind Næss * Fixing coverity issue #436156 Signed-off-by: Eivind Næss * Compile errors Signed-off-by: Eivind Næss [paulus@ozlabs.org - Squashed to avoid breaking bisection] Signed-off-by: Eivind Næss Signed-off-by: Paul Mackerras --- chat/chat.c | 14 +++++++++++--- pppd/auth.c | 2 +- pppd/chap_ms.c | 2 +- pppd/eap.c | 2 +- pppd/ipv6cp.c | 2 +- pppd/options.c | 2 ++ pppd/plugins/pppoatm/pppoatm.c | 2 +- pppd/plugins/pppoe/plugin.c | 2 +- pppd/plugins/radius/radius.c | 3 ++- pppd/plugins/radius/radrealms.c | 4 ++++ pppd/ppp-md4.c | 2 +- 11 files changed, 26 insertions(+), 11 deletions(-) diff --git a/chat/chat.c b/chat/chat.c index f625a8a..0740229 100644 --- a/chat/chat.c +++ b/chat/chat.c @@ -1133,7 +1133,7 @@ int chat_send (register char *s) if (verbose) msgf("timeout set to %d seconds", timeout); - + free(s); return 0; } @@ -1247,8 +1247,11 @@ int write_char(int c) int put_string(register char *s) { + char *s1; quiet = 0; + s = clean(s, 1); + s1 = s; if (verbose) { if (quiet) @@ -1263,8 +1266,10 @@ int put_string(register char *s) register char c = *s++; if (c != '\\') { - if (!write_char (c)) + if (!write_char (c)) { + free(s1); return 0; + } continue; } @@ -1283,8 +1288,10 @@ int put_string(register char *s) break; default: - if (!write_char (c)) + if (!write_char (c)) { + free(s1); return 0; + } break; } checksigs(); @@ -1292,6 +1299,7 @@ int put_string(register char *s) alarm(0); alarmed = 0; + free(s1); return (1); } diff --git a/pppd/auth.c b/pppd/auth.c index d27f630..202d557 100644 --- a/pppd/auth.c +++ b/pppd/auth.c @@ -1240,7 +1240,7 @@ np_finished(int unit, int proto) static void check_maxoctets(void *arg) { - unsigned int used; + unsigned int used = 0; ppp_link_stats_st stats; if (ppp_get_link_stats(&stats)) { diff --git a/pppd/chap_ms.c b/pppd/chap_ms.c index d1e0cf8..6b2a026 100644 --- a/pppd/chap_ms.c +++ b/pppd/chap_ms.c @@ -686,7 +686,7 @@ GenerateAuthenticatorResponse(unsigned char* PasswordHashHash, int i; PPP_MD_CTX *ctx; - u_char Digest[SHA_DIGEST_LENGTH]; + u_char Digest[SHA_DIGEST_LENGTH] = {}; int hash_len; u_char Challenge[8]; diff --git a/pppd/eap.c b/pppd/eap.c index 40f08b3..7015466 100644 --- a/pppd/eap.c +++ b/pppd/eap.c @@ -2654,7 +2654,7 @@ eap_response(eap_state *esp, u_char *inp, int id, int len) char tmp[MAXNAMELEN+1]; strcpy(tmp, strrchr(rhostname, '\\') + 1); - strcpy(rhostname, tmp); + strlcpy(rhostname, tmp, sizeof(rhostname)); } if (chap_verify_hook) diff --git a/pppd/ipv6cp.c b/pppd/ipv6cp.c index 795f8a9..a36b1d9 100644 --- a/pppd/ipv6cp.c +++ b/pppd/ipv6cp.c @@ -1490,7 +1490,7 @@ ipv6cp_script_done(void *arg) static void ipv6cp_script(char *script) { - char strspeed[32], strlocal[32], strremote[32]; + char strspeed[32], strlocal[64], strremote[64]; char *argv[8]; sprintf(strspeed, "%d", baud_rate); diff --git a/pppd/options.c b/pppd/options.c index f2ff59d..214adae 100644 --- a/pppd/options.c +++ b/pppd/options.c @@ -1795,6 +1795,8 @@ loadplugin(char **argv) } info("Plugin %s loaded.", arg); (*init)(); + if (path != arg) + free(path); return 1; errclose: diff --git a/pppd/plugins/pppoatm/pppoatm.c b/pppd/plugins/pppoatm/pppoatm.c index 207e5bf..39d3dbd 100644 --- a/pppd/plugins/pppoatm/pppoatm.c +++ b/pppd/plugins/pppoatm/pppoatm.c @@ -92,7 +92,7 @@ static int setdevname_pppoatm(const char *cp, const char **argv, int doit) return 1; memcpy(&pvcaddr, &addr, sizeof pvcaddr); - strlcpy(devnam, cp, MAXPATHLEN); + strlcpy(devnam, cp, sizeof(devnam)); ppp_set_devnam(devnam); devstat.st_mode = S_IFSOCK; if (the_channel != &pppoa_channel) { diff --git a/pppd/plugins/pppoe/plugin.c b/pppd/plugins/pppoe/plugin.c index ee9d343..c7ace96 100644 --- a/pppd/plugins/pppoe/plugin.c +++ b/pppd/plugins/pppoe/plugin.c @@ -389,7 +389,7 @@ PPPoEDevnameHook(char *cmd, char **argv, int doit) /* Close socket */ close(fd); if (r && doit) { - strlcpy(devnam, cmd, MAXPATHLEN); + strlcpy(devnam, cmd, sizeof(devnam)); if (the_channel != &pppoe_channel) { the_channel = &pppoe_channel; diff --git a/pppd/plugins/radius/radius.c b/pppd/plugins/radius/radius.c index b4bc896..11cf27c 100644 --- a/pppd/plugins/radius/radius.c +++ b/pppd/plugins/radius/radius.c @@ -649,7 +649,8 @@ radius_setparams(VALUE_PAIR *vp, char *msg, REQUEST_INFO *req_info, break; case PW_CLASS: /* Save Class attribute to pass it in accounting request */ - if (vp->lvalue <= MAXCLASSLEN) { + // if (vp->lvalue <= MAXCLASSLEN) { // <- Attribute could be this big, but vp->strvalue is limited to AUTH_STRING_LEN characters + if (vp->lvalue <= AUTH_STRING_LEN) { rstate.class_len=vp->lvalue; memcpy(rstate.class, vp->strvalue, rstate.class_len); } /* else too big for our buffer - ignore it */ diff --git a/pppd/plugins/radius/radrealms.c b/pppd/plugins/radius/radrealms.c index a0dde00..ab923cc 100644 --- a/pppd/plugins/radius/radrealms.c +++ b/pppd/plugins/radius/radrealms.c @@ -148,9 +148,13 @@ lookup_realm(char const *user, if (accts->max) *acctserver = accts; + else + free(accts); if (auths->max) *authserver = auths; + else + free(auths); return; } diff --git a/pppd/ppp-md4.c b/pppd/ppp-md4.c index aa5fece..c581110 100644 --- a/pppd/ppp-md4.c +++ b/pppd/ppp-md4.c @@ -326,7 +326,7 @@ MD4Update(MD4_CTX *MDp, unsigned char *X, unsigned int count) } else if (count > 512) /* Check for count too large */ { - printf("\nError: MD4Update called with illegal count value %d.", + printf("\nError: MD4Update called with illegal count value %u.", count); return; } -- 2.39.2