Libguestfs - Jun 2019 - [nbdkit PATCH] extents: Cap maximum reply length

A devious plugin can cause an 8-fold increase in the reply size in
relation to the request size, by alternating status every byte of the
request. In the worst case, this can cause the client to reject our
response and kill the connection because the response is too large.
What's more, it consumes a lot of memory to track that many extents.
Let's put an upper bound on the maximum number of extents we are
willing to return (1M extents is an 8M reply chunk).

Pre-patch, this can be used to demonstrate the problem:

$ nbdkit -f sh - <<\EOF
#!/bin/bash
size=$((9*1024*1024))
case $1 in
  get_size) echo $size ;;
  pread) dd iflag=skip_bytes,count_bytes skip=$4 count=$3 if=/dev/zero || exit 1
;;
  can_extents) ;;
  extents)
    # Unrealistic in real life, but works to provoke the bug. For a full 9M
    # query, this produces ~100M for nbdkit to parse, and in turn tries to
    # produce a 72M reply chunk if we don't cap extents.
    for ((i=$4;i<$4+$3;i++)); do
      echo $i 1 $((i&1))
    done ;;
  *) exit 2 ;;
esac
EOF

$ ~/libnbd/run sh/nbdsh
...
nbd> h.connect_tcp("localhost","10809")
nbd> def f(data,metacontext,offset,e):
...  print ("entries:%d" % len(e))
...
nbd> h.block_status(9*1024*1024,0,0,f)
Traceback (most recent call last):
  File "/usr/lib64/python3.7/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
  File "/home/eblake/libnbd/python/nbd.py", line 577, in block_status
    return libnbdmod.block_status (self._o, count, offset, data, extent, flags)
RuntimeError: nbd_block_status: invalid server reply length

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/extents.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/server/extents.c b/server/extents.c
index c4224916..0ebd77b1 100644
--- a/server/extents.c
+++ b/server/extents.c
@@ -45,6 +45,12 @@

 #include "internal.h"

+/* Cap nr_extents to avoid sending over-large replies to the client,
+ * and to avoid a plugin with frequent alternations consuming too much
+ * memory.
+ */
+#define MAX_EXTENTS (1 * 1024 * 1024)
+
 struct nbdkit_extents {
   struct nbdkit_extent *extents;
   size_t nr_extents, allocated;
@@ -163,8 +169,8 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
   if (length == 0)
     return 0;

-  /* Ignore extents beyond the end of the range. */
-  if (offset >= exts->end)
+  /* Ignore extents beyond the end of the range, or if list is full. */
+  if (offset >= exts->end || exts->nr_extents == MAX_EXTENTS)
     return 0;

   /* Shorten extents that overlap the end of the range. */
-- 
2.20.1

On Mon, Jun 17, 2019 at 03:05:06PM -0500, Eric Blake
wrote:
>A devious plugin can cause an 8-fold increase in the reply size in
>relation to the request size, by alternating status every byte of the
>request. In the worst case, this can cause the client to reject our
>response and kill the connection because the response is too large.
>What's more, it consumes a lot of memory to track that many extents.
>Let's put an upper bound on the maximum number of extents we are
>willing to return (1M extents is an 8M reply chunk).
>
>Pre-patch, this can be used to demonstrate the problem:
>
>$ nbdkit -f sh - <<\EOF
>#!/bin/bash
>size=$((9*1024*1024))
>case $1 in
>  get_size) echo $size ;;
>  pread) dd iflag=skip_bytes,count_bytes skip=$4 count=$3 if=/dev/zero ||
exit 1 ;;
>  can_extents) ;;
>  extents)
>    # Unrealistic in real life, but works to provoke the bug. For a full 9M
>    # query, this produces ~100M for nbdkit to parse, and in turn tries to
>    # produce a 72M reply chunk if we don't cap extents.
>    for ((i=$4;i<$4+$3;i++)); do
>      echo $i 1 $((i&1))
>    done ;;
>  *) exit 2 ;;
>esac
>EOF
>
>$ ~/libnbd/run sh/nbdsh
>...
>nbd> h.connect_tcp("localhost","10809")
>nbd> def f(data,metacontext,offset,e):
>...  print ("entries:%d" % len(e))
>...
>nbd> h.block_status(9*1024*1024,0,0,f)
>Traceback (most recent call last):
>  File "/usr/lib64/python3.7/code.py", line 90, in runcode
>    exec(code, self.locals)
>  File "<console>", line 1, in <module>
>  File "/home/eblake/libnbd/python/nbd.py", line 577, in
block_status
>    return libnbdmod.block_status (self._o, count, offset, data, extent,
flags)
>RuntimeError: nbd_block_status: invalid server reply length
>
>Signed-off-by: Eric Blake <eblake@redhat.com>
>---
> server/extents.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
>diff --git a/server/extents.c b/server/extents.c
>index c4224916..0ebd77b1 100644
>--- a/server/extents.c
>+++ b/server/extents.c
>@@ -45,6 +45,12 @@
>
> #include "internal.h"
>
>+/* Cap nr_extents to avoid sending over-large replies to the client,
>+ * and to avoid a plugin with frequent alternations consuming too much
>+ * memory.
>+ */
>+#define MAX_EXTENTS (1 * 1024 * 1024)
>+
> struct nbdkit_extents {
>   struct nbdkit_extent *extents;
>   size_t nr_extents, allocated;
>@@ -163,8 +169,8 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
>   if (length == 0)
>     return 0;
>
>-  /* Ignore extents beyond the end of the range. */
>-  if (offset >= exts->end)
>+  /* Ignore extents beyond the end of the range, or if list is full. */
>+  if (offset >= exts->end || exts->nr_extents == MAX_EXTENTS)
>     return 0;
>
It looks OK, just one thought would be to either return the information to the
caller or expose the MAX_EXTENTS somehow.  That way the plugin could just
immediately return without wasting time on (not) adding more and more extents.

But I can't think of a way to do it nicely.  This function could return
something else than 0, but this does not seem to be used in nbdkit very much.
Exposing MAX_EXTENTS would, on the other hand, had to be done at runtime and
wiring it into the plugins would be too much duplicated code.

So that's just an idea.
>   /* Shorten extents that overlap the end of the range. */
>-- 
>2.20.1
>
>_______________________________________________
>Libguestfs mailing list
>Libguestfs@redhat.com
>https://www.redhat.com/mailman/listinfo/libguestfs

Already upstream so not a review, just pointing out ...

On Mon, Jun 17, 2019 at 03:05:06PM -0500, Eric Blake
wrote:
> $ ~/libnbd/run sh/nbdsh
~/libnbd/run sets PATH, so you can just do:

$ ~/libnbd/run nbdsh

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW