Bug 104588

I only skimmed the dbus sources, but if I am not mistaken, it does not matter whether it is a username or uid. dbus-daemon always resolves the user-entry in its own user-database structure. And regardless of the key, the NSS database is always queried, either via `getpwnam()` or via `getpwuid()`.

I might be mistaken here, so I'd appreciate if someone can verify this. If it is true, it might need a more elaborate fix.

I am working on some actual tests for this, but.. yeah.. lets see when I get to this.

(In reply to David Herrmann from comment #1)
> I only skimmed the dbus sources, but if I am not mistaken, it does not
> matter whether it is a username or uid. dbus-daemon always resolves the
> user-entry in its own user-database structure. And regardless of the key,
> the NSS database is always queried, either via `getpwnam()` or via
> `getpwuid()`.

Unfortunately, yes, you are correct. Well, we can short-circuit the all-numeric code path, at least...

(In reply to Simon McVittie from comment #0)
> the EXTERNAL SASL mechanism as used in dbus
> can take three sorts of authorization identity [...]
> 
> * the empty string
> 
> * an all-numeric string
> 
> * a non-all-numeric string

The same is true for DBUS_COOKIE_SHA1, but DBUS_COOKIE_SHA1 has to use the user database anyway, because it relies on the ability to find the home directory.

(Also, it's documented in the spec in terms of usernames, even though what libdbus' client actually sends is the numeric euid and GDBus servers don't support anything except the numeric euid.)

Created attachment 136684 [details] [review]
Clarify which files are Unix-specific

dbus-spawn.c and dbus-userdb* don't have obviously-Unix-specific names,
but are Unix-specific anyway.

Created attachment 136685 [details] [review]
_dbus_credentials_add_from_user: Add a fast-path for  numeric strings

The very common case for this function is that during AUTH EXTERNAL,
it receives a Unix uid encoded as an ASCII decimal integer. There is
no need to look up such uids in the system's user database
(/etc/password or NSS) when the only information we are going to use
from the DBusUserInfo struct is the uid anyway. This avoids taking
the lock and performing a potentially time-consuming NSS lookup.

This changes behaviour in one corner case: if a privileged process has
used one of the set*uid family of functions to set its effective uid
to a numeric uid that does not exist in the system's user database,
we would previously fail. Now, we succeed anyway: it is true to say
in the DBusCredentials that the process has uid 12345, even if uid
12345 does not correspond to any named user.

Created attachment 136686 [details] [review]
_dbus_credentials_add_from_user: Add proper error  reporting

While I'm changing its signature anyway, I might as well fix a
long-standing FIXME.

Created attachment 136687 [details] [review]
_dbus_credentials_add_from_user: Only accept numeric uid  for EXTERNAL

In the well-known system dbus-daemon, it's desirable to avoid looking
up non-numeric authorization identities in the user database, because
that could deadlock with NSS modules that directly or indirectly
require the system bus. Add a flag for whether the username will be
looked up in the userdb, and don't set that flag for EXTERNAL auth
(which is what we use on the system bus, and on the session bus
if not configured otherwise).

DBUS_COOKIE_SHA1 authentication is documented in terms of the
username (although in fact libdbus sends a numeric uid there too,
and GDBus only accepts a numeric uid) so continue to use the userdb
for that mechanism. DBUS_COOKIE_SHA1 needs to use the userdb on Unix
anyway, otherwise it won't find the user's home directory.

Created attachment 136688 [details] [review]
DBusAuthScript: Make USERNAME_HEX differ from USERID_HEX

Previously, USERID_HEX and USERNAME_HEX were both replaced by the hex
encoding of the numeric uid, something like 31303030 for "1000".
Now USERNAME_HEX is something like 736d6376 for "smcv". This is only
supported on Unix, but no authentication mechanisms use usernames on
Windows anyway.

This would require changing the tests that make use of USERNAME_HEX
if we had any, but we currently don't.

Created attachment 136689 [details] [review]
test: Add a test for authenticating with an empty  authorization identity

Created attachment 136690 [details] [review]
test: Add a test-case for EXTERNAL auth rejecting  usernames

Comment on attachment 136684 [details] [review]
Clarify which files are Unix-specific

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

Would it make sense to rename these files to have a `-unix` suffix, and drop the comment (but keep the preprocessor stuff)?

Comment on attachment 136685 [details] [review]
_dbus_credentials_add_from_user: Add a fast-path for  numeric strings

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

r+

Comment on attachment 136686 [details] [review]
_dbus_credentials_add_from_user: Add proper error  reporting

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

r+

(In reply to Philip Withnall from comment #11)
> Would it make sense to rename these files to have a `-unix` suffix, and drop
> the comment (but keep the preprocessor stuff)?

dbus-spawn.c: yes, there's a matching dbus-spawn-win.c.

dbus-userdb{,-util}.c: not so sure here. I think I'd lean towards adding the -unix suffix when the same functionality is implemented differently on Unix and Windows, but not adding the -unix suffix when a particular abstraction simply doesn't exist on one of the platforms.

Comment on attachment 136687 [details] [review]
_dbus_credentials_add_from_user: Only accept numeric uid  for EXTERNAL

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

I haven’t reviewed the reasoning behind only accepting uid for EXTERNAL authentications, so I’m assuming that reasoning is sound for this review to hold.

r+

Comment on attachment 136688 [details] [review]
DBusAuthScript: Make USERNAME_HEX differ from USERID_HEX

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

r+

Comment on attachment 136689 [details] [review]
test: Add a test for authenticating with an empty  authorization identity

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

r+

Looks like cmake/test/CMakeLists.txt uses a wildcard for the data/auth directory, so this commit doesn’t have to modify CMakeLists.txt to ensure the new script is installed.

Comment on attachment 136690 [details] [review]
test: Add a test-case for EXTERNAL auth rejecting  usernames

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

r+

Created attachment 136728 [details] [review]
dbus-spawn.c: Eliminate trailing whitespace

Otherwise the pre-commit hook won't let me rename it.

---

`git show --ignore-space-at-eol HEAD~` confirms that this patch doesn't do anything else.

Created attachment 136729 [details] [review]
dbus-spawn-unix: Rename from dbus-spawn

This file is the Unix counterpart of dbus-spawn-win.c, so it's less
confusing for it to have an indicative name.

(In reply to Simon McVittie from comment #20)
> Created attachment 136729 [details] [review]
> dbus-spawn-unix: Rename from dbus-spawn
> 
> This file is the Unix counterpart of dbus-spawn-win.c, so it's less
> confusing for it to have an indicative name.

You can't see the rename in Splinter, but I promise it's there (view the raw patch to see it).

(In reply to Philip Withnall from comment #11)
> Comment on attachment 136684 [details] [review]
> Clarify which files are Unix-specific

This change (and the resulting file-renaming) is not actually required for the functional change that was the point of this bug, so I'm going to push that (assuming CI passes) and do the clarification/renaming afterwards.

Opinions welcome (from maintainers, David, Tom, whoever) on how much of this should go to the 1.12.x stable-branch. It's arguably a compat break (for very obscure D-Bus clients), but it's also arguably a denial-of-service fix for the system bus (if you have problematically-slow or deadlock-prone NSS plugins).

Comment on attachment 136728 [details] [review]
dbus-spawn.c: Eliminate trailing whitespace

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

Heh, yes. r+

Comment on attachment 136729 [details] [review]
dbus-spawn-unix: Rename from dbus-spawn

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

r+

Created attachment 136755 [details] [review]
_dbus_credentials_add_from_user: Check return of add_unix_uid

Coverity CID 253543.

---

Regression in Attachment #136685 [details].

Possibly one day we should add _DBUS_GNUC_WARN_UNUSED_RESULT to everything that can OOM, but if so, that should be a separate bug.

Comment on attachment 136755 [details] [review]
_dbus_credentials_add_from_user: Check return of add_unix_uid

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

r+. Thanks Coverity. Thoverity.

All merged for dbus 1.13.0, thanks.

(In reply to Simon McVittie from comment #22)
> Opinions welcome (from maintainers, David, Tom, whoever) on how much of this
> should go to the 1.12.x stable-branch. It's arguably a compat break (for
> very obscure D-Bus clients), but it's also arguably a denial-of-service fix
> for the system bus (if you have problematically-slow or deadlock-prone NSS
> plugins).

Please reopen if you feel strongly that the key patches need to be backported to 1.12.x.