Unix FD Passing

[Lennart Poettering]

On Sat, 25.04.09 23:41, Havoc Pennington (havoc.pennington at gmail.com) wrote:

> @@ -2047,7 +2166,7 @@ dbus_message_iter_get_fixed_array (DBusMessageIter  *iter,
>    _dbus_return_if_fail (_dbus_message_iter_check (real));
>    _dbus_return_if_fail (value != NULL);
>    _dbus_return_if_fail ((subtype == DBUS_TYPE_INVALID) ||
> -                         dbus_type_is_fixed (subtype));
> +                        (dbus_type_is_fixed (subtype) && subtype !=
> DBUS_TYPE_UNIX_FD));
>
> This means that a binding or app can't check dbus_type_is_fixed() to
> see if get_fixed_array/append_fixed_array will work. The simplest fix
> is probably to just go ahead and support an array of fd.

Brr. That was a beast. Arrays of fds are now supported.

> +#define DBUS_MAX_QUEUED_FDS 1024
>
> This should probably be one of the configurable limits in the bus
> config file (see 'man dbus-daemon'), probably handled the same way as
> dbus_connection_set_max_message_size() and set_max_received_size().

Indeed. Added dbus_connection_set_max_message_unix_fds() and
set_max_received_unix_fds().

They closely mimic the behaviour of the max_size functions you are
referring to. set_max_received_unix_fds() is also used to size the
fd queue array, since it doesn't make sense to have space in the array
for more fds that could be "live" at the same time.

> #ifdef HAVE_UNIX_FD_PASSING
>       /* Does fd passing even make sense with encoded data? */
>       _dbus_assert(!DBUS_TRANSPORT_CAN_SEND_UNIX_FD(transport));
> #endif
>
> Agreed, doesn't really make sense, assertion is fine. We don't support
> SSL anyhow ;-)
>
> You don't have to #ifdef this though. If we don't HAVE_UNIX_FD_PASSING
> then DBUS_TRANSPORT_CAN_SEND_UNIX_FD should always be FALSE.

Dropped the #ifdef.

> +#include "dbus-message-private.h"
>
> _dbus_message_get_network_data() basically exists to avoid including
> this header. I'd add _dbus_message_get_unix_fd_data().
>
> +          else
>  #endif
> -
>            if (socket_transport->message_bytes_written < header_len)
>
> This should just have the braces and indent the else block so it's
> clear it can be an else block; the braces are harmless if there isn't
> really an else in front.
>
> Another option is to make _dbus_message_get_unix_fd_data() return
> NULL/len=0 array of fds if there are none, and make
> _dbus_write_socket_with_unix_fds_two() nicely handle NULL/0 for the fd
> array, and then avoid any #ifdef in here.

Fixed.

> Hmm. In fact, I think it's probably wrong to have much if any #ifdef
> in this file. The runtime checks for CAN_SEND_UNIX_FD should always be
> sufficient. The #ifdef should pretty much all be in -sysdeps-unix.c,
> plus maybe some small #ifdef to avoid extra struct fields.
>
> +              if (!(transport->unix_fds = dbus_new(int, DBUS_MAX_QUEUED_FDS)))
>
> I would guess the most common number of fds to pass is 1, so
> allocating 1024 seems a little harsh. Though it is nice to avoid
> realloc headaches, which are error-prone. :-/
> (cosmetic: again, avoid test and assign on same line)

For the reasons pointed out earlier we cannot really do the usual
realloc() logic here: the kernel APIs simply drop fds when there is no
space in the fd array we pass to it. And we cannot get them back and
we cannot retry with a bigger buffer. Which means that we need to pass
an fd array to it that is as big as it could ever get right from the
beginning. That certainly sucks. I tried to minimize impact though by
allocating that array as late as possible and only when fd passing is
really negotiated. (which isn't worth much though, since in I guess in
99% of all cases fd passing *will* be negotiated.)

>
> +          if (!transport->unix_fds)
> +              if (!(transport->unix_fds = dbus_new(int, DBUS_MAX_QUEUED_FDS)))
>
> There should be braces here too, after the first if, and '== NULL'
> rather than "!"

I have split this now up anyway. i.e. the dbus_new() is in a seperate
line, which you ask for in a similar case elsewhere.

>
> +          bytes_read = _dbus_read_socket_with_unix_fds(socket_transport->fd,
> +                                                       buffer,
> socket_transport->max_bytes_read_per_iteration,
> +                                                       fds,
> +                                                       &n_fds);
>
> In dbus-transport.c,
> +#ifdef HAVE_UNIX_FD_PASSING
> +#include "dbus-sysdeps-unix.h"
> +#endif
>
> This implies that there's code in dbus-transport.c that should be in
> dbus-transport-socket.c or dbus-transport-unix.c. Basically all the
> fd-passing-related code should be in those other files. There are two
> options I can think of; abstract it for sockets and have the related
> functions fail on some sockets (TCP, Windows); or virtualize the
> functionality and implement it only in the unix domain socket
> transport.
>
> +#ifdef HAVE_UNIX_FD_PASSING
> +              /* Remove the requested number of fds from the fd queue
> +                 and attach them to the message */
>
> This block of code should really be in dbus-message.c, with a function
> called from the transport, rather than having the transport mucking
> around in DBusMessage internals.
>
> Thought: the incoming byte buffer is in DBusMessageLoader in
> dbus-message.c, not in DBusTransport. It may be that locating the
> incoming fds queue in DBusMessageLoader also makes the new code more
> analogous to the old code and just means fds are passed around
> whenever we pass the byte stream data around.

