Skip to content

screen: fixes for CVE-2025-4680[245] (r151054) #3922

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

Merged
merged 1 commit into from
May 23, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion build/screen/build.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@
# }}}
#
# Copyright 2017 OmniTI Computer Consulting, Inc. All rights reserved.
# Copyright 2024 OmniOS Community Edition (OmniOSce) Association.
# Copyright 2025 OmniOS Community Edition (OmniOSce) Association.

. ../../lib/build.sh

PROG=screen
VER=4.9.1
DASHREV=1
PKG=terminal/screen
SUMMARY="GNU Screen terminal multiplexer"
DESC="A full-screen window manager that multiplexes a physical "
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
From 089b97333844e05a4693a3ee726ac6bdb2d6bf59 Mon Sep 17 00:00:00 2001
From: Matthias Gerstner <[email protected]>
Date: Wed, 7 May 2025 12:59:05 +0200
Subject: [PATCH 1/3] attacher.c: prevent temporary 0666 mode on PTYs to fix
CVE-2025-46802

This temporary chmod of the PTY to mode 0666 is most likely a remnant of
past times, before the PTY file descriptor was passed to the target
session via the UNIX domain socket.

This chmod() causes a race condition during which any other user in the
system can open the PTY for reading and writing, and thus allows PTY
hijacking.

Simply remove this logic completely.
---
attacher.c | 27 ---------------------------
screen.c | 19 -------------------
2 files changed, 46 deletions(-)

diff --git a/attacher.c b/attacher.c
index c35ae7a..16b151e 100644
--- a/attacher.c
+++ b/attacher.c
@@ -73,7 +73,6 @@ extern int MasterPid, attach_fd;
#ifdef MULTIUSER
extern char *multi;
extern int multiattach, multi_uid, own_uid;
-extern int tty_mode, tty_oldmode;
# ifndef USE_SETEUID
static int multipipe[2];
# endif
@@ -160,9 +159,6 @@ int how;

if (pipe(multipipe))
Panic(errno, "pipe");
- if (chmod(attach_tty, 0666))
- Panic(errno, "chmod %s", attach_tty);
- tty_oldmode = tty_mode;
eff_uid = -1; /* make UserContext fork */
real_uid = multi_uid;
if ((ret = UserContext()) <= 0)
@@ -174,11 +170,6 @@ int how;
Panic(errno, "UserContext");
close(multipipe[1]);
read(multipipe[0], &dummy, 1);
- if (tty_oldmode >= 0)
- {
- chmod(attach_tty, tty_oldmode);
- tty_oldmode = -1;
- }
ret = UserStatus();
#ifdef LOCK
if (ret == SIG_LOCK)
@@ -224,9 +215,6 @@ int how;
xseteuid(multi_uid);
xseteuid(own_uid);
#endif
- if (chmod(attach_tty, 0666))
- Panic(errno, "chmod %s", attach_tty);
- tty_oldmode = tty_mode;
}
# endif /* USE_SETEUID */
#endif /* MULTIUSER */
@@ -423,13 +411,6 @@ int how;
ContinuePlease = 0;
# ifndef USE_SETEUID
close(multipipe[1]);
-# else
- xseteuid(own_uid);
- if (tty_oldmode >= 0)
- if (chmod(attach_tty, tty_oldmode))
- Panic(errno, "chmod %s", attach_tty);
- tty_oldmode = -1;
- xseteuid(real_uid);
# endif
}
#endif
@@ -505,14 +486,6 @@ AttacherFinit SIGDEFARG
close(s);
}
}
-#ifdef MULTIUSER
- if (tty_oldmode >= 0)
- {
- if (setuid(own_uid))
- Panic(errno, "setuid");
- chmod(attach_tty, tty_oldmode);
- }
-#endif
exit(0);
SIGRETURN;
}
diff --git a/screen.c b/screen.c
index 7653cd1..1a23e1a 100644
--- a/screen.c
+++ b/screen.c
@@ -230,8 +230,6 @@ char *multi_home;
int multi_uid;
int own_uid;
int multiattach;
-int tty_mode;
-int tty_oldmode = -1;
#endif

char HostName[MAXSTR];
@@ -1009,9 +1007,6 @@ int main(int ac, char** av)

/* ttyname implies isatty */
SetTtyname(true, &st);
-#ifdef MULTIUSER
- tty_mode = (int)st.st_mode & 0777;
-#endif

