Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TACACS+]: Extract tacacs support functions into library and fix memory leak issue. #8659

Merged
merged 13 commits into from
Oct 14, 2021
228 changes: 228 additions & 0 deletions src/tacacs/pam/0007-Fix-memory-leak-when-parse-configuration.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,228 @@
From a706774c8aa51bdbef4ab17c7792699d576c532b Mon Sep 17 00:00:00 2001
From: liuh <liuh@microsoft.com>
Date: Fri, 1 Oct 2021 22:31:26 +0800
Subject: [PATCH 1/3] Fix memory leak when parse configuration.

---
pam_tacplus.c | 10 +++--
support.c | 108 +++++++++++++++++++++++++++++++++++---------------
support.h | 4 +-
3 files changed, 84 insertions(+), 38 deletions(-)

diff --git a/pam_tacplus.c b/pam_tacplus.c
index 9fc6be7..79d8e34 100644
--- a/pam_tacplus.c
+++ b/pam_tacplus.c
@@ -322,8 +322,9 @@ int pam_sm_authenticate (pam_handle_t * pamh, int flags,
}
status = PAM_SUCCESS;
communicating = 0;
- active_server.addr = tac_srv[srv_i].addr;
- active_server.key = tac_srv[srv_i].key;
+ active_server.addr = tac_srv[srv_i].addr;
+ /* copy secret to key */
+ snprintf(active_server.key, sizeof(active_server.key), "%s", tac_srv[srv_i].key);