I need to think about this one.

>
> +              if (!(message->unix_fds =
> _dbus_memdup(transport->unix_fds, u * sizeof(int))))
>
> sizeof(message->unix_fds[0]) would avoid a subtle bug if someone ever
> changed the type of the array for some reason.

Hmm, I am not conviced. Unix fds are ints. Everybody knows that. Since
1970. It's highly unlikely that this will change anytime soon...

But anyway, changed.

> Also the usual point about not mixing assign and test.

Fixed.

> Patch: introduce dbus_connection_can_send_type()
>
> Even more generic would be dbus_connection_has_feature(connection,
> "UNIX_FD") ... thoughts?

Hmm. Not convinced. That would be too generic I guess. ;-)

For two reasons:

1) Having a function _can_send_type() is pretty useful for writing
generic code that can handle all datatypes the same way without having
to know which feature string maps to which datatype.

2) features might have paramaters, which wouldn't fit into this
syntax. (see below for more about nego with params). I think we
shouldn't try to guess how future exentsions look like, so I'd rather
not press their identifiers into a specific form right now.
>
> +  _dbus_return_val_if_fail (_dbus_type_is_valid(type), FALSE);
>
> We could simply return FALSE with an invalid type (in a defined way,
> without an assertion failure), which would mean that you could check
> at runtime whether *libdbus* supported the type, not only whether the
> connection did.

Changed that. And documented that people can rely on that behaviour.

> patch: unix-fd: when sending a message with unix fds verify that the
> connection can do it
>
> As we were discussing a bit earlier, I think this may be failing too
> early. I think it might be better to synthesize an error-reply
> DBusMessage, similar to what we do if a message times out. That will
> also mean that things behave consistently if our connection to the bus
> daemon supports fds, but the other app we're talking to does not, in
> which case this failure will happen later (inside the bus daemon).

Hmm, what would we do then for the API that deals with preallocated
messages? Synthesizing a DBusMessage might not be possible on OOM.

So what is more important: that
dbus_connection_send_preallocated() and dbus_connection_send() work
the same way or that you get async errors back?

> Patch: auth: add fd passing negotiation support
>
> As discussed already, s/-/_/ and maybe we do NEGOTIATE <featurename>
> instead of NEGOTIATE_UNIX_FD.

Did the s/-/_/. Left NEGOTIATE_UNIX_FD a single word however.

I still believe that making this a NEGOTIATE with arguments would send
the wrong signal to the developers: i.e. it would suggest that the
negotiation must happen in a single command at one place. However I'd
like to allow more complex nego logics implemented later. i.e. that
you can issue nego commands both before and after auth (in case you
negotiate stuff that is needed for auth vs. you want to negotiate
stuff that is security sensitive). And logic such as "try to nego A,
when it is available go on also nego B and C. If it is not available
nego D, E and after that B, but no C".

In addition, I think it would make sense to allow the different
negotiated features themselves take parameters later on. If the
negotiate feature name is already itself a parameter this becomes a
bit ugly, doesn't it? This could for example be useful to negotiate
additional protocol features such as an SHM extension for exchanging
larger memory blobs. i.e. "NEGOTIATE_SHM_SEG /foobar/waldo" which
would then tell the server that the client will use an SHM seg that
can be accessed via shm_open("/foobar/waldo").

This is just a question of syntax. Both syntaxes are mostly
equivalent. However, this is *negotiation* we are talking about,
i.e. making things future proof. Since we don't know what the future
holds for us we shouldn't try to be smart and define a syntax that
promises a certain syntax for future extensions that might turn out to
make sense then.

Or in other words: let's allow future extensions introduce their own
independant commands, with syntaxes that then make most sense to the
developoers.

> +    case DBUS_AUTH_COMMAND_NEGOTIATE_UNIX_FD:
> +      return send_error (auth, "Need to authenticate first");
>
> Before reading the code, I was imagining that negotiation was before
> auth. I think this will at least technically break the previous spec
> to do it after:
>
>       <para>
>         If authentication succeeds after exchanging DATA commands,
>         an OK command must be sent to the client.
>       </para>
>       <para>
>         The first octet received by the client after the \r\n of the OK
>         command must be the first octet of the authenticated/encrypted
>         stream of D-Bus messages.
>       </para>
>       <para>
>         The first octet received by the server after the \r\n of the BEGIN
>         command from the client must be the first octet of the
>         authenticated/encrypted stream of D-Bus messages.
>       </para>

I actually did notice that in the spec. But since it is the client
that issues the negotiation requests this is not too much of an issue
I think. If we simply update the spec to allow further commands after
the auth completed this shouldn't break any existing
implementation. If there is anything that could break, then it is
servers -- not the clients, but afaik although there are multiple
client implementations, there's only 1.5 server implementations
available (and only 1.0 of it actively used) right now, and afaics
they are not vulnerable to this.

Of course, we could move the fd nego before the auth. But somehow the
paranoid guy I am feels that unix fds are special and security
senstive and should not be enabled before auth has succeeded.

I have now fixed almost issues you raised. I'll fix a couple of more
things and update the spec and then I'd like to ask you to do a second
quick review.

Thanks,

Lennart

--
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4