Libguestfs - Mar 2020 - Re: [PATCH nbdkit v2] New filter: limit: Limit number of clients that can connect.

This is a second version of the limit filter.  v1 was posted here:
https://www.redhat.com/archives/libguestfs/2020-March/msg00015.html
I didn't bother to repost the other three patches because they are the
same.

The difference is this version of the filter takes security more
seriously.  It shouldn't be possible for malicious clients to connect
more than limit=N times to the plugin now, which should be the main
"threat" that this filter protects against.

However malicious clients could still play tricks at the TCP level
(eg. half-opened connections), connect as many times as they want to
the server, and have as many NBD negotiations going on (albeit
slowly).  It would be possible to mitigate this further in the filter
by counting "preconnected" clients separately, but the filter does not
do this at present.  Most likely if you were serious about this you'd
want some other kind of protection outside the server.

I'll note here that nbdkit has no time limit on NBD protocol
negotiation.  Possibly it should.

$ nbdkit null
$ telnet localhost 10809
Trying ::1...
Connected to localhost.
Escape character is '^]'.
NBDMAGICIHAVEOPT       <--- sits forever

Rich.

---
 docs/nbdkit-service.pod                     |   1 +
 filters/exitlast/nbdkit-exitlast-filter.pod |   1 +
 filters/ip/nbdkit-ip-filter.pod             |   1 +
 filters/limit/nbdkit-limit-filter.pod       |  61 +++++++++
 filters/rate/nbdkit-rate-filter.pod         |   1 +
 configure.ac                                |   2 +
 filters/limit/Makefile.am                   |  66 ++++++++++
 tests/Makefile.am                           |   3 +
 filters/limit/limit.c                       | 129 ++++++++++++++++++++
 tests/test-limit.sh                         |  81 ++++++++++++
 TODO                                        |   2 -
 11 files changed, 346 insertions(+), 2 deletions(-)

diff --git a/docs/nbdkit-service.pod b/docs/nbdkit-service.pod
index ecc8e94b..62ce7831 100644
--- a/docs/nbdkit-service.pod
+++ b/docs/nbdkit-service.pod
@@ -145,6 +145,7 @@ L</SOCKET ACTIVATION>.
 L<nbdkit(1)>,
 L<nbdkit-exitlast-filter(1)>,
 L<nbdkit-ip-filter(1)>,
+L<nbdkit-limit-filter(1)>,
 L<systemd(1)>,
 L<systemd.socket(5)>,
 L<syslog(3)>,
diff --git a/filters/exitlast/nbdkit-exitlast-filter.pod
b/filters/exitlast/nbdkit-exitlast-filter.pod
index e0fbd6ea..207ad4c8 100644
--- a/filters/exitlast/nbdkit-exitlast-filter.pod
+++ b/filters/exitlast/nbdkit-exitlast-filter.pod
@@ -43,6 +43,7 @@ C<nbdkit-exitlast-filter> first appeared in nbdkit 1.20.
 
 L<nbdkit(1)>,
 L<nbdkit-ip-filter(1)>,
+L<nbdkit-limit-filter(1)>,
 L<nbdkit-rate-filter(1)>,
 L<nbdkit-captive(1)>,
 L<nbdkit-service(1)>,
diff --git a/filters/ip/nbdkit-ip-filter.pod b/filters/ip/nbdkit-ip-filter.pod
index 1621ebec..8ff98651 100644
--- a/filters/ip/nbdkit-ip-filter.pod
+++ b/filters/ip/nbdkit-ip-filter.pod
@@ -146,6 +146,7 @@ C<nbdkit-ip-filter> first appeared in nbdkit 1.18.
 
 L<nbdkit(1)>,
 L<nbdkit-exitlast-filter(1)>,
+L<nbdkit-limit-filter(1)>,
 L<nbdkit-filter(3)>.
 
 =head1 AUTHORS
