This patch adds an option '-x' to abrt-dump-oops which makes the created dumpdir and it's content readable by all. The old kerneloops application also informed all users about the oops. The patch for applet will follow once this is accepted.
Please review, J.
On Wed, 2011-03-23 at 18:36 +0100, Jiri Moskovcak wrote:
This patch adds an option '-x' to abrt-dump-oops which makes the created dumpdir and it's content readable by all. The old kerneloops application also informed all users about the oops. The patch for applet will follow once this is accepted.
Please review,
--- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -321,7 +321,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags) else { if (!(flags & DD_FAIL_QUIETLY_EACCES)) - perror_msg("Can't access '%s'", dir); + VERB1 perror_msg("Can't access '%s'", dir); }
This makes abrt commands (abrt-action-foo with -d INACCESSIBLE_DIR etc) fail without any error message unless -v is given. This has a high potential to confuse users.
There are places where you do want to suppress messages on EACCESS errors. Use DD_FAIL_QUIETLY_EACCES flag in dd_opendir() call in those places instead of blanket message suppression like above.
struct dump_dir *dd_create(const char *dir, uid_t uid) { + mode_t dir_mode; + /* we need world readable dump dir for some problems like kerneloops + * where is no sensitive data and which should be visible and reportable + * by all + */ + if(uid == (uid_t)-1L) + dir_mode = 0755; + else + dir_mode = 0750;
Unfortunately, uid -1 is already overloaded to mean "don't chown files":
/* Pass uid = (uid_t)-1L to disable chown'ing of newly created files * (IOW: if you aren't running under root): */ struct dump_dir *dd_create(const char *dir, uid_t uid);
See, for example, its usage here:
struct dump_dir *steal_directory(const char *base_dir, const char *dump_dir_name) { const char *base_name = strrchr(dump_dir_name, '/'); if (base_name) base_name++; else base_name = dump_dir_name; struct dump_dir *dd_dst; unsigned count = 100; char *dst_dir_name = concat_path_file(base_dir, base_name); while (1) { dd_dst = dd_create(dst_dir_name, (uid_t)-1); ...
I think the solution here is to add mode_t parameter to dd_create(). Then caller can pass 640, 644, or event 600 or 666, as needed. (inside dd_create(), don't forget to use (mode | ((mode & 0444)>>2)) in mkdir, to copy r bits to x...).
Consider saving mode in new dd->dd_mode field in dd_opendir/dd_create, and using it instead of hardcoded 644 when creating new files and and in dd_sanitize_mode_and_owner().
/* "Why 0640?!" See dd_create() for security analysis */ unlink(path); - int fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, 0640); + int fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, 0644);
You edited the code but forgot to edit the comment (s/640/644).
- uid_t my_euid = geteuid(); + uid_t my_euid = (uid_t)-1L; + if (!world_readable_dump) + my_euid = geteuid(); + ... - struct dump_dir *dd = dd_create(path, /*uid:*/ my_euid); + struct dump_dir *dd = dd_create(path, /*uid:*/ (uid_t)-1L);
Looks wrong: you are passing -1L all the time, not only when world_readable_dump == true.
if (dd) { - dd_create_basic_files(dd, /*uid:*/ 0); + dd_create_basic_files(dd, /*uid:*/ my_euid);
This will create FILENAME_UID == "-1" if my_euid is -1. Did you consider the alternative method, where dd_create_basic_files simply doesn't create FILENAME_UID if my_euid is -1 above? (This might require small tweak in the daemon to make sure it allows deletion by anyone if FILENAME_UID is missing).
+ OPT_BOOL('x', NULL, NULL, _("Make the dump directory world readable")), //oopses doesn't contain any sensitive info, and even the old koops app was showing the oopses to all users
Please wrap the line (say, put comment above it).
+ if(opts & OPT_x) + world_readable_dump = true;
Simpler way to do it is "world_readable_dump = (opts & OPT_x);"
On 03/24/2011 01:57 PM, Denys Vlasenko wrote:
On Wed, 2011-03-23 at 18:36 +0100, Jiri Moskovcak wrote:
This patch adds an option '-x' to abrt-dump-oops which makes the created dumpdir and it's content readable by all. The old kerneloops application also informed all users about the oops. The patch for applet will follow once this is accepted.
Please review,
--- a/src/lib/dump_dir.c +++ b/src/lib/dump_dir.c @@ -321,7 +321,7 @@ struct dump_dir *dd_opendir(const char *dir, int flags) else { if (!(flags& DD_FAIL_QUIETLY_EACCES))
perror_msg("Can't access '%s'", dir);
VERB1 perror_msg("Can't access '%s'", dir); }
This makes abrt commands (abrt-action-foo with -d INACCESSIBLE_DIR etc) fail without any error message unless -v is given. This has a high potential to confuse users.
There are places where you do want to suppress messages on EACCESS errors. Use DD_FAIL_QUIETLY_EACCES flag in dd_opendir() call in those places instead of blanket message suppression like above.
- ok, I was trying to fix the error messages in the gui, this shouldn't even go to this patch, will fix it on the gui side..
struct dump_dir *dd_create(const char *dir, uid_t uid) {
- mode_t dir_mode;
- /* we need world readable dump dir for some problems like
kerneloops
* where is no sensitive data and which should be visible and
reportable
* by all
*/
- if(uid == (uid_t)-1L)
dir_mode = 0755;
- else
dir_mode = 0750;
Unfortunately, uid -1 is already overloaded to mean "don't chown files":
/* Pass uid = (uid_t)-1L to disable chown'ing of newly created files
- (IOW: if you aren't running under root):
*/ struct dump_dir *dd_create(const char *dir, uid_t uid);
See, for example, its usage here:
struct dump_dir *steal_directory(const char *base_dir, const char *dump_dir_name) { const char *base_name = strrchr(dump_dir_name, '/'); if (base_name) base_name++; else base_name = dump_dir_name; struct dump_dir *dd_dst; unsigned count = 100; char *dst_dir_name = concat_path_file(base_dir, base_name); while (1) { dd_dst = dd_create(dst_dir_name, (uid_t)-1); ...
I think the solution here is to add mode_t parameter to dd_create(). Then caller can pass 640, 644, or event 600 or 666, as needed. (inside dd_create(), don't forget to use (mode | ((mode& 0444)>>2)) in mkdir, to copy r bits to x...).
- that was the first idea I had, but then decided to try not to break the API, but it doesn't work as well as I expected, so I will use the additional parameter
Consider saving mode in new dd->dd_mode field in dd_opendir/dd_create, and using it instead of hardcoded 644 when creating new files and and in dd_sanitize_mode_and_owner().
- ok
/* "Why 0640?!" See dd_create() for security analysis */ unlink(path);
- int fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, 0640);
- int fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, 0644);
You edited the code but forgot to edit the comment (s/640/644).
- uid_t my_euid = geteuid();
- uid_t my_euid = (uid_t)-1L;
- if (!world_readable_dump)
my_euid = geteuid();
...
struct dump_dir *dd = dd_create(path, /*uid:*/ my_euid);
struct dump_dir *dd = dd_create(path, /*uid:*/ (uid_t)-1L);
Looks wrong: you are passing -1L all the time, not only when world_readable_dump == true.
- right, I forgot to fix this..
if (dd) {
dd_create_basic_files(dd, /*uid:*/ 0);
dd_create_basic_files(dd, /*uid:*/ my_euid);
This will create FILENAME_UID == "-1" if my_euid is -1. Did you consider the alternative method, where dd_create_basic_files simply doesn't create FILENAME_UID if my_euid is -1 above? (This might require small tweak in the daemon to make sure it allows deletion by anyone if FILENAME_UID is missing).
- ok, will try that
OPT_BOOL('x', NULL, NULL, _("Make the dump directory world readable")), //oopses doesn't contain any sensitive info, and even the old koops app was showing the oopses to all users
Please wrap the line (say, put comment above it).
- ok
- if(opts& OPT_x)
world_readable_dump = true;
Simpler way to do it is "world_readable_dump = (opts& OPT_x);"
- ok, will change it
Thanks for the hints, will send the updated patch asap. J.
a new patch with all comments from Denys included. run-tested, seems to work, even the applet shows the notification. Pushed to git.
Please review, Jirka
crash-catcher@lists.fedorahosted.org