Bug 61787

Summary: on Windows, _dbus_read_credentials_socket() is a lie
Product: dbus Reporter: Simon McVittie <smcv>
Component: coreAssignee: Havoc Pennington <hp>
Status: RESOLVED FIXED QA Contact:
Severity: major    
Priority: medium CC: ralf.habacker, smcv
Version: unspecified   
Hardware: Other   
OS: Windows (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: Retrieve pid and sid from tcp connection
Mingw plattform fixes
some fixes
GetTcpTable2() isn't available in msvc windows XP headers (indicated by using _WIN32_WINNT 0x0501).
fixed related debug mesage eol
Make _dbus_credentials_add_pid() plattform independent.
Do not retrieve credential information from the wrong side of the connection.
Refactored to use dbus_pid_t instead of unsigned long.
Rename the term 'unix_pid' to 'pid' in variables and functions.
Use dbus_pid_t instead of unsigned long.
Add function get_peer_pid_from_tcp_handle() which returns pid and sid from tcp connection peer.
Further debug message eol fix
Use table class which has been implemented in wine 1.5.3.
Add debug message for returning from get_peer_pid_from_tcp_handle().
Add function get_peer_pid_from_tcp_handle() which returns pid and sid from tcp connection peer (combined)
updated

Description Simon McVittie 2013-03-04 12:29:57 UTC
On Bug #54445 I wrote:

> (In reply to comment #41)
> >       dict entry(
> >          string "ProcessID"
> >          variant             uint32 8
> >       )
> 
> Where did this pid come from?!
> 
> _dbus_read_credentials_socket() appears to be mis-implemented: it claims that
> every peer - even if it authenticated using the ANONYMOUS mechanism - has the
> pid and SID of the *current* process (the dbus-daemon, in this case).

What should happen is more like this:

* _dbus_read_credentials_socket() should read the byte, but not add any
  credentials, because on Windows, you can't tell who is at the other
  end of a socket. (Unless you can, in which case, do that.)

* If a peer has authenticated itself with a SASL mechanism that can
  reasonably be expected to prove its identity - of which the only one
  we support on Windows is DBUS_COOKIE_SHA1, which demonstrates the
  ability to write to the user's home directory - then it should pick up
  the SID that way. It should not pick up the process ID, because you
  can't tell the process ID like this.

* If a peer has authenticated itself using SASL ANONYMOUS
  (disabled by default), it should not have any credentials.
Comment 1 Ralf Habacker 2013-03-06 00:14:24 UTC
(In reply to comment #0)
> On Bug #54445 I wrote:
> 
> > (In reply to comment #41)
> > >       dict entry(
> > >          string "ProcessID"
> > >          variant             uint32 8
> > >       )
> > 
> > Where did this pid come from?!
> > 
> > _dbus_read_credentials_socket() appears to be mis-implemented: it claims that
> > every peer - even if it authenticated using the ANONYMOUS mechanism - has the
> > pid and SID of the *current* process (the dbus-daemon, in this case).

Exactly. in _dbus_read_credentials_socket() there is called later
...
  _dbus_credentials_add_from_current_process (credentials);

dbus_bool_t
_dbus_read_credentials_socket  (int              handle,
                                DBusCredentials *credentials,
                                DBusError       *error)
{
  int bytes_read = 0;
  DBusString buf;
  
  // could fail due too OOM
  if (_dbus_string_init(&buf))
    {
      bytes_read = _dbus_read_socket(handle, &buf, 1 );

      if (bytes_read > 0) 
        _dbus_verbose("got one zero byte from server");

      _dbus_string_free(&buf);
    }

  _dbus_credentials_add_from_current_process (credentials);
  _dbus_verbose("FIXME: get faked credentials from current process");

  return TRUE;
}

> 
> What should happen is more like this:
> 
> * _dbus_read_credentials_socket() should read the byte, 

this is already done, see above. 

>   but not add any credentials, because on Windows, you can't tell 
>   who is at the other end of a socket.
>   (Unless you can, in which case, do that.)

At least for local host connections with the help of GetExtendedTypTable() http://msdn.microsoft.com/en-us/library/windows/desktop/aa365928%28v=vs.85%29.aspx it is possible to get the ports and pid's of all dbus-daemon tcp connections (marked with '*')

0100007f 62617 00000000 0 8 *  listen 
0100007f 39412 00000000 0 8 *  listen 
0100007f 39412 0100007f 24546 8 *  
0100007f 24802 0100007f 39412 69   (2. dbus-monitor)
0100007f 24546 0100007f 39412 55   (1. dbus-monitor)
0100007f 39412 0100007f 24802 8 *  

From the pid the process owner (sid) could be fetched. There is still the question how to identify the related client in the list. 
 
> * If a peer has authenticated itself with a SASL mechanism that can
>   reasonably be expected to prove its identity - of which the only one
>   we support on Windows is DBUS_COOKIE_SHA1, which demonstrates the
>   ability to write to the user's home directory - then it should pick up
>   the SID that way. It should not pick up the process ID, because you
>   can't tell the process ID like this.
> 
> * If a peer has authenticated itself using SASL ANONYMOUS
>   (disabled by default), it should not have any credentials.

If it would be possible to get the peers connection port from the socket descriptor, the pid and sid are fetchable in any case as mentioned above.
Comment 2 Simon McVittie 2013-03-06 11:11:40 UTC
(In reply to comment #1)
> > * If a peer has authenticated itself with a SASL mechanism that can
> >   reasonably be expected to prove its identity [...] then it should pick up
> >   the SID that way.

Actually, that's not this function. On Unix, the information from this function is only used if you're doing SASL EXTERNAL, to prove that you're who you say you are; Unix will always return "I have no idea" from the equivalent function for TCP sockets (and then if the peer authenticates with DBUS_COOKIE_SHA1, we'll use the authenticated uid from that instead).

So, on Windows, a minimal implementation of this function would be to return "I don't know" (no information); and in the "I don't know" case, another part of libdbus should fill in the SID.

What happens if you just delete the _dbus_credentials_add_from_current_process() call, which is the part that's highly misleading? Assuming Windows uses DBUS_COOKIE_SHA1 authentication, it should continue to work OK and provide the SID (only).

Of course, ideally, more information would be better:

> >   but not add any credentials, because on Windows, you can't tell 
> >   who is at the other end of a socket.
> >   (Unless you can, in which case, do that.)
> 
> At least for local host connections with the help of GetExtendedTypTable()
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa365928%28v=vs.
> 85%29.aspx it is possible to get the ports and pid's of all dbus-daemon tcp
> connections (marked with '*')

Great! In that case, we could have had SASL EXTERNAL authentication on Windows all along...

(This is roughly the C API equivalent of Linux's netstat tool, right?)

> 0100007f 62617 00000000 0 8 *  listen 
> 0100007f 39412 00000000 0 8 *  listen 
> 0100007f 39412 0100007f 24546 8 *  
> 0100007f 24802 0100007f 39412 69   (2. dbus-monitor)
> 0100007f 24546 0100007f 39412 55   (1. dbus-monitor)
> 0100007f 39412 0100007f 24802 8 *  
> 
> From the pid the process owner (sid) could be fetched. There is still the
> question how to identify the related client in the list. 

dbus-daemon can call getpeername() on the socket to identify the other end (in this case 127.0.0.1:24802 and 127.0.0.1:24546, right?), then if it's an address of type AF_INET (IPv4) and the address is 127.0.0.1, look it up in the table. 55 and 69 are dbus-monitor process IDs here, right? Then presumably you can go from PID 55 to the corresponding SID relatively easily.
Comment 3 Ralf Habacker 2013-03-06 22:08:31 UTC
Created attachment 76052 [details] [review]
Retrieve pid and sid from tcp connection

These patches are a followup of https://bugs.freedesktop.org/show_bug.cgi?id=54445#c50
Comment 4 Ralf Habacker 2013-03-06 22:08:53 UTC
Created attachment 76053 [details] [review]
Mingw plattform fixes
Comment 5 Ralf Habacker 2013-03-06 22:09:49 UTC
Created attachment 76054 [details] [review]
some fixes

- fixed comparing ipv4 localhost
- get_peer_pid_from_tcp_handle(): disable ipv6 handling because ipv6 do not works currently
- _dbus_read_credentials_socket(): do not return error when not getting peers pid
Comment 6 Ralf Habacker 2013-03-07 09:49:40 UTC
Created attachment 76098 [details] [review]
GetTcpTable2() isn't available in msvc windows XP headers (indicated by using _WIN32_WINNT 0x0501).
Comment 7 Ralf Habacker 2013-03-07 09:50:33 UTC
Created attachment 76099 [details] [review]
fixed related debug mesage eol
Comment 8 Ralf Habacker 2013-03-07 09:50:58 UTC
Created attachment 76100 [details] [review]
Make _dbus_credentials_add_pid() plattform independent.
Comment 9 Ralf Habacker 2013-03-07 10:05:02 UTC
(In reply to comment #50)
> > +  getpeername(handle, (struct sockaddr*)&addr, &len);
> 
> coding style: getpeername (handle, (struct sockaddr *) &addr...);
> 
> (and many more instances of the same things)
this is very hard to follow when not working with dbus code every day. We really need an indent or similar setting, which would makes it much easier. Simply run indent ... and most of the style changes are ready. 

> The conventional label name for the end of a function in libdbus is "out" (I
> prefer "finally" personally, but libdbus already has a convention).
> 
> This can be simplified to this form, which I often use elsewhere -
> considerably less goto'ing around.
> 
>     dbus_bool_t retval = FALSE;
>     ...
>         if (bad thing)
>           goto out;
>     ...
>         if (other bad thing)
>           goto out;
>     ...
> 
>     retval = TRUE;
> 
>   out:
>     if (sid != NULL)
>       LocalFree (sid);
> 
>     return retval;
>   }

There are several places where goto's are used 

     28 again:
     62 default:
      3 done:
      1 end:
      1 err:
     18 error:
      5 fail:
     60 failed:

      2 failure:
      6 finish:
      2 finished:
      1 invalid:
      2 lose:
      9 next:
      2 noconv:
     13 nomem:
     46 oom:
    114 out:
      5 restart:
      4 retry:
      1 unlock:

Do you have ever thought about some basic refactoring, which would make this goto thing mostly obsolate 

for example 

class DBusSID {
public: 
    bool fromPID(dbus_pid_t pid);
};

void function()
{
   ... 
   DBusSID a;
   a.fromPID(apid); 
   ... 

   if (error)
       return true; 
   else 
       return false;

   // DBusSID instance is cleanup automatically in the destructor 
}
Comment 10 Simon McVittie 2013-03-07 12:32:58 UTC
Comment on attachment 76052 [details] [review]
Retrieve pid and sid from tcp connection

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

::: dbus/dbus-sysdeps-win.c
@@ +117,5 @@
> +  dbus_pid_t result;
> +  DWORD size;
> +  MIB_TCPTABLE2 *tcp_table;
> +  int i;
> +  int localhost;

maybe use dbus_bool_t for this?

@@ +126,5 @@
> +  if (addr.ss_family == AF_INET)
> +    {
> +      struct sockaddr_in *s = (struct sockaddr_in *)&addr;
> +      peer_port = ntohs (s->sin_port);
> +      localhost = s->sin_addr.s_addr == INADDR_LOOPBACK;

I'd prefer some extra parentheses to make this a little clearer:

    localhost = (s->sin_addr.s_addr == INADDR_LOOPBACK);

(and similar for the IPv6 case)

@@ +129,5 @@
> +      peer_port = ntohs (s->sin_port);
> +      localhost = s->sin_addr.s_addr == INADDR_LOOPBACK;
> +    }
> +  else
> +    { /* AF_INET6 */

else if (addr.ss_family == AF_INET6)
  {
    ... this ...
  }
else
  {
    _dbus_verbose ("no idea what address family %d is", addr.ss_family);
    return 0;
  }
Comment 11 Simon McVittie 2013-03-07 12:40:15 UTC
Comment on attachment 76053 [details] [review]
Mingw plattform fixes

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

::: dbus/dbus-sysdeps-win.c
@@ +60,5 @@
>  /* Declarations missing in mingw's headers */
>  extern BOOL WINAPI ConvertStringSidToSidA (LPCSTR  StringSid, PSID *Sid);
>  extern BOOL WINAPI ConvertSidToStringSidA (PSID Sid, LPSTR *StringSid);
>  
> +#ifndef _MSC_VER

On mingw, I think this is going to fail to compile if _WIN32_WINNT is set to a version that *does* declare them.

Can we just set _WIN32_WINNT (that's the "target Windows version", right?) to a "new enough" version (I think that means Vista)? Or do you still support XP?

@@ +84,5 @@
> +  DWORD dwNumEntries;
> +  MIB_TCPROW2 table[ANY_SIZE];
> +} MIB_TCPTABLE2,*PMIB_TCPTABLE2;
> +
> +ULONG WINAPI GetTcpTable2(

As far as I understand it, the purpose of the _WIN32_WINNT stuff is to avoid using symbols that aren't available on all supported versions of Windows. Is that true?

If we call GetTcpTable2, isn't that going to mean that on XP, applications using libdbus will just fail to start up, because the required symbol isn't found in the relevant DLL?

As far as I understand it, the standard trick for "conditional functionality" is to use GetProcAddress() to see whether it exists at runtime...
Comment 12 Simon McVittie 2013-03-07 12:41:53 UTC
Comment on attachment 76054 [details] [review]
some fixes

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

::: dbus/dbus-sysdeps-win.c
@@ +1815,4 @@
>    pid = get_peer_pid_from_tcp_handle (handle);
>  
>    if (pid == 0)
> +     return TRUE;

Ah yes, this was wrong before (and I missed that while reviewing). Please include this 1-line change in the first patch in the series.
Comment 13 Simon McVittie 2013-03-07 12:42:35 UTC
Comment on attachment 76099 [details] [review]
fixed related debug mesage eol

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

::: dbus/dbus-sysdeps-win.c
@@ +1807,4 @@
>        bytes_read = _dbus_read_socket(handle, &buf, 1 );
>  
>        if (bytes_read > 0) 
> +        _dbus_verbose("got one zero byte from server\n");

Fine, feel free to apply this "ahead".
Comment 14 Simon McVittie 2013-03-07 12:55:39 UTC
(In reply to comment #9)
> this is very hard to follow when not working with dbus code every day. We
> really need an indent or similar setting, which would makes it much easier.
> Simply run indent ... and most of the style changes are ready. 

"I know it when I see it" ;-)

It's basically Gtk style, which is similar to GNU style but without "hard" tabs, if that helps... I don't know a set of indent options that produces Gtk style, though. ("indent -nut" is probably a good start.)

> There are several places where goto's are used

Right, for different purposes. "again" is for EAGAIN handling, "default" is actually a case label rather than a goto label, "out" is like C++'s finally, and it seems "oom" and "failed" are our conventional equivalents for C++'s catch.

> class DBusSID {

No, libdbus is not going to be converted into C++. C is the lowest-common-denominator language for system libraries, with a stable ABI per platform; C++ needs an extra runtime library and has ABI concerns.

(In situations where C++ is acceptable, I'd recommend GDBus - which is not actually C++ but has a similar "weight" of runtime libraries and assumptions - or Qt.)
Comment 15 Simon McVittie 2013-03-07 13:00:28 UTC
Comment on attachment 76100 [details] [review]
Make _dbus_credentials_add_pid() plattform independent.

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

Is dbus_pid_t guaranteed to be big enough for Windows process IDs?

Looking at the source code, dbus_pid_t appears to be unsigned long, and GetCurrentProcessId() returns a DWORD; so I think we're fine there. ISO C says a long is at least 32 bits, and AIUI DWORD is always 32-bit unsigned?

This would benefit from a second line of commit message that says something like "Windows also has numeric process IDs that fit in an unsigned long, so there's no reason this has to be Unix-specific".
Comment 16 Simon McVittie 2013-03-07 13:08:31 UTC
(In reply to comment #2)
> So, on Windows, a minimal implementation of this function would be to return
> "I don't know" (no information); and in the "I don't know" case, another
> part of libdbus should fill in the SID.

Attachment #76099 [details] "fixed related debug mesage eol" is fine to apply immediately.

While we work out whether GetTcpTable2() is portable to all Windows versions that you want to support... could you please see what happens if you just delete the call to _dbus_credentials_add_from_current_process() (and the _dbus_verbose that follows) from _dbus_read_credentials_socket()?

That would fix the original reason I opened this bug - that function providing information that is untrue - and if I understand correctly, user equivalence will be authenticated via DBUS_COOKIE_SHA1, just like it is for TCP sockets on Unix. That way, you'd have a SID but no PID.

Then, if you can work out how to reconcile the GetTcpTable2() stuff with Windows-version-compatibility, we can do that part later (providing proper support for EXTERNAL authentication, as used over Unix sockets on Unix).
Comment 17 Simon McVittie 2013-03-07 13:14:49 UTC
Comment on attachment 76100 [details] [review]
Make _dbus_credentials_add_pid() plattform independent.

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

This is incomplete, and will break the build on Unix if applied as-is - there are a couple of calls to ...add_unix_pid() in dbus-sysdeps-unix.c which would also need changing.

If you're going to make this change, you should probably also change ...get_unix_pid(), and the unix_pid struct member, to just say "pid" too :-)

dbus_connection_get_unix_process_id() can't be renamed because it's in the public API, but you could add dbus_connection_get_process_id() (returning the same thing) if you want.

Similarly, the GetConnectionUnixProcessID D-Bus method can't be renamed, because it's API. I would be inclined to not add a GetConnectionProcessID, because GetConnectionCredentials is better anyway.
Comment 18 Ralf Habacker 2013-03-08 09:21:56 UTC
(In reply to comment #13)
> Comment on attachment 76099 [details] [review] [review]
> fixed related debug mesage eol
> 
> Review of attachment 76099 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-sysdeps-win.c
> @@ +1807,4 @@
> >        bytes_read = _dbus_read_socket(handle, &buf, 1 );
> >  
> >        if (bytes_read > 0) 
> > +        _dbus_verbose("got one zero byte from server\n");
> 
> Fine, feel free to apply this "ahead".

done
Comment 19 Ralf Habacker 2013-03-08 09:48:47 UTC
(In reply to comment #16)
> (In reply to comment #2)
> > So, on Windows, a minimal implementation of this function would be to return
> > "I don't know" (no information); and in the "I don't know" case, another
> > part of libdbus should fill in the SID.
> 
> Attachment #76099 [details] "fixed related debug mesage eol" is fine to
> apply immediately.
> 
> While we work out whether GetTcpTable2() is portable to all Windows versions
> that you want to support... could you please see what happens if you just
> delete the call to _dbus_credentials_add_from_current_process() (and the
> _dbus_verbose that follows) from _dbus_read_credentials_socket()?
> 
> That would fix the original reason I opened this bug - that function
> providing information that is untrue

will send a separate patch 

>  - and if I understand correctly, user
> equivalence will be authenticated via DBUS_COOKIE_SHA1, just like it is for
> TCP sockets on Unix. That way, you'd have a SID but no PID.

I have to dig into 

> 
> Then, if you can work out how to reconcile the GetTcpTable2() stuff with
> Windows-version-compatibility, we can do that part later (providing proper
> support for EXTERNAL authentication, as used over Unix sockets on Unix).

in this bug or in a different bug ?
Comment 20 Ralf Habacker 2013-03-08 10:25:00 UTC
Created attachment 76149 [details] [review]
Do not retrieve credential information from the wrong side of the connection.
Comment 21 Ralf Habacker 2013-03-08 11:37:52 UTC
(In reply to comment #17)
> Comment on attachment 76100 [details] [review] [review]
> Make _dbus_credentials_add_pid() plattform independent.
> 
> Review of attachment 76100 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This is incomplete, and will break the build on Unix if applied as-is -
> there are a couple of calls to ...add_unix_pid() in dbus-sysdeps-unix.c
> which would also need changing.

ups, I used the configured project in qtcreator and did the replace on the project level not on the file level. Will fix.  
> 
> If you're going to make this change, you should probably also change
> ...get_unix_pid(), and the unix_pid struct member, to just say "pid" too :-)

I will prepare a patch containing both changes. 

> dbus_connection_get_unix_process_id() can't be renamed because it's in the
> public API, 

> but you could add dbus_connection_get_process_id() (returning
> the same thing) if you want.

Would not be adding 

dbus_connection_get_process_id() 
dbus_connection_get_process_user() 

and mark 

dbus_connection_get_unix_process_id()
dbus_connection_get_unix_user() 

as deprecated and remove it in dbus-2.0 as good solution ? 

> Similarly, the GetConnectionUnixProcessID D-Bus method can't be renamed,
> because it's API. I would be inclined to not add a GetConnectionProcessID,
> because GetConnectionCredentials is better anyway.

Similar to the dbus interface. Mark 

GetConnectionUnixUser
GetConnectionUnixProcessID

as deprecated and remove it in dbus 2.0
Comment 22 Ralf Habacker 2013-03-08 12:20:48 UTC
(In reply to comment #15)
> Comment on attachment 76100 [details] [review] [review]
> Make _dbus_credentials_add_pid() plattform independent.
> 
> Review of attachment 76100 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Is dbus_pid_t guaranteed to be big enough for Windows process IDs?
> 
> Looking at the source code, dbus_pid_t appears to be unsigned long, and
> GetCurrentProcessId() returns a DWORD; so I think we're fine there. ISO C
> says a long is at least 32 bits, and AIUI DWORD is always 32-bit unsigned?
yes, http://msdn.microsoft.com/en-us/library/cc230318.aspx

> This would benefit from a second line of commit message that says something
> like "Windows also has numeric process IDs that fit in an unsigned long, so
> there's no reason this has to be Unix-specific".

okay
Comment 23 Ralf Habacker 2013-03-08 12:21:14 UTC
Created attachment 76157 [details] [review]
Refactored to use dbus_pid_t instead of unsigned long.
Comment 24 Ralf Habacker 2013-03-08 12:23:56 UTC
Created attachment 76158 [details] [review]
Rename the term 'unix_pid' to 'pid' in variables and functions.
Comment 25 Ralf Habacker 2013-03-08 12:24:58 UTC
Created attachment 76159 [details] [review]
Use dbus_pid_t instead of unsigned long.

fixed message
Comment 26 Simon McVittie 2013-03-08 13:05:39 UTC
(In reply to comment #19)
> > Then, if you can work out how to reconcile the GetTcpTable2() stuff with
> > Windows-version-compatibility, we can do that part later (providing proper
> > support for EXTERNAL authentication, as used over Unix sockets on Unix).
> 
> in this bug or in a different bug ?

I don't mind.

(In reply to comment #21)
> Would not be adding 
> 
> dbus_connection_get_process_id() 
> dbus_connection_get_process_user() 
> 
> and mark 
> 
> dbus_connection_get_unix_process_id()
> dbus_connection_get_unix_user() 
> 
> as deprecated and remove it in dbus-2.0 as good solution ? 

I would rather not mark the ..._unix_... versions as deprecated until we have had a stable release where the other versions exist. It's not as if it'll be any extra code to have both.

> Similar to the dbus interface. Mark 
> 
> GetConnectionUnixUser
> GetConnectionUnixProcessID
> 
> as deprecated and remove it in dbus 2.0

I'm not sure that there will ever be a D-Bus 2.0, but when GetConnectionCredentials lands, yes we can flag these as deprecated - that's the purpose of GetConnectionCredentials.
Comment 27 Simon McVittie 2013-03-08 13:06:54 UTC
Comment on attachment 76149 [details] [review]
Do not retrieve credential information from the wrong side of the connection.

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

Needs a newline at the end of the string, I think?

Does it work? If your dbus-daemon has this patch, is DBUS_COOKIE_SHA1 enough to get session bus users authenticated? If it is, add the newline and ship it :-)
Comment 28 Simon McVittie 2013-03-08 13:08:57 UTC
Comment on attachment 76158 [details] [review]
Rename the term 'unix_pid' to 'pid' in variables and functions.

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

Looks good.
Comment 29 Simon McVittie 2013-03-08 13:14:38 UTC
Comment on attachment 76159 [details] [review]
Use dbus_pid_t instead of unsigned long.

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

I can see why you'd want this, but no, this is not OK.

At the moment, dbus_pid_t is internal to dbus. It's at least as large as an unsigned long, but we could make it larger if we needed to (if we encounter a platform with 32-bit long but 64-bit pids) and that wouldn't be an ABI break. Admittedly, dbus_connection_get_unix_process_id() wouldn't work very well on such a platform, but it would at least be able to return FALSE for large pids; we could add a dbus_connection_get_large_process_id() with a larger "out" parameter, if we really needed one.

By contrast, moving dbus_pid_t into an externally-visible header would freeze it at "same size as an unsigned long" forever (or until our next ABI break, which in practice is basically forever).

::: dbus/dbus-connection.h
@@ +27,4 @@
>  #ifndef DBUS_CONNECTION_H
>  #define DBUS_CONNECTION_H
>  
> +#include <dbus/dbus-sysdeps.h>

dbus-sysdeps.h is not a public or installed header, so it cannot be included by a public or installed header.
Comment 30 Ralf Habacker 2013-03-08 14:32:57 UTC
Created attachment 76166 [details] [review]
Add function get_peer_pid_from_tcp_handle() which returns pid and sid from tcp connection peer.

fixed win api includes problem by using GetExtendedTcpTable()
Comment 31 Ralf Habacker 2013-03-08 17:00:59 UTC
(In reply to comment #27)
> Comment on attachment 76149 [details] [review] [review]
> Do not retrieve credential information from the wrong side of the connection.
> 
> Review of attachment 76149 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Needs a newline at the end of the string, I think?
>ha
> Does it work? If your dbus-daemon has this patch, is DBUS_COOKIE_SHA1 enough
> to get session bus users authenticated? If it is, add the newline and ship
> it :-)

I think it is: 
1. added <auth>DBUS_COOKIE_SHA1</auth> to session.conf
2. started dbus-daemon 
3. connected with dbus-monitor 

in the dbus verbose log I found 

55: [dbus/dbus-auth.c(1704):handle_auth] server: Trying mechanism DBUS_COOKIE_SHA1
55: [dbus/dbus-transport.c(655):auth_via_default_rules] Client authorized as SID 'S-1-5-21-0-0-0-1000'matching our SID 'S-1-5-21-0-0-0-1000'

which looks like what you expected.
Comment 32 Ralf Habacker 2013-03-08 22:19:34 UTC
Created attachment 76195 [details] [review]
Further debug message eol fix
Comment 33 Ralf Habacker 2013-03-08 22:21:00 UTC
Created attachment 76197 [details] [review]
Use table class which has been implemented in wine 1.5.3.
Comment 34 Ralf Habacker 2013-03-08 22:22:47 UTC
Created attachment 76198 [details] [review]
Add debug message for returning from get_peer_pid_from_tcp_handle().
Comment 35 Ralf Habacker 2013-03-08 22:30:30 UTC
(In reply to comment #28)
> Comment on attachment 76158 [details] [review] [review]
> Rename the term 'unix_pid' to 'pid' in variables and functions.
> 
> Review of attachment 76158 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks good.

applied
Comment 36 Simon McVittie 2013-03-11 11:40:27 UTC
Comment on attachment 76195 [details] [review]
Further debug message eol fix

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

r+
Comment 37 Simon McVittie 2013-03-11 11:45:54 UTC
Comment on attachment 76197 [details] [review]
Use table class which has been implemented in wine 1.5.3.

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

Please clarify why this is a good thing. Some possible things you might have meant include:

"there is no Wine version that implements TCP_TABLE_OWNER_PID_CONNECTIONS, but Wine >= 1.5.3 implements TCP_TABLE_OWNER_PID_ALL, so use ALL to make this testable under Wine"

"TCP_TABLE_OWNER_PID_ALL is better than TCP_TABLE_OWNER_PID_CONNECTIONS because [...], and we don't need to avoid ALL for the benefit of testing under Wine, because Wine has implemented it since 1.5.3"
Comment 38 Simon McVittie 2013-03-11 11:46:21 UTC
Comment on attachment 76198 [details] [review]
Add debug message for returning from get_peer_pid_from_tcp_handle().

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

::: dbus/dbus-sysdeps-win.c
@@ +187,4 @@
>          result = p->dwOwningPid;
>      }
>  
> +  _dbus_verbose ("got pid %d\n", result);

Sure, if this is useful information to have...
Comment 39 Simon McVittie 2013-03-11 11:47:50 UTC
(In reply to comment #31)
> I think it is: 
> 1. added <auth>DBUS_COOKIE_SHA1</auth> to session.conf
> 2. started dbus-daemon 
> 3. connected with dbus-monitor 

Just to confirm, if session.conf does not include <auth>DBUS_COOKIE_SHA1</auth>, does connecting a dbus-monitor to a dbus-daemon still work?

(Hopefully it does, because the default if there is no <auth/> is to try all possible mechanisms, I think.)
Comment 40 Ralf Habacker 2013-03-11 13:16:15 UTC
(In reply to comment #39)
> (In reply to comment #31)
> > I think it is: 
> > 1. added <auth>DBUS_COOKIE_SHA1</auth> to session.conf
> > 2. started dbus-daemon 
> > 3. connected with dbus-monitor 
> 
> Just to confirm, if session.conf does not include
> <auth>DBUS_COOKIE_SHA1</auth>, does connecting a dbus-monitor to a
> dbus-daemon still work?
> 
> (Hopefully it does, because the default if there is no <auth/> is to try all
> possible mechanisms, I think.)

yes, this is done in 

static dbus_bool_t
auth_via_default_rules (DBusTransport *transport)
{
....

  /* By default, connection is allowed if the client is 1) root or 2)
   * has the same UID as us or 3) anonymous is allowed.
   */

on windows case 1 and 3 applies.
Comment 41 Ralf Habacker 2013-03-11 13:21:00 UTC
(In reply to comment #38)
> Comment on attachment 76198 [details] [review] [review]
> Add debug message for returning from get_peer_pid_from_tcp_handle().
> 
> Review of attachment 76198 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-sysdeps-win.c
> @@ +187,4 @@
> >          result = p->dwOwningPid;
> >      }
> >  
> > +  _dbus_verbose ("got pid %d\n", result);
> 
> Sure, if this is useful information to have...

yes this makes debugging easier. :-) 

Any problem to combine 

- Further debug message eol fix
- Use table class which has been implemented in wine 1.5.3
- Add debug message for returning from get_peer_pid_from_tcp_handle

and 
- Add function get_peer_pid_from_tcp_handle() which returns pid and sid from tcp connection peer

into one commit ? I would add the wine related note.
Comment 42 Simon McVittie 2013-03-11 14:41:39 UTC
(In reply to comment #41)
> Any problem to combine 
> 
> - Further debug message eol fix
> - Use table class which has been implemented in wine 1.5.3
> - Add debug message for returning from get_peer_pid_from_tcp_handle
> 
> and 
> - Add function get_peer_pid_from_tcp_handle() which returns pid and sid from
> tcp connection peer
> 
> into one commit ?

That's fine, but please put that commit (including the note about why you chose that table) up for review.
Comment 43 Ralf Habacker 2013-03-11 15:47:20 UTC
Created attachment 76342 [details] [review]
Add function get_peer_pid_from_tcp_handle() which returns  pid and sid from tcp connection peer (combined)
Comment 44 Ralf Habacker 2013-03-26 11:58:09 UTC
(In reply to comment #43)
> Created attachment 76342 [details] [review] [review]
> Add function get_peer_pid_from_tcp_handle() which returns  pid and sid from
> tcp connection peer (combined)

Hi Simon, any time for review ? Thanks :-)
Comment 45 Simon McVittie 2013-04-04 14:49:19 UTC
Comment on attachment 76342 [details] [review]
Add function get_peer_pid_from_tcp_handle() which returns  pid and sid from tcp connection peer (combined)

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

Looks good, just a couple of things:

::: dbus/dbus-sysdeps-win.c
@@ +109,5 @@
> + * @param handle tcp socket descriptor
> + * @return process id or 0 in case the process id could not be fetched
> + */
> +dbus_pid_t
> +get_peer_pid_from_tcp_handle (int handle)

This function should be static (or prefixed with _dbus_ if it's used outside this file).

@@ +178,5 @@
> +      return 0;
> +    }
> +
> +  result = 0;
> +  for (i = 0; i < (int) tcp_table->dwNumEntries; i++)

Make i a DWORD or whatever the appropriate type is, then you won't need a cast?
Comment 46 Ralf Habacker 2013-04-04 15:30:52 UTC
Created attachment 77431 [details] [review]
updated
Comment 47 Ralf Habacker 2013-04-05 09:23:18 UTC
Comment on attachment 77431 [details] [review]
updated

> 
> Looks good, just a couple of things:
> 
fixed and applied to master.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.