fl = fcntl(0, F_GETFL, 0);
if (fl != -1 && (fl & (O_RDWR|O_RDONLY|O_WRONLY)) == O_RDWR)
@@ -2170,20 +2165,6 @@ DEFINE_VARARGS_FN(Panic)
if (D_userpid)
Kill(D_userpid, SIG_BYE);
}
-#ifdef MULTIUSER
- if (tty_oldmode >= 0) {
-
-# ifdef USE_SETEUID
- if (setuid(own_uid))
- xseteuid(own_uid); /* may be a loop. sigh. */
-# else
- setuid(own_uid);
-# endif
-
- debug1("Panic: changing back modes from %s\n", attach_tty);
- chmod(attach_tty, tty_oldmode);
- }
-#endif
eexit(1);
}

--
2.49.0

Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
From 1e2a17d8437e5f284daeae54f0b4722b48eb0e30 Mon Sep 17 00:00:00 2001
From: Matthias Gerstner <[email protected]>
Date: Wed, 7 May 2025 13:12:53 +0200
Subject: [PATCH 2/3] Avoid file existence test information leaks to fix
CVE-2025-46804

In setuid-root context the current error messages give away whether
certain paths not accessible by the real user exist and what type they
have. To prevent this only output generic error messages in setuid-root
context.

In some situations, when an error is pertaining a directory and the
directory is owner by the real user then we can still output more
detailed diagnostics.

This change can lead to less helpful error messages when Screen is
install setuid-root. More complex changes would be needed to avoid this
(e.g. only open the `SocketPath` with raised privileges when
multi-attach is requested).

There might still be lingering some code paths that allow such
information leaks, since `SocketPath` is a global variable that is used
across the code base. The majority of issues should be caught with this
fix, however.
---
screen.c | 43 +++++++++++++++++++++++++++++++++----------
socket.c | 9 +++++++--
2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/screen.c b/screen.c
index 1a23e1a..024239d 100644
--- a/screen.c
+++ b/screen.c
@@ -1122,15 +1122,28 @@ int main(int ac, char** av)
#endif
}

- if (stat(SockPath, &st) == -1)
- Panic(errno, "Cannot access %s", SockPath);
- else
- if (!S_ISDIR(st.st_mode))
+ if (stat(SockPath, &st) == -1) {
+ if (eff_uid == real_uid) {
+ Panic(errno, "Cannot access %s", SockPath);
+ } else {
+ Panic(0, "Error accessing %s", SockPath);
+ }
+ } else if (!S_ISDIR(st.st_mode)) {
+ if (eff_uid == real_uid || st.st_uid == real_uid) {
Panic(0, "%s is not a directory.", SockPath);
+ } else {
+ Panic(0, "Error accessing %s", SockPath);
+ }
+ }
#ifdef MULTIUSER
if (multi) {
- if ((int)st.st_uid != multi_uid)
- Panic(0, "%s is not the owner of %s.", multi, SockPath);
+ if ((int)st.st_uid != multi_uid) {
+ if (eff_uid == real_uid || st.st_uid == real_uid) {
+ Panic(0, "%s is not the owner of %s.", multi, SockPath);
+ } else {
+ Panic(0, "Error accessing %s", SockPath);
+ }
+ }
}
else
#endif
@@ -1145,8 +1158,13 @@ int main(int ac, char** av)
#endif
}

- if ((st.st_mode & 0777) != 0700)
- Panic(0, "Directory %s must have mode 700.", SockPath);
+ if ((st.st_mode & 0777) != 0700) {
+ if (eff_uid == real_uid || st.st_uid == real_uid) {
+ Panic(0, "Directory %s must have mode 700.", SockPath);
+ } else {
+ Panic(0, "Error accessing %s", SockPath);
+ }
+ }
if (SockMatch && index(SockMatch, '/'))
Panic(0, "Bad session name '%s'", SockMatch);
SockName = SockPath + strlen(SockPath) + 1;
@@ -1184,8 +1202,13 @@ int main(int ac, char** av)
else
exit(9 + (fo || oth ? 1 : 0) + fo);
}
- if (fo == 0)
- Panic(0, "No Sockets found in %s.\n", SockPath);
+ if (fo == 0) {
+ if (eff_uid == real_uid || st.st_uid == real_uid) {
+ Panic(0, "No Sockets found in %s.\n", SockPath);
+ } else {
+ Panic(0, "Error accessing %s", SockPath);
+ }
+ }
Msg(0, "%d Socket%s in %s.", fo, fo > 1 ? "s" : "", SockPath);
eexit(0);
}
diff --git a/socket.c b/socket.c
index 54d8cb8..6c3502f 100644
--- a/socket.c
+++ b/socket.c
@@ -169,8 +169,13 @@ bool *is_sock;
xsetegid(real_gid);
#endif

