On 2014-08-28 14:07:48, Seth Arnold wrote: > On Thu, Aug 28, 2014 at 02:41:21AM -0500, Tyler Hicks wrote: > > Unrequested replies are message types that are typically replies, such > > as error and method_return message types, but have not been requested by > > the recipient. > > > > The AppArmor mediation code in dbus-daemon allows requested reply > > messages through if the original message was allowed. However, > > unrequested reply messages should be checked against the system policy > > to make certain that they should be allowed. > > > > This test verifies that the dbus-daemon is properly querying system > > policy when it detects that a message is an unrequested reply. > > > > Signed-off-by: Tyler Hicks <[email protected]> > > Acked-by: Seth Arnold <[email protected]> > > There's some small suggestions for usability improvements inline: > > Thanks > > > --- > > tests/regression/apparmor/Makefile | 7 +- > > tests/regression/apparmor/dbus.inc | 22 ++ > > tests/regression/apparmor/dbus_unrequested_reply.c | 221 > > +++++++++++++++++++++ > > .../regression/apparmor/dbus_unrequested_reply.sh | 126 ++++++++++++ > > 4 files changed, 374 insertions(+), 2 deletions(-) > > create mode 100644 tests/regression/apparmor/dbus_unrequested_reply.c > > create mode 100644 tests/regression/apparmor/dbus_unrequested_reply.sh > > > > diff --git a/tests/regression/apparmor/Makefile > > b/tests/regression/apparmor/Makefile > > index 13bc5d3..c9374f5 100644 > > --- a/tests/regression/apparmor/Makefile > > +++ b/tests/regression/apparmor/Makefile > > @@ -126,7 +126,7 @@ endif > > > > #only do dbus if proper libs are installl > > ifneq (,$(shell pkg-config --exists dbus-1 && echo TRUE)) > > -SRC+=dbus_eavesdrop.c dbus_message.c dbus_service.c > > +SRC+=dbus_eavesdrop.c dbus_message.c dbus_service.c > > dbus_unrequested_reply.c > > else > > $(warning ${nl}\ > > > > ************************************************************************${nl}\ > > @@ -190,7 +190,7 @@ TESTS=access \ > > > > #only do dbus if proper libs are installl > > ifneq (,$(shell pkg-config --exists dbus-1 && echo TRUE)) > > -TESTS+=dbus_eavesdrop dbus_message dbus_service > > +TESTS+=dbus_eavesdrop dbus_message dbus_service dbus_unrequested_reply > > endif > > > > # Tests that can crash the kernel should be placed here > > @@ -224,6 +224,9 @@ dbus_message: dbus_message.c dbus_common.o > > dbus_service: dbus_message dbus_service.c dbus_common.o > > ${CC} ${CFLAGS} ${LDFLAGS} $(filter-out dbus_message, $^) -o $@ > > ${LDLIBS} $(shell pkg-config --cflags --libs dbus-1) > > > > +dbus_unrequested_reply: dbus_service dbus_unrequested_reply.c dbus_common.o > > + ${CC} ${CFLAGS} ${LDFLAGS} $(filter-out dbus_service, $^) -o $@ > > ${LDLIBS} $(shell pkg-config --cflags --libs dbus-1) > > + > > tests: all > > @if [ `whoami` = "root" ] ;\ > > then \ > > diff --git a/tests/regression/apparmor/dbus.inc > > b/tests/regression/apparmor/dbus.inc > > index 539d128..57cb849 100755 > > --- a/tests/regression/apparmor/dbus.inc > > +++ b/tests/regression/apparmor/dbus.inc > > @@ -98,6 +98,28 @@ sendmethod() > > send "$bus" "method_call" "$dest" "$path" "${iface}.Method" > > } > > > > +# parameters: bus message_type destination > > +# > > +# destination must be a connection name > > +sendunrequestedreply() > > +{ > > + out=$(./dbus_unrequested_reply --$1 --type=$2 --name=$3 2>&1) > > + if [ $? -ne 0 ] > > + then > > + fatalerror "$out" > > + fi > > +} > > + > > +sendmethodreturn() > > +{ > > + sendunrequestedreply "$bus" "method_return" "$dest" > > +} > > + > > +senderror() > > +{ > > + sendunrequestedreply "$bus" "error" "$dest" > > +} > > + > > compare_logs() > > { > > local msg > > diff --git a/tests/regression/apparmor/dbus_unrequested_reply.c > > b/tests/regression/apparmor/dbus_unrequested_reply.c > > new file mode 100644 > > index 0000000..143f292 > > --- /dev/null > > +++ b/tests/regression/apparmor/dbus_unrequested_reply.c > > @@ -0,0 +1,221 @@ > > +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ > > +/* dbus_message.c Utility program to send messages from the command line > > + * > > + * Copyright (C) 2003 Philip Blundell <[email protected]> > > + * Copyright (C) 2014 Canonical, Ltd. > > + * > > + * Originally dbus-send.c from the dbus package. It has been heavily > > modified > > + * to work within the regression test framework. > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > + * > > + * You should have received a copy of the GNU General Public License > > + * along with this program; if not, write to the Free Software > > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > > 02110-1301 USA > > + * > > + */ > > + > > +#define _GNU_SOURCE > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <unistd.h> > > +#include <sys/types.h> > > +#include <sys/stat.h> > > +#include <fcntl.h> > > + > > +#include "dbus_common.h" > > + > > +DBusConnection *connection; > > +DBusError error; > > +DBusBusType type = DBUS_BUS_SESSION; > > +const char *type_str = NULL; > > +const char *name = NULL; > > +int message_type = DBUS_MESSAGE_TYPE_INVALID; > > +const char *address = NULL; > > +int session_or_system = FALSE; > > +int log_fd = -1; > > + > > +static void usage(int ecode) > > +{ > > + char *prefix = ecode ? "FAIL: " : ""; > > + > > + fprintf(stderr, > > + "%6sUsage: dbus_unrequested_reply [ADDRESS] --name=NAME > > --type=TYPE\n" > > + " ADDRESS\t\t--system, --session (default), or > > --address=ADDR\n" > > + " NAME\t\tthe message destination\n" > > + " TYPE\t\tmethod_return or error\n", > > + prefix); > > + exit(ecode); > > +} > > + > > +static int do_unrequested_reply(void) > > +{ > > + DBusMessage *message; > > + > > + if (message_type == DBUS_MESSAGE_TYPE_METHOD_RETURN) { > > + message = dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_RETURN); > > + > > + if (message) { > > + dbus_message_set_no_reply(message, TRUE); > > + > > + /* Make up an invalid reply_serial */ > > + if (!dbus_message_set_reply_serial(message, > > + 123456789)) { > > + fprintf(stderr, > > + "FAIL: Couldn't set reply_serial\n"); > > + return 1; > > + } > > + } > > + } else if (message_type == DBUS_MESSAGE_TYPE_ERROR) { > > + message = dbus_message_new(DBUS_MESSAGE_TYPE_ERROR); > > + > > + if (message) { > > + dbus_message_set_no_reply(message, TRUE); > > + > > + /* Make up an invalid reply_serial */ > > + if (!dbus_message_set_reply_serial(message, > > + 123456789)) { > > + fprintf(stderr, > > + "FAIL: Couldn't set reply_serial\n"); > > + return 1; > > + } > > + > > + /* Make up an error */ > > + if (!dbus_message_set_error_name(message, > > + DBUS_ERROR_PROPERTY_READ_ONLY)) { > > + fprintf(stderr, > > + "FAIL: Couldn't set error name\n"); > > + return 1; > > + } > > + } > > + } else { > > + fprintf(stderr, "FAIL: Internal error, unknown message type\n"); > > + return 1; > > + } > > + > > + if (message == NULL) { > > + fprintf(stderr, "FAIL: Couldn't allocate D-Bus message\n"); > > + return 1; > > + } > > + > > + if (!dbus_message_set_destination(message, name)) { > > + fprintf(stderr, "FAIL: Not enough memory\n"); > > + return 1; > > + } > > + > > + log_message(log_fd, "sent ", message); > > + dbus_connection_send(connection, message, NULL); > > + dbus_connection_flush(connection); > > + > > + dbus_message_unref(message); > > + > > + return 0; > > +} > > + > > +int main(int argc, char *argv[]) > > +{ > > + int i, rc; > > + > > + if (argc < 3) > > + usage(1); > > + > > + for (i = 1; i < argc; i++) { > > + char *arg = argv[i]; > > + > > + if (strcmp(arg, "--system") == 0) { > > + type = DBUS_BUS_SYSTEM; > > + session_or_system = TRUE; > > + } else if (strcmp(arg, "--session") == 0) { > > + type = DBUS_BUS_SESSION; > > + session_or_system = TRUE; > > + } else if (strstr(arg, "--address") == arg) { > > + address = strchr(arg, '='); > > + > > + if (address == NULL) { > > + fprintf(stderr, > > + "FAIL: \"--address=\" requires an > > ADDRESS\n"); > > + usage(1); > > + } else { > > + address = address + 1; > > + } > > + } else if (strstr(arg, "--name=") == arg) > > + name = strchr(arg, '=') + 1; > > No NULL check here.. > > > + else if (strstr(arg, "--type=") == arg) > > + type_str = strchr(arg, '=') + 1; > > No NULL check here.. > > > + else if (strstr(arg, "--log=") == arg) { > > + char *path = strchr(arg, '=') + 1; > > No NULL check here..
Thanks for the review. I'll add these checks in before committing to
trunk.
Tyler
>
> > +
> > + log_fd = open(path, O_CREAT | O_TRUNC | O_WRONLY,
> > + S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP |
> > + S_IROTH | S_IWOTH);
> > + if (log_fd < 0) {
> > + fprintf(stderr,
> > + "FAIL: Couldn't open log file \"%s\":
> > %m\n",
> > + path);
> > + exit(1);
> > + }
> > + } else if (!strcmp(arg, "--help"))
> > + usage(0);
> > + else if (arg[0] == '-')
> > + usage(1);
> > + else
> > + usage(1);
> > + }
> > +
> > + if (!name)
> > + usage(1);
> > +
> > + if (!type_str) {
> > + usage(1);
> > + } else {
> > + message_type = dbus_message_type_from_string(type_str);
> > + if (message_type != DBUS_MESSAGE_TYPE_METHOD_RETURN &&
> > + message_type != DBUS_MESSAGE_TYPE_ERROR) {
> > + fprintf(stderr,
> > + "FAIL: Message type \"%s\" is not supported\n",
> > + type_str);
> > + exit(1);
> > + }
> > + }
> > +
> > + if (session_or_system && address != NULL) {
> > + fprintf(stderr,
> > + "FAIL: \"--address\" may not be used with \"--system\"
> > or \"--session\"\n");
> > + usage(1);
> > + }
> > +
> > + dbus_error_init(&error);
> > +
> > + if (address != NULL)
> > + connection = dbus_connection_open(address, &error);
> > + else
> > + connection = dbus_bus_get(type, &error);
> > +
> > + if (connection == NULL) {
> > + fprintf(stderr,
> > + "FAIL: Failed to open connection to \"%s\" message bus:
> > %s\n",
> > + (address !=
> > + NULL) ? address : ((type ==
> > + DBUS_BUS_SYSTEM) ? "system" :
> > + "session"), error.message);
> > + dbus_error_free(&error);
> > + exit(1);
> > + } else if (address != NULL)
> > + dbus_bus_register(connection, &error);
> > +
> > + rc = do_unrequested_reply();
> > + dbus_connection_unref(connection);
> > + if (rc == 0)
> > + printf("PASS\n");
> > +
> > + exit(rc);
> > +}
> > diff --git a/tests/regression/apparmor/dbus_unrequested_reply.sh
> > b/tests/regression/apparmor/dbus_unrequested_reply.sh
> > new file mode 100644
> > index 0000000..1cfd8d4
> > --- /dev/null
> > +++ b/tests/regression/apparmor/dbus_unrequested_reply.sh
> > @@ -0,0 +1,126 @@
> > +#! /bin/bash
> > +# Copyright (C) 2013 Canonical, Ltd.
> > +#
> > +# This program is free software; you can redistribute it and/or
> > +# modify it under the terms of the GNU General Public License as
> > +# published by the Free Software Foundation, version 2 of the
> > +# License.
> > +
> > +#=NAME dbus_unrequested_reply
> > +#=DESCRIPTION
> > +# This test verifies that unrequested reply messages are not allowed
> > through.
> > +#=END
> > +
> > +pwd=`dirname $0`
> > +pwd=`cd $pwd ; /bin/pwd`
> > +
> > +bin=$pwd
> > +
> > +. $bin/prologue.inc
> > +requires_features dbus
> > +. $bin/dbus.inc
> > +
> > +service="--$bus --name=$dest $path $iface"
> > +unconfined_log="${tmpdir}/unconfined.log"
> > +confined_log="${tmpdir}/confined.log"
> > +
> > +ur_runtestbg()
> > +{
> > + local lock=${tmpdir}/lock
> > + local lockfd=-1
> > + local args=$service
> > +
> > + if [ $# -gt 2 ]
> > + then
> > + args="--log=$3 $args"
> > + fi
> > +
> > + exec {lockfd}>$lock
> > + flock -n $lockfd
> > + args="--lock-fd=$lockfd $args"
> > +
> > + runtestbg "$1" "$2" $args
> > +
> > + exec {lockfd}>&-
> > + flock -w 30 $lock true
> > + rm $lock
> > +}
> > +
> > +ur_checktestbg()
> > +{
> > + kill -SIGTERM $_pid 2>/dev/null
> > + checktestbg "$@"
> > +}
> > +
> > +ur_runchecktest()
> > +{
> > + ur_runtestbg "$@"
> > + ur_checktestbg
> > +}
> > +
> > +ur_gendbusprofile()
> > +{
> > + gendbusprofile "$confined_log w,
> > + dbus bind bus=$bus name=$dest,
> > + $@"
> > +}
> > +
> > +start_bus
> > +
> > +settest dbus_service
> > +
> > +# Start a dbus service and send unrequested method_return and error
> > messages to
> > +# the service. The service should always start and stop just fine. The test
> > +# results hinge on comparing the message log from confined services to the
> > +# message log from the initial unconfined run.
> > +
> > +# Do an unconfined run to get an "expected" log for comparisons
> > +ur_runtestbg "unrequested_reply (method_return, unconfined)" pass
> > $unconfined_log
> > +sendmethodreturn
> > +ur_checktestbg
> > +
> > +# All dbus perms are granted so the logs should be equal
> > +ur_gendbusprofile "dbus,"
> > +ur_runtestbg "unrequested_reply (method_return, dbus allowed)" pass
> > $confined_log
> > +sendmethodreturn
> > +ur_checktestbg "compare_logs $unconfined_log eq $confined_log"
> > +
> > +# Only send perm is granted so the confined service should not be able to
> > +# receive unrequested replies from the client
> > +ur_gendbusprofile "dbus send,"
> > +ur_runtestbg "unrequested_reply (method_return, send allowed)" pass
> > $confined_log
> > +sendmethodreturn
> > +ur_checktestbg "compare_logs $unconfined_log ne $confined_log"
> > +
> > +# Send and receive perms are granted so the logs should be equal
> > +ur_gendbusprofile "dbus (send receive),"
> > +ur_runtestbg "unrequested_reply (method_return, send receive allowed)"
> > pass $confined_log
> > +sendmethodreturn
> > +ur_checktestbg "compare_logs $unconfined_log eq $confined_log"
> > +
> > +# Now test unrequested error replies
> > +
> > +# Do an unconfined run to get an "expected" log for comparisons
> > +removeprofile
> > +ur_runtestbg "unrequested_reply (error, unconfined)" pass $unconfined_log
> > +senderror
> > +ur_checktestbg
> > +
> > +# All dbus perms are granted so the logs should be equal
> > +ur_gendbusprofile "dbus,"
> > +ur_runtestbg "unrequested_reply (error, dbus allowed)" pass $confined_log
> > +senderror
> > +ur_checktestbg "compare_logs $unconfined_log eq $confined_log"
> > +
> > +# Only send perm is granted so the confined service should not be able to
> > +# receive unrequested replies from the client
> > +ur_gendbusprofile "dbus send,"
> > +ur_runtestbg "unrequested_reply (error, send allowed)" pass $confined_log
> > +senderror
> > +ur_checktestbg "compare_logs $unconfined_log ne $confined_log"
> > +
> > +# Send and receive perms are granted so the logs should be equal
> > +ur_gendbusprofile "dbus (send receive),"
> > +ur_runtestbg "unrequested_reply (error, send receive allowed)" pass
> > $confined_log
> > +senderror
> > +ur_checktestbg "compare_logs $unconfined_log eq $confined_log"
> > --
> > 2.1.0
> >
> >
> > --
> > AppArmor mailing list
> > [email protected]
> > Modify settings or unsubscribe at:
> > https://lists.ubuntu.com/mailman/listinfo/apparmor
> >
> --
> AppArmor mailing list
> [email protected]
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: Digital signature
-- AppArmor mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
