[MIR] whoopsie-daisy

[Evan]

Main inclusion request for whoopise-daisy, the Ubuntu crash database submission daemon.

No violations in MainInclusionRequirements. The package is maintained by a Canonical employee and is native to the Ubuntu archive. This software is necessary to implement https://blueprints.launchpad.net/ubuntu/+spec/foundations-p-crash-database. All the dependencies, of which there are few (libcurl, glib), are in main.

[Michael Terry (mterry)]

Assigning to Jamie since this package parses crash report files manually.

But aside from that security review, this seems a little not-ready? The upstream bzr is a +junk branch, and there's comments like "this code should be removed before we go production"

[Evan (ev)]

I've fixed the +junk branch and production comment in trunk (now lp:whoopsie-daisy). Do hold off on the security review, as I have changes in trunk to drop privileges and read the system UUID, amongst other things.

I've been holding off on uploading these. Dropping privileges breaks communication with Network Manager, as that has an at_console DBus security setting. Also, the permissions on /var/crash are such that an unprivileged user will not be able to read the crash files. I'd like to solve this by making it +r for the whoopsie group.

I'll let you know via this bug report when I have something ready for the security review.

Cheers!

[Michael Terry (mterry)]

Evan, if all you are using NetworkManager for is watching connected-status (quick scan of code made me think so), maybe you can get by with the new GNetworkMonitor in gio: http://developer.gnome.org/gio/unstable/GNetworkMonitor.html

[Evan (ev)]

Michael,

Indeed, I have a note in the code about using that when it's in the archive (at the time, it wasn't). I'll give it another look.

[Evan (ev)]

Okay, whoopsie-daisy 0.1.3 is ready for review.

The daemon now drops privileges and all capabilities, except for CAP_FOWNER. Keeping CAP_FOWNER allows it to ignore the sticky bit in /var/crash and remove the .upload files, which are created by regular users to indicate that a crash report should be submitted. It limits this capability to just /var/crash by bind-mounting / read-only in /var/tmp, then bind-mounting /var/crash read-write on top of it, and finally chrooting into this. It does all of this in a separate mount namespace.

I've moved away from NetworkManager to GNetworkMonitor. This removes the need to pass the at_console check, and affords us an easy way to check if the crash submission server is accessible.

[Evan (ev)]

James Troup has helpfully pointed out that the CAP_FOWNER stuff is entirely unnecessary. We can just write a .uploaded file once we've processed the crash, and delete those in cron. Sometimes the simplest solution... :-/

[Evan (ev)]

Jamie,

Any luck with this? Whoopsie 0.1.5 is now in the archive, which drops the CAP_FOWNER code (0.1.4) and adds a user interface in the control center for an administrator to disable crash reporting.

Cheers!

[Jamie Strandboge (jdstrand)]

