|
Description
Simon McVittie
2015-01-26 20:29:46 UTC
Created attachment 112842 [details] [review] [PATCH 1/6] Factor out some utility functions from test/dbus-daemon* In the process, make test_kill_pid() safer: do not try to terminate more than one pid, or the NULL handle. Created attachment 112843 [details] [review] [PATCH 2/6] Generate test configuration files via build-time sed, not configure This means we can generate a version that works when installed, from the same source files. Created attachment 112844 [details] [review] [PATCH 3/6] test: implement GLib-style "installed tests" We run each test twice: * once with the system's session.conf, as an integration test (test-cases that need a special configuration are automatically skipped) * once with our special test configuration files, which provide better coverage Created attachment 112845 [details] [review] [PATCH 4/6] Add infrastructure to run bits of tests under an alternative uid --- This is not going to be enabled by default under either "make check" or installed-tests, because it requires running individual tests as root, but sandboxed environments like autopkgtest can use it selectively for better coverage. Created attachment 112846 [details] [review] [PATCH 5/6] Add a test for uid-controlled permissions This is technical debt from mitigating CVE-2014-8148, which should really have had a regression test at the time. Created attachment 112847 [details] [review] [PATCH 6/6] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root Unlike the initial mitigation for CVE-2014-8148, we now allow uid 0 to call UpdateActivationEnvironment. There's no point in root doing that, but there's also no reason why it's particularly bad - if an attacker is uid 0 we've already lost - and it simplifies use of this function for future things that do want to be callable by root, like BecomeMonitor for #46787. Colin, I hear you like this sort of thing :-) Created attachment 112855 [details] [review] Fix distcheck --- Can be squashed into Attachment #112843 [details] if desired. Comment on attachment 112842 [details] [review] [PATCH 1/6] Factor out some utility functions from test/dbus-daemon* Review of attachment 112842 [details] [review]: ----------------------------------------------------------------- ::: test/test-utils-glib.c @@ +1,1 @@ > +#include <config.h> Missing a copyright header which seems to be present on other files in test/. @@ +33,5 @@ > +{ > + GError *error = NULL; > + GString *address; > + gint address_fd; > + gchar *argv[] = { This could be (const gchar * const *), I think. The @binary and @configuration parameters could be (const gchar *). @@ +49,5 @@ > + NULL, /* child_setup */ > + NULL, /* user data */ > + daemon_pid, > + NULL, /* child's stdin = /dev/null */ > + &address_fd, I think @address_fd is leaked when this function returns. Shouldn’t it be closed when the GPid is closed? ::: test/test-utils-glib.h @@ +1,1 @@ > +#ifndef TEST_UTILS_GLIB_H Missing a copyright header which seems to be present on other files in test/. Comment on attachment 112843 [details] [review] [PATCH 2/6] Generate test configuration files via build-time sed, not configure Review of attachment 112843 [details] [review]: ----------------------------------------------------------------- Looks good to me. Comment on attachment 112844 [details] [review] [PATCH 3/6] test: implement GLib-style "installed tests" Review of attachment 112844 [details] [review]: ----------------------------------------------------------------- ::: test/Makefile.am @@ +457,5 @@ > +$(installable_test_meta): %.test: % Makefile > + $(AM_V_GEN) ( \ > + echo '[Test]'; \ > + echo 'Type=session'; \ > + echo 'Exec=env DBUS_TEST_HOME=$$(pwd)/home.tmp $(testexecdir)/$*'; \ Should this be $(DESTDIR)$(testexecdir)/$*? Comment on attachment 112845 [details] [review] [PATCH 4/6] Add infrastructure to run bits of tests under an alternative uid Review of attachment 112845 [details] [review]: ----------------------------------------------------------------- ::: test/test-utils-glib.c @@ +75,5 @@ > "--print-address=1", /* stdout */ > NULL > }; > +#ifdef DBUS_UNIX > + struct passwd *pwd = NULL; Should be const. @@ +103,5 @@ > + case TEST_USER_ROOT: > + break; > + > + case TEST_USER_MESSAGEBUS: > + pwd = getpwnam (DBUS_USER); Is it worth using getpwnam_r() here to avoid potential threading issues in future? ::: test/test-utils-glib.h @@ +11,5 @@ > + TEST_USER_ME, > + TEST_USER_ROOT, > + TEST_USER_MESSAGEBUS, > + TEST_USER_OTHER > +} TestUser; This could do with a brief doc comment. e.g. Who is OTHER? Comment on attachment 112846 [details] [review] [PATCH 5/6] Add a test for uid-controlled permissions Review of attachment 112846 [details] [review]: ----------------------------------------------------------------- ::: test/test-utils-glib.c @@ +278,5 @@ > + /* For now we only do tests like this on Linux, because I don't know how > + * safe this use of setresuid() is on other platforms */ > +#if defined(HAVE_GETRESUID) && defined(HAVE_SETRESUID) && defined(__linux__) > + uid_t ruid, euid, suid; > + struct passwd *pwd; const. @@ +313,5 @@ > + (unsigned long) ruid, (unsigned long) euid, (unsigned long) suid); > + return NULL; > + } > + > + pwd = getpwnam (username); Might want to use getpwnam_r() here. ::: test/uid-permissions.c @@ +1,4 @@ > +/* Integration tests for the dbus-daemon's uid-based hardening > + * > + * Author: Simon McVittie <simon.mcvittie@collabora.co.uk> > + * Copyright © 2010-2011 Nokia Corporation The © sign has been mangled here, but that might just be a Bugzilla/Splinter issue. @@ +146,5 @@ > +{ > + dbus_error_free (&f->e); > + g_clear_error (&f->ge); > + > + test_main_context_unref (f->ctx); f->conn and f->daemon_pid are leaked here. Comment on attachment 112847 [details] [review] [PATCH 6/6] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root Review of attachment 112847 [details] [review]: ----------------------------------------------------------------- Note: I do not know enough about the intricacies of the D-Bus security model to review this patch for security. My review is purely of the code. ::: bus/driver.c @@ +85,5 @@ > +static dbus_bool_t > +bus_driver_check_caller_is_privileged (DBusConnection *connection, > + BusTransaction *transaction, > + DBusMessage *message, > + DBusError *error) Would be nice to have a comment giving the high-level version of what this does, with some security justification (or a pointer to the justification in a bug report or ML or something). e.g. What does ‘privileged’ mean? @@ +97,5 @@ > + > + /* Yes this repetition is pretty horrible, but there's no > + * bus_context_log_valist() or dbus_set_error_valist() or > + * bus_context_log_literal() or dbus_set_error_literal(). > + */ Should be fairly easy to add one such method, and it would simplify the code here and below. @@ +133,5 @@ > + return TRUE; > +#elif DBUS_WIN > + char *windows_sid; > + > + if (!dbus_connection_get_windows_user (connection, &windows_sid)) @windows_sid is leaked on the success path. Comment on attachment 112855 [details] [review] Fix distcheck Review of attachment 112855 [details] [review]: ----------------------------------------------------------------- ++ (In reply to Philip Withnall from comment #9) > Missing a copyright header which seems to be present on other files in test/. Fair point, will fix. It's all from dbus-daemon.c, so it can be similarly permissive. > This could be (const gchar * const *), I think. The @binary and > @configuration parameters could be (const gchar *). I'll peer at it and work out how many casts this would need. It's possible that the answer is "yeah, but no" if the resulting code is needlessly ugly - I prefer to avoid sprinkling these casts through utility code, because C doesn't have C++'s const_cast<T> which guarantees that only the constness changed. > I think @address_fd is leaked when this function returns. Shouldn’t it be > closed when the GPid is closed? I think it can be closed as soon as we've read the address. I'll check. (In reply to Philip Withnall from comment #11) > > + echo 'Exec=env DBUS_TEST_HOME=$$(pwd)/home.tmp $(testexecdir)/$*'; \ > > Should this be $(DESTDIR)$(testexecdir)/$*? No, because when we copy $(DESTDIR)/foo to /foo (e.g. via a package build and installation), we will not change the content. It can only be correct for the final location *or* the DESTDIR staging area, not both; and for installed-tests we want the former. You'll notice that "make installcheck" is skipped if DESTDIR is non-empty, because the final location gets hard-coded in a few places, so it wouldn't work properly in the presence of a DESTDIR. (In reply to Philip Withnall from comment #12) > > + struct passwd *pwd = NULL; > > Should be const. Fair. For some reason the official prototype is that it returns non-const, but assigning a mutable pointer to a const-pointer variable is fine anyway. > > + case TEST_USER_MESSAGEBUS: > > + pwd = getpwnam (DBUS_USER); > > Is it worth using getpwnam_r() here to avoid potential threading issues in > future? The reentrant version is more complicated to use, and these tests will fall over in a heap if you try to thread them anyway (setenv() and setresuid() manipulate process-global state), so I'm going to say "no". In practice, libdbus is never going to be threaded behind the scenes like GDBus is, because dbus-daemon and the dbus-*util*.c files it uses are not thread-safe. > ::: test/test-utils-glib.h > @@ +11,5 @@ > > + TEST_USER_ME, > > + TEST_USER_ROOT, > > + TEST_USER_MESSAGEBUS, > > + TEST_USER_OTHER > > +} TestUser; > > This could do with a brief doc comment. e.g. Who is OTHER? Yeah, probably. It's hinted at in configure.ac - OTHER is any unprivileged user of the distribution integrator's choice, typically 'nobody', or maybe 'daemon' or something. The constraint is that they must fail the "is privileged for this bus" check for a bus running as 'messagebus' (except 'messagebus' also has a distribution-specific name). (In reply to Philip Withnall from comment #13) > > + struct passwd *pwd; > > const. OK > > + pwd = getpwnam (username); > > Might want to use getpwnam_r() here. As above, I'd rather not. > > + * Copyright © 2010-2011 Nokia Corporation > > The © sign has been mangled here, but that might just be a Bugzilla/Splinter > issue. As on the other bug, I'm pretty sure it's Splinter's fault. > > + test_main_context_unref (f->ctx); > > f->conn and f->daemon_pid are leaked here. Good catch, will check that. (In reply to Philip Withnall from comment #14) > > +static dbus_bool_t > > +bus_driver_check_caller_is_privileged (DBusConnection *connection, > > + BusTransaction *transaction, > > + DBusMessage *message, > > + DBusError *error) > > Would be nice to have a comment giving the high-level version of what this > does, with some security justification (or a pointer to the justification in > a bug report or ML or something). e.g. What does ‘privileged’ mean? Reasonable. I stole the security model from kdbus: it's approximately "it is usually pointless to protect against this uid because they can ptrace me anyway" :-) > > + /* Yes this repetition is pretty horrible, but there's no > > + * bus_context_log_valist() or dbus_set_error_valist() or > > + * bus_context_log_literal() or dbus_set_error_literal(). > > + */ > > Should be fairly easy to add one such method, and it would simplify the code > here and below. So you'd think. I spent a little while poking at dbus_set_error, but the combination of va_list and trying to be OOM-safe meant I would have had to do a lot of refactoring and possibly duplicate a bunch of code, and my assessment was that it would deter reviewers more than it would help. If you'd be up for reviewing it, I wouldn't mind having another go, but I'd rather not have it block functionally necessary things. > > + if (!dbus_connection_get_windows_user (connection, &windows_sid)) > > @windows_sid is leaked on the success path. Good catch, will fix. (In reply to Philip Withnall from comment #9) > [argv] could be (const gchar * const *), I think. > The @binary and @configuration parameters could be (const gchar *). g_spawn_async_with_pipes() takes a gchar **, so I can only make any of those changes if I insert a non-const-correct cast. I'll attach the resulting patch and let you decide which way you think looks less bad... > I think @address_fd is leaked when this function returns. Indeed. > Shouldn’t it be closed when the GPid is closed? It can be closed immediately. > Missing a copyright header which seems to be present on other files in test/. Fixed. (In reply to Simon McVittie from comment #17) > > const. > > OK Fixed, at the cost of yet another cast :'-( > > > + /* Yes this repetition is pretty horrible, but there's no > > > + * bus_context_log_valist() or dbus_set_error_valist() or > > > + * bus_context_log_literal() or dbus_set_error_literal(). > > > + */ > > > > Should be fairly easy to add one such method, and it would simplify the code > > here and below. > > So you'd think. I spent a little while poking at dbus_set_error, but the > combination of va_list and trying to be OOM-safe meant [bad things] Actually I was mostly just confused by the fact that dbus_set_error() supports format == NULL and worried that va_start after that would be a problem - but in fact that's no worse than calling dbus_set_error() with a literal message (no % specifiers and no following arguments). To reduce the number of malloc/free cycles, the right thing to do is actually to make a temporary DBusError on the stack, log its message, then move it into the real DBusError (which might be NULL but dbus_move_error copes with that). (In reply to Simon McVittie from comment #18) > g_spawn_async_with_pipes() takes a gchar **, so I can only make any of those > changes if I insert a non-const-correct cast. I'll attach the resulting > patch and let you decide which way you think looks less bad... Looking at the resulting patch, I think it isn't so bad. Created attachment 113053 [details] [review] [01/11] Bump required GLib version to 2.36 This is for g_close(), which the next commit will use. It also lets us rely on g_type_init() being a no-op (since 2.32 the type system is always initialized by a global constructor). Created attachment 113054 [details] [review] [02/11] Factor out some utility functions from test/dbus-daemon* In the process, make test_kill_pid() safer: do not try to terminate more than one pid, or the NULL handle. Also stop leaking the address_fd in spawn_dbus_daemon, a pre-existing bug that was spotted by Philip Withnall during review. Created attachment 113055 [details] [review] [03/11] Generate test configuration files via build-time sed, not configure --- Philip already reviewed this, as two separate patches. Created attachment 113056 [details] [review] [04/11] test: implement GLib-style "installed tests" --- Unchanged since Philip reviewed the previous version; I think his query is in fact a non-issue (see above). Created attachment 113057 [details] [review] [05/11] Add infrastructure to run bits of tests under an alternative uid --- Now with const pointers (and casts because g_spawn_async_with_pipes() is not const-correct), and documentation for the TestUser enum. Created attachment 113058 [details] [review] [06/11] Add a test for uid-controlled permissions This is technical debt from mitigating CVE-2014-8148, which should really have had a regression test at the time. --- Now more const-correct and less leaky. Created attachment 113059 [details] [review] [07/11] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root --- Now with documentation, and not leaking the Windows SID. Repetitive logging/error code will be fixed in a subsequent patch. Created attachment 113060 [details] [review] [08/11] bus: put the printf attribute in the header where it will do more good Now we can actually notice incorrect format strings in other translation units. --- New, minor fix. Created attachment 113061 [details] [review] [09/11] bus_context_log_literal: add simplified version of bus_context_log Created attachment 113062 [details] [review] [10/11] _dbus_set_error_valist: add Created attachment 113063 [details] [review] [11/11] bus_context_log_and_set_error: add and use Comment on attachment 113053 [details] [review] [01/11] Bump required GLib version to 2.36 Review of attachment 113053 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 113054 [details] [review] [02/11] Factor out some utility functions from test/dbus-daemon* Review of attachment 113054 [details] [review]: ----------------------------------------------------------------- ::: test/test-utils-glib.c @@ +110,5 @@ > + > + g_usleep (G_USEC_PER_SEC / 10); > + } > + > + g_close (address_fd, NULL); I assume this means dbus-daemon gracefully ignores EPIPE? Comment on attachment 113057 [details] [review] [05/11] Add infrastructure to run bits of tests under an alternative uid Review of attachment 113057 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 113058 [details] [review] [06/11] Add a test for uid-controlled permissions Review of attachment 113058 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 113059 [details] [review] [07/11] bus driver: factor out bus_driver_check_caller_is_privileged, and allow root Review of attachment 113059 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 113060 [details] [review] [08/11] bus: put the printf attribute in the header where it will do more good Review of attachment 113060 [details] [review]: ----------------------------------------------------------------- ++! Comment on attachment 113061 [details] [review] [09/11] bus_context_log_literal: add simplified version of bus_context_log Review of attachment 113061 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 113062 [details] [review] [10/11] _dbus_set_error_valist: add Review of attachment 113062 [details] [review]: ----------------------------------------------------------------- Commit message terse. ::: dbus/dbus-errors.c @@ +378,5 @@ > +{ > + DBusRealError *real; > + DBusString str; > + > + _dbus_assert (name != NULL); This doesn’t do the check: _dbus_return_if_error_is_set (error); Comment on attachment 113063 [details] [review] [11/11] bus_context_log_and_set_error: add and use Review of attachment 113063 [details] [review]: ----------------------------------------------------------------- ++ Updated reviews done. Any patches I haven’t explicitly re-reviewed are still ++ from before. (In reply to Simon McVittie from comment #16) > (In reply to Philip Withnall from comment #11) > > > + echo 'Exec=env DBUS_TEST_HOME=$$(pwd)/home.tmp $(testexecdir)/$*'; \ > > > > Should this be $(DESTDIR)$(testexecdir)/$*? > > No, because when we copy $(DESTDIR)/foo to /foo (e.g. via a package build > and installation), we will not change the content. It can only be correct > for the final location *or* the DESTDIR staging area, not both; and for > installed-tests we want the former. > > You'll notice that "make installcheck" is skipped if DESTDIR is non-empty, > because the final location gets hard-coded in a few places, so it wouldn't > work properly in the presence of a DESTDIR. Fair enough. Thanks for the explanation. > > > + case TEST_USER_MESSAGEBUS: > > > + pwd = getpwnam (DBUS_USER); > > > > Is it worth using getpwnam_r() here to avoid potential threading issues in > > future? > > The reentrant version is more complicated to use, and these tests will fall > over in a heap if you try to thread them anyway (setenv() and setresuid() > manipulate process-global state), so I'm going to say "no". > > In practice, libdbus is never going to be threaded behind the scenes like > GDBus is, because dbus-daemon and the dbus-*util*.c files it uses are not > thread-safe. I thought that might be the case, but thought it warranted mentioning anyway. :-) > > ::: test/test-utils-glib.h > > @@ +11,5 @@ > > > + TEST_USER_ME, > > > + TEST_USER_ROOT, > > > + TEST_USER_MESSAGEBUS, > > > + TEST_USER_OTHER > > > +} TestUser; > > > > This could do with a brief doc comment. e.g. Who is OTHER? > > Yeah, probably. It's hinted at in configure.ac - OTHER is any unprivileged > user of the distribution integrator's choice, typically 'nobody', or maybe > 'daemon' or something. The constraint is that they must fail the "is > privileged for this bus" check for a bus running as 'messagebus' (except > 'messagebus' also has a distribution-specific name). ++ for your ‘brief’ comment! (In reply to Philip Withnall from comment #32) > > + g_close (address_fd, NULL); > > I assume this means dbus-daemon gracefully ignores EPIPE? Actually no; if we have MSG_NOSIGNAL, SIGPIPE is not ignored. However, dbus-daemon should not be writing anything to its stdout, other than its address + "\n" where requested (which we do, here). If it did, it would be annoying to capture the address, because you wouldn't know what was address and what was other rubbish... (In reply to Philip Withnall from comment #38) > Comment on attachment 113062 [details] [review] > [10/11] _dbus_set_error_valist: add > > Commit message terse. Well, yes. It does what its name says: dbus_set_error(), but with a va_list :-) > ::: dbus/dbus-errors.c > @@ +378,5 @@ > > +{ > > + DBusRealError *real; > > + DBusString str; > > + > > + _dbus_assert (name != NULL); > > This doesn’t do the check: > _dbus_return_if_error_is_set (error); dbus checks (as in return_if_fail) are not intended to be used in non-public APIs, and actually assert that the first character of the function name is not "_". You're meant to use asserts instead, apparently. (I didn't write this rule.) The check is effectively done anyway, just after the error == NULL early return, because we assert that error->name and error->message are both NULL - that's a simplified version of dbus_error_is_set(). (In reply to Simon McVittie from comment #41) > (In reply to Philip Withnall from comment #32) > > > + g_close (address_fd, NULL); > > > > I assume this means dbus-daemon gracefully ignores EPIPE? > > Actually no; if we have MSG_NOSIGNAL, SIGPIPE is not ignored. However, > dbus-daemon should not be writing anything to its stdout, other than its > address + "\n" where requested (which we do, here). If it did, it would be > annoying to capture the address, because you wouldn't know what was address > and what was other rubbish... Fair enough. (In reply to Simon McVittie from comment #42) > dbus checks (as in return_if_fail) are not intended to be used in non-public > APIs, and actually assert that the first character of the function name is > not "_". You're meant to use asserts instead, apparently. (I didn't write > this rule.) > > The check is effectively done anyway, just after the error == NULL early > return, because we assert that error->name and error->message are both NULL > - that's a simplified version of dbus_error_is_set(). OK! One remaining thing: if possible, it would be great to exercise the installed-tests in gnome-continuous or via autopkgtest in Debian (or, better yet, both). Once this is merged we should look at spinning a new release and package, and talking to cwalters about whether he wants it in gnome-ci yet. Fixed in git for 1.9.8, thanks. (In reply to Philip Withnall from comment #43) > One remaining thing: if possible, it would be great to exercise the > installed-tests in gnome-continuous or via autopkgtest in Debian (or, better > yet, both). Once this is merged we should look at spinning a new release and > package, and talking to cwalters about whether he wants it in gnome-ci yet. Indeed. I'm looking into doing a 1.9.8 immediately. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.