Skip to content

Commit

Permalink
kde-plasma/kwallet-pam: Fix memleaks and dropping privileges
Browse files Browse the repository at this point in the history
Package-Manager: Portage-2.3.8, Repoman-2.3.3
  • Loading branch information
a17r committed Aug 31, 2017
1 parent 20d6b0a commit de57d49
Show file tree
Hide file tree
Showing 4 changed files with 368 additions and 0 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
From f3b230f7f3bf39dc46b97a216aa7c28595d20a7a Mon Sep 17 00:00:00 2001
From: Fabian Vogt <[email protected]>
Date: Thu, 3 Aug 2017 09:50:30 +0200
Subject: Check for a graphical session

Summary:
Avoid running if it detects a text session. This can be overridden by adding
"force_run" as argument.

Test Plan:
Put pam_kwallet5.so as optional in a global common-session pam file
that is included by all other services. It is not invoked when logging in from
a tty with getty, sudo or su and still works when using SDDM. When adding
force_run it runs in all cases.

Reviewers: #plasma

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D7125
---
pam_kwallet.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/pam_kwallet.c b/pam_kwallet.c
index cba57e7..46720a5 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
@@ -246,6 +249,24 @@ static void cleanup_free(pam_handle_t *pamh, void *ptr, int error_status)
free(ptr);
}

+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);
+}
+
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);
@@ -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) {
--
cgit v0.11.2

173 changes: 173 additions & 0 deletions kde-plasma/kwallet-pam/files/kwallet-pam-5.10.5-cleanups.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
From a33ec22b96e837899528b05963eae8ea6b01171a Mon Sep 17 00:00:00 2001
From: Fabian Vogt <[email protected]>
Date: Thu, 3 Aug 2017 09:02:14 +0200
Subject: Several cleanups

Summary:
- No cppcheck warnings anymore
- Use snprintf everywhere
- Avoid pointless multiplication with sizeof(char)
- Avoid memory leaks

Test Plan: Still builds, works the same as before.

Reviewers: #plasma

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D7123
---
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
}
--
cgit v0.11.2

49 changes: 49 additions & 0 deletions kde-plasma/kwallet-pam/files/kwallet-pam-5.10.5-privileges.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
From 1a01e1eb870e1ab1d96a8641f1f3500af646c974 Mon Sep 17 00:00:00 2001
From: Fabian Vogt <[email protected]>
Date: Thu, 3 Aug 2017 09:27:10 +0200
Subject: Avoid dropping privileges by initializing gcrypt secmem

Summary:
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.

Test Plan: Can use sudo again when pam_kwallet is involved.

Reviewers: #plasma

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D7124
---
pam_kwallet.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/pam_kwallet.c b/pam_kwallet.c
index 46720a5..20d9603 100644
--- a/pam_kwallet.c
+++ b/pam_kwallet.c
@@ -722,12 +722,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);

--
cgit v0.11.2

59 changes: 59 additions & 0 deletions kde-plasma/kwallet-pam/kwallet-pam-5.10.5-r1.ebuild
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# Copyright 1999-2017 Gentoo Foundation
# Distributed under the terms of the GNU General Public License v2

EAPI=6

inherit kde5

DESCRIPTION="KWallet PAM module to not enter password again"
LICENSE="LGPL-2.1"
KEYWORDS="~amd64 ~arm ~x86"
IUSE=""

DEPEND="
dev-libs/libgcrypt:0=
virtual/pam
"
RDEPEND="${DEPEND}
net-misc/socat
"

PATCHES=(
"${FILESDIR}/${P}-cleanups.patch"
"${FILESDIR}/${P}-check-graphical.patch"
"${FILESDIR}/${P}-privileges.patch"
)

src_configure() {
local mycmakeargs=(
-DCMAKE_INSTALL_LIBDIR="/$(get_libdir)"
-DKWALLET4=0
)
kde5_src_configure
}

pkg_postinst() {
check_dm() {
if [[ -e "${ROOT}${2}" ]] ; then
if grep -Eq "auth\s+optional\s+pam_kwallet5.so" "${ROOT}${2}" && \
grep -Eq "session\s+optional\s+pam_kwallet5.so" "${ROOT}${2}" ; then
elog " ${1} - ${2} ...GOOD"
else
ewarn " ${1} - ${2} ...BAD"
fi
fi
}
elog "This package enables auto-unlocking of kde-frameworks/kwallet:5."
elog "List of things to make it work:"
elog "1. Use standard blowfish encryption instead of GPG"
elog "2. Use same password for login and kwallet"
elog "3. A display manager with support for PAM"
elog "4.a Have the following lines in the display manager's pam.d file:"
elog " -auth optional pam_kwallet5.so"
elog " -session optional pam_kwallet5.so auto_start"
elog "4.b Checking installed DMs..."
has_version "x11-misc/sddm" && check_dm "SDDM" "/etc/pam.d/sddm"
has_version "x11-misc/lightdm" && check_dm "LightDM" "/etc/pam.d/lightdm"
elog
elog "See also: https://wiki.gentoo.org/wiki/KDE#KWallet_auto-unlocking"
}

0 comments on commit de57d49

Please sign in to comment.