So far we have separate methods to get: * the uid on Unix systems (documented) * the pid on Unix systems (documented) * the SID on Windows systems (undocumented in the spec, but it returns a string SID, e.g. "S-1-5-18" for LOCAL_SYSTEM) * the SELinux context (an undocumented method returning a blob of bytes in an undocumented format) * the Solaris (?) audit session data (an undocumented method returning a blob of bytes in an undocumented format) On Bug #47581, Brian wants to add yet another method, this time for Smack, which I believe is Linux-specific. I don't really want to add it at all, and certainly not on the o.fd.DBus interface. The Maemo libcreds developers had a patch to add yet another method, returning their libcreds identifiers (I forget what format that was in), again Linux-specific. Again, I resisted adding it to the o.fd.DBus interface. There are a number of problems with one-method-per-credential: * Platforms that don't have a particular form of credentials end up with methods that are completely useless to them. For instance, Ralf has complained that the SELinux stuff is pretty useless on Windows (Bug #32407). * If you're something like PolicyKit and you want to find out all the credentials of a process, you have to call up to 5 methods (assuming Linux with an unknown LSM), which seems rather silly. D-Bus normally encourages round-trip reduction. * The majority of the credentials-getting methods are undocumented. Perhaps if you're a SELinux expert, there is an obvious, intuitive interpretation of a byte-blob as a SELinux context, but I'm not a SELinux expert. * The majority of the code in the credentials-getting methods is uninteresting boilerplate. I propose to add this method: GetConnectionCredentials (s: Bus_Name) -> a{sv}: Credentials which returns all of the credentials in one go. Each LSM (or whatever) can define its own payload for this method. In-tree functionality should use a short name like "UnixUserID" and document it in the D-Bus Specification. Out-of-tree patches can either document their extensions in the D-Bus Specification, or use a reversed-domain prefix like org.maemo.Libcreds.
Created attachment 66552 [details] [review] 1/3] bus driver: factor out common code to get a named connection
Created attachment 66554 [details] [review] 2/3] Convert a{sv} helpers from Stats into generic utility code
Created attachment 66555 [details] [review] 3/3] GetConnectionCredentials: add The initial set of credentials is just UnixUserID and UnixProcessID. The rest can follow when someone is sufficiently interested to actually test them.
Comment on attachment 66555 [details] [review] 3/3] GetConnectionCredentials: add Review of attachment 66555 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +5637,5 @@ > + <tbody> > + <row> > + <entry>UnixUserID</entry> > + <entry>UINT32</entry> > + <entry>The numeric Unix user ID, as defined by POSIX</entry> Open issue for bearded Unix gurus: should this be an INT64 or UINT64? @@ +5642,5 @@ > + </row> > + <row> > + <entry>UnixProcessID</entry> > + <entry>UINT32</entry> > + <entry>The numeric Unix process ID, as defined by POSIX</entry> Likewise.
Created attachment 66559 [details] [review] a sketch of how to add Solaris ADT, SELinux and Windows credentials We should support and document the other forms of credentials currently supported with undocumented methods, but I run Linux without a LSM, so I can't test any of them. So, here's my untested code. Someone who cares about each of these platforms should finish them off.
(In reply to comment #5) > a sketch of how to add Solaris ADT, SELinux and Windows credentials Ralf or other Windows people: I think the Windows parts of this patch are nearly implemented, if you want to finish them off and get that bit included. In particular, I documented what WindowsSID means. Is the string form of a SID, e.g. "S-1-5-18", the most appropriate representation of a Windows user that we can have?
(In reply to comment #6) > (In reply to comment #5) > > a sketch of how to add Solaris ADT, SELinux and Windows credentials > > Ralf or other Windows people: I think the Windows parts of this patch are > nearly implemented, if you want to finish them off and get that bit included. I will take a look > In particular, I documented what WindowsSID means. > > Is the string form of a SID, e.g. "S-1-5-18", the most appropriate > representation of a Windows user that we can have? no, this is a so called "well known sid" - see http://msdn.microsoft.com/en-us/library/aa379649.aspx the generic format is documented for example at http://en.wikipedia.org/wiki/Security_Identifier#Overview The format of an SID can be illustrated using the following example: "S-1-5-21-3623811015-3361044348-30300820-1013"; S The string is a SID. 1 The revision level (the version of the SID specification). 5 The identifier authority value. 21-3623811015-3361044348-30300820 Domain or local computer identifier 1013 A Relative ID (RID). (identifies user) I would change the doc to the following + <entry>WindowsSID</entry> + <entry>STRING</entry> + <entry>The Windows security identifier in its string form, + e.g. "S-1-5-21-3623811015-3361044348-30300820-1013" for + a domain or local computer user or "S-1-5-18" for the + LOCAL_SYSTEM user</entry>
Comment on attachment 66559 [details] [review] a sketch of how to add Solaris ADT, SELinux and Windows credentials Review of attachment 66559 [details] [review]: ----------------------------------------------------------------- I saw a glib based test case - dbus on windows is currently build without glib so there are only manual tests possible - how to do ? ::: bus/driver.c @@ +1625,5 @@ > +#endif > + > + /* FIXME: someone on a Windows system needs to implement and test this */ > +#if 0 > + windows_sid = FIXME; What should be filled in here ? In case you are refering to the current process user the following need to be filled in: char *windows_sid = NULL; dbus_getsid(&windows_sid); @@ +1629,5 @@ > + windows_sid = FIXME; > + > + if (windows_sid != NULL) > + { > + if (!_dbus_asv_add_string (&array_iter, "WindowsSID", windows_sid)) { LocalFree(windows_sid); @@ +1631,5 @@ > + if (windows_sid != NULL) > + { > + if (!_dbus_asv_add_string (&array_iter, "WindowsSID", windows_sid)) > + goto oom; > + } } LocalFree(windows_sid);
Created attachment 66569 [details] [review] a sketch of how to add Solaris ADT, SELinux and Windows credentials - with windows implementation
Created attachment 66578 [details] [review] a sketch of how to add Solaris ADT, SELinux and Windows credentials - fixed windows implementation The previous patch contains an unrelated CMakelist.txt, which has been cleaned
(In reply to comment #10) > Created attachment 66578 [details] [review] [review] > a sketch of how to add Solaris ADT, SELinux and Windows credentials - fixed > windows implementation > > The previous patch contains an unrelated CMakelist.txt, which has been cleaned Got a running version with this patches on windows (cross compiled on linux with mingw) Running dbus-daemon and dbusviewer with wine and called GetConnectionCredentials "WindowsSID". qdbusviewer returned nothing and dbus-daemon reported 11: [bus/driver.c(2079):bus_driver_handle_message] Driver got a method call: GetConnectionCredentials 11: [bus/driver.c(2102):bus_driver_handle_message] Found driver handler for GetConnectionCredentials 11: [bus/driver.c(62):bus_driver_get_conn_helper] asked for credentials of connection WindowsSID 11: [bus/driver.c(2128):bus_driver_handle_message] Driver handler returned failure 11: [dbus/dbus-connection.c(2934):dbus_connection_get_is_connected] LOCK 11: [dbus/dbus-connection.c(413):_dbus_connection_unlock] UNLOCK I changed the related patch in bus/driver.c to //#ifdef DBUS_WIN { char *windows_sid = NULL; _dbus_getsid (&windows_sid); _dbus_verbose ("get windows sid %s\n", windows_sid); if (windows_sid != NULL) { if (!_dbus_asv_add_string (&array_iter, "WindowsSID", windows_sid)) { LocalFree (windows_sid); goto oom; } LocalFree (windows_sid); } } //#endif to see if this is called anyway but did not found the term "get windows sid" in the dbus-daemon log file created with DBUS_VERBOSE=1
Comment on attachment 66578 [details] [review] a sketch of how to add Solaris ADT, SELinux and Windows credentials - fixed windows implementation Review of attachment 66578 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +1536,5 @@ > DBusMessageIter array_iter; > unsigned long ulong_val; > const char *service; > +#if 0 > + /* only used by unfinished bits - Solaris/SELinux */ I'd prefer it if you could do a patch that adds the Windows parts, and omits the unfinished Solaris/SELinux bits. People who like Solaris or SELinux can get a prototype from my earlier patch and make it work. @@ +1623,5 @@ > + goto oom; > + } > +#endif > + > +#ifdef DBUS_WIN In principle this shouldn't need to be #ifdef DBUS_WIN, because the "get the Windows SID of the thing at the other end" function should just return NULL on non-Windows. @@ +1626,5 @@ > + > +#ifdef DBUS_WIN > + { > + char *windows_sid = NULL; > + _dbus_getsid(&windows_sid); This should get the SID of the process at the other end of the connection, not the SID of the bus daemon. If that's not something you can do on Windows then we should just leave it out, and you don't need to do anything. ::: doc/dbus-specification.xml @@ +5650,5 @@ > + <entry>STRING</entry> > + <entry>The Windows security identifier in its string form, > + e.g. "S-1-5-21-3623811015-3361044348-30300820-1013" for > + domain or local computer user or "S-1-5-18" for the > + LOCAL_SYSTEM user</entry> Fair enough. ::: test/dbus-daemon.c @@ +436,5 @@ > + g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==, > + DBUS_TYPE_STRING); > + dbus_message_iter_get_basic (&var_iter, &sid); > + g_message ("%s of this process is %s", name, sid); > + /* FIXME: assert that it's correct */ The idea was that this should call _dbus_getsid() to see what the SID of this process is, and compare it with what the dbus-daemon says our SID is.
(In reply to comment #7) > > Is the string form of a SID, e.g. "S-1-5-18", the most appropriate > > representation of a Windows user that we can have? > > no, this is a so called "well known sid" - see > http://msdn.microsoft.com/en-us/library/aa379649.aspx Right, what I meant is: is "S-1-5-18" or "S-1-5-21-3623811015-3361044348-30300820-1013" a more appropriate format to represent a Windows user than "LOCAL_SYSTEM" or "Ralf"? And it sounds as though the answer is "yes". (The corresponding thing on Unix is that we use a numeric uid like 0 or 1000 rather than the string name like root or smcv, because it's the number that's actually used when making authentication decisions.)
(In reply to comment #11) > Running dbus-daemon and dbusviewer with wine and called > GetConnectionCredentials "WindowsSID". That should fail with NameHasNoOwner. The right thing would be something like GetConnectionCredentials ":1.42" or GetConnectionCredentials "com.example.MyService". It should return a map from string to variant. With my three patches, that map will be empty on Windows; with your patch in addition, it should contain { "WindowsSID": "S-blahblahblah" }.
Created attachment 66626 [details] [review] windows credential implementation Used available function to retrieves the windows sid of a connection
(In reply to comment #13) > (In reply to comment #7) > > > Is the string form of a SID, e.g. "S-1-5-18", the most appropriate > > > representation of a Windows user that we can have? > > > > no, this is a so called "well known sid" - see > > http://msdn.microsoft.com/en-us/library/aa379649.aspx > > Right, what I meant is: is "S-1-5-18" or > "S-1-5-21-3623811015-3361044348-30300820-1013" a more appropriate format to > represent a Windows user than "LOCAL_SYSTEM" or "Ralf"? And it sounds as though > the answer is "yes". yes, names in the mentioned form are not unique. > (The corresponding thing on Unix is that we use a numeric uid like 0 or 1000 > rather than the string name like root or smcv, because it's the number that's > actually used when making authentication decisions.) yes
(In reply to comment #14) > (In reply to comment #11) > > Running dbus-daemon and dbusviewer with wine and called > > GetConnectionCredentials "WindowsSID". > > That should fail with NameHasNoOwner. > > The right thing would be something like GetConnectionCredentials ":1.42" or > GetConnectionCredentials "com.example.MyService". It should return a map from > string to variant. With my three patches, that map will be empty on Windows; > with your patch in addition, it should contain { "WindowsSID": "S-blahblahblah" > }. Okay, I was misleaded by trying to call the function with org.freedesktop.DBus which returns 13: [bus/connection.c(2269):bus_transaction_send_error_reply] Sending error reply org.freedesktop.DBus.Error.NameHasNoOwner "Could not get credentials of name 'org.freedesktop.DBus': no such name" 13: [bus/connection.c(2014):bus_transaction_send_from_driver] Sending (no interface) (no member) org.freedesktop.DBus.Error.NameHasNoOwner from driver 13: [bus/policy.c(1068):bus_client_policy_check_can_receive] (policy) checking receive rules, eavesdropping = 0
Created attachment 66629 [details] [review] windows credential implementation - free's string
Comment on attachment 66629 [details] [review] windows credential implementation - free's string Review of attachment 66629 [details] [review]: ----------------------------------------------------------------- This can't be merged until someone reviews my first three patches on this bug and says "yes". Any opinion on those patches? ::: bus/driver.c @@ +1569,5 @@ > } > > + if (dbus_connection_get_windows_user (conn, &windows_sid)) > + { > + if (!_dbus_asv_add_string (&array_iter, "WindowsSID", windows_sid)) Thanks, this looks better. It still needs a paragraph in the spec explaining what WindowsSID means - the one in your previous patch looked OK. Is the SID guaranteed to be UTF-8? From the documentation on MSDN it looks as though it's always ASCII, which is a subset of UTF-8, so that's fine. (If it's possible to get, say, Latin-1 characters in the SID, we'd have to send it as a byte array instead of a UTF-8 string.)
I'm going over them today (among other tasks).
Comment on attachment 66552 [details] [review] 1/3] bus driver: factor out common code to get a named connection Review of attachment 66552 [details] [review]: ----------------------------------------------------------------- Ship it
Comment on attachment 66554 [details] [review] 2/3] Convert a{sv} helpers from Stats into generic utility code Review of attachment 66554 [details] [review]: ----------------------------------------------------------------- ::: bus/stats.c @@ +1,3 @@ > /* stats.c - statistics from the bus driver > * > + * Copyright © 2011-2012 Nokia Corporation Why are you adding a Nokia copyright here? And I guess that mojibake is only Bugzilla, right? @@ +59,4 @@ > _dbus_list_get_stats (&in_use, &in_free_list, &allocated); > + > + if (!_dbus_asv_add_uint32 (&arr_iter, "Serial", stats_serial++) || > + !asv_add_uint32 (&arr_iter, "ListMemPoolUsedBytes", in_use) || You've removed asv_add_uint32. Looks like you forgot to add the "_dbus_" prefix to this line and the next two.
Comment on attachment 66555 [details] [review] 3/3] GetConnectionCredentials: add Review of attachment 66555 [details] [review]: ----------------------------------------------------------------- It's fine otherwise. I'd even ship it. ::: doc/dbus-specification.xml @@ +5637,5 @@ > + <tbody> > + <row> > + <entry>UnixUserID</entry> > + <entry>UINT32</entry> > + <entry>The numeric Unix user ID, as defined by POSIX</entry> I don't know any system using PIDs and UIDs larger than 32-bit. We could go for 64-bit just in case, if it's no harm (and I don't think it is). Then again, we could also define that it could be either type and the caller should check. PS: pid_t is technically signed because it's used in some functions like kill(2) and wait(2) where negative numbers have special meaning, usually process groups. But PIDs are always positive.
Comment on attachment 66554 [details] [review] 2/3] Convert a{sv} helpers from Stats into generic utility code Review of attachment 66554 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-asv-util.h @@ +34,5 @@ > + DBusMessageIter *arr_iter); > +void _dbus_asv_abandon (DBusMessageIter *iter, > + DBusMessageIter *arr_iter); > + > +dbus_bool_t _dbus_asv_open_entry (DBusMessageIter *arr_iter, You don't need to export all these functions. Some of them are internal and should be static.
Created attachment 66741 [details] [review] Windows implemention of GetConnectionCredentials (Reviewed)
Comment on attachment 66555 [details] [review] 3/3] GetConnectionCredentials: add Review of attachment 66555 [details] [review]: ----------------------------------------------------------------- > UnixProcessID. does this covers the process id from a dbus connection on windows too - then I suggest to rename UnixProcessID to ProcessID otherwise to add a different WindowsProcessID
(In reply to comment #26) > does this covers the process id from a dbus connection on windows too Not as far as I know. It's a Unix pid_t as returned by getpid() (a temporarily-unique non-negative integer per process). GetCurrentProcessId() on Windows seems to be analogous. The thing that makes it useful on Unix, but probably not on Windows, is that by doing credentials-passing across a Unix socket, the dbus-daemon can find out who is at the other end of the connection (user ID, one group ID, and process ID) in a secure way - it isn't possible to fake your process ID in this message unless you are privileged (root or CAP_SYS_ADMIN). That isn't possible over TCP, so I don't think the process ID would ever be available on Windows anyway? (If it is, we can always add a WindowsProcessID later.) This credentials stuff is mostly of interest for consumption by security software like PolicyKit, on buses like the standard system bus that act as a security boundary - so to be honest it's not particularly relevant on Windows, where D-Bus isn't a standard OS component and there's no system bus.
(In reply to comment #27) > Not as far as I know. It's a Unix pid_t as returned by getpid() (a > temporarily-unique non-negative integer per process). GetCurrentProcessId() on > Windows seems to be analogous. Then we should rename and simply call it ProcessID. > The thing that makes it useful on Unix, but probably not on Windows, is that by > doing credentials-passing across a Unix socket, the dbus-daemon can find out > who is at the other end of the connection (user ID, one group ID, and process > ID) in a secure way - it isn't possible to fake your process ID in this message > unless you are privileged (root or CAP_SYS_ADMIN). > > That isn't possible over TCP, so I don't think the process ID would ever be > available on Windows anyway? (If it is, we can always add a WindowsProcessID > later.) I don't see the problem of just doing it now. Whether the D-Bus server can get the remote connection's PID or not is completely irrelevant. If it manages to get it, it's a 32-bit unsigned integer anyway.
(I'll try to get back to this sometime.)
For future reference if someone other than me wants to finish my patches from this bug: (In reply to comment #22) > Why are you adding a Nokia copyright here? I was maintaining D-Bus for Maemo/MeeGo Harmattan at the time I wrote it. > And I guess that mojibake is only Bugzilla, right? Yes, the actual patch is valid UTF-8. (In reply to comment #28) > Then we should rename and simply call it ProcessID. That seems reasonable. > I don't see the problem of just doing it now. Whether the D-Bus server can > get the remote connection's PID or not is completely irrelevant. If it > manages to get it, it's a 32-bit unsigned integer anyway. Fair enough.
Comment on attachment 66552 [details] [review] 1/3] bus driver: factor out common code to get a named connection pushed, thanks
Created attachment 75650 [details] [review] [1] Convert a{sv} helpers from Stats into generic utility code --- I believe this addresses all of Thiago's comments. _dbus_asv_open_entry, _dbus_asv_close_entry and _dbus_asv_abandon_entry are now static (but are still named the same, to reduce churn if/when we add credentials that have a more complex type).
Created attachment 75651 [details] [review] [2] GetConnectionCredentials: add The initial set of credentials is just UnixUserID and ProcessID. The rest can follow when someone is sufficiently interested to actually test them. --- ProcessID is still 32-bit unsigned, but now has non-Unix-specific naming in case it ever becomes implementable on Windows.
Created attachment 75652 [details] [review] Windows implementation of GetConnectionCredentials. From: Ralf Habacker <ralf.habacker@freenet.de> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> --- Rebased.
Created attachment 75653 [details] [review] [3] Document GetAdtAuditSessionData and GetConnectionSELinuxSecurityContext These are only part of the DBus interface because dbus-daemon didn't previously support multiple interfaces. I don't know enough about either of these security frameworks to know what they return, but perhaps one day someone who knows about Solaris or SELinux will tell us...
Created attachment 75654 [details] [review] [WiP] sketch of how to implement Solaris ADT, SELinux context Deliberately not a git-format-patch: I have not compiled this, let alone tested it. If anyone wants to implement either of these, this is a starting point.
(In reply to comment #19) > Is the SID guaranteed to be UTF-8? From the documentation on MSDN it looks > as though it's always ASCII, which is a subset of UTF-8, so that's fine. Ralf, could you confirm this please? If you're not sure, I suppose we could always throw in a call to dbus_validate_utf8(), and just not include the SID in the reply (meaning "I don't know, if in doubt don't trust this process") if it turns out to be non-UTF-8, the same way I didn't include the Unix uid/pid if they're beyond 32-bit. That'd probably be a good idea for other frameworks' identifiers too, actually.
(In reply to comment #37) > (In reply to comment #19) > > Is the SID guaranteed to be UTF-8? From the documentation on MSDN it looks > > as though it's always ASCII, which is a subset of UTF-8, so that's fine. > > Ralf, could you confirm this please? > http://msdn.microsoft.com/en-us/library/windows/desktop/aa379602%28v=vs.85%29.aspx states two forms: 1. standard string representation, which is definitive ASCII 2. string SID's listed on the mentioned page are also ASCII > If you're not sure, I suppose we could always throw in a call to > dbus_validate_utf8(), and just not include the SID in the reply (meaning "I > don't know, if in doubt don't trust this process") if it turns out to be > non-UTF-8, the same way I didn't include the Unix uid/pid if they're beyond > 32-bit. yes, seems to be the best way. > > That'd probably be a good idea for other frameworks' identifiers too, > actually. yes
Created attachment 75689 [details] [review] Windows implementation of GetConnectionCredentials (with utf8 validation check)
Comment on attachment 75689 [details] [review] Windows implementation of GetConnectionCredentials (with utf8 validation check) Review of attachment 75689 [details] [review]: ----------------------------------------------------------------- review+ (conditional on my patches, obviously).
(In reply to comment #40) > Comment on attachment 75689 [details] [review] [review] > Windows implementation of GetConnectionCredentials (with utf8 validation > check) > > Review of attachment 75689 [details] [review] [review]: > ----------------------------------------------------------------- > > review+ Just for the record, here is a test case to check the results. 1. start daemon wine bin/dbus-daemon.exe --session --print-address & ----- output ----- <connect-string-output> 2. start test client DBUS_SESSION_BUS_ADDRESS=<connect-string-output> wine bin/dbus-monitor.exe > out.log & 3. get connection name DBUS_SESSION_BUS_ADDRESS=<connect-string-output> wine bin/dbus-send.exe \ --print-reply --reply-timeout=120000 --type=method_call \ --dest='org.freedesktop.DBus' '/org/freedesktop/DBus' org.freedesktop.DBus.ListNames ----- output ----- method return sender=org.freedesktop.DBus -> dest=:1.35 reply_serial=2 array [ string "org.freedesktop.DBus" string ":1.35" string ":1.24" ] 3. call method DBUS_SESSION_BUS_ADDRESS=<connect-string-output> wine bin/dbus-send.exe \ --print-reply --reply-timeout=120000 --type=method_call \ --dest='org.freedesktop.DBus' '/org/freedesktop/DBus' \ org.freedesktop.DBus.GetConnectionCredentials string::1.24 ----- output ----- method return sender=org.freedesktop.DBus -> dest=:1.36 reply_serial=2 array [ dict entry( string "ProcessID" variant uint32 8 ) dict entry( string "WindowsSID" variant string "S-1-5-21-0-0-0-1000" ) ]
(In reply to comment #40) > > (conditional on my patches, obviously). hope Thiago will find the time to review :-)
(In reply to comment #41) > dict entry( > string "ProcessID" > variant uint32 8 > ) Where did this pid come from?! _dbus_read_credentials_socket() appears to be mis-implemented: it claims that every peer - even if it authenticated using the ANONYMOUS mechanism - has the pid and SID of the *current* process (the dbus-daemon, in this case). This is wrong. I'll clone a bug.
(In reply to comment #41) > 2. start test client > DBUS_SESSION_BUS_ADDRESS=<connect-string-output> wine > bin/dbus-monitor.exe > out.log & I assume the idea here is that this dbus-monitor... > org.freedesktop.DBus.GetConnectionCredentials string::1.24 ... is the owner of :1.24. > dict entry( > string "ProcessID" > variant uint32 8 > ) GetConnectionCredentials is claiming that this is the pid of dbus-monitor. I don't think it actually is: I think it's the pid of dbus-daemon. > dict entry( > string "WindowsSID" > variant string "S-1-5-21-0-0-0-1000" > ) This is your SID (user ID), which is presumably true for both the dbus-daemon and the dbus-monitor, unless someone else has joined your session bus.
(In reply to comment #43) > This is wrong. I'll clone a bug. Bug #61787
(In reply to comment #43) > (In reply to comment #41) > > dict entry( > > string "ProcessID" > > variant uint32 8 > > ) > > Where did this pid come from?! Remember dbus-daemon has been running with wine. This is the windows pid for dbus-daemon.exe > > _dbus_read_credentials_socket() appears to be mis-implemented: it claims > that every peer - even if it authenticated using the ANONYMOUS mechanism - > has the pid and SID of the *current* process (the dbus-daemon, in this case). >
(In reply to comment #46) > Remember dbus-daemon has been running with wine. This is the windows pid for > dbus-daemon.exe Right. My complaint wasn't that this is a Wine-emulated Windows pid rather than a Linux pid - that's fine, and expected - but that providing the dbus-daemon pid here is not useful information, and not what GetConnectionCredentials is documented to do: GetConnectionCredentials(X) should return the credentials of the connected process X, not those of the dbus-daemon itself. If that means that it can't return ProcessID on Windows, then it shouldn't return ProcessID on Windows - giving information that can't be trusted to be correct is worse than giving no information. This wrong process ID is Bug #61787, which (IMO) shouldn't block this one: I believe GetConnectionUnixProcessID is equally wrong on Windows, for the same reason. If you can't tell the peer's process ID, which I don't think you can, then GCUPI should just raise an error.
Created attachment 76017 [details] [review] Retrieve pid and sid from tcp connection peer
Created attachment 76018 [details] [review] Windows implementation of GetConnectionCredentials (fixed)
Comment on attachment 76017 [details] [review] Retrieve pid and sid from tcp connection peer Review of attachment 76017 [details] [review]: ----------------------------------------------------------------- This is actually a fix for Bug #61787 I think? Some style points and a couple of simplifications, but I'm glad this is something we can do on Windows after all. ::: cmake/dbus/CMakeLists.txt @@ +266,4 @@ > if(WINCE) > target_link_libraries(dbus-1 ws2) > else(WINCE) > + target_link_libraries(dbus-1 ws2_32 advapi32 netapi32 iphlpapi) If iphlpapi is necessary, please add -liphlpapi to NETWORK_libs in configure.ac too. (Search for ws2_32 to find the right place - there's only one instance.) ::: dbus/dbus-sysdeps-win.c @@ +113,5 @@ > + */ > +int get_peer_pid_from_tcp_handle(int handle) > +{ > + struct sockaddr_storage addr; > + socklen_t len = sizeof addr; Coding style: we usually say sizeof (thing) with the parentheses. @@ +119,5 @@ > + char peer_ipstr[INET6_ADDRSTRLEN]; > + > + DWORD result; > + DWORD size; > + MIB_TCPTABLE2 *tcpTable; Coding style: I'd prefer tcp_table. @@ +122,5 @@ > + DWORD size; > + MIB_TCPTABLE2 *tcpTable; > + int i; > + > + getpeername(handle, (struct sockaddr*)&addr, &len); coding style: getpeername (handle, (struct sockaddr *) &addr...); (and many more instances of the same things) @@ +125,5 @@ > + > + getpeername(handle, (struct sockaddr*)&addr, &len); > + > + /* deal with both IPv4 and IPv6: */ > + if (addr.ss_family == AF_INET) { indentation in D-Bus style, please @@ +128,5 @@ > + /* deal with both IPv4 and IPv6: */ > + if (addr.ss_family == AF_INET) { > + struct sockaddr_in *s = (struct sockaddr_in *)&addr; > + peer_port = ntohs(s->sin_port); > + inet_ntop(AF_INET, &s->sin_addr, peer_ipstr, sizeof(peer_ipstr)); No need to use inet_ntop(), just compare s->sin_addr.s_addr with INADDR_LOOPBACK (both are in network byte order, iirc). @@ +132,5 @@ > + inet_ntop(AF_INET, &s->sin_addr, peer_ipstr, sizeof(peer_ipstr)); > + } else { /* AF_INET6 */ > + struct sockaddr_in6 *s = (struct sockaddr_in6 *)&addr; > + peer_port = ntohs(s->sin6_port); > + inet_ntop(AF_INET6, &s->sin6_addr, peer_ipstr, sizeof(peer_ipstr)); No need to use inet_ntop(), just memcmp it with in6addr_loopback. (Assuming Windows has that...) @@ +136,5 @@ > + inet_ntop(AF_INET6, &s->sin6_addr, peer_ipstr, sizeof(peer_ipstr)); > + } > + > + /* check for localhost */ > + if (strcmp(peer_ipstr,"127.0.0.1") != 0) This would be wrong for IPv6, but as noted, you don't need to use string comparisons anyway. @@ +820,4 @@ > * @returns process sid > */ > static dbus_bool_t > +_dbus_getsid(char **sid, int process_id) space before ( Are Windows process IDs really "int", or should they be DWORD or something? @@ +826,5 @@ > TOKEN_USER *token_user = NULL; > DWORD n; > PSID psid; > int retval = FALSE; > + HANDLE process_handle = OpenProcess(PROCESS_ALL_ACCESS, FALSE, process_id); Is this going to fail if we don't own the other process? Can we "open" it in a less enthusiastic mode, i.e. "just look at it"? @@ +1761,5 @@ > int bytes_read = 0; > DBusString buf; > + char *sid = NULL; > + int pid; > + int retval = TRUE; I'd make this start FALSE, and set it to TRUE just before the "out" label. @@ +1782,2 @@ > > + _dbus_credentials_add_unix_pid(credentials, pid); This should probably be renamed to ...add_pid if it's equally valid for Windows (but that can be a separate change). @@ +1791,5 @@ > + goto end; > + > +failed: > + retval = FALSE; > +end: The conventional label name for the end of a function in libdbus is "out" (I prefer "finally" personally, but libdbus already has a convention). This can be simplified to this form, which I often use elsewhere - considerably less goto'ing around. dbus_bool_t retval = FALSE; ... if (bad thing) goto out; ... if (other bad thing) goto out; ... retval = TRUE; out: if (sid != NULL) LocalFree (sid); return retval; }
Comment on attachment 76017 [details] [review] Retrieve pid and sid from tcp connection peer followups are assigned to bug 61787
(In reply to comment #51) > Comment on attachment 76017 [details] [review] [review] > Retrieve pid and sid from tcp connection peer > > followups are assigned to bug 61787 After this has been fixed, is there anything left to do for me ?
(In reply to comment #52) > After this has been fixed, is there anything left to do for me ? You could review my three patches here? :-)
Comment on attachment 76018 [details] [review] Windows implementation of GetConnectionCredentials (fixed) Review of attachment 76018 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +1572,5 @@ > } > > + if (dbus_connection_get_windows_user (conn, &windows_sid)) > + { > + if (_dbus_check_is_valid_utf8 (windows_sid)) Now that I look at this again: this is not actually correct. According to dbus-marshal-validate.h, _dbus_check_foo() is only meant to be used in _dbus_return_[val_]if_fail(), and in an attempt to enforce that, it isn't defined if checks are disabled. (Virtually nobody actually sees that failure mode, because compiling libdbus without checks doesn't usually make sense.) You could use dbus_validate_utf8 (windows_sid, NULL) from dbus-syntax.h, now that I've added it.
Comment on attachment 75650 [details] [review] [1] Convert a{sv} helpers from Stats into generic utility code Review of attachment 75650 [details] [review]: ----------------------------------------------------------------- patch apply fails with recent dbus master source apply: Convert a{sv} helpers from Stats into generic utility code error: patch failed: bus/stats.c:193 error: bus/stats.c: patch does not apply Patch failed at 0001 Convert a{sv} helpers from Stats into generic utility code
(In reply to comment #55) > patch apply fails with recent dbus master source Yeah, it conflicts. I'll update it.
Created attachment 81340 [details] [review] [1] Convert a{sv} helpers from Stats into generic utility code
Created attachment 81341 [details] [review] [2] GetConnectionCredentials: add
Created attachment 81342 [details] [review] [3] Document GetAdtAuditSessionData and GetConnectionSELinuxSecurityContext
Comment on attachment 81340 [details] [review] [1] Convert a{sv} helpers from Stats into generic utility code Review of attachment 81340 [details] [review]: ----------------------------------------------------------------- looks good
(In reply to comment #54) > Comment on attachment 76018 [details] [review] [review] > Windows implementation of GetConnectionCredentials (fixed) > > Review of attachment 76018 [details] [review] [review]: > ----------------------------------------------------------------- > You could use dbus_validate_utf8 (windows_sid, NULL) from dbus-syntax.h, now > that I've added it. There is the problem that dbus_validate_utf8 needs a DBusString and windows_sid is a char pointer
(In reply to comment #61) > There is the problem that dbus_validate_utf8 needs a DBusString and > windows_sid is a char pointer You can either use dbus_validate_utf8() (which takes a const char *), or _dbus_string_init_const() and _dbus_string_validate_utf8() (which takes a DBusString). _dbus_validate_utf8() is an alias for _dbus_string_validate_utf8(), but dbus_validate_utf8() (without the underscore) is a wrapper with a different signature. I believe the justification for _dbus_check_is_valid_utf8() being unavailable when compiling without checks was that when parsing a binary D-Bus message, _dbus_string_validate_utf8() is the right thing, and _dbus_check_is_valid_utf8() would be wrong (because binary D-Bus messages aren't '\0'-terminated and it is necessary to check for out-of-bounds reading). However, I added dbus_validate_utf8() (and similar functions) because it seemed stupid for bindings like dbus-python to have to reimplement them. If you want to turn _dbus_check_is_valid_utf8 (x) into a macro wrapper around dbus_validate_utf8 (x, NULL), and so on, I wouldn't object.
Created attachment 81389 [details] [review] windows implementation of GetConnectionCredential (update)
Comment on attachment 81389 [details] [review] windows implementation of GetConnectionCredential (update) Review of attachment 81389 [details] [review]: ----------------------------------------------------------------- Looks good, if someone says "yes" to my patches on this bug. ::: bus/driver.c @@ +1576,5 @@ > + DBusString str; > + dbus_bool_t result; > + _dbus_string_init_const (&str, windows_sid); > + result = _dbus_validate_utf8 (&str, 0, _dbus_string_get_length (&str)); > + _dbus_string_free (&str); You don't actually need this _dbus_string_free() call, but it's OK either way.
(In reply to comment #60) > [1] Convert a{sv} helpers from Stats into generic utility code > looks good Applied for 1.7.6, thanks
Comment on attachment 81341 [details] [review] [2] GetConnectionCredentials: add Review of attachment 81341 [details] [review]: ----------------------------------------------------------------- I found one function not very descriptive, which makes code reading harder. Patch otherwise looks good. I would not say that the issue is a blocker. ::: test/dbus-daemon.c @@ +311,4 @@ > } > > static void > +pc_store (DBusPendingCall *pc, the name of the function is not very descriptive @@ +428,5 @@ > +#endif > + > +#ifdef G_OS_WIN32 > + /* FIXME: when implemented: > + g_assert (seen & SEEN_WINDOWS_SID); the related windows patch need to address this
Comment on attachment 81342 [details] [review] [3] Document GetAdtAuditSessionData and GetConnectionSELinuxSecurityContext Review of attachment 81342 [details] [review]: ----------------------------------------------------------------- formally this patch looks good.
Created attachment 82573 [details] [review] [PATCH] Fix build with "--enable-stats" Fix build failure introduced by these changes.
Comment on attachment 82573 [details] [review] [PATCH] Fix build with "--enable-stats" Review of attachment 82573 [details] [review]: ----------------------------------------------------------------- ::: bus/stats.c @@ +166,3 @@ > !_dbus_asv_add_uint32 (&arr_iter, "PeakBusNames", > bus_connection_get_peak_bus_names (stats_connection)) || > !_dbus_asv_add_uint32 (&arr_iter, "UniqueName", The bug here is that I should have been adding a string, not a uint32. Adding the char * (pointer to the first byte in the string) to the packet as an integer makes no sense. @@ +167,5 @@ > bus_connection_get_peak_bus_names (stats_connection)) || > !_dbus_asv_add_uint32 (&arr_iter, "UniqueName", > + /* > + * need a dbus_uint32_t but get a const char *, which sizeof > + * equals sizeof (unsigned long). As a general note, this is not true in general: in particular, it's false on 64-bit Windows and on Linux "x32", which are LLP64 platforms. (I don't personally think the performance gains for x32 are worth the latent bugs they will uncover, but some people want to use it anyway.) @@ +173,5 @@ > + * sizeof (unsigned long) != sizeof (unsigned int), so we'll > + * get error=pointer-to-int-cast > + * > + * However, we'll dead if we get an address larger than > + * UINT_MAX, Will it happen? FYI: yes it can.
Created attachment 82643 [details] [review] [PATCH v2] Fix build with "--enable-stats" use _dbus_asv_add_string()
Comment on attachment 82643 [details] [review] [PATCH v2] Fix build with "--enable-stats" Thanks, applied.
(In reply to comment #64) > windows implementation of GetConnectionCredential (update) I think you accidentally included this in commit 600621dbc80. I reverted that commit, though, because the manual test indicated that it was incorrectly allowing ANONYMOUS too often. I'd prefer to have the test enabled before merging this, if possible. (In reply to comment #66) > @@ +428,5 @@ > > +#endif > > + > > +#ifdef G_OS_WIN32 > > + /* FIXME: when implemented: > > + g_assert (seen & SEEN_WINDOWS_SID); > > the related windows patch need to address this Please do - I don't have a development environment on Windows, so I can't really test it properly. The other thing that this test ought to check is that the SID we report is correct (basically, copy the logic for UnixUserID).
(In reply to comment #72) > (In reply to comment #64) > > windows implementation of GetConnectionCredential (update) > > I think you accidentally included this in commit 600621dbc80. Happened while manual rebasing/patching the patches from #39720, seems to be caused by an unclear git environment in which an applied patch from this bug stays around - sorry for that. > (In reply to comment #66) > > @@ +428,5 @@ > > > +#endif > > > + > > > +#ifdef G_OS_WIN32 > > > + /* FIXME: when implemented: > > > + g_assert (seen & SEEN_WINDOWS_SID); > > > > the related windows patch need to address this > > Please do - I don't have a development environment on Windows, so I can't > really test it properly. The problem here is, that dbus-glib, which is required, isn't available as a package on windows native nor as mingw32 cross compiled package yet. > The other thing that this test ought to check is that the SID we report is correct (basically, copy the logic for UnixUserID). You are refering to the compare to _DBUS_UINT32_MAX in + if (dbus_connection_get_unix_user (conn, &ulong_val) && + ulong_val <= _DBUS_UINT32_MAX) the validation of the sid already happens in _dbus_getsid() from which all connection related sid's are fetch ... psid = token_user->User.Sid; if (!IsValidSid (psid)) { _dbus_verbose("%s invalid sid\n",__FUNCTION__); goto failed; } .
(In reply to comment #73) > The problem here is, that dbus-glib, which is required, isn't available as a > package on windows native nor as mingw32 cross compiled package yet. It's reasonably easy to cross-compile, assuming you have a cross-compiled GLib to compile it against - that's what I do to smoke-test dbus/mingw. > > The other thing that this test ought to check is that the SID we report is correct (basically, copy the logic for UnixUserID). > > You are refering to the compare to _DBUS_UINT32_MAX in > + if (dbus_connection_get_unix_user (conn, &ulong_val) && > + ulong_val <= _DBUS_UINT32_MAX) No, I'm referring to the actual regression test in test/dbus-daemon.c: if (g_strcmp0 (name, "UnixUserID") == 0) { #ifdef G_OS_UNIX guint32 u32; g_assert (!(seen & SEEN_UNIX_USER)); g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==, DBUS_TYPE_UINT32); dbus_message_iter_get_basic (&var_iter, &u32); g_message ("%s of this process is %u", name, u32); g_assert_cmpuint (u32, ==, geteuid ()); seen |= SEEN_UNIX_USER; #else g_assert_not_reached (); #endif } There should be a similar block for WindowsSID which on Windows, checks that the WindowsSID is a string, gets the SID of the current process via Win32 API for comparison, checks that they are equal, and records SEEN_WINDOWS_SID; and on Unix, crashes with g_assert_not_reached().
(In reply to comment #74) > (In reply to comment #73) > > The problem here is, that dbus-glib, which is required, isn't available as a > > package on windows native nor as mingw32 cross compiled package yet. > got it It has not been named dbus-glib instead it is named mingw32-dbus-1-glib BTW: http://www.freedesktop.org/wiki/Software/DBusBindings/ states that dbus-glib is obsolate and gdbus should used. Does this apply to dbus too ? > > > The other thing that this test ought to check is that the SID we report is correct (basically, copy the logic for UnixUserID). > > > > You are refering to the compare to _DBUS_UINT32_MAX in > > + if (dbus_connection_get_unix_user (conn, &ulong_val) && > > + ulong_val <= _DBUS_UINT32_MAX) > > No, I'm referring to the actual regression test in test/dbus-daemon.c: > > if (g_strcmp0 (name, "UnixUserID") == 0) > { > #ifdef G_OS_UNIX > guint32 u32; > > g_assert (!(seen & SEEN_UNIX_USER)); > g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==, > DBUS_TYPE_UINT32); > dbus_message_iter_get_basic (&var_iter, &u32); > g_message ("%s of this process is %u", name, u32); > g_assert_cmpuint (u32, ==, geteuid ()); > seen |= SEEN_UNIX_USER; > #else > g_assert_not_reached (); > #endif > } > > There should be a similar block for WindowsSID which on Windows, checks that > the WindowsSID is a string, gets the SID of the current process via Win32 > API for comparison, checks that they are equal, and records > SEEN_WINDOWS_SID; and on Unix, crashes with g_assert_not_reached(). okay, that seem doable.
Created attachment 84562 [details] [review] Add testcase for windows sid to test dbus-daemon could not test yet because of blocker #68496
Comment on attachment 84562 [details] [review] Add testcase for windows sid to test dbus-daemon Review of attachment 84562 [details] [review]: ----------------------------------------------------------------- ::: test/dbus-daemon.c @@ +81,5 @@ > gchar *argv[] = { > binary, > configuration, > +#ifdef DBUS_WIN > + "--print-address", /* stdout */ I'd prefer this: "--print-address=1", /* to stdout */ #ifdef DBUS_UNIX "--nofork", #endif NULL }; or if stdout isn't necessarily fd 1 in Windows, drop the "=1" everywhere to minimize the difference (bare --print-address is equivalent to --print-address=1 under Unix, so that's OK - I only included the "=1" to be more explicit). @@ +409,5 @@ > #endif > } > + else if (g_strcmp0 (name, "WindowsSID") == 0) > + { > +#ifdef G_OS_WIN32 This is more or less what I had in mind, thanks. @@ +421,5 @@ > + dbus_message_iter_get_basic (&var_iter, &sid); > + g_message ("%s of this process is %s", name, sid); > + if (_dbus_getsid (&self_sid, 0)) > + result = strcmp (self_sid, sid); > + g_assert_cmpuint (result, ==, 0); I'd prefer: if (_dbus_getsid (&self_sid, 0)) g_error ("_dbus_getsid() failed"); g_assert_cmpstr (self_sid, ==, sid); since that prints better diagnostics if they differ, or if one of them is NULL. (g_error() is always fatal, and g_assert_cmpstr() is a macro wrapper around g_strcmp0().)
(In reply to comment #77) > Comment on attachment 84562 [details] [review] [review] > Add testcase for windows sid to test dbus-daemon > > Review of attachment 84562 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: test/dbus-daemon.c > @@ +81,5 @@ > > gchar *argv[] = { > > binary, > > configuration, > > +#ifdef DBUS_WIN > > + "--print-address", /* stdout */ > > I'd prefer this: > > "--print-address=1", /* to stdout */ > #ifdef DBUS_UNIX > "--nofork", > #endif > NULL > }; > > or if stdout isn't necessarily fd 1 in Windows, drop the "=1" everywhere to > minimize the difference (bare --print-address is equivalent to > --print-address=1 under Unix, so that's OK - I only included the "=1" to be > more explicit). I remember that with =1 the daemon could not be started, need to recheck. > I'd prefer: > > if (_dbus_getsid (&self_sid, 0)) You probably meant if (!_dbus_getsid (&self_sid, 0)) > g_error ("_dbus_getsid() failed"); > > g_assert_cmpstr (self_sid, ==, sid); > > since that prints better diagnostics if they differ, or if one of them is > NULL. (g_error() is always fatal, and g_assert_cmpstr() is a macro wrapper > around g_strcmp0().)
Created attachment 84801 [details] [review] Test for windows sid (update)
Created attachment 84802 [details] [review] Assert when not seen WindowsSID on windows test complete
Created attachment 84804 [details] [review] Export _dbus_getsid for test applications required for patch #8481
Comment on attachment 84804 [details] [review] Export _dbus_getsid for test applications Review of attachment 84804 [details] [review]: ----------------------------------------------------------------- Hmm. This is necessary for the test to be able to use it, but not sufficient: if a module is using "_dbus*" symbols, it needs the internal library (at least in Autotools-land, where we have symbol visibility). Please commit this, but not the other patches from this bug right now. I'll put together a patch for the Autotools build system.
Comment on attachment 81341 [details] [review] [2] GetConnectionCredentials: add applied
Comment on attachment 81342 [details] [review] [3] Document GetAdtAuditSessionData and GetConnectionSELinuxSecurityContext applied
Comment on attachment 84802 [details] [review] Assert when not seen WindowsSID on windows This seems to be included in Attachment #84801 [details], so it's probably obsolete? (I would have asked you to squash them into one commit anyway, tbh.)
(In reply to comment #82) > Export _dbus_getsid for test applications This is included in Attachment #84801 [details] too?
Comment on attachment 84801 [details] [review] Test for windows sid (update) Review of attachment 84801 [details] [review]: ----------------------------------------------------------------- ::: test/dbus-daemon.c @@ +418,5 @@ > + g_assert (!(seen & SEEN_WINDOWS_SID)); > + g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==, > + DBUS_TYPE_STRING); > + dbus_message_iter_get_basic (&var_iter, &sid); > + g_message ("%s of this process is %s", name, sid); here probably setting result to an initial value is missing.
This no longer blocks Bug #47581: I've merged enough for that.
(In reply to comment #85) > Comment on attachment 84802 [details] [review] [review] > Assert when not seen WindowsSID on windows > > This seems to be included in Attachment #84801 [details], so it's probably > obsolete? > > (I would have asked you to squash them into one commit anyway, tbh.) yes, i made this patch obsolate
(In reply to comment #86) > (In reply to comment #82) > > Export _dbus_getsid for test applications > > This is included in Attachment #84801 [details] too? seems so, made it obsolate
(In reply to comment #82) > Please commit this, but not the other patches from this bug right now. I'll > put together a patch for the Autotools build system. Let's defer all of this until we have a test (without the credentials stuff) building and passing. I saw a patch from you somewhere which removed the --nofork on Windows, which AIUI is a prerequisite for any of these tests working on Windows - remind me, which bug is that on?
(In reply to comment #91) > (In reply to comment #82) > > Please commit this, but not the other patches from this bug right now. I'll > > put together a patch for the Autotools build system. > > Let's defer all of this until we have a test (without the credentials stuff) > building and passing. I saw a patch from you somewhere which removed the > --nofork on Windows, which AIUI is a prerequisite for any of these tests > working on Windows - remind me, which bug is that on? you are probably refering to https://bugs.freedesktop.org/attachment.cgi?id=84801 @@ -78,8 +81,12 @@ spawn_dbus_daemon (gchar *binary, gchar *argv[] = { binary, configuration, +#ifdef DBUS_WIN + "--print-address", /* stdout */ +#else "--nofork", "--print-address=1", /* stdout */ +#endif NULL
(In reply to comment #91) > I saw a patch from you somewhere which removed the > --nofork on Windows, which AIUI is a prerequisite for any of these tests > working on Windows Bug #68852 might make this unnecessary. I hope so. (In reply to comment #82) > if a module is using "_dbus*" symbols, it needs the internal > library (at least in Autotools-land, where we have symbol visibility). Bug #68852 should make it easier for the dbus-daemon test to be compiled against (dbus-glib and) libdbus-1.la on Unix, but libdbus-internal.la (so it can see _dbus_getsid()) on Windows. I'd like to confirm that that test passes on Windows *without* the Bug #54445 stuff before I do that, though :-)
With the patches from Bug #68852 merged, it should be possible to do something like this (untested): if DBUS_WIN test_dbus_daemon_CPPFLAGS = $(static_cppflags) test_dbus_daemon_LDADD = libdbus-testutils-internal.la else !DBUS_WIN test_dbus_daemon_CPPFLAGS = test_dbus_daemon_LDADD = \ $(testutils_shared_if_possible) \ $(GLIB_LIBS) \ $(NULL) endif !DBUS_WIN to give test-dbus-daemon access to _dbus_getsid() on Windows, but link it dynamically (when possible) on Unix.
For those who like Smack, SELinux or other environments with more credentials than just the POSIX ones: the OS-independent and POSIX parts of this are in git master, so now is the time to implement credentials-getting mechanisms for your favourite LSM or other credentials framework (please track as a separate bug, and use Attachment #75654 [details] as inspiration). What's left on this bug is getting feature parity with the old credentials mechanisms on Windows, which is blocked by getting the regression test to work; that doesn't block Smack, SELinux, Solaris ADT etc.
This is not Linux-specific, and is a feature enhancement, not a security vulnerability; removing security tag again. If GetConnectionCredentials() can return incorrect information, *that* would be a security vulnerability. (If so, please email me privately.)
(In reply to comment #95) > For those who like Smack, SELinux or other environments with more > credentials than just the POSIX ones: the OS-independent and POSIX parts of > this are in git master, so now is the time to implement credentials-getting > mechanisms for your favourite LSM or other credentials framework (please > track as a separate bug, and use Attachment #75654 [details] as inspiration). Note that this WIP code has run-time errors. It attempts to append multiple bytes direcly to a variant container. What needs to happen is for the variant container to be opened, then an array container to be opened, and then the byte array to be appended. The middle step is missing from the WIP. I've attached a patch to Bug #75113 that creates a new a{sv} helper called _dbus_asv_add_byte_array(). It may be of interest to anyone wanting to finish out this WIP code.
Unassigning this, I'm not working on it. I think the missing thing here is making the Windows test work, or possibly verifying that it already works.
Created attachment 113318 [details] [review] Test for windows sid (rebased)
Created attachment 113319 [details] [review] Minor optimization in _dbus_getsid() This patch is required for attachment 113318 [details] [review].
With attachment 81389 [details] [review], attachment 113319 [details] [review], attachment 113318 [details] [review] applied I get from running test-dbus-daemon: /creds: ** Message: ProcessID of this process is 76 ** Message: WindowsSID of this process is S-1-5-21-0-0-0-1000 ** Message: self sid S-1-5-21-0-0-0-1000 0 OK /processid: ** Message: GetConnectionUnixProcessID returned 76 OK /echo/session: OK /echo/limited: OK /no-reply/disconnect: OK /no-reply/timeout: OK /canonical-path/uae: OK
> ** Message: self sid S-1-5-21-0-0-0-1000 0 expect this additional debug message not contained in the test case.
Comment on attachment 113318 [details] [review] Test for windows sid (rebased) Review of attachment 113318 [details] [review]: ----------------------------------------------------------------- ::: test/dbus-daemon.c @@ +382,5 @@ > + g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==, > + DBUS_TYPE_STRING); > + dbus_message_iter_get_basic (&var_iter, &sid); > + g_message ("%s of this process is %s", name, sid); > + if (_dbus_getsid (&self_sid, 0)) At the moment you can't call _dbus_getsid() unless the executable is statically linked to libdbus-internal, so if you want to do this, you will have to do something like: if DBUS_WIN # statically linked so we can call _dbus_getsid() test_dbus_daemon_CPPFLAGS = $(static_cppflags) test_dbus_daemon_LDADD = \ libdbus-testutils-internal.la \ $(GLIB_LIBS) \ $(NULL) else test_dbus_daemon_CPPFLAGS = $(testutils_shared_if_possible_cppflags) test_dbus_daemon_LDADD = \ $(testutils_shared_if_possible_libs) \ $(GLIB_LIBS) \ $(NULL) endif (and maybe the CMake equivalent) - I would really prefer to keep as many tests as possible dynamically linked on Unix, because it makes them much more useful as integration tests. Alternatively, if someone reviews Bug #83115 one day, then we can access _dbus functions even in dynamically-linked tests. The other possibility is to put the comparison with _dbus_getsid in #ifdef DBUS_STATIC_BUILD. @@ +384,5 @@ > + dbus_message_iter_get_basic (&var_iter, &sid); > + g_message ("%s of this process is %s", name, sid); > + if (_dbus_getsid (&self_sid, 0)) > + result = strcmp (self_sid, sid); > + g_assert_cmpuint (result, ==, 0); No need for 'result', just use g_assert_cmpstr (self_sid, ==, sid); which does the same strcmp internally, but also gives a better assertion-failure message if it fails. I think you're leaking self_sid here? Please dbus_free() it.
Comment on attachment 113319 [details] [review] Minor optimization in _dbus_getsid() Review of attachment 113319 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +993,5 @@ > PSID psid; > int retval = FALSE; > > + HANDLE process_handle = process_id == 0 ? GetCurrentProcess() > + : OpenProcess(is_winxp_sp3_or_lower() ? PROCESS_QUERY_INFORMATION : PROCESS_QUERY_LIMITED_INFORMATION, FALSE, process_id); This seems like too many ?: operators :-) I think it'd be a lot clearer like this: HANDLE process_handle; if (process_id == 0) process_handle = GetCurrentProcess (); else if (is_winxp_sp3_or_lower ()) process_handle = OpenProcess (PROCESS_QUERY_INFORMATION, FALSE, process_id); else process_handle = OpenProcess (PROCESS_QUERY_LIMITED_INFORMATION, FALSE, process_id);
Created attachment 113321 [details] [review] Minor optimization in _dbus_getsid() (update) Made if else chain easier
Comment on attachment 113321 [details] [review] Minor optimization in _dbus_getsid() (update) Review of attachment 113321 [details] [review]: ----------------------------------------------------------------- Much better :-)
(In reply to Simon McVittie from comment #103) > I would really prefer to keep as many > tests as possible dynamically linked on Unix Clarification: on Unix with Autotools. I don't mind if some or all of the tests are consistently statically-linked under CMake if that makes your life easier.
(In reply to Simon McVittie from comment #103) > Alternatively, if someone reviews Bug #83115 one day, then we can access > _dbus functions even in dynamically-linked tests. Given that this touches the entire build system and has 4 months' worth of merge conflicts, I would recommend doing one of the others for now, and we can clean it up if Bug #83115 is ever merged. I should have known this would happen when I worked on Bug #83115 :-(
Created attachment 113330 [details] [review] Add test for Windows SID (update) - fix leak - use g_assert_cmpstr
(In reply to Simon McVittie from comment #103) > Comment on attachment 113318 [details] [review] [review] > Test for windows sid (rebased) > > Review of attachment 113318 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: test/dbus-daemon.c > @@ +382,5 @@ > > + g_assert_cmpuint (dbus_message_iter_get_arg_type (&var_iter), ==, > > + DBUS_TYPE_STRING); > > + dbus_message_iter_get_basic (&var_iter, &sid); > > + g_message ("%s of this process is %s", name, sid); > > + if (_dbus_getsid (&self_sid, 0)) > > At the moment you can't call _dbus_getsid() unless the executable is > statically linked to libdbus-internal, We are using it currently only in a bus not installed test application, so no problem here. > No need for 'result', just use > > g_assert_cmpstr (self_sid, ==, sid); fixed > I think you're leaking self_sid here? Please dbus_free() it. sure, but need to use LocalFree().
(In reply to Ralf Habacker from comment #110) > > At the moment you can't call _dbus_getsid() unless the executable is > > statically linked to libdbus-internal, > > We are using it currently only in a bus not installed test application, so > no problem here. test-dbus-daemon (dbus-daemon.c) is built dynamically-linked under Autotools, and optionally installed as an integration test on Unix.
(In reply to Simon McVittie from comment #111) > test-dbus-daemon (dbus-daemon.c) is built dynamically-linked under > Autotools, and optionally installed as an integration test on Unix. Actually, it's statically linked to libdbus-internal if you don't have dbus-glib, which people compiling for Windows probably don't... so your patch is theoretically not right in all situations, but it's good enough in practice. Fixed in git for 1.9.12, thanks! (I pushed it myself rather than waiting for you, because I wanted to avoid conflicts when I work on Bug #89041, which will also touch test-dbus-daemon.)
Created attachment 113360 [details] [review] bus_driver_handle_get_connection_credentials: do not assert on OOM dbus_connection_get_windows_user is documented to return TRUE but put NULL in its argument if OOM is reached. --- Happened to spot this while adapting this code for the Linux security label (also a string) on Bug #89041. Yes, it's a weird calling convention, but it's what the docs say.
Comment on attachment 113360 [details] [review] bus_driver_handle_get_connection_credentials: do not assert on OOM Review of attachment 113360 [details] [review]: ----------------------------------------------------------------- looks good - I already committed to master
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.