Hi Richard,
On Wed, Jun 3, 2020 at 1:01 PM Richard Haines
<richard_c_haines@xxxxxxxxxxxxxx> wrote:
>
> The main changes have been to sctp_asconf_params_client.c and
> sctp_asconf_params_server.c to make them more reliable for running the
> client and server on different systems.
>
> Updated common code in sctp_common.c
>
> Removed obsolete code that was used to test permissions that never made it
> to the final commit.
>
> Also reviewed and updated tests to check policy rules for denying
> access to all possible net/sctp kernel hooks as described in the kernel
> Documentation/security/SCTP.rst
This looks promising, but would it be possible to split the patch into
a series of smaller changes? It is hard to review such a large patch
that changes a lot of things at once...
>
> Signed-off-by: Richard Haines <richard_c_haines@xxxxxxxxxxxxxx>
> ---
> policy/test_sctp.te | 98 ++++--
> tests/sctp/.gitignore | 3 -
> tests/sctp/Makefile | 2 +-
> tests/sctp/sctp_asconf_params_client.c | 320 ++++++++-----------
> tests/sctp/sctp_asconf_params_server.c | 269 +++++++++-------
> tests/sctp/sctp_client.c | 4 +-
> tests/sctp/sctp_common.c | 189 ++++++++++-
> tests/sctp/sctp_common.h | 12 +-
> tests/sctp/sctp_connectx.c | 33 +-
> tests/sctp/sctp_peeloff_server.c | 42 +--
> tests/sctp/sctp_server.c | 4 +-
> tests/sctp/sctp_set_params.c | 205 ------------
> tests/sctp/sctp_set_peer_addr.c | 415 -------------------------
> tests/sctp/sctp_set_pri_addr.c | 135 --------
> tests/sctp/test | 95 +++++-
> 15 files changed, 688 insertions(+), 1138 deletions(-)
> delete mode 100644 tests/sctp/sctp_set_params.c
> delete mode 100644 tests/sctp/sctp_set_peer_addr.c
> delete mode 100644 tests/sctp/sctp_set_pri_addr.c
>
> diff --git a/policy/test_sctp.te b/policy/test_sctp.te
> index 3b16db1..380e442 100644
> --- a/policy/test_sctp.te
> +++ b/policy/test_sctp.te
> @@ -60,6 +60,12 @@ corenet_sctp_bind_all_nodes(test_sctp_server_t)
> corenet_inout_generic_node(test_sctp_server_t)
> corenet_inout_generic_if(test_sctp_server_t)
>
> +# Don't allow ports < 1024
> +# neverallow test_sctp_server_t self:capability net_bind_service
> +# neverallow test_sctp_server_t reserved_port_t:sctp_socket name_bind;
> +dontaudit test_sctp_server_t self:netlink_route_socket { create getattr bind };
> +dontaudit test_sctp_server_t self:udp_socket { getattr connect };
Could you clarify what these dontaudit rules are for?
That's the only thing that caught my eye after a quick glance. I'm
still hoping you will be able to split up the patch into more
digestible chunks, so I'm not going to look at it deeper just yet. If
that proves impossible or too difficult, I'll try to review the patch
as-is, but please understand that it may take a long time then...
Thanks,
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
