Bug 83499 - document the assumption that makes our use of credentials-passing secure
Summary: document the assumption that makes our use of credentials-passing secure
Status: ASSIGNED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.5
Hardware: All All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch, security
Depends on:
Blocks:
 
Reported: 2014-09-04 13:45 UTC by Simon McVittie
Modified: 2016-05-10 17:11 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments
dbus-sysdeps-unix: document the assumption that makes our use of credentials secure (1.97 KB, patch)
2014-09-11 12:14 UTC, Simon McVittie
Details | Splinter Review
dbus-sysdeps-win: document the credentials assumptions that we're making (2.59 KB, patch)
2014-09-11 12:21 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2014-09-04 13:45:27 UTC
Here is a potential attack by a malicious message sender:

* connect to the D-Bus socket
* race with dbus-daemon, and win:
  - queue up the entire handshake, plus a malicious message, in the outgoing
    socket buffer
  - make the D-Bus connection not close-on-exec
  - exec() a setuid process, or fd-pass the D-Bus connection to a more
    privileged process
* dbus-daemon checks the sender's credentials (let's say "uid 1000"), and
  thinks it has the credentials of the setuid process (e.g. "uid 0")
* now dbus-daemon reads the malicious message, determines that uid 0 may
  send it, and forwards it to the recipient, with no indication that
  uid 1000 was actually responsible

We are not vulnerable to this, but it is not instantly obvious why not.

Having done some research in a bunch of OS kernels, the reason we're not vulnerable is that in practice, SO_PEERCRED and analogous APIs like getpeereid() are based on the kernel stashing the credentials as part of the socket descriptor early in its life cycle, either at the time that it is created with socket() (*BSD) or at the time it connect()s or listen()s (Linux).

Linux LSMs also have a hook to do the same with their LSM-specific identity info; as of 3.17-rc3 this is implemented for SELinux and Smack, but not AppArmor. Ubuntu's patched kernel adds the same thing for AppArmor.

If you think about it further, it becomes clear that a kernel would have a hard time implementing "check what the credentials of the other end of this socket are right now", because in the presence of fd-passing, "the other end" is non-unique. So I'm reasonably confident that this isn't suddenly going to change.

However, I think we should document this assumption so that unsuitable APIs are not added as new code paths for _dbus_read_credentials_socket() in future.

In particular, any API of the form "get the pid, then get that process's $SPECIAL_CREDENTIAL (from /proc or otherwise), then decide based on that" cannot be relied upon for security, because (1) exec() exists, and so do setuid binaries, and (2) even if exec() didn't exist, pids get recycled.
Comment 1 Simon McVittie 2014-09-11 12:14:49 UTC
Created attachment 106126 [details] [review]
dbus-sysdeps-unix: document the assumption that makes our  use of credentials secure
Comment 2 Simon McVittie 2014-09-11 12:21:21 UTC
Created attachment 106127 [details] [review]
dbus-sysdeps-win: document the credentials assumptions  that we're making

Unlike on Unix, I'm not at all sure that these assumptions are
actually valid. If they're not, we should revert the features
added in 1.7.2 and 1.7.6 for fd.o#61787 and fd.o#66060, and
instead report that all bus clients have an unknown uid and pid.

---

Sorry, Ralf; I should have seen this when I reviewed Bug #61787 and Bug #66060, but I didn't.

If this information cannot be relied on, then we should not provide it, which unfortunately would mean reverting Bug #61787 and Bug #66060. I don't know a way to do this securely, but perhaps you do?

I'm not putting this under embargo to do a coordinated security release, because I don't consider D-Bus on Windows to be something that has security support, is suitable to be a privilege boundary, or is robust against local attackers anyway (see also Bug #35311); and before Bug #61787 was fixed, we were reporting completely wrong information anyway.
Comment 3 Ralf Habacker 2014-09-11 18:52:32 UTC
(In reply to comment #2)
> Created attachment 106127 [details] [review] [review]
> dbus-sysdeps-win: document the credentials assumptions  that we're making
> 
> Unlike on Unix, I'm not at all sure that these assumptions are
> actually valid. If they're not, we should revert the features
> added in 1.7.2 and 1.7.6 for fd.o#61787 and fd.o#66060, and
> instead report that all bus clients have an unknown uid and pid.
> 
In the short I found a winsock security related article in msdn 
http://msdn.microsoft.com/en-us/library/windows/desktop/ms740139%28v=vs.85%29.aspx. Unfortunally I did not found the time to study it yet.
Comment 4 Ralf Habacker 2014-09-11 21:32:55 UTC
At http://blogs.msdn.com/b/oldnewthing/archive/2011/01/07/10112755.aspx there is mentioned 

"The process ID is a value associated with the process object, and as long as the process object is still around, so too will its process ID. The process object remains as long as the process is still running (the process implicitly retains a reference to itself) or as long as somebody still has a handle to the process object."
Comment 5 Simon McVittie 2014-09-12 10:34:14 UTC
(In reply to comment #4)
> "The process ID is a value associated with the process object, and as long
> as the process object is still around, so too will its process ID. The
> process object remains as long as the process is still running (the process
> implicitly retains a reference to itself) or as long as somebody still has a
> handle to the process object."

I don't think this has any bearing on what I wrote in Attachment #106127 [details], unless there's some way we can obtain a handle to the process object that started the TCP connection (other than via "get process ID of other end" and "get a handle for process ID 12345", which may have the race that I described).

If we *can* do that, then the _dbus_getsid() call can be made safe by retaining that handle until after _dbus_getsid() has returned, or by changing its signature so we pass a handle as the parameter instead of a pid.
Comment 6 Simon McVittie 2014-09-12 10:47:09 UTC
(In reply to comment #3)
> In the short I found a winsock security related article in msdn 
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms740139%28v=vs.
> 85%29.aspx. Unfortunally I did not found the time to study it yet.

WSAImpersonateSocketPeer() and WSARevertImpersonation() look potentially promising: the calling thread can (maybe) impersonate the other end of the socket, ask the Windows API what (process and?) SID it's currently impersonating, then turn off impersonation and return the results.

The rest of the functions mentioned there don't look immediately relevant.

Please open a separate bug if you want to try implementing that: the thing I'm interested in in the short term is documenting our assumptions, mainly on Unix.
Comment 7 Simon McVittie 2014-10-09 10:45:39 UTC
Any feedback on this from Unix people? It's been nearly a month.

I would really like to get this stuff documented, particularly on Unix where we are actively recommending dbus-daemon as a security boundary.

If the assumptions are not actually true in the Windows case, we can apply this patch anyway, or not, whichever people prefer, and deal with the failed assumptions as a separate issue.
Comment 8 Ralf Habacker 2014-10-09 10:58:40 UTC
(In reply to Simon McVittie from comment #5)
> (In reply to comment #4)
> > "The process ID is a value associated with the process object, and as long
> > as the process object is still around, so too will its process ID. The
> > process object remains as long as the process is still running (the process
> > implicitly retains a reference to itself) or as long as somebody still has a
> > handle to the process object."
> 
> I don't think this has any bearing on what I wrote in Attachment #106127 [details]
> [details], unless there's some way we can obtain a handle to the process
> object that started the TCP connection (other than via "get process ID of
> other end" and "get a handle for process ID 12345", which may have the race
> that I described).
> 
> If we *can* do that, then the _dbus_getsid() call can be made safe by
> retaining that handle until after _dbus_getsid() has returned, or by
> changing its signature so we pass a handle as the parameter instead of a pid.

You wrote in description: 
"... pids get recycled."

I unterstand comment #4 in a way that as long as dbus-daemon has an open handle to the client, the client pid could *NOT* be reused, which is one part of the problem. If the pid could not be reused, the problem does not exist or are I'm completly wrong ?
Comment 9 Ralf Habacker 2014-10-09 11:06:33 UTC
(In reply to Ralf Habacker from comment #8)
> (In reply to Simon McVittie from comment #5)
> > (In reply to comment #4)
> > > "The process ID is a value associated with the process object, and as long
> > > as the process object is still around, so too will its process ID. The
> > > process object remains as long as the process is still running (the process
> > > implicitly retains a reference to itself) or as long as somebody still has a
> > > handle to the process object."
> > 
> > I don't think this has any bearing on what I wrote in Attachment #106127 [details]
> > [details], unless there's some way we can obtain a handle to the process
> > object that started the TCP connection (other than via "get process ID of
> > other end" and "get a handle for process ID 12345", which may have the race
> > that I described).

You are refering to the case, that pid hijacking has already been in place, when dbus-daemon tries to get the client process handle after the client connects ?
Comment 10 Simon McVittie 2014-10-09 11:16:26 UTC
(In reply to Ralf Habacker from comment #9)
> > > I don't think this has any bearing on what I wrote in Attachment #106127 [details], > > > unless there's some way we can obtain a handle to the process
> > > object that started the TCP connection (other than via "get process ID of
> > > other end" and "get a handle for process ID 12345", which may have the race
> > > that I described).
> 
> You are refering to the case, that pid hijacking has already been in place,
> when dbus-daemon tries to get the client process handle after the client
> connects ?

Yes. If we don't already have a handle to the client process, we can't guarantee that its pid will not be recycled before we can get that handle.

This is the relevant race:

* process A (pid 1234) runs as user AA
* process A connects to dbus-daemon from 127.0.0.1:2345
* process A writes a message M into the outgoing TCP socket buffer
* dbus-daemon looks up 127.0.0.1:2345 in the TCP table and finds pid 1234
* all of this happens before dbus-daemon proceeds:
  - process A (pid 1234) exits
  - the attacker starts exactly enough processes that process-ID space wraps
    around (on Linux this would be 2**15 minus the number of running
    processes)
  - privileged process B starts as user BB, and gets assigned pid 1234
* dbus-daemon sees that pid 1234 is owned by BB
* dbus-daemon reads M from the TCP socket and behaves as if BB had sent it,
  but actually, AA sent it

Unix systems typically only have 15-bit process IDs, so making process IDs wrap around is entirely feasible. I don't know whether that is the case on Windows: if Windows uses the full 32-bit space, then this attack seems very unlikely to succeed.

I also don't know whether it is possible for process A to exit and have its process ID re-used while leaving unprocessed stuff queued in the TCP buffer.
Comment 11 Ralf Habacker 2014-10-09 11:21:27 UTC
(In reply to Simon McVittie from comment #10)
> (In reply to Ralf Habacker from comment #9)
... 
> Unix systems typically only have 15-bit process IDs, so making process IDs
> wrap around is entirely feasible. I don't know whether that is the case on
> Windows: if Windows uses the full 32-bit space, then this attack seems very
> unlikely to succeed.

http://stackoverflow.com/questions/17868218/what-is-the-maximum-process-id-on-windows
Comment 12 Ralf Habacker 2014-10-09 11:30:15 UTC
(In reply to Simon McVittie from comment #10)
> (In reply to Ralf Habacker from comment #9)
...
> I also don't know whether it is possible for process A to exit and have its
> process ID re-used while leaving unprocessed stuff queued in the TCP buffer.

3rdparty answer http://stackoverflow.com/questions/13265732/what-happens-in-a-tcp-connection-if-the-receiving-process-stops-or-suspends
Comment 13 Ralf Habacker 2014-10-09 11:32:20 UTC
(In reply to Ralf Habacker from comment #12)
> (In reply to Simon McVittie from comment #10)
> > (In reply to Ralf Habacker from comment #9)
> ...
> > I also don't know whether it is possible for process A to exit and have its
> > process ID re-used while leaving unprocessed stuff queued in the TCP buffer.
> 
> 3rdparty answer
> http://stackoverflow.com/questions/13265732/what-happens-in-a-tcp-connection-
> if-the-receiving-process-stops-or-suspends

here another one http://info.prelert.com/blog/portable-tcp-sockets-or-not

"When a process exits with open TCP connections the operating system is responsible for closing them. It turns out that Windows does this differently to most Unix-like operating systems. When Linux, Solaris and Mac OS X close a TCP connection left open by a process, they send a FIN segment to the remote end of the connection, which indicates a graceful close of the socket. When Windows closes a TCP connection left open by a process, it sends an RST segment, which indicates a reset."
Comment 14 Alban Crequy 2014-10-09 12:33:08 UTC
Comment on attachment 106126 [details] [review]
dbus-sysdeps-unix: document the assumption that makes our  use of credentials secure

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

The text looks correct to me.
Comment 15 Ralf Habacker 2014-10-11 08:18:41 UTC
(In reply to Ralf Habacker from comment #13)
> (In reply to Ralf Habacker from comment #12)
> > (In reply to Simon McVittie from comment #10)
> > > (In reply to Ralf Habacker from comment #9)
> > ...
> > > I also don't know whether it is possible for process A to exit and have its
> > > process ID re-used while leaving unprocessed stuff queued in the TCP buffer.
> > 
> > 3rdparty answer
> > http://stackoverflow.com/questions/13265732/what-happens-in-a-tcp-connection-
> > if-the-receiving-process-stops-or-suspends
> 
> here another one http://info.prelert.com/blog/portable-tcp-sockets-or-not
> 
> "When a process exits with open TCP connections the operating system is
> responsible for closing them. It turns out that Windows does this
> differently to most Unix-like operating systems. When Linux, Solaris and Mac
> OS X close a TCP connection left open by a process, they send a FIN segment
> to the remote end of the connection, which indicates a graceful close of the
> socket. When Windows closes a TCP connection left open by a process, it
> sends an RST segment, which indicates a reset."

Looks like that dbus-daemon will be informed about the client process exit, which lead to the solution, if not already present, to let dbus-daemon to handles the FIN or RST signal.
Comment 16 Ralf Habacker 2014-10-14 12:28:54 UTC
(In reply to Simon McVittie from comment #10)

Here are my notes according to comment #15:
 
> This is the relevant race:
> 
> * process A (pid 1234) runs as user AA
> * process A connects to dbus-daemon from 127.0.0.1:2345
> * process A writes a message M into the outgoing TCP socket buffer
> * dbus-daemon looks up 127.0.0.1:2345 in the TCP table and finds pid 1234
> * all of this happens before dbus-daemon proceeds:
>   - process A (pid 1234) exits
>   - the attacker starts exactly enough processes that process-ID space wraps
>     around (on Linux this would be 2**15 minus the number of running
>     processes)
>   - privileged process B starts as user BB, and gets assigned pid 1234
> * dbus-daemon sees that pid 1234 is owned by BB
> * dbus-daemon reads M from the TCP socket

  * dbus-daemon gets a FIN (RST) signal and drops the connection[1]

>  and behaves as if BB had sent it, but actually, AA sent it

  no more any issue with this connection.

[1] According to http://stackoverflow.com/questions/17705239/is-there-a-way-to-detect-that-tcp-socket-has-been-closed-by-the-remote-peer-wit it is possible to detect this case by calling recv() with MSG_PEEK flags or the check the return value from a select() call.
Comment 17 Ralf Habacker 2014-10-16 22:04:55 UTC
(In reply to Ralf Habacker from comment #16)
> (In reply to Simon McVittie from comment #10)
> 
> Here are my notes according to comment #15:
>  
> > This is the relevant race:
> > 
> > * process A (pid 1234) runs as user AA
> > * process A connects to dbus-daemon from 127.0.0.1:2345
> > * process A writes a message M into the outgoing TCP socket buffer
> > * dbus-daemon looks up 127.0.0.1:2345 in the TCP table and finds pid 1234
> > * all of this happens before dbus-daemon proceeds:
> >   - process A (pid 1234) exits

I tried to find a real work test case. At first I patched dbus-monitor.c and exits immediatly after a unix domain socket connection has been established: 

  if (address != NULL)
    {
      fprintf(stderr, "before connect");

      connection = dbus_connection_open (address, &error);
      if (connection)
        {
          fprintf(stderr, "exit after connect");
          exit(1);

The related dbus-daemon message is: 

30696: [dbus/dbus-transport-socket.c(378):exchange_credentials] Failed to read credentials Failed to read credentials byte (zero-length read)
30696: [dbus/dbus-transport.c(504):_dbus_transport_disconnect] start
Comment 18 Simon McVittie 2014-10-29 16:22:27 UTC
Comment on attachment 106126 [details] [review]
dbus-sysdeps-unix: document the assumption that makes our  use of credentials secure

Applied for 1.9.2
Comment 19 Simon McVittie 2016-04-13 16:47:20 UTC
(In reply to Ralf Habacker from comment #16)
> (In reply to Simon McVittie from comment #10)
> > This is the relevant race:
> > 
> > * process A (pid 1234) runs as user AA
> > * process A connects to dbus-daemon from 127.0.0.1:2345
> > * process A writes a message M into the outgoing TCP socket buffer
> > * dbus-daemon looks up 127.0.0.1:2345 in the TCP table and finds pid 1234
> > * all of this happens before dbus-daemon proceeds:
> >   - process A (pid 1234) exits
> >   - the attacker starts exactly enough processes that process-ID space wraps
> >     around (on Linux this would be 2**15 minus the number of running
> >     processes)
> >   - privileged process B starts as user BB, and gets assigned pid 1234
> > * dbus-daemon sees that pid 1234 is owned by BB
> > * dbus-daemon reads M from the TCP socket
> 
>   * dbus-daemon gets a FIN (RST) signal and drops the connection[1]

To the best of my knowledge, everything comes out of the socket in sequence: first the dbus-daemon will read the message M, and *then*, on its next attempt to read the socket after that, it will see the "end of file" event (select() says the socket is readable, recv() returns 0) representing the FIN/RST. Your Stack Overflow reference doesn't seem to contradict that.

If the process ID can be recycled while the dbus-daemon is still reading the message M, such that the operation "start from a process ID, get a corresponding HANDLE" yields a HANDLE to a different process, then we have a problem.

Even if your Stack Overflow reference did contradict that, I wouldn't really be comfortable with using second-hand information (Stack Overflow answers) as proof that something we rely on for security is in fact true - I'd really prefer references to actual OS documentation (for Unix that's POSIX/SUS, man pages or source code; for Windows, MSDN would be suitable).

(In reply to Ralf Habacker from comment #17)
> I tried to find a real work test case. At first I patched dbus-monitor.c and
> exits immediatly after a unix domain socket connection has been established

This doesn't really prove anything, because your patched dbus-monitor is not attempting to send messages and have them processed after it exits. Yes, you've demonstrated that we can detect a non-malicious process exiting "early" when it does not make any particular attempt to circumvent this; that's not really the question though. We don't need to defend against non-malicious processes, because they're non-malicious :-)


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.