qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] nbd/server: Avoid long error message assertions CVE-2020


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH 1/2] nbd/server: Avoid long error message assertions CVE-2020-10761
Date: Wed, 10 Jun 2020 16:52:06 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.1

10.06.2020 16:39, Eric Blake wrote:
On 6/10/20 3:57 AM, Vladimir Sementsov-Ogievskiy wrote:
08.06.2020 21:26, Eric Blake wrote:
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>
---
  nbd/server.c               | 28 +++++++++++++++++++++++++---
  tests/qemu-iotests/143     |  4 ++++
  tests/qemu-iotests/143.out |  2 ++
  3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index 02b1ed080145..ec130303586d 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -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 */

s/NUL/NULL/

NULL is the pointer (typically 4 or 8 bytes); NUL is the character (exactly one 
byte in all multi-byte-encodings like UTF-8, or sizeof(wchar_t) when using wide 
characters).


Hmm. It may break if we use it in parallel in two coroutines or threads.. Not 
sure, is it possible now, neither of course will it be possible in future.

After some testing (including adding some temporary sleep() into the code), it 
looks like 'qemu-nbd -e 2' is currently serialized (we don't start responding 
to a second client until we are done negotiating with the first); on that 
grounds, we are not risking that information leaks from one client to another.  
But you are correct that it is not obvious, and that if we do have a situation 
where two threads can try to allow an NBD connection, then this static buffer 
could leak information from one client to another.  So I'll need to post a v2.


I'd avoid creating functions returning  instead use g_strdup_printf(), like

char *tmp = g_strdup_printf("%.80s...", name);

   ( OR, if you want explicit constant: g_strdup_printf("%.*s...", SANE_LENGTH, 
name) )

... report error ...

g_free(tmp)

Using g_strdup_printf also is safer as we don't need to care about buf size.

malloc'ing the buffer is not too bad; error messages are not the hot path. I'll 
change it along those lines for v2.

@@ -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));
      }


Hmm, maybe instead of assertion, shrink message in 
nbd_negotiate_send_rep_verr() too?
This will save us from forgotten (or future) uses of the function.

Truncating in nbd_negotiate_send_rep_verr is arbitrary; it does not have the context of 
what makes sense to truncate.  With an artificially short length, and a client request 
for "longname_from_the_client", the difference would be between:

export "longnam..." not present
export "longname_from_the_cl...

As a user, I prefer the first form, but truncating in 
nbd_negotiate_send_rep_verr is the second form.


Shrinking name is better, as it provides better message on result. But 
generally shrink
all two long messages in nbd_negotiate_send_rep_verr() (maybe, together with 
error_report())
seems a good thing for me.

What would error_report() accomplish?  Logging on the server that we are 
truncating a message sent back to the client doesn't help the client. This is 
not a case of protocol violation, and a server sending an error message to the 
client is not out of the ordinary.


I mean the following:

If we have too long message in nbd_negotiate_send_rep_verr() it's a bug, and 
assertion is correct. But in this particular case, It seems better to use 
error_report() instead of assertion and shrink the message for client, just to 
avoid similar CVEs in future. So if the situation will come again (we call 
nbd_negotiate_send_rep_verr() from some new place, forgetting to check length 
of user-provided part of the message) it will not be a CVE but just user will 
have not-well-formed message and we'll have errror-report in server log.

--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]