Comment 18 for bug 913694

On Fri, Feb 10, 2012 at 4:19 PM, Jamie Strandboge <email address hidden> wrote:
> 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).

Thank you so much for this comprehensive review.

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

Verified how? I could ensure there's an http:// on the front and a NUL
terminator on the end, but beyond that I'm unsure what else we could
do.

> * get_system_uuid():
>  * should check return of open() call

Done (r147).

>  * 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'

Done (r148).

>  * 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

Is that really sensitive information to begin with? It's the system
UUID. I can't envision an attack vector that involves it.

I'm happy to remove the call to disable secure memory, if you'd prefer
though. I don't mind either way.

>  * sha512 should be verified as != NULL after gcry_md_open() and fail

Done (r148).

>  * gcry_md_read () can return NULL. The circumstance is covered by the code but check for != NULL is easy and future proof.

Done (r149).

>  * 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)

Seems reasonable enough. Done (r150) with the minor alteration of
checking for HASHLEN / 2.

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

Done (r151).

> 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

Is it really a symlink race if it doesn't write to the file?

The problem I see is that if it just uses open and O_CREAT | O_EXCL,
if something goes terribly wrong and whoopsie never removes
/var/lock/whoopsie, no further instances can be started.

Made the change to use /var/lock/whoopsie in r152.

> * daemonize():
>  * umask(0) call usually before fork, but should be fine

Done anyway (r153).

>  * 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

Done (r154).

>  * 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);
>    }

Done (r155).

> * 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 */

Done (r156).

> * 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

Given that /var/crash is world writable, why does this matter? Just
trying to understand what I'm defending against.

I've made the change in r157, but I'm not sure how I could restructure
it to prevent a "time of check to time of use" attack. Suggestions
welcome there.

>  * 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! :)

I'll work on this now, with the hope of getting it done by the end of
the day tomorrow.

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

Handled by replacing all calls to *alloc with their glib counterparts,
which abort() on failure (r158).

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

Fixed in r159. It looks as though I failed to remove that when I moved
some code around in r90.

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

Fixed in r161.

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

Fixed in r162.

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

The properties are marked as read-only, as defined in
whoopsie-preferences.xml. Attempting to set them through d-feet
returns:
"org.freedesktop.DBus.Error.InvalidArgs: Property `ReportCrashes' is
not writable."

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

Fixed in r163.

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

Fixed in r164.

> In summary, once the following are fixed, ACK for main inclusion:
> * get_crash_db_url() needs to verify the url

Need guidance, as mentioned above.

> * fix get_system_uuid() as recommended

Fixed in r147.

> * fix dropprivileges() as recommended

Fixed in r151.

> * fix DoS/TOCTOU for /tmp/.whoopsie-lock

Need guidance, as mentioned above. Partial fix in r152.

> * adjust daemonize() as recommended

Fixed in r153.

> * fix upload_to_crash_file() as recommended

Fixed in r156.

> * fix parse_report() as recommended, and fix bugs with invalid data

Working on this now. I just wanted to give you as much as a reply as I
can, given how close we are to Feature Freeze, and because I have the
aforementioned questions.

> * check return codes of all *alloc calls

Fixed in r158.

> * verify necessity of 'free (system_uuid);'

Fixed in r159.

> * integrate my invalid data tests into test suite somehow

See above.

> * consider moving the home directory to somewhere else (eg /nonexistent)

Fixed in r164.

> Thanks for your attention in these matters.

Thank you again for looking into this.