if (ctrl & PAM_TAC_DEBUG)
syslog(LOG_DEBUG, "%s: active srv %d", __FUNCTION__, srv_i);
@@ -819,8 +820,9 @@ int pam_sm_chauthtok(pam_handle_t * pamh, int flags,
status = PAM_SUCCESS;
communicating = 0;

- active_server.addr = tac_srv[srv_i].addr;
- active_server.key = tac_srv[srv_i].key;
+ active_server.addr = tac_srv[srv_i].addr;
+ /* copy secret to key */
+ snprintf(active_server.key, sizeof(active_server.key), "%s", tac_srv[srv_i].key);

if (ctrl & PAM_TAC_DEBUG)
syslog(LOG_DEBUG, "%s: active srv %d", __FUNCTION__, srv_i);
diff --git a/support.c b/support.c
index be1f21b..b940a40 100644
--- a/support.c
+++ b/support.c
@@ -30,7 +30,12 @@
#include <stdlib.h>
#include <string.h>

+/* tacacs server information */
tacplus_server_t tac_srv[TAC_PLUS_MAXSERVERS];
+struct addrinfo tac_srv_addr[TAC_PLUS_MAXSERVERS];
+struct sockaddr tac_sock_addr[TAC_PLUS_MAXSERVERS];
+struct sockaddr_in6 tac_sock6_addr[TAC_PLUS_MAXSERVERS];
+
int tac_srv_no = 0;

char tac_service[64];
@@ -38,7 +43,12 @@ char tac_protocol[64];
char tac_prompt[64];
char *__vrfname=NULL;
char tac_source_ip[64];
-struct addrinfo *tac_source_addr = NULL;
+
+/* source address */
+struct addrinfo *tac_source_addr=NULL;
+struct addrinfo tac_source_address;
+struct sockaddr tac_source_sock_addr;
+struct sockaddr_in6 tac_source_sock6_addr;

void _pam_log(int err, const char *format,...) {
char msg[256];
@@ -172,6 +182,57 @@ int tacacs_get_password (pam_handle_t * pamh, int flags

return PAM_SUCCESS;
}
+
+/*
+ * Set tacacs server addrinfo.
+ */
+void set_tacacs_server_addr(int tac_srv_no, struct addrinfo* server) {
liuh-80 marked this conversation as resolved.
Show resolved Hide resolved
+ tac_srv[tac_srv_no].addr = &(tac_srv_addr[tac_srv_no]);
+ memcpy(tac_srv[tac_srv_no].addr, server, sizeof(struct addrinfo));
+
+ if (server->ai_family == AF_INET6) {
+ tac_srv[tac_srv_no].addr->ai_addr = (struct sockaddr *)&(tac_sock6_addr[tac_srv_no]);
+ memcpy(tac_srv[tac_srv_no].addr->ai_addr, server->ai_addr, sizeof(struct sockaddr_in6));
+ }
+ else {
+ tac_srv[tac_srv_no].addr->ai_addr = &(tac_sock_addr[tac_srv_no]);
+ memcpy(tac_srv[tac_srv_no].addr->ai_addr, server->ai_addr, sizeof(struct sockaddr));
+ }
+
+ tac_srv[tac_srv_no].addr->ai_canonname = NULL;
+ tac_srv[tac_srv_no].addr->ai_next = NULL;
+}
+
+/* set source ip address for the outgoing tacacs packets */
+void set_source_ip(const char *tac_source_ip) {
liuh-80 marked this conversation as resolved.
Show resolved Hide resolved
+
+ struct addrinfo hints, *source_address;
+ int rv;
+
+ /* set the source ip address for the tacacs packets */
+ memset(&hints, 0, sizeof(hints));
+ hints.ai_family = AF_UNSPEC;
+ hints.ai_socktype = SOCK_STREAM;
+ if ((rv = getaddrinfo(tac_source_ip, NULL, &hints,
+ &source_address)) != 0) {
+ _pam_log(LOG_ERR, "error setting the source ip information");
+ } else {
+ tac_source_addr = &tac_source_address;
+ memcpy(tac_source_addr, source_address, sizeof(struct addrinfo));
+
+ if (source_address->ai_family == AF_INET6) {
+ tac_source_addr->ai_addr = (struct sockaddr *)&(tac_source_sock6_addr);
+ memcpy(tac_source_addr->ai_addr, source_address->ai_addr, sizeof(struct sockaddr_in6));
+ }
+ else {
+ tac_source_addr->ai_addr = &(tac_source_sock_addr);
+ memcpy(tac_source_addr->ai_addr, source_address->ai_addr, sizeof(struct sockaddr));
+ }
+
+ freeaddrinfo(source_address);
+ _pam_log(LOG_DEBUG, "source ip is set");
+ }
+}

int _pam_parse (int argc, const char **argv) {
int ctrl = 0;
@@ -186,11 +247,6 @@ int _pam_parse (int argc, const char **argv) {
tac_prompt[0] = 0;
tac_login[0] = 0;
tac_source_ip[0] = 0;
-
- if (tac_source_addr != NULL) {
- freeaddrinfo(tac_source_addr);
- tac_source_addr = NULL;
- }

for (ctrl = 0; argc-- > 0; ++argv) {
if (!strcmp (*argv, "debug")) { /* all */
@@ -246,10 +302,16 @@ int _pam_parse (int argc, const char **argv) {
}
if ((rv = getaddrinfo(server_name, (port == NULL) ? "49" : port, &hints, &servers)) == 0) {
for(server = servers; server != NULL && tac_srv_no < TAC_PLUS_MAXSERVERS; server = server->ai_next) {
- tac_srv[tac_srv_no].addr = server;
- tac_srv[tac_srv_no].key = current_secret;
+ /* set server address with allocate memory */
+ set_tacacs_server_addr(tac_srv_no, server);
liuh-80 marked this conversation as resolved.
Show resolved Hide resolved
+
+ /* copy secret to key */
+ snprintf(tac_srv[tac_srv_no].key, sizeof(tac_srv[tac_srv_no].key), "%s", current_secret);
tac_srv_no++;
}
+
+ /* release servers memory */
+ freeaddrinfo(servers);
} else {
_pam_log (LOG_ERR,
"skip invalid server: %s (getaddrinfo: %s)",
@@ -266,10 +328,11 @@ int _pam_parse (int argc, const char **argv) {

/* if 'secret=' was given after a 'server=' parameter, fill in the current secret */
for(i = tac_srv_no-1; i >= 0; i--) {
- if (tac_srv[i].key != NULL)
+ if (tac_srv[i].key[0] != 0)
break;

- tac_srv[i].key = current_secret;
+ /* copy secret to key */
+ snprintf(tac_srv[i].key, sizeof(tac_srv[i].key), "%s", current_secret);
}
} else if (!strncmp (*argv, "timeout=", 8)) {
/* FIXME atoi() doesn't handle invalid numeric strings well */
@@ -284,8 +347,8 @@ int _pam_parse (int argc, const char **argv) {
__vrfname = strdup(*argv + 4);
} else if (!strncmp (*argv, "source_ip=", strlen("source_ip="))) {
/* source ip for the packets */
- strncpy (tac_source_ip, *argv + strlen("source_ip="), sizeof(tac_source_ip));
- set_source_ip (tac_source_ip, &tac_source_addr);
+ strncpy (tac_source_ip, *argv + strlen("source_ip="), sizeof(tac_source_ip));
liuh-80 marked this conversation as resolved.
Show resolved Hide resolved
+ set_source_ip (tac_source_ip);
} else {
_pam_log (LOG_WARNING, "unrecognized option: %s", *argv);
}
@@ -308,23 +371,4 @@ int _pam_parse (int argc, const char **argv) {
}

return ctrl;
-} /* _pam_parse */
-
-/* set source ip address for the outgoing tacacs packets */
-void set_source_ip(const char *tac_source_ip,
- struct addrinfo **source_address) {
-
- struct addrinfo hints;
- int rv;
-
- /* set the source ip address for the tacacs packets */
- memset(&hints, 0, sizeof(hints));
- hints.ai_family = AF_UNSPEC;
- hints.ai_socktype = SOCK_STREAM;
- if ((rv = getaddrinfo(tac_source_ip, NULL, &hints,
- source_address)) != 0) {
- _pam_log(LOG_ERR, "error setting the source ip information");
- } else {
- _pam_log(LOG_DEBUG, "source ip is set");
- }
-}
+} /* _pam_parse */
\ No newline at end of file
diff --git a/support.h b/support.h
index b1faf43..e20e227 100644
--- a/support.h
+++ b/support.h
@@ -27,8 +27,8 @@
#include <security/pam_modules.h>

typedef struct {
- struct addrinfo *addr;
- const char *key;
+ struct addrinfo *addr;
+ char key[256];
} tacplus_server_t;

extern tacplus_server_t tac_srv[TAC_PLUS_MAXSERVERS];
--
2.17.1.windows.2

Loading