On Thu, 2020-09-17 at 09:51 +0200, Ondrej Mosnacek wrote:
> On Wed, Aug 26, 2020 at 2:37 PM Richard Haines
> <richard_c_haines@xxxxxxxxxxxxxx> wrote:
> > Reviewed the tests using kernel tree:
> > Documentation/security/SCTP.rst, this
> > added one additional test.
> >
> > The main changes have been to sctp_asconf_params_client.c and
> > sctp_asconf_params_server.c to support the tests, and also to make
> > them
> > more reliable for running the client and server on different
> > systems.
> >
> > Updated common code in sctp_common.c for sctp event handling and
> > updated
> > relevant programs to use handle_event()
> >
> > Removed obsolete code/policy.
> >
> > Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> > ---
> > policy/test_sctp.te | 75 ++++-
> > tests/sctp/.gitignore | 1 -
> > tests/sctp/Makefile | 3 +-
> > tests/sctp/sctp_asconf_params_client.c | 322 ++++++++-----------
> > tests/sctp/sctp_asconf_params_server.c | 275 +++++++++-------
> > tests/sctp/sctp_common.c | 189 ++++++++++-
> > tests/sctp/sctp_common.h | 12 +-
> > tests/sctp/sctp_peeloff_server.c | 42 +--
> > tests/sctp/sctp_server.c | 4 +-
> > tests/sctp/sctp_set_peer_addr.c | 415 -------------------
> > ------
> > tests/sctp/test | 70 ++++-
> > 11 files changed, 623 insertions(+), 785 deletions(-)
> > delete mode 100644 tests/sctp/sctp_set_peer_addr.c
> >
> [...]
> > diff --git a/tests/sctp/.gitignore b/tests/sctp/.gitignore
> > index 8671c27..c022b11 100644
> > --- a/tests/sctp/.gitignore
> > +++ b/tests/sctp/.gitignore
> > @@ -6,4 +6,3 @@ sctp_client
> > sctp_connectx
> > sctp_peeloff_server
> > sctp_server
> > -sctp_set_peer_addr
> > diff --git a/tests/sctp/Makefile b/tests/sctp/Makefile
> > index f5dfdae..fbe8fb0 100644
> > --- a/tests/sctp/Makefile
> > +++ b/tests/sctp/Makefile
> > @@ -1,4 +1,5 @@
> > -TARGETS = sctp_client sctp_server sctp_bind sctp_bindx
> > sctp_connectx sctp_set_peer_addr sctp_asconf_params_client
> > sctp_asconf_params_server sctp_peeloff_server
> > +TARGETS = sctp_client sctp_server sctp_bind sctp_bindx
> > sctp_connectx \
> > +sctp_asconf_params_client sctp_asconf_params_server
> > sctp_peeloff_server
>
> Minor nit: please indent the continuation line (with 1 tab).
>
> > DEPS = sctp_common.c sctp_common.h
> > CFLAGS ?= -Wall
> > diff --git a/tests/sctp/sctp_asconf_params_client.c
> > b/tests/sctp/sctp_asconf_params_client.c
> > index 12522f3..ada5982 100644
> > --- a/tests/sctp/sctp_asconf_params_client.c
> > +++ b/tests/sctp/sctp_asconf_params_client.c
> > @@ -29,11 +29,9 @@
> > static void usage(char *progname)
> > {
> > fprintf(stderr,
> > - "usage: %s [-v] [-n] addr port\n"
> > + "usage: %s [-v] addr port\n"
> > "\nWhere:\n\t"
> > "-v Print status information.\n\t"
> > - "-n No bindx_rem will be received from server.
> > This happens\n\t"
> > - " when the client and server are on different
> > systems.\n\t"
> > "addr IPv4 or IPv6 address (MUST NOT be loopback
> > address).\n\t"
> > "port port.\n", progname);
> >
> > @@ -46,75 +44,90 @@ static void usage(char *progname)
> > exit(1);
> > }
> >
> > -static int peer_count, peer_count_err;
> > -static void getpaddrs_alarm(int sig)
> > -{
> > - fprintf(stderr,
> > - "Get peer address count timer expired - carry on
> > test\n");
> > - peer_count += 1;
> > - peer_count_err = true;
> > -}
> > -
> > -static void getprimaddr_alarm(int sig)
> > -{
> > - fprintf(stderr, "Get primary address timer expired - end
> > test.\n");
> > - exit(1);
> > -}
> > -
> > -static void get_primaddr(char *addr_buf, int socket)
> > +static int get_set_primaddr(int socket, sctp_assoc_t id, bool
> > verbose)
> > {
> > int result;
> > - struct sctp_prim prim;
> > - struct sockaddr_in *in_addr;
> > - struct sockaddr_in6 *in6_addr;
> > - struct sockaddr *paddr;
> > + struct sctp_prim prim; /* Defined in linux/sctp.t */
>
> linux/sctp.t -> linux/sctp.h
>
> > socklen_t prim_len;
> > - const char *addr_ptr = NULL;
> > +
> > + /*
> > + * At this point the new primary address is already set. To
> > test the
> > + * bind permission, just reset the address.
> > + */
> >
> > memset(&prim, 0, sizeof(struct sctp_prim));
> > prim_len = sizeof(struct sctp_prim);
> > + prim.ssp_assoc_id = id;
> >
> > result = getsockopt(socket, IPPROTO_SCTP,
> > SCTP_PRIMARY_ADDR,
> > &prim, &prim_len);
> > if (result < 0) {
> > - perror("getsockopt: SCTP_PRIMARY_ADDR");
> > - exit(1);
> > + perror("Client getsockopt: SCTP_PRIMARY_ADDR");
> > + return 50;
> > }
> >
> > - paddr = (struct sockaddr *)&prim.ssp_addr;
> > - if (paddr->sa_family == AF_INET) {
> > - in_addr = (struct sockaddr_in *)&prim.ssp_addr;
> > - addr_ptr = inet_ntop(AF_INET, &in_addr->sin_addr,
> > addr_buf,
> > - INET6_ADDRSTRLEN);
> > - } else if (paddr->sa_family == AF_INET6) {
> > - in6_addr = (struct sockaddr_in6 *)&prim.ssp_addr;
> > - addr_ptr = inet_ntop(AF_INET6, &in6_addr-
> > >sin6_addr, addr_buf,
> > - INET6_ADDRSTRLEN);
> > + if (verbose)
> > + print_addr_info((struct sockaddr *)&prim.ssp_addr,
> > + "Client getsockopt -
> > SCTP_PRIMARY_ADDR:");
> > +
> > + /*
> > + * If the new primary address is an IPv6 local link
> > address, it will
> > + * have been received by the DAR process with a scope id of
> > '0'.
> > + * Therefore when the setsockopt is called it will error
> > with EINVAL.
> > + * To resolve this set scope_id=1 (first link) before the
> > call.
> > + */
> > + struct sockaddr_in6 *addr6;
> > + struct sockaddr *sin;
> > +
> > + sin = (struct sockaddr *)&prim.ssp_addr;
> > +
> > + if (sin->sa_family == AF_INET6) {
> > + addr6 = (struct sockaddr_in6 *)sin;
> > + if (IN6_IS_ADDR_LINKLOCAL(&addr6->sin6_addr) &&
> > + ((struct sockaddr_in6 *)addr6)->sin6_scope_id
> > == 0) {
> > + ((struct sockaddr_in6 *)addr6)-
> > >sin6_scope_id = 1;
> > + if (verbose)
> > + printf("Client set new Local Link
> > primary address scope_id=1\n");
> > + }
> > }
> > - if (!addr_ptr) {
> > - perror("inet_ntop");
> > - exit(1);
> > +
> > + /*
> > + * This tests the net/sctp/socket.c
> > sctp_setsockopt_primary_addr()
> > + * SCTP_PRIMARY_ADDR function by setting policy to:
> > + * allow sctp_asconf_params_client_t self:sctp_socket {
> > bind };
> > + * or:
> > + * neverallow sctp_asconf_params_client_t
> > self:sctp_socket { bind };
> > + */
> > + result = setsockopt(socket, IPPROTO_SCTP,
> > SCTP_PRIMARY_ADDR,
> > + &prim, prim_len);
> > + if (result < 0) {
> > + perror("Client setsockopt: SCTP_PRIMARY_ADDR");
> > + return 51;
> > }
> > + if (verbose)
> > + print_addr_info((struct sockaddr *)&prim.ssp_addr,
> > + "Client set SCTP_PRIMARY_ADDR
> > to:");
> > + return 0;
> > }
> >
> > int main(int argc, char **argv)
> > {
> > - int opt, client_sock, result, len;
> > + int opt, client_sock, result, flags = 0, on = 1;
> > struct addrinfo client_hints, *client_res;
> > - struct sockaddr *paddrs;
> > - bool verbose = false, no_bindx_rem = false;
> > - char client_prim_addr1[INET6_ADDRSTRLEN];
> > - char client_prim_addr2[INET6_ADDRSTRLEN];
> > - char buffer[1024];
> > -
> > - while ((opt = getopt(argc, argv, "vn")) != -1) {
> > + struct sctp_sndrcvinfo sinfo;
> > + struct sockaddr_storage sin;
> > + socklen_t sinlen = sizeof(sin);
> > + struct timeval tm;
> > + bool verbose = false;
> > + char buffer[512];
> > + char msg[] = "Send peer address";
> > + char *rcv_new_addr_buf = NULL;
> > +
> > + while ((opt = getopt(argc, argv, "v")) != -1) {
> > switch (opt) {
> > case 'v':
> > verbose = true;
> > break;
> > - case 'n':
> > - no_bindx_rem = true;
> > - break;
> > default:
> > usage(argv[0]);
> > }
> > @@ -123,176 +136,117 @@ int main(int argc, char **argv)
> > if ((argc - optind) != 2)
> > usage(argv[0]);
> >
> > - /* Set up client side and connect */
> > memset(&client_hints, 0, sizeof(struct addrinfo));
> > - client_hints.ai_socktype = SOCK_STREAM;
> > + client_hints.ai_socktype = SOCK_SEQPACKET;
> > client_hints.ai_protocol = IPPROTO_SCTP;
> > result = getaddrinfo(argv[optind], argv[optind + 1],
> > &client_hints, &client_res);
> > if (result < 0) {
> > - fprintf(stderr, "getaddrinfo - client: %s\n",
> > + fprintf(stderr, "Client getaddrinfo err: %s\n",
> > gai_strerror(result));
> > exit(1);
> > }
> >
> > -
> > - /* printf("Client scopeID: %d\n",
> > - * ((struct sockaddr_in6 *)client_res->ai_addr)-
> > >sin6_scope_id);
> > - */
> > -
> > client_sock = socket(client_res->ai_family, client_res-
> > >ai_socktype,
> > client_res->ai_protocol);
> > if (client_sock < 0) {
> > - perror("socket");
> > + perror("Client socket");
> > + freeaddrinfo(client_res);
> > exit(1);
> > }
> >
> > - result = connect(client_sock, client_res->ai_addr,
> > - client_res->ai_addrlen);
> > + /* Need to set a timeout if no reply from server */
> > + memset(&tm, 0, sizeof(struct timeval));
> > + tm.tv_sec = 1;
>
> Please use a higher value here (I'd recommend at least 3 seconds) -
> see:
> https://lore.kernel.org/selinux/20200827081100.1954467-1-omosnace@xxxxxxxxxx/
>
> [...]
>
> Other than the minor stuff above this looks good to me. Thank you for
> the patch and for bearing with my slow reviews :)
I'm in no hurry. I'll post an updated patch next week.
>
> --
> Ondrej Mosnacek
> Software Engineer, Platform Security - SELinux kernel
> Red Hat, Inc.
>
