@@ -217,7 +217,7 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
msg = g_strdup_vprintf(fmt, va);
len = strlen(msg);
- assert(len < 4096);
+ assert(len < NBD_MAX_STRING_SIZE);
trace_nbd_negotiate_send_rep_err(msg);
ret = nbd_negotiate_send_rep_len(client, type, len, errp);
if (ret < 0) {
@@ -231,6 +231,27 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type,
return 0;
}
+/*
+ * Truncate a potentially-long user-supplied string into something
+ * more suitable for an error reply.
+ */
+static const char *
+nbd_truncate_name(const char *name)
+{
+#define SANE_LENGTH 80
+ static char buf[SANE_LENGTH + 3 + 1]; /* Trailing '...', NUL */
+
+ if (strlen(name) < SANE_LENGTH) {
+ return name;
+ }
+ memcpy(buf, name, SANE_LENGTH);
+ buf[SANE_LENGTH] = '.';
+ buf[SANE_LENGTH + 1] = '.';
+ buf[SANE_LENGTH + 2] = '.';
+ buf[SANE_LENGTH + 3] = '\0';
+ return buf;
+}
+
/* Send an error reply.
* Return -errno on error, 0 on success. */
static int GCC_FMT_ATTR(4, 5)
@@ -597,7 +618,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
if (!exp) {
return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_UNKNOWN,
errp, "export '%s' not present",
- name);
+ nbd_truncate_name(name));
}
/* Don't bother sending NBD_INFO_NAME unless client requested it */
@@ -996,7 +1017,8 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
meta->exp = nbd_export_find(export_name);
if (meta->exp == NULL) {
return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp,
- "export '%s' not present", export_name);
+ "export '%s' not present",
+ nbd_truncate_name(export_name));
}
ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), errp);
@@ -58,6 +58,10 @@ _send_qemu_cmd $QEMU_HANDLE \
$QEMU_IO_PROG -f raw -c quit \
"nbd+unix:///no_such_export?socket=$SOCK_DIR/nbd" 2>&1 \
| _filter_qemu_io | _filter_nbd
+# Likewise, with longest possible name permitted in NBD protocol
+$QEMU_IO_PROG -f raw -c quit \
+ "nbd+unix:///$(printf %4096d 1 | tr ' ' a)?socket=$SOCK_DIR/nbd" 2>&1 \
+ | _filter_qemu_io | _filter_nbd | sed 's/aa.*aa/aa...aa/'
_send_qemu_cmd $QEMU_HANDLE \
"{ 'execute': 'quit' }" \
@@ -5,6 +5,8 @@ QA output created by 143
{"return": {}}
qemu-io: can't open device nbd+unix:///no_such_export?socket=SOCK_DIR/nbd: Requested export not available
server reported: export 'no_such_export' not present
+qemu-io: can't open device nbd+unix:///aa...aa1?socket=SOCK_DIR/nbd: Requested export not available
+server reported: export 'aa...aa...' not present
{ 'execute': 'quit' }
{"return": {}}
{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}}
Ever since commit 36683283 (v2.8), the server code asserts that error strings sent to the client are well-formed per the protocol by not exceeding the maximum string length of 4096. At the time the server first started sending error messages, the assertion could not be triggered, because messages were completely under our control. However, over the years, we have added latent scenarios where a client could trigger the server to attempt an error message that would include the client's information if it passed other checks first: - requesting NBD_OPT_INFO/GO on an export name that is not present (commit 0cfae925 in v2.12 echoes the name) - requesting NBD_OPT_LIST/SET_META_CONTEXT on an export name that is not present (commit e7b1948d in v2.12 echoes the name) At the time, those were still safe because we flagged names larger than 256 bytes with a different message; but that changed in commit 93676c88 (v4.2) when we raised the name limit to 4096 to match the NBD string limit. (That commit also failed to change the magic number 4096 in nbd_negotiate_send_rep_err to the just-introduced named constant.) So with that commit, long client names appended to server text can now trigger the assertion, and thus be used as a denial of service attack against a server. As a mitigating factor, if the server requires TLS, the client cannot trigger the problematic paths unless it first supplies TLS credentials, and such trusted clients are less likely to try to intentionally crash the server. Reported-by: Xueqiang Wei <xuwei@redhat.com> CC: qemu-stable@nongnu.org Fixes: https://bugzilla.redhat.com/1843684 CVE-2020-10761 Fixes: 93676c88d7 Signed-off-by: Eric Blake <eblake@redhat.com> Message-Id: <20200608182638.3256473-2-eblake@redhat.com> --- nbd/server.c | 28 +++++++++++++++++++++++++--- tests/qemu-iotests/143 | 4 ++++ tests/qemu-iotests/143.out | 2 ++ 3 files changed, 31 insertions(+), 3 deletions(-)