8:57 p.m.
Fuzzing with afl found a bug where a 27 byte client sequence
can cause nbdkit to report a strange error message:
$ printf %s $'000\1IHAVEOPT000\6'$'000\7'$'000\1x00' | tr 0
'\0' |
./nbdkit -s memory size=1m >/dev/null
nbdkit: memory: error: client exceeded maximum number of options (32)
The culprit? The client is hanging up on a message boundary,
so conn->recv() returns 0 for EOF instead of -1 for read error,
and our code blindly continues on in a for loop (re-)parsing
the leftover data from the previous option.
Let's fix things to uniformly quit trying to negotiate with a client
that has early EOF at a message boundary, just as we do for one that
quits mid-field, with the one difference that we treat a message
boundary as a warning instead of an error because a client hanging up
after an option response that it didn't like (rather than sending
NBD_OPT_ABORT to inform the server that it won't be negotiating
further) is a surprisingly common behavior among some existing clients.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
src/connections.c | 60 ++++++++++++++++++++++++++++++-----------------
1 file changed, 39 insertions(+), 21 deletions(-)
diff --git a/src/connections.c b/src/connections.c
index 58ed6b0..577f466 100644
--- a/src/connections.c
+++ b/src/connections.c
@@ -600,6 +600,31 @@ send_newstyle_option_reply_info_export (struct connection *conn,
return 0;
}
+/* Sub-function during _negotiate_handshake_newstyle, to uniformly handle
+ * a client hanging up on a message boundary.
+ */
+static int __attribute__ ((format (printf, 4, 5)))
+conn_recv_full (struct connection *conn, void *buf, size_t len,
+ const char *fmt, ...)
+{
+ int r = conn->recv (conn, buf, len);
+ va_list args;
+
+ if (r == -1) {
+ va_start (args, fmt);
+ nbdkit_verror (fmt, args);
+ va_end (args);
+ return -1;
+ }
+ if (r == 0) {
+ /* During negotiation, client EOF on message boundary is less
+ * severe than failure in the middle of the buffer. */
+ debug ("client closed input socket, closing connection");
+ return -1;
+ }
+ return r;
+}
+
/* Sub-function of _negotiate_handshake_newstyle_options below. It
* must be called on all non-error paths out of the options for-loop
* in that function.
@@ -639,10 +664,9 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
const char *optname;
for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) {
- if (conn->recv (conn, &new_option, sizeof new_option) == -1) {
- nbdkit_error ("read: %m");
+ if (conn_recv_full (conn, &new_option, sizeof new_option,
+ "read: %m") == -1)
return -1;
- }
version = be64toh (new_option.version);
if (version != NEW_VERSION) {
@@ -675,10 +699,9 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
switch (option) {
case NBD_OPT_EXPORT_NAME:
- if (conn->recv (conn, data, optlen) == -1) {
- nbdkit_error ("read: %s: %m", name_of_nbd_opt (option));
+ if (conn_recv_full (conn, data, optlen,
+ "read: %s: %m", name_of_nbd_opt (option)) == -1)
return -1;
- }
/* Apart from printing it, ignore the export name. */
data[optlen] = '\0';
debug ("newstyle negotiation: %s: "
@@ -715,10 +738,9 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
== -1)
return -1;
- if (conn->recv (conn, data, optlen) == -1) {
- nbdkit_error ("read: %s: %m", name_of_nbd_opt (option));
+ if (conn_recv_full (conn, data, optlen,
+ "read: %s: %m", name_of_nbd_opt (option)) == -1)
return -1;
- }
continue;
}
@@ -738,10 +760,9 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
== -1)
return -1;
- if (conn->recv (conn, data, optlen) == -1) {
- nbdkit_error ("read: %s: %m", name_of_nbd_opt (option));
+ if (conn_recv_full (conn, data, optlen,
+ "read: %s: %m", name_of_nbd_opt (option)) == -1)
return -1;
- }
continue;
}
@@ -780,10 +801,9 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
case NBD_OPT_INFO:
case NBD_OPT_GO:
optname = name_of_nbd_opt (option);
- if (conn->recv (conn, data, optlen) == -1) {
- nbdkit_error ("read: %s: %m", optname);
+ if (conn_recv_full (conn, data, optlen,
+ "read: %s: %m", optname) == -1)
return -1;
- }
if (optlen < 6) { /* 32 bit export length + 16 bit nr info */
debug ("newstyle negotiation: %s option length < 6", optname);
@@ -880,10 +900,9 @@ _negotiate_handshake_newstyle_options (struct connection *conn)
/* Unknown option. */
if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) == -1)
return -1;
- if (conn->recv (conn, data, optlen) == -1) {
- nbdkit_error ("read: %m");
+ if (conn_recv_full (conn, data, optlen,
+ "read: %m") == -1)
return -1;
- }
}
/* Note, since it's not very clear from the protocol doc, that the
@@ -931,10 +950,9 @@ _negotiate_handshake_newstyle (struct connection *conn)
}
/* Client now sends us its 32 bit flags word ... */
- if (conn->recv (conn, &conn->cflags, sizeof conn->cflags) == -1) {
- nbdkit_error ("read: %m");
+ if (conn_recv_full (conn, &conn->cflags, sizeof conn->cflags,
+ "read: %m") == -1)
return -1;
- }
conn->cflags = be32toh (conn->cflags);
/* ... which we check for accuracy. */
debug ("newstyle negotiation: client flags: 0x%x", conn->cflags);
--
2.17.2