|
Description
LRN
2016-06-18 12:06:26 UTC
Created attachment 124586 [details] [review] Support SSPI NTLM authentication mechanism Note that attachment 124586 [details] [review] lacks many important parts (for one, it has no #ifdefs and thus will prevent dbus from being built for non-Windows hosts; i'm not sure how platform-dependency should be handled in this case - other mechanisms don't seem to have any platform-dependent bits in dbus-auth.c itself). (In reply to LRN from comment #2) > Note that attachment 124586 [details] [review] lacks many important parts > (for one, it has no #ifdefs and thus will prevent dbus from being built for > non-Windows hosts This is obviously a showstopper: D-Bus is a portable project, and its primary target platform is GNU-ish Unix. Possible approaches include: * Split out the SSPI-specifics into a separate source file, and call into that source file from dbus-auth.c, with the calls bracketed by #ifdef DBUS_WIN * Split out the SSPI-specifics into a separate source file, and call into that source file from dbus-auth.c, with stub implementations on other platforms that always raise a "not supported" error * Leave the implementation in dbus-auth.c, but make it #ifdef DBUS_WIN The other major things that I think block this are: * implementation review (preferably from someone who knows Windows APIs as well as from someone with experience in designing for security) * a description of the protocol mapping in the D-Bus Specification (doc/dbus-specification.xml) (In reply to Simon McVittie from comment #3) > (In reply to LRN from comment #2) > > Note that attachment 124586 [details] [review] [review] lacks many important parts > > (for one, it has no #ifdefs and thus will prevent dbus from being built for > > non-Windows hosts > > This is obviously a showstopper: D-Bus is a portable project, and its > primary target platform is GNU-ish Unix. Obviously, current version of the patch is more of a proof-of-concept, and maybe something to perform tests on (i've tried authenticating between manual-authz.exe and dbus-send.exe, seems to be working for me). > > Possible approaches include: > > * Split out the SSPI-specifics into a separate source file, and call into > that source file from dbus-auth.c, with the calls bracketed by > #ifdef DBUS_WIN > > * Split out the SSPI-specifics into a separate source file, and call into > that source file from dbus-auth.c, with stub implementations on other > platforms that always raise a "not supported" error If there are plans to support this authentication mechanism on non-Windows OSes (how?), then the second approach seems better. Otherwise, i'll probably prefer the first approach. Also, note that some data types that are needed to be present in DBusAuth structure (CredHandle, for example) are Windows-specific. Does dbus need this structure to have constant, predictable size? If yes, i can put the necessary data into a sub-struct, which is defined in another file, and then have dummy fields for non-Windows implementations. Otherwise a simple #ifdef would work. > The other major things that I think block this are: > > * implementation review (preferably from someone who knows Windows APIs > as well as from someone with experience in designing for security) I could try drafting fanc (he reviews W32 patches for glib; he's going to be reviewing gdbus patches eventually anyway), though i don't know how much security experience does he have. I should probably also state that *i* don't really have security experience (although the design of SSPI API doesn't allow for much errors; the usual policy is "anything goes wrong - return FALSE"). > > * a description of the protocol mapping in the D-Bus Specification > (doc/dbus-specification.xml) Which chapter should it be in? Also, do you have any examples of protocol mapping descriptions (i have no idea what it is)? Some other things: * Right now either side (client and server) is willing to send 0-length DATA messages to signify that their respective SSPI function gave them a 0-length buffer. This seems suboptimal. And then there's, independently, a "done" boolean variable, which is set depending on the return value of the SSPI function. The main question here is: does dbus allow OK to be received in a state "waiting for DATA"? If yes, then server should just send OK when SSPI says the authentication is done (after sending any non-0-length buffer returned by SSPI). What if server sends the last DATA buffer, then sends OK, but client responds with more DATA? Basically, parts of the protocol are "offloaded" into SSP, and this only works when SSP behaves in a sane way (i.e. does not generate any further DATA buffers after the authentication is done, except for the last buffer returned from the last call). Either party should probably just error-out when SSP behaves unexpectedly... The other question: DBus doesn't seem to concern itself with server authenticating itself to the client. If SSPI tells the client that the authentication process is done (and gives 0-length buffer, so there's nothing to send), what should the client do? Go into "wait for OK" state? Remain in "wait for DATA"? Given this ambiguity, maybe sending 0-length DATA is the right thing - it would tell the other side to not to expect any more DATA messages. If authentication is not done by that point, they can error out. * How soon does dbus shut down the DBusAuth object? Since dbus is not going to use SSPI for encryption (although DBus spec talks about "encrypted" data stream after BEGIN), then it follows that dbus should free SSPI credentials and context once authentication is done. Also, does dbus re-create DBusAuth object anew for each AUTH command (i.e. does it re-use it after REJECT)? * My implementation makes use of initial_data (the optional argument to AUTH) on the client side. Is that okay? Do i need to design things in some kind of special way for them to work for AUTH-with-no-initial-data case? SSPI doesn't seem to have "initial server challenge", and client initiates the exchange without any input from the server (although dumps of the buffer contents, at least for NTLM, show that the first buffer sent by the client contains mostly identity information, so this is not surprising). * Other authentication mechanisms involve "desired" credentials, which are later checked against the actual authenticated credentials, resulting in REJECT if they don't match. My SSPI-based mechanism currently doesn't have this. Once authentication is done, server just gets a security context (and an access token), and OS guarantees that this information (including the SID of the user that token belongs to) is authentic. Does server need to do any other checks at this point (i do understand that there's GetConnectionCredentials(), which can allow other checks to be made later on; not sure where and how it's used though)? Note that EXTERNAL authentication can, in theory, work without "desired" credentials - in case of *nix, you'd just get uid/pid from the socket, and treat them as authentic (because OS checks them); in case of Windows, you'd get SID of the user that runs the client process. Although there *is* an "is superset" credentials check somewhere... Should SSPI mechanism have "desired" credentials? For client - to decide who it wants to authenticate as (although authenticating as someone else is quite difficult, and better be done prior to establishing DBus connection, via impersonation). For server - to verify that the client understands *who* it actually is. (In reply to Simon McVittie from comment #3) > (In reply to LRN from comment #2) > > Note that attachment 124586 [details] [review] [review] lacks many important parts > > (for one, it has no #ifdefs and thus will prevent dbus from being built for > > non-Windows hosts > > The other major things that I think block this are: > > * implementation review (preferably from someone who knows Windows APIs > as well as from someone with experience in designing for security) > git says that most W32-related contributions to dbus came from Simon McVittie (you) and Ralf Habacker, so these people would be the logical choice for reviewing my patches. Also Tor Lillqvist, but i doubt he'd be willing to review dbus patches now (he doesn't even review GTK+ patches, and W32 GTK+ port was *his* doing). (In reply to LRN from comment #5) > git says that most W32-related contributions to dbus came from Simon > McVittie (you) and Ralf Habacker, so these people would be the logical > choice for reviewing my patches. Also Tor Lillqvist, but i doubt he'd be > willing to review dbus patches now (he doesn't even review GTK+ patches, and > W32 GTK+ port was *his* doing). It will be possible for me to look at the patch after my vacation, which ends at Jul 5. I did not worked with SSPI before, so I need to get into this stuff more in detail. Any hints welcome. (In reply to Ralf Habacker from comment #6) > (In reply to LRN from comment #5) > > git says that most W32-related contributions to dbus came from Simon > > McVittie (you) and Ralf Habacker, so these people would be the logical > > choice for reviewing my patches. Also Tor Lillqvist, but i doubt he'd be > > willing to review dbus patches now (he doesn't even review GTK+ patches, and > > W32 GTK+ port was *his* doing). > > It will be possible for me to look at the patch after my vacation, which > ends at Jul 5. I did not worked with SSPI before, so I need to get into this > stuff more in detail. Any hints welcome. MSDN SSPI example[1] programs (i've used these as a reference point) Stackoverflow (for example - [2]) This[3] article and, basically, everything you can google up (usually you'd use function name as a keyword, and maybe name of an SSP). Please do say how platform-dependenness *should* be handled (pick one of the approaches outlined earlier), and answer any of my present questions that you can. That will allow me to improve the code from a proof-of-concept to a patch that can be seriously considered for merging. Specifically, HACKING file states that Windows headers should be only included in sysdeps-win.c and the like, so i guess i'll need some kind of way to wrap SSPI calls, leaving them in systeps-win.c, and only allowing auth.c to use these wrappers...If you know a better way, do share. [1] https://msdn.microsoft.com/en-us/library/windows/desktop/aa379449(v=vs.85).aspx [2] https://stackoverflow.com/questions/9788318/what-targetname-to-use-when-calling-initializesecuritycontext-negotiate [3] http://www.informit.com/articles/article.aspx?p=20989 (In reply to LRN from comment #4) > If there are plans to support this authentication mechanism on non-Windows > OSes (how?), then the second approach seems better. I don't know how or why we'd ever do that. > Otherwise, i'll probably prefer the first approach. That sounds best to me. > Also, note that some data types that are needed to be present in DBusAuth > structure (CredHandle, for example) are Windows-specific. > Does dbus need this structure to have constant, predictable size? The right answer to this and all your other questions is likely to be "please consult the source code". Unfortunately, I don't think any of the current maintainers were involved in adding the current authentication mechanisms. > > * a description of the protocol mapping in the D-Bus Specification > > (doc/dbus-specification.xml) > > Which chapter should it be in? <https://dbus.freedesktop.org/doc/dbus-specification.html#auth-mechanisms> "Authentication mechanisms", similar to our only D-Bus-specific mechanism so far, DBUS_COOKIE_SHA1. > Also, do you have any examples of protocol > mapping descriptions (i have no idea what it is)? What I mean is a textual description of how a generic SSPI conversation gets turned into challenges and responses to be passed via DATA messages. https://tools.ietf.org/html/rfc4752 might be useful inspiration, although it doesn't need to be that formal. > * Right now either side (client and server) is willing to send 0-length DATA > messages to signify that their respective SSPI function gave them a 0-length > buffer. This seems suboptimal. *shrug* if this is how SSPI works, it's how SSPI works... > The main question here is: does dbus allow OK to be received in a state > "waiting for DATA"? See <https://dbus.freedesktop.org/doc/dbus-specification.html#auth-states> "Authentication state diagrams". Does that answer your question? > Either party should > probably just error-out when SSP behaves unexpectedly... As a general design principle: yes. > The other question: DBus doesn't seem to concern itself with server > authenticating itself to the client. Correct. The D-Bus client is assumed to have obtained a listening address from "the system" by some trusted out-of-band mechanism - in practice, it normally inherits an environment variable from its parent. We assume that the desired server will continue to occupy that address. > If SSPI tells the client that the > authentication process is done (and gives 0-length buffer, so there's > nothing to send), what should the client do? Sorry, I don't know. You know more about SSPI than I do. What would a GSSAPI client do in the same situation? > * How soon does dbus shut down the DBusAuth object? Don't trust me on this, read the source code instead :-) > Since dbus is not going > to use SSPI for encryption (although DBus spec talks about "encrypted" data > stream after BEGIN) In principle, a SASL mechanism might continue to rewrite the data stream after it has successfully completed the handshake, for example to encrypt it or to insert integrity checks (HMAC), and I've even seen code in libdbus to cope with the need to map between the wire-level data stream and the unencrypted D-Bus stream. In practice, none of the mechanisms supported by libdbus actually do this, and the code to do that isn't tested (and therefore probably doesn't actually work). > * My implementation makes use of initial_data (the optional argument to > AUTH) on the client side. Is that okay? Do i need to design things in some > kind of special way for them to work for AUTH-with-no-initial-data case? In SASL jargon, this is an "initial response". I think SASL was originally designed for protocols where the server always speaks first - challenge/response, challenge/response, etc. - so the server's data is always called a challenge and the client's is called a response. The "initial response" was bolted on later because it saves a round-trip. Some protocols don't support an initial response (IMAP originally didn't), so a fully generic SASL mechanism should cope with being unable to send one. However, we know that D-Bus does support an initial response, and you're designing this mechanism for D-Bus and not for general use, so you can just assume it will happen. > * Other authentication mechanisms involve "desired" credentials, which are > later checked against the actual authenticated credentials, resulting in > REJECT if they don't match. My SSPI-based mechanism currently doesn't have > this. Once authentication is done, server just gets a security context (and > an access token), and OS guarantees that this information (including the SID > of the user that token belongs to) is authentic. The desired credentials are the "authorization identity" in SASL jargon. The important thing is that if we have inconsistent information, we must either make sure to forget about anything that has not been authenticated securely, or abort the whole process. Strictly speaking, in SASL the client can say that it wants some identity, the "authorization identity"; then authenticate to prove that it has some other identity, the "authentication identity"; and then the server might apply a rule to see whether the authentication identity is allowed (authorized) to become the authorization identity (think of sudo - I authenticate that I am smcv and the system recognizes that I am authorized to be root). In practice, D-Bus never does this: the authentication identity is the same thing as the authorization identity. > Does server need to do any > other checks at this point (i do understand that there's > GetConnectionCredentials(), which can allow other checks to be made later > on; not sure where and how it's used though)? It's like this: SASL handshake: * new client: I am uid 1000 * server: really? prove it * new client: (proves it) * server: OK, I'll believe you While switching from SASL to D-Bus: * server calls the function configured with dbus_connection_set_unix_user_function() or dbus_connection_set_windows_user_function() to see whether uid 1000 is even allowed on this bus - if not, server disconnects them * server allocates a unique name for the new client, say :1.23 * server remembers that connection :1.23 is uid 1000 Later, using D-Bus messages: * :1.42 method call: GetConnectionCredentials(":1.23") * server checks what it remembered earlier, and sees that connection :1.23 has uid 1000 * server reply: { "UnixUserID": 1000 } (For Windows, replace all UID integers with SID strings and relabel accordingly.) > Note that EXTERNAL authentication can, in theory, work without "desired" > credentials It can, but that's not how we use them. We always send the uid, in some stupid encoding (I think it's the hex-encoded decimal-encoded uid or something, so uid 1000 becomes "31303030"). > Should SSPI mechanism have "desired" credentials? For client - to decide who > it wants to authenticate as (although authenticating as someone else is > quite difficult, and better be done prior to establishing DBus connection, > via impersonation). For server - to verify that the client understands *who* > it actually is. If you want this to be a fully general SASL mechanism, you might want to include support for specifying an identity, but for D-Bus' purposes I wouldn't bother. Created attachment 124713 [details] [review] Support SSPI NTLM authentication mechanism v2: * Put W32-specific code into sysdeps-win.c, auth.c only calls a few internal functions that generally outline the authentication process. That said, there are still ifdefs in auth.c, getting rid of them will be more difficult. * Don't allow empty DATA messages. * Rely on SSPI being sane, treat any insane behaviour as an error. Created attachment 124714 [details] [review] Add DBUS_WINDOWS_SSPI_NTLM spec section Ping. Sorry, I don't have the bandwidth right now for detailed review of a security-sensitive area of dbus, that I'm not already familiar with, on an OS I don't use. This is on the queue, and we'll get there when we get there. (In reply to Simon McVittie from comment #12) > Sorry, I don't have the bandwidth right now for detailed review of a > security-sensitive area of dbus, that I'm not already familiar with, on an > OS I don't use. > > This is on the queue, and we'll get there when we get there. Actually, i was upping the bug because of Ralf Habacker's comment about him being able to review this after July 5. (In reply to Ralf Habacker from comment #6) > It will be possible for me to look at the patch after my vacation, which > ends at Jul 5. I did not worked with SSPI before, so I need to get into this > stuff more in detail. Any hints welcome. Review is delayed until mid of this week. You mentioned in comment 4 that you used manual-authz.exe and/or dbus-send.exe to check the implemention. Can you give an explanation how you used this tools ? (In reply to Ralf Habacker from comment #14) > (In reply to Ralf Habacker from comment #6) > > It will be possible for me to look at the patch after my vacation, which > > ends at Jul 5. I did not worked with SSPI before, so I need to get into this > > stuff more in detail. Any hints welcome. > > Review is delayed until mid of this week. > > You mentioned in comment 4 that you used manual-authz.exe and/or > dbus-send.exe to check the implemention. Can you give an explanation how you > used this tools ? Run bash, $ cd ${blddir} $ DBUS_VERBOSE=1 DBUS_TEST_DATA="${instdir}/mingw/libexec/installed-tests/dbus/data" PATH="${instdir}/mingw/bin:$PATH" ./test/.libs/manual-authz.exe write down the ${portnumber} that dbus prints (for Normal server, but it's entertaining to try other servers as well) Run cmd[1], $ cd ${instdir}/mingw/bin $ dbus-send --address=tcp:host=127.0.0.1,port=${portnumber} --dest=org.freedesktop.DBus --type=method_call --print-reply /org/freedesktop/DBus org.freedesktop.DBus.ListNames and observe what manual-authz dumps in stderr. Giving DBUS_VERBOSE=1 to dbus-send is also a possibility. ${instdir} is the directory that was used in "make install DESTDIR=${instdir}" ${blddir} is the directory from which configure script was run and where makefiles ended up [1] Can't use MSYS-bash, because dbus-send is a Windows application, so MSYS will try to mangle the "/org/freedesktop/DBus org.freedesktop.DBus.ListNames" argument. Therefore, dbus-send should be run from a Windows command line shell, appropriately set up ("set PATH=/mingw/bin;%PATH%" mostly, where "/mingw" is the MinGW root directory) Created attachment 125016 [details] [review] Add cmake support for sspi implementation. Bug: (In reply to LRN from comment #15) > $ dbus-send --address=tcp:host=127.0.0.1,port=${portnumber} > --dest=org.freedesktop.DBus --type=method_call --print-reply > /org/freedesktop/DBus org.freedesktop.DBus.ListNames > > and observe what manual-authz dumps in stderr. > Giving DBUS_VERBOSE=1 to dbus-send is also a possibility. Tried this with cmake cross compile build (see appended patch) and did run this with wine, which fails to connect. Because it may fail caused by wine itself I'm going to check this tomorrow on a native windows host. BTW: I guess activating sspi for the server requires <auth>DBUS_WINDOWS_SSPI_NTLM</auth> in related session.conf with autotools too. (In reply to Ralf Habacker from comment #17) > > BTW: I guess activating sspi for the server requires > <auth>DBUS_WINDOWS_SSPI_NTLM</auth> in related session.conf with autotools > too. The tests i've been doing worked because the server had the list of acceptable auth mechanisms set to NULL, which means "all mechanisms are acceptable". I didn't go further than that (i.e. running actual programs with an actual, installed dbus daemon), mainly because i don't have any programs to test. The only DBus server that normally runs on my system is the short-lived bus that glib creates. (In reply to Ralf Habacker from comment #17) > Because it may fail caused by wine > itself I'm going to check this tomorrow on a native windows host. The mentioned sspi example did not work out of the box and contains logic errors at calling QueryContextAttributes(): hctxt is not set. Also the original testcase do not support providing server and user name on command line. I placed a fixed and workable version at https://cgit.freedesktop.org/~rhabacker/dbus/commit/?h=sspi&id=58360164138479ac8337c0148dcbc338f1a162ef > Tried this with cmake cross compile build (see appended patch) and did run > this with wine, which fails to connect. Explanation: I personally develop dbus in a cross compile environment and run dbus tools normally with wine, which works mostly out of the box and has the advantage to be able to quick run changes on linux and windows. Running the mentioned test case with wine returns an error on creating the InitializeSecurityContext wine bin/sspi-server.exe $USER Waiting for client to connect... Listening ! fixme:secur32:nego_AcquireCredentialsHandleW forwarding to NTLM Could not authenticate the socket. wine bin/sspi-client.exe $HOST $USER Socket created. fixme:secur32:nego_AcquireCredentialsHandleW forwarding to NTLM fixme:netapi32:NetWkstaUserGetInfo Level 1 processing is partially implemented fixme:advapi:LsaOpenPolicy ((null),0x69ef58,0x00000001,0x69ef50) stub fixme:advapi:LsaClose (0xcafe) stub InitializeSecurityContext failed error - code 00000000 Exiting. I'm going to file a wine bug report. > (In reply to LRN from comment #15) > > $ dbus-send --address=tcp:host=127.0.0.1,port=${portnumber} > > --dest=org.freedesktop.DBus --type=method_call --print-reply > > /org/freedesktop/DBus org.freedesktop.DBus.ListNames > > > > and observe what manual-authz dumps in stderr. > > Giving DBUS_VERBOSE=1 to dbus-send is also a possibility. Thanks for this pointer. After be able to run the sspi example on windows, this test scenario (with the <auth>DBUS_WINDOWS_SSPI_NTLM</auth> addition in session.conf) should help to check if the sspi implementation works. On native Windows 7 32bit I did: 1. Make sure <auth>DBUS_WINDOWS_SSPI_NTLM</auth> is included in <dbus-install-root>\bus\session.conf 2. run <dbus-install-root>\bin\dbus-daemon.exe --config-file=<dbus-install-root>\bus\session.conf --print-address 3. run <dbus-install-root>\bin\dbus-send.exe --address=.... --dest=org.freedesktop.DBus --type=method_call --print-reply /org/freedesktop/DBus org.freedesktop.DBus.ListNames From the appended server.log it could be shown that the client has been authenticated using 'DBUS_WINDOWS_SSPI_NTLM'. 2848: 0x1a40: 1468403343.220758 [dbus/dbus-auth.c(1380):handle_server_data_windows_sspi_ntlm_mech] server: authenticated client using DBUS_WINDOWS_SSPI_NTLM ... 2848: 0x1a40: 1468403343.891560 [dbus/dbus-auth.c(2305):goto_state] server: going from state WaitingForBegin to state Authenticated Next try with dbus-monitor works too. Created attachment 125050 [details]
dbus-send log
Created attachment 125051 [details]
server.log (dbus-daemon log)
Any progress on this? Comment on attachment 124713 [details] [review] Support SSPI NTLM authentication mechanism Review of attachment 124713 [details] [review]: ----------------------------------------------------------------- I have no idea whether the Win32 API side of this is right, this is just a (relatively brief) review of the libdbus side. ::: dbus/dbus-auth.c @@ +1332,5 @@ > + DBusString challenge; > + > + if (!_dbus_sspi_ntlm_next_challenge (&auth->sspi_ntlm, data, &challenge, &done, &error)) > + { > + if (dbus_error_is_set (&error)) You should never need to do this. If a function takes a DBusError * parameter, it should *always* set the error on failure: it is incorrect to have both a failure mode where the error is set, and a failure mode where it isn't. (_dbus_return_if_fail() is an exception to that rule, but non-public functions can't call that anyway.) @@ +1350,5 @@ > + if (!done) > + { > + if (_dbus_string_get_length (&challenge) == 0) > + { > + send_error (auth, "SSPI produced 0-length challenge, but authentication is not done yet"); Is this an API guarantee of SSPI? @@ +1363,5 @@ > + } > + > + if (!_dbus_sspi_ntlm_fetch_credentials (&auth->sspi_ntlm, auth->authorized_identity, &error)) > + { > + if (dbus_error_is_set (&error)) Same again @@ +1395,5 @@ > + DBusError error = DBUS_ERROR_INIT; > + > + if (!_dbus_sspi_ntlm_initial_response (&auth->sspi_ntlm, response, &error)) > + { > + if (dbus_error_is_set (&error)) Same @@ +1425,5 @@ > + > + return FALSE; > + } > + > + if (_dbus_string_get_length (&response) > 0) Is it an API guarantee of SSPI that a zero-length response always indicates success, or does _dbus_sspi_ntlm_next_response need to return an enum { OK, CONTINUE, ERROR, OOM } or something? (See also Bug #97298) ::: dbus/dbus-sysdeps-win.c @@ +56,4 @@ > #include <iphlpapi.h> > > /* Declarations missing in mingw's and windows sdk 7.0 headers */ > +#ifndef __MINGW64_VERSION_MAJOR Why? Re-declaring a symbol that was already declared, with an identical signature, is allowed. If this is needed for a particular toolchain, it should be a separate change, first. @@ +2540,4 @@ > if (SymFromAddr (GetCurrentProcess (), sf.AddrPC.Offset, &displacement, pSymbol)) > { > if (displacement) > + DPRINTF ("%3d %s+0x%" PRIx64, i++, pSymbol->Name, displacement); This is orthogonal to the change you are making. If it is necessary for some reason, do this first as a separate change. My understanding is that all significant Windows compilers and C libraries understand "%I64x" as equivalent to Standard C PRIx64, but not all Windows compilers define PRIx64. If some compiler doesn't support "%I64x", we probably need to do this instead? #ifndef PRIx64 #define PRIx64 "%I64x" #endif @@ +3610,5 @@ > +#define def_sspi_err(literal,meaning) { literal, #literal, meaning }, > + > +static DBusSSPIErrorEntry dbus_sspi_error_map[] = > +{ > + def_sspi_err (SEC_E_ALGORITHM_MISMATCH, "The client and server cannot communicate because they do not possess a common algorithm.") Is there no standard Windows function analogous to strerror() that would give us this information? I have a nasty suspicion that these string literals might be copy/pasted from somewhere. If they are, they are probably numerous enough for that to be copyright infringement, unless there's a suitable copyright license that can be cited. Leaving out the meaning column and just having the literals would be fine; slightly less nice to debug, but realistically if you're debugging this stuff you're going to have MSDN permanently open in a browser anyway :-) @@ +3730,5 @@ > + meaning = "Meaning unknown"; > + } > + > + if (func_call) > + dbus_set_error (error, "win32.error", "%s returned 0x%08lx (%s): %s", func_call, status, literal, meaning); I'm reasonably sure we don't own the DNS name error.win32. Use one of the standard errors from dbus-protocol.h, probably DBUS_ERROR_AUTH_FAILED, or add a new one. @@ +3867,5 @@ > + if (ss != SEC_E_OK) > + { > + _dbus_win_warn_sspi_status ("server", "QuerySecurityPackageInfoW (NTLM)", ss); > + > + return FALSE; Do not return FALSE without setting the error. You probably want to set the error *instead of* complaining to stderr. Errors are meant to bubble up the stack like C++/Java/etc. exceptions or GLib GError until they reach someone who can handle them. (Most likely by complaining to stderr, but that's not our problem :-) @@ +3877,5 @@ > + > + if (ss != SEC_E_OK) > + _dbus_win_warn_sspi_status ("server", "FreeContextBuffer (PSecPkgInfoW)", ss); > + > + sspi_ntlm->output_buffer = dbus_malloc (sspi_ntlm->message_limit); We prefer to use DBusString for memory buffers, preallocated if necessary. It's our equivalent of both GString and GByteArray, and does its best to avoid buffer overruns and other such badness. @@ +3937,5 @@ > + } > + > + _dbus_string_init_const_len (challenge, > + output_sec_buffer.pvBuffer, > + output_sec_buffer.cbBuffer); You can't do this. init_const doesn't copy the data, and output_sec_buffer is on the stack, so it will have become invalid by the time you return. You will have to use _dbus_string_append(), which can fail. The convention in most sysdeps functions is that the caller is expected to pass in an initialized string, to which the function will append what it needs to - for a simple example look at the implementations and callers of _dbus_append_user_from_current_process(). @@ +3969,5 @@ > + if (ss != SEC_E_OK) > + { > + _dbus_win_warn_sspi_status ("server", "QuerySecurityContextToken ()", ss); > + > + return FALSE; Again, set the error instead of spamming stdout. @@ +3976,5 @@ > + result = _dbus_get_token_sid (&sid, client_token); > + CloseHandle (client_token); > + > + if (!result) > + return result; Set the error. If the only reason why the function you are calling can fail is out-of-memory and so it doesn't have a DBusError * parameter, then you're responsible for calling _DBUS_SET_OOM() on its behalf. @@ +3993,5 @@ > +void > +_dbus_sspi_ntlm_free_info (DBusSSPINTLMInfo *sspi_ntlm) > +{ > + if (sspi_ntlm->output_buffer != NULL) > + dbus_free (sspi_ntlm->output_buffer); I'm fairly sure dbus_free() is guaranteed to be NULL-tolerant. @@ +4037,5 @@ > + if (ss != SEC_E_OK) > + { > + _dbus_win_warn_sspi_status ("client", "QuerySecurityPackageInfoW ()", ss); > + > + return FALSE; Again, this function doesn't set the error where it should. @@ +4058,5 @@ > + return FALSE; > + } > + > + sspi_ntlm->free_credentials = TRUE; > + sspi_ntlm->output_buffer = dbus_malloc (sspi_ntlm->message_limit); We prefer to use a DBusString for memory buffers, preallocated to the right length if necessary. dbus_malloc() and all the DBusString functions are allowed to fail (unlike g_malloc() which would just abort). libdbus code is expected to cope with that by setting the error (if there is one) with _DBUS_SET_OOM, and failing. @@ +4101,5 @@ > + return FALSE; > + } > + } > + > + _dbus_string_init_const_len (&plaintext, output_sec_buffer.pvBuffer, output_sec_buffer.cbBuffer); This one is OK, because plaintext is not used after the function returns. @@ +4106,5 @@ > + > + result = _dbus_string_hex_encode (&plaintext, 0, initial_response, > + _dbus_string_get_length (initial_response)); > + > + _dbus_string_free (&plaintext); This is not needed, because you used init_const, which does not copy or allocate memory. @@ +4135,5 @@ > + SecBufferDesc output_sec_buffers_descriptor = { SECBUFFER_VERSION, 1, &output_sec_buffer }; > + ULONG attributes; > + > + input_sec_buffer.cbBuffer = _dbus_string_get_length (data); > + input_sec_buffer.pvBuffer = (void *) _dbus_string_get_const_data (data); Use _dbus_string_get_data() if you need a non-const reference. @@ +4167,5 @@ > + ss != SEC_I_CONTINUE_NEEDED) > + { > + _dbus_win_warn_sspi_status ("client", "InitializeSecurityContextW ()", ss); > + > + return FALSE; Error-handling again @@ +4185,5 @@ > + } > + > + _dbus_string_init_const_len (response, > + output_sec_buffer.pvBuffer, > + output_sec_buffer.cbBuffer); Needs to be copied again, init_const won't do ::: dbus/dbus-sysdeps-win.h @@ +115,5 @@ > +#ifndef DBUS_WINCE > + CredHandle credentials; /**< SSPI Credentials. */ > + CtxtHandle context; /**< SSPI Context. */ > + size_t message_limit; /**< Maximum size of an SSPI NTLM message. */ > + char *output_buffer; /**< Pre-allocated buffer for an SSPI NTLM message. */ I'd very much prefer a DBusString. DBusString is our equivalent of both GString and GByteArray. Comment on attachment 124714 [details] [review] Add DBUS_WINDOWS_SSPI_NTLM spec section Review of attachment 124714 [details] [review]: ----------------------------------------------------------------- ::: doc/dbus-specification.xml @@ +3065,5 @@ > + <para> > + As demanded by SSPI documentation, each party must send every buffer > + given by SSPI to the other party, including the last buffer that is > + received when SSPI indicates that context negotiation is finished. > + However, if buffer has zero length, it should not be sent. If SSPI Can't we send zero-length buffers? Does SSPI guarantee not to produce zero-length buffers? Comment on attachment 125016 [details] [review] Add cmake support for sspi implementation. Review of attachment 125016 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +469,4 @@ > # bus-test expects a non empty string > set (DBUS_USER "Administrator") > set (DBUS_TEST_USER "guest") > + set (DBUS_SESSION_CONF_MAYBE_AUTH_EXTERNAL "<auth>DBUS_WINDOWS_SSPI_NTLM</auth>") This (and the corresponding constant in Autotools) should be renamed to DBUS_SESSION_CONF_AUTH_MECHANISMS or something, if we're going to use it like this. However, I don't think this can be done, at least not for a few years. On Unix, we can safely make dbus-daemon only accept EXTERNAL auth, because in practice all D-Bus implementations implement EXTERNAL auth. The same is not true for DBUS_WINDOWS_SSPI_NTLM on Windows: deployed versions of GDBus, dbus-java, dbus-sharp, etc. do not implement it, so they would be broken by forcing this as the only auth mechanism allowed. It would be fine to emit "<!--<auth>DBUS_WINDOWS_SSPI_NTLM</auth>-->" or something, as an example for security-conscious sysadmins who know that every D-Bus client they are interested in does in fact support that mechanism. (In reply to LRN from comment #4) > The main question here is: does dbus allow OK to be received in a state > "waiting for DATA"? Yes it does, see the spec. It turns out the current implementation *only* does this (Bug #97298). > If yes, then server should just send OK when SSPI says > the authentication is done (after sending any non-0-length buffer returned > by SSPI). Yes, I think so. > The other question: DBus doesn't seem to concern itself with server > authenticating itself to the client. If SSPI tells the client that the > authentication process is done (and gives 0-length buffer, so there's > nothing to send), what should the client do? Go into "wait for OK" state? Yes. As far as I understand it, that is the intended use for "waiting for OK". (In reply to Simon McVittie from comment #25) > It would be fine to emit "<!--<auth>DBUS_WINDOWS_SSPI_NTLM</auth>-->" or > something, as an example for security-conscious sysadmins who know that > every D-Bus client they are interested in does in fact support that > mechanism. Expanding on that: If you say <auth>FOO</auth> <auth>BAR</auth> then dbus-daemon will *only* accept FOO and BAR authentication, breaking clients that support neither FOO nor BAR. If you have no <auth> elements at all, then dbus-daemon will in principle accept anything that it understands (and the way LRN's patch works, the libdbus client-side will try NTLM first). ANONYMOUS is special-cased, for reasonably obvious reasons. Anonymous clients are only allowed to remain connected if dbus-daemon is specially configured (and I've seriously considered deleting the code that would allow it, to reduce the scope for developers like those described in http://www.bbc.co.uk/news/technology-33650491 shooting themselves in the foot). (In reply to Simon McVittie from comment #23) > Comment on attachment 124713 [details] [review] [review] > Support SSPI NTLM authentication mechanism > > Review of attachment 124713 [details] [review] [review]: > ----------------------------------------------------------------- > > I have no idea whether the Win32 API side of this is right, this is just a > (relatively brief) review of the libdbus side. (by convention, if i didn't reply to a piece of code comment, it means "okay, will do that" or "noted, will go back to it once some other issues are cleared up") > > ::: dbus/dbus-auth.c > @@ +1332,5 @@ > > + DBusString challenge; > > + > > + if (!_dbus_sspi_ntlm_next_challenge (&auth->sspi_ntlm, data, &challenge, &done, &error)) > > + { > > + if (dbus_error_is_set (&error)) > > You should never need to do this. If a function takes a DBusError * > parameter, it should *always* set the error on failure: it is incorrect to > have both a failure mode where the error is set, and a failure mode where it > isn't. > > (_dbus_return_if_fail() is an exception to that rule, but non-public > functions can't call that anyway.) So dbus should send a DBusError to the other party even if the error is internal to dbus implementation (i.e. failed to query the OS for something)? In my current version of the code _dbus_sspi_ntlm_next_challenge() only sets DBusError in one case - when input buffer has 0 length, which is an error caused by the other party and thus requires a notification. At least, that's what i was thinking. > > @@ +1350,5 @@ > > + if (!done) > > + { > > + if (_dbus_string_get_length (&challenge) == 0) > > + { > > + send_error (auth, "SSPI produced 0-length challenge, but authentication is not done yet"); > > Is this an API guarantee of SSPI? Two pieces of SSPI (one on the client side and one on the server side) communicate only through the buffers that they produce and that the application code (in this case - dbus) sends between the parties. If an SSPI function produced 0-length buffer to be sent, it means that further negotiation is impossible. If negotiation is not "done" (as reported by the same SSPI function) by that point, it can't be completed. So yes, i'd say that this is a guarantee. > @@ +1425,5 @@ > > + > > + return FALSE; > > + } > > + > > + if (_dbus_string_get_length (&response) > 0) > > Is it an API guarantee of SSPI that a zero-length response always indicates > success, or does _dbus_sspi_ntlm_next_response need to return an enum { OK, > CONTINUE, ERROR, OOM } or something? (See also Bug #97298) Internally _dbus_sspi_ntlm_next_response() calls InitializeSecurityContextW(), which does return a status (in old version of this code it had a 'done' variable, which it set from that status, same as the other function), but current code ignores the 'done' part of it. The reason why is that even when we're 'done', we still must send any buffer that SSPI gives us. Because the client side does not need to do anything special once it's done (we're only authenticating on the server side, the client doesn't check server credentials, for example), it does not check for that. The code assumes that if it has a buffer to send, then server will reply something (OK or whatever DBus spec says it should reply), and if we have no buffer to send, then we're done and should wait for the OK from the server. This is just the theory, from generic SSPI documentation. Now, a month later, i don't remember the details (will have to read the old logs to be sure), but in practice the client gets 'done' status and a non-0-length buffer, while the server gets 'done' status and a 0-length buffer. Thus the "wait for OK from server on 0-length buffer" on the client side is mostly theoretical, never happens in practice for NTLM. This seemed a reasonable compromise between being paranoid about SSPI implementation (which we can't control) and not sending unnecessary data. The expectation is that both sides use sane SSPI implementation (usually the *same* SSPI implementation), and it wouldn't do completely stupid stuff (for example, exchange non-0-length buffers indefinitely, or produce 0-length buffer and expect it to be sent). > > ::: dbus/dbus-sysdeps-win.c > @@ +56,4 @@ > > #include <iphlpapi.h> > > > > /* Declarations missing in mingw's and windows sdk 7.0 headers */ > > +#ifndef __MINGW64_VERSION_MAJOR > > Why? Re-declaring a symbol that was already declared, with an identical > signature, is allowed. Because signatures in dbus-sysdeps-win.c and in mingw-w64 headers are different: ../../repo/dbus/dbus-sysdeps-win.c:60:20: warning: 'ConvertStringSidToSidA' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] extern BOOL WINAPI ConvertStringSidToSidA (LPCSTR StringSid, PSID *Sid); ^ ../../repo/dbus/dbus-sysdeps-win.c:61:20: warning: 'ConvertSidToStringSidA' redeclared without dllimport attribute: previous dllimport ignored [-Wattributes] extern BOOL WINAPI ConvertSidToStringSidA (PSID Sid, LPSTR *StringSid); > > If this is needed for a particular toolchain, it should be a separate > change, first. Okay, will split it into it own patch. > > @@ +2540,4 @@ > > if (SymFromAddr (GetCurrentProcess (), sf.AddrPC.Offset, &displacement, pSymbol)) > > { > > if (displacement) > > + DPRINTF ("%3d %s+0x%" PRIx64, i++, pSymbol->Name, displacement); > > This is orthogonal to the change you are making. If it is necessary for some > reason, do this first as a separate change. Okay. > > My understanding is that all significant Windows compilers and C libraries > understand "%I64x" as equivalent to Standard C PRIx64, but not all Windows > compilers define PRIx64. > > If some compiler doesn't support "%I64x", we probably need to do this > instead? > > #ifndef PRIx64 > #define PRIx64 "%I64x" > #endif > GCC will warn about the use of I64, depending on which printf implementation is used. PRIx64 will expand to the right format specifier, depending on which printf implementation is being used (MSVCRT or MinGW-ANSI). Read https://stackoverflow.com/questions/10678124/mingw-gcc-unknown-conversion-type-character-h-snprintf for more information. > @@ +3610,5 @@ > > +#define def_sspi_err(literal,meaning) { literal, #literal, meaning }, > > + > > +static DBusSSPIErrorEntry dbus_sspi_error_map[] = > > +{ > > + def_sspi_err (SEC_E_ALGORITHM_MISMATCH, "The client and server cannot communicate because they do not possess a common algorithm.") > > Is there no standard Windows function analogous to strerror() that would > give us this information? No, as far as i know. > > I have a nasty suspicion that these string literals might be copy/pasted > from somewhere. If they are, they are probably numerous enough for that to > be copyright infringement, unless there's a suitable copyright license that > can be cited. The strings are from https://msdn.microsoft.com/en-us/library/windows/desktop/aa380499(v=vs.85).aspx > > Leaving out the meaning column and just having the literals would be fine; > slightly less nice to debug, but realistically if you're debugging this > stuff you're going to have MSDN permanently open in a browser anyway :-) If MSDN is not a suitable source of fair-use error description strings, then using literals only #literals would be acceptable, i think. > @@ +3877,5 @@ > > + > > + if (ss != SEC_E_OK) > > + _dbus_win_warn_sspi_status ("server", "FreeContextBuffer (PSecPkgInfoW)", ss); > > + > > + sspi_ntlm->output_buffer = dbus_malloc (sspi_ntlm->message_limit); > > We prefer to use DBusString for memory buffers, preallocated if necessary. > It's our equivalent of both GString and GByteArray, and does its best to > avoid buffer overruns and other such badness. I've used dbus_malloc() instead of a DBusString, because this buffer is passed to SSPI and filled by SSPI. Thus all the niceties that DBusString API might provide are completely lost. AFAIU. > > @@ +3937,5 @@ > > + } > > + > > + _dbus_string_init_const_len (challenge, > > + output_sec_buffer.pvBuffer, > > + output_sec_buffer.cbBuffer); > > You can't do this. init_const doesn't copy the data, and output_sec_buffer > is on the stack, so it will have become invalid by the time you return. > > You will have to use _dbus_string_append(), which can fail. The convention > in most sysdeps functions is that the caller is expected to pass in an > initialized string, to which the function will append what it needs to - for > a simple example look at the implementations and callers of > _dbus_append_user_from_current_process(). output_sec_buffer.pvBuffer is actually sspi_ntlm->output_buffer and is heap-allocated, as explained above. I've used _dbus_string_init_const_len() here to avoid copying the buffer contents. I did so on the assumption that the contents will be quickly sent and the pointer to them discarded soon after. If that is unacceptable, i will have to add an extra API (say, _dbus_sspi_ntlm_get_max_buffer_size()), which will return the size of the buffer, and the caller will have to allocate a new DBusString of that size for each challenge and response. There's data->challenge, which, i think, can be used as a pre-allocated buffer on the server side, but the client side doesn't have anything like that. Should that be added to DBusAuth? Given that the max buffer size is known beforehand, i think it's advantageous to pre-allocate it. Note that it must be DBusAuth, it can't be buried in DBusSSPINTLMInfo, which is opaque to the upper-level code. > @@ +4106,5 @@ > > + > > + result = _dbus_string_hex_encode (&plaintext, 0, initial_response, > > + _dbus_string_get_length (initial_response)); > > + > > + _dbus_string_free (&plaintext); > > This is not needed, because you used init_const, which does not copy or > allocate memory. For symmetry. One string-init (even if it's const) -> one string-free. (In reply to Simon McVittie from comment #24) > Comment on attachment 124714 [details] [review] [review] > Add DBUS_WINDOWS_SSPI_NTLM spec section > > Review of attachment 124714 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: doc/dbus-specification.xml > @@ +3065,5 @@ > > + <para> > > + As demanded by SSPI documentation, each party must send every buffer > > + given by SSPI to the other party, including the last buffer that is > > + received when SSPI indicates that context negotiation is finished. > > + However, if buffer has zero length, it should not be sent. If SSPI > > Can't we send zero-length buffers? > > Does SSPI guarantee not to produce zero-length buffers? SSPI does not guarantee not to produce zero-length buffers. If it has nothing to send, it will fill 0 bytes of the buffer it is given by the caller. I've decided that sending zero-length buffers is unoptimal, and wrote the spec accordingly. If client got 0-length buffer and SSPI says that authentication is done (as mentioned above, does not happen in practice), then it follows that the server-side authentication was also done, and the client just waits for the server to send whatever it is supposed to send next. If server got 0-length buffer and SSPI says that authentication is done, then server can do its credentials magic and then reply with an OK or REJECT. Sending 0-length buffer followed by OK or REJECT seems completely unnecessary to me. (In reply to LRN from comment #28) > (In reply to Simon McVittie from comment #23) > > You should never need to do this. If a function takes a DBusError * > > parameter, it should *always* set the error on failure: it is incorrect to > > have both a failure mode where the error is set, and a failure mode where it > > isn't. > > So dbus should send a DBusError to the other party even if the error is > internal to dbus implementation (i.e. failed to query the OS for something)? I didn't say that. Whether a particular error is sent to the other party is up to the DBusAuth implementor. However, the calling convention for DBusError is that it is like GError: if there is a "bad" return value (FALSE, 0, -1, NULL) that indicates that an error occurred, and a DBusError *, then you should set the DBusError if and only if you return the "bad" return value. If there is a subset of errors that should be sent to the peer, then you can check for a particular machine-readable "name" in the DBusError, or you can have an extra parameter that indicates whether the DBusError should be sent the peer, or return an enum { OK, CONTINUE, LOCAL_ERROR, SEND_ERROR }, or something. > If an SSPI > function produced 0-length buffer to be sent, it means that further > negotiation is impossible. If negotiation is not "done" (as reported by the > same SSPI function) by that point, it can't be completed. Hmm, that's quite weird in SASL terms, but if that's what the documentation says then that's fine. > GCC will warn about the use of I64, depending on which printf implementation > is used. PRIx64 will expand to the right format specifier, depending on > which printf implementation is being used (MSVCRT or MinGW-ANSI). My understanding had been that we only support the MSVCRT code path, because D-Bus compiled for Windows ought to be ABI-compatible with D-Bus compiled with MSVC. Ralf, can you shed any light on this? > The strings are from > https://msdn.microsoft.com/en-us/library/windows/desktop/aa380499(v=vs.85). > aspx ... > If MSDN is not a suitable source of fair-use error description strings, then > using literals only #literals would be acceptable, i think. I suspect this chunk of text is too large and creative (word choice, etc.) to be uncopyrightable, and fair use is too vague (and too variable between jurisdictions) to rely on, so literals only please. > > We prefer to use DBusString for memory buffers, preallocated if necessary. > > It's our equivalent of both GString and GByteArray, and does its best to > > avoid buffer overruns and other such badness. > > I've used dbus_malloc() instead of a DBusString, because this buffer is > passed to SSPI and filled by SSPI. Thus all the niceties that DBusString API > might provide are completely lost. AFAIU. Even so, I'd prefer to preallocate a DBusString, because doing something like buffer.pvBuffer = _dbus_string_get_data (this_string); buffer.cbBuffer = _dbus_string_get_length (this_string); makes it more obvious that we've got the buffer and length paired up correctly. libdbus is security-sensitive code, so it is not enough to be correct: we should aim to be *obviously* correct. > > > + _dbus_string_init_const_len (challenge, > > > + output_sec_buffer.pvBuffer, > > > + output_sec_buffer.cbBuffer); > > > > You can't do this. init_const doesn't copy the data, and output_sec_buffer > > is on the stack, so it will have become invalid by the time you return. > > > > You will have to use _dbus_string_append(), which can fail. The convention > > in most sysdeps functions is that the caller is expected to pass in an > > initialized string, to which the function will append what it needs to - for > > a simple example look at the implementations and callers of > > _dbus_append_user_from_current_process(). > > output_sec_buffer.pvBuffer is actually sspi_ntlm->output_buffer and is > heap-allocated, as explained above. Oh, I see... and you explicitly dbus_free() it in the caller, which I didn't spot and flag as incorrect during my review. No, this isn't how you use DBusString - it's going to confuse future maintainers. A "const" DBusString is meant to be a reference to a string constant, or a very short term reference to a string known to be valid right now; a "non-const" DBusString is meant to manage its own memory. There are two valid ways you can go here: * populate a non-const DBusString (it can be the same one that I recommended you use instead of dbus_malloc() in fact), and return the result via that * return a dbus_malloc()'d buffer via a char ** argument, which the caller is expected to dbus_free() of which I would strongly prefer the first. Mixing the two is worse than either. > If that is unacceptable, i will have to add an extra API (say, > _dbus_sspi_ntlm_get_max_buffer_size()), which will return the size of the > buffer, and the caller will have to allocate a new DBusString of that size > for each challenge and response. You don't have to do that. You can require the caller to pass in a DBusString, use _dbus_string_set_length() to extend it to at least the required buffer size, give it to Windows to fill, truncate it to the number of bytes that were filled if it wasn't all used (_dbus_string_set_length() again), and pass it back to the caller already populated. The caller can use _dbus_string_free() when it has finished with the challenge/response, and allocate a new DBusString when it needs one; or it can keep one DBusString across multiple challenges and responses. DBusString tracks the allocated size independently of the length (with the obvious invariant allocated >= length) so if you preallocate to the desired size on first use, each subsequent set_length() will not have to realloc. > There's data->challenge, which, i think, > can be used as a pre-allocated buffer on the server side, but the client > side doesn't have anything like that. Should that be added to DBusAuth? If it makes it easier, you can; but I honestly don't think a few malloc/free cycles is going to make a significant performance difference. D-Bus connections are already relatively "expensive" to set up. > > This is not needed, because you used init_const, which does not copy or > > allocate memory. > > For symmetry. One string-init (even if it's const) -> one string-free. init_const specifically doesn't need this, so if there is no code path in which a particular string is non-const, I would prefer not to mislead the reader. (In reply to Simon McVittie from comment #29) > > GCC will warn about the use of I64, depending on which printf implementation > > is used. PRIx64 will expand to the right format specifier, depending on > > which printf implementation is being used (MSVCRT or MinGW-ANSI). > > My understanding had been that we only support the MSVCRT code path, because > D-Bus compiled for Windows ought to be ABI-compatible with D-Bus compiled > with MSVC. Ralf, can you shed any light on this? From https://en.wikipedia.org/wiki/MinGW#Programming_language_support "MinGW links by default to the Windows OS component library MSVCRT, which is the C library that Visual C version 6.0 linked to ... MinGW-w64 has resolved these issues, and provides fully POSIX compliant printf functionality." (In reply to Ralf Habacker from comment #30) > "MinGW links by default to the Windows OS component library MSVCRT, which is > the C library that Visual C version 6.0 linked to ... MinGW-w64 has resolved > these issues, and provides fully POSIX compliant printf functionality." I know that MinGW-w64 provides a better/different printf. What I meant was, what is your policy as D-Bus-on-Windows maintainer regarding supported C runtimes and compilers? I know we used to support compilers that don't define PRIx64 and so on. Has that ceased to be the case? Do all interesting compilers define those standard macros now? Do you have a baseline version of MSVC that is the minimum you are willing to support? (If you don't, please choose one, so we can consider brokenness with that version to be a bug and brokenness with older versions to be WONTFIX.) Similarly, we used to support the C runtime libraries that would be used by linking with MSVC (MSVCRT) or by linking with mingw32 (also MSVCRT). Is this policy still in effect? When linking with mingw-w64, do we aim to support both of its configurations - GNU printf ("%lld" etc.) and Microsoft printf ("%I64d" etc.) - or do you require a specific one of them to be selected? mingw-w64 appears to use GNU printf if any of _POSIX, _POSIX_SOURCE, _POSIX_C_SOURCE, _ISOC99_SOURCE, _XOPEN_SOURCE, _XOPEN_SOURCE_EXTENDED, _GNU_SOURCE or _SVID_SOURCE is defined, or a Microsoft(-compatible) printf otherwise. I think this means dbus will select GNU printf because we are using AC_USE_SYSTEM_EXTENSIONS, but we could avoid AC_USE_SYSTEM_EXTENSIONS for builds that target Windows if you would prefer to standardize on using Microsoft-compatible printf on Windows. (In reply to Simon McVittie from comment #31) > I know we used to support compilers that don't define PRIx64 and so on. Has > that ceased to be the case? Do all interesting compilers define those > standard macros now? It appears we already have a hard requirement for PRIx64 on everything that is not Windows, and nobody has complained. At the moment a couple of files use this idiom: #if !defined(PRIx64) && defined(DBUS_WIN) #define PRIx64 "I64x" #endif If all supported Windows compilers also define PRIx64, we should delete those snippets and be happy. My understanding is that mingw-w64 already defines PRIx64, unconditionally, to the right thing for whatever printf implementation we are going to get if we call one of the printf() family of functions. If not all supported Windows compilers define PRIx64, I think we should remove those snippets, and put one copy of the same thing in dbus-sysdeps.h. I would also be happy to define any of the other ISO C PRIwhatever macros in a similar way, if they are needed. (In reply to Simon McVittie from comment #32) > > If all supported Windows compilers also define PRIx64, dbus website states the following supported compilers: 1. MSVC 2010 2. mingw-w32/w64(gcc) 3. cygwin(gcc). issue 1: 'I64x' is supported since msvc2005 (see https://msdn.microsoft.com/de-de/library/tcxf1dw6(v=vs.80).aspx), so it is okay. issue 2: > My understanding is that mingw-w64 already defines PRIx64, unconditionally, agreed: it is defined unconditional at inttypes.h:112:#define PRIx64 "I64x" issue 3: cygwin uses unix api, so it should be the same definition as on native linux > @@ +3730,5 @@ > > + meaning = "Meaning unknown"; > > + } > > + > > + if (func_call) > > + dbus_set_error (error, "win32.error", "%s returned 0x%08lx (%s): %s", func_call, status, literal, meaning); > > I'm reasonably sure we don't own the DNS name error.win32. Use one of the > standard errors from dbus-protocol.h, probably DBUS_ERROR_AUTH_FAILED, or > add a new one. "win32.error" is from https://cgit.freedesktop.org/dbus/dbus/commit/?id=70bfc74e54ac8a9a93885710cd8350d1a58b3406 and https://cgit.freedesktop.org/dbus/dbus/commit/?id=2eca6a2241f0f49009c85704170159bf46c5dbb8 I didn't invent that. Created attachment 125806 [details] [review] Support SSPI NTLM authentication mechanism v3: * Add DBusString "response" field to DBusAuth, use it and the "challenge" field in place of the "output_buffer" field, which is now removed * NTLM functions now have a "local_error" argument which they set to TRUE if the error is local and FALSE if it should be sent to the other party. * NTLM functions always set the error when they fail, higher-level code does not check that the error is set anymore. _DBUS_ASSERT_ERROR_IS_SET () is added to make sure unset error is not tolerated. * _dbus_get_token_sid() gets an error argument and sets it on failure. Note that _dbus_getsid(), which calls _dbus_get_token_sid(), doesn't get one and uses its own internal DBusError instance, which it dumps to stderr (too many functions that i don't want to touch are using _dbus_getsid(), not all of them have DBusError). * Use _dbus_win_set_error_from_win_error() to set the DBusError instance instead of using _dbus_win_warn_win_error() to dump the error message to stderr. Also use _dbus_win_set_error_from_sspi_status() instead of _dbus_win_warn_sspi_status() for the same reason. * Removed message descriptions from dbus_sspi_error_map. * Use DBUS_ERROR_FAILED instead of "win32.error" * Use _dbus_string_get_data (data); instead of (void *) _dbus_string_get_const_data (data); * NTLM functions always take a caller-allocated buffer and change its size by calling _dbus_string_set_length() on it. They also internally call _dbus_string_set_length() again, after SSPI fills the buffer, to ensure that dbus code knows the length of actual data in the buffer. * _dbus_sspi_ntlm_fetch_credentials() logic slightly changed to be better prepared for expanding its implementation later on (to fetch more data from the client_token object). Comment on attachment 125806 [details] [review] Support SSPI NTLM authentication mechanism Review of attachment 125806 [details] [review]: ----------------------------------------------------------------- I tried to 'git am' this patch to git master which failed. Apply: Support SSPI NTLM authentication mechanism fatal: sha1 information is lacking or useless (dbus/dbus-sysdeps-win.c). error: could not build fake ancestor Applying of patch at 0001 Support SSPI NTLM authentication mechanism Looks that this patch is somehow broken and may need a rebase or resend Created attachment 125828 [details] [review] Support SSPI NTLM authentication mechanism v4: * Rebased against master Comment on attachment 125828 [details] [review] Support SSPI NTLM authentication mechanism Review of attachment 125828 [details] [review]: ----------------------------------------------------------------- I applied this patch to git master, created a mingw32 build and did run the tests from comment 19, which are failing with a "could not connect" error. In dbus-daemon verbose log I cannot see any SSPI related AUTH messages, although I added sspi auth mech to session.conf as done for previous testing. (In reply to Ralf Habacker from comment #38) > > I applied this patch to git master, created a mingw32 build and did run the > tests from comment 19, which are failing with a "could not connect" error. > In dbus-daemon verbose log I cannot see any SSPI related AUTH messages, > although I added sspi auth mech to session.conf as done for previous testing. Got it, which was caused by a build system issue. The build dir uses an unpatch dbus source and dbus-daemon did not detect on startup the unsupported <auth> mechanismus. Looks a check on bus startup would be good. Created attachment 125883 [details] [review] On bus startup check given auth in config file against supported mechanismen. This fixes a security hole caused by dbus-daemon ignoring misspelled or unsupported auth mechanismen in bus config file and falling back silently to a less secure authentification level. Bug: Comment on attachment 125883 [details] [review] On bus startup check given auth in config file against supported mechanismen. Review of attachment 125883 [details] [review]: ----------------------------------------------------------------- > This fixes a security hole In future please report things that you think are a security vulnerability to dbus-security@lists.freedesktop.org, or in a new bug marked as only visible to the "D-Bus Security Group". I don't think this is *actually* a security vulnerability, though, for two reasons: * if a sysadmin reconfigures <auth>, they had better know what they are doing; * we do not support any auth mechanisms that we consider to be dangerously insecure, except for ANONYMOUS which has to be enabled explicitly in another way Comment on attachment 125883 [details] [review] On bus startup check given auth in config file against supported mechanismen. Review of attachment 125883 [details] [review]: ----------------------------------------------------------------- > This fixes a security hole caused by dbus-daemon ignoring misspelled or unsupported auth mechanismen Is this really what happens? As far as I can see from both reading the code and testing (dbus 1.11.4-1 on Debian), if you write <auth>NO</auth> <auth>NOPE</auth> then all connection attempts will fail, because the client doesn't try authentication mechanisms NO or NOPE; and if it did, the server wouldn't accept them anyway, because it doesn't know how. Similarly, if you write <auth>NOPE</auth> <auth>EXTERNAL</auth> then that's equivalent to a bare <auth>EXTERNAL</auth>. Or do you have a test-case demonstrating otherwise? I've reviewed the code changes just in case I'm wrong about the dbus-daemon's behaviour. ::: bus/bus.c @@ +414,5 @@ > + if (!_dbus_auth_is_supported_mechanismus (&name)) > + { > + DBusString list; > + _dbus_string_init (&list); > + _dbus_auth_get_supported_mechanismen (&list); Wouldn't _dbus_auth_set_mechanisms() be a better place to do this check? That would mean it's all in DBusAuth, and the new functions you added could be static. As far as I can see, the dbus-daemon gracefully handles setup_server() failing, and will exit; setup_server() indirectly calls _dbus_auth_set_mechanisms(). You don't handle _dbus_string_init() or _dbus_auth_print_supported_mechanisms() (see below for more about the name) failing. Something like this would be appropriate: if (!_dbus_string_init (&list)) goto oom; if (!_dbus_auth_print_supported_mechanisms (&list)) { _dbus_string_free (&list); goto oom; } dbus_set_error (... etc. @@ +420,5 @@ > + "Unsupported auth mechanismus \"%s\" in bus config file detected. Supported mechanismen are \"%s\".", > + link->data, > + _dbus_string_get_const_data (&list)); > + _dbus_string_free (&list); > + retval = FALSE; retval should already be FALSE here, you shouldn't need to set it. ::: dbus/dbus-auth.c @@ +3008,5 @@ > + * @param auth the auth mechanismus to query for > + * @returns #TRUE when auth mechanismus is supported > + */ > +dbus_bool_t > +_dbus_auth_is_supported_mechanismus(DBusString *name) mechanismus -> mechanism (everywhere, not just here) Space before ( please. @@ +3022,5 @@ > + * @param string to hold the supported auth mechanismen > + * @returns #FALSE on oom > + */ > +dbus_bool_t > +_dbus_auth_get_supported_mechanismen(DBusString *buffer) mechanismen -> mechanisms (everywhere, not just here) I'd prefer this function to have a name that indicates that it's returning a human-readable string, not something that callers are expected to parse. _dbus_auth_print_supported_mechanisms() would work, perhaps? (Or maybe you can think of something better?) @@ +3032,5 @@ > + while (all_mechanisms[i].mechanism != NULL) > + { > + if (i > 0) > + { > + if (!_dbus_string_append (buffer, " ")) I'd prefer ", " so the list looks a bit more natural. Created attachment 125914 [details] [review] On bus startup check given auth in config file against supported mechanismen. With recent code starting dbus-daemon with an unsupported auth mechanism let dbus-daemon silently ignore this issue. Clients connecting to this server fails to connect without any descriptive explanation of the root cause, only the message 'Rejected client connection due to lack of memory' error is reported in dbus-daemon verbose log, which is disabled in production environments. With this patch dbus-daemon checks the supported auth mechanisms on startup and shuts down with a descriptive error message, which gives admin an immediate feedback on service startup/restart. The proposal to perform this check in _dbus_auth_set_mechanisms() has the drawback that the check would not be performed on bus startup, only at client side and on handling client connections at server side. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=96577 Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 125915 [details] [review] On bus startup check given auth in config file against supported mechanisms. With recent code starting dbus-daemon with an unsupported auth mechanism let dbus-daemon silently ignore this issue. Clients connecting to this server fails to connect without any descriptive explanation of the root cause, only the message 'Rejected client connection due to lack of memory' error is reported in dbus-daemon verbose log, which is disabled in production environments. With this patch dbus-daemon checks the supported auth mechanisms on startup and shuts down with a descriptive error message, which gives admin an immediate feedback on service startup/restart. The proposal to perform this check in _dbus_auth_set_mechanisms() has the drawback that the check would not be performed on bus startup, only at client side and on handling client connections at server side. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=96577 Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de> (In reply to Ralf Habacker from comment #43) > Created attachment 125914 [details] [review] [review] > On bus startup check given auth in config file against supported mechanismen. > superseeded because of spelling error in title (In reply to LRN from comment #37) > Created attachment 125828 [details] [review] [review] > Support SSPI NTLM authentication mechanism > > v4: > * Rebased against master As this patch now has got libdbus reviews we can concentrate on the sspi related stuff, which is from what I can see: 1. correct sspi api usage 2. correct handling of api return values 3. correct usage of libdbus functions There is a good sspi explanation at http://davenport.sourceforge.net/ntlm.html#ntlmsspAndSspi. Does your implementation follow the mentioned rules or if there are differences, where are the differences ? (In reply to Simon McVittie from comment #41) > I don't think this is *actually* a security vulnerability, though, for two > reasons: > > * if a sysadmin reconfigures <auth>, they had better know what they are > doing; > * we do not support any auth mechanisms that we consider to be dangerously > insecure, except for ANONYMOUS which has to be enabled explicitly in another > way good to hear. (In reply to Simon McVittie from comment #41) > Similarly, if you write > > <auth>NOPE</auth> > <auth>EXTERNAL</auth> > > then that's equivalent to a bare <auth>EXTERNAL</auth>. The problem here is that dbus-damon does not report the usage of unsupported of No and NOPE. > Or do you have a test-case demonstrating otherwise? The uses case is not obviously but still present. Assume a user uses dbus 1.11.5 with sspi patch on windows using an installer, which has <install-root>/share/session.conf marked a config file and does not overwrite on updates. To raise the security level the admin adds <auth>DBUS_WINDOWS_SSPI_NTLM</auth> to share/session.conf. Now he finds out that 1.11.5 contains a bug and downgrades to 1.11.3 without sspi patch and restarts dbus-daemon. Please note that share/session.conf still contains <auth>DBUS_WINDOWS_SSPI_NTLM</auth>. clients can still connect to the server but not using DBUS_WINDOWS_SSPI_NTLM. Another admin may audit dbus installation by inspecting share/session.conf and see DBUS_WINDOWS_SSPI_NTLM, wich let him think that dbus connection are secured by DBUS_WINDOWS_SSPI_NTLM, which is not the case in real. The second issue is that dbus verbose is disabled in production environments, which means client connection failures by using an invalid auth mechanismn isn't reported to an admin wich raises the required time to find the issue. Third: with dbus verbose log enabled recent code using an unsupported auth prints the message 'Rejected client connection due to lack of memory' which looks not very helpful to find the root cause. I believe to let dbus-daemon shutdown in case of unsupported config parameters helps admin to find config issues easier. (In reply to Ralf Habacker from comment #46) > (In reply to LRN from comment #37) > > Created attachment 125828 [details] [review] [review] [review] > > Support SSPI NTLM authentication mechanism > > > > v4: > > * Rebased against master > > There is a good sspi explanation at > http://davenport.sourceforge.net/ntlm.html#ntlmsspAndSspi. Does your > implementation follow the mentioned rules or if there are differences, where > are the differences ? Which rules are you referring to? The link above points to a high-level description of SSPI-NTLM authentication (with a bit more emphasis on NTLM, given that there are tons of explanations of NTLM protocol minutiae before and after that). My code does match that overview. Again, knowledge of NTLM details, which that documentation provides, allows the author to make very specific statements about the effects of various SSPI calls and the contents of messages exchanged between the client and the server, as well as the number of such messages. That matches the behaviour of SSPI-NTLM that i've observed, although these expectations are mostly not present in my patch. If you think it's a good idea, i could try to add these to the patch. For example, hardcode (and check for) the expectation that the exchange is: client (type1) -> server (type2)-> client (type3, done) -> server (done) (it's probably not a good idea to dig into messages to check their contents, but i can easily keep track of the number of messages being exchanged) "Local Authentication" from that documentation page refers to some short-circuiting for the cases when client and server are on the same machine, and, again, has NTLM protocol details that are of no relevance. The only difference is the size of the messages exchanged and other mostly invisible things. Even the number of messages exchanged remains the same. "Datagram Authentication" does not apply (one needs to explicitly request datagram style authentication both client- and server-side, which we don't do). > As this patch now has got libdbus reviews we can concentrate on the sspi > related stuff, which is from what I can see: > > 1. correct sspi api usage The main difficulty here is the lack of SSPI expertise. At best i (and you, it seems) can only state that the SSPI API calls are made in the way that matches documentation and numerous examples from the net, because this is the first time i've used SSPI. Security-wise, that feels somewhat weak. It would be better to have a developer with W32 security experience review that, but i have no idea where to find one or how to convince one to do code review for a free software project. > 2. correct handling of api return values If you mean SSPI API, then that is much easier. MSDN lists very specific return values for functions like InitializeSecurityContext(), and my code does check for them. > 3. correct usage of libdbus functions I had hoped that this was fixed in 3rd version of the patch. (In reply to Ralf Habacker from comment #48) > (In reply to Simon McVittie from comment #41) > > Similarly, if you write > > > > <auth>NOPE</auth> > > <auth>EXTERNAL</auth> > > > > then that's equivalent to a bare <auth>EXTERNAL</auth>. > > The problem here is that dbus-damon does not report the usage of unsupported > of No and NOPE. Right, so it isn't a security vulnerability. Reporting the misconfiguration (as in your patch) is still a good improvement to have, of course. > Assume a user uses dbus 1.11.5 with sspi patch on windows using an > installer, which has <install-root>/share/session.conf marked a config file > and does not overwrite on updates. That installer would be incorrect. Part of the purpose of moving session.conf from $sysconfdir to $datadir was to make it completely clear that it is part of the D-Bus implementation and should not be modified. However, if the sysadmin modified ${sysconfdir}/dbus-1/session-local.conf to add <auth> (which *is* considered correct), then they would run into the same problem as your scenario on downgrade. > <auth>DBUS_WINDOWS_SSPI_NTLM</auth> to share/session.conf. Now he finds out > that 1.11.5 contains a bug and downgrades to 1.11.3 without sspi patch and > restarts dbus-daemon. Please note that share/session.conf still contains > <auth>DBUS_WINDOWS_SSPI_NTLM</auth>. ... > clients can still connect to the server but not using DBUS_WINDOWS_SSPI_NTLM. No, this isn't the case. All clients would fail to connect to the server, because the dbus-daemon will not accept their DBUS_COOKIE_SHA1 authentication. Comment on attachment 125915 [details] [review] On bus startup check given auth in config file against supported mechanisms. Review of attachment 125915 [details] [review]: ----------------------------------------------------------------- Looks good with some minor style/comment tweaks. > The proposal to perform this check in _dbus_auth_set_mechanisms() has the > drawback that the check would not be performed on bus startup, only at > client side and on handling client connections at server side. Oh, you're right; DBusServer doesn't do this until new client connections come in. No need to mention it specifically in the commit message I don't think, but it's a valid reason why this is better than what I suggested. ::: dbus/dbus-auth.c @@ +3012,5 @@ > +_dbus_auth_is_supported_mechanism (DBusString *name) > +{ > + _dbus_assert (name != NULL); > + > + return find_mech(name, NULL) != 0; Minor: space before opening parenthesis Doesn't find_mech() return a pointer? If it does, "!= NULL" would be clearer. @@ +3016,5 @@ > + return find_mech(name, NULL) != 0; > +} > + > +/** > + * Return string containing all supported auth mechanisms. I'd prefer "Return a human-readable string containing..." to make it completely clear that this isn't intended to be parsed. @@ +3032,5 @@ > + while (all_mechanisms[i].mechanism != NULL) > + { > + if (i > 0) > + { > + if (!_dbus_string_append (buffer, ",")) I'd prefer ", " (comma, space) (In reply to Simon McVittie from comment #50) > > clients can still connect to the server but not using DBUS_WINDOWS_SSPI_NTLM. > > No, this isn't the case. All clients would fail to connect to the server, > because the dbus-daemon will not accept their DBUS_COOKIE_SHA1 > authentication. Because if there would be only A <auth>NOPE</auth> tag in session.conf dbus-daemon tries to use auth "NOPE" *only*, which isn't available ? If so, I do not understand why the server dump the following string in verbose log (and also to the client) "REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS" From https://dbus.freedesktop.org/doc/dbus-specification.html#auth-protocol "... Optionally, the REJECTED command has a space-separated list of available auth mechanisms as arguments. If a server ever provides a list of supported mechanisms, it must provide the same list each time it sends a REJECTED message. Clients are free to ignore all lists received after the first. " which says: I cannot auth, please use any of the following available auth mech EXTERNAL, DBUS_COOKIE_SHA1 or ANONYMOUS (In reply to Ralf Habacker from comment #52) > Because if there would be only A <auth>NOPE</auth> tag in session.conf > dbus-daemon tries to use auth "NOPE" *only*, which isn't available ? Almost that. Because if there is only <auth>NOPE</auth>, dbus-daemon will reject any authentication mechanism that isn't NOPE. > If so, I do not understand why the server dump the following string in > verbose log (and also to the client) > > "REJECTED EXTERNAL DBUS_COOKIE_SHA1 ANONYMOUS" > > From https://dbus.freedesktop.org/doc/dbus-specification.html#auth-protocol > > "... Optionally, the REJECTED command has a space-separated list of > available auth mechanisms as arguments. If a server ever provides a list of > supported mechanisms, it must provide the same list each time it sends a > REJECTED message. Clients are free to ignore all lists received after the > first. " > > which says: I cannot auth, please use any of the following available auth > mech EXTERNAL, DBUS_COOKIE_SHA1 or ANONYMOUS I think this is dbus-daemon saying that the mechanisms it *understands* are EXTERNAL, DBUS_COOKIE_SHA1 and ANONYMOUS, without considering what it will actually *accept*. The mechanisms that will work in practice are the intersection of the mechanisms the dbus-daemon implements, and the mechanisms it is configured to accept. (In reply to Simon McVittie from comment #53) > I think this is dbus-daemon saying that the mechanisms it *understands* are > EXTERNAL, DBUS_COOKIE_SHA1 and ANONYMOUS, without considering what it will > actually *accept*. Actually, this suggests a possible refinement to your (Ralf's) patch. At the moment, dbus-daemon will start up successfully regardless of <auth> configuration. Your patch changes this behaviour to only starting up successfully if *every* <auth> element is a supported mechanism. Do we actually want startup to be successful if *at least one* <auth> element is a supported mechanism? That would mean that (in a few years' time when DBUS_WINDOWS_SSPI_NTLM is sufficiently widely-supported) we would be able to write <auth>EXTERNAL</auth> <auth>DBUS_WINDOWS_SSPI_NTLM</auth> and have it work cross-platform - on Unix, EXTERNAL would be used and DBUS_WINDOWS_SSPI_NTLM would be ignored, while on Windows, the opposite would be true. (In reply to Simon McVittie from comment #53) > I think this is dbus-daemon saying that the mechanisms it *understands* are > EXTERNAL, DBUS_COOKIE_SHA1 and ANONYMOUS, without considering what it will > actually *accept*. That's arguably a bug: it would be better for it to reply with a list of what it *would* accept, i.e. the intersection of what's implemented and what's configured. Created attachment 125953 [details] [review] On bus startup check given auth in config file against supported mechanisms. With recent code starting dbus-daemon with an unsupported auth mechanism let dbus-daemon silently ignore this issue. Clients connecting to this server fails to connect without any descriptive explanation of the root cause, only the message 'Rejected client connection due to lack of memory' error is reported in dbus-daemon verbose log, which is disabled in production environments. With this patch dbus-daemon checks the supported auth mechanisms on startup and shuts down with a descriptive error message, which gives admin an immediate feedback on service startup/restart. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=96577 Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de> Created attachment 125954 [details] [review] Let dbus-daemon allow all supported auth mechanismen in case of empty auth tag in bus config file. Created attachment 125955 [details] [review] Let dbus-daemon return allowed auth mechanisms in case of rejected auth. Allowed auth mechanisms are the ones that are implemented and configured in bus config file. Comment on attachment 125955 [details] [review] Let dbus-daemon return allowed auth mechanisms in case of rejected auth. Review of attachment 125955 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-auth.c @@ +1650,4 @@ > goto nomem; > > i = 0; > + while (auth->allowed_mechs[i] != NULL) DBusServer is public API, so we can't assume that the dbus-daemon has called dbus_server_set_auth_mechanisms() - its user might be some random user of libdbus like telepathy-gabble, which runs a private DBusServer for peer-to-peer connections. (This is another reason why you were right not to do the validation in _dbus_auth_set_allowed_mechanisms(), actually - it would be a behaviour change for third-party users of DBusServer.) Even if it has done that, nothing guarantees that this allowed mechanism is supported? I don't think we want to send REJECTED EXTERNAL MISC OTHER if the unsupported MISC and OTHER mechanisms are configured to be allowed - they'll never work, so we shouldn't suggest that they would be. So I would prefer something like this, which I think addresses both of those: while (all_mechanisms[i].mechanism != NULL) { /* skip mechanisms that aren't allowed */ if (auth->allowed_mechs != NULL && !string_array_contains (auth->allowed_mechs, all_mechanisms[i].mechanism) continue; ... append the space and the name as before ... } Comment on attachment 125954 [details] [review] Let dbus-daemon allow all supported auth mechanismen in case of empty auth tag in bus config file. Review of attachment 125954 [details] [review]: ----------------------------------------------------------------- > Let dbus-daemon allow all supported auth mechanismen If it has become necessary to do this, where previously it wasn't, then you've broken libdbus' public API. I think the patch I just reviewed is the one with the relevant API break. Comment on attachment 125953 [details] [review] On bus startup check given auth in config file against supported mechanisms. Review of attachment 125953 [details] [review]: ----------------------------------------------------------------- ::: bus/bus.c @@ +410,5 @@ > while (link != NULL) > { > + DBusString name; > + _dbus_string_init_const (&name, link->data); > + if (!_dbus_auth_is_supported_mechanism (&name)) Do we really want to bail out on <auth>NO</auth> <!-- unsupported --> <auth>DBUS_COOKIE_SHA1</auth> <!-- supported --> <auth>NOPE</auth> <!-- unsupported --> or would it be better to continue startup as long as *at least one* allowed mechanism is supported? (See previous comment for rationale) This looks valid as an implementation of insisting that every <auth> must represent a supported mechanism, though. (In reply to Simon McVittie from comment #61) > Comment on attachment 125953 [details] [review] [review] > > or would it be better to continue startup as long as *at least one* allowed > mechanism is supported? (See previous comment for rationale) > > This looks valid as an implementation of insisting that every <auth> must > represent a supported mechanism, though. From the admin point of view I'm happy to be noticed about misconfigurations as early as possible. (In reply to LRN from comment #49) > My code does match that overview. My resume is that recent dbus code contains support to identify clients (getting sid) for local hosts only by inspecting the internal tcp tables. With your patch there is now a secure and extendable way for the dbus-daemon to identify clients connecting from local *and* remote hosts based on the Microsoft equivalent of the GSS-API (Generic Security Service Application Program Interface, RFC 2743). [1] > If you think it's a good idea, i could try to add these to the patch. For > example, hardcode (and check for) the expectation that the exchange is: > client (type1) -> server (type2)-> client (type3, done) -> server (done) > (it's probably not a good idea to dig into messages to check their contents, agreed > but i can easily keep track of the number of messages being exchanged) Adding this would make the implementation more secure. [2] > Security-wise, that feels somewhat weak. It would be better > to have a developer with W32 security experience review that, > but i have no idea where to find one or how to convince > one to do code review for a free software project. We can mark this stuff as experimental in the readme and should add a notice that we would like to get some security review is possible. > > > 2. correct handling of api return values > > If you mean SSPI API, then that is much easier. MSDN lists very specific > return values for functions like InitializeSecurityContext(), and my code > does check for them. So this is okay. > > 3. correct usage of libdbus functions > > I had hoped that this was fixed in 3rd version of the patch. looks good. Just one question related to [1]: Can you say what would be required to implement in dbus-daemon to use this new auth mech for a system bus on windows ? As far as I can see there is now left over to add - [2] if you like to implement - to complete the remaining bus setup related patches and - to update README.win with a note about the new feature and some config hints (In reply to Ralf Habacker from comment #63) > With your patch there is now a secure and extendable way for the dbus-daemon > to identify clients connecting from local *and* remote hosts Please, no remote. NTLM is not considered secure (IIRC, authentication process can be intercepted and duplicated, to allow an attacker to authenticate as the client to some other server) when used over the network. For it to be secure you have to encrypt (and authenticate, to avoid MitM attacks) the connection by some other means before you start the NTLM authentication process. At least, that is what the Internet says. > > > 3. correct usage of libdbus functions > > > > I had hoped that this was fixed in 3rd version of the patch. > looks good. > > Just one question related to [1]: Can you say what would be required to > implement in dbus-daemon to use this new auth mech for a system bus on > windows ? What are the differences between session and system buses? Also, what is "system bus" on Windows? For example, on Debian, AFAIU, system bus starts up early and can be used, for example, by systemd. How does that translate to Windows? A service? Does dbus already support being a system bus on Windows (and your question is only about the new auth mechanism), or does it not (and your question is about adding that support)? On an assumption that system bus is a service, i see no obstacles for using SSPI-NTLM with it. The fact that there's usually an integrity level gap between services and user processes shouldn't affect sockets (which is the only way to use DBus as, AFAIU, dbus doesn't support pipes on Windows), and only the client is authenticated (so there won't be situations where the client freaks out when it sees that the server authenticates as SYSTEM or NETWORK SERVICE or some other privileged account, because the server does not authenticate to the client). Successful authentication results in a user SID being available, the existing external authentication mechanism provides the same thing, so no changes here either. > > As far as I can see there is now left over to add > - [2] if you like to implement One wrinkle i foresee with this is that i can only test on actual SSPI implementation in Windows. I.e. i have no idea how to make SSPI "misbehave", to test that the message number check actually catches these cases. Although, if i make it very simple...H-m-m-m...For example, set two counters to 0 on initialization of the auth context, then increase them by 1 for each message sent and received. Once authentication is about to be finished, check that these counters have appropriate values. That probably can't go wrong (famous last words...). > - to complete the remaining bus setup related patches Um...? > - to update README.win with a note about the new feature and some config > hints Is that a Note To Self? :) (In reply to LRN from comment #64) > Please, no remote. NTLM is not considered secure Indeed. Remote D-Bus is not secure either: it's just TCP, with no integrity or confidentiality protection. I have been very tempted to make dbus-daemon refuse to do TCP at all, except for 127.0.0.1 or ::1 on Windows. If you want to use remote D-Bus, tunnel it over TLS or SSH or something (on Unix, some systemd tools use D-Bus over ssh). > What are the differences between session and system buses? One session bus for each user's high-level "session" (whatever that means, can vary depending on platform) connects that user's apps and services together, for example some GNOME or KDE app talking to Telepathy, Rygel or Tracker. It is implemented as a dbus-daemon running as your normal uid. On Windows, I would assume that this means you get at most one session bus per login session (starts when you type your name and password, terminates when you log out), implemented as a dbus-daemon.exe running as your normal SID. On Unix, there is also a system bus, which connects user applications (like GNOME or KDE apps) to system services (like NetworkManager and PackageKit), and is implemented as a dbus-daemon running as a system user with a name like _dbus or messagebus. The canonical reference is <https://dbus.freedesktop.org/doc/dbus-specification.html#message-bus-types> in the spec. > Also, what is "system bus" on Windows? Nonexistent. The system bus is not particularly useful on Windows, because D-Bus is not part of the Windows OS, so Microsoft's OS-wide services all communicate with user apps in some way that does not involve D-Bus; this basically removes the system bus' reason to exist. I also don't think the Windows D-Bus port is really robust enough to be used as a security boundary: it contains sprintf into fixed-size buffers, a weird form of EXTERNAL authentication, etc. which are not present in the equivalent Unix code. If a system bus on Windows was supported, then I think you are correct to say that it would be a Windows service. (Windows services are the things that run as LOCAL_SYSTEM or some other non-human user, right?) As a result of the lack of support for a system bus on Windows, system.conf and the system bus test-cases assume Unix throughout, so they would all need fixing. (In reply to LRN from comment #64) > (In reply to Ralf Habacker from comment #63) > > With your patch there is now a secure and extendable way for the dbus-daemon > > to identify clients connecting from local *and* remote hosts > > Please, no remote. NTLM is not considered secure (IIRC, authentication > process can be intercepted and duplicated, to allow an attacker to > authenticate as the client to some other server) when used over the network. > For it to be secure you have to encrypt (and authenticate, to avoid MitM > attacks) the connection by some other means before you start the NTLM > authentication process. At least, that is what the Internet says. So it would be better to use the "Negotiate" security package instead as mentioned at https://en.wikipedia.org/wiki/NT_LAN_Manager#Availability_and_use_of_NTLM [1] > > > > > 3. correct usage of libdbus functions > > > > > > I had hoped that this was fixed in 3rd version of the patch. > > looks good. > > > > As far as I can see there is now left over to add > > - [2] if you like to implement > > One wrinkle i foresee with this is that i can only test on actual SSPI > implementation in Windows. I.e. i have no idea how to make SSPI "misbehave", > to test that the message number check actually catches these cases. May be this helps http://rc.quest.com/topics/authtest/ > > > - to complete the remaining bus setup related patches > > Um...? Simon and I need to resume and finish the outstanding tasks, > > > - to update README.win with a note about the new feature and some config > > hints > > Is that a Note To Self? :) yes and no. It is important to specific the scope of this implementation. We need to document the requirements and limitations. If we choose to add [1] we need to take a look at the kerberos stuff on windows and how to use/configure it, (In reply to Ralf Habacker from comment #66) > (In reply to LRN from comment #64) > > (In reply to Ralf Habacker from comment #63) > > > With your patch there is now a secure and extendable way for the dbus-daemon > > > to identify clients connecting from local *and* remote hosts > > > > Please, no remote. NTLM is not considered secure (IIRC, authentication > > process can be intercepted and duplicated, to allow an attacker to > > authenticate as the client to some other server) when used over the network. > > For it to be secure you have to encrypt (and authenticate, to avoid MitM > > attacks) the connection by some other means before you start the NTLM > > authentication process. At least, that is what the Internet says. > > So it would be better to use the "Negotiate" security package instead as > mentioned at > https://en.wikipedia.org/wiki/NT_LAN_Manager#Availability_and_use_of_NTLM [1] No. "Negotiate" implies that NTLM or Kerberos (whichever is available, preferring Kerberos, as more secure) are used, transparently for the caller. However, Kerberos is more needy than NTLM (i.e. it is more picky about the argumments to SSPI functions, whereas for NTLM it's enough to just pass NULL for most of them; so much for transparency...). My current code will not work if you just change SSP provider from "NTLM" to "Kerberos" (thus changing it to "Negotiate" will mean that "NTLM" will be effectively used). Probably. I haven't tried. Also, Kerberos is not a Windows-specific authentication mechanism. If DBus is to support Kerberos authentication, it would be better to do it explicitly, as a separate authentication mechanism. Adding Kerberos support to DBus (and dbus) is completely out of scope of this bug. > > > > > > > > 3. correct usage of libdbus functions > > > > > > > > I had hoped that this was fixed in 3rd version of the patch. > > > looks good. > > > > > > As far as I can see there is now left over to add > > > - [2] if you like to implement > > > > One wrinkle i foresee with this is that i can only test on actual SSPI > > implementation in Windows. I.e. i have no idea how to make SSPI "misbehave", > > to test that the message number check actually catches these cases. > > May be this helps http://rc.quest.com/topics/authtest/ Will check it out. (In reply to LRN from comment #67) > (In reply to Ralf Habacker from comment #66) > > (In reply to LRN from comment #64) > > > > > > One wrinkle i foresee with this is that i can only test on actual SSPI > > > implementation in Windows. I.e. i have no idea how to make SSPI "misbehave", > > > to test that the message number check actually catches these cases. > > > > May be this helps http://rc.quest.com/topics/authtest/ > > Will check it out. Nah, it's rubbish. Windows client and server test apps available there are just simple applications that use SSPI to communicate, and allow SSPI functions arguments to be changed by supplying different commandline arguments without recompiling the programs (paradoxically, because the only way to get these binaries is to *compile* them yourself, rendering the point of commandline adjustibility moot). What i meant by "making SSPI misbehave" is changing the number of messages that SSPI needs to exchange to authenticate (thus testing my hypothetical message-counting code). Supplying wrong arguments to SSPI function won't achieve that. Created attachment 126376 [details] [review] Support SSPI NTLM authentication mechanism v5: * Count messages. The implementation will ensure that the client will not send more than two SSPI buffers, and that server won't send more than one SSPI buffer. Three messages in total. This is consistent with NTLM protocol spec. * Revert nitpicks concerning this cast: (void *) _dbus_string_get_const_data (data) the _const_ part is necessary to avoid a warning about getting non-const data buffer pointer from const DBusString *data. Added comments to clarify why the cast is there. CAVEAT EMPTOR: While testing the reaction either paty had to the number of messages being wrong (by lowering the number of expected messages below 2 (client) and 1 (server)), i've discovered erratic behaviour in which data was sent to the function repeatedly, despite causing errors. Careful studying of caller code and other mechanism finally revealed that data-processing functions in authentication mechanisms must only return FALSE on OutOfMemory errors, and return TRUE in all other cases. While adjusting my code to do so, i've realized that my code is not OOM-reentrant. That is, each call to a data function (either client or server one) will perform multiple actions. For example, server data processor will: 1) bump inbound_message_count by 1 2) feed the data to SSPI and get the next challenge in return 3) send the challenge back to the client, if it isn't 0-long 4) do a sanity check (challenge must not be 0-long if authentication is not completed) 5) if authentication is completed, fetch credentials from the token 6) if authentication is completed, send OK Some of these actions can fail due to OOM, in which case the function must return FALSE, dbus will free up some memory (somehow), then *call the function again with the same arguments*. So, if the function did what it had to, and failed, for example, at step 3 (OOM while sending), it will be later called again, and will, again, try to feed the same data to SSPI (step 2). SSPI, obviously, will error out on this. Even doing the first action (bumping incoming message count) too many times will result in erroring out (since message count check will fail later on). The only solution for this that comes to mind is to maintain an extra state field, which data functions should use to remember which actions were taken. That way the data functions will avoid erroneously repeating some of the steps that were already done, and will instead continue from the spot at which they were interrupted by an OOM condition. The big question is: should we care that much about OOM on Windows, given that dbus does not work as a system bus there? Because this only affects OOM, and *current* reaction to OOM is unnecessarily harsh (error out immediately instead of freeing up some memory and retrying), but is not pathological. Any progress on this? (In reply to LRN from comment #69) > Created attachment 126376 [details] [review] [review] > Support SSPI NTLM authentication mechanism > > v5: It needs a rebase In file included from /home/ralf/src/dbus-2/dbus/dbus-auth.c:25:0: /home/ralf/src/dbus-2/dbus/dbus-auth.h:100:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes] char ** _dbus_auth_get_supported_mechanisms_list (); > The only solution for this that comes to mind is to maintain an extra state > field, which data functions should use to remember which actions were taken. > That way the data functions will avoid erroneously repeating some of the steps > that were already done, and will instead continue from the spot at which they > were interrupted by an OOM condition. Makes sense. The implementation will be more robust. > > The big question is: should we care that much about OOM on Windows, given > that dbus does not work as a system bus there? There are request to run dbus-daemon as service which is a similar case. > Because this only affects OOM, > and *current* reaction to OOM is unnecessarily harsh (error out immediately > instead of freeing up some memory and retrying) If you would support that in the windows code does main dbus code support this too or is main dbus code irrelevant for that case ? (In reply to Ralf Habacker from comment #71) > (In reply to LRN from comment #69) > > Created attachment 126376 [details] [review] [review] [review] > > Support SSPI NTLM authentication mechanism > > > > v5: > It needs a rebase Will do. > > The only solution for this that comes to mind is to maintain an extra state > > field, which data functions should use to remember which actions were taken. > > That way the data functions will avoid erroneously repeating some of the steps > > that were already done, and will instead continue from the spot at which they > > were interrupted by an OOM condition. > Makes sense. The implementation will be more robust. > > > > The big question is: should we care that much about OOM on Windows, given > > that dbus does not work as a system bus there? > There are request to run dbus-daemon as service which is a similar case. Is that a "yes, Windows version of dbus must handle OOM 100% correctly, your patch is good as far as OOM handling goes" or "no, we don't care much about handling OOM on Windows, your patch needs work to implement OOM reentrancy"? > > Because this only affects OOM, > > and *current* reaction to OOM is unnecessarily harsh (error out immediately > > instead of freeing up some memory and retrying) > If you would support that in the windows code does main dbus code support > this too or is main dbus code irrelevant for that case ? Irrelevant. Well, the dbus code that frees up memory on OOM *might (or might not) be* broken or unimplemented on Windows (i haven't looked, and until somebody looks, the code in a state of quantum superposition and Windows OOM handling exists and works, and doesn't exist and doesn't work at the same time), but that is going to be someone else's problem. Created attachment 127621 [details] [review] Support SSPI NTLM authentication mechanism v6: * Correctly handle OOM in the code. This means checking all functions that can allocate memory and returning FALSE when they do fail. This also means correctly re-starting authentication from the right place in the code when dbus calls the same function again later on in response to an OOM error. * Documentation fixes for the new code Note that there are still some unhandled OOMs in sysdeps-win.c, but since i haven't touched these parts of the code at all, i left them as they are. They can be fixed separately later on. Also, SSP will most likely allocate some memory internally, and can conceivably fail due to OOM. However, dbus does not detect that as an OOM, just as SSP failing. It should probably be possible to check for the return value having the SEC_E_INSUFFICIENT_MEMORY code, but that still doesn't tell us whether we can retry the call later, with more memory available. MSDN says nothing on the subject. So i didn't touch that for now. (In reply to Ralf Habacker from comment #71) > (In reply to LRN from comment #69) > > Created attachment 126376 [details] [review] [review] [review] > > Support SSPI NTLM authentication mechanism > > > > v5: > It needs a rebase > > In file included from /home/ralf/src/dbus-2/dbus/dbus-auth.c:25:0: > /home/ralf/src/dbus-2/dbus/dbus-auth.h:100:1: warning: function declaration > isn’t a prototype [-Wstrict-prototypes] > char ** _dbus_auth_get_supported_mechanisms_list (); > I've rebased the v6 patch (above) on top of git master, and it compiled just fine. I've tried to find the _dbus_auth_get_supported_mechanisms_list() in my code and failed to do so. That was when i realized that you were referring not to my patch, but to attachment 125954 [details] [review] by Ralf Habacker. You should probably ask him to rebase his patch. Created attachment 127685 [details] [review] Let dbus-daemon allow all supported auth mechanismen in case of empty auth tag in bus config file. (In reply to LRN from comment #74) > you were referring not to my patch, but to attachment 125954 [details] [review] fixed,thanks for this pointer (In reply to LRN from comment #73) > Created attachment 127621 [details] [review] [review] > Support SSPI NTLM authentication mechanism > > v6: > * Correctly handle OOM in the code. This means checking all > functions that can allocate memory and returning FALSE when > they do fail. > This also means correctly re-starting authentication from > the right place in the code when dbus calls the same function > again later on in response to an OOM error. > * Documentation fixes for the new code Because there is no really test app out there (I am assuming this from comment #68 and my own search) we need to add some test code to verify that this is working as expected also for the future. Looking at the current available test applications there is manual-authz. But it does not real connect, so it is unclear it would be better to extend this or another test app. > Note that there are still some unhandled OOMs in sysdeps-win.c, > but since i haven't touched these parts of the code at all, i left > them as they are. They can be fixed separately later on. okay > Also, SSP will most likely allocate some memory internally, and > can conceivably fail due to OOM. However, dbus does not detect that > as an OOM, just as SSP failing. It should probably be possible to > check for the return value having the SEC_E_INSUFFICIENT_MEMORY code, which may have also different reason see http://www.winterdom.com/development/2009/07/16/initializesecuritycontext-and-sec_e_insufficient_memory.html > but that still doesn't tell us whether we can retry the call later, > with more memory available. MSDN says nothing on the subject. So i > didn't touch that for now. okay, maybe a note in the code may help to remember. (In reply to Ralf Habacker from comment #77) > Because there is no really test app out there (I am assuming this from > comment #68 and my own search) we need to add some test code to verify that > this is working as expected also for the future. Looking at the current > available test applications there is manual-authz. But it does not real > connect, so it is unclear it would be better to extend this or another test > app. @Simon: Any hints or preferences for this ? (In reply to Ralf Habacker from comment #78) > Looking at the current > available test applications there is manual-authz. But it does not real > connect For usage instructions for manual-authz, see: https://bugs.freedesktop.org/show_bug.cgi?id=39720#c93 It just listens, and you are expected to connect using gdbus or dbus-send or something - that's why it's a manual test. (I'd be happy to review a patch that adds that information in comments.) (In reply to Ralf Habacker from comment #78) > @Simon: Any hints or preferences for this ? Sorry, I don't understand Windows APIs in detail, and I don't have a great deal of time available for D-Bus right now. I'll try to get to this when I can. Any news on this? (In reply to LRN from comment #80) > Any news on this? As mentioned in comment 79 I think the next step is to add a related auth test to the set of dbus unit tests named 'test-auth' or similar. It should call the required dbus api functions to provide and test supported auth methods. (In reply to Ralf Habacker from comment #81) > (In reply to LRN from comment #80) > > Any news on this? > > As mentioned in comment 79 I think the next step is to add a related auth > test to the set of dbus unit tests named 'test-auth' or similar. It should > call the required dbus api functions to provide and test supported auth > methods. I copied some code from manual-authz.c #include <config.h> #include <glib.h> #include <dbus/dbus.h> #include <stdlib.h> #ifdef G_OS_UNIX #include <unistd.h> #include <sys/types.h> #endif #include "test-utils.h" typedef struct { DBusError e; TestMainContext *ctx; DBusServer *server; } Fixture; static void oom (void) G_GNUC_NORETURN; static void oom (void) { g_error ("out of memory"); abort (); } static void assert_no_error (const DBusError *e) { if (G_UNLIKELY (dbus_error_is_set (e))) g_error ("expected success but got error: %s: %s", e->name, e->message); } static DBusHandlerResult server_message_cb (DBusConnection *conn, DBusMessage *message, void *data) { if (dbus_message_is_signal (message, DBUS_INTERFACE_LOCAL, "Disconnected")) { dbus_connection_unref (conn); return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } if (dbus_message_get_type (message) == DBUS_MESSAGE_TYPE_METHOD_CALL) { DBusMessage *reply = dbus_message_new_method_return (message); const char *hello = "Hello, world!"; unsigned long uid; char *sid; if (dbus_connection_get_unix_user (conn, &uid)) { g_message ("message from uid %lu", uid); } else if (dbus_connection_get_windows_user (conn, &sid)) { if (sid == NULL) oom (); g_message ("message from sid \"%s\"", sid); dbus_free (sid); } else if (dbus_connection_get_is_anonymous (conn)) { g_message ("message from Anonymous"); } else { g_message ("message from ... someone?"); } if (reply == NULL) oom (); if (!dbus_message_append_args (reply, DBUS_TYPE_STRING, &hello, DBUS_TYPE_INVALID)) oom (); if (!dbus_connection_send (conn, reply, NULL)) oom (); dbus_message_unref (reply); return DBUS_HANDLER_RESULT_HANDLED; } return DBUS_HANDLER_RESULT_NOT_YET_HANDLED; } static dbus_bool_t same_uid_win_func (DBusConnection *conn, const char *sid, void *data) { g_message ("checking whether Windows user \"%s\" owns this process", sid); g_message ("Stub implementation consistent with dbus-sysdeps-util-win: " "assume they do"); return TRUE; } static void new_conn_cb (DBusServer *server, DBusConnection *conn, void *data) { Fixture *f = data; dbus_connection_ref (conn); test_connection_setup (f->ctx, conn); if (!dbus_connection_add_filter (conn, server_message_cb, f, NULL)) oom (); } static void setup (Fixture *f, const gchar *listen_addr, const gchar *auth_mech) { char *auth_mechs[] = { NULL, NULL }; char *connect_addr; DBusConnection *connection; DBusError error; auth_mechs[0] = auth_mech; dbus_error_init (&error); f->server = dbus_server_listen (listen_addr, &f->e); assert_no_error (&f->e); g_assert (f->server != NULL); dbus_server_set_new_connection_function (f->server, new_conn_cb, f, NULL); dbus_server_set_auth_mechanisms (f->server, auth_mechs); test_server_setup (f->ctx, f->server); connect_addr = dbus_server_get_address (f->server); g_message ("server:\n%s", connect_addr); connection = dbus_connection_open_private (connect_addr, &error); if (connection == NULL) fprintf(stderr, "%s %s\n", error.name, error.message); else fprintf(stderr, "connection okay\n"); dbus_server_disconnect (f->server); dbus_connection_close (connection); dbus_free (connect_addr); } int main (int argc, char **argv) { Fixture f = { DBUS_ERROR_INIT, test_main_context_get () }; if (argc >= 2) setup (&f, argv[1], "DBUS_WINDOWS_SSPI_NTLM"); else setup (&f, "tcp:host=127.0.0.1", "DBUS_WINDOWS_SSPI_NTLM"); for (;;) test_main_context_iterate (f.ctx, TRUE); /* never returns */ return 0; } and get ralf@bramsche:~/src/dbus-2-cmake-cross-x86-build> bin/test-auth.exe ** Message: server: tcp:host=127.0.0.1,port=44632,guid=8d5ffbf2800688048e83d9195883c493 fixme:netapi32:NetWkstaUserGetInfo Level 1 processing is partially implemented fixme:advapi:LsaOpenPolicy ((null),0x66f788,0x00000001,0x66f780) stub fixme:advapi:LsaClose (0xcafe) stub connection okay ralf@bramsche:~/src/dbus-2-cmake-cross-x86-build> DBUS_VERBOSE=1 bin/test-auth.exe 53: 0x0036: 1485030554.648618 [dbus/dbus-memory.c(262):_dbus_decrement_fail_alloc_counter] TODO: memory allocation testing errors disabled for now 53: 0x0036: 1485030554.648776 [dbus/dbus-socket-set-poll.c(98):_dbus_socket_set_poll_new] new socket set at 00253BC0 53: 0x0036: 1485030554.650694 [dbus/dbus-server.c(165):_dbus_server_init_base] Initialized server on address tcp:host=127.0.0.1,port=33155,guid=3a4bfcfcb876af92f17121e05883c49a 53: 0x0036: 1485030554.650982 [dbus/dbus-watch.c(318):_dbus_watch_list_set_functions] Adding a read watch on fd 176 using newly-set add watch function 53: 0x0036: 1485030554.651116 [dbus/dbus-socket-set-poll.c(146):socket_set_poll_add] before adding fd 176 to 00253BC0, 0 en/0 res/1 alloc ** Message: server: tcp:host=127.0.0.1,port=33155,guid=3a4bfcfcb876af92f17121e05883c49a 53: 0x0036: 1485030554.653444 [dbus/dbus-connection.c(1868):_dbus_connection_open_internal] opening private connection to: tcp:host=127.0.0.1,port=33155,guid=3a4bfcfcb876af92f17121e05883c49a 53: 0x0036: 1485030554.654039 [dbus/dbus-transport-socket.c(1404):_dbus_transport_new_for_tcp_socket] Successfully connected to tcp socket 127.0.0.1:33155 fixme:netapi32:NetWkstaUserGetInfo Level 1 processing is partially implemented fixme:advapi:LsaOpenPolicy ((null),0x66f788,0x00000001,0x66f780) stub fixme:advapi:LsaClose (0xcafe) stub 53: 0x0036: 1485030554.670741 [dbus/dbus-auth.c(2453):goto_state] client: going from state NeedSendAuth to state WaitingForData 53: 0x0036: 1485030554.670977 [dbus/dbus-transport.c(204):_dbus_transport_init_base] Initialized transport on address tcp:host=127.0.0.1,port=33155 53: 0x0036: 1485030554.671246 [dbus/dbus-connection.c(1360):_dbus_connection_new_for_transport] LOCK 53: 0x0036: 1485030554.671407 [dbus/dbus-transport-socket.c(181):check_read_watch] fd = 184 53: 0x0036: 1485030554.671515 [dbus/dbus-transport-socket.c(227):check_read_watch] setting read watch enabled = 0 53: 0x0036: 1485030554.671619 [dbus/dbus-transport-socket.c(165):check_write_watch] check_write_watch(): needed = 1 on connection 002547F0 watch 002543A0 fd = 184 outgoing messages exist 0 53: 0x0036: 1485030554.671724 [dbus/dbus-connection.c(406):_dbus_connection_unlock] UNLOCK connection okay 53: 0x0036: 1485030554.672369 [dbus/dbus-watch.c(424):_dbus_watch_list_remove_watch] Removing watch on fd 176 53: 0x0036: 1485030554.672496 [dbus/dbus-socket-set-poll.c(221):socket_set_poll_remove] after removing fd 176 from 00253BC0, 0 en/0 res/1 alloc 53: 0x0036: 1485030554.672611 [dbus/dbus-watch.c(686):dbus_watch_set_data] Setting watch fd 4294967295 data to data = 00000000 function = 00000000 from data = 00000000 function = 00000000 53: 0x0036: 1485030554.672789 [dbus/dbus-sysdeps-win.c(512):_dbus_close_socket] socket=176, 53: 0x0036: 1485030554.672904 [dbus/dbus-connection.c(2937):dbus_connection_close] LOCK 53: 0x0036: 1485030554.673036 [dbus/dbus-connection.c(2861):_dbus_connection_close_possibly_shared_and_unlock] Disconnecting 002547F0 53: 0x0036: 1485030554.673131 [dbus/dbus-transport.c(504):_dbus_transport_disconnect] start 53: 0x0036: 1485030554.673224 [dbus/dbus-transport-socket.c(1028):socket_disconnect] 53: 0x0036: 1485030554.673315 [dbus/dbus-transport-socket.c(76):free_watches] start 53: 0x0036: 1485030554.673467 [dbus/dbus-watch.c(686):dbus_watch_set_data] Setting watch fd 4294967295 data to data = 00000000 function = 00000000 from data = 00000000 function = 00000000 53: 0x0036: 1485030554.673602 [dbus/dbus-watch.c(686):dbus_watch_set_data] Setting watch fd 4294967295 data to data = 00000000 function = 00000000 from data = 00000000 function = 00000000 53: 0x0036: 1485030554.673698 [dbus/dbus-transport-socket.c(98):free_watches] end 53: 0x0036: 1485030554.673843 [dbus/dbus-sysdeps-win.c(512):_dbus_close_socket] socket=184, 53: 0x0036: 1485030554.673958 [dbus/dbus-transport.c(515):_dbus_transport_disconnect] end 53: 0x0036: 1485030554.674074 [dbus/dbus-connection.c(4268):_dbus_connection_get_dispatch_status_unlocked] dispatch status = complete is_connected = 0 53: 0x0036: 1485030554.674199 [dbus/dbus-connection.c(4230):notify_disconnected_and_dispatch_complete_unlocked] Sending disconnect message 53: 0x0036: 1485030554.674303 [dbus/dbus-connection.c(560):_dbus_connection_queue_synthesized_message_link] Synthesized message 002548D8 added to incoming queue 002547F0, 1 incoming 53: 0x0036: 1485030554.674408 [dbus/dbus-connection.c(406):_dbus_connection_unlock] UNLOCK Looks that that there is something missing to trigger SSPI auth. Created attachment 129096 [details] [review] Add a test-auth test program I've looked at other tests, and they seem to be wroking differently, so i've cobbled together this test. Though i'm not sure what exactly is that you want from it, so it might be insufficient for some uses. In this form it can only test that authentication is successful or not, and the only condition that i have found how to change is the config file. Thus, i'm not sure it can, for example, send damaged data, or check user credentials, or bail in the middle of an auth process, as all that stuff seems to be handled internally by dbus test helper API. Comment on attachment 125953 [details] [review] On bus startup check given auth in config file against supported mechanisms. Review of attachment 125953 [details] [review]: ----------------------------------------------------------------- Looking at this again (prompted by Bug #99512) I think we should have this pseudocode: at_least_one_auth = FALSE foreach mechanism if it's a supported mechanism at_least_one_auth = TRUE if (!at_least_one_auth) fail That would mean that your example on Bug #99512 would still fail (because its only allowed auth mechanism is <auth>INVALID</auth> which is not supported), but my example from the earlier review would succeed. (In reply to LRN from comment #83) > Thus, i'm not sure it can, for example, > send damaged data, or check user credentials, or bail in the middle of an > auth process, as all that stuff seems to be handled internally by dbus test > helper API. Correct, libdbus does not support deliberately getting the authentication protocol wrong. You would have to connect to the socket "by hand" using a GSocket or similar, and implement a subset of the D-Bus authentication protocol (it's text-based after the initial '\0', see the D-Bus Specification). (In reply to Simon McVittie from comment #84) > That would mean that your example on Bug #99512 would still fail (because > its only allowed auth mechanism is <auth>INVALID</auth> which is not > supported), but my example from the earlier review would succeed. It would also mean that, if/when SSPI is production-ready, we could ship a configuration file with <auth>EXTERNAL</auth> <auth>DBUS_WINDOWS_SSPI</auth> on all platforms, and it would be equivalent to saying <auth>EXTERNAL</auth> on Unix and <auth>DBUS_WINDOWS_SSPI</auth> on Windows. (In reply to Simon McVittie from comment #86) > (In reply to Simon McVittie from comment #84) > > That would mean that your example on Bug #99512 would still fail (because > > its only allowed auth mechanism is <auth>INVALID</auth> which is not > > supported), but my example from the earlier review would succeed. > > It would also mean that, if/when SSPI is production-ready, we could ship a > configuration file with > > <auth>EXTERNAL</auth> > <auth>DBUS_WINDOWS_SSPI</auth> > > on all platforms, and it would be equivalent to saying <auth>EXTERNAL</auth> > on Unix and <auth>DBUS_WINDOWS_SSPI</auth> on Windows. Minor correction: it would be equivalent to saying <auth>EXTERNAL</auth> and <auth>DBUS_WINDOWS_SSPI</auth> on Windows. IIRC, Windows dbus implementation has a hacky external authentication mechanism (it reads system socket table to find the process ID of the process that owns a particular socket, which allows it to deduce that the process that is connected to it and infer the user as the owner of that process). (In reply to LRN from comment #87) > IIRC, Windows dbus implementation > has a hacky external authentication mechanism (it reads system socket table > to find the process ID of the process that owns a particular socket, which > allows it to deduce that the process that is connected to it and infer the > user as the owner of that process). I was assuming we'd remove that hack once DBUS_WINDOWS_SSPI was release-quality. Created attachment 129246 [details] [review] Let dbus-daemon return allowed auth mechanisms in case of rejected auth. Allowed auth mechanisms are the ones that are implemented and configured in bus config file. Comment on attachment 129246 [details] [review] Let dbus-daemon return allowed auth mechanisms in case of rejected auth. moved to bug 99621 Comment on attachment 125953 [details] [review] On bus startup check given auth in config file against supported mechanisms. moved to bug 99622 Comment on attachment 127685 [details] [review] Let dbus-daemon allow all supported auth mechanismen in case of empty auth tag in bus config file. On moving this patch to a different bug I recognized that DBusAuth is already designed to support any auth in case struct member allowed_mechs is zero, which looks to make this patch obsolate except there is a subtile reason I was faced on testing sspi implementation. Need to retest this later. Created attachment 129328 [details] [review] Add sspi test to test-auth. - rebased on basic test-auth. Created attachment 129336 [details] [review] Add sspi test to test-auth test case. - autotools compile fix (In reply to Simon McVittie from comment #86) > It would also mean that, if/when SSPI is production-ready, we could ship a > configuration file with > > <auth>EXTERNAL</auth> > <auth>DBUS_WINDOWS_SSPI</auth> > > on all platforms, and it would be equivalent to saying <auth>EXTERNAL</auth> > on Unix and <auth>DBUS_WINDOWS_SSPI</auth> on Windows. Why should it be useful to add DBUS_WINDOWS_SSPI in a unix only daemon configuration ? It would imply that DBUS_WINDOWS_SSPI would be available on unix which is not the case. (In reply to Simon McVittie from comment #85) > Correct, libdbus does not support deliberately getting the authentication > protocol wrong. > > You would have to connect to the socket "by hand" using a GSocket or > similar, and implement a subset of the D-Bus authentication protocol (it's > text-based after the initial '\0', see the D-Bus Specification). Aren't there any internal hooks in the auth processing code which could be used to manipulate what to send or has been received ? (In reply to Ralf Habacker from comment #95) > Why should it be useful to add DBUS_WINDOWS_SSPI in a unix only daemon > configuration ? If we were able to configure authentication mechanisms the way I suggested, it wouldn't necessarily have to be Unix-only. Reducing differences between the supported configurations seems like a good thing in general. (In reply to Ralf Habacker from comment #96) > Aren't there any internal hooks in the auth processing code which could be > used to manipulate what to send or has been received ? Not that I know of, and to be honest I'd prefer not to add new ones (a bug in that part of the code would be really bad). (In reply to Simon McVittie from comment #97) > (In reply to Ralf Habacker from comment #95) > > Why should it be useful to add DBUS_WINDOWS_SSPI in a unix only daemon > > configuration ? > > If we were able to configure authentication mechanisms the way I suggested, > it wouldn't necessarily have to be Unix-only. Reducing differences between > the supported configurations seems like a good thing in general. There is no need to change anything with recent source. Beside several platform specific default settings like session bus address we already have DBUS_SESSION_CONF_MAYBE_AUTH_EXTERNAL for exactly that purpose. It already have different platform related values. Configuring cmake builds on Windows with -DDBUS_SESSION_CONF_MAYBE_AUTH_EXTERNAL=DBUS_WINDOWS_SSPI_NTLM creates a session.conf using sspi only. Autotools on Windows defines DBUS_SESSION_CONF_MAYBE_AUTH_EXTERNAL as empty and needs a patch to provide a similar command line parameter. A remaining simple patch to set sspi as default auth on windows in case sspi may finished this support. Decided to give the "simple dbus auth SSPI client that deliberately sends bad data" thing a go. Turns out, to avoid re-implementing the *whole* dbus API, i might need these internal dbus functions: _dbus_transport_open _dbus_transport_disconnect _dbus_transport_unref _dbus_string_init_preallocated _dbus_transport_get_socket_fd How should i proceed? Put these into separate helper library and link to the testcase? Duplicate the code? I can possibly make do without the string function, but the transport API is another matter. Currently my testcase establishes a transport connection (using an extremely simplified copy of DBusConnection functions) using the transport API, then grabs the socket FD from the transport connection and just does a sequential send/recv until an assert fails or it receives OK from the server (the testcase is client-side only; the server is a normal dbus test daemon; i think i can also deliberately mangle server data after it's received, so i don't really need any server-side code). I've looked at the transport API, and it seems pretty extensive, and i wouldn't want to re-implement all the minutae (such as handling different addresses and such). Silly me, that's what libdbus-internal.la is for! Created attachment 132690 [details] [review] DBUS_WINDOWS_SSPI_NTLM custom test This is a simple socket client that connects to a dbus test server daemon and tries to authenticate using the DBUS_WINDOWS_SSPI_NTLM mechanism. The code that does authentication is completely independent from libdbus, and is completely bare. The test is explicitly Windows-only, as it drops all platform-independent abstractions that libdbus would have provided (not to mention the fact that DBUS_WINDOWS_SSPI_NTLM is Windows-only...). Currently the test does not do any mangling, but i fully intend to add that in later versions. I'm attaching it here for preview - i need to know whether this approach is acceptable in general, before i start hammering out the little details Created attachment 132703 [details] [review] Support SSPI NTLM authentication mechanism v7 v7: * Ensure that errors are correctly freed (found this using a testcase!) * Make server side send REJECTED instead of ERROR, because that's what other server-side code does. A pity though - this discards a lot of my carefully prepared error messages :( They do get logged on the server, at least. Sending errors didn't work well with the server states (send_error doesn't change the state, nor does it reset the auth process, which i need for the corrupt test to work). * Correctly reset SSPI message counters - multiple auth attempts on the same connection now work correcly (found this using a testcase too!) Created attachment 132704 [details] [review] DBUS_WINDOWS_SSPI_NTLM custom test v2 This is a simple socket client that connects to a dbus test server daemon and tries to authenticate using the DBUS_WINDOWS_SSPI_NTLM mechanism. The code that does authentication is completely independent from libdbus, and is completely bare. The test is explicitly Windows-only, as it drops all platform-independent abstractions that libdbus would have provided (not to mention the fact that DBUS_WINDOWS_SSPI_NTLM itself is Windows-only...). The code will try to authenticate multiple times, changing a different byte in a different buffer each time, and then expecting the authentication to success or fail depending on what is changed (the test has limited, hardcoded awareness of the NTLM message structure for that purpose). v2: * Handle disconnects better. Will retry current test iteration on send/recv error. * Added buffer corruption. All corruption is done on the side of the client, but since it corrupts both incoming and outgoing buffers, that should be sufficient. * Slightly streamlined the util code. The code looks gnarly though. Also, i cannot be 100% sure that the corrupti offsets that i exclude from testing are *actually* irrelevant, i can only be sure that they are irrelevant for *my* machine (although, AFAIU, all (most?) of them are in the "data" sections, and fixed (header) parts of the buffers are still checked). A new version of test-auth.c is needed, but i can't provide it - attachment 129336 [details] [review] is a modification of some other commit that is not currently in master. Most likely it's some private variation of my test-auth program that never got attached here for some reason. Anyway, the changes are minor - just need to give test_get_dbus_daemon() one extra argument, as the API changed since then. There is a corresponding GLib patch in https://bugzilla.gnome.org/show_bug.cgi?id=794206 to add this to GDBus. It’s blocked on SSPI support landing here first. -- 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/150. |
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.