From 2aaf0fdc238a7ce766a727b0d2c797d766eb6158 Mon Sep 17 00:00:00 2001 From: Eugene Crosser Date: Sat, 20 Apr 2013 15:50:51 +0400 Subject: [PATCH] Stop leaks of memory and of privileges Fix several memory leaks and mishandling of the privilege status where a function returned failure indication, and previously allocated memory was not freed (and the referece was lost), or previously droped privileges where not restored. --- drop_privs.c | 29 ++++++++++++++++++++++++----- pam_yubico.c | 35 ++++++++++++++++++++++------------- util.c | 4 ++-- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/drop_privs.c b/drop_privs.c index 58e389e..e423d4e 100644 --- a/drop_privs.c +++ b/drop_privs.c @@ -55,7 +55,7 @@ static uid_t saved_euid; static gid_t saved_egid; -static gid_t *saved_groups; +static gid_t *saved_groups = NULL; static int saved_groups_length; #endif /* HAVE_PAM_MODUTIL_DROP_PRIV */ @@ -83,6 +83,11 @@ int drop_privileges(struct passwd *pw, pam_handle_t *pamh) { saved_euid = geteuid(); saved_egid = getegid(); + if ((saved_euid == pw->pw_uid) && (saved_egid == pw->pw_gid)) { + D (("Privilges already dropped, pretend it is all right")); + return 0; + } + saved_groups_length = getgroups(0, NULL); if (saved_groups_length < 0) { D (("getgroups: %s", strerror(errno))); @@ -90,6 +95,7 @@ int drop_privileges(struct passwd *pw, pam_handle_t *pamh) { } if (saved_groups_length > 0) { + if (saved_groups) free(saved_groups); /* size might have changed */ saved_groups = malloc(saved_groups_length * sizeof(gid_t)); if (saved_groups == NULL) { D (("malloc: %s", strerror(errno))); @@ -98,26 +104,30 @@ int drop_privileges(struct passwd *pw, pam_handle_t *pamh) { if (getgroups(saved_groups_length, saved_groups) < 0) { D (("getgroups: %s", strerror(errno))); - return -1; + goto free_out; } } if (initgroups(pw->pw_name, pw->pw_gid) < 0) { D (("initgroups: %s", strerror(errno))); - return -1; + goto free_out; } if (setegid(pw->pw_gid) < 0) { D (("setegid: %s", strerror(errno))); - return -1; + goto free_out; } if (seteuid(pw->pw_uid) < 0) { D (("seteuid: %s", strerror(errno))); - return -1; + goto free_out; } return 0; +free_out: + free(saved_groups); + saved_groups = NULL; + return -1; #endif /* HAVE_PAM_MODUTIL_DROP_PRIV */ } @@ -131,6 +141,11 @@ int restore_privileges(pam_handle_t *pamh) { _privs_location(1); return res; #else + if ((saved_euid == geteuid()) && (saved_egid == getegid())) { + D (("Privilges already as requested, pretend it is all right")); + return 0; + } + if (seteuid(saved_euid) < 0) { D (("seteuid: %s", strerror(errno))); return -1; @@ -141,6 +156,10 @@ int restore_privileges(pam_handle_t *pamh) { return -1; } + if (saved_groups == NULL) { + D (("saved groups are empty, looks like a program error!")); + return -1; + } if (setgroups(saved_groups_length, saved_groups) < 0) { D (("setgroups: %s", strerror(errno))); return -1; diff --git a/pam_yubico.c b/pam_yubico.c index 60a66ae..290056c 100644 --- a/pam_yubico.c +++ b/pam_yubico.c @@ -241,7 +241,8 @@ authorize_user_token (struct cfg *cfg, if (drop_privileges(p, pamh) < 0) { D (("could not drop privileges")); - return 0; + retval = 0; + goto free_out; } retval = check_user_token (cfg, userfile, username, otp_id); @@ -249,9 +250,11 @@ authorize_user_token (struct cfg *cfg, if (restore_privileges(pamh) < 0) { DBG (("could not restore privileges")); - return 0; + retval = 0; + goto free_out; } +free_out: free (userfile); } @@ -508,34 +511,34 @@ do_challenge_response(pam_handle_t *pamh, struct cfg *cfg, const char *username) fd = open(userfile, O_RDONLY, 0); if (fd < 0) { DBG (("Cannot open file: %s (%s)", userfile, strerror(errno))); - goto out; + goto restpriv_out; } if (fstat(fd, &st) < 0) { DBG (("Cannot stat file: %s (%s)", userfile, strerror(errno))); close(fd); - goto out; + goto restpriv_out; } if (!S_ISREG(st.st_mode)) { DBG (("%s is not a regular file", userfile)); close(fd); - goto out; + goto restpriv_out; } f = fdopen(fd, "r"); if (f == NULL) { DBG (("fdopen: %s", strerror(errno))); close(fd); - goto out; + goto restpriv_out; } if (! load_chalresp_state(f, &state, cfg->debug)) - goto out; + goto restpriv_out; if (fclose(f) < 0) { f = NULL; - goto out; + goto restpriv_out; } f = NULL; @@ -614,20 +617,20 @@ do_challenge_response(pam_handle_t *pamh, struct cfg *cfg, const char *username) /* Write out the new file */ tmpfile = malloc(strlen(userfile) + 1 + 4); if (! tmpfile) - goto out; + goto restpriv_out; strcpy(tmpfile, userfile); strcat(tmpfile, ".tmp"); fd = open(tmpfile, O_WRONLY | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR); if (fd < 0) { DBG (("Cannot open file: %s (%s)", tmpfile, strerror(errno))); - goto out; + goto restpriv_out; } f = fdopen(fd, "w"); if (! f) { close(fd); - goto out; + goto restpriv_out; } errstr = "Error updating Yubikey challenge, please check syslog or contact your system administrator"; @@ -635,11 +638,11 @@ do_challenge_response(pam_handle_t *pamh, struct cfg *cfg, const char *username) goto out; if (fclose(f) < 0) { f = NULL; - goto out; + goto restpriv_out; } f = NULL; if (rename(tmpfile, userfile) < 0) { - goto out; + goto restpriv_out; } if (restore_privileges(pamh) < 0) { @@ -650,6 +653,12 @@ do_challenge_response(pam_handle_t *pamh, struct cfg *cfg, const char *username) DBG(("Challenge-response success!")); errstr = NULL; errno = 0; + goto out; + +restpriv_out: + if (restore_privileges(pamh) < 0) { + DBG (("could not restore privileges")); + } out: if (yk_errno) { diff --git a/util.c b/util.c index 3066706..e1874bd 100644 --- a/util.c +++ b/util.c @@ -37,6 +37,7 @@ #include #include #include +#include #include "util.h" @@ -213,11 +214,10 @@ get_user_challenge_file(YK_KEY *yk, const char *chalresp_path, const char *usern int len; /* 0xffffffff == 4294967295 == 10 digits */ len = strlen(chalresp_path == NULL ? "challenge" : username) + 1 + 10 + 1; - if ((filename = malloc(len)) != NULL) { + if ((filename = alloca(len)) != NULL) { int res = snprintf(filename, len, "%s-%i", chalresp_path == NULL ? "challenge" : username, serial); if (res < 0 || res > len) { /* Not enough space, strangely enough. */ - free(filename); filename = NULL; } }