From b0abdde8871b0366868b917df040a8880165ba30 Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 11 Jan 2010 07:17:54 +0100 Subject: DebugDump: use more restrictive modes Signed-off-by: Denys Vlasenko --- lib/Utils/DebugDump.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) (limited to 'lib/Utils/DebugDump.cpp') diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp index b4c3ee49..86e198c6 100644 --- a/lib/Utils/DebugDump.cpp +++ b/lib/Utils/DebugDump.cpp @@ -244,13 +244,18 @@ void CDebugDump::Create(const char *pDir, int64_t uid) Lock(); m_bOpened = true; - if (mkdir(m_sDebugDumpDir.c_str(), 0700) == -1) + /* Was creating it with mode 0700, 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(), 0500) == -1) { UnLock(); m_bOpened = false; throw CABRTException(EXCEP_DD_OPEN, "Can't create dir '%s'", pDir); } - if (chmod(m_sDebugDumpDir.c_str(), 0700) == -1) + /* paranoia? mkdir should have done it already */ + if (chmod(m_sDebugDumpDir.c_str(), 0500) == -1) { UnLock(); m_bOpened = false; @@ -361,7 +366,12 @@ static void LoadTextFile(const char *pPath, std::string& pData) static void SaveBinaryFile(const char *pPath, const char* pData, unsigned pSize) { - int fd = open(pPath, O_WRONLY | O_TRUNC | O_CREAT, 0666); + /* Was creating it with mode 0666, but this allows the user to replace + * file's contents, changing security-sensitive data + * (e.g. "uid", "analyzer", "executable") + */ + unlink(pPath); + int fd = open(pPath, O_WRONLY | O_TRUNC | O_CREAT, 0444); if (fd < 0) { throw CABRTException(EXCEP_DD_SAVE, "Can't open file '%s'", pPath); @@ -393,6 +403,7 @@ void CDebugDump::SaveText(const char* pName, const char* pData) std::string fullPath = concat_path_file(m_sDebugDumpDir.c_str(), pName); SaveBinaryFile(fullPath.c_str(), pData, strlen(pData)); } + void CDebugDump::SaveBinary(const char* pName, const char* pData, unsigned pSize) { if (!m_bOpened) -- cgit From b1c4304104910c4bc066cd43f9784fe2f3ddf1ad Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 11 Jan 2010 07:21:31 +0100 Subject: *: cast pids and uigs to long, not int Signed-off-by: Denys Vlasenko --- lib/Utils/DebugDump.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'lib/Utils/DebugDump.cpp') diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp index 86e198c6..cb3a082c 100644 --- a/lib/Utils/DebugDump.cpp +++ b/lib/Utils/DebugDump.cpp @@ -209,8 +209,8 @@ void CDebugDump::Lock() error_msg_and_die("Locking bug on '%s'", m_sDebugDumpDir.c_str()); std::string lockFile = m_sDebugDumpDir + ".lock"; - char pid_buf[sizeof(int)*3 + 2]; - sprintf(pid_buf, "%u", (unsigned)getpid()); + char pid_buf[sizeof(long)*3 + 2]; + sprintf(pid_buf, "%lu", (long)getpid()); while ((m_bLocked = GetAndSetLock(lockFile.c_str(), pid_buf)) != true) { sleep(1); /* was 0.5 seconds */ @@ -267,7 +267,7 @@ void CDebugDump::Create(const char *pDir, int64_t uid) { /* if /var/cache/abrt is writable by all, _aborting_ here is not useful */ /* let's just warn */ - perror_msg("can't change '%s' ownership to %u:%u", m_sDebugDumpDir.c_str(), (int)uid, (int)gid); + perror_msg("can't change '%s' ownership to %lu:%lu", m_sDebugDumpDir.c_str(), (long)uid, (long)gid); } SaveText(FILENAME_UID, to_string(uid).c_str()); -- cgit From 14ef0cfe72faf6696df3ef8f42927e9458ccbeeb Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 11 Jan 2010 07:22:13 +0100 Subject: *: misc fixes Signed-off-by: Denys Vlasenko --- lib/Utils/DebugDump.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'lib/Utils/DebugDump.cpp') diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp index cb3a082c..dd992aab 100644 --- a/lib/Utils/DebugDump.cpp +++ b/lib/Utils/DebugDump.cpp @@ -115,7 +115,15 @@ static bool GetAndSetLock(const char* pLockFile, const char* pPID) char pid_buf[sizeof(pid_t)*3 + 4]; ssize_t r = readlink(pLockFile, pid_buf, sizeof(pid_buf) - 1); if (r < 0) + { + if (errno == ENOENT) + { + /* Looks like pLockFile was deleted */ + usleep(10 * 1000); /* avoid CPU eating loop */ + continue; + } perror_msg_and_die("Can't read lock file '%s'", pLockFile); + } pid_buf[r] = '\0'; if (strcmp(pid_buf, pPID) == 0) @@ -228,7 +236,7 @@ void CDebugDump::UnLock() } } -void CDebugDump::Create(const char *pDir, int64_t uid) +void CDebugDump::Create(const char *pDir, uid_t uid) { if (m_bOpened) { -- cgit From edf6beb585dc38c365ccbdaae85756b2814e1329 Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 11 Jan 2010 12:09:57 +0100 Subject: *: assorted fixes prompted by security analysis; more to come Signed-off-by: Denys Vlasenko --- lib/Utils/DebugDump.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/Utils/DebugDump.cpp') diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp index dd992aab..cd45493e 100644 --- a/lib/Utils/DebugDump.cpp +++ b/lib/Utils/DebugDump.cpp @@ -307,7 +307,7 @@ static void DeleteFileDir(const char *pDir) } } closedir(dir); - if (remove(pDir) == -1) + if (rmdir(pDir) == -1) { throw CABRTException(EXCEP_DD_DELETE, "Can't remove dir %s", pDir); } -- cgit From 20645ae11a8a9a89cc712896b2f72a25bc62c8db Mon Sep 17 00:00:00 2001 From: Denys Vlasenko Date: Mon, 11 Jan 2010 12:12:53 +0100 Subject: DebugDump: more consistent logic in setting mode and uid:gid on dump dir With comments! yay. Before it, too restrictive mode was preventing python craches to be handled. Signed-off-by: Karel Klic Signed-off-by: Denys Vlasenko --- lib/Utils/DebugDump.cpp | 71 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 53 insertions(+), 18 deletions(-) (limited to 'lib/Utils/DebugDump.cpp') diff --git a/lib/Utils/DebugDump.cpp b/lib/Utils/DebugDump.cpp index cd45493e..3c00bb7c 100644 --- a/lib/Utils/DebugDump.cpp +++ b/lib/Utils/DebugDump.cpp @@ -65,7 +65,9 @@ CDebugDump::CDebugDump() : m_sDebugDumpDir(""), m_pGetNextFileDir(NULL), m_bOpened(false), - m_bLocked(false) + m_bLocked(false), + m_uid(0), + m_gid(0) {} CDebugDump::~CDebugDump() @@ -97,6 +99,15 @@ void CDebugDump::Open(const char *pDir) } Lock(); m_bOpened = true; + /* In case caller would want to create more files, he'll need uid:gid */ + m_uid = 0; + m_gid = 0; + struct stat stat_buf; + if (stat(m_sDebugDumpDir.c_str(), &stat_buf) == 0) + { + m_uid = stat_buf.st_uid; + m_gid = stat_buf.st_gid; + } } bool CDebugDump::Exist(const char* pPath) @@ -236,6 +247,23 @@ void CDebugDump::UnLock() } } +/* Create a fresh empty debug dump dir. + * + * Security: we should not allow users to write new files or write + * into existing ones, but they should be able to read them. + * + * We currently have only three callers: + * kernel oops hook: uid=0 + * this hook runs under 0:0 + * 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 + * + * 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_. + */ void CDebugDump::Create(const char *pDir, uid_t uid) { if (m_bOpened) @@ -256,26 +284,32 @@ void CDebugDump::Create(const char *pDir, uid_t uid) * any file in the directory, changing security-sensitive data * (e.g. "uid", "analyzer", "executable") */ - if (mkdir(m_sDebugDumpDir.c_str(), 0500) == -1) + if (mkdir(m_sDebugDumpDir.c_str(), 0570) == -1) { UnLock(); m_bOpened = false; throw CABRTException(EXCEP_DD_OPEN, "Can't create dir '%s'", pDir); } - /* paranoia? mkdir should have done it already */ - if (chmod(m_sDebugDumpDir.c_str(), 0500) == -1) +#if 0 + /* paranoia. mkdir did it already */ + if (chmod(m_sDebugDumpDir.c_str(), 0570) == -1) { UnLock(); m_bOpened = false; throw CABRTException(EXCEP_DD_OPEN, "Can't change mode of '%s'", pDir); } - struct passwd* pw = getpwuid(uid); - gid_t gid = pw ? pw->pw_gid : uid; - if (chown(m_sDebugDumpDir.c_str(), uid, gid) == -1) +#endif + + m_uid = uid; + m_gid = 0; + struct group *gr = getgrnam("abrt"); + if (gr) + m_gid = gr->gr_gid; + else + error_msg("Group 'abrt' does not exist, using gid 0"); + if (chown(m_sDebugDumpDir.c_str(), m_uid, m_gid) == -1) { - /* if /var/cache/abrt is writable by all, _aborting_ here is not useful */ - /* let's just warn */ - perror_msg("can't change '%s' ownership to %lu:%lu", m_sDebugDumpDir.c_str(), (long)uid, (long)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()); @@ -372,18 +406,19 @@ static void LoadTextFile(const char *pPath, std::string& pData) fclose(fp); } -static void SaveBinaryFile(const char *pPath, const char* pData, unsigned pSize) +static void SaveBinaryFile(const char *pPath, const char* pData, unsigned pSize, uid_t uid, gid_t gid) { - /* Was creating it with mode 0666, but this allows the user to replace - * file's contents, changing security-sensitive data - * (e.g. "uid", "analyzer", "executable") - */ + /* "Why 0460?!" See ::Create() for security analysis */ unlink(pPath); - int fd = open(pPath, O_WRONLY | O_TRUNC | O_CREAT, 0444); + int fd = open(pPath, O_WRONLY | O_TRUNC | O_CREAT, 0460); if (fd < 0) { throw CABRTException(EXCEP_DD_SAVE, "Can't open file '%s'", pPath); } + if (fchown(fd, uid, gid) == -1) + { + perror_msg("can't change '%s' ownership to %lu:%lu", pPath, (long)uid, (long)gid); + } unsigned r = full_write(fd, pData, pSize); close(fd); if (r != pSize) @@ -409,7 +444,7 @@ void CDebugDump::SaveText(const char* pName, const char* pData) throw CABRTException(EXCEP_DD_OPEN, "DebugDump is not opened"); } std::string fullPath = concat_path_file(m_sDebugDumpDir.c_str(), pName); - SaveBinaryFile(fullPath.c_str(), pData, strlen(pData)); + SaveBinaryFile(fullPath.c_str(), pData, strlen(pData), m_uid, m_gid); } void CDebugDump::SaveBinary(const char* pName, const char* pData, unsigned pSize) @@ -419,7 +454,7 @@ void CDebugDump::SaveBinary(const char* pName, const char* pData, unsigned pSize throw CABRTException(EXCEP_DD_OPEN, "DebugDump is not opened"); } std::string fullPath = concat_path_file(m_sDebugDumpDir.c_str(), pName); - SaveBinaryFile(fullPath.c_str(), pData, pSize); + SaveBinaryFile(fullPath.c_str(), pData, pSize, m_uid, m_gid); } void CDebugDump::InitGetNextFile() -- cgit 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). --- lib/Utils/DebugDump.cpp | 51 ++++++++++++++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 20 deletions(-) (limited to 'lib/Utils/DebugDump.cpp') 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) { -- cgit