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.