Comment 16 for bug 913694

I spent quite a bit of time looking at whoopsie and overall I think it looks to be in pretty good shape, but needs some updates (see below).

Since whoopsie is started as root, I looked very carefully at the section before it drops privileges. Here is what I found:
* sets up signals: fine

* parses arguments: fine

* get_crash_db_url(): needs to verify the url. Granted, it should be tightly controlled via the initscript or the root user, but still. Otherwise fine

* get_system_uuid():
 * should check return of open() call
 * http://www.gnupg.org/documentation/manuals/gcrypt/Initializing-the-library.html states that gcry_check_version() should be used for a number of reasons, not least of which to initialize 'some subsystems'
 * intially surprised that you disabled secure memory, but I guess since this is a root process the contents of /sys/class/dmi/id/product_uuid shouldn't leak out
 * sha512 should be verified as != NULL after gcry_md_open() and fail
 * gcry_md_read () can return NULL. The circumstance is covered by the code but check for != NULL is easy and future proof.
 * while you are careful with calculating md_len, it would be easier to maintain if sha512_system_uuid was not set to 129 explicitly. Not a security issue. Perhaps doing:

     #define HASHLEN 128
     static char sha512_system_uuid[HASHLEN+1] = {0};
     ...
     if (md_len != HASHLEN)
         exit (EXIT_FAILURE)

Like I said, the math is right now, but if something changes in the code you could have a stack buffer overflow in hex_to_car when writing to buf (get_system_uuid)

* dropprivileges(): drops to 'whoopsie' user and looks fine except setenv should check the return codes

Then I looked a bit at the non-root parts and found:
* can DoS whoopsie by creating /tmp/.whoopsie-lock. Also symlink race. Move to /var/lock/whoopsie instead (which has whoopsie write). Maybe easier to just pass O_EXCL and check open() does not return -1. This would not allow for the helpful error message though

* daemonize():
 * umask(0) call usually before fork, but should be fine
 * should close open file descriptors for future proofing. Ie, before fork use getrlimit(RLIMIT_NOFILE,&rl) and then after the chdir, use something like (this means you can drop the 3 close(STD...) calls):
    for (i=0;i<rl.rlim_max && i<1024;i++)
      close(i)
    can lose the 3 close(STD...) calls then
 * should also attach stdin, stdout and stderr to /dev/null with something like:
    fd0 = open("/dev/null", O_RDWR);
    fd1 = dup(0);
    fd2 = dup(0);
    openlog(...)
    if (fd0 != 0 || fd1 != 1 || fd2 != 2) {
        syslog(...);
        exit (EXIT_FAILURE);
    }

* upload_to_crash_file: please explicitly NULL terminate crash_file. Eg:
   crash_file = malloc (len + 7); /* .crash */
   memcpy (crash_file, upload_file, len);
   strcpy (crash_file + len, ".crash");
   crash_file[len+6] = '\0'; /* ensure NULL-terminated */

* I spent quite a bit of time desk checking parse_report() and it seems to dtrt when faced with valid input, however it has some problems with invalid input. I was able to create some weird key/value pairs as well as segfault whoopsie with just a little bit of effort. Since /var/crash is world-writable, need to be careful about the files it processes
 * should only process regular files in /var/crash
 * should not follow symlinks
 * should perform input validation on keys and values

I wrote some test cases to demonstrate the problem and will attach the tarball to this bug. Basically I pulled parse_report() out into its own file, compiled it and then ran it over the test data. Please incorporate this test data into your testsuite and use a similar methodology as the test.sh script. I also advise in your test scripts to verify that the output of parse_report() is what you want it to be, even in the face of malformed data. Finally, my invalid test cases are in no way comprehensive-- please add more! :)

* all malloc's and realloc's should have their return values verified (man malloc)

* in whoopsie.c's main(), I found 'free (system_uuid);' but didn't see why it was there

* has testsuite, runs in build, has some compiler warnings which would ideally be fixed (not a blocker)

* uses curl safely (ie doesn't disable SSL)

* polkit: 'gnome-control-center whoopsie-daisy'. verified to work correctly and verify permissions appropriately (ie, requires authentication for Set* operations, but not the Properties)

In terms of packaging, here are some thoughts:
* should maybe redirect chmod and chgrp stderr in /var/crash to /dev/null

* more comfortable with home directory as /nonexistent or possibly /var/crash

* except for the aforementioned testsuite compiler warnings, the build logs look fine

In summary, once the following are fixed, ACK for main inclusion:
* get_crash_db_url() needs to verify the url
* fix get_system_uuid() as recommended
* fix dropprivileges() as recommended
* fix DoS/TOCTOU for /tmp/.whoopsie-lock
* adjust daemonize() as recommended
* fix upload_to_crash_file() as recommended
* fix parse_report() as recommended, and fix bugs with invalid data
* check return codes of all *alloc calls
* verify necessity of 'free (system_uuid);'
* integrate my invalid data tests into test suite somehow
* consider moving the home directory to somewhere else (eg /nonexistent)

Thanks for your attention in these matters.