As documented on Bug #104224, the EXTERNAL SASL mechanism as used in dbus can take three sorts of authorization identity on Unix (which are hex-encoded in the wire protocol, but for simplicity I'll write them unencoded here): * the empty string: use whatever uid came from credentials-passing * an all-numeric string like "1000" (in practice every client uses this): parse as ASCII decimal and use that uid * a non-all-numeric string like "smcv": look it up in NSS (/etc/passwd and friends) and use the uid of the resulting struct passwd Tom Gundersen pointed out that if an NSS module tries to use D-Bus itself, then it's very easy to get a deadlock. The dbus-daemon would be more robust if we didn't do this. (The Windows implementation doesn't have this issue: it has never accepted human-facing usernames like "Administrator" in this part of the protocol, and always uses SIDs, the equivalent of a Unix uid, which I believe can be converted to their internal representation without any scope for user-defined code running.) This is technically a compatibility break, but in practice the only implementation I found that ever sends non-numeric strings is the rarely-used dbus-java, which only does so if it can't get the numeric uid for some reason (although I should double-check that before releasing anything). So we can probably make the change anyway?
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.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.