Andrew Geissler | 475cb72 | 2020-07-10 16:00:51 -0500 | [diff] [blame^] | 1 | From 5c4fe018c025740fef4a0a4421e8162db0c3eefd Mon Sep 17 00:00:00 2001 |
| 2 | From: Eric Blake <eblake@redhat.com> |
| 3 | Date: Mon, 8 Jun 2020 13:26:37 -0500 |
| 4 | Subject: [PATCH] nbd/server: Avoid long error message assertions |
| 5 | CVE-2020-10761 |
| 6 | |
| 7 | Ever since commit 36683283 (v2.8), the server code asserts that error |
| 8 | strings sent to the client are well-formed per the protocol by not |
| 9 | exceeding the maximum string length of 4096. At the time the server |
| 10 | first started sending error messages, the assertion could not be |
| 11 | triggered, because messages were completely under our control. |
| 12 | However, over the years, we have added latent scenarios where a client |
| 13 | could trigger the server to attempt an error message that would |
| 14 | include the client's information if it passed other checks first: |
| 15 | |
| 16 | - requesting NBD_OPT_INFO/GO on an export name that is not present |
| 17 | (commit 0cfae925 in v2.12 echoes the name) |
| 18 | |
| 19 | - requesting NBD_OPT_LIST/SET_META_CONTEXT on an export name that is |
| 20 | not present (commit e7b1948d in v2.12 echoes the name) |
| 21 | |
| 22 | At the time, those were still safe because we flagged names larger |
| 23 | than 256 bytes with a different message; but that changed in commit |
| 24 | 93676c88 (v4.2) when we raised the name limit to 4096 to match the NBD |
| 25 | string limit. (That commit also failed to change the magic number |
| 26 | 4096 in nbd_negotiate_send_rep_err to the just-introduced named |
| 27 | constant.) So with that commit, long client names appended to server |
| 28 | text can now trigger the assertion, and thus be used as a denial of |
| 29 | service attack against a server. As a mitigating factor, if the |
| 30 | server requires TLS, the client cannot trigger the problematic paths |
| 31 | unless it first supplies TLS credentials, and such trusted clients are |
| 32 | less likely to try to intentionally crash the server. |
| 33 | |
| 34 | We may later want to further sanitize the user-supplied strings we |
| 35 | place into our error messages, such as scrubbing out control |
| 36 | characters, but that is less important to the CVE fix, so it can be a |
| 37 | later patch to the new nbd_sanitize_name. |
| 38 | |
| 39 | Consideration was given to changing the assertion in |
| 40 | nbd_negotiate_send_rep_verr to instead merely log a server error and |
| 41 | truncate the message, to avoid leaving a latent path that could |
| 42 | trigger a future CVE DoS on any new error message. However, this |
| 43 | merely complicates the code for something that is already (correctly) |
| 44 | flagging coding errors, and now that we are aware of the long message |
| 45 | pitfall, we are less likely to introduce such errors in the future, |
| 46 | which would make such error handling dead code. |
| 47 | |
| 48 | Reported-by: Xueqiang Wei <xuwei@redhat.com> |
| 49 | CC: qemu-stable@nongnu.org |
| 50 | Fixes: https://bugzilla.redhat.com/1843684 CVE-2020-10761 |
| 51 | Fixes: 93676c88d7 |
| 52 | Signed-off-by: Eric Blake <eblake@redhat.com> |
| 53 | Message-Id: <20200610163741.3745251-2-eblake@redhat.com> |
| 54 | Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> |
| 55 | |
| 56 | Upstream-Status: Backport [https://github.com/qemu/qemu/commit/5c4fe018c025740fef4a0a4421e8162db0c3eefd] |
| 57 | CVE: CVE-2020-10761 |
| 58 | Signed-off-by: Chee Yang Lee <chee.yang.lee@intel.com> |
| 59 | |
| 60 | --- |
| 61 | nbd/server.c | 23 ++++++++++++++++++++--- |
| 62 | tests/qemu-iotests/143 | 4 ++++ |
| 63 | tests/qemu-iotests/143.out | 2 ++ |
| 64 | 3 files changed, 26 insertions(+), 3 deletions(-) |
| 65 | |
| 66 | diff --git a/nbd/server.c b/nbd/server.c |
| 67 | index 02b1ed08014..20754e9ebc3 100644 |
| 68 | --- a/nbd/server.c |
| 69 | +++ b/nbd/server.c |
| 70 | @@ -217,7 +217,7 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type, |
| 71 | |
| 72 | msg = g_strdup_vprintf(fmt, va); |
| 73 | len = strlen(msg); |
| 74 | - assert(len < 4096); |
| 75 | + assert(len < NBD_MAX_STRING_SIZE); |
| 76 | trace_nbd_negotiate_send_rep_err(msg); |
| 77 | ret = nbd_negotiate_send_rep_len(client, type, len, errp); |
| 78 | if (ret < 0) { |
| 79 | @@ -231,6 +231,19 @@ nbd_negotiate_send_rep_verr(NBDClient *client, uint32_t type, |
| 80 | return 0; |
| 81 | } |
| 82 | |
| 83 | +/* |
| 84 | + * Return a malloc'd copy of @name suitable for use in an error reply. |
| 85 | + */ |
| 86 | +static char * |
| 87 | +nbd_sanitize_name(const char *name) |
| 88 | +{ |
| 89 | + if (strnlen(name, 80) < 80) { |
| 90 | + return g_strdup(name); |
| 91 | + } |
| 92 | + /* XXX Should we also try to sanitize any control characters? */ |
| 93 | + return g_strdup_printf("%.80s...", name); |
| 94 | +} |
| 95 | + |
| 96 | /* Send an error reply. |
| 97 | * Return -errno on error, 0 on success. */ |
| 98 | static int GCC_FMT_ATTR(4, 5) |
| 99 | @@ -595,9 +608,11 @@ static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) |
| 100 | |
| 101 | exp = nbd_export_find(name); |
| 102 | if (!exp) { |
| 103 | + g_autofree char *sane_name = nbd_sanitize_name(name); |
| 104 | + |
| 105 | return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_UNKNOWN, |
| 106 | errp, "export '%s' not present", |
| 107 | - name); |
| 108 | + sane_name); |
| 109 | } |
| 110 | |
| 111 | /* Don't bother sending NBD_INFO_NAME unless client requested it */ |
| 112 | @@ -995,8 +1010,10 @@ static int nbd_negotiate_meta_queries(NBDClient *client, |
| 113 | |
| 114 | meta->exp = nbd_export_find(export_name); |
| 115 | if (meta->exp == NULL) { |
| 116 | + g_autofree char *sane_name = nbd_sanitize_name(export_name); |
| 117 | + |
| 118 | return nbd_opt_drop(client, NBD_REP_ERR_UNKNOWN, errp, |
| 119 | - "export '%s' not present", export_name); |
| 120 | + "export '%s' not present", sane_name); |
| 121 | } |
| 122 | |
| 123 | ret = nbd_opt_read(client, &nb_queries, sizeof(nb_queries), errp); |
| 124 | diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143 |
| 125 | index f649b361950..d2349903b1b 100755 |
| 126 | --- a/tests/qemu-iotests/143 |
| 127 | +++ b/tests/qemu-iotests/143 |
| 128 | @@ -58,6 +58,10 @@ _send_qemu_cmd $QEMU_HANDLE \ |
| 129 | $QEMU_IO_PROG -f raw -c quit \ |
| 130 | "nbd+unix:///no_such_export?socket=$SOCK_DIR/nbd" 2>&1 \ |
| 131 | | _filter_qemu_io | _filter_nbd |
| 132 | +# Likewise, with longest possible name permitted in NBD protocol |
| 133 | +$QEMU_IO_PROG -f raw -c quit \ |
| 134 | + "nbd+unix:///$(printf %4096d 1 | tr ' ' a)?socket=$SOCK_DIR/nbd" 2>&1 \ |
| 135 | + | _filter_qemu_io | _filter_nbd | sed 's/aaaa*aa/aa--aa/' |
| 136 | |
| 137 | _send_qemu_cmd $QEMU_HANDLE \ |
| 138 | "{ 'execute': 'quit' }" \ |
| 139 | diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out |
| 140 | index 1f4001c6013..fc9c0a761fa 100644 |
| 141 | --- a/tests/qemu-iotests/143.out |
| 142 | +++ b/tests/qemu-iotests/143.out |
| 143 | @@ -5,6 +5,8 @@ QA output created by 143 |
| 144 | {"return": {}} |
| 145 | qemu-io: can't open device nbd+unix:///no_such_export?socket=SOCK_DIR/nbd: Requested export not available |
| 146 | server reported: export 'no_such_export' not present |
| 147 | +qemu-io: can't open device nbd+unix:///aa--aa1?socket=SOCK_DIR/nbd: Requested export not available |
| 148 | +server reported: export 'aa--aa...' not present |
| 149 | { 'execute': 'quit' } |
| 150 | {"return": {}} |
| 151 | {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} |