Hi,
here is a patch that fixes /var/cache/abrt/* permissions by allowing users to read, but not to change their crash data. It 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).
Current git version is broken. We create a debug dump directory without write access for user, and applications run by that user (abrt-python-hook) cannot create files in that directory, even when they run with the right group (=abrt group, which has the write access) Furthermore, user can chmod directories he owns, so he can add write access himself. So abrt group is useless in this case.
The only problem with this patch is that it assumes that "private groups" are used on the system. That is, a user "karel" belongs to group "karel", and not to a common group "users". I do not know whether this is true for all desktop and server deployments. (If all users share the same group, they will also be able to read other user's crashes, and we do not want that.)
An alternative to this patch is setting the option MakeCompatCore default and make everything in /var/cache/abrt/ readable only by ABRT, as it was proposed by Jiri
Jiri, Denys, Nikola, what do you think about it?
Karel
On Mon, 2010-01-11 at 18:48 +0100, Karel Klic wrote:
Hi,
here is a patch that fixes /var/cache/abrt/* permissions by allowing users to read, but not to change their crash data. It 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).
Yes, I was thinking about it and having abrt user seems to be a workable solution.
Current git version is broken. We create a debug dump directory without write access for user, and applications run by that user (abrt-python-hook) cannot create files in that directory, even when they run with the right group (=abrt group, which has the write access) Furthermore, user can chmod directories he owns, so he can add write access himself. So abrt group is useless in this case.
Yep.
(I am a bit surprised that process with uid:gid 111:222 cannot create new files in a directory owned by 111:222 with mode r-xrwx---)
The only problem with this patch is that it assumes that "private groups" are used on the system. That is, a user "karel" belongs to group "karel", and not to a common group "users". I do not know whether this is true for all desktop and server deployments.
Yes, unix permission model does not allow us to give two _uids_ read access to files, thus if dump files are owned by user "abrt", there is no way to open them for reading for user "ufoo" too, we can only open it for reading for his group "gfoo". I think this is good enough for now.
(If all users share the same group, they will also be able to read other user's crashes, and we do not want that.)
We may try to use ACL on filesystems which have them. Lets leave it as a TODO.
An alternative to this patch is setting the option MakeCompatCore default and make everything in /var/cache/abrt/ readable only by ABRT, as it was proposed by Jiri
Well, we want to run gdb etc on the corefile, and we do not trust gdb enough to run it under root. Thus we have to make at least corefile readable by processes running under user's uid:gid.
+ /* Was used to override umask, which might remove write access to group. + * but currently we do not need it. + */ + if (chmod(m_sDebugDumpDir.c_str(), 0750) == -1)
Why "was"? Perhaps "mkdir's mode (above) can be affected by umask, fix it"
- 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");
Lets keep indentation consistent at least within the same file. (Wwe already have different styles across files, but at least within one file it is so far consistent)
Apart from these nits, patch looks ok. Please apply.
-- vda
Pushed to git.
On 01/12/2010 11:50 AM, Denys Vlasenko wrote:
On Mon, 2010-01-11 at 18:48 +0100, Karel Klic wrote:
Hi,
here is a patch that fixes /var/cache/abrt/* permissions by allowing users to read, but not to change their crash data. It 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).
Yes, I was thinking about it and having abrt user seems to be a workable solution.
Current git version is broken. We create a debug dump directory without write access for user, and applications run by that user (abrt-python-hook) cannot create files in that directory, even when they run with the right group (=abrt group, which has the write access) Furthermore, user can chmod directories he owns, so he can add write access himself. So abrt group is useless in this case.
Yep.
(I am a bit surprised that process with uid:gid 111:222 cannot create new files in a directory owned by 111:222 with mode r-xrwx---)
The only problem with this patch is that it assumes that "private groups" are used on the system. That is, a user "karel" belongs to group "karel", and not to a common group "users". I do not know whether this is true for all desktop and server deployments.
Yes, unix permission model does not allow us to give two _uids_ read access to files, thus if dump files are owned by user "abrt", there is no way to open them for reading for user "ufoo" too, we can only open it for reading for his group "gfoo". I think this is good enough for now.
(If all users share the same group, they will also be able to read other user's crashes, and we do not want that.)
We may try to use ACL on filesystems which have them. Lets leave it as a TODO.
An alternative to this patch is setting the option MakeCompatCore default and make everything in /var/cache/abrt/ readable only by ABRT, as it was proposed by Jiri
Well, we want to run gdb etc on the corefile, and we do not trust gdb enough to run it under root. Thus we have to make at least corefile readable by processes running under user's uid:gid.
- /* Was used to override umask, which might remove write access to group.
* but currently we do not need it.*/- if (chmod(m_sDebugDumpDir.c_str(), 0750) == -1)
Why "was"? Perhaps "mkdir's mode (above) can be affected by umask, fix it"
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");Lets keep indentation consistent at least within the same file. (Wwe already have different styles across files, but at least within one file it is so far consistent)
Apart from these nits, patch looks ok. Please apply.
-- vda
crash-catcher@lists.fedorahosted.org
-
Denys Vlasenko -
Karel Klic