- if ((dirp = opendir(SockPath)) == 0)
- Panic(errno, "Cannot opendir %s", SockPath);
+ if ((dirp = opendir(SockPath)) == 0) {
+ if (eff_uid == real_uid) {
+ Panic(errno, "Cannot opendir %s", SockPath);
+ } else {
+ Panic(0, "Error accessing %s", SockPath);
+ }
+ }

slist = 0;
slisttail = &slist;
--
2.49.0

Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
From f5d0230db7f61e1494573cbc846177464eb322d4 Mon Sep 17 00:00:00 2001
From: Matthias Gerstner <[email protected]>
Date: Wed, 7 May 2025 15:03:38 +0200
Subject: [PATCH 3/3] socket.c: don't send signals with root privileges to fix
CVE-2025-46805

The CheckPid() function was introduced to address CVE-2023-24626, to
prevent sending SIGCONT and SIGHUP to arbitrary PIDs in the system. This
fix still suffers from a TOCTOU race condition. The client can replace
itself by a privileged process, or try to cycle PIDs until a privileged
process receives the original PID.

To prevent this, always send signals using the real privileges. Keep
CheckPid() for error diagnostics. If sending the actual signal fails
later on then there will be no more error reporting.

It seems the original bugfix already introduced a regression when
attaching to another's user session that is not owned by root. In this
case the target sessions runs with real uid X, while for sending a
signal to the `pid` provided by the client real uid Y (or root
privileges) are required.

This is hard to properly fix without this regression. On Linux pidfds
could be used to allow safely sending signals to other PIDs as root
without involving race conditions. In this case the client PID should
also be obtained via the UNIX domain socket's SO_PEERCRED option,
though.
---
socket.c | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/socket.c b/socket.c
index 6c3502f..d6621fa 100644
--- a/socket.c
+++ b/socket.c
@@ -831,6 +831,11 @@ int pid;
return UserStatus();
}

+static void KillUnpriv(pid_t pid, int sig) {
+ UserContext();
+ UserReturn(kill(pid, sig));
+}
+
#ifdef hpux
/*
* From: "F. K. Bruner" <[email protected]>
@@ -916,14 +921,14 @@ struct win *wi;
{
Msg(errno, "Could not perform necessary sanity checks on pts device.");
close(i);
- Kill(pid, SIG_BYE);
+ KillUnpriv(pid, SIG_BYE);
return -1;
}
if (strcmp(ttyname_in_ns, m->m_tty))
{
Msg(errno, "Attach: passed fd does not match tty: %s - %s!", ttyname_in_ns, m->m_tty[0] != '\0' ? m->m_tty : "(null)");
close(i);
- Kill(pid, SIG_BYE);
+ KillUnpriv(pid, SIG_BYE);
return -1;
}
/* m->m_tty so far contains the actual name of the pts device in the
@@ -940,19 +945,19 @@ struct win *wi;
{
Msg(errno, "Attach: passed fd does not match tty: %s - %s!", m->m_tty, myttyname ? myttyname : "NULL");
close(i);
- Kill(pid, SIG_BYE);
+ KillUnpriv(pid, SIG_BYE);
return -1;
}
}
else if ((i = secopen(m->m_tty, O_RDWR | O_NONBLOCK, 0)) < 0)
{
Msg(errno, "Attach: Could not open %s!", m->m_tty);
- Kill(pid, SIG_BYE);
+ KillUnpriv(pid, SIG_BYE);
return -1;
}
#ifdef MULTIUSER
if (attach)
- Kill(pid, SIGCONT);
+ KillUnpriv(pid, SIGCONT);
#endif

#if defined(ultrix) || defined(pyr) || defined(NeXT)
@@ -965,7 +970,7 @@ struct win *wi;
{
write(i, "Attaching from inside of screen?\n", 33);
close(i);
- Kill(pid, SIG_BYE);
+ KillUnpriv(pid, SIG_BYE);
Msg(0, "Attach msg ignored: coming from inside.");
return -1;
}
@@ -976,7 +981,7 @@ struct win *wi;
{
write(i, "Access to session denied.\n", 26);
close(i);
- Kill(pid, SIG_BYE);
+ KillUnpriv(pid, SIG_BYE);
Msg(0, "Attach: access denied for user %s.", user);
return -1;
}
@@ -1294,7 +1299,7 @@ ReceiveMsg()
Msg(0, "Query attempt with bad pid(%d)!", m.m.command.apid);
}
else {
- Kill(m.m.command.apid,
+ KillUnpriv(m.m.command.apid,
(queryflag >= 0)
? SIGCONT
: SIG_BYE); /* Send SIG_BYE if an error happened */
--
2.49.0

Loading