[Libguestfs] [nbdkit PATCH v2 4/7] server: Improve check of string validity
Eric Blake
eblake at redhat.com
Sat Sep 28 04:48:46 UTC 2019
Factor out a common function for checking whether a client export name
is valid, checking for length and no embedded NULs (in the future, we
may also add a check for well-formed UTF-8). Checking for embedded
NUL is important now that we allow plugins to alter content based on
the client's request: if the client requested 'a\0b' (against the NBD
protocol), they would be surprised if the reflection plugin only gave
back 'a'. Rejecting bad strings is easier than altering our API to
pass through lengths to preserve embedded NUL.
Use the new functions to simplify existing code in NBD_OPT_EXPORT_NAME
and NBD_OPT_GO, but also to validate the export name handed to
NBD_OPT_SET_META_CONTEXT.
No known existing client makes it easy to send 'a\0b' as an export or
meta context name against the protocol, but well-place breakpoints in
gdb on a client process allowed me to test this manually.
Signed-off-by: Eric Blake <eblake at redhat.com>
---
server/protocol-handshake-newstyle.c | 62 ++++++++++++++++++++++------
tests/test-long-name.sh | 18 ++++++++
2 files changed, 67 insertions(+), 13 deletions(-)
diff --git a/server/protocol-handshake-newstyle.c b/server/protocol-handshake-newstyle.c
index 0cbcc3d5..34958360 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -210,6 +210,43 @@ finish_newstyle_options (struct connection *conn, uint64_t *exportsize)
return 0;
}
+static int
+check_string (uint32_t option, char *buf, uint32_t len, uint32_t maxlen,
+ const char *name)
+{
+ if (len > NBD_MAX_STRING || len > maxlen) {
+ nbdkit_error ("%s: %s too long", name_of_nbd_opt (option), name);
+ return -1;
+ }
+ if (strnlen (buf, len) != len) {
+ nbdkit_error ("%s: %s may not include NUL bytes",
+ name_of_nbd_opt (option), name);
+ return -1;
+ }
+ /* TODO: Check for valid UTF-8? */
+ return 0;
+}
+
+/* Sub-function of negotiate_handshake_newstyle_options, to grab and
+ * validate an export name.
+ */
+static int
+check_export_name (struct connection *conn, uint32_t option, char *buf,
+ uint32_t exportnamelen, uint32_t maxlen, bool save)
+{
+ if (check_string (option, buf, exportnamelen, maxlen, "export name") == -1)
+ return -1;
+
+ assert (exportnamelen < sizeof conn->exportname);
+ if (save) {
+ memcpy (conn->exportname, buf, exportnamelen);
+ conn->exportname[exportnamelen] = '\0';
+ }
+ debug ("newstyle negotiation: %s: client requested export '%.*s'",
+ name_of_nbd_opt (option), (int) exportnamelen, buf);
+ return 0;
+}
+
static int
negotiate_handshake_newstyle_options (struct connection *conn)
{
@@ -273,12 +310,8 @@ negotiate_handshake_newstyle_options (struct connection *conn)
if (conn_recv_full (conn, data, optlen,
"read: %s: %m", name_of_nbd_opt (option)) == -1)
return -1;
- /* Print the export name and save it in the connection. */
- data[optlen] = '\0';
- debug ("newstyle negotiation: %s: client requested export '%s'",
- name_of_nbd_opt (option), data);
- assert (optlen < sizeof conn->exportname);
- strcpy (conn->exportname, data);
+ if (check_export_name (conn, option, data, optlen, optlen, true) == -1)
+ return -1;
/* We have to finish the handshake by sending handshake_finish. */
if (finish_newstyle_options (conn, &exportsize) == -1)
@@ -418,11 +451,9 @@ negotiate_handshake_newstyle_options (struct connection *conn)
/* FIXME: Our current MAX_OPTION_LENGTH prevents us from receiving
* an export name at the full NBD_MAX_STRING length.
*/
- assert (exportnamelen < sizeof conn->exportname);
- memcpy (conn->exportname, &data[4], exportnamelen);
- conn->exportname[exportnamelen] = '\0';
- debug ("newstyle negotiation: %s: client requested export '%s'",
- optname, conn->exportname);
+ if (check_export_name (conn, option, &data[4], exportnamelen,
+ optlen - 6, true) == -1)
+ return -1;
/* The spec is confusing, but it is required that we send back
* NBD_INFO_EXPORT, even if the client did not request it!
@@ -541,9 +572,13 @@ negotiate_handshake_newstyle_options (struct connection *conn)
continue;
}
- /* Discard the export name. */
+ /* Discard the export name, after validating it. */
memcpy (&exportnamelen, &data[0], 4);
exportnamelen = be32toh (exportnamelen);
+ what = "validating export name";
+ if (check_export_name (conn, option, &data[4], exportnamelen,
+ optlen - 8, false) == -1)
+ goto opt_meta_invalid_option_len;
opt_index = 4 + exportnamelen;
/* Read the number of queries. */
@@ -583,7 +618,8 @@ negotiate_handshake_newstyle_options (struct connection *conn)
querylen = be32toh (querylen);
opt_index += 4;
what = "reading query string";
- if (opt_index + querylen > optlen)
+ if (check_string (option, &data[opt_index], querylen,
+ optlen - opt_index, "meta context query") == -1)
goto opt_meta_invalid_option_len;
debug ("newstyle negotiation: %s: %s %.*s",
diff --git a/tests/test-long-name.sh b/tests/test-long-name.sh
index 7b0b43ad..86aefbaf 100755
--- a/tests/test-long-name.sh
+++ b/tests/test-long-name.sh
@@ -38,6 +38,7 @@ fail=0
# Test handling of NBD maximum string length of 4k.
+requires qemu-io --version
requires qemu-nbd --version
name16=1234567812345678
@@ -71,6 +72,23 @@ case $out in
fail=1 ;;
esac
+# Use largest possible export name, then oversize, with NBD_OPT_EXPORT_NAME.
+nbdkit -U - --mask-handshake=0 null --run 'qemu-io -r -f raw -c quit \
+ nbd+unix:///'$name4k'\?socket=$unixsocket' || fail=1
+# qemu 4.1 did not length check, letting it send an invalid NBD client
+# request which nbdkit must filter out. Later qemu might refuse to
+# send the request (like libnbd does), at which point this is no longer
+# testing nbdkit proper, so we may remove it later:
+nbdkit -U - --mask-handshake=0 null --run 'qemu-io -r -f raw -c quit \
+ nbd+unix:///'a$name4k'\?socket=$unixsocket' && fail=1
+
+# Repeat with NBD_OPT_GO.
+nbdkit -U - null --run 'qemu-io -r -f raw -c quit \
+ nbd+unix:///'$name1k$name1k'\?socket=$unixsocket' || fail=1
+# FIXME: Right now, we can't accept full 4k length - this should succeed
+nbdkit -U - null --run 'qemu-io -r -f raw -c quit \
+ nbd+unix:///'$almost4k'\?socket=$unixsocket' && fail=1
+
# The rest of this test uses the ‘qemu-nbd --list’ option added in qemu 4.0.
if ! qemu-nbd --help | grep -sq -- --list; then
echo "$0: skipping because qemu-nbd does not support the --list option"
--
2.21.0
More information about the Libguestfs
mailing list