|
|
dda0b2 |
From 611f931d75feaf516da8097e6a69ff084b4ca38c Mon Sep 17 00:00:00 2001
|
|
|
dda0b2 |
From: "Richard W.M. Jones" <rjones@redhat.com>
|
|
|
dda0b2 |
Date: Wed, 11 Sep 2019 09:47:30 +0100
|
|
|
dda0b2 |
Subject: [PATCH 2/2] server: Wait until handshake complete before calling
|
|
|
dda0b2 |
.open callback
|
|
|
dda0b2 |
MIME-Version: 1.0
|
|
|
dda0b2 |
Content-Type: text/plain; charset=UTF-8
|
|
|
dda0b2 |
Content-Transfer-Encoding: 8bit
|
|
|
dda0b2 |
|
|
|
dda0b2 |
Currently we call the plugin .open callback as soon as we receive a
|
|
|
dda0b2 |
TCP connection:
|
|
|
dda0b2 |
|
|
|
dda0b2 |
$ nbdkit -fv --tls=require --tls-certificates=tests/pki null \
|
|
|
dda0b2 |
--run "telnet localhost 10809"
|
|
|
dda0b2 |
[...]
|
|
|
dda0b2 |
Trying ::1...
|
|
|
dda0b2 |
Connected to localhost.
|
|
|
dda0b2 |
Escape character is '^]'.
|
|
|
dda0b2 |
nbdkit: debug: accepted connection
|
|
|
dda0b2 |
nbdkit: debug: null: open readonly=0 ◀ NOTE
|
|
|
dda0b2 |
nbdkit: null[1]: debug: newstyle negotiation: flags: global 0x3
|
|
|
dda0b2 |
NBDMAGICIHAVEOPT
|
|
|
dda0b2 |
|
|
|
dda0b2 |
In plugins such as curl, guestfs, ssh, vddk and others we do a
|
|
|
dda0b2 |
considerable amount of work in the .open callback (such as making a
|
|
|
dda0b2 |
remote connection or launching an appliance). Therefore we are
|
|
|
dda0b2 |
providing an easy Denial of Service / Amplification Attack for
|
|
|
dda0b2 |
unauthorized clients to cause a lot of work to be done for only the
|
|
|
dda0b2 |
cost of a simple TCP 3 way handshake.
|
|
|
dda0b2 |
|
|
|
dda0b2 |
This commit moves the call to the .open callback after the NBD
|
|
|
dda0b2 |
handshake has mostly completed. In particular TLS authentication must
|
|
|
dda0b2 |
be complete before we will call into the plugin.
|
|
|
dda0b2 |
|
|
|
dda0b2 |
It is unlikely that there are plugins which really depend on the
|
|
|
dda0b2 |
current behaviour of .open (which I found surprising even though I
|
|
|
dda0b2 |
guess I must have written it). If there are then we could add a new
|
|
|
dda0b2 |
.connect callback or similar to allow plugins to get control at the
|
|
|
dda0b2 |
earlier point in the connection.
|
|
|
dda0b2 |
|
|
|
dda0b2 |
After this change you can see that the .open callback is not called
|
|
|
dda0b2 |
from just a simple TCP connection:
|
|
|
dda0b2 |
|
|
|
dda0b2 |
$ ./nbdkit -fv --tls=require --tls-certificates=tests/pki null \
|
|
|
dda0b2 |
--run "telnet localhost 10809"
|
|
|
dda0b2 |
[...]
|
|
|
dda0b2 |
Trying ::1...
|
|
|
dda0b2 |
Connected to localhost.
|
|
|
dda0b2 |
Escape character is '^]'.
|
|
|
dda0b2 |
nbdkit: debug: accepted connection
|
|
|
dda0b2 |
nbdkit: null[1]: debug: newstyle negotiation: flags: global 0x3
|
|
|
dda0b2 |
NBDMAGICIHAVEOPT
|
|
|
dda0b2 |
xx
|
|
|
dda0b2 |
nbdkit: null[1]: debug: newstyle negotiation: client flags: 0xd0a7878
|
|
|
dda0b2 |
nbdkit: null[1]: error: client requested unknown flags 0xd0a7878
|
|
|
dda0b2 |
Connection closed by foreign host.
|
|
|
dda0b2 |
nbdkit: debug: null: unload plugin
|
|
|
dda0b2 |
|
|
|
dda0b2 |
Signed-off-by: Richard W.M. Jones <rjones@redhat.com>
|
|
|
dda0b2 |
(cherry picked from commit c05686f9577fa91b6a3a4d8c065954ca6fc3fd62)
|
|
|
dda0b2 |
(cherry picked from commit e06cde00659ff97182173d0e33fff784041bcb4a)
|
|
|
dda0b2 |
---
|
|
|
dda0b2 |
src/connections.c | 83 +++++++++++++++++++++++++++----------------------------
|
|
|
dda0b2 |
1 file changed, 40 insertions(+), 43 deletions(-)
|
|
|
dda0b2 |
|
|
|
dda0b2 |
diff --git a/src/connections.c b/src/connections.c
|
|
|
dda0b2 |
index e1ffeff..6e7c080 100644
|
|
|
dda0b2 |
--- a/src/connections.c
|
|
|
dda0b2 |
+++ b/src/connections.c
|
|
|
dda0b2 |
@@ -246,12 +246,6 @@ _handle_single_connection (int sockin, int sockout)
|
|
|
dda0b2 |
if (!conn)
|
|
|
dda0b2 |
goto done;
|
|
|
dda0b2 |
|
|
|
dda0b2 |
- lock_request (conn);
|
|
|
dda0b2 |
- r = backend->open (backend, conn, readonly);
|
|
|
dda0b2 |
- unlock_request (conn);
|
|
|
dda0b2 |
- if (r == -1)
|
|
|
dda0b2 |
- goto done;
|
|
|
dda0b2 |
-
|
|
|
dda0b2 |
/* NB: because of an asynchronous exit backend can be set to NULL at
|
|
|
dda0b2 |
* just about any time.
|
|
|
dda0b2 |
*/
|
|
|
dda0b2 |
@@ -261,17 +255,11 @@ _handle_single_connection (int sockin, int sockout)
|
|
|
dda0b2 |
plugin_name = "(unknown)";
|
|
|
dda0b2 |
threadlocal_set_name (plugin_name);
|
|
|
dda0b2 |
|
|
|
dda0b2 |
- /* Prepare (for filters), called just after open. */
|
|
|
dda0b2 |
- lock_request (conn);
|
|
|
dda0b2 |
- if (backend)
|
|
|
dda0b2 |
- r = backend->prepare (backend, conn);
|
|
|
dda0b2 |
- else
|
|
|
dda0b2 |
- r = 0;
|
|
|
dda0b2 |
- unlock_request (conn);
|
|
|
dda0b2 |
- if (r == -1)
|
|
|
dda0b2 |
- goto done;
|
|
|
dda0b2 |
-
|
|
|
dda0b2 |
- /* Handshake. */
|
|
|
dda0b2 |
+ /* NBD handshake.
|
|
|
dda0b2 |
+ *
|
|
|
dda0b2 |
+ * Note that this calls the backend .open callback when it is safe
|
|
|
dda0b2 |
+ * to do so (eg. after TLS authentication).
|
|
|
dda0b2 |
+ */
|
|
|
dda0b2 |
if (negotiate_handshake (conn) == -1)
|
|
|
dda0b2 |
goto done;
|
|
|
dda0b2 |
|
|
|
dda0b2 |
@@ -408,12 +396,43 @@ free_connection (struct connection *conn)
|
|
|
dda0b2 |
free (conn);
|
|
|
dda0b2 |
}
|
|
|
dda0b2 |
|
|
|
dda0b2 |
+/* Common code used by oldstyle and newstyle protocols to:
|
|
|
dda0b2 |
+ *
|
|
|
dda0b2 |
+ * - call the backend .open method
|
|
|
dda0b2 |
+ *
|
|
|
dda0b2 |
+ * - get the export size
|
|
|
dda0b2 |
+ *
|
|
|
dda0b2 |
+ * - compute the eflags (same between oldstyle and newstyle
|
|
|
dda0b2 |
+ * protocols)
|
|
|
dda0b2 |
+ *
|
|
|
dda0b2 |
+ * The protocols must defer this as late as possible so that
|
|
|
dda0b2 |
+ * unauthorized clients can't cause unnecessary work in .open by
|
|
|
dda0b2 |
+ * simply opening a TCP connection.
|
|
|
dda0b2 |
+ */
|
|
|
dda0b2 |
static int
|
|
|
dda0b2 |
-compute_eflags (struct connection *conn, uint16_t *flags)
|
|
|
dda0b2 |
+protocol_common_open (struct connection *conn,
|
|
|
dda0b2 |
+ uint64_t *exportsize, uint16_t *flags)
|
|
|
dda0b2 |
{
|
|
|
dda0b2 |
+ int64_t size;
|
|
|
dda0b2 |
uint16_t eflags = NBD_FLAG_HAS_FLAGS;
|
|
|
dda0b2 |
int fl;
|
|
|
dda0b2 |
|
|
|
dda0b2 |
+ if (backend->open (backend, conn, readonly) == -1)
|
|
|
dda0b2 |
+ return -1;
|
|
|
dda0b2 |
+
|
|
|
dda0b2 |
+ /* Prepare (for filters), called just after open. */
|
|
|
dda0b2 |
+ if (backend->prepare (backend, conn) == -1)
|
|
|
dda0b2 |
+ return -1;
|
|
|
dda0b2 |
+
|
|
|
dda0b2 |
+ size = backend->get_size (backend, conn);
|
|
|
dda0b2 |
+ if (size == -1)
|
|
|
dda0b2 |
+ return -1;
|
|
|
dda0b2 |
+ if (size < 0) {
|
|
|
dda0b2 |
+ nbdkit_error (".get_size function returned invalid value "
|
|
|
dda0b2 |
+ "(%" PRIi64 ")", size);
|
|
|
dda0b2 |
+ return -1;
|
|
|
dda0b2 |
+ }
|
|
|
dda0b2 |
+
|
|
|
dda0b2 |
fl = backend->can_write (backend, conn);
|
|
|
dda0b2 |
if (fl == -1)
|
|
|
dda0b2 |
return -1;
|
|
|
dda0b2 |
@@ -463,6 +482,7 @@ compute_eflags (struct connection *conn, uint16_t *flags)
|
|
|
dda0b2 |
conn->is_rotational = 1;
|
|
|
dda0b2 |
}
|
|
|
dda0b2 |
|
|
|
dda0b2 |
+ *exportsize = size;
|
|
|
dda0b2 |
*flags = eflags;
|
|
|
dda0b2 |
return 0;
|
|
|
dda0b2 |
}
|
|
|
dda0b2 |
@@ -471,7 +491,6 @@ static int
|
|
|
dda0b2 |
_negotiate_handshake_oldstyle (struct connection *conn)
|
|
|
dda0b2 |
{
|
|
|
dda0b2 |
struct old_handshake handshake;
|
|
|
dda0b2 |
- int64_t r;
|
|
|
dda0b2 |
uint64_t exportsize;
|
|
|
dda0b2 |
uint16_t gflags, eflags;
|
|
|
dda0b2 |
|
|
|
dda0b2 |
@@ -483,21 +502,11 @@ _negotiate_handshake_oldstyle (struct connection *conn)
|
|
|
dda0b2 |
return -1;
|
|
|
dda0b2 |
}
|
|
|
dda0b2 |
|
|
|
dda0b2 |
- r = backend->get_size (backend, conn);
|
|
|
dda0b2 |
- if (r == -1)
|
|
|
dda0b2 |
+ if (protocol_common_open (conn, &exportsize, &eflags) == -1)
|
|
|
dda0b2 |
return -1;
|
|
|
dda0b2 |
- if (r < 0) {
|
|
|
dda0b2 |
- nbdkit_error (".get_size function returned invalid value "
|
|
|
dda0b2 |
- "(%" PRIi64 ")", r);
|
|
|
dda0b2 |
- return -1;
|
|
|
dda0b2 |
- }
|
|
|
dda0b2 |
- exportsize = (uint64_t) r;
|
|
|
dda0b2 |
conn->exportsize = exportsize;
|
|
|
dda0b2 |
|
|
|
dda0b2 |
gflags = 0;
|
|
|
dda0b2 |
- if (compute_eflags (conn, &eflags) < 0)
|
|
|
dda0b2 |
- return -1;
|
|
|
dda0b2 |
-
|
|
|
dda0b2 |
debug ("oldstyle negotiation: flags: global 0x%x export 0x%x",
|
|
|
dda0b2 |
gflags, eflags);
|
|
|
dda0b2 |
|
|
|
dda0b2 |
@@ -607,19 +616,7 @@ send_newstyle_option_reply_info_export (struct connection *conn,
|
|
|
dda0b2 |
static int
|
|
|
dda0b2 |
finish_newstyle_options (struct connection *conn)
|
|
|
dda0b2 |
{
|
|
|
dda0b2 |
- int64_t r;
|
|
|
dda0b2 |
-
|
|
|
dda0b2 |
- r = backend->get_size (backend, conn);
|
|
|
dda0b2 |
- if (r == -1)
|
|
|
dda0b2 |
- return -1;
|
|
|
dda0b2 |
- if (r < 0) {
|
|
|
dda0b2 |
- nbdkit_error (".get_size function returned invalid value "
|
|
|
dda0b2 |
- "(%" PRIi64 ")", r);
|
|
|
dda0b2 |
- return -1;
|
|
|
dda0b2 |
- }
|
|
|
dda0b2 |
- conn->exportsize = (uint64_t) r;
|
|
|
dda0b2 |
-
|
|
|
dda0b2 |
- if (compute_eflags (conn, &conn->eflags) < 0)
|
|
|
dda0b2 |
+ if (protocol_common_open (conn, &conn->exportsize, &conn->eflags) == -1)
|
|
|
dda0b2 |
return -1;
|
|
|
dda0b2 |
|
|
|
dda0b2 |
debug ("newstyle negotiation: flags: export 0x%x", conn->eflags);
|
|
|
dda0b2 |
--
|
|
|
dda0b2 |
1.8.3.1
|
|
|
dda0b2 |
|