Accepting request 1208491 from security
OBS-URL: https://build.opensuse.org/request/show/1208491 OBS-URL: https://build.opensuse.org/package/show/openSUSE:Factory/oath-toolkit?expand=0&rev=15
This commit is contained in:
Ana Guerrero
Git OBS Bridge
commit
e6406e274c
@ -1,8 +1,7 @@
|
||||
From 4302b149a186ba8ca155ea7e211c25fac112a3ef Mon Sep 17 00:00:00 2001
|
||||
From 345ae06e0f698bdb1e9b4529e5a882f12df04426 Mon Sep 17 00:00:00 2001
|
||||
From: Matthias Gerstner <matthias.gerstner@suse.de>
|
||||
Date: Wed, 11 Sep 2024 14:09:25 +0200
|
||||
Date: Wed, 16 Oct 2024 09:58:35 +0200
|
||||
Subject: [PATCH] usersfile: fix potential security issues in PAM module
|
||||
context (CVE-2024-47191)
|
||||
|
||||
With the addition of the possibility to place a usersfile also into
|
||||
a user's home directory via variable expansion of ${HOME} and ${USER} in
|
||||
@ -68,11 +67,28 @@ for this is that locks obtained with F_SETLK cannot be inherited to
|
||||
child processes, which we need to do now. flock() would also have been
|
||||
an alternative, but it has unfortunate properties if the lockfile should
|
||||
be located on a network file system.
|
||||
|
||||
This is a follow-up version of the patch that addresses a few
|
||||
shortcomings of the originally shared patch:
|
||||
|
||||
- setgroups() is invoked to drop supplementary group membership. Without
|
||||
this the forked process wrongly retains the root group membership.
|
||||
Since the privilege drop is only an additional hardening measure, the
|
||||
original patch should still prove safe.
|
||||
- the usersfile is checked for additional hard-links; if the link count
|
||||
is not zero, then the file is rejected. This prevents possible hard
|
||||
link attacks on the end of the unprivileged user. With the Linux
|
||||
kernel sysctl protected_hardlinks set to 1 (the usual default on most
|
||||
distributions), this attack will not work either way.
|
||||
- O_NOCTTY has been added to the open() call of the usersfile. This
|
||||
makes this aspect explicit, although the code already checks that the
|
||||
file is a regular file, so the situation shouldn't arise in the first
|
||||
place.
|
||||
---
|
||||
liboath/errors.c | 7 +-
|
||||
liboath/oath.h.in | 7 +-
|
||||
liboath/usersfile.c | 694 ++++++++++++++++++++++++++++++++++++--------
|
||||
3 files changed, 580 insertions(+), 128 deletions(-)
|
||||
liboath/oath.h.in | 8 +-
|
||||
liboath/usersfile.c | 706 ++++++++++++++++++++++++++++++++++++--------
|
||||
3 files changed, 593 insertions(+), 128 deletions(-)
|
||||
|
||||
diff --git a/liboath/errors.c b/liboath/errors.c
|
||||
index c1725f9..67ed008 100644
|
||||
@ -93,10 +109,10 @@ index c1725f9..67ed008 100644
|
||||
|
||||
/**
|
||||
diff --git a/liboath/oath.h.in b/liboath/oath.h.in
|
||||
index b8b4fbd..dc3e973 100644
|
||||
index b8b4fbd..5ad7045 100644
|
||||
--- a/liboath/oath.h.in
|
||||
+++ b/liboath/oath.h.in
|
||||
@@ -152,9 +152,14 @@ extern "C"
|
||||
@@ -152,9 +152,15 @@ extern "C"
|
||||
OATH_FILE_CLOSE_ERROR = -25,
|
||||
OATH_FILE_CHOWN_ERROR = -26,
|
||||
OATH_FILE_STAT_ERROR = -27,
|
||||
@ -105,18 +121,19 @@ index b8b4fbd..dc3e973 100644
|
||||
+ OATH_WAIT_ERROR = -30,
|
||||
+ OATH_SETUID_ERROR = -31,
|
||||
+ OATH_SETGID_ERROR = -32,
|
||||
+ OATH_SETGROUPS_ERROR = -33,
|
||||
/* When adding anything here, update OATH_LAST_ERROR, errors.c
|
||||
and tests/tst_errors.c. */
|
||||
- OATH_LAST_ERROR = -27
|
||||
+ OATH_LAST_ERROR = -33
|
||||
+ OATH_LAST_ERROR = -34
|
||||
} oath_rc;
|
||||
|
||||
/* Global */
|
||||
diff --git a/liboath/usersfile.c b/liboath/usersfile.c
|
||||
index 3b139d1..5bd3700 100644
|
||||
index 3b139d1..c9625f4 100644
|
||||
--- a/liboath/usersfile.c
|
||||
+++ b/liboath/usersfile.c
|
||||
@@ -29,10 +29,229 @@
|
||||
@@ -29,10 +29,239 @@
|
||||
#include <unistd.h> /* For ssize_t. */
|
||||
#include <fcntl.h> /* For fcntl. */
|
||||
#include <errno.h> /* For errno. */
|
||||
@ -124,6 +141,7 @@ index 3b139d1..5bd3700 100644
|
||||
#include <sys/stat.h> /* For S_IRUSR, S_IWUSR. */
|
||||
+#include <sys/wait.h> /* For wait */
|
||||
+#include <sys/stat.h> /* For stat */
|
||||
+#include <grp.h> /* For setgroups */
|
||||
|
||||
#ifndef _WIN32
|
||||
|
||||
@ -264,7 +282,7 @@ index 3b139d1..5bd3700 100644
|
||||
+ * recovery mechanism if we obtain a lock on the file, but the file has
|
||||
+ * already been replaced by a new version. This situation is detected by
|
||||
+ * opening the file again after the lock has been placed and comparing the
|
||||
+ * inode numbers. If the no longer match, then the new file has to be locked
|
||||
+ * inode numbers. If they no longer match, then the new file has to be locked
|
||||
+ * instead.
|
||||
+ *
|
||||
+ * On successful return ctx->fd will be valid and locked and ctx->st will
|
||||
@ -282,6 +300,15 @@ index 3b139d1..5bd3700 100644
|
||||
+ if (fstat(ctx->fd, &ctx->st) != 0)
|
||||
+ return OATH_FILE_STAT_ERROR;
|
||||
+
|
||||
+ /* don't allow hard-linked files, which would allow to fool our logic -
|
||||
+ * this can only happen if protected_hardlinks is disabled in the kernel,
|
||||
+ * though */
|
||||
+ if (ctx->st.st_nlink > 1)
|
||||
+ {
|
||||
+ close(ctx->fd);
|
||||
+ return OATH_FILE_OPEN_ERROR;
|
||||
+ }
|
||||
+
|
||||
+ /* lock and retry opening until all is consistent, abort after a couple of
|
||||
+ * times, it's unlikely that we race all the time (could be a DoS attempt) */
|
||||
+ for (int i = 0; i < 5; i++)
|
||||
@ -293,7 +320,7 @@ index 3b139d1..5bd3700 100644
|
||||
+ /* we need to open it read-write for write-locking it via fcntl(),
|
||||
+ * otherwise we wouldn't need write access for the file, since we'll
|
||||
+ * atomically replace it with a new one. */
|
||||
+ int err = reopen_path_fd(&ctx->fd, O_RDWR|O_CLOEXEC);
|
||||
+ int err = reopen_path_fd(&ctx->fd, O_RDWR|O_CLOEXEC|O_NOCTTY);
|
||||
+ if (err != OATH_OK)
|
||||
+ return err;
|
||||
+
|
||||
@ -346,7 +373,7 @@ index 3b139d1..5bd3700 100644
|
||||
static int
|
||||
parse_type (const char *str, unsigned *digits, unsigned *totpstepsize)
|
||||
{
|
||||
@@ -298,8 +517,92 @@ update_usersfile2 (const char *username,
|
||||
@@ -298,8 +527,92 @@ update_usersfile2 (const char *username,
|
||||
return OATH_OK;
|
||||
}
|
||||
|
||||
@ -440,7 +467,7 @@ index 3b139d1..5bd3700 100644
|
||||
const char *username,
|
||||
const char *otp,
|
||||
FILE *infh,
|
||||
@@ -307,9 +610,7 @@ update_usersfile (const char *usersfile,
|
||||
@@ -307,9 +620,7 @@ update_usersfile (const char *usersfile,
|
||||
size_t *n, char *timestamp, uint64_t new_moving_factor,
|
||||
size_t skipped_users)
|
||||
{
|
||||
@ -450,7 +477,7 @@ index 3b139d1..5bd3700 100644
|
||||
|
||||
/* Rewind input file. */
|
||||
{
|
||||
@@ -321,112 +622,236 @@ update_usersfile (const char *usersfile,
|
||||
@@ -321,112 +632,236 @@ update_usersfile (const char *usersfile,
|
||||
clearerr (infh);
|
||||
}
|
||||
|
||||
@ -564,7 +591,9 @@ index 3b139d1..5bd3700 100644
|
||||
+ rc = OATH_FILE_SYNC_ERROR;
|
||||
+ goto out;
|
||||
+ }
|
||||
+
|
||||
|
||||
- if (rc == OATH_OK && fstat (fileno (infh), &insb) == -1)
|
||||
- rc = OATH_FILE_STAT_ERROR;
|
||||
+out:
|
||||
+ if (outfd >= 0)
|
||||
+ close(outfd);
|
||||
@ -575,8 +604,10 @@ index 3b139d1..5bd3700 100644
|
||||
+ return rc;
|
||||
+}
|
||||
|
||||
- if (rc == OATH_OK && fstat (fileno (infh), &insb) == -1)
|
||||
- rc = OATH_FILE_STAT_ERROR;
|
||||
- if (rc == OATH_OK
|
||||
- && fchown (fileno (outfh), insb.st_uid, insb.st_gid) != 0)
|
||||
- rc = OATH_FILE_CHOWN_ERROR;
|
||||
- }
|
||||
+static int
|
||||
+oath_process_usersfile (struct usersfile_ctx *ctx,
|
||||
+ const char *username,
|
||||
@ -591,29 +622,28 @@ index 3b139d1..5bd3700 100644
|
||||
+ int rc;
|
||||
+ size_t skipped_users;
|
||||
|
||||
- if (rc == OATH_OK
|
||||
- && fchown (fileno (outfh), insb.st_uid, insb.st_gid) != 0)
|
||||
- rc = OATH_FILE_CHOWN_ERROR;
|
||||
- }
|
||||
- /* On success, flush the buffers. */
|
||||
- if (rc == OATH_OK && fflush (outfh) != 0)
|
||||
- rc = OATH_FILE_FLUSH_ERROR;
|
||||
+ infh = fdopen (ctx->fd, "r");
|
||||
+ if (infh == NULL)
|
||||
+ return OATH_FILE_OPEN_ERROR;
|
||||
|
||||
- /* On success, flush the buffers. */
|
||||
- if (rc == OATH_OK && fflush (outfh) != 0)
|
||||
- rc = OATH_FILE_FLUSH_ERROR;
|
||||
+ /* ownership has been transferred to the FILE stream now */
|
||||
+ ctx->fd = -1;
|
||||
|
||||
- /* On success, sync the disks. */
|
||||
- if (rc == OATH_OK && fsync (fileno (outfh)) != 0)
|
||||
- rc = OATH_FILE_SYNC_ERROR;
|
||||
+ rc = parse_usersfile (username, otp, window, passwd, last_otp,
|
||||
+ infh, &line, &n, &new_moving_factor, &skipped_users);
|
||||
+ /* ownership has been transferred to the FILE stream now */
|
||||
+ ctx->fd = -1;
|
||||
|
||||
- /* Close the file regardless of success. */
|
||||
- if (fclose (outfh) != 0)
|
||||
- rc = OATH_FILE_CLOSE_ERROR;
|
||||
+ rc = parse_usersfile (username, otp, window, passwd, last_otp,
|
||||
+ infh, &line, &n, &new_moving_factor, &skipped_users);
|
||||
|
||||
- /* On success, overwrite the usersfile with the new copy. */
|
||||
- if (rc == OATH_OK && rename (newfilename, usersfile) != 0)
|
||||
- rc = OATH_FILE_RENAME_ERROR;
|
||||
+ if (rc == OATH_OK)
|
||||
+ {
|
||||
+ char timestamp[30];
|
||||
@ -622,15 +652,12 @@ index 3b139d1..5bd3700 100644
|
||||
+ time_t t;
|
||||
+ size_t l;
|
||||
|
||||
- /* On success, overwrite the usersfile with the new copy. */
|
||||
- if (rc == OATH_OK && rename (newfilename, usersfile) != 0)
|
||||
- rc = OATH_FILE_RENAME_ERROR;
|
||||
+ if (time (&t) == (time_t) - 1)
|
||||
+ return OATH_TIME_ERROR;
|
||||
|
||||
- /* Something has failed, don't leave garbage lying around. */
|
||||
- if (rc != OATH_OK)
|
||||
- unlink (newfilename);
|
||||
+ if (time (&t) == (time_t) - 1)
|
||||
+ return OATH_TIME_ERROR;
|
||||
+
|
||||
+ if (localtime_r (&t, &now) == NULL)
|
||||
+ return OATH_TIME_ERROR;
|
||||
|
||||
@ -638,6 +665,11 @@ index 3b139d1..5bd3700 100644
|
||||
+ l = strftime (timestamp, max, TIME_FORMAT_STRING, &now);
|
||||
+ if (l != 20)
|
||||
+ return OATH_TIME_ERROR;
|
||||
+
|
||||
+ rc = update_usersfile (ctx, username, otp, infh,
|
||||
+ &line, &n, timestamp, new_moving_factor,
|
||||
+ skipped_users);
|
||||
+ }
|
||||
|
||||
- /* Complete, close the lockfile */
|
||||
- if (fclose (lockfh) != 0)
|
||||
@ -645,11 +677,6 @@ index 3b139d1..5bd3700 100644
|
||||
- if (unlink (lockfile) != 0)
|
||||
- rc = OATH_FILE_UNLINK_ERROR;
|
||||
- free (lockfile);
|
||||
+ rc = update_usersfile (ctx, username, otp, infh,
|
||||
+ &line, &n, timestamp, new_moving_factor,
|
||||
+ skipped_users);
|
||||
+ }
|
||||
+
|
||||
+ free (line);
|
||||
+ fclose (infh);
|
||||
|
||||
@ -769,7 +796,7 @@ index 3b139d1..5bd3700 100644
|
||||
/**
|
||||
* oath_authenticate_usersfile:
|
||||
* @usersfile: string with user credential filename, in UsersFile format
|
||||
@@ -460,52 +885,69 @@ oath_authenticate_usersfile (const char *usersfile,
|
||||
@@ -460,52 +895,71 @@ oath_authenticate_usersfile (const char *usersfile,
|
||||
size_t window,
|
||||
const char *passwd, time_t *last_otp)
|
||||
{
|
||||
@ -846,6 +873,8 @@ index 3b139d1..5bd3700 100644
|
||||
+ if (cpid == 0)
|
||||
+ {
|
||||
+ /* child */
|
||||
+ if (setgroups(0, NULL) != 0)
|
||||
+ exit (abs(OATH_SETGROUPS_ERROR));
|
||||
+ if (setgid (ctx.st.st_gid) != 0)
|
||||
+ exit (abs(OATH_SETGID_ERROR));
|
||||
+ if (setuid (ctx.st.st_uid) != 0)
|
||||
@ -881,5 +910,5 @@ index 3b139d1..5bd3700 100644
|
||||
|
||||
#else /* _WIN32 */
|
||||
--
|
||||
2.44.2
|
||||
2.45.2
|
||||
|
||||
|
||||
@ -1,3 +1,9 @@
|
||||
-------------------------------------------------------------------
|
||||
Wed Oct 16 14:24:27 UTC 2024 - Jan Zerebecki <jan.suse@zerebecki.de>
|
||||
|
||||
- Update 0001-usersfile-fix-potential-security-issues-in-PAM-modul.patch
|
||||
with bsc#1231699 improvements for security fix CVE-2024-47191
|
||||
|
||||
-------------------------------------------------------------------
|
||||
Fri Sep 13 15:10:22 UTC 2024 - Jan Zerebecki <jan.suse@zerebecki.de>
|
||||
|
||||
|
||||
Loading…
Reference in New Issue
Block a user