[Libguestfs] [nbdkit PATCH 3/5] filters: Allow nbdkit_next_context_open outside client connection
Eric Blake
eblake at redhat.com
Fri May 7 01:59:38 UTC 2021
Until now, attempts to create a new context outside a client
connection (such as during .after_fork) triggered an assertion
failure: backend_open insisted on having a valid connection. Even if
we merely drop that assertion, there are other issues to worry about:
nbdkit_is_tls() depends on the current connection to know if the
client negotiated TLS; if the backend interns a string with
nbdkit_string_intern() while there is a current connection, then we
risk use-after-free since that string's lifetime ends when the client
disconnects even if the context stays around; and so on. Since we
never promised filter ABI stability, we can address this by adding a
parameter to state the intent of whether the filter intends to share a
single backend context among multiple client connections. Existing
uses all pass false (the context lifetime is tied to the client
connection, so life continues as before), but we now open the door to
a filter that multiplexes connections, where shared connections now
have a global interned string lifetime, a choice of TLS determined by
the command line, and better documentation.
The handling of nbdkit_is_tls is the most interesting aspect: if a
user were to mix a multiplexing filter in front of the tls-fallback
filter, we want to never expose data from the real plugin unless
--tls=require was on the command line, because otherwise we'd have a
lot more code to audit and touch to make sure the multiplexing filter
doesn't accidentally leak data to an insecure client. But if the
tls-fallback filter is in front of the multiplexing filter, things
should just work: the multiplexing filter is not reached except by TLS
clients.
---
docs/nbdkit-filter.pod | 21 +++++++++++++++++++--
include/nbdkit-filter.h | 2 +-
server/internal.h | 5 ++++-
server/backend.c | 27 ++++++++++++++++++++-------
server/filters.c | 10 +++++++---
server/plugins.c | 19 ++++++++++---------
server/protocol-handshake.c | 2 +-
server/public.c | 24 ++++++++++++++++--------
server/test-public.c | 12 +++++++++++-
server/threadlocal.c | 9 +++++++++
filters/retry/retry.c | 2 +-
tests/test-layers-filter.c | 2 +-
12 files changed, 100 insertions(+), 35 deletions(-)
diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 9816f4e8..d2cdde13 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -216,7 +216,8 @@ backend pointer has a stable lifetime from the time of C<.after_fork>
until C<.unload>.
nbdkit_next *nbdkit_next_context_open (nbdkit_backend *backend,
- int readonly, const char *exportname);
+ int readonly, const char *exportname,
+ int shared);
This function attempts to open a new context into the plugin in
relation to the filter's current C<backend>. The C<readonly> and
@@ -227,6 +228,22 @@ C<nbdkit_context_set_next>. The filter should be careful to not
violate any threading model restrictions of the plugin if it opens
more than one context.
+If C<shared> is false, this function must be called while servicing an
+existing client connection, and the new context will share the same
+connection details (export name, tls status, and shorter interned
+string lifetimes) as the current connection, and thus should not be
+used after the client connection ends. Conversely, if C<shared> is
+true, this function may be called outside of a current client
+connection (such as during C<.after_fork>), and the resulting context
+may be freely shared among multiple client connections. In shared
+mode, it will not be possible for the plugin to differentiate content
+based on the client export name, the result of the plugin calling
+nbdkit_is_tls() will depend solely whether C<--tls=require> was on the
+command line, the lifetime of interned strings (via
+C<nbdkit_strdup_intern> and friends) lasts for the life of the filter,
+and the filter must take care to not expose potentially-secure
+information from the backend to an insecure client.
+
void nbdkit_next_context_close (nbdkit_next *next);
This function closes a context into the plugin. If the context has
@@ -582,7 +599,7 @@ functionality can be obtained manually (other than error checking) by
using the following:
nbdkit_context_set_next (context, nbdkit_next_context_open
- (nbdkit_context_get_backend (context), readonly, exportname));
+ (nbdkit_context_get_backend (context), readonly, exportname, false));
The value of C<context> in this call has a lifetime that lasts until
the counterpart C<.close>, and it is this value that may be passed to
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 662f52cb..a91de88f 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -162,7 +162,7 @@ NBDKIT_EXTERN_DECL (nbdkit_backend *, nbdkit_context_get_backend,
(nbdkit_context *context));
NBDKIT_EXTERN_DECL (nbdkit_next *, nbdkit_next_context_open,
(nbdkit_backend *backend, int readonly,
- const char *exportname));
+ const char *exportname, int shared));
NBDKIT_EXTERN_DECL (void, nbdkit_next_context_close, (nbdkit_next *next));
NBDKIT_EXTERN_DECL (nbdkit_next *, nbdkit_context_set_next,
(nbdkit_context *context, nbdkit_next *next));
diff --git a/server/internal.h b/server/internal.h
index a8283195..d2ddf271 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -208,6 +208,7 @@ struct context {
void *handle; /* Plugin or filter handle. */
struct backend *b; /* Backend that provided handle. */
struct context *c_next; /* Underlying context, only when b->next != NULL. */
+ struct connection *conn; /* Active connection at context creation, if any. */
unsigned char state; /* Bitmask of HANDLE_* values */
@@ -418,7 +419,8 @@ extern const char *backend_default_export (struct backend *b, int readonly)
* exportname if they need to refer to it later.
*/
extern struct context *backend_open (struct backend *b,
- int readonly, const char *exportname)
+ int readonly, const char *exportname,
+ int shared)
__attribute__((__nonnull__ (1, 3)));
extern int backend_prepare (struct context *c)
__attribute__((__nonnull__ (1)));
@@ -526,6 +528,7 @@ extern int threadlocal_get_error (void);
extern void *threadlocal_buffer (size_t size);
extern void threadlocal_set_conn (struct connection *conn);
extern struct connection *threadlocal_get_conn (void);
+extern struct context *threadlocal_get_context (void);
extern struct context *threadlocal_push_context (struct context *ctx);
extern void threadlocal_pop_context (struct context **ctx);
diff --git a/server/backend.c b/server/backend.c
index 2244df7a..73994f56 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -234,24 +234,37 @@ static struct nbdkit_next_ops next_ops = {
};
struct context *
-backend_open (struct backend *b, int readonly, const char *exportname)
+backend_open (struct backend *b, int readonly, const char *exportname,
+ int shared)
{
- GET_CONN;
- struct context *c = malloc (sizeof *c);
- PUSH_CONTEXT_FOR_SCOPE (c);
+ struct connection *conn = threadlocal_get_conn ();
+ bool using_tls;
+ struct context *c;
+ if (!shared && !conn) {
+ nbdkit_error ("attempt to open non-shared context outside a connection");
+ return NULL;
+ }
+ if (shared)
+ using_tls = tls == 2;
+ else
+ using_tls = conn->using_tls;
+
+ c = malloc (sizeof *c);
if (c == NULL) {
nbdkit_error ("malloc: %m");
return NULL;
}
+ PUSH_CONTEXT_FOR_SCOPE (c);
controlpath_debug ("%s: open readonly=%d exportname=\"%s\" tls=%d",
- b->name, readonly, exportname, conn->using_tls);
+ b->name, readonly, exportname, using_tls);
c->next = next_ops;
c->handle = NULL;
c->b = b;
c->c_next = NULL;
+ c->conn = shared ? NULL : conn;
c->state = 0;
c->exportsize = -1;
c->can_write = readonly ? 0 : -1;
@@ -266,7 +279,7 @@ backend_open (struct backend *b, int readonly, const char *exportname)
c->can_cache = -1;
/* Determine the canonical name for default export */
- if (!*exportname) {
+ if (!*exportname && conn) {
exportname = backend_default_export (b, readonly);
if (exportname == NULL) {
nbdkit_error ("default export (\"\") not permitted");
@@ -278,7 +291,7 @@ backend_open (struct backend *b, int readonly, const char *exportname)
/* Most filters will call next_open first, resulting in
* inner-to-outer ordering.
*/
- c->handle = b->open (c, readonly, exportname, conn->using_tls);
+ c->handle = b->open (c, readonly, exportname, using_tls);
controlpath_debug ("%s: open returned handle %p", b->name, c->handle);
if (c->handle == NULL) {
diff --git a/server/filters.c b/server/filters.c
index 136fa672..1df8e1c9 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -250,7 +250,8 @@ static int
next_open (struct context *c, int readonly, const char *exportname)
{
struct backend *b = nbdkit_context_get_backend (c);
- struct context *c_next = nbdkit_next_context_open (b, readonly, exportname);
+ struct context *c_next = nbdkit_next_context_open (b, readonly, exportname,
+ false);
struct context *old;
if (c_next == NULL)
@@ -691,10 +692,13 @@ nbdkit_context_get_backend (struct context *c)
struct context *
nbdkit_next_context_open (struct backend *b,
- int readonly, const char *exportname)
+ int readonly, const char *exportname, int shared)
{
+ struct context *c = threadlocal_get_context ();
+
assert (b);
- return backend_open (b, readonly, exportname);
+ assert (!c || b == c->b->next);
+ return backend_open (b, readonly, exportname, shared || !c || !c->conn);
}
void
diff --git a/server/plugins.c b/server/plugins.c
index b5cb5971..457c8c86 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -313,13 +313,11 @@ static void *
plugin_open (struct context *c, int readonly, const char *exportname,
int is_tls)
{
- GET_CONN;
struct backend *b = c->b;
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
void *r;
assert (p->plugin.open != NULL);
- assert (conn->exportname == NULL);
/* Save the exportname since the lifetime of the string passed in
* here is likely to be brief. In addition this provides a place
@@ -333,13 +331,16 @@ plugin_open (struct context *c, int readonly, const char *exportname,
* will still need to save the export name in the handle because of
* the lifetime issue.
*/
- conn->exportname = nbdkit_strdup_intern (exportname);
- if (conn->exportname == NULL)
- return NULL;
+ if (c->conn) {
+ assert (c->conn->exportname == NULL);
+ c->conn->exportname = nbdkit_strdup_intern (exportname);
+ if (c->conn->exportname == NULL)
+ return NULL;
+ }
r = p->plugin.open (readonly);
- if (r == NULL)
- conn->exportname = NULL;
+ if (r == NULL && c->conn)
+ c->conn->exportname = NULL;
return r;
}
@@ -362,14 +363,14 @@ plugin_finalize (struct context *c)
static void
plugin_close (struct context *c)
{
- GET_CONN;
struct backend *b = c->b;
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
assert (c->handle);
if (p->plugin.close)
p->plugin.close (c->handle);
- conn->exportname = NULL;
+ if (c->conn)
+ c->conn->exportname = NULL;
}
static const char *
diff --git a/server/protocol-handshake.c b/server/protocol-handshake.c
index e0183173..67762192 100644
--- a/server/protocol-handshake.c
+++ b/server/protocol-handshake.c
@@ -80,7 +80,7 @@ protocol_common_open (uint64_t *exportsize, uint16_t *flags,
uint16_t eflags = NBD_FLAG_HAS_FLAGS;
int fl;
- conn->top_context = backend_open (top, read_only, exportname);
+ conn->top_context = backend_open (top, read_only, exportname, false);
if (conn->top_context == NULL)
return -1;
diff --git a/server/public.c b/server/public.c
index 9608600e..af55c4bc 100644
--- a/server/public.c
+++ b/server/public.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2021 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -759,14 +759,14 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
const char *
nbdkit_export_name (void)
{
- struct connection *conn = threadlocal_get_conn ();
+ struct context *c = threadlocal_get_context ();
- if (!conn) {
+ if (!c || !c->conn) {
nbdkit_error ("no connection in this thread");
return NULL;
}
- return conn->exportname;
+ return c->conn->exportname;
}
/* This function will be deprecated for API V3 users. The preferred
@@ -775,14 +775,21 @@ nbdkit_export_name (void)
int
nbdkit_is_tls (void)
{
- struct connection *conn = threadlocal_get_conn ();
+ struct context *c = threadlocal_get_context ();
- if (!conn) {
+ if (!c) {
nbdkit_error ("no connection in this thread");
return -1;
}
- return conn->using_tls;
+ if (!c->conn) {
+ /* If a filter opened this backend outside of a client connection,
+ * then we can only claim tls when the command line required it.
+ */
+ return tls == 2;
+ }
+
+ return c->conn->using_tls;
}
int
@@ -980,7 +987,8 @@ free_interns (void)
static const char *
add_intern (char *str)
{
- struct connection *conn = threadlocal_get_conn ();
+ struct context *c = threadlocal_get_context ();
+ struct connection *conn = c ? c->conn : NULL;
string_vector *list = conn ? &conn->interns : &global_interns;
if (string_vector_append (list, str) == -1) {
diff --git a/server/test-public.c b/server/test-public.c
index 141e9eb5..fef12043 100644
--- a/server/test-public.c
+++ b/server/test-public.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2018-2020 Red Hat Inc.
+ * Copyright (C) 2018-2021 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -75,6 +75,12 @@ threadlocal_get_conn (void)
abort ();
}
+struct context *
+threadlocal_get_context (void)
+{
+ abort ();
+}
+
int
connection_get_status (void)
{
@@ -87,6 +93,10 @@ backend_default_export (struct backend *b, int readonly)
abort ();
}
+int tls;
+
+/* Unit tests. */
+
static bool
test_nbdkit_parse_size (void)
{
diff --git a/server/threadlocal.c b/server/threadlocal.c
index 644239b7..ad2ffd09 100644
--- a/server/threadlocal.c
+++ b/server/threadlocal.c
@@ -229,6 +229,15 @@ threadlocal_get_conn (void)
return threadlocal ? threadlocal->conn : NULL;
}
+/* Get the current context associated with this thread, if available */
+struct context *
+threadlocal_get_context (void)
+{
+ struct threadlocal *threadlocal = pthread_getspecific (threadlocal_key);
+
+ return threadlocal ? threadlocal->ctx : NULL;
+}
+
/* Set (or clear) the context using the current thread */
struct context *
threadlocal_push_context (struct context *ctx)
diff --git a/filters/retry/retry.c b/filters/retry/retry.c
index f1c5d37f..fb1d0526 100644
--- a/filters/retry/retry.c
+++ b/filters/retry/retry.c
@@ -227,7 +227,7 @@ do_retry (struct retry_handle *h, struct retry_data *data,
/* Open a new connection. */
new_next = nbdkit_next_context_open (nbdkit_context_get_backend (h->context),
h->readonly || force_readonly,
- h->exportname);
+ h->exportname, false);
if (new_next == NULL) {
*err = ESHUTDOWN;
goto again;
diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c
index 25ecb585..565b5616 100644
--- a/tests/test-layers-filter.c
+++ b/tests/test-layers-filter.c
@@ -152,7 +152,7 @@ test_layers_filter_open (nbdkit_next_open *next, nbdkit_context *nxdata,
backend = nbdkit_context_get_backend (nxdata);
assert (backend != NULL);
- n = nbdkit_next_context_open (backend, readonly, exportname);
+ n = nbdkit_next_context_open (backend, readonly, exportname, 0);
if (n == NULL) {
free (h);
return NULL;
--
2.31.1
More information about the Libguestfs
mailing list