Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)

[Corinna Vinschen]

On Oct 13 10:20, Corinna Vinschen wrote:
 On Oct 13 07:37, Christian Franke wrote:
  Corinna Vinschen wrote:
  On Oct 10 20:04, Corinna Vinschen wrote:
  In short, the whole code is written under the assumption that any sane
  application calling nonblocking connect would always call select/poll to
  check if connect succeeded in the first place.  Obviously, as postfix
  shows, this is a wrong assumption.
  
  I'm not yet sure how to fix this, but I'll look into this next week.
  I applied a fix which, I think, is much more elegant than the former
  solution.  The af_local_connect call is now called as soon as an
  FD_CONNECT event is generated and read by a call to wait_event.  It
  worked for me, so I have tender hopes that I didn't miss something.
  
  I also applied your patch on top of this new stuff and I'm just building
  a new developer snapshot for testing.
  
  A quick test of current postfix draft with the snapshot works as expected.
  Thanks.
 
 Did you run other network-related tools, too, in the meantime?  Any
 fallout which could be a result my change?
 
 In setsockopt I added a check for
  socket family and type so setsockopt(SO_PEERCRED) only works for
  AF_LOCAL/SOCK_STREAM sockets, just as the entire handshake stuff.
  
  Probably not needed because this check was already in
  af_local_set_no_getpeereid() itself.
 
 Doh!  I reverted this part of my change.  I completely missed the
 redundancy here, sorry.
 
 I
  also added a comment to explain why we do this and a FIXME comment so we
  don't forget we're still looking for a more generic solution for the
  SO_PEERCRED exchange.
  
  Definitely, at least because the current AF_LOCAL emulation has some
  security issues.
 
 -v?

Btw., I'd be grateful if we could discuss this on cygwin-developers,
if you don't mind.


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


pgpQNuD7aE3st.pgp
Description: PGP signature

Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)

[Corinna Vinschen]

On Oct 10 20:04, Corinna Vinschen wrote:
 On Oct 10 18:36, Christian Franke wrote:
  After a nonblocking connect(), postfix calls poll() with pollfd.events =
  POLLIN only. If poll() succeeds, it calls recv(). This fails with ENOTCONN
  because the state is still connect_pending.
 
 Oh.  So it doesn't check if the connect succeeded?  Does it check the
 poll result for POLLERR or does it explicitely check for revents==POLLIN?
 
 Hmm.
 
 [...time passes...]
 
 It looks like you catched a long-standing bug here.
 
 This isn't even AF_LOCAL specific.  The original comment in the
 write_selected branch is misleading: The AF_LOCAL specific part is just
 the call to af_local_connect, not setting the connect_state.  There was
 a previous, longer comment at one point which I shortened for no good
 reason in 2005:
 
   /* eid credential transaction on successful non-blocking connect.
  Since the read bit indicates an error, don't start transaction
  if it's set. */
 
 However, If I'm not completely mistaken, your patch would only work in
 the aforementioned scenario if setsockopt(SO_PEERCRED) has been called.
 Otherwise the handshake would be skipped on the connect side and thus
 the handshake would fail on the server side.  There's also the problem
 that read_ready may indicate an error.  And POLLERR is only set if the
 socket is polled for POLLOUT so a failing connect would go unnoticed.
 
 In short, the whole code is written under the assumption that any sane
 application calling nonblocking connect would always call select/poll to
 check if connect succeeded in the first place.  Obviously, as postfix
 shows, this is a wrong assumption. 
 
 I'm not yet sure how to fix this, but I'll look into this next week.

I applied a fix which, I think, is much more elegant than the former
solution.  The af_local_connect call is now called as soon as an
FD_CONNECT event is generated and read by a call to wait_event.  It
worked for me, so I have tender hopes that I didn't miss something.

I also applied your patch on top of this new stuff and I'm just building
a new developer snapshot for testing.  In setsockopt I added a check for
socket family and type so setsockopt(SO_PEERCRED) only works for
AF_LOCAL/SOCK_STREAM sockets, just as the entire handshake stuff.  I
also added a comment to explain why we do this and a FIXME comment so we
don't forget we're still looking for a more generic solution for the
SO_PEERCRED exchange.


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


pgpUY3U4vWNig.pgp
Description: PGP signature

Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)

[Corinna Vinschen]

On Oct  9 20:21, Christian Franke wrote:
 Corinna Vinschen wrote:
 +int
 +fhandler_socket::af_local_set_no_getpeereid ()
 +{
 +  if (get_addr_family () != AF_LOCAL || get_socket_type () != SOCK_STREAM)
 +{
 +  set_errno (EINVAL);
 +  return -1;
 +}
 +  if (connect_state () != unconnected)
   ^^^'
 
 Wouldn't it make sense to allow this call in the listener state as well?
 
 It should work, but I don't see any real world use case.

Indeed.  Another question, though.

I was just looking into applying your patch when I got thinking over the
change in select.cc once more.  You're setting the connect_state from
connect_pending to connected there when there's something to read on the
socket.

This puzzles me.  A completed connection attempt should set the
write_selected flag (see function peek_socket).  The AF_LOCAL handling
in the

  if (me-write_selected  me-write_ready)

case in set_bits should cover this.  What situation is your special case
covering which is not already covered by the write_selected case?


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


pgpf5aAtKeXWZ.pgp
Description: PGP signature

Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)

[Christian Franke]

Corinna Vinschen wrote:

I was just looking into applying your patch when I got thinking over the
change in select.cc once more.  You're setting the connect_state from
connect_pending to connected there when there's something to read on the
socket.

This puzzles me.  A completed connection attempt should set the
write_selected flag (see function peek_socket).


No, peek_socket() does not change write_selected. It sets write_read if 
write_selected is set.




   The AF_LOCAL handling
in the

   if (me-write_selected  me-write_ready)

case in set_bits should cover this.  What situation is your special case
covering which is not already covered by the write_selected case?


If only read status is requested via select()/poll(), write_selected is 
always false and the connect_pending=connected transition is never done.


After a nonblocking connect(), postfix calls poll() with pollfd.events = 
POLLIN only. If poll() succeeds, it calls recv(). This fails with 
ENOTCONN because the state is still connect_pending.


Christian

Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)

[Corinna Vinschen]

On Oct 10 18:36, Christian Franke wrote:
 Corinna Vinschen wrote:
 I was just looking into applying your patch when I got thinking over the
 change in select.cc once more.  You're setting the connect_state from
 connect_pending to connected there when there's something to read on the
 socket.
 
 This puzzles me.  A completed connection attempt should set the
 write_selected flag (see function peek_socket).
 
 No, peek_socket() does not change write_selected. It sets write_read if
 write_selected is set.

Uh, sorry, I mistyped.  You're right, of course.

The AF_LOCAL handling
 in the
 
if (me-write_selected  me-write_ready)
 
 case in set_bits should cover this.  What situation is your special case
 covering which is not already covered by the write_selected case?
 
 If only read status is requested via select()/poll(), write_selected is
 always false and the connect_pending=connected transition is never done.
 
 After a nonblocking connect(), postfix calls poll() with pollfd.events =
 POLLIN only. If poll() succeeds, it calls recv(). This fails with ENOTCONN
 because the state is still connect_pending.

Oh.  So it doesn't check if the connect succeeded?  Does it check the
poll result for POLLERR or does it explicitely check for revents==POLLIN?

Hmm.

[...time passes...]

It looks like you catched a long-standing bug here.

This isn't even AF_LOCAL specific.  The original comment in the
write_selected branch is misleading: The AF_LOCAL specific part is just
the call to af_local_connect, not setting the connect_state.  There was
a previous, longer comment at one point which I shortened for no good
reason in 2005:

  /* eid credential transaction on successful non-blocking connect.
 Since the read bit indicates an error, don't start transaction
 if it's set. */

However, If I'm not completely mistaken, your patch would only work in
the aforementioned scenario if setsockopt(SO_PEERCRED) has been called.
Otherwise the handshake would be skipped on the connect side and thus
the handshake would fail on the server side.  There's also the problem
that read_ready may indicate an error.  And POLLERR is only set if the
socket is polled for POLLOUT so a failing connect would go unnoticed.

In short, the whole code is written under the assumption that any sane
application calling nonblocking connect would always call select/poll to
check if connect succeeded in the first place.  Obviously, as postfix
shows, this is a wrong assumption. 