Just starting to look at this, but an 'apt-get install whoopsie' results in:
Setting up whoopsie (0.1.5) ...
chgrp: cannot access `/var/crash/*.crash': No such file or directory
dpkg: error processing whoopsie (--configure):
 subprocess installed post-installation script returned error exit status 1
Processing triggers for libc-bin ...
ldconfig deferred processing now taking place
Errors were encountered while processing:
 whoopsie
E: Sub-process /usr/bin/dpkg returned an error code (1)

[Evan (ev)]

0.1.6 fixed that, which should be in the archive by now.

[Jamie Strandboge (jdstrand)]

Still working through this and trying to understand the design. I did find that after installing the package it doesn't start:
$ sudo whoopsie

** WARNING **: Could not get crash database location.

I'm assuming this is because of this line in /etc/init/whoopsie.conf:
env CRASH_DB_URL=http://localhost:8080

"dpkg-query -f '${Conffiles}' -W" tells me that /etc/init/whoopsie.conf is a conffile, which doesn't seem to make sense as this CRASH_DB_URL does not seem to be a reasonable default.

[Jamie Strandboge (jdstrand)]

The https://blueprints.launchpad.net/ubuntu/+spec/foundations-p-crash-database has a lot of TODO items. Are all these changes expected to be part of this same source package which is intended to be in main?

[Evan (ev)]

Most of those work items have to do with the backend service that this reports to.

It's currently defaulted to localhost as we're still in the process of deploying the backend service on Canonical's servers.

[Evan (ev)]

Oh, and yes, as a matter of convenience I've put that backend service in the source package. However, once we're past Feature Freeze, I'm likely to deploy updates to it from bzr.

[Jamie Strandboge (jdstrand)]

I get a segmentation fault when try to access the whoopsie-daisy preferences:
$ gnome-control-center whoopsie-daisy
** WARNING **: Could not load interface file Failed to open file '/build/buildd/whoopsie-daisy-0.1.5/debian/whoopsie/usr/share/gnome-control-center/ui/whoopsie.ui': No such file or directory
Segmentation fault (core dumped)

[Evan (ev)]

Fixed in 0.1.7. Apologies for that oversight.

[Jamie Strandboge (jdstrand)]

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 wor...

[Evan (ev)]

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 t...

[Jamie Strandboge (jdstrand)]

Thanks for taking care of these items. I'll respond to your questions here:

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

Verified in a manner that will allow whoopsie to fail gracefully when faced with invalid input. Checking for http:// or https:// and is nul terminated is probably good enough, since this is administrator controlled.

>> * 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 systemUUID. I can't envision an attack vector that involves it.

Well, this was more of a thought provoking comment than a firm recommendation. The file is only accessible by root, but I couldn't really think of an attack vector that would expose this via whoopsie where the user was non-privileged that didn't require physical presence. I might not have been creative enough, but it is easy enough to remove later if needed.

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

Yes, you open it and can create an arbitrary file. It isn't a big deal on 12.04 because we have Yama in the kernel to guard against it, but still.

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

Hmm, well, /var/lock also has 1777 permissions, so this didn't address the issue. What I meant was create the directory /var/local/whoopsie, and put your lock file in there. Doing this is enough to fix both the TOCTOU and the DoS.

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

Because it is world writable, people can put anything they want in there and whoopsie could process it. As such, whoopsie needs to be coded defensively since it is processing untrusted and unfiltered content. While is is excellent that whoospie is running as non-root at this point, we still want to make sure that it handles invalid input gracefully such that whoopsie won't crash or be subverted into executing arbitrary code. I'm not sure I understand the TOCTOU you are referring to here-- we just want parse_report() to fail gracefully in the presence of invalid input. Once it is uploaded to the server, the server will of course need to do its own input validation.

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

[Evan (ev)]

On Tue, Feb 14, 2012 at 7:16 PM, Jamie Strandboge <email address hidden> wrote:
>>> * 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.
>
> Verified in a manner that will allow whoopsie to fail gracefully when
> faced with invalid input. Checking for http:// or https:// and is nul
> terminated is probably good enough, since this is administrator
> controlled.

Fixed in r167.

>> Is that really sensitive information to begin with? It's the
> systemUUID. I can't envision an attack vector that involves it.
>
> Well, this was more of a thought provoking comment than a firm
> recommendation. The file is only accessible by root, but I couldn't
> really think of an attack vector that would expose this via whoopsie
> where the user was non-privileged that didn't require physical presence.
> I might not have been creative enough, but it is easy enough to remove
> later if needed.

Sure. Paranoia wins here :). I've removed the call to disabling secure
memory (r165).

> Hmm, well, /var/lock also has 1777 permissions, so this didn't address
> the issue. What I meant was create the directory /var/local/whoopsie,
> and put your lock file in there. Doing this is enough to fix both the
> TOCTOU and the DoS.

Done (r166).

>> Given that /var/crash is world writable, why does this matter? Just
> trying to understand what I'm defending against.
>
> Because it is world writable, people can put anything they want in there
> and whoopsie could process it. As such, whoopsie needs to be coded
> defensively since it is processing untrusted and unfiltered content.
> While is is excellent that whoospie is running as non-root at this
> point, we still want to make sure that it handles invalid input
> gracefully such that whoopsie won't crash or be subverted into executing
> arbitrary code. I'm not sure I understand the TOCTOU you are referring
> to here-- we just want parse_report() to fail gracefully in the presence
> of invalid input. Once it is uploaded to the server, the server will of
> course need to do its own input validation.

Apologies for being unclear, I was asking why it matters from a
security perspective whether whoopsie is following symlinks or reading
from block devices when /var/crash is world writable and any user can
feed it anything they so desire (read-only and as a regular user, of
course).

The TOCTOU I mentioned is that presumably an attack could replace a
regular file in /var/crash with a symlink between the calls to
g_file_test and g_mapped_file_new.

> I was not clear here. I was trying to say that the policykit parts were
> done correctly. Sorry for the confusion.

No worries!

[Evan (ev)]

On Tue, Feb 14, 2012 at 6:09 PM, Evan Dandrea
<email address hidden> wrote:
> On Fri, Feb 10, 2012 at 4:19 PM, Jamie Strandboge <email address hidden> wrote:
>> * 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.

Done (r167).

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

Done (r168-179).

I believe with these changes I've addressed your remaining concerns,
but do let me know if there's anything else that's caught your eye or
anything you feel I've missed. Jamie, from your perspective, is the
package ready to go into main?

Thanks again for all of your hard work reviewing this and for the sample data.

[Jamie Strandboge (jdstrand)]

You're welcome. Looking at the bzr branch it looks like you addressed all the issues. Thanks! :)

[Jamie Strandboge (jdstrand)]

Feel free to mark this bug as 'Fix Committed' once you upload an updated whoopsie to the archive with these changes.

[Launchpad Janitor (janitor)]

This bug was fixed in the package whoopsie-daisy - 0.1.8

---------------
whoopsie-daisy (0.1.8) precise; urgency=low

  * Security fixes. Thanks Jamie Strandboge for the review.
    - Check the return value of the open call in get_system_uuid.
    - Properly initialize libcrypt.
    - Check that the call to gcry_md_open succeeds
    - Ensure that reading the SHA512 message digest succeeds.
    - Protect against changes to the message digest length creating a
      security vulnerability.
    - Check the returncode of setenv.
    - Use /var/lock/whoopsie instead of /tmp/.whoopsie-lock.
    - umask is usually called before fork.
    - Future-proof by using getrlimit instead of explicitly closing STD*
    - Redirect stdin, stdout, and stderr to /dev/null.
    - Ensure strings created in update_to_crash_file are NULL-terminated.
    - Only process regular files in /var/crash.
    - Replace calls to *alloc with g_*alloc, which calls abort() on
      failure.
    - Remove unused system_uuid pointer.
    - Fix warnings in make check.
    - Initialize all of curl.
    - Redirect stderr to null in chgrp and chmod calls.
    - Set home directory to /nonexistent.
    - Enable libcrypt secure memory.
    - Put the lock file in /var/lock/whoopsie/.
    - Sanity check the CRASH_DB_URL environment variable.
    - Added tests:
      - Check handling of embedded NUL bytes.
      - Verify that symlinks in /var/crash produce the correct error
        message.
      - Verify that keys without values in reports produce an error message.
      - Ensure that the report does not start with a value.
      - Correctly identify a report without spaces as malformed.
      - Verify that directories in /var/crash produce the correct error
        message.
      - Ensure that blank lines in a report are treated as errors.
      - Ensure that carriage returns are escaped.
      - Do not start multi-line values with a newline.
      - Check that a valid report has the exact expected contents.
      - Ensure that other variants of embedded carriage returns are escaped.
      - Verify that reports without a trailing newline are handled properly.
  * Change crash database URL to http://daisy.ubuntu.com.
  * Main inclusion request approved (LP: #913694).
 -- Evan Dandrea <email address hidden> Thu, 16 Feb 2012 16:37:35 +0000

[Colin Watson (cjwatson)]

Promoted to main.