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).

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());
   }