diff --git a/filters/limit/nbdkit-limit-filter.pod
b/filters/limit/nbdkit-limit-filter.pod
new file mode 100644
index 00000000..e334decb
--- /dev/null
+++ b/filters/limit/nbdkit-limit-filter.pod
@@ -0,0 +1,61 @@
+=head1 NAME
+
+nbdkit-limit-filter - limit number of clients that can connect
+
+=head1 SYNOPSIS
+
+ nbdkit --filter=limit PLUGIN [limit=N]
+
+=head1 DESCRIPTION
+
+C<nbdkit-limit-filter> is an nbdkit filter that limits the number of
+clients which can connect concurrently.  If more than C<limit=N>
+(default: 1) clients try to connect at the same time then later
+clients are rejected.
+
+=head1 PARAMETERS
+
+=over 4
+
+=item B<limit=>N
+
+Limit the number of concurrent clients to C<N>.  This parameter is
+optional.  If not specified then the limit defaults to 1.  You can
+also set this to 0 to make the number of clients unlimited (ie.
+disable the filter).
+
+=back
+
+=head1 FILES
+
+=over 4
+
+=item F<$filterdir/nbdkit-limit-filter.so>
+
+The filter.
+
+Use C<nbdkit --dump-config> to find the location of C<$filterdir>.
+
+=back
+
+=head1 VERSION
+
+C<nbdkit-limit-filter> first appeared in nbdkit 1.20.
+
+=head1 SEE ALSO
+
+L<nbdkit(1)>,
+L<nbdkit-exitlast-filter(1)>,
+L<nbdkit-ip-filter(1)>,
+L<nbdkit-noparallel-filter(1)>,
+L<nbdkit-rate-filter(1)>,
+L<nbdkit-filter(3)>,
+L<nbdkit-plugin(3)>.
+
+=head1 AUTHORS
+
+Richard W.M. Jones
+
+=head1 COPYRIGHT
+
+Copyright (C) 2020 Red Hat Inc.
diff --git a/filters/rate/nbdkit-rate-filter.pod
b/filters/rate/nbdkit-rate-filter.pod
index acf7d8b0..fc5f211d 100644
--- a/filters/rate/nbdkit-rate-filter.pod
+++ b/filters/rate/nbdkit-rate-filter.pod
@@ -130,6 +130,7 @@ L<nbdkit(1)>,
 L<nbdkit-blocksize-filter(1)>,
 L<nbdkit-delay-filter(1)>,
 L<nbdkit-exitlast-filter(1)>,
+L<nbdkit-limit-filter(1)>,
 L<nbdkit-filter(3)>,
 L<iptables(8)>,
 L<tc(8)>.
diff --git a/configure.ac b/configure.ac
index 0b980238..2db4b546 100644
--- a/configure.ac
+++ b/configure.ac
@@ -102,6 +102,7 @@ filters="\
         extentlist \
         fua \
         ip \
+        limit \
         log \
         nocache \
         noextents \
@@ -1029,6 +1030,7 @@ AC_CONFIG_FILES([Makefile
                  filters/extentlist/Makefile
                  filters/fua/Makefile
                  filters/ip/Makefile
+                 filters/limit/Makefile
                  filters/log/Makefile
                  filters/nocache/Makefile
                  filters/noextents/Makefile
diff --git a/filters/limit/Makefile.am b/filters/limit/Makefile.am
new file mode 100644
index 00000000..14d53ec4
--- /dev/null
+++ b/filters/limit/Makefile.am
@@ -0,0 +1,66 @@
+# nbdkit
+# Copyright (C) 2019-2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+include $(top_srcdir)/common-rules.mk
+
+EXTRA_DIST = nbdkit-limit-filter.pod
+
+filter_LTLIBRARIES = nbdkit-limit-filter.la
+
+nbdkit_limit_filter_la_SOURCES = \
+	limit.c \
+	$(top_srcdir)/include/nbdkit-filter.h \
+	$(NULL)
+
+nbdkit_limit_filter_la_CPPFLAGS = \
+	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/common/utils \
+	$(NULL)
+nbdkit_limit_filter_la_CFLAGS = $(WARNINGS_CFLAGS)
+nbdkit_limit_filter_la_LDFLAGS = \
+	-module -avoid-version -shared \
+	-Wl,--version-script=$(top_srcdir)/filters/filters.syms \
+	$(NULL)
+nbdkit_limit_filter_la_LIBADD = \
+	$(top_builddir)/common/utils/libutils.la \
+	$(NULL)
+
+if HAVE_POD
+
+man_MANS = nbdkit-limit-filter.1
+CLEANFILES += $(man_MANS)
+
+nbdkit-limit-filter.1: nbdkit-limit-filter.pod
+	$(PODWRAPPER) --section=1 --man $@ \
+	    --html $(top_builddir)/html/$@.html \
+	    $<
+
+endif HAVE_POD
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2ac8fb31..324339c1 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1064,6 +1064,9 @@ TESTS += test-fua.sh
 # ip filter test.
 TESTS += test-ip-filter.sh
 
+# limit filter test.
+TESTS += test-limit.sh
+
 # log filter test.
 TESTS += test-log.sh
 
diff --git a/filters/limit/limit.c b/filters/limit/limit.c
new file mode 100644
index 00000000..563481fa
--- /dev/null
+++ b/filters/limit/limit.c
@@ -0,0 +1,129 @@
+/* nbdkit
+ * Copyright (C) 2019-2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <pthread.h>
+
+#include <nbdkit-filter.h>
+
+#include "cleanup.h"
+
+/* Counts client connections. */
+static unsigned connections;
+static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
+
+/* Client limit (0 = filter is disabled). */
+static unsigned limit = 1;
+
+static int
+limit_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
+              const char *key, const char *value)
+{
+  if (strcmp (key, "limit") == 0) {
+    if (nbdkit_parse_unsigned ("limit", value, &limit) == -1)
+      return -1;
+    return 0;
+  }
+
+  return next (nxdata, key, value);
+}
+
+static void
+too_many_clients_error (void)
+{
+  nbdkit_error ("limit: too many clients connected, connection
rejected");
+}
+
+/* We limit connections in the preconnect stage (in particular before
+ * any heavyweight NBD or TLS negotiations has been done).  However we
+ * count connections in the open/close calls since clients can drop
+ * out between preconnect and open.
+ */
+static int
+limit_preconnect (nbdkit_next_preconnect *next, nbdkit_backend *nxdata,
+                  int readonly)
+{
+  if (next (nxdata, readonly) == -1)
+    return -1;
+
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+
+  if (limit > 0 && connections >= limit) {
+    too_many_clients_error ();
+    return -1;
+  }
+
+  return 0;
+}
+
+static void *
+limit_open (nbdkit_next_open *next, nbdkit_backend *nxdata,
+            int readonly)
+{
+  if (next (nxdata, readonly) == -1)
+    return NULL;
+
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+
+  /* We have to check again because clients can artificially slow down
+   * the NBD negotiation in order to bypass the limit otherwise.
+   */
+  if (limit > 0 && connections >= limit) {
+    too_many_clients_error ();
+    return NULL;
+  }
+
+  connections++;
+  return NBDKIT_HANDLE_NOT_NEEDED;
+}
+
+static void
+limit_close (void *handle)
+{
+  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
+  connections--;
+}
+
+static struct nbdkit_filter filter = {
+  .name              = "limit",
+  .longname          = "nbdkit limit filter",
+  .config            = limit_config,
+  .preconnect        = limit_preconnect,
+  .open              = limit_open,
+  .close             = limit_close,
+};
+
+NBDKIT_REGISTER_FILTER(filter)
diff --git a/tests/test-limit.sh b/tests/test-limit.sh
new file mode 100755
index 00000000..aa7c2b19
--- /dev/null
+++ b/tests/test-limit.sh
@@ -0,0 +1,81 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-2020 Red Hat Inc.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+source ./functions.sh
+set -e
+set -x
+
+requires nbdsh --version
+
+sock=`mktemp -u`
+files="limit.pid $sock"
+cleanup_fn rm -f $files
+
+# Start nbdkit with the limit filter and a limit of 2 clients.
+start_nbdkit -P limit.pid -U $sock --filter=limit memory size=1M limit=2
+
+export sock
+
+nbdsh -c - <<'EOF'
+import os
+import sys
+import time
+
+sock = os.environ["sock"]
+
+# It should be possible to connect two clients.
+# Note that nbdsh creates the ‘h’ handle implicitly.
+h.connect_unix (sock)
+h2 = nbd.NBD ()
+h2.connect_unix (sock)
+
+# A third connection is expected to fail.
+try:
+    h3 = nbd.NBD ()
+    h3.connect_unix (sock)
+    # This should not happen.
+    sys.exit (1)
+except nbd.Error:
+    pass
+
+# Close one of the existing connections.
+del h2
+
+# There's a possible race between closing the client socket
+# and nbdkit noticing and closing the connection.
+time.sleep (5)
+
+# Now a new connection should be possible.
+h4 = nbd.NBD ()
+h4.connect_unix (sock)
+
+EOF
diff --git a/TODO b/TODO
index cd2e47a5..e094c332 100644
--- a/TODO
+++ b/TODO
@@ -10,8 +10,6 @@ General ideas for improvements
   sizes and threads, as that should make it easier to identify
   systematic issues.
 
-* Limit number of incoming connections (like qemu-nbd -e).
-
 * For parallel plugins, only create threads on demand from parallel
   client requests, rather than pre-creating all threads at connection
   time, up to the thread pool size limit.  Of course, once created, a
-- 
2.25.0

On 3/4/20 1:44 PM, Richard W.M. Jones wrote:
> ---
>   docs/nbdkit-service.pod                     |   1 +
>   filters/exitlast/nbdkit-exitlast-filter.pod |   1 +
>   filters/ip/nbdkit-ip-filter.pod             |   1 +
>   filters/limit/nbdkit-limit-filter.pod       |  61 +++++++++
>   filters/rate/nbdkit-rate-filter.pod         |   1 +
>   configure.ac                                |   2 +
>   filters/limit/Makefile.am                   |  66 ++++++++++
>   tests/Makefile.am                           |   3 +
>   filters/limit/limit.c                       | 129 ++++++++++++++++++++
>   tests/test-limit.sh                         |  81 ++++++++++++
>   TODO                                        |   2 -
>   11 files changed, 346 insertions(+), 2 deletions(-)
> 
> +++ b/filters/limit/nbdkit-limit-filter.pod
> @@ -0,0 +1,61 @@
> +=head1 NAME
> +
> +nbdkit-limit-filter - limit number of clients that can connect
Is this number of clients that can connect in parallel, or number of 
clients that connect in series?
> +
> +=head1 SYNOPSIS
> +
> + nbdkit --filter=limit PLUGIN [limit=N]
> +
> +=head1 DESCRIPTION
> +
> +C<nbdkit-limit-filter> is an nbdkit filter that limits the number of
> +clients which can connect concurrently.  If more than C<limit=N>
> +(default: 1) clients try to connect at the same time then later
> +clients are rejected.
> +
> +=head1 PARAMETERS
> +
> +=over 4
> +
> +=item B<limit=>N
> +
> +Limit the number of concurrent clients to C<N>.  This parameter is
> +optional.  If not specified then the limit defaults to 1.  You can
> +also set this to 0 to make the number of clients unlimited (ie.
> +disable the filter).
> +
To some extent, you can also disable parallel connections with the 
noparallel-filter set to serialize=connections (but in that mode, the 
second client blocks waiting for its turn to connect, rather than the 
server dropping the second client immediately).  Probably worth 
cross-linking the two man pages to highlight this difference in behavior.
> +=head1 SEE ALSO
> +
> +L<nbdkit(1)>,
> +L<nbdkit-exitlast-filter(1)>,
> +L<nbdkit-ip-filter(1)>,
> +L<nbdkit-noparallel-filter(1)>,
> +L<nbdkit-rate-filter(1)>,
> +L<nbdkit-filter(3)>,
> +L<nbdkit-plugin(3)>.
Hmm - you did link to noparallel here, but noparallel doesn't link back.
> +++ b/filters/limit/limit.c
> +
> +/* Counts client connections. */
> +static unsigned connections;
> +static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
> +
> +/* Client limit (0 = filter is disabled). */
> +static unsigned limit = 1;
> +
> +static int
> +limit_config (nbdkit_next_config *next, nbdkit_backend *nxdata,
> +              const char *key, const char *value)
> +{
> +  if (strcmp (key, "limit") == 0) {
> +    if (nbdkit_parse_unsigned ("limit", value, &limit) ==
-1)
> +      return -1;
> +    return 0;
> +  }
> +
> +  return next (nxdata, key, value);
> +}
> +
> +static void
> +too_many_clients_error (void)
> +{
> +  nbdkit_error ("limit: too many clients connected, connection
rejected");
> +}
> +
> +/* We limit connections in the preconnect stage (in particular before
> + * any heavyweight NBD or TLS negotiations has been done).  However we
> + * count connections in the open/close calls since clients can drop
> + * out between preconnect and open.
> + */
Seems reasonable.
> +static int
> +limit_preconnect (nbdkit_next_preconnect *next, nbdkit_backend *nxdata,
> +                  int readonly)
> +{
> +  if (next (nxdata, readonly) == -1)
> +    return -1;
> +
> +  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
> +
> +  if (limit > 0 && connections >= limit) {
> +    too_many_clients_error ();
> +    return -1;
> +  }
> +
> +  return 0;
> +}
> +
> +static void *
> +limit_open (nbdkit_next_open *next, nbdkit_backend *nxdata,
> +            int readonly)
> +{
> +  if (next (nxdata, readonly) == -1)
> +    return NULL;
> +
> +  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
> +
> +  /* We have to check again because clients can artificially slow down
> +   * the NBD negotiation in order to bypass the limit otherwise.
> +   */
> +  if (limit > 0 && connections >= limit) {
> +    too_many_clients_error ();
> +    return NULL;
> +  }
> +
> +  connections++;
> +  return NBDKIT_HANDLE_NOT_NEEDED;
> +}
> +
> +static void
> +limit_close (void *handle)
> +{
> +  ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
> +  connections--;
> +}
Overall looks good.  As you say, we may want to enhance nbdkit with a 
way to enforce that a client complete handshake within a given time 
limit rather than dragging their feet, but that is separate from this 
filter.

ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org