Possibly relevant GDBus bug: https://bugzilla.gnome.org/show_bug.cgi?id=655664

(In reply to comment #0)
> I believe that the way D-Bus currently works is that connecting to a session
> bus instance owned by another user should fail. This sorta works but is
> currently implemented in a way by which the authentication step succeeds BUT
> the peer is immediately disconnected.. which is apparent when the peer invokes
> the Hello() method.

You're right, that is clearly not how it should be. I'd be happy to review a patch (or write one, if you'll review it + some of the backlog).

I'll try to work on a patch for this...

Any progress on this? Cosimo might steal this bug if you're not working on it?

Yeah, sorry, didn't get to do any work on this bug yet.

(In reply to comment #5)
> Yeah, sorry, didn't get to do any work on this bug yet.

I'll give a look at it then, if you don't mind

(In reply to comment #6)
> (In reply to comment #5)
> > Yeah, sorry, didn't get to do any work on this bug yet.
> 
> I'll give a look at it then, if you don't mind

I don't mind at all - thanks for looking into it!

See also http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=633652

(In reply to comment #0)
> I believe that the way D-Bus currently works is that connecting to a session
> bus instance owned by another user should fail. This sorta works but is
> currently implemented in a way by which the authentication step succeeds BUT
> the peer is immediately disconnected.. which is apparent when the peer invokes
> the Hello() method. See [1] for evidence of this.

Cosimo and I discussed this on IRC, this is a summary.

I believe (but I could be wrong - I suspect only Havoc really understands this stuff) that the intention is that the SASL stuff at the beginning provides authentication, but not authorization. (If you are bateman connecting to davidz's bus: it confirms that you are really bateman, but does not confirm that you are allowed to connect to davidz's bus.)

Then, we do the authorization (is bateman allowed to connect to this bus? no.) once the D-Bus session is fully up. In theory, this would allow sending back useful error messages. (But it's never that easy in D-Bus land.)

I started off suggesting that Cosimo should do authorization during authentication, but we've more or less come to the conclusion that that would be a layering violation (dbus-auth needs to call the DBusConnection's DBusAllowUnixUserFunction), and perhaps it can't send back useful error messages anyway (or perhaps it can, I don't know the SASL stuff in detail).

My next suggestion was to reply to Hello() with an error, then terminate the connection. However, that makes us DoS'able, unless done very carefully: bateman can connect to davidz's bus n times, where n is the fd limit for a single process, and not send Hello(); and now davidz can't connect.

Another option is to define a new signal, something like:

    signal org.freedesktop.DBus.Disconnecting(s: error_code, s: message)
    @error_code: a D-Bus error name, typically
        "org.freedesktop.DBus.Error.AccessDenied" or
        "org.freedesktop.DBus.Error.CorruptMessage"
    @message: a human-readable message describing the error in the C locale

    Emitted by org.freedesktop.DBus:/org/freedesktop/DBus just before it
    disconnects you for a protocol violation, authorization failure or
    other fatal error. This is similar to an ERROR reply, but instead of
    being a reply to a single message, it is a reply to an error that
    invalidates the entire connection, such as authorization failure
    (connecting to someone else's session bus) or sending a corrupt message.

    It is appropriate for clients to map the error code and message to
    their native representation (e.g. GError) in the same way they would
    for an ERROR reply's ERROR_NAME field and first string argument.

    Clients may assume that no more messages will follow this one.

    This message may be received before the reply to Hello().
    If it is, it will not have a DESTINATION field.

... which would also solve Bug #34726 while we're at it (by introducing the new CorruptMessage error).

Technicalities: sending the signal might be a best-effort thing: if the buffer is full or there isn't enough memory, we'd just disconnect the bad client. That should only happen to abusive clients anyway - in practice, sockets should have at least a few KiB of buffer space into which we can write the message. See Bug #34726 for more details.

Hey,

I'm not sure it's useful to make the distinction between authentication and authorization at the connection setup level (e.g. the part up to and including the BEGIN command). Specifically, the SASL authentication part is very explicitly a _proper_ subset of the whole connection setup ordeal - there's no reason that authorization shouldn't happen at that level. In fact, it's a good idea to terminate the connection as early as possible if we know that an authenticated identity is not authorized to connect.

For the record, other notable parts of the connection setup include negotiating e.g. unix fd capability and in the future a bunch of other things... surely, making decisions on whether we want to exchange FDs or accept values with the MAYBE type is even more a 'layering violation' than actually deciding if the authenticated identity is authorized.

Either way, the libdbus implementation is free to do what it wants... but I don't think inventing extensions to the connection setup part of the protocol (e.g. causing other implementations to support those extensions if we want better error handling in other implementations) is a good idea ... and note that in the GDBus implementation, we allow authorization decisions as part of the connection setup

 http://developer.gnome.org/gio/unstable/GDBusAuthObserver.html#GDBusAuthObserver.description

In fact, with the way this is set up, we can add new policy hooks without breaking ABI in GDBus (for example, a GDBusConnection::allow-maybe-types signal to determine if the connection should allow the use of MAYBE types).

Layers apart, which I'd like to keep as clean as possible, dbus-auth.c has no clue of the DBusTransport, which I somehow need in order to understand if I'm acting as session or system bus.

System buses in fact should allow connection from anybody, while the identity restriction will apply only for sessions buses.

Acting on Hello() return values, beside being DoS prone, would not really work (or it would be harder than expected) since the connection is dropped before even knowing that the method call is Hello().

The authorization steps are taken on any passing message (AFAIK), which makes sense.

I started considering adding a state to the DBusAuth automaton after AUTHENTICATED, it would be an hidden state, which would be included in the AUTH answer (OK -> both results succeeded, REJECT, one failed).

This state can be explicit or implicitly done at the end of the AUTHENTICATED state (it might be seen a layer mixup, but we can make it clear that it's authorization by big comments)

This state would use for the first time during the connection initialization the user_function, but not forcing a disconnection upon being not authorized, giving back a proper Error instead.

Authorization in SASL, for what I can see, is usually done via some callbacks, which for dbus are user functions I believe.

Not sure if it will break something spec side or for backward compatibility, as long as we don't add any handshake step the client should wait for.
It should be the same, as far as I can see.

It's actually harder than what I thought and it needs some refactoring in order to be able to call DBusAllowUnixUserFunction and the Win one from DBusAuth or a similar approach.

The user functions are in DBusTransport and need a DBusConnection.

The solution of passing to _dbus_auth_server_new() a DBusTransport* (which would be the class where DBusAuth cronstructor is call, actually) is the fastest but it would have circular references. transport->auth->transport, which I dislike.

I'd delegate the authorization step (ie the policy checking + uid/sid checking) to another class (DBusAuthorize?) which would contain the user functions and references to whatever they need (DBusConnection in primis).

This object would be instantiated from DBusTransport and be used within both DBusTransport to authorize each message when DBusAuth is already in AUTHENTICATED state (as it happens now) disconnecting on FAIL, and to authorize the first time the user in the connection during SASL, as it is required in order to REJECT the handshake ASAP.

It should be possible.
I'll go deeper in thinking about it if the solution is considered good enough.

Created attachment 51903 [details] [review]
Factor out DBusAuthorization from DBusTransport

Authorization is currently held in DBusTransport, indepentently from the authtentication process, which means that there was no way to answer REJECTED if the user's identity is not authorized; instead the connection was simply cut off.

The first step is to take those authorization bits off the transport and put them in a different structure.

Created attachment 51904 [details] [review]
Use DBusAuthorization

within DBusAuth, use DBusAuthorization at the end of the EXTERNAL mech, pretty much the same way it is done in the transport, but REJECTing instead of disconnecting.

Also the auth script is using the authorization struct, but not having any callback (user_functions) set up, it will be tested against the default_rules.

bus/connection.c will set the daemon's own callbacks which is not run by dbus/dbus-test

Created attachment 51905 [details] [review]
avoid assertions calling methods with collateral effects

This is not related to this bug.

assertions shouldn't call functions that actually act on the state of the connection. In this case if the transport is not authenticated, it will be called _do_work() which triggers the authentication automaton.

When DBUS_DISABLE_ASSERTION is set, the method would not be called.

Either this this or check only if the transport->authenticated bit is set without processing anything else are viable solution, depending on what should be done.

Created attachment 51906 [details] [review]
Fix doc

This docstring is not precise, it lacks to things:

1- a move in the auth automaton might be triggered, better specify it.

2- a bit of confusion on when the method returns TRUE:
- it will return true when it has been authenticated and then disconnected.
- but if transport->authenticated is FALSE, it will return FALSE if transport->disconnected is TRUE.

I'm aware that DBusAuthorization is lacking refs for DBusConnection, I got a bit confused about how to do it properly.

_set_connection is called with the connection's lock held, so I can use
_dbus_connection_ref_unlocked()

What about unreferencing it? During finalization the lock is not held, is it safe to hold it while calling _unref_unlocked()?
DBusConnection at this time should already be disconnected (not verified, though).

Calling the public method dbus_connection_unref() fails asserting about the dbus generation, probably shutdown has been called, which AFAIK it does not make it suitable to be used in this situation.


Also, testing is tricky since the actual user_functions are defined in 
bus/connection.c but the auth script is run in dbus/dbut-test.
SILLY_CREDENTIALS should check it, but I'm not 100% sure of it. I'll give it a look ASAP, if the approach taken with those patches does not break anything.

This is gdbus output on connecting on someone else's session bus

GDBus-debug:Address: In g_dbus_address_get_for_bus_sync() for bus type `session'
GDBus-debug:Address: env var DBUS_SESSION_BUS_ADDRESS=`unix:abstract=/tmp/dbus-QljWevEAYD,guid=22e4054d1b52f8a68b26f288000608a9'
GDBus-debug:Address: env var DBUS_SYSTEM_BUS_ADDRESS is not set
GDBus-debug:Address: env var DBUS_STARTER_BUS_TYPE is not set
GDBus-debug:Address: Returning address `unix:abstract=/tmp/dbus-QljWevEAYD,guid=22e4054d1b52f8a68b26f288000608a9' for bus type `session'
GDBus-debug:Auth: CLIENT: initiating
GDBus-debug:Auth: CLIENT: sent credentials `GCredentials:linux-ucred:pid=16704,uid=1000,gid=1000'
GDBus-debug:Auth: CLIENT: writing `AUTH\r\n'
GDBus-debug:Auth: CLIENT: WaitingForReject
GDBus-debug:Auth: CLIENT: WaitingForReject, read 'REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS'
GDBus-debug:Auth: CLIENT: Trying to choose mechanism
GDBus-debug:Auth: CLIENT: Trying mechanism `EXTERNAL'
GDBus-debug:Auth: CLIENT: writing `AUTH EXTERNAL 31303030\r\n'
GDBus-debug:Auth: CLIENT: WaitingForOK
GDBus-debug:Auth: CLIENT: WaitingForOK, read `REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS'
GDBus-debug:Auth: CLIENT: Trying to choose mechanism
GDBus-debug:Auth: CLIENT: Trying mechanism `DBUS_COOKIE_SHA1'
GDBus-debug:Auth: CLIENT: writing `AUTH DBUS_COOKIE_SHA1 31303030\r\n'
GDBus-debug:Auth: CLIENT: WaitingForData
GDBus-debug:Auth: CLIENT: WaitingForData, read=`REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS'
GDBus-debug:Auth: CLIENT: Done, authenticated=0
Error connecting: In WaitingForData: unexpected response `REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS'

While the system one authenticates.

(In reply to comment #18)
> GDBus-debug:Auth: CLIENT: sent credentials
> `GCredentials:linux-ucred:pid=16704,uid=1000,gid=1000'
> GDBus-debug:Auth: CLIENT: writing `AUTH\r\n'
> GDBus-debug:Auth: CLIENT: WaitingForReject
> GDBus-debug:Auth: CLIENT: WaitingForReject, read 'REJECTED EXTERNAL
> DBUS_COOKIE_SHA1 ANONYMOUS'
> GDBus-debug:Auth: CLIENT: Trying to choose mechanism
> GDBus-debug:Auth: CLIENT: Trying mechanism `EXTERNAL'
> GDBus-debug:Auth: CLIENT: writing `AUTH EXTERNAL 31303030\r\n'
> GDBus-debug:Auth: CLIENT: WaitingForOK
> GDBus-debug:Auth: CLIENT: WaitingForOK, read `REJECTED EXTERNAL
> DBUS_COOKIE_SHA1 ANONYMOUS'

I feel as though the REJECTED response isn't necessarily right here: the client side seems to treat REJECTED as "you're not going to get in with that mechanism, but try again". As far as I can tell, in the same situation GDBus' service-side would just summarily disconnect the client. The client can distinguish between this and a later disconnection, because authorization failure occurs before BEGIN, and the sort of disconnection that should trigger exit-on-disconnect occurs after.

Ideally, we'd perhaps negotiate a way for the server to tell the client what was wrong, with an error domain and code? Maybe something like this:

    C: AUTH
    S: REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS
    C: NEGOTIATE_FATAL
    S: AGREE_FATAL
    C: AUTH EXTERNAL 31303030
    S: FATAL org.freedesktop.DBus.Error.AccessDenied This bus belongs to uid 1234, but your authenticated uid is 1000.
    S: <disconnects, without waiting for a reply>

(I didn't include quoting in FATAL: libdbus includes double quotes whenever it says ERROR, but I don't know why, since any parsing is simpler without them. I don't think we need a quoting mechanism, we can just say that FATAL messages never include newlines.)

Or we could even remove the negotiation of support for FATAL: the server is about to disconnect the client anyway, so it doesn't matter if the client won't understand.

I'll have a look at your patches in detail.

(In reply to comment #15)
> avoid assertions calling methods with collateral effects
> 
> This is not related to this bug.
> 
> assertions shouldn't call functions that actually act on the state of the
> connection. In this case if the transport is not authenticated, it will be
> called _do_work() which triggers the authentication automaton.
> 
> When DBUS_DISABLE_ASSERTION is set, the method would not be called.
> 
> Either this this or check only if the transport->authenticated bit is set
> without processing anything else are viable solution, depending on what should
> be done.

Well spotted, this is nasty.

This is not actually a bug, since _dbus_connection_queue_received_message_link can't be called until the transport has already finished authentication and there's nothing more to do (hence the assertion) - but it's stupid that a function with a name like _dbus_transport_get_is_authenticated can ever have side-effects!

I think I'm going to apply it anyway, just for clarity (and then rename the function (on master), to have a more useful name).

(In reply to comment #19)
> (In reply to comment #18)
> > GDBus-debug:Auth: CLIENT: sent credentials
> > `GCredentials:linux-ucred:pid=16704,uid=1000,gid=1000'
> > GDBus-debug:Auth: CLIENT: writing `AUTH\r\n'
> > GDBus-debug:Auth: CLIENT: WaitingForReject
> > GDBus-debug:Auth: CLIENT: WaitingForReject, read 'REJECTED EXTERNAL
> > DBUS_COOKIE_SHA1 ANONYMOUS'
> > GDBus-debug:Auth: CLIENT: Trying to choose mechanism
> > GDBus-debug:Auth: CLIENT: Trying mechanism `EXTERNAL'
> > GDBus-debug:Auth: CLIENT: writing `AUTH EXTERNAL 31303030\r\n'
> > GDBus-debug:Auth: CLIENT: WaitingForOK
> > GDBus-debug:Auth: CLIENT: WaitingForOK, read `REJECTED EXTERNAL
> > DBUS_COOKIE_SHA1 ANONYMOUS'
> 
> I feel as though the REJECTED response isn't necessarily right here: the client
> side seems to treat REJECTED as "you're not going to get in with that
> mechanism, but try again". As far as I can tell, in the same situation GDBus'
> service-side would just summarily disconnect the client. The client can
> distinguish between this and a later disconnection, because authorization
> failure occurs before BEGIN, and the sort of disconnection that should trigger
> exit-on-disconnect occurs after.
> 
> Ideally, we'd perhaps negotiate a way for the server to tell the client what
> was wrong, with an error domain and code? Maybe something like this:
> 
>     C: AUTH
>     S: REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS
>     C: NEGOTIATE_FATAL
>     S: AGREE_FATAL
>     C: AUTH EXTERNAL 31303030
>     S: FATAL org.freedesktop.DBus.Error.AccessDenied This bus belongs to uid
> 1234, but your authenticated uid is 1000.
>     S: <disconnects, without waiting for a reply>
> 
> (I didn't include quoting in FATAL: libdbus includes double quotes whenever it
> says ERROR, but I don't know why, since any parsing is simpler without them. I
> don't think we need a quoting mechanism, we can just say that FATAL messages
> never include newlines.)

I don't think we need FATAL - for all we know, it could be that the client actually possesses a credential that will work with, say, the DBUS_COOKIE_SHA1 authentication method (or whatever authentication methods are left, if any). But the server doesn't know this so it shouldn't tell the client "nah, don't try again". Therefore, I think it's appropriate to use REJECTED.

(I do agree, however, it would be nice to include more information but retain the semantics of REJECTED, maybe REJECTED_WITH_REASON or something like that?)

(In reply to comment #21)
> I don't think we need FATAL - for all we know, it could be that the client
> actually possesses a credential that will work with, say, the DBUS_COOKIE_SHA1
> authentication method (or whatever authentication methods are left, if any).

By way of background, just to make sure everyone is using the same terminology:

Authentication is proving that you are who you say you are. It's a technical process.

Authorization is deciding whether you are allowed access. It's a policy decision.

The model used by SASL involves two identities: the identity associated with the authentication credentials (authentication identity), and an identity to act as (authorization identity). They're usually the same, but can differ (for instance, in some systems - with appropriate super-user powers - I could authenticate as smcv and be authorized to act as root, authenticate as root and be authorized to impersonate davidz, or even authenticate as smcv and be authorized to impersonate davidz).

A SASL server performs both authentication and authorization. A successful outcome of a SASL handshake is that the server believes that you are davidz (authentication) *and* is willing to let davidz connect and act as the authorization identity, which is usually also davidz (authorization).

One failed outcome of SASL is an authorization failure: "yes, I believe that you are davidz, but no, this is smcv's session bus and you're not allowed in".

Another failed outcome of SASL is an authentication failure: "you haven't proved to my satisfaction that you're davidz", or even (e.g. with Unix credentials-passing) "I know you're definitely not davidz".

---

It certainly makes sense to be able to retry after authentication failure, because the common case of authentication failure is "you still haven't proved it", and you might be able to prove it in a different way.

I don't think it necessarily makes sense to (be able to) retry after authorization failure, particularly if you don't/can't change who you're authenticating as: however many times you prove that you're davidz, you still can't connect to my session bus. It might be better to use a REJECTED_REASON extension to signal this, and let the *client* decide to stop sooner, though (if the client keeps trying, knowing it will fail, it's potentially a DoS - but I think dbus-daemon handles this correctly, by dropping the oldest authentication attempts when it runs out of incomplete-connection "slots").

Created attachment 51967 [details] [review]
Fix confusion between "is it authenticated?" and "try to  authenticate"

Historically, _dbus_transport_get_is_authenticated() has had the
side-effect of trying to advance the authentication state machine (if
there's enough buffered input to do so). This seems an inappropriate
activity for what looks like a simple getter.

Split it into _dbus_transport_try_to_authenticate (which does what it
always used to do) and _dbus_transport_peek_is_authenticated (which
is the simple getter version).

To minimize the difference in behaviour for the stable branch of D-Bus,
I've only used _dbus_transport_peek_is_authenticated where it was used
in an assertion, which should clearly not have side effects (and I've
checked that the asserting function cannot be called until both
authentication and authorization have completed). Replacing most of the
calls to try_to_authenticate with peek_is_authenticated is a possible
piece of future work (the ones in DBusConnection certainly look as
though they really want to be peek_is_authenticated).

Based on patches from Cosimo Alfarano, who noticed this
assertion-with-side-effects.

---

(In reply to comment #20)
> I think I'm going to apply it anyway, just for clarity (and then rename the
> function (on master), to have a more useful name).

On second thoughts, calling it for its side-effects looks rather silly, when what the function really wants to do is assert that the SASL dance has finished and we're in "actually D-Bus" mode: so I propose this patch for 1.4, and we can try the future work mentioned in the commit message for 1.5.

(In reply to comment #22)
> I don't think it necessarily makes sense to (be able to) retry after
> authorization failure, particularly if you don't/can't change who you're
> authenticating as: however many times you prove that you're davidz, you still
> can't connect to my session bus. It might be better to use a REJECTED_REASON
> extension to signal this, and let the *client* decide to stop sooner, though
> (if the client keeps trying, knowing it will fail, it's potentially a DoS - but
> I think dbus-daemon handles this correctly, by dropping the oldest
> authentication attempts when it runs out of incomplete-connection "slots").

I think it's entirely reasonable for the decider-component of an authorization engine to base its decision on the data exchanged in the authentication session(s) - in fact, that's exactly how the trivial "only allow uid XYZ to connect" policy is implemented - by looking at the UID passed along as data in the EXTERNAL authentication method (this is also checked against the UNIX-credentials passed along during the first NUL-byte of the connection setup per the spec - and if it does not match we ERROR out).

But you could imagine more sophisticated setups where you exchange certificates in some authentication method - for example, imagine an authentication method where the client peer presents a certificate signed by someone the system has decided to trust (using some form of CA and PKI). And suppose that your message bus daemon is configured to authorize such clients no matter what their UID is (if any). This is e.g. useful for secure transports where you can't pass UNIX-credentials - for example TCP transports.

In fact, thinking more about it, I think it's entirely unreasonable to disconnect the client just because the authorization engine decides, based on the data from the _first_ used authentication method, that the client is not authorized to connect. Not to mention that this would put an undue burden on the client implementation to try to figure out which authentication method to use first (and such guess-work never really works).

Copy the copyright information from dbus-transport.[ch] into dbus-authorization.[ch] and update it as appropriate. If you want to fish through git history for any copyright holders who didn't add themselves at the time, or extra copyright years (I'm pretty sure Red Hat employees have written copyrightable things in those files since 2003!), that'd be good too.

Include dbus-internals.h in dbus-authorization.h (even if unneeded) to signal that this is an internal header.

> +  DBusAuthorization *authorization;                    /**< Authorization conversation */

It's not a conversation. "Authorization policy"?

Perhaps call it DBusAuthorizer?

> -void
> +inline void

extern inline is non-portable (the old gcc extension and the ISO meaning are not the same) and highly confusing. Functions should either be static inline (equivalent to a macro, but type-safe) or not marked as inline at all.

The callers of these functions are in a different translation unit (.c file) so they can't be inlined anyway, unless link-time optimization is performed. Just make them normal (extern) functions - they're not on a fast-path or anything.

In _dbus_transport_set_unix_user_function, if transport->is_server is false, you'll need to free the data immediately, otherwise it'll be leaked. Same for Windows.

I'd be inclined to change the signature of _dbus_authorization_do_authorization to look like this:

    dbus_bool_t _dbus_authorization_do_authorization (DBusAuthorization *,
                                                      DBusConnection    *,
                                                      DBusCredentials   *);

and remove _dbus_authorization_set_connection - this would fix the "authorization has a borrowed ref to the connection" problem, by only giving the connection to the authorizer for just long enough to make the authorization decision, during which time the caller is meant to hold a ref. This would also remove your "maybe-FIXME".

> DBusAuthorization *_dbus_authorization_new (void);
> void _dbus_authorization_set_connection (DBusAuthorization *self,
>     DBusConnection *connection);
> DBusAuthorization * _dbus_authorization_ref (DBusAuthorization *self);
> void _dbus_authorization_unref (DBusAuthorization *self);
> void _dbus_authorization_set_unix_authorization_callback (DBusAuthorization *self,
>     DBusAllowUnixUserFunction function, void *data,
>     DBusFreeFunction free_data_function, void **old_data,
>     DBusFreeFunction *old_free_data_function);
> void _dbus_authorization_set_windows_authorization_callback (DBusAuthorization *self,
>     DBusAllowWindowsUserFunction function, void *data,
>     DBusFreeFunction free_data_function, void **old_data,
>     DBusFreeFunction *old_free_data_function);
> dbus_bool_t _dbus_authorization_do_authorization (DBusAuthorization *self, DBusCredentials *creds);
> void _dbus_authorization_set_allow_anonymous (DBusAuthorization *self,
> dbus_bool_t value);

Normal libdbus coding style involves tedious lining-up-arguments, as seen in GNU and GNOME projects. If you don't want to do that, I'll overlook it :-P

> /* Authorization functions, used as callback by SASL (implemented by
>    * DBUsAuth) */

DBusAuth

Does DBusAuthorization need to be refcounted? Does anything ever hold more than one ref?

>  * The callback will be called at the end of the EXTERNAL authentication
>  * mechanism and on every message.

"This is called after authentication, to check whether the authenticated user is authorized"?

You get 10 penguin points for using the word "predicate" correctly. :-)

>   allow = (*self->unix_authorization_cb) (self->connection,
>                                   uid,
>                                   self->unix_data);

I'd prefer "... = (self->unix_authorization_cb) (...)" or even "... = self->unix_authorization_cb (...)".

Coding style: line up the arguments below the first argument. (Yeah yeah I know.)

Pre-existing bug, but since we're unlocking and re-locking around a call out to user code, I'd feel safer if you called _dbus_connection_ref_unlocked before the unlock, and _dbus_connection_unref_unlocked before returning.

The same comments apply to the Windows version.

>   self->allow_anonymous = value != FALSE;

I'd prefer "... = (value != FALSE);".

>        if (!_dbus_credentials_add_credential (auth->authorized_identity,

Pre-existing: I can't help thinking this should be called "authenticated_identity" or "authorization_identity" (it's being used in an authorization decision, by definition it hasn't been authenticated yet!).

> +          _dbus_verbose ("%s: desired identity does not match server identity: "
> +              "not authorized\n", DBUS_AUTH_NAME (auth));
> +          return send_rejected (auth);

Misleading debug message: the reason it's authenticated might not be that it doesn't match, it might be that the user-supplied callback arbitrarily decided it doesn't like you any more :-)

This would be the place to send a more informative "REJECTED_BECAUSE" if previously negotiated.

I'd like to see a regression test (perhaps in, or based on, test/loopback.c) that uses user-supplied Unix and Windows uid-filtering functions, rejects everyone in those functions, sets a flag to say they've been called, and asserts that one or the other was called.

(In reply to comment #24)
> But you could imagine more sophisticated setups where you exchange certificates
> in some authentication method - for example, imagine an authentication method
> where the client peer presents a certificate signed by someone the system has
> decided to trust (using some form of CA and PKI).

A certificate doesn't usually authenticate that you are uid 501, it usually authenticates that you are "CN=David Zeuthen,O=Red Hat Inc." or something. In SASL-land, you can go in one of two directions from here:

* "CN=David Zeuthen,O=Red Hat Inc." (authentication identity =
  authorization identity) is authorized to connect to this session bus

* "CN=David Zeuthen,O=Red Hat Inc." (authentication identity) is authorized
  to act as uid 501 (authorization identity), and hence to connect to this
  session bus

(Or substitute "the holder of the certificate with SHA-1 = blahblahblah" as the authentication identity if you don't trust PKI.)

I believe the client is meant to state up-front who it's trying to act as (authorization identity), rather than having the server guess - except that if no authorization identity is provided, it's implicitly equal to the authentication identity.

The D-Bus Specification isn't an RFC-compliant use of SASL at the moment, because it doesn't define "the syntax and semantics of non-empty authorization identity strings". By inspection of how we implement EXTERNAL, on Unix our authorization identity string is an ASCII numeric uid, and on Windows it's a Windows SID (the hex-encoding looks as though it's meant to be considered to be part of our SASL implementation, rather than part of the authz identity).

We also don't define "how authentication identities (which are mechanism specific) and authorization identities (which are protocol specific) relate to each other", which is a SHOULD. We'd need to do this per SASL mechanism.

Our EXTERNAL implementation currently hard-codes that if you ask for an authorization identity, it must equal your authentication identity, which doesn't seem right somehow; if we want to allow root to connect to users' session buses, 

> In fact, thinking more about it, I think it's entirely unreasonable to
> disconnect the client just because the authorization engine decides, based on
> the data from the _first_ used authentication method, that the client is not
> authorized to connect.

You've convinced me that you and Cosimo are right that using REJECTED is a better way (and REJECTED_BECAUSE would be better still).

Authz failures followed by a success seem pretty unlikely, though? This is a series of authentication failures in pseudo-SASL:

    S: Welcome to Red Hat secret labs. ID please
    C: LIBRARY_CARD David
    S: No, I said ID!
    C: DRIVING_LICENSE David
    S: No, that's expired.
    C: PASSPORT David
    S: Come in.

and this is a series of authorization failures:

    S: Welcome to Red Hat secret labs. ID please
    C: PASSPORT Simon
    S: No, Simon doesn't work here.
    C: PASSPORT Cosimo
    S: No, Cosimo doesn't work here.
    C: PASSPORT David
    S: Come in.

> Our EXTERNAL implementation currently hard-codes that if you ask for an
> authorization identity, it must equal your authentication identity, which
> doesn't seem right somehow; if we want to allow root to connect to users'
> session buses,

... then it might make more sense to say "if you are authenticated as root, you are authorized to impersonate any user of your choice" (so in the authentication stage you say "I want to impersonate smcv", and the authorization callback decides - based on your credentials - whether to let you get away with it).

Amusingly, DBusTransport's default authz implementation (which Cosimo moved to DBusAuthorization) lets root connect to any DBusServer, but dbus-daemon overrides the unix_user_function, even for the session bus, to not allow that.

(In reply to comment #25)
> Copy the copyright information from dbus-transport.[ch] into
> dbus-authorization.[ch] and update it as appropriate. If you want to fish
> through git history for any copyright holders who didn't add themselves at the
> time, or extra copyright years (I'm pretty sure Red Hat employees have written
> copyrightable things in those files since 2003!), that'd be good too.

done
 
> Include dbus-internals.h in dbus-authorization.h (even if unneeded) to signal
> that this is an internal header.

done

> > +  DBusAuthorization *authorization;                    /**< Authorization conversation */
> 
> It's not a conversation. "Authorization policy"?

makes sense.


> Perhaps call it DBusAuthorizer?

yeah.

I was also tempted to rename DBusAuth in DBusAuthentication, but then I thought that it has bits of authorization inside and after the refactoring, even more.

Auth anyway is confusing, unless we mean auth*, which might be.

> > -void
> > +inline void
> 
> extern inline is non-portable (the old gcc extension and the ISO meaning are
> not the same) and highly confusing. Functions should either be static inline
> (equivalent to a macro, but type-safe) or not marked as inline at all.
> 
> The callers of these functions are in a different translation unit (.c file) so
> they can't be inlined anyway, unless link-time optimization is performed. Just
> make them normal (extern) functions - they're not on a fast-path or anything.

ok.

> In _dbus_transport_set_unix_user_function, if transport->is_server is false,
> you'll need to free the data immediately, otherwise it'll be leaked. Same for
> Windows.

true, thanks.

> I'd be inclined to change the signature of _dbus_authorization_do_authorization
> to look like this:
> 
>     dbus_bool_t _dbus_authorization_do_authorization (DBusAuthorization *,
>                                                       DBusConnection    *,
>                                                       DBusCredentials   *);
> 
> and remove _dbus_authorization_set_connection - this would fix the
> "authorization has a borrowed ref to the connection" problem, by only giving
> the connection to the authorizer for just long enough to make the authorization
> decision, during which time the caller is meant to hold a ref. This would also
> remove your "maybe-FIXME".

ok, I didn't want to change more than what needed, but it will simplify a lot.

> > DBusAuthorization *_dbus_authorization_new (void);
> > void _dbus_authorization_set_connection (DBusAuthorization *self,
> >     DBusConnection *connection);
> > DBusAuthorization * _dbus_authorization_ref (DBusAuthorization *self);
> > void _dbus_authorization_unref (DBusAuthorization *self);
> > void _dbus_authorization_set_unix_authorization_callback (DBusAuthorization *self,
> >     DBusAllowUnixUserFunction function, void *data,
> >     DBusFreeFunction free_data_function, void **old_data,
> >     DBusFreeFunction *old_free_data_function);
> > void _dbus_authorization_set_windows_authorization_callback (DBusAuthorization *self,
> >     DBusAllowWindowsUserFunction function, void *data,
> >     DBusFreeFunction free_data_function, void **old_data,
> >     DBusFreeFunction *old_free_data_function);
> > dbus_bool_t _dbus_authorization_do_authorization (DBusAuthorization *self, DBusCredentials *creds);
> > void _dbus_authorization_set_allow_anonymous (DBusAuthorization *self,
> > dbus_bool_t value);
> 
> Normal libdbus coding style involves tedious lining-up-arguments, as seen in
> GNU and GNOME projects. If you don't want to do that, I'll overlook it :-P
> 
> > /* Authorization functions, used as callback by SASL (implemented by
> >    * DBUsAuth) */
> 
> DBusAuth
> 
> Does DBusAuthorization need to be refcounted? Does anything ever hold more than
> one ref?

Nope, I'm refcounting it since the same was done for DBusAuth, which is used only in DBusTransport with one refrence (and then tested in dbus-auth-script.c but actually never really used).

I can remove it for simplicity, leave it or remove it from both (which I was tempted to, when I noticed it).

> >  * The callback will be called at the end of the EXTERNAL authentication
> >  * mechanism and on every message.
> 
> "This is called after authentication, to check whether the authenticated user
> is authorized"?

Yes, I was going to add a mention to the fact that the check is done for every message too, but it actually seems not true.

Which turns useless calling _dbus_authorization_do_authorization() in the transport, since either
* it already passed authorization in EXTERNAL (making it useless) or 
* it will never reach that part of code, being REJECTED

once the transport->authenticated is TRUE, it will always return immediately, ie, without authorizing anymore.

Going to cut a bit of more code there.

I fixed also dbus_connection_set_windows_user_function() doc, which still mentioned a disconnection on fail.


> You get 10 penguin points for using the word "predicate" correctly. :-)


Not me unfortunately, it was a comment move from transport's documentation :)

> >   allow = (*self->unix_authorization_cb) (self->connection,
> >                                   uid,
> >                                   self->unix_data);
> 
> I'd prefer "... = (self->unix_authorization_cb) (...)" or even "... =
> self->unix_authorization_cb (...)".

Me too, but in other places (DBusAuth and DBusConnection at least) it's used that way, I kept it consistent. No problem to change it if you insist ;)

> Coding style: line up the arguments below the first argument. (Yeah yeah I
> know.)
> 
> Pre-existing bug, but since we're unlocking and re-locking around a call out to
> user code, I'd feel safer if you called _dbus_connection_ref_unlocked before
> the unlock, and _dbus_connection_unref_unlocked before returning.

Done. not it's around the cb call, to be sure.

transport_get_is_authenticated did already that but DBusAuth didn't and anyway the transport won't authorize directly anymore (which probably means that we can remove the connection ref/unref there? by the comment it seemed to be there just because of user callbacks)
 
> >   self->allow_anonymous = value != FALSE;
> I'd prefer "... = (value != FALSE);".

Just a copied code, but yeah.

(In reply to comment #26)
> >        if (!_dbus_credentials_add_credential (auth->authorized_identity,
> 
> Pre-existing: I can't help thinking this should be called
> "authenticated_identity" or "authorization_identity" (it's being used in an
> authorization decision, by definition it hasn't been authenticated yet!).

I had the impression that there was a distinction between authorization at connect time and policy as two different stages.

Actually the real authentication part (checking the user is actually who is asserting to be) is not done in DBusAuth, It's pretty much all authorization and verify that the conversation makes sense.

The real Authentication part is done in dbus/dbus-sysdeps-unix.c (for unix) 
_dbus_read_credentials_socket() for instance, since the syscall on the socket will return a trustworthy value, and this method is called before the DBusAuth machine is fired.
For windows it seems all a bit fake, so no real trustworthy SID is obtained in dbys-sysdeps.win.c, but DBusAuth does not care. I might not understand it, though.

Under this assumption, DBusAuth and DBusAuhtorization (or Authorizer) might be merged together only as Authorizer, we remove a struct and clearify that it's actually an authorization process, while the authentication is preventively done . This to call things with their own name.

Otherwise I might try to check if more refactoring is possible bringing the authentication part in DBusAuth, but it really seems useless to mess up with it.

Does it all make sense? I'm a bit foggy today ;)

(In reply to comment #30)
> Which turns useless calling _dbus_authorization_do_authorization() in the
> transport, since either
> * it already passed authorization in EXTERNAL (making it useless) or 
> * it will never reach that part of code, being REJECTED

Careful. Remember, there's more than one way to authenticate as a given uid - EXTERNAL and DBUS_COOKIE_SHA1, at least. You need to make sure that authorization is done at least once for whatever authentication mechanism is used.

(I agree it'd be better to authorize exactly once if we can, though.)

> I had the impression that there was a distinction between authorization at
> connect time and policy as two different stages.

Policy is a general term, in my mind: bus/policy.c implements both the "is this Unix/Windows user OK?" callbacks (policy for who can connect at all), and policy for individual messages/bus-name-owning transactions.

> Actually the real authentication part (checking the user is actually who is
> asserting to be) is not done in DBusAuth, It's pretty much all authorization
> and verify that the conversation makes sense.

This might be true for EXTERNAL, but DBUS_COOKIE_SHA1 does the authentication at a later stage, I think?

> The real Authentication part is done in dbus/dbus-sysdeps-unix.c (for unix) 
> _dbus_read_credentials_socket() for instance, since the syscall on the socket
> will return a trustworthy value, and this method is called before the DBusAuth
> machine is fired.

Yes, EXTERNAL authentication is a special case - think of it as saying "you already know who I am".

> For windows it seems all a bit fake, so no real trustworthy SID is obtained in
> dbys-sysdeps.win.c, but DBusAuth does not care. I might not understand it,
> though.

I think Windows people mostly use the nonce-tcp: transport instead of tcp:. I'm not entirely clear on what it does - perhaps it adds a redundant authentication layer and uses that to establish trust for EXTERNAL?

(Cc Ralf: how is nonce-tcp formed? Is it better than doing DBUS_COOKIE_SHA1 authentication - which is essentially proof of shared home directory ownership - over a tcp: transport?)

(In reply to comment #32)
> (In reply to comment #30)
> > Which turns useless calling _dbus_authorization_do_authorization() in the
> > transport, since either
> > * it already passed authorization in EXTERNAL (making it useless) or 
> > * it will never reach that part of code, being REJECTED
> 
> Careful. Remember, there's more than one way to authenticate as a given uid -
> EXTERNAL and DBUS_COOKIE_SHA1, at least. You need to make sure that
> authorization is done at least once for whatever authentication mechanism is
> used.

Yes, but this was needed anyway if we need to REJECT on not authorized credentials when SHA1 is chosen. Otherwise on SHA1, connection will be cut as before, which I suppose it's not what want anyway.

Yes, each mech is now responsible to use DBusAuthorization (unless it can be called in a common place after specific mech code is executed, I'll check).

> > Actually the real authentication part (checking the user is actually who is
> > asserting to be) is not done in DBusAuth, It's pretty much all authorization
> > and verify that the conversation makes sense.
> 
> This might be true for EXTERNAL, but DBUS_COOKIE_SHA1 does the authentication
> at a later stage, I think?

Yes, EXTERNAL authentication is "contextual", while SHA1 is a proper alice-bob conversation.

The point is that DBusAuth is mixed Authentication and Authorization, depending on the mech, so (renaming proposal a part) why not merging the two structs anyway?

Unless the new DBusAuthorization is actually meant to be the "user's authorization policy", from this POV, it can be either way.
SASL authorization anyway delegates the user('s library) to authorize the identity.


>> I'd be inclined to change the signature of >>_dbus_authorization_do_authorization
>> to look like this:
>> 
>>     dbus_bool_t _dbus_authorization_do_authorization (DBusAuthorization *,
>>                                                       DBusConnection    *,
>>                                                       DBusCredentials   *);
>> 
>> and remove _dbus_authorization_set_connection - this would fix the
>> "authorization has a borrowed ref to the connection" problem, by only giving
>> the connection to the authorizer for just long enough to make the authorization
>> decision, during which time the caller is meant to hold a ref. This would also
>> remove your "maybe-FIXME".
>ok, I didn't want to change more than what needed, but it will simplify a lot

This actually moves the ref problem for the connection into DBusAuth, which has no connection reference and would need one now.

Created attachment 52219 [details] [review]
Factor out DBusAuthorization from DBusTransport

Fixed Simon suggestions with the exception of connection references.

connection is needed by user functions to be called, hence either DBusAuthorization or DBusAuth need to have DBusConnetion as struct member.

Since the user callback is called from the mech, a connection needs to be at reach (in this case by _do_authorization())

Created attachment 52220 [details] [review]
Use DBusAuthorization

Same here, Simon review's suggestion applied

Created attachment 52221 [details] [review]
rename authorized_identity

Applying Simon's suggestion of renaming authorized_identity into authenticated_identity.

Here the authorization is done only as last step, right before the lifetime for the member being over (after authorization, it doesn't matter who we are for DBusAuth, either we are in or we are out)

Created attachment 52222 [details] [review]
Allow ANONONYMOUS in tests

This fixes testes to allow ANONYMOUS to go through authorization, which wasn't done by the mech before.

Now mechs are responsible for authorizing the identity, which means that anonymous connection needs to be explicitly enabled in order to default_rules to authorized ANONYMOUSly

Created attachment 52223 [details] [review]
Do not authorize twice

Not that the authorization is within SASL mechs, there is no need to reauthorized in the transport

Created attachment 52224 [details] [review]
no refcounting for DBusAuth and DBusAuthorization

Refcounting for DBusAuth and DBusAuthorization are useless.

The lifecycle for those two struct is the same of DBusTransport and private to it, there is no possibility of external reference.

From this point of view, referencing the connection within DBusAuthorization is useless (beside being tricky unreferencing it due to connection/transport coupling).

We just keep referencing and lockiong the connection when user's callback are called.

Created attachment 52225 [details] [review]
free window's callback as well

on connection finalization we don't know what the user set, it's safer to unset both unix and windows CB here

I forgot to mention that now all the three SASL mechs devined in DBusAuth now do authorization, this is why tests needs to take care of the allow_anonymous bit now and that I removed completely the authorization part within the transport

Created attachment 52469 [details] [review]
fix connection_set_allow_anonymous doc

Mechs steps have been changed, it's worth to mention that set_allow_anonymous does not grant anymore the OK.

Now that the authorization step can REJECT, if the server sets any user_function there are good chances that the authorization will fail.
This at least because the user_functions cannot check whether the allow_anon bit is set.

So unless the default function (default_rules) or weak user_function is used, it won't be enough.

NOTE: This might break API compatibility or specs, any comment?

dbus_connection_allows_anonymous() can be introduced if needed.

Comment on attachment 52219 [details] [review]
Factor out DBusAuthorization from DBusTransport

Review of attachment 52219 [details] [review]:
-----------------------------------------------------------------

Looks OK so far

Comment on attachment 52220 [details] [review]
Use DBusAuthorization

Review of attachment 52220 [details] [review]:
-----------------------------------------------------------------

Still looking good so far.

::: dbus/dbus-auth.c
@@ +1139,5 @@
> +        }
> +      else
> +        {
> +          _dbus_verbose ("%s: desired identity does not match server identity: "
> +                         "not authorized\n", DBUS_AUTH_NAME (auth));

This should say something like "authenticated identity was not authorized by server", I think?

Comment on attachment 52220 [details] [review]
Use DBusAuthorization

Review of attachment 52220 [details] [review]:
-----------------------------------------------------------------

(In reply to comment #44)
> Still looking good so far.

Actually, not quite...

::: dbus/dbus-auth.c
@@ +1139,5 @@
> +        }
> +      else
> +        {
> +          _dbus_verbose ("%s: desired identity does not match server identity: "
> +                         "not authorized\n", DBUS_AUTH_NAME (auth));

This should say something like "authenticated identity was not authorized by server", I think?

@@ +1247,5 @@
> + 
> +  /* Do a authorization of the transport, in order to REJECT immediately the
> +   * connection if needed (FDO#39720) */
> +  if (!_dbus_authorization_do_authorization (DBUS_AUTH_SERVER (auth)->authorization,
> +                                             auth->authenticated_identity))

This is a behaviour change, because _dbus_authorization_do_authorization() doesn't normally allow anonymous users.

I think the correct logic would be something like this in _dbus_authorization_do_authorization():

if (_dbus_credentials_are_anonymous (auth_identity))
  allow = self->allow_anonymous;
else if (self->unix_authorization_cb ...)
  allow = ...;
else if (self->windows_authorization_cb ...)
  allow = ...;
else
  allow = auth_via_default_rules (...);

Comment on attachment 52221 [details] [review]
rename authorized_identity

Review of attachment 52221 [details] [review]:
-----------------------------------------------------------------

Sure

Comment on attachment 52222 [details] [review]
Allow ANONONYMOUS in tests

Review of attachment 52222 [details] [review]:
-----------------------------------------------------------------

Yeah, fine

Comment on attachment 52223 [details] [review]
Do not authorize twice

Review of attachment 52223 [details] [review]:
-----------------------------------------------------------------

This is the one we need to be most careful with, because it's removing restrictions. :-)

It does look correct, though, except:

::: dbus/dbus-transport.c
@@ -570,5 @@
>        if (transport->disconnected)
>          return FALSE;
>  
> -      /* paranoia ref since we call user callbacks sometimes */
> -      _dbus_connection_ref_unlocked (transport->connection);

You've removed the matching unref at the end, but not removed some matching unrefs in error paths (I can see at least the "Client expected GUID '%s' and we got '%s' from the server\n" case).

Comment on attachment 52224 [details] [review]
no refcounting for DBusAuth and DBusAuthorization

Review of attachment 52224 [details] [review]:
-----------------------------------------------------------------

OK

Comment on attachment 52225 [details] [review]
free window's callback as well

Review of attachment 52225 [details] [review]:
-----------------------------------------------------------------

Looks good

(In reply to comment #42)
> Mechs steps have been changed, it's worth to mention that set_allow_anonymous
> does not grant anymore the OK.

I don't think this is acceptable; but if you do what I suggested in Comment #45, you won't need to change the docs, because the behaviour hasn't changed.

I think this needs some integration tests, either automatic or manual: in particular, we should check that behaviour hasn't changed, except in the one case where we wanted it to (making authenticated-but-not-authorized connections change from OK-then-disconnect to REJECT).

(In reply to comment #45)
> This is a behaviour change, because _dbus_authorization_do_authorization()
> doesn't normally allow anonymous users.

This is not, in fact, true. If the auth identity is anonymous, then it contains neither a Unix uid nor a Windows sid, so _dbus_authorization_do_authorization() falls through to auth_via_default_rules(), which respects allow_anonymous.

The thing that's weird, noted in Attachment #52469 [details], is not new: if a unix_user_function or a windows_user_function has been set, then it can reject certain uids or sids, even if anonymous authentication would have been allowed! So I think Attachment #52469 [details] is also fine.

I wrote a manual test for authentication/authorization, and everything seemed OK after applying Cosimo's patches. I'll attach that test, and its results, here.

So the only things needing fixing are:

(In reply to comment #44)
> > +          _dbus_verbose ("%s: desired identity does not match server identity: "
> > +                         "not authorized\n", DBUS_AUTH_NAME (auth));
> 
> This should say something like "authenticated identity was not authorized by
> server", I think?

Trivial, not very important

(In reply to comment #48)
> ::: dbus/dbus-transport.c
> @@ -570,5 @@
> >        if (transport->disconnected)
> >          return FALSE;
> >  
> > -      /* paranoia ref since we call user callbacks sometimes */
> > -      _dbus_connection_ref_unlocked (transport->connection);
> 
> You've removed the matching unref at the end, but not removed some matching
> unrefs in error paths (I can see at least the "Client expected GUID '%s'
> and we got '%s' from the server\n" case).

This is a potential use-after-free and needs fixing

Here are some test results.

I ran servers under my own uid, and connected to them with both Unix and TCP
sockets from my own and another uid. I used GDBus, which tries another
authentication mechanism if the first one is REJECTed.

Key to servers:
- normal: default policy
- anon-allowed: allow_anonymous(TRUE)
- anon-only: allow_anonymous(TRUE), Unix/Windows user functions reject all
- anon mech only: ANONYMOUS is the only allowed mechanism,
                  allow_anonymous(TRUE)
- anon disallowed: ANONYMOUS is the only allowed mechanism but the
                   anonymous user is not authorized
- permissive: Unix/Windows user functions allow all
- unhappy: Unix/Windows user functions reject all
- same-uid: Unix user function only allows the dbus-daemon uid
- same-uid or anon: allow_anonymous(TRUE), same Unix user function as same-uid

Key to results:
- me: authenticated and authorized as the same user as the server
- other: authenticated and authorized as the other user
- anon: authenticated and authorized as ANONYMOUS
- closed: not authenticated, or authenticated but not authorized;
          failure is reported as "OK" followed by the connection being
          closed ("Error: The connection is closed" in GDBus)
- rejected: all auth attempts were REJECTed and the client gave up
            ("Error connecting: Exhausted all available authentication
            mechanisms" in GDBus)

With current master:

                   me, Unix    other, Unix     me, TCP    other, TCP
normal               me           closed         me         closed
anon-allowed         me           other[1]       me          anon[2]
anon-only          closed[3]      closed[3]    closed[3]     anon[2]
anon mech only      anon           anon         anon         anon
anon disallowed    closed         closed       closed       closed
permissive           me           other          me         closed[4]
unhappy            closed         closed       closed       closed
same-uid             me           closed         me         closed
same-uid or anon     me           closed[3]      me          anon[2]

[1] This only works because allow_anonymous(TRUE) changes the default
    policy to allow all uids too

[2] EXTERNAL cannot be used over TCP; DBUS_COOKIE_SHA1 is not
    authenticated because it can only prove that you are the same uid
    as the server, not that you are another uid; so ANONYMOUS is used

[3] This is a bit silly; the client could have authenticated as ANONYMOUS
    if it had known that specifying the user ID would fail

[4] The TCP client cannot authenticate except as the unauthorized anonymous
    user, for the same reason as [2]

With Cosimo's patches:

                   me, Unix    other, Unix     me, TCP    other, TCP
normal               me          rejected        me        rejected
anon-allowed         me           other[1]       me          anon[2]
anon-only           anon[5]        anon[5]     anon[5]       anon[2]
anon mech only      anon           anon         anon         anon
anon disallowed   rejected       rejected     rejected     rejected
permissive           me           other          me        rejected[4]
unhappy           rejected       rejected     rejected     rejected
same-uid             me          rejected        me        rejected
same-uid or anon     me            anon[5]       me          anon[2]

[5] non-anonymous connection is not authorized and is rejected, so the
    client falls back to ANONYMOUS and succeeds

All cases without footnote [5] are the same as before, except that
closed becomes rejected, as requested in Comment #0. So I think the branch
is good, if it gets the fixes from Comment #53.

Created attachment 57713 [details] [review]
Add a simple manual test for authentication/authorization

Here's the manual test-case I added, please review.

(In reply to comment #53)
> So the only things needing fixing are:

I've picked these up myself. Code review required.

> > This should say something like "authenticated identity was not authorized by
> > server", I think?

Done

> (In reply to comment #48)
> > You've removed the matching unref at the end, but not removed some matching
> > unrefs in error paths (I can see at least the "Client expected GUID '%s'
> > and we got '%s' from the server\n" case).
> 
> This is a potential use-after-free and needs fixing

Done, and added a test which crashes without the fix/passes with it.

Created attachment 58314 [details] [review]
trivial: re-word authorization failure message

Created attachment 58315 [details] [review]
_dbus_transport_get_is_authenticated: avoid unnecessary  unref

The commit "Remove transport's call to
_dbus_authorization_do_authorization()" removed a ref/unref pair
in the normal code path, but did not remove the corresponding unref
from an error case.

---

This could be squashed into Attachment #52223 [details].

Created attachment 58316 [details] [review]
Add a test-case for trying to connect with the wrong  GUID

---

This catches the bug fixed by Attachment #58315 [details].

Comment on attachment 51967 [details] [review]
Fix confusion between "is it authenticated?" and "try to  authenticate"

Review of attachment 51967 [details] [review]:
-----------------------------------------------------------------

I like it and it makes sense. I don't understand the reason for "despite its name" note, now that it's called "try_to_authenticate": isn't it clear that there's an action involved after the renaming?

(In reply to comment #60)
> I don't understand the reason for "despite its
> name" note, now that it's called "try_to_authenticate": isn't it clear that
> there's an action involved after the renaming?

I meant that as "despite its name, dbus_connection_get_is_authenticated() has side-effects" (and the same for get_unix_user(), get_unix_process_id(), get_adt_audit_session_data(), get_windows_user()).

You're right, it'd be clearer if I replaced "this function" with the actual function name (or at least its unique tail) in each case.

Any opinions (from anyone...) on my four other patches here, which follow/fix your branch?

(In reply to comment #61)
> Any opinions (from anyone...) on my four other patches here, which follow/fix
> your branch?

This is not going to be in 1.6.0, I've given up on waiting for feedback.

(In reply to comment #61)
> Any opinions (from anyone...) on my four other patches here, which
> follow/fix your branch?

Comment on attachment 58314 [details] [review]
trivial: re-word authorization failure message

Review of attachment 58314 [details] [review]:
-----------------------------------------------------------------

looks good

Comment on attachment 52219 [details] [review]
Factor out DBusAuthorization from DBusTransport

Review of attachment 52219 [details] [review]:
-----------------------------------------------------------------

do not apply to git master, conflicts in dbus/Makefile.am and dbus/dbus-transport.c

Comment on attachment 52221 [details] [review]
rename authorized_identity

Review of attachment 52221 [details] [review]:
-----------------------------------------------------------------

error: patch failed: dbus/dbus-auth.c:1132
error: dbus/dbus-auth.c: patch does not apply

Comment on attachment 52223 [details] [review]
Do not authorize twice

Review of attachment 52223 [details] [review]:
-----------------------------------------------------------------

error: patch failed: dbus/dbus-auth.c:1132
error: dbus/dbus-auth.c: patch does not apply

Comment on attachment 52224 [details] [review]
no refcounting for DBusAuth and DBusAuthorization

Review of attachment 52224 [details] [review]:
-----------------------------------------------------------------

error: patch failed: dbus/dbus-auth.c:2358
error: dbus/dbus-auth.c: patch does not apply
error: patch failed: dbus/dbus-authorization.c:29
error: dbus/dbus-authorization.c: patch does not apply
error: patch failed: dbus/dbus-authorization.h:33
error: dbus/dbus-authorization.h: patch does not apply
Patch failed at 0001 Remove refcounting from DBusAuth and DBusAuthorization

Comment on attachment 57713 [details] [review]
Add a simple manual test for authentication/authorization

Review of attachment 57713 [details] [review]:
-----------------------------------------------------------------

error: patch failed: test/Makefile.am:95
error: test/Makefile.am: patch does not apply
Patch failed at 0001 Add a simple manual test for authentication/authorization

Remaining patches does not apply because of failed earlier patches. Need to be rebased to recent git master.

Also applying patches is very hard because file names do not match entries displayed attachement list and are number prefix are not ordered ascending, which makes git am 39720/*.patch impossible.

Comment on attachment 52225 [details] [review]
free window's callback as well

Review of attachment 52225 [details] [review]:
-----------------------------------------------------------------

may be applied independent of the other patches.

Comment on attachment 52225 [details] [review]
free window's callback as well

committed, as this has already been reviewed by simon.

Comment on attachment 51967 [details] [review]
Fix confusion between "is it authenticated?" and "try to  authenticate"

Review of attachment 51967 [details] [review]:
-----------------------------------------------------------------

The comments "Note that despite its name,..." should be removed.

Comment on attachment 51967 [details] [review]
Fix confusion between "is it authenticated?" and "try to  authenticate"

removed note and commited to master

Comment on attachment 52219 [details] [review]
Factor out DBusAuthorization from DBusTransport

rebased and applied to git master

Comment on attachment 52220 [details] [review]
Use DBusAuthorization

fixed review issue and commited to git master

Comment on attachment 52221 [details] [review]
rename authorized_identity

fixed patch and applied to master

Comment on attachment 52222 [details] [review]
Allow ANONONYMOUS in tests

applied to master

Created attachment 84480 [details] [review]
Remove transport-s-call-to-dbus-authorization (update)

rebased

Comment on attachment 52224 [details] [review]
no refcounting for DBusAuth and DBusAuthorization

rebased and applied to git master

Comment on attachment 52469 [details] [review]
fix connection_set_allow_anonymous doc

Review of attachment 52469 [details] [review]:
-----------------------------------------------------------------

looks good.

Comment on attachment 52469 [details] [review]
fix connection_set_allow_anonymous doc

applied to git master

Comment on attachment 57713 [details] [review]
Add a simple manual test for authentication/authorization

Review of attachment 57713 [details] [review]:
-----------------------------------------------------------------

rebased patch, which now compiles and runs on linux - looks good so far except the one check on which test test app print the following message and hangs. 

** Message: Same-UID-or-anon server:
tcp:host=127.0.0.1,port=56081,guid=5f604d1b5a41540ebd4000cf5216ae6f

Comment on attachment 58315 [details] [review]
_dbus_transport_get_is_authenticated: avoid unnecessary  unref

Review of attachment 58315 [details] [review]:
-----------------------------------------------------------------

I'm going to merge this into the related patch.

Created attachment 84481 [details] [review]
Remove transport-s-call-to-dbus-authorization (update2)

Comment on attachment 57713 [details] [review]
Add a simple manual test for authentication/authorization

rebased and applied

Comment on attachment 58314 [details] [review]
trivial: re-word authorization failure message

rebased and applied

Comment on attachment 58316 [details] [review]
Add a test-case for trying to connect with the wrong  GUID

looks good, applied to master

(In reply to comment #83)
> Comment on attachment 57713 [details] [review] [review]
> Add a simple manual test for authentication/authorization
> 
> Review of attachment 57713 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> rebased patch, which now compiles and runs on linux - looks good so far
> except the one check on which test test app print the following message and
> hangs. 
> 
> ** Message: Same-UID-or-anon server:
> tcp:host=127.0.0.1,port=56081,guid=5f604d1b5a41540ebd4000cf5216ae6f

Got it: the test app only starts server, print out the server addresses to which a dbus client should connect to ?

Comment on attachment 84481 [details] [review]
Remove transport-s-call-to-dbus-authorization (update2)

applied

(In reply to comment #89)
> (In reply to comment #83)
> > Comment on attachment 57713 [details] [review] [review] [review]
> > Add a simple manual test for authentication/authorization
> > 
> > Review of attachment 57713 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > rebased patch, which now compiles and runs on linux - looks good so far
> > except the one check on which test test app print the following message and
> > hangs. 
> > 
> > ** Message: Same-UID-or-anon server:
> > tcp:host=127.0.0.1,port=56081,guid=5f604d1b5a41540ebd4000cf5216ae6f
> 
> Got it: the test app only starts server, print out the server addresses to
> which a dbus client should connect to ?

I tried to connect to the "normal" server on linux with a different user and got

24262: [dbus/dbus-auth.c(2201):process_command] server: got command "AUTH EXTERNAL 31303030"
24262: [dbus/dbus-auth.c(1718):handle_auth] server: Trying mechanism EXTERNAL
24262: [dbus/dbus-auth.c(1638):process_data] server: data: '1000'
24262: [dbus/dbus-userdb.c(166):_dbus_user_database_lookup] No cache for UID 1002
24262: [dbus/dbus-userdb.c(166):_dbus_user_database_lookup] No cache for UID 1000
24262: [dbus/dbus-auth.c(1144):handle_server_data_external_mech] server: desired identity not found in socket credentials
24262: [dbus/dbus-auth.c(430):shutdown_mech] server: Shutting down mechanism EXTERNAL


24262: [dbus/dbus-auth.c(2201):process_command] server: got command "AUTH DBUS_COOKIE_SHA1 31303030"
24262: [dbus/dbus-auth.c(1718):handle_auth] server: Trying mechanism DBUS_COOKIE_SHA1
24262: [dbus/dbus-auth.c(1638):process_data] server: data: '1000'
24262: [dbus/dbus-userdb.c(156):_dbus_user_database_lookup] Using cache for UID 1000 information
24262: [dbus/dbus-userdb.c(156):_dbus_user_database_lookup] Using cache for UID 1000 information
Using your real home directory for testing, set DBUS_TEST_HOMEDIR to avoid
24262: [dbus/dbus-keyring.c(805):_dbus_keyring_new_for_credentials] didn't load an existing keyring: No such file or directory
24262: [dbus/dbus-keyring.c(818):_dbus_keyring_new_for_credentials] Creating keyring directory: Failed to create directory /home/admin/.dbus-keyrings: Permission denied

24262: [dbus/dbus-auth.c(619):sha1_handle_first_client_response] server: Could not get a cookie ID to send to client: No such file or directory
24262: [dbus/dbus-auth.c(430):shutdown_mech] server: Shutting down mechanism DBUS_COOKIE_SHA1

24262: [dbus/dbus-auth.c(2201):process_command] server: got command "AUTH ANONYMOUS 6c69626462757320312e352e3132"
24262: [dbus/dbus-auth.c(1718):handle_auth] server: Trying mechanism ANONYMOUS
24262: [dbus/dbus-auth.c(1638):process_data] server: data: 'libdbus 1.5.12'
24262: [dbus/dbus-auth.c(1224):handle_server_data_anonymous_mech] server: ANONYMOUS client sent trace string: 'libdbus 1.5.12'
24262: [dbus/dbus-auth.c(2148):goto_state] server: going from state WaitingForAuth to state WaitingForBegin
24262: [dbus/dbus-auth.c(1242):handle_server_data_anonymous_mech] server: authenticated client as anonymous


According to comment #54, which is   

.. 
                   me, Unix    other, Unix     me, TCP    other, TCP
normal               me          rejected        me        rejected
... 

should this not be rejected  ?

(In reply to comment #89)
> (In reply to comment #83)
> > Comment on attachment 57713 [details] [review] [review] [review]
> > Add a simple manual test for authentication/authorization
> > 
> > Review of attachment 57713 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > rebased patch, which now compiles and runs on linux - looks good so far
> > except the one check on which test test app print the following message and
> > hangs. 
> > 
> > ** Message: Same-UID-or-anon server:
> > tcp:host=127.0.0.1,port=56081,guid=5f604d1b5a41540ebd4000cf5216ae6f
> 
> Got it: the test app only starts server, print out the server addresses to
> which a dbus client should connect to ?

I tried to connect to the "normal" server on linux with a different user and got

24262: [dbus/dbus-auth.c(2201):process_command] server: got command "AUTH EXTERNAL 31303030"
24262: [dbus/dbus-auth.c(1718):handle_auth] server: Trying mechanism EXTERNAL
24262: [dbus/dbus-auth.c(1638):process_data] server: data: '1000'
24262: [dbus/dbus-userdb.c(166):_dbus_user_database_lookup] No cache for UID 1002
24262: [dbus/dbus-userdb.c(166):_dbus_user_database_lookup] No cache for UID 1000
24262: [dbus/dbus-auth.c(1144):handle_server_data_external_mech] server: desired identity not found in socket credentials
24262: [dbus/dbus-auth.c(430):shutdown_mech] server: Shutting down mechanism EXTERNAL


24262: [dbus/dbus-auth.c(2201):process_command] server: got command "AUTH DBUS_COOKIE_SHA1 31303030"
24262: [dbus/dbus-auth.c(1718):handle_auth] server: Trying mechanism DBUS_COOKIE_SHA1
24262: [dbus/dbus-auth.c(1638):process_data] server: data: '1000'
24262: [dbus/dbus-userdb.c(156):_dbus_user_database_lookup] Using cache for UID 1000 information
24262: [dbus/dbus-userdb.c(156):_dbus_user_database_lookup] Using cache for UID 1000 information
Using your real home directory for testing, set DBUS_TEST_HOMEDIR to avoid
24262: [dbus/dbus-keyring.c(805):_dbus_keyring_new_for_credentials] didn't load an existing keyring: No such file or directory
24262: [dbus/dbus-keyring.c(818):_dbus_keyring_new_for_credentials] Creating keyring directory: Failed to create directory /home/admin/.dbus-keyrings: Permission denied

24262: [dbus/dbus-auth.c(619):sha1_handle_first_client_response] server: Could not get a cookie ID to send to client: No such file or directory
24262: [dbus/dbus-auth.c(430):shutdown_mech] server: Shutting down mechanism DBUS_COOKIE_SHA1

24262: [dbus/dbus-auth.c(2201):process_command] server: got command "AUTH ANONYMOUS 6c69626462757320312e352e3132"
24262: [dbus/dbus-auth.c(1718):handle_auth] server: Trying mechanism ANONYMOUS
24262: [dbus/dbus-auth.c(1638):process_data] server: data: 'libdbus 1.5.12'
24262: [dbus/dbus-auth.c(1224):handle_server_data_anonymous_mech] server: ANONYMOUS client sent trace string: 'libdbus 1.5.12'
24262: [dbus/dbus-auth.c(2148):goto_state] server: going from state WaitingForAuth to state WaitingForBegin
24262: [dbus/dbus-auth.c(1242):handle_server_data_anonymous_mech] server: authenticated client as anonymous


According to comment #54, which is   

.. 
                   me, Unix    other, Unix     me, TCP    other, TCP
normal               me          rejected        me        rejected
... 

should this not be rejected  ?

24262: [dbus/dbus-auth.c(2201):process_command] server: got command "AUTH ANONYMOUS 6c69626462757320312e352e3132"
24262: [dbus/dbus-auth.c(1718):handle_auth] server: Trying mechanism ANONYMOUS

The reason is that anonymous is in the all_mechanisms[] list, which is fetched in handle_auth.c.

(In reply to comment #70)
> Remaining patches does not apply because of failed earlier patches. Need to
> be rebased to recent git master.

Sorry, I should have tried the rebase before pinging reviewers.

Something has gone wrong in the rebase process, so I'm afraid I'm probably going to back out the whole lot and try again later. I now get the same failure mode as you: "Normal server" (and all the others) accept ANONYMOUS connections from another uid.

A good way to test this is:

* run manual-authz (listens on TCP by default)
* as the same and a different user, run "gdbus call -a $addr -d org.freedesktop.DBus -o / -m org.freedesktop.DBus.Peer.Ping" where $addr is one of the many addresses that manual-authz prints
* re-run manual-authz as "manual-authz unix:tmpdir=/tmp" and repeat with the resulting Unix addresses

You don't need verbose mode for that: it's enough to observe that gdbus does or doesn't print error messages.

> 24262: [dbus/dbus-auth.c(1242):handle_server_data_anonymous_mech] server:
> authenticated client as anonymous

That's not directly harmful; what *is* harmful is that, having decided that the peer is the anonymous pseudo-user, we don't fail the authorization step. See Comment #22 for the subtle distinction between authentication and authorization.

I reverted this whole patch series, except for:

(In reply to comment #86)
> Add a simple manual test for authentication/authorization
> Add a test-case for trying to connect with the wrong  GUID
> Fix confusion between "is it authenticated?" and "try to  authenticate"
> free window's callback as well

We should make sure the manual test produces the expected results before landing this stuff. Sorry about that.

(In reply to comment #94)
> > 24262: [dbus/dbus-auth.c(1242):handle_server_data_anonymous_mech] server:
> > authenticated client as anonymous
> 
> That's not directly harmful; what *is* harmful is that, having decided that
> the peer is the anonymous pseudo-user, we don't fail the authorization step.
> See Comment #22 for the subtle distinction between authentication and
> authorization.

You are sure, that this patch set worked in 2013-03 as expected ? I did not see an reports from that time.

(In reply to comment #96)
> You are sure, that this patch set worked in 2013-03 as expected ? I did not
> see an reports from that time.

What happened in 2013-03?

I'm pretty sure these worked in 2012-03 when I last tested them... yes, they've had a significant amount of time to diverge :-(

(In reply to comment #62)
> (In reply to comment #61)
> > Any opinions (from anyone...) on my four other patches here, which follow/fix
> > your branch?
> 
> This is not going to be in 1.6.0, I've given up on waiting for feedback.

Do you have any idea about the reasons, why these feature did not get in last year ? It looks like a useful security feature. 

I can imagine that the stuff may be to complex for contributors, which starts with a little problem and, while working, may be faced with more an more details and requirements, which may exceed peoples budget in time or knowledge,.especially for non daily hackers. 

I personally putted several attempts to extend/fix things on the windows side on hold, like a dbus installer for windows, dbus as windows service and others. It's a pitty that the donated time seems to be lost, which may result into motivation downgrades for further work and not implemented features. :-(

The question is if there are ways to optimize the current situation for example 
- to reduce the learning curve
- to makes dbus hackers donated budgets more effective ? 
- to provide a development roadmap/priorisation, so that people can concentrate on the most important things 
- to get more maintenance and development resources from companies or single peoples  
- to reduce dbus complexity and/or make development more transparent

(In reply to comment #98)
> Do you have any idea about the reasons, why these feature did not get in
> last year ?

When I reviewed Cosimo's patches, I found problems (which Cosimo wasn't able to fix immediately). After a few weeks I fixed those problems myself, but there was nobody to review my changes...

> It looks like a useful security feature. 

It's security-related (which is why it's particularly important to get it right!) but more of a usability fix than a security fix. The current behaviour is secure, but unhelpful: it behaves as if authentication/authorization had succeeded, then immediately disconnects the connection (which, for a client, is indistinguishable from successful authn/authz followed by the dbus-daemon crashing). The idea of these patches is that in the same situation, authentication/authorization should actually report failure.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/53.