[LEDE-DEV] ubus: ubusd segfault on lookup

[Delio Brignoli]

Hi Felix,

> On 23 Aug 2016, at 11:40, Felix Fietkau <nbd at nbd.name> wrote:
> 
> On 2016-08-18 12:19, Delio Brignoli wrote:
>> Hello Felix,
>> 
>> ubusd_handle_remove_object() currently sends object removal event
>> followed by a reply to the peer’s remove object request. However the
>> payload of the reply is the same as the object removal event. This
>> results in the peer not clearing the type id in struct ubus_object_type
>> because UBUS_ATTR_OBJTYPE is missing form the reply. If the peer
>> attempts to add the object again, the object will end up having a NULL
>> type field and a lookup will trigger a NULL pointer dereference in
>> used_prot.c:131
>> 
>> Swapping the order of ubus_proto_send_msg_from_blob() and
>> ubusd_free_object() (see diff below) the peer sees the expected
>> messages. Is this an acceptable fix for the issue? It looks like
>> ubusd_create_object() allows object with NULL type to be created,
>> probably because they are used internally, but shouldn’t it reply with
>> an error when a peer tries to add an object with a non-existent type id?
>> 
>> Thank you
>> —
>> Delio
>> 
>> 
>> diff --git a/ubusd_proto.c b/ubusd_proto.c
>> index 0af11f2..d0cfa10 100644
>> --- a/ubusd_proto.c
>> +++ b/ubusd_proto.c
>> @@ -130,8 +130,8 @@ static int ubusd_handle_remove_object(struct ubus_client *cl, struct ubus_msg_bu
>>        if (obj->type && obj->type->refcount == 1)
>>                blob_put_int32(&b, UBUS_ATTR_OBJTYPE, obj->type->id.id);
>> 
>> -       ubusd_free_object(obj);
>>        ubus_proto_send_msg_from_blob(cl, ub, UBUS_MSG_DATA);
>> +       ubusd_free_object(obj);
> I've added some fixes for NULL pointer dereference issues, however I
> don't quite understand how this patch changes anything. As far as I can
> tell, ubus_proto_send_msg_from_blob does not access obj.
> Could you please provide some more context for your proposed fix?

I’ll try explaining again but knowing which bit of my original explanation isn’t clear would help me preparing a better answer… so here we go:

Both ubusd_free_object() (eventually via ubusd_create_object_event_msg()) and ubus_proto_send_msg_from_blob() use the same message buffer. So ubusd_handle_remove_object() builds the payload which gets (indirectly) overwritten by the call to ubusd_free_object() and then sent again by ubus_proto_send_msg_from_blob().

ubusd_handle_remove_object() should result in two messages being send: 1 object removed reply to the client’s request and 1 object removed event. Instead, because of the reply’s payload being overwritten by the event message payload, the client will see a reply which never contains the type id of the object being removed which means that the reference count of the type on the client side never reaches zero. So if the type’s reference count in the daemon reaches zero the type id becomes invalid and is removed *but* the client will still think it has a valid type id and uses it next time it registers an object of the same type, at which point the daemon will attempt a lookup of the type id and fail resulting in a NULL type pointer which triggers a NULL pointer dereference in used_prot.c:131.

Swapping the order of those two function calls fixes the issue because the payload prepared by ubusd_handle_remove_object() is sent before being (indirectly) overwritten by ubusd_free_object().

Thank you
—
Delio