From 51db465ce56e2da5bd3b035e85161706f67a6c4f65d13227e8cf431be5eb4c6d Mon Sep 17 00:00:00 2001 From: Fabian Vogt Date: Wed, 9 Aug 2017 08:40:05 +0000 Subject: [PATCH] - Add various patches (pending upstream review) to fix several issues: * 0001-Several-cleanups.patch * 0002-Avoid-dropping-privileges-by-initializing-gcrypt-sec.patch * 0003-Check-for-a-graphical-session.patch OBS-URL: https://build.opensuse.org/package/show/KDE:Frameworks5/pam_kwallet?expand=0&rev=38 --- 0001-Several-cleanups.patch | 162 ++++++++++++++++++ ...rivileges-by-initializing-gcrypt-sec.patch | 38 ++++ 0003-Check-for-a-graphical-session.patch | 72 ++++++++ pam_kwallet.changes | 9 + pam_kwallet.spec | 9 + 5 files changed, 290 insertions(+) create mode 100644 0001-Several-cleanups.patch create mode 100644 0002-Avoid-dropping-privileges-by-initializing-gcrypt-sec.patch create mode 100644 0003-Check-for-a-graphical-session.patch diff --git a/0001-Several-cleanups.patch b/0001-Several-cleanups.patch new file mode 100644 index 0000000..4bde8e2 --- /dev/null +++ b/0001-Several-cleanups.patch @@ -0,0 +1,162 @@ +From a6369519e080b741fea731ab42bb19e84c6e6fdb Mon Sep 17 00:00:00 2001 +From: Fabian Vogt +Date: Thu, 3 Aug 2017 09:02:14 +0200 +Subject: [PATCH 1/3] Several cleanups + +- No cppcheck warnings anymore +- Use snprintf everywhere +- Avoid pointless multiplication with sizeof(char) +- Avoid memory leaks +--- + pam_kwallet.c | 44 ++++++++++++++++++++++++++++++++------------ + 1 file changed, 32 insertions(+), 12 deletions(-) + +diff --git a/pam_kwallet.c b/pam_kwallet.c +index d88c5e0..cba57e7 100644 +--- a/pam_kwallet.c ++++ b/pam_kwallet.c +@@ -151,13 +151,14 @@ static int set_env(pam_handle_t *pamh, const char *name, const char *value) + //We do not return because pam_putenv might work + } + +- char *pamEnv = malloc(strlen(name) + strlen(value) + 2); //2 is for = and \0 ++ size_t pamEnvSize = strlen(name) + strlen(value) + 2; //2 is for = and \0 ++ char *pamEnv = malloc(pamEnvSize); + if (!pamEnv) { + pam_syslog(pamh, LOG_WARNING, "%s: Impossible to allocate memory for pamEnv", logPrefix); + return -1; + } + +- sprintf (pamEnv, "%s=%s", name, value); ++ snprintf (pamEnv, pamEnvSize, "%s=%s", name, value); + int ret = pam_putenv(pamh, pamEnv); + free(pamEnv); + +@@ -240,6 +241,11 @@ cleanup: + return result; + } + ++static void cleanup_free(pam_handle_t *pamh, void *ptr, int error_status) ++{ ++ free(ptr); ++} ++ + PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, const char **argv) + { + pam_syslog(pamh, LOG_INFO, "%s: pam_sm_authenticate\n", logPrefix); +@@ -297,14 +303,17 @@ PAM_EXTERN int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, cons + return PAM_IGNORE; + } + +- char *key = malloc(sizeof(char) * KWALLET_PAM_KEYSIZE); +- if (kwallet_hash(password, userInfo, key) != 0) { ++ char *key = malloc(KWALLET_PAM_KEYSIZE); ++ if (!key || kwallet_hash(password, userInfo, key) != 0) { ++ free(key); + pam_syslog(pamh, LOG_ERR, "%s: Fail into creating the hash", logPrefix); + return PAM_IGNORE; + } + +- result = pam_set_data(pamh, kwalletPamDataKey, key, NULL); ++ result = pam_set_data(pamh, kwalletPamDataKey, key, cleanup_free); ++ + if (result != PAM_SUCCESS) { ++ free(key); + pam_syslog(pamh, LOG_ERR, "%s: Impossible to store the hashed password: %s", logPrefix + , pam_strerror(pamh, result)); + return PAM_IGNORE; +@@ -385,9 +394,8 @@ cleanup: + static int better_write(int fd, const char *buffer, int len) + { + size_t writtenBytes = 0; +- int result; + while(writtenBytes < len) { +- result = write(fd, buffer + writtenBytes, len - writtenBytes); ++ int result = write(fd, buffer + writtenBytes, len - writtenBytes); + if (result < 0) { + if (errno != EAGAIN && errno != EINTR) { + return -1; +@@ -450,6 +458,7 @@ static void start_kwallet(pam_handle_t *pamh, struct passwd *userInfo, const cha + if (result != PAM_SUCCESS) { + pam_syslog(pamh, LOG_ERR, "%s: Impossible to set %s env, %s", + logPrefix, envVar, pam_strerror(pamh, result)); ++ free(fullSocket); + return; + } + +@@ -459,12 +468,15 @@ static void start_kwallet(pam_handle_t *pamh, struct passwd *userInfo, const cha + if (strlen(fullSocket) > sizeof(local.sun_path)) { + pam_syslog(pamh, LOG_ERR, "%s: socket path %s too long to open", + logPrefix, fullSocket); ++ free(fullSocket); + return; + } + strcpy(local.sun_path, fullSocket); ++ free(fullSocket); ++ fullSocket = NULL; + unlink(local.sun_path);//Just in case it exists from a previous login + +- pam_syslog(pamh, LOG_INFO, "%s: final socket path: %s", logPrefix, fullSocket); ++ pam_syslog(pamh, LOG_INFO, "%s: final socket path: %s", logPrefix, local.sun_path); + + size_t len = strlen(local.sun_path) + sizeof(local.sun_family); + if (bind(envSocket, (struct sockaddr *)&local, len) == -1) { +@@ -477,7 +489,7 @@ static void start_kwallet(pam_handle_t *pamh, struct passwd *userInfo, const cha + return; + } + +- if (chown(fullSocket, userInfo->pw_uid, userInfo->pw_gid) == -1) { ++ if (chown(local.sun_path, userInfo->pw_uid, userInfo->pw_gid) == -1) { + pam_syslog(pamh, LOG_INFO, "%s: Couldn't change ownership of the socket", logPrefix); + return; + } +@@ -655,7 +667,8 @@ int kwallet_hash(const char *passphrase, struct passwd *userInfo, char *key) + #else + char *fixpath = "share/apps/kwallet/kdewallet.salt"; + #endif +- char *path = (char*) malloc(strlen(userInfo->pw_dir) + strlen(kdehome) + strlen(fixpath) + 3);//3 == / and \0 ++ size_t pathSize = strlen(userInfo->pw_dir) + strlen(kdehome) + strlen(fixpath) + 3;//3 == /, / and \0 ++ char *path = (char*) malloc(pathSize); + sprintf(path, "%s/%s/%s", userInfo->pw_dir, kdehome, fixpath); + + struct stat info; +@@ -666,21 +679,26 @@ int kwallet_hash(const char *passphrase, struct passwd *userInfo, char *key) + FILE *fd = fopen(path, "r"); + if (fd == NULL) { + syslog(LOG_ERR, "%s: Couldn't open file: %s because: %d-%s", logPrefix, path, errno, strerror(errno)); ++ free(path); + return 1; + } +- salt = (char*) malloc(sizeof(char) * KWALLET_PAM_SALTSIZE); ++ salt = (char*) malloc(KWALLET_PAM_SALTSIZE); + memset(salt, '\0', KWALLET_PAM_SALTSIZE); + fread(salt, KWALLET_PAM_SALTSIZE, 1, fd); + fclose(fd); + } ++ free(path); ++ + if (salt == NULL) { + syslog(LOG_ERR, "%s-kwalletd: Couldn't create or read the salt file", logPrefix); + return 1; + } + + gcry_error_t error; ++ + error = gcry_control(GCRYCTL_INIT_SECMEM, 32768, 0); + if (error != 0) { ++ free(salt); + syslog(LOG_ERR, "%s-kwalletd: Can't get secure memory: %d", logPrefix, error); + return 1; + } +@@ -691,5 +709,7 @@ int kwallet_hash(const char *passphrase, struct passwd *userInfo, char *key) + GCRY_KDF_PBKDF2, GCRY_MD_SHA512, + salt, KWALLET_PAM_SALTSIZE, + KWALLET_PAM_ITERATIONS,KWALLET_PAM_KEYSIZE, key); +- return 0; ++ ++ free(salt); ++ return (int) error; // gcry_kdf_derive returns 0 on success + } +-- +2.13.2 + diff --git a/0002-Avoid-dropping-privileges-by-initializing-gcrypt-sec.patch b/0002-Avoid-dropping-privileges-by-initializing-gcrypt-sec.patch new file mode 100644 index 0000000..d77821a --- /dev/null +++ b/0002-Avoid-dropping-privileges-by-initializing-gcrypt-sec.patch @@ -0,0 +1,38 @@ +From a8153375a5006f5ca766b58a1a8f488699314a74 Mon Sep 17 00:00:00 2001 +From: Fabian Vogt +Date: Thu, 3 Aug 2017 09:27:10 +0200 +Subject: [PATCH 2/3] Avoid dropping privileges by initializing gcrypt secmem + +It's a documented side effect that initialization of secure memory in gcrypt +drops privileges if getuid() != geteuid(). This results in breaking setuid +callers, like sudo or su. +--- + pam_kwallet.c | 6 ++++++ + 1 file changed, 6 insertions(+) + +diff --git a/pam_kwallet.c b/pam_kwallet.c +index cba57e7..dc61115 100644 +--- a/pam_kwallet.c ++++ b/pam_kwallet.c +@@ -696,12 +696,18 @@ int kwallet_hash(const char *passphrase, struct passwd *userInfo, char *key) + + gcry_error_t error; + ++ /* We cannot call GCRYCTL_INIT_SECMEM as it drops privileges if getuid() != geteuid(). ++ * PAM modules are in many cases executed through setuid binaries, which this call ++ * would break. ++ * It was never effective anyway as neither key nor passphrase are in secure memory, ++ * which is a prerequisite for secure operation... + error = gcry_control(GCRYCTL_INIT_SECMEM, 32768, 0); + if (error != 0) { + free(salt); + syslog(LOG_ERR, "%s-kwalletd: Can't get secure memory: %d", logPrefix, error); + return 1; + } ++ */ + + gcry_control (GCRYCTL_INITIALIZATION_FINISHED, 0); + +-- +2.13.2 + diff --git a/0003-Check-for-a-graphical-session.patch b/0003-Check-for-a-graphical-session.patch new file mode 100644 index 0000000..1a5d015 --- /dev/null +++ b/0003-Check-for-a-graphical-session.patch @@ -0,0 +1,72 @@ +From f5f27799e1b6875be7f34edac3a9f98a2b550b2c Mon Sep 17 00:00:00 2001 +From: Fabian Vogt +Date: Thu, 3 Aug 2017 09:50:30 +0200 +Subject: [PATCH 3/3] Check for a graphical session + +Avoid running if it detects a text session. This can be overridden by adding +"force_run" as argument. +--- + pam_kwallet.c | 26 ++++++++++++++++++++++++++ + 1 file changed, 26 insertions(+) + +diff --git a/pam_kwallet.c b/pam_kwallet.c +index dc61115..34bc045 100644 +--- a/pam_kwallet.c ++++ b/pam_kwallet.c +@@ -72,6 +72,7 @@ const static char *kwalletd = NULL; + const static char *socketPath = NULL; + const static char *kwalletPamDataKey = NULL; + const static char *logPrefix = NULL; ++static int force_run = 0; + + #ifdef KWALLET5 + const static char *envVar = "PAM_KWALLET5_LOGIN"; +@@ -98,6 +99,8 @@ static void parseArguments(int argc, const char **argv) + kwalletd = argv[x] + 9; + } else if (strstr(argv[x], "socketPath=") != NULL) { + socketPath= argv[x] + 11; ++ } else if (strcmp(argv[x], "force_run") == 0) { ++ force_run = 1; + } + } + #ifdef KWALLET5 +@@ -241,6 +244,24 @@ cleanup: + return result; + } + ++static int is_graphical_session(pam_handle_t *pamh) ++{ ++ //Detect a graphical session ++ const char *pam_tty = NULL, *pam_xdisplay = NULL, ++ *xdg_session_type = NULL, *display = NULL; ++ ++ pam_get_item(pamh, PAM_TTY, (const void**) &pam_tty); ++#ifdef PAM_XDISPLAY ++ pam_get_item(pamh, PAM_XDISPLAY, (const void**) &pam_xdisplay); ++#endif ++ xdg_session_type = get_env(pamh, "XDG_SESSION_TYPE"); ++ ++ return (pam_xdisplay && strlen(pam_xdisplay) != 0) ++ || (pam_tty && pam_tty[0] == ':') ++ || (xdg_session_type && strcmp(xdg_session_type, "x11") == 0) ++ || (xdg_session_type && strcmp(xdg_session_type, "wayland") == 0); ++} ++ + static void cleanup_free(pam_handle_t *pamh, void *ptr, int error_status) + { + free(ptr); +@@ -537,6 +558,11 @@ PAM_EXTERN int pam_sm_open_session(pam_handle_t *pamh, int flags, int argc, cons + + parseArguments(argc, argv); + ++ if (!force_run && !is_graphical_session(pamh)) { ++ pam_syslog(pamh, LOG_INFO, "%s: not a graphical session, skipping. Use force_run parameter to ignore this.", logPrefix); ++ return PAM_IGNORE; ++ } ++ + int result; + result = pam_set_data(pamh, "sm_open_session", "1", NULL); + if (result != PAM_SUCCESS) { +-- +2.13.2 + diff --git a/pam_kwallet.changes b/pam_kwallet.changes index be16e71..dc91559 100644 --- a/pam_kwallet.changes +++ b/pam_kwallet.changes @@ -1,3 +1,12 @@ +------------------------------------------------------------------- +Wed Aug 9 08:39:00 UTC 2017 - fabian@ritter-vogt.de + +- Add various patches (pending upstream review) to fix several + issues: + * 0001-Several-cleanups.patch + * 0002-Avoid-dropping-privileges-by-initializing-gcrypt-sec.patch + * 0003-Check-for-a-graphical-session.patch + ------------------------------------------------------------------- Wed Aug 2 08:48:28 UTC 2017 - fabian@ritter-vogt.de diff --git a/pam_kwallet.spec b/pam_kwallet.spec index b9cf07d..68db0f3 100644 --- a/pam_kwallet.spec +++ b/pam_kwallet.spec @@ -25,6 +25,12 @@ Group: System/GUI/KDE Url: http://www.kde.org/ Source0: http://download.kde.org/stable/plasma/%{version}/kwallet-pam-%{version}.tar.xz Source1: baselibs.conf +# PATCH-FIX-UPSTREAM +Patch1: 0001-Several-cleanups.patch +# PATCH-FIX-UPSTREAM +Patch2: 0002-Avoid-dropping-privileges-by-initializing-gcrypt-sec.patch +# PATCH-FIX-UPSTREAM +Patch3: 0003-Check-for-a-graphical-session.patch BuildRequires: extra-cmake-modules >= 1.2.0 BuildRequires: kf5-filesystem BuildRequires: libgcrypt-devel >= 1.5.0 @@ -52,6 +58,9 @@ module. %prep %setup -q -n kwallet-pam-%{version} +%patch1 -p1 +%patch2 -p1 +%patch3 -p1 %build %cmake_kf5 -d build -- -DLIBEXEC_INSTALL_DIR=%{_kf5_libexecdir} -DCMAKE_INSTALL_PREFIX=/