On Wed, Apr 20, 2016 at 01:20:14PM +0200, Pavel Březina wrote:
On 04/20/2016 11:56 AM, Jakub Hrozek wrote:
> Hi Pavel,
>
> can you check if this is the right thing to do for methods that parse
> arguments on their own?
>
> To reproduce, it was enough to run:
> sudo dbus-send --print-reply --system
> --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe
> org.freedesktop.sssd.infopipe.GetUserAttr string:admin
> string:gecos
> instead of the proper:
> sudo dbus-send --print-reply --system
> --dest=org.freedesktop.sssd.infopipe /org/freedesktop/sssd/infopipe
> org.freedesktop.sssd.infopipe.GetUserAttr string:admin
> array:string:gecos
>
>
> 0001-IFP-Do-not-crash-on-invalid-arguments-to-GetUserAttr.patch
>
>
> From 220b9124e81961a3febe814a0cb70d4fcfc2ade2 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek<jhrozek(a)redhat.com>
> Date: Wed, 20 Apr 2016 11:54:31 +0200
> Subject: [PATCH] IFP: Do not crash on invalid arguments to GetUserAttr
>
> ---
> src/responder/ifp/ifpsrv_cmd.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/responder/ifp/ifpsrv_cmd.c b/src/responder/ifp/ifpsrv_cmd.c
> index
399e83e0255700df5a24491a3eb33b4f92040ca7..43dbce53c0395edfd7e8e6d87b609c52e835eaa0 100644
> --- a/src/responder/ifp/ifpsrv_cmd.c
> +++ b/src/responder/ifp/ifpsrv_cmd.c
> @@ -117,7 +117,7 @@ ifp_user_get_attr_unpack_msg(struct ifp_attr_req *attr_req)
> DBUS_TYPE_INVALID);
> if (parsed == false) {
> DEBUG(SSSDBG_OP_FAILURE, "Could not parse arguments\n");
> - return EOK; /* handled */
> + return EINVAL;
> }
>
> /* Copy the attributes to maintain memory hierarchy with talloc */
> -- 2.4.11
>
An sbus method handler returns EOK if the message was handled (reply was
sent) even if this was an error case. See:
> void
> sbus_request_invoke_or_finish(struct sbus_request *dbus_req,
> sbus_msg_handler_fn handler_fn,
> void *handler_data,
> sbus_method_invoker_fn invoker_fn)
> {
> DBusError error;
> int ret;
>
> if (invoker_fn != NULL) {
> ret = invoker_fn(dbus_req, handler_fn);
> } else if (handler_fn != NULL) {
> ret = handler_fn(dbus_req, handler_data);
> } else {
> ret = EINVAL;
> }
>
> switch(ret) {
> case EOK:
> return;
^ message was handled, therefore we do not do anything else here
> case ENOMEM:
> DEBUG(SSSDBG_CRIT_FAILURE, "Out of memory handling DBus
message\n");
> sbus_request_finish(dbus_req, NULL);
> break;
> default:
> dbus_error_init(&error);
> dbus_set_error_const(&error, DBUS_ERROR_FAILED, INTERNAL_ERROR);
> sbus_request_fail_and_finish(dbus_req, &error);
> break;
^ otherwise we report error
> }
> }
Because sbus_request is freed when you sent reply over
sbus_request_finish/fail_and_finish or whatever (which happens internally
when the params are invalid) you access freed memory in the default case if
!EOK is returned.
This is exactly what I ran into.
So you can return EINVAL in ifp_user_get_attr_unpack_msg but you need to
return EOK from ifp_user_get_attr in this case. Maybe we can create a custom
code ERR_SBUS_MESSAGE_HANDLED? for this.
OK, done. I inspected all calls of sbus_request_parse_or_finish() and I
didn't see any other place to use this error code, but it would be nice
if you checked as well during the code review.