From 71fb2d7e690640b391b76b5432f07b4a81351c8b Mon Sep 17 00:00:00 2001 From: Karel Klic Date: Tue, 12 Jan 2010 14:26:08 +0100 Subject: Fixing /var/cache/abrt/ permissions by allowing users to read, but not to change their crash data. Adds abrt user, changes abrt-hook-python to use suid instead of sgid bit (uid=abrt), sets /var/cache/abrt and every dump subdirectory to be owned by abrt user. Read access for users and their own crashes is provided by group (/var/cache/abrt/ccpp-xxxx-xx has user's group). --- abrt.spec | 5 +++-- lib/Utils/DebugDump.cpp | 51 +++++++++++++++++++++++++----------------- src/Daemon/Daemon.cpp | 14 ++++++------ src/Hooks/abrt-hook-python.cpp | 2 +- 4 files changed, 42 insertions(+), 30 deletions(-) diff --git a/abrt.spec b/abrt.spec index 85038e68..630e54c8 100644 --- a/abrt.spec +++ b/abrt.spec @@ -250,6 +250,7 @@ rm -rf $RPM_BUILD_ROOT %pre getent group abrt >/dev/null || groupadd -f --system abrt +getent passwd abrt >/dev/null || useradd --system -g abrt -d /etc/abrt -s /sbin/nologin abrt exit 0 %post @@ -279,7 +280,7 @@ fi %config(noreplace) %{_sysconfdir}/%{name}/%{name}.conf %config(noreplace) %{_sysconfdir}/dbus-1/system.d/dbus-%{name}.conf %{_initrddir}/%{name}d -%dir %attr(0775, root, abrt) %{_localstatedir}/cache/%{name} +%dir %attr(0755, abrt, abrt) %{_localstatedir}/cache/%{name} %dir /var/run/%{name} %dir %{_sysconfdir}/%{name} %dir %{_sysconfdir}/%{name}/plugins @@ -386,7 +387,7 @@ fi %files addon-python %defattr(-,root,root,-) -%attr(2755, root, abrt) %{_libexecdir}/abrt-hook-python +%attr(4755, abrt, abrt) %{_libexecdir}/abrt-hook-python %{_libdir}/%{name}/libPython.so* %{python_site}/*.py* diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp index 3c00bb7c..a0a52ab9 100644 --- a/lib/Utils/DebugDump.cpp +++ b/lib/Utils/DebugDump.cpp @@ -251,6 +251,9 @@ void CDebugDump::UnLock() * * Security: we should not allow users to write new files or write * into existing ones, but they should be able to read them. + * + * @param uid + * Crashed application's User Id * * We currently have only three callers: * kernel oops hook: uid=0 @@ -258,11 +261,10 @@ void CDebugDump::UnLock() * ccpp hook: uid=uid of crashed user's binary * this hook runs under 0:0 * python hook: uid=uid of crashed user's script - * this hook runs under uid:abrt + * this hook runs under abrt:gid * - * Currently, we set dir's uid to uid parameter, but we do not allow - * write acces to the owner; and we set gid to abrt's group id - * and we give write access to _group_. + * Currently, we set dir's gid to passwd(uid)->pw_gid parameter, and we set uid to + * abrt's user id. We do not allow write access to group. */ void CDebugDump::Create(const char *pDir, uid_t uid) { @@ -280,36 +282,45 @@ void CDebugDump::Create(const char *pDir, uid_t uid) Lock(); m_bOpened = true; - /* Was creating it with mode 0700, but this allows the user to replace - * any file in the directory, changing security-sensitive data + /* Was creating it with mode 0700 and user as the owner, but this allows + * the user to replace any file in the directory, changing security-sensitive data * (e.g. "uid", "analyzer", "executable") */ - if (mkdir(m_sDebugDumpDir.c_str(), 0570) == -1) + if (mkdir(m_sDebugDumpDir.c_str(), 0750) == -1) { UnLock(); m_bOpened = false; throw CABRTException(EXCEP_DD_OPEN, "Can't create dir '%s'", pDir); } -#if 0 - /* paranoia. mkdir did it already */ - if (chmod(m_sDebugDumpDir.c_str(), 0570) == -1) + + /* mkdir's mode (above) can be affected by umask, fix it */ + if (chmod(m_sDebugDumpDir.c_str(), 0750) == -1) { UnLock(); m_bOpened = false; throw CABRTException(EXCEP_DD_OPEN, "Can't change mode of '%s'", pDir); } -#endif - m_uid = uid; + /* Get ABRT's user id */ + m_uid = 0; + struct passwd *pw = getpwnam("abrt"); + if (pw) + m_uid = pw->pw_uid; + else + error_msg("User 'abrt' does not exist, using uid 0"); + + /* Get crashed application's group id */ m_gid = 0; - struct group *gr = getgrnam("abrt"); - if (gr) - m_gid = gr->gr_gid; + pw = getpwuid(uid); + if (pw) + m_gid = pw->pw_gid; else - error_msg("Group 'abrt' does not exist, using gid 0"); + error_msg("User %lu does not exist, using gid 0", (long)uid); + if (chown(m_sDebugDumpDir.c_str(), m_uid, m_gid) == -1) { - perror_msg("can't change '%s' ownership to %lu:%lu", m_sDebugDumpDir.c_str(), (long)m_uid, (long)m_gid); + perror_msg("can't change '%s' ownership to %lu:%lu", m_sDebugDumpDir.c_str(), + (long)m_uid, (long)m_gid); } SaveText(FILENAME_UID, to_string(uid).c_str()); @@ -408,12 +419,12 @@ static void LoadTextFile(const char *pPath, std::string& pData) static void SaveBinaryFile(const char *pPath, const char* pData, unsigned pSize, uid_t uid, gid_t gid) { - /* "Why 0460?!" See ::Create() for security analysis */ + /* "Why 0640?!" See ::Create() for security analysis */ unlink(pPath); - int fd = open(pPath, O_WRONLY | O_TRUNC | O_CREAT, 0460); + int fd = open(pPath, O_WRONLY | O_TRUNC | O_CREAT, 0640); if (fd < 0) { - throw CABRTException(EXCEP_DD_SAVE, "Can't open file '%s'", pPath); + throw CABRTException(EXCEP_DD_SAVE, "Can't open file '%s': %s", pPath, errno ? strerror(errno) : "errno == 0"); } if (fchown(fd, uid, gid) == -1) { diff --git a/src/Daemon/Daemon.cpp b/src/Daemon/Daemon.cpp index 0f9c6228..09d8ab8b 100644 --- a/src/Daemon/Daemon.cpp +++ b/src/Daemon/Daemon.cpp @@ -632,7 +632,7 @@ static void start_syslog_logging() logmode = LOGMODE_SYSLOG; } -static void ensure_writable_dir(const char *dir, mode_t mode, const char *group) +static void ensure_writable_dir(const char *dir, mode_t mode, const char *user) { struct stat sb; @@ -641,12 +641,12 @@ static void ensure_writable_dir(const char *dir, mode_t mode, const char *group) if (stat(dir, &sb) != 0 || !S_ISDIR(sb.st_mode)) error_msg_and_die("'%s' is not a directory", dir); - struct group *gr = getgrnam(group); - if (!gr) - perror_msg_and_die("Can't find group '%s'", group); + struct passwd *pw = getpwnam(user); + if (!pw) + perror_msg_and_die("Can't find user '%s'", user); - if ((sb.st_uid != 0 || sb.st_gid != gr->gr_gid) && chown(dir, 0, gr->gr_gid) != 0) - perror_msg_and_die("Can't set owner 0:%u on '%s'", (unsigned int)gr->gr_gid, dir); + if ((sb.st_uid != pw->pw_uid || sb.st_gid != pw->pw_gid) && chown(dir, pw->pw_uid, pw->pw_gid) != 0) + perror_msg_and_die("Can't set owner %u:%u on '%s'", (unsigned int)pw->pw_uid, (unsigned int)pw->pw_gid, dir); if ((sb.st_mode & 07777) != mode && chmod(dir, mode) != 0) perror_msg_and_die("Can't set mode %o on '%s'", mode, dir); } @@ -657,7 +657,7 @@ static void sanitize_dump_dir_rights() * us with thousands of bogus or malicious dumps */ /* 07000 bits are setuid, setgit, and sticky, and they must be unset */ /* 00777 bits are usual "rwxrwxrwx" access rights */ - ensure_writable_dir(DEBUG_DUMPS_DIR, 0775, "abrt"); + ensure_writable_dir(DEBUG_DUMPS_DIR, 0755, "abrt"); /* debuginfo cache */ ensure_writable_dir(DEBUG_DUMPS_DIR"-di", 0755, "root"); /* temp dir */ diff --git a/src/Hooks/abrt-hook-python.cpp b/src/Hooks/abrt-hook-python.cpp index d7fca67d..b921fba2 100644 --- a/src/Hooks/abrt-hook-python.cpp +++ b/src/Hooks/abrt-hook-python.cpp @@ -111,7 +111,7 @@ int main(int argc, char** argv) CDebugDump dd; try { - dd.Create(path, geteuid()); + dd.Create(path, getuid()); } catch (CABRTException &e) { error_msg_and_die("Error while creating debug dump: %s", e.what()); } -- cgit