Datagram disassociation should be done via AF_UNSPEC

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Datagram disassociation should be done via AF_UNSPEC

astian
(Sorry if you are getting this message duplicated, I tried sending it to
tech@ before but it seems to get stuck somewhere along the way.)

I found this while looking at netcat and it seemed odd:

        if (family == AF_UNIX && uflag) {
                if (connect(s, NULL, 0) == -1)
                        err(1, "connect");
        }

POSIX 2008 [0] says:

  For SOCK_DGRAM sockets, [...] If the sa_family member of address is
  AF_UNSPEC, the socket's peer address shall be reset.

It also says:

  Issue 7
  [...]
    - Austin Group Interpretation 1003.1-2001 #188 is applied, changing
      the method used to reset a peer address for a datagram socket.
  [...]

Indeed, in POSIX 2004 [1] it said:

  For SOCK_DGRAM sockets, [...] If address is a null address for the
  protocol, the socket's peer address shall be reset.

Which is similar to connect(2):

  Datagram sockets may dissolve the association by connecting to an
  invalid address, such as a null address.

Now, I don't know if "null address for the protocol" was at any point in
time supposed to be interpreted as NULL-pointer/zero-size as opposed to
zeroed structure for example (notice that AF_UNSPEC is 0) or whether
such a thing ever worked in OpenBSD, but it certainly doesn't now and
hasn't for a while: it fails in sys_connect -> sockargs when checking
for the sockaddr size.

OpenBSD does support disassociating via a zeroed sockaddr, but the
syscall fails and it shouldn't; in fact, it supports disassociating by
using *any* sort of address as long as it's consistent in terms of size
because the disassociation (sodisconnect) happens before even looking at
the user-supplied sockaddr.  I believe this should to be cleaned up but
it is a change in syscall semantics (according to CVS this code,
soconnect, mostly comes directly from NetBSD from 23 years ago).

I did not look at what other BSD do, but below is a possible minimal
patch that aims to retain most of the current behaviour, which might or
might not be TRTTD.  It should apply to "current".

Cheers.

0: https://pubs.opengroup.org/onlinepubs/9699919799/functions/connect.html
1: https://pubs.opengroup.org/onlinepubs/009695399/functions/connect.html

--- sys/kern/uipc_socket.c 2019-08-09 17:27:45 +0000
+++ sys/kern/uipc_socket.c 2019-08-10 17:41:07 +0000
@@ -66,6 +66,8 @@ void soreaper(void *);
 void soput(void *);
 int somove(struct socket *, int);
 
+int nam2sa(struct mbuf *, struct sockaddr **);
+
 void filt_sordetach(struct knote *kn);
 int filt_soread(struct knote *kn, long hint);
 void filt_sowdetach(struct knote *kn);
@@ -344,7 +346,8 @@ soaccept(struct socket *so, struct mbuf
 int
 soconnect(struct socket *so, struct mbuf *nam)
 {
- int error;
+ struct sockaddr *sa;
+ int error = 0;
 
  soassertlocked(so);
 
@@ -353,14 +356,16 @@ soconnect(struct socket *so, struct mbuf
  /*
  * If protocol is connection-based, can only connect once.
  * Otherwise, if connected, try to disconnect first.
- * This allows user to disconnect by connecting to, e.g.,
- * a null address.
+ * This allows the user to disconnect by connecting to an invalid
+ * address, although only a "zero" address (AF_UNSPEC) will not
+ * result in error.
  */
  if (so->so_state & (SS_ISCONNECTED|SS_ISCONNECTING) &&
     ((so->so_proto->pr_flags & PR_CONNREQUIRED) ||
     (error = sodisconnect(so))))
  error = EISCONN;
- else
+ else if (!(so->so_type == SOCK_DGRAM && nam2sa(nam, &sa) == 0 &&
+    sa->sa_family == AF_UNSPEC))
  error = (*so->so_proto->pr_usrreq)(so, PRU_CONNECT,
     NULL, nam, NULL, curproc);
  return (error);
@@ -1965,6 +1970,19 @@ sohasoutofband(struct socket *so)
 }
 
 int
+nam2sa(struct mbuf *nam, struct sockaddr **soa)
+{
+ struct sockaddr *sa = mtod(nam, struct sockaddr *);
+
+ if (nam->m_len < offsetof(struct sockaddr, sa_data))
+ return EINVAL;
+ if (sa->sa_len != nam->m_len)
+ return EINVAL;
+ *soa = sa;
+ return 0;
+}
+
+int
 soo_kqfilter(struct file *fp, struct knote *kn)
 {
  struct socket *so = kn->kn_fp->f_data;
--- lib/libc/sys/connect.2 2019-08-10 17:34:42 +0000
+++ lib/libc/sys/connect.2 2019-08-10 17:38:22 +0000
@@ -73,7 +73,7 @@ only once; datagram sockets may use
 .Fn connect
 multiple times to change their association.
 Datagram sockets may dissolve the association
-by connecting to an invalid address, such as a null address.
+by connecting to an address of family AF_UNSPEC.
 .Pp
 If the socket is in non-blocking mode and the connection cannot be
 completed immediately, or if it is interrupted by a signal,
--- usr.bin/nc/netcat.c 2019-08-09 17:30:13 +0000
+++ usr.bin/nc/netcat.c 2019-08-10 17:47:16 +0000
@@ -629,7 +629,9 @@ main(int argc, char *argv[])
  tls_free(tls_cctx);
  }
  if (family == AF_UNIX && uflag) {
- if (connect(s, NULL, 0) == -1)
+ /* AF_UNSPEC == 0 */
+ if (connect(s, &(struct sockaddr){0},
+    sizeof(struct sockaddr)) == -1)
  err(1, "connect");
  }
 

Reply | Threaded
Open this post in threaded view
|

Re: Datagram disassociation should be done via AF_UNSPEC

astian
Ping?

A week ago I also create 6 other threads about netcat, I wonder if
they were received.

Cheers.

astian:

> (Sorry if you are getting this message duplicated, I tried sending it to
> tech@ before but it seems to get stuck somewhere along the way.)
>
> I found this while looking at netcat and it seemed odd:
>
> if (family == AF_UNIX && uflag) {
> if (connect(s, NULL, 0) == -1)
> err(1, "connect");
> }
>
> POSIX 2008 [0] says:
>
>   For SOCK_DGRAM sockets, [...] If the sa_family member of address is
>   AF_UNSPEC, the socket's peer address shall be reset.
>
> It also says:
>
>   Issue 7
>   [...]
>     - Austin Group Interpretation 1003.1-2001 #188 is applied, changing
>       the method used to reset a peer address for a datagram socket.
>   [...]
>
> Indeed, in POSIX 2004 [1] it said:
>
>   For SOCK_DGRAM sockets, [...] If address is a null address for the
>   protocol, the socket's peer address shall be reset.
>
> Which is similar to connect(2):
>
>   Datagram sockets may dissolve the association by connecting to an
>   invalid address, such as a null address.
>
> Now, I don't know if "null address for the protocol" was at any point in
> time supposed to be interpreted as NULL-pointer/zero-size as opposed to
> zeroed structure for example (notice that AF_UNSPEC is 0) or whether
> such a thing ever worked in OpenBSD, but it certainly doesn't now and
> hasn't for a while: it fails in sys_connect -> sockargs when checking
> for the sockaddr size.
>
> OpenBSD does support disassociating via a zeroed sockaddr, but the
> syscall fails and it shouldn't; in fact, it supports disassociating by
> using *any* sort of address as long as it's consistent in terms of size
> because the disassociation (sodisconnect) happens before even looking at
> the user-supplied sockaddr.  I believe this should to be cleaned up but
> it is a change in syscall semantics (according to CVS this code,
> soconnect, mostly comes directly from NetBSD from 23 years ago).
>
> I did not look at what other BSD do, but below is a possible minimal
> patch that aims to retain most of the current behaviour, which might or
> might not be TRTTD.  It should apply to "current".
>
> Cheers.
>
> 0: https://pubs.opengroup.org/onlinepubs/9699919799/functions/connect.html
> 1: https://pubs.opengroup.org/onlinepubs/009695399/functions/connect.html
>
> --- sys/kern/uipc_socket.c 2019-08-09 17:27:45 +0000
> +++ sys/kern/uipc_socket.c 2019-08-10 17:41:07 +0000
> @@ -66,6 +66,8 @@ void soreaper(void *);
>  void soput(void *);
>  int somove(struct socket *, int);
>  
> +int nam2sa(struct mbuf *, struct sockaddr **);
> +
>  void filt_sordetach(struct knote *kn);
>  int filt_soread(struct knote *kn, long hint);
>  void filt_sowdetach(struct knote *kn);
> @@ -344,7 +346,8 @@ soaccept(struct socket *so, struct mbuf
>  int
>  soconnect(struct socket *so, struct mbuf *nam)
>  {
> - int error;
> + struct sockaddr *sa;
> + int error = 0;
>  
>   soassertlocked(so);
>  
> @@ -353,14 +356,16 @@ soconnect(struct socket *so, struct mbuf
>   /*
>   * If protocol is connection-based, can only connect once.
>   * Otherwise, if connected, try to disconnect first.
> - * This allows user to disconnect by connecting to, e.g.,
> - * a null address.
> + * This allows the user to disconnect by connecting to an invalid
> + * address, although only a "zero" address (AF_UNSPEC) will not
> + * result in error.
>   */
>   if (so->so_state & (SS_ISCONNECTED|SS_ISCONNECTING) &&
>      ((so->so_proto->pr_flags & PR_CONNREQUIRED) ||
>      (error = sodisconnect(so))))
>   error = EISCONN;
> - else
> + else if (!(so->so_type == SOCK_DGRAM && nam2sa(nam, &sa) == 0 &&
> +    sa->sa_family == AF_UNSPEC))
>   error = (*so->so_proto->pr_usrreq)(so, PRU_CONNECT,
>      NULL, nam, NULL, curproc);
>   return (error);
> @@ -1965,6 +1970,19 @@ sohasoutofband(struct socket *so)
>  }
>  
>  int
> +nam2sa(struct mbuf *nam, struct sockaddr **soa)
> +{
> + struct sockaddr *sa = mtod(nam, struct sockaddr *);
> +
> + if (nam->m_len < offsetof(struct sockaddr, sa_data))
> + return EINVAL;
> + if (sa->sa_len != nam->m_len)
> + return EINVAL;
> + *soa = sa;
> + return 0;
> +}
> +
> +int
>  soo_kqfilter(struct file *fp, struct knote *kn)
>  {
>   struct socket *so = kn->kn_fp->f_data;
> --- lib/libc/sys/connect.2 2019-08-10 17:34:42 +0000
> +++ lib/libc/sys/connect.2 2019-08-10 17:38:22 +0000
> @@ -73,7 +73,7 @@ only once; datagram sockets may use
>  .Fn connect
>  multiple times to change their association.
>  Datagram sockets may dissolve the association
> -by connecting to an invalid address, such as a null address.
> +by connecting to an address of family AF_UNSPEC.
>  .Pp
>  If the socket is in non-blocking mode and the connection cannot be
>  completed immediately, or if it is interrupted by a signal,
> --- usr.bin/nc/netcat.c 2019-08-09 17:30:13 +0000
> +++ usr.bin/nc/netcat.c 2019-08-10 17:47:16 +0000
> @@ -629,7 +629,9 @@ main(int argc, char *argv[])
>   tls_free(tls_cctx);
>   }
>   if (family == AF_UNIX && uflag) {
> - if (connect(s, NULL, 0) == -1)
> + /* AF_UNSPEC == 0 */
> + if (connect(s, &(struct sockaddr){0},
> +    sizeof(struct sockaddr)) == -1)
>   err(1, "connect");
>   }
>  
>
>