public inbox for cygwin-patches@cygwin.com
 help / color / mirror / Atom feed
* [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
@ 2014-09-25 12:40 Christian Franke
  2014-10-09 18:00 ` Corinna Vinschen
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Franke @ 2014-09-25 12:40 UTC (permalink / raw)
  To: cygwin-patches

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

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


[-- Attachment #2: setsockopt-so_peercred.patch --]
[-- Type: text/x-patch, Size: 5080 bytes --]

2014-09-25  Christian Franke  <franke@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 --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 6a39685..75eb726 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -473,6 +473,9 @@ set_bits (select_record *me, fd_set *readfds, fd_set *writefds,
   if (me->read_selected && me->read_ready)
     {
       UNIX_FD_SET (me->fd, readfds);
+      /* Special AF_LOCAL handling. */
+      if ((sock = me->fh->is_socket ()) && sock->connect_state () == connect_pending)
+	sock->connect_state (connected);
       ready++;
     }
   if (me->write_selected && me->write_ready)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-09-25 12:40 [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...) Christian Franke
@ 2014-10-09 18:00 ` Corinna Vinschen
  2014-10-09 18:22   ` Christian Franke
  0 siblings, 1 reply; 10+ messages in thread
From: Corinna Vinschen @ 2014-10-09 18:00 UTC (permalink / raw)
  To: cygwin-patches

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-10-09 18:00 ` Corinna Vinschen
@ 2014-10-09 18:22   ` Christian Franke
  2014-10-10 11:07     ` Corinna Vinschen
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Franke @ 2014-10-09 18:22 UTC (permalink / raw)
  To: cygwin-patches

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-10-09 18:22   ` Christian Franke
@ 2014-10-10 11:07     ` Corinna Vinschen
  2014-10-10 16:36       ` Christian Franke
  0 siblings, 1 reply; 10+ messages in thread
From: Corinna Vinschen @ 2014-10-10 11:07 UTC (permalink / raw)
  To: cygwin-patches

[-- Attachment #1: Type: text/plain, Size: 1318 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-10-10 11:07     ` Corinna Vinschen
@ 2014-10-10 16:36       ` Christian Franke
  2014-10-10 18:04         ` Corinna Vinschen
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Franke @ 2014-10-10 16:36 UTC (permalink / raw)
  To: cygwin-patches

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-10-10 16:36       ` Christian Franke
@ 2014-10-10 18:04         ` Corinna Vinschen
  2014-10-11 18:36           ` Corinna Vinschen
  0 siblings, 1 reply; 10+ messages in thread
From: Corinna Vinschen @ 2014-10-10 18:04 UTC (permalink / raw)
  To: cygwin-patches

[-- Attachment #1: Type: text/plain, Size: 2831 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-10-10 18:04         ` Corinna Vinschen
@ 2014-10-11 18:36           ` Corinna Vinschen
  2014-10-13  5:38             ` Christian Franke
  0 siblings, 1 reply; 10+ messages in thread
From: Corinna Vinschen @ 2014-10-11 18:36 UTC (permalink / raw)
  To: cygwin-patches

[-- Attachment #1: Type: text/plain, Size: 2718 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-10-11 18:36           ` Corinna Vinschen
@ 2014-10-13  5:38             ` Christian Franke
  2014-10-13  8:20               ` Corinna Vinschen
  0 siblings, 1 reply; 10+ messages in thread
From: Christian Franke @ 2014-10-13  5:38 UTC (permalink / raw)
  To: cygwin-patches

Corinna Vinschen wrote:
> 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.

A quick test of current postfix draft with the snapshot works as 
expected. Thanks.


>    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.


>    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.

Thanks,
Christian

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-10-13  5:38             ` Christian Franke
@ 2014-10-13  8:20               ` Corinna Vinschen
  2014-10-13  9:05                 ` Corinna Vinschen
  0 siblings, 1 reply; 10+ messages in thread
From: Corinna Vinschen @ 2014-10-13  8:20 UTC (permalink / raw)
  To: cygwin-patches

[-- Attachment #1: Type: text/plain, Size: 1920 bytes --]

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?


Thanks,
Corinna

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...)
  2014-10-13  8:20               ` Corinna Vinschen
@ 2014-10-13  9:05                 ` Corinna Vinschen
  0 siblings, 0 replies; 10+ messages in thread
From: Corinna Vinschen @ 2014-10-13  9:05 UTC (permalink / raw)
  To: cygwin-patches

[-- Attachment #1: Type: text/plain, Size: 2138 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-10-13  9:05 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-25 12:40 [PATCH] Disable AF_UNIX handshake with setsockopt(..., SO_PEERCRED, ...) Christian Franke
2014-10-09 18:00 ` Corinna Vinschen
2014-10-09 18:22   ` Christian Franke
2014-10-10 11:07     ` Corinna Vinschen
2014-10-10 16:36       ` Christian Franke
2014-10-10 18:04         ` Corinna Vinschen
2014-10-11 18:36           ` Corinna Vinschen
2014-10-13  5:38             ` Christian Franke
2014-10-13  8:20               ` Corinna Vinschen
2014-10-13  9:05                 ` Corinna Vinschen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).