Timothy Redaelli via dev <[email protected]> writes:
> Add a new "pfd:" passive stream class that accepts a pre-opened file
> descriptor number. This is the core building block for systemd socket
> activation, where systemd opens and binds the listening socket before
> starting the service.
>
> The pfd_open() function validates that the file descriptor refers to
> a listening stream socket via getsockopt(SO_TYPE) and
> getsockopt(SO_ACCEPTCONN), sets it non-blocking, and wraps it in an
> fd_pstream. Unlike punix:, the unlink_path is NULL because the
> service does not own the socket file.
>
> For security, pfd: remotes are restricted to the command line
> (--remote=pfd:N). Runtime addition via ovsdb-server/add-remote
> or the database is rejected at three entry points
> (ovsdb_server_add_remote, add_manager_options, query_db_remotes),
> preventing an attacker with database write access from hijacking
> arbitrary file descriptors.
>
> Reported-at: https://issues.redhat.com/browse/FDP-3413
> Co-authored-by: Lubomir Rintel <[email protected]>
> Signed-off-by: Lubomir Rintel <[email protected]>
> Signed-off-by: Timothy Redaelli <[email protected]>
> ---
> Documentation/ref/ovsdb.7.rst | 12 ++++++++
> lib/stream-provider.h | 1 +
> lib/stream-unix.c | 53 +++++++++++++++++++++++++++++++++++
> lib/stream.c | 5 ++++
> ovsdb/ovsdb-server.c | 23 ++++++++++++++-
> 5 files changed, 93 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ref/ovsdb.7.rst b/Documentation/ref/ovsdb.7.rst
> index 42541dd7e..cf1ef3736 100644
> --- a/Documentation/ref/ovsdb.7.rst
> +++ b/Documentation/ref/ovsdb.7.rst
> @@ -709,6 +709,18 @@ punix:<file>
> <file> to mimic the behavior of a Unix domain socket. The ACLs of the
> named
> pipe include LocalSystem, Administrators, and Creator Owner.
>
> +pfd:<fd>
> + Listen on a pre-opened file descriptor <fd>. The file descriptor must
> + refer to a bound, listening Unix domain stream socket. This is intended
> + for use with systemd socket activation, where systemd opens the socket
> + and passes it to the service.
> +
> + For security, ``pfd:`` may only be specified on the command line
> + (``--remote=pfd:<fd>``). It is rejected if added at runtime via
> + ``ovsdb-server/add-remote`` or through the database.
> +
> + This connection method is not supported on Windows.
> +
> All IP-based connection methods accept IPv4 and IPv6 addresses. To specify
> an
> IPv6 address, wrap it in square brackets, e.g. ``ssl:[::1]:6640``. Passive
> IP-based connection methods by default listen for IPv4 connections only; use
> diff --git a/lib/stream-provider.h b/lib/stream-provider.h
> index 44e3c6431..ddd468b09 100644
> --- a/lib/stream-provider.h
> +++ b/lib/stream-provider.h
> @@ -195,6 +195,7 @@ extern const struct pstream_class ptcp_pstream_class;
> #ifndef _WIN32
> extern const struct stream_class unix_stream_class;
> extern const struct pstream_class punix_pstream_class;
> +extern const struct pstream_class pfd_pstream_class;
> #else
> extern const struct stream_class windows_stream_class;
> extern const struct pstream_class pwindows_pstream_class;
> diff --git a/lib/stream-unix.c b/lib/stream-unix.c
> index d265efb83..5cb4e93d2 100644
> --- a/lib/stream-unix.c
> +++ b/lib/stream-unix.c
> @@ -136,3 +136,56 @@ const struct pstream_class punix_pstream_class = {
> NULL,
> };
>
> +/* Pre-opened file descriptor passive stream.
> + *
> + * Used for systemd socket activation: systemd opens and binds the socket,
> + * then passes it to the service as a pre-opened file descriptor. */
> +
> +static int
> +pfd_open(const char *name, char *suffix,
> + struct pstream **pstreamp, uint8_t dscp OVS_UNUSED)
> +{
> + char *end;
> + long fd;
> +
> + fd = strtol(suffix, &end, 10);
> + if (*end != '\0' || fd < 0 || fd > INT_MAX) {
> + VLOG_ERR("%s: bad file descriptor", name);
> + return ERANGE;
> + }
I would change this to:
if (!str_to_long(suffix, 10, &fd) || fd < 0) {
VLOG_ERR("%s: bad file descriptor", name);
return EINVAL;
}
> + /* Verify it is a listening stream socket. */
> + int sock_type;
> + socklen_t len = sizeof sock_type;
> + if (getsockopt(fd, SOL_SOCKET, SO_TYPE, &sock_type, &len)) {
> + VLOG_ERR("%s: not a socket (%s)", name, ovs_strerror(errno));
> + return errno;
> + }
> + if (sock_type != SOCK_STREAM) {
> + VLOG_ERR("%s: not a stream socket (type %d)", name, sock_type);
> + return EINVAL;
> + }
> + int listening;
> + len = sizeof listening;
> + if (getsockopt(fd, SOL_SOCKET, SO_ACCEPTCONN, &listening, &len)
> + || !listening) {
> + VLOG_ERR("%s: not a listening socket", name);
> + return EINVAL;
> + }
> +
> + int error = set_nonblocking(fd);
> + if (error) {
> + return error;
> + }
> +
> + return new_fd_pstream(xstrdup(name), fd, punix_accept, NULL, pstreamp);
Should we also drain down the socket when it gets opened? What I mean
is, in case we crash and systemd restarts us does the socket that the
service hold actually drain us down? I guess that isn't clear to me and
I worry about some kind of invalid data sitting in the queue.
I also wonder if we should also do something like
recv(,MSG_DONTWAIT|MSG_PEEK) and make sure that the socket is still
open. The manpage from systemd seems to imply that if the socket gets
closed the fd itself is no longer considered in a good state. I'm not
sure how the systemd service works for it.
> +}
> +
> +const struct pstream_class pfd_pstream_class = {
> + "pfd",
> + false,
> + pfd_open,
> + NULL,
> + NULL,
> + NULL,
> +};
> diff --git a/lib/stream.c b/lib/stream.c
> index feaa1cb2d..b3b21588a 100644
> --- a/lib/stream.c
> +++ b/lib/stream.c
> @@ -69,6 +69,7 @@ static const struct pstream_class *pstream_classes[] = {
> &ptcp_pstream_class,
> #ifndef _WIN32
> &punix_pstream_class,
> + &pfd_pstream_class,
> #else
> &pwindows_pstream_class,
> #endif
> @@ -147,6 +148,10 @@ stream_usage(const char *name, bool active, bool passive,
> #endif
> printf(" punix:FILE "
> "listen on Unix domain socket FILE\n");
> +#ifndef _WIN32
> + printf(" pfd:FD "
> + "listen on pre-opened file descriptor FD\n");
> +#endif
> }
>
> #ifdef HAVE_OPENSSL
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index 7c3a5ef11..2af62071e 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -1425,6 +1425,12 @@ add_manager_options(struct shash *remotes, const
> struct ovsdb_row *row)
> return;
> }
>
> + if (!strncmp("pfd:", target, 4)) {
> + VLOG_WARN_RL(&rl, "pfd: remotes can only be specified on the "
> + "command line; ignoring \"%s\" from database", target);
> + return;
> + }
> +
> options = add_remote(remotes, target, NULL);
> if (ovsdb_util_read_integer_column(row, "max_backoff", &max_backoff)) {
> options->rpc.max_backoff = max_backoff;
> @@ -1485,7 +1491,16 @@ query_db_remotes(const char *name, const struct shash
> *all_dbs,
>
> datum = &row->fields[column->index];
> for (i = 0; i < datum->n; i++) {
> - add_remote(remotes, json_string(datum->keys[i].s), NULL);
> + const char *t = json_string(datum->keys[i].s);
> + if (!strncmp("pfd:", t, 4)) {
> + static struct vlog_rate_limit pfd_rl
> + = VLOG_RATE_LIMIT_INIT(1, 1);
> + VLOG_WARN_RL(&pfd_rl, "pfd: remotes can only be "
> + "specified on the command line; ignoring "
> + "\"%s\" from database", t);
> + continue;
> + }
> + add_remote(remotes, t, NULL);
> }
> }
> } else if (column->type.key.type == OVSDB_TYPE_UUID
> @@ -2291,6 +2306,12 @@ ovsdb_server_add_remote(struct unixctl_conn *conn, int
> argc OVS_UNUSED,
> return;
> }
>
> + if (!strncmp("pfd:", remote, 4)) {
> + unixctl_command_reply_error(conn,
> + "pfd: remotes can only be specified on the command line");
> + return;
> + }
> +
> retval = (strncmp("db:", remote, 3)
> ? NULL
> : parse_db_column(config->all_dbs, remote,
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
