On Fri, Feb 10, 2012 at 4:19 PM, Jamie Strandboge <[email protected]> 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. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/913694 Title: [MIR] whoopsie-daisy To manage notifications about this bug go to: https://bugs.launchpad.net/ubuntu/+source/whoopsie-daisy/+bug/913694/+subscriptions -- ubuntu-bugs mailing list [email protected] https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