I'm not yet sure how to fix this, but I'll look into this next week.


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


pgpUkGv1EWEht.pgp
Description: PGP signature

Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)

[Corinna Vinschen]

Hi Christian,

On Sep 25 14:40, Christian Franke wrote:
 This is a workaround for this problem which blocks ITP postfix:
 https://cygwin.com/ml/cygwin/2014-08/msg00420.html
 
 With the patch, this disables the secret+cred handshakes of the AF_UNIX
 emulation:
 
 int sd = socket(AF_UNIX, SOCK_STREAM, 0);
 
 setsockopt(sd, SOL_SOCKET, SO_PEERCRED, NULL, 0);
 
 Postfix works if socket() calls are replaced by the above.
 
 Calls of getsockopt(..., SO_PEERCRED, ...) and getpeereid() would fail with 
 ENOTSUP then. These are not used by postfix.
 
 Christian
 
Patch looks good.  I'm just going to move the no_getpeereid flag into
the status block.  Also:

 +int
 +fhandler_socket::af_local_set_no_getpeereid ()
 +{
 +  if (get_addr_family () != AF_LOCAL || get_socket_type () != SOCK_STREAM)
 +{
 +  set_errno (EINVAL);
 +  return -1;
 +}
 +  if (connect_state () != unconnected)
 ^^^'

Wouldn't it make sense to allow this call in the listener state as well?


Thanks,
Corinna

-- 
Corinna Vinschen  Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat


pgp4pkwHdaD53.pgp
Description: PGP signature

Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)

[Christian Franke]

Corinna Vinschen wrote:

+int
+fhandler_socket::af_local_set_no_getpeereid ()
+{
+  if (get_addr_family () != AF_LOCAL || get_socket_type () != SOCK_STREAM)
+{
+  set_errno (EINVAL);
+  return -1;
+}
+  if (connect_state () != unconnected)

  ^^^'

Wouldn't it make sense to allow this call in the listener state as well?


It should work, but I don't see any real world use case.

Christian

[PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)

[Christian Franke]

This is a workaround for this problem which blocks ITP postfix:
https://cygwin.com/ml/cygwin/2014-08/msg00420.html

With the patch, this disables the secret+cred handshakes of the AF_UNIX 
emulation:


int sd = socket(AF_UNIX, SOCK_STREAM, 0);

setsockopt(sd, SOL_SOCKET, SO_PEERCRED, NULL, 0);

Postfix works if socket() calls are replaced by the above.

Calls of getsockopt(..., SO_PEERCRED, ...) and getpeereid() would fail with 
ENOTSUP then. These are not used by postfix.

Christian

2014-09-25  Christian Franke  fra...@computer.org

	Add setsockopt(sd, SOL_SOCKET, SO_PEERCRED, NULL, 0) to disable
	initial handshake on AF_LOCAL sockets.

	* fhandler.h (fhandler_socket::no_getpeereid): New variable.
	(fhandler_socket::af_local_set_no_getpeereid): New prototype.
	* fhandler_socket.cc (fhandler_socket::fhandler_socket):
	Initialize no_getpeereid.
	(fhandler_socket::af_local_connect): Skip handshake if
	no_getpeereid is set.  Add debug output.
	(fhandler_socket::af_local_accept): Likewise.
	(fhandler_socket::af_local_set_no_getpeereid): New function.
	(fhandler_socket::af_local_copy): Copy no_getpeereid.
	(fhandler_socket::getpeereid): Fail if no_getpeereid is set.
	* net.cc (cygwin_setsockop): Add SO_PEERCRED.
	* select.cc (set_bits): Set socket connected state also if read
	bit is requested.

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index cf4de07..6c2c3d4 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -492,6 +492,7 @@ class fhandler_socket: public fhandler_base
   pid_t sec_peer_pid;
   uid_t sec_peer_uid;
   gid_t sec_peer_gid;
+  bool no_getpeereid;
   void af_local_set_secret (char *);
   void af_local_setblocking (bool , bool );
   void af_local_unsetblocking (bool, bool);
@@ -504,6 +505,7 @@ class fhandler_socket: public fhandler_base
   int af_local_accept ();
  public:
   int af_local_connect ();
+  int af_local_set_no_getpeereid ();
   void af_local_set_sockpair_cred ();
 
  private:
diff --git a/winsup/cygwin/fhandler_socket.cc b/winsup/cygwin/fhandler_socket.cc
index 0354ee2..256ac67 100644
--- a/winsup/cygwin/fhandler_socket.cc
+++ b/winsup/cygwin/fhandler_socket.cc
@@ -231,6 +231,7 @@ fhandler_socket::fhandler_socket () :
   wsock_events (NULL),
   wsock_mtx (NULL),
   wsock_evt (NULL),
+  no_getpeereid(false),
   prot_info_ptr (NULL),
   sun_path (NULL),
   peer_sun_path (NULL),
@@ -402,7 +403,10 @@ fhandler_socket::af_local_connect ()
   if (get_addr_family () != AF_LOCAL || get_socket_type () != SOCK_STREAM)
 return 0;
 
-  debug_printf (af_local_connect called);
+  debug_printf (af_local_connect called, no_getpeereid=%d, no_getpeereid);
+  if (no_getpeereid)
+return 0;
+
   connect_state (connect_credxchg);
   af_local_setblocking (orig_async_io, orig_is_nonblocking);
   if (!af_local_send_secret () || !af_local_recv_secret ()
@@ -422,7 +426,10 @@ fhandler_socket::af_local_accept ()
 {
   bool orig_async_io, orig_is_nonblocking;
 
-  debug_printf (af_local_accept called);
+  debug_printf (af_local_accept called, no_getpeereid=%d, no_getpeereid);
+  if (no_getpeereid)
+return 0;
+
   connect_state (connect_credxchg);
   af_local_setblocking (orig_async_io, orig_is_nonblocking);
   if (!af_local_recv_secret () || !af_local_send_secret ()
@@ -438,6 +445,25 @@ fhandler_socket::af_local_accept ()
   return 0;
 }
 
+int
+fhandler_socket::af_local_set_no_getpeereid ()
+{
+  if (get_addr_family () != AF_LOCAL || get_socket_type () != SOCK_STREAM)
+{
+  set_errno (EINVAL);
+  return -1;
+}
+  if (connect_state () != unconnected)
+{
+  set_errno (EALREADY);
+  return -1;
+}
+
+  debug_printf (no_getpeereid set);
+  no_getpeereid = true;
+  return 0;
+}
+
 void
 fhandler_socket::af_local_set_cred ()
 {
@@ -462,6 +488,7 @@ fhandler_socket::af_local_copy (fhandler_socket *sock)
   sock-sec_peer_pid = sec_peer_pid;
   sock-sec_peer_uid = sec_peer_uid;
   sock-sec_peer_gid = sec_peer_gid;
+  sock-no_getpeereid = no_getpeereid;
 }
 
 void
@@ -2287,6 +2314,11 @@ fhandler_socket::getpeereid (pid_t *pid, uid_t *euid, gid_t *egid)
   set_errno (EINVAL);
   return -1;
 }
+  if (no_getpeereid)
+{
+  set_errno (ENOTSUP);
+  return -1;
+}
   if (connect_state () != connected)
 {
   set_errno (ENOTCONN);
diff --git a/winsup/cygwin/net.cc b/winsup/cygwin/net.cc
index b6c0f72..c5ca15e 100644
--- a/winsup/cygwin/net.cc
+++ b/winsup/cygwin/net.cc
@@ -810,6 +810,16 @@ cygwin_setsockopt (int fd, int level, int optname, const void *optval,
   fhandler_socket *fh = get (fd);
   if (!fh)
 	__leave;
+
+  if (optname == SO_PEERCRED  level == SOL_SOCKET)
+	{
+	  if (optval || optlen)
+	set_errno (EINVAL);
+	  else
+	res = fh-af_local_set_no_getpeereid ();
+	  __leave;
+	}
+
   /* Old applications still use the old WinSock1 IPPROTO_IP values. */
   if (level == IPPROTO_IP  CYGWIN_VERSION_CHECK_FOR_USING_WINSOCK1_VALUES)
 	optname = convert_ws1_ip_optname (optname);
diff