| Message ID | 20180413192605.2145-2-nirsof@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Apr 13, 2018 at 10:26:03PM +0300, Nir Soffer wrote: > When a management application expose images using qemu-nbd, it needs a > secure way to allow temporary access to the disk. Using a random export > name can solve this problem: > > nbd://server:10809/22965f19-9ab5-4d18-94e1-cbeb321fa433 > > Assuming that the url is passed to the user in a secure way, and the > user is using TLS to access the image. > > However, since qemu-nbd implements NBD_OPT_LIST, anyone can easily find > the secret export: > > $ nbd-client -l server 10809 > Negotiation: .. > 22965f19-9ab5-4d18-94e1-cbeb321fa433 > > Add a new --nolist option, disabling listing, similar the "allowlist" > nbd-server configuration option. > > When used, listing exports will fail like this: > > $ nbd-client -l localhost 10809 > Negotiation: .. > > E: listing not allowed by server. > Server said: Listing exports is forbidden > > Signed-off-by: Nir Soffer <nirsof@gmail.com> Tested-by: Richard W.M. Jones <rjones@redhat.com> The code looks good to me too, so ACK. Rich. > blockdev-nbd.c | 2 +- > include/block/nbd.h | 1 + > nbd/server.c | 7 +++++++ > qemu-nbd.c | 9 ++++++++- > qemu-nbd.texi | 2 ++ > 5 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/blockdev-nbd.c b/blockdev-nbd.c > index 65a84739ed..b9a885dc4b 100644 > --- a/blockdev-nbd.c > +++ b/blockdev-nbd.c > @@ -37,7 +37,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, > { > qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server"); > nbd_client_new(NULL, cioc, > - nbd_server->tlscreds, NULL, > + nbd_server->tlscreds, NULL, true, > nbd_blockdev_client_closed); > } > > diff --git a/include/block/nbd.h b/include/block/nbd.h > index fcdcd54502..5c6b6272a0 100644 > --- a/include/block/nbd.h > +++ b/include/block/nbd.h > @@ -308,6 +308,7 @@ void nbd_client_new(NBDExport *exp, > QIOChannelSocket *sioc, > QCryptoTLSCreds *tlscreds, > const char *tlsaclname, > + bool allow_list, > void (*close_fn)(NBDClient *, bool)); > void nbd_client_get(NBDClient *client); > void nbd_client_put(NBDClient *client); > diff --git a/nbd/server.c b/nbd/server.c > index 9e1f227178..7b91922d1d 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -115,6 +115,7 @@ struct NBDClient { > > bool structured_reply; > NBDExportMetaContexts export_meta; > + bool allow_list; > > uint32_t opt; /* Current option being negotiated */ > uint32_t optlen; /* remaining length of data in ioc for the option being > @@ -1032,6 +1033,10 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, > case NBD_OPT_LIST: > if (length) { > ret = nbd_reject_length(client, false, errp); > + } else if (!client->allow_list) { > + ret = nbd_negotiate_send_rep_err(client, > + NBD_REP_ERR_POLICY, errp, > + "Listing exports is forbidden"); > } else { > ret = nbd_negotiate_handle_list(client, errp); > } > @@ -2141,6 +2146,7 @@ void nbd_client_new(NBDExport *exp, > QIOChannelSocket *sioc, > QCryptoTLSCreds *tlscreds, > const char *tlsaclname, > + bool allow_list, > void (*close_fn)(NBDClient *, bool)) > { > NBDClient *client; > @@ -2158,6 +2164,7 @@ void nbd_client_new(NBDExport *exp, > object_ref(OBJECT(client->sioc)); > client->ioc = QIO_CHANNEL(sioc); > object_ref(OBJECT(client->ioc)); > + client->allow_list = allow_list; > client->close_fn = close_fn; > > co = qemu_coroutine_create(nbd_co_client_start, client); > diff --git a/qemu-nbd.c b/qemu-nbd.c > index 0af0560ad1..b63d4d9e8b 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -52,6 +52,7 @@ > #define QEMU_NBD_OPT_TLSCREDS 261 > #define QEMU_NBD_OPT_IMAGE_OPTS 262 > #define QEMU_NBD_OPT_FORK 263 > +#define QEMU_NBD_OPT_NOLIST 264 > > #define MBR_SIZE 512 > > @@ -66,6 +67,7 @@ static int shared = 1; > static int nb_fds; > static QIONetListener *server; > static QCryptoTLSCreds *tlscreds; > +static bool allow_list = true; > > static void usage(const char *name) > { > @@ -86,6 +88,7 @@ static void usage(const char *name) > " -v, --verbose display extra debugging information\n" > " -x, --export-name=NAME expose export by name\n" > " -D, --description=TEXT with -x, also export a human-readable description\n" > +" --nolist do not list export\n" > "\n" > "Exposing part of the image:\n" > " -o, --offset=OFFSET offset into the image\n" > @@ -355,7 +358,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, > nb_fds++; > nbd_update_server_watch(); > nbd_client_new(newproto ? NULL : exp, cioc, > - tlscreds, NULL, nbd_client_closed); > + tlscreds, NULL, allow_list, nbd_client_closed); > } > > static void nbd_update_server_watch(void) > @@ -523,6 +526,7 @@ int main(int argc, char **argv) > { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT }, > { "export-name", required_argument, NULL, 'x' }, > { "description", required_argument, NULL, 'D' }, > + { "nolist", no_argument, NULL, QEMU_NBD_OPT_NOLIST }, > { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS }, > { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, > { "trace", required_argument, NULL, 'T' }, > @@ -717,6 +721,9 @@ int main(int argc, char **argv) > case 'D': > export_description = optarg; > break; > + case QEMU_NBD_OPT_NOLIST: > + allow_list = false; > + break; > case 'v': > verbose = 1; > break; > diff --git a/qemu-nbd.texi b/qemu-nbd.texi > index 9a84e81eed..010b29588f 100644 > --- a/qemu-nbd.texi > +++ b/qemu-nbd.texi > @@ -85,6 +85,8 @@ the new style NBD protocol negotiation > @item -D, --description=@var{description} > Set the NBD volume export description, as a human-readable > string. Requires the use of @option{-x} > +@item --nolist > +Do not allow the client to fetch a list of exports from this server. > @item --tls-creds=ID > Enable mandatory TLS encryption for the server by setting the ID > of the TLS credentials object previously created with the --object > -- > 2.14.3
On Fri, Apr 13, 2018 at 10:26:03PM +0300, Nir Soffer wrote: > When a management application expose images using qemu-nbd, it needs a > secure way to allow temporary access to the disk. Using a random export > name can solve this problem: > > nbd://server:10809/22965f19-9ab5-4d18-94e1-cbeb321fa433 > > Assuming that the url is passed to the user in a secure way, and the > user is using TLS to access the image. > > However, since qemu-nbd implements NBD_OPT_LIST, anyone can easily find > the secret export: > > $ nbd-client -l server 10809 > Negotiation: .. > 22965f19-9ab5-4d18-94e1-cbeb321fa433 > > Add a new --nolist option, disabling listing, similar the "allowlist" > nbd-server configuration option. > > When used, listing exports will fail like this: > > $ nbd-client -l localhost 10809 > Negotiation: .. > > E: listing not allowed by server. > Server said: Listing exports is forbidden Essentially this is abusing the export name as a crude authentication token. There are NBD servers that expect NBD_OPT_LIST to always succeeed when they detect that the new style protocol is available. I really hate the idea of making it possible to break the NBD_OPT_LIST functionality via a command line arg like this. Furthermore, applications are *not* considering the export names to be security sensitive data, so will not be taking any precautions to ensure they remain secret, as they would do with authentication credentials. Again I really hate the idea of using NBD exports an an auth credential. So I don't think we should be suggesting that security through obscurity of the export name is a supported approach to securing NBD. I understand the desire to be able to secure NBD exports though, so think we need to come up with some kind of supportable solution for this. There are two approaches we should take - Add support for TLS client certification whitelisting. eg every client has a unique identity based on the distinguished name (dname) in the x509 cert they were issued. The NBD server can be told which of these dnames should be a permitted to connect. This is supported in VNC for years, and I've had patches pending to support this in a QEMU for chardevs NBD and migration for a while. These were stalled on way to convert -object ... syntax into nested QOM objects. - Define a mapping of the SASL protocol ontop NBD. SASL is a generic pluggable authentication mechanism for network protocols. It is already used in libvirt, VNC and SPICE, and would easily fit in with NBD from a conceptual POV. When used in combination with TLS, this offers a wide range of auth mechanisms from simple username+password, to full integration with Kerberos. If this need is urgent, I think we could partially unblock the TLS x509 whitelisting support without much difficulty. We haven't been pushing hard to unblock it simply because no one was urgently blocked by its absence so far. This provides a strong solution, but the difficulty is that the server may not know the x509 dname of the permitted client, which makes it hard to use in practice. SASL with a simple username+password scheme is thus still very compelling to implement, but will obviously take longer due to the amount of code/spec work required. Regards, Daniel
On Mon, Apr 16, 2018 at 11:31:18AM +0100, Daniel P. Berrangé wrote: > Essentially this is abusing the export name as a crude authentication > token. There are NBD servers that expect NBD_OPT_LIST to always succeeed I guess you mean "NBD clients" ... > when they detect that the new style protocol is available. I really hate > the idea of making it possible to break the NBD_OPT_LIST functionality > via a command line arg like this. The specific use case I have in mind is virt-v2v forked an instance of ‘qemu-img convert’ which connects to the NBD server. Of course this does also reveal a flaw in the plan because ... > Furthermore, applications are *not* considering the export names to be > security sensitive data, so will not be taking any precautions to ensure > they remain secret, as they would do with authentication credentials. > Again I really hate the idea of using NBD exports an an auth credential. ‘ps ax’ on the conversion server will reveal the export name/ticket from the qemu-img command line. > So I don't think we should be suggesting that security through obscurity of > the export name is a supported approach to securing NBD. > > I understand the desire to be able to secure NBD exports though, so think > we need to come up with some kind of supportable solution for this. There > are two approaches we should take > > - Add support for TLS client certification whitelisting. eg every client > has a unique identity based on the distinguished name (dname) in the > x509 cert they were issued. The NBD server can be told which of these > dnames should be a permitted to connect. This is supported in VNC for > years, and I've had patches pending to support this in a QEMU for chardevs > NBD and migration for a while. These were stalled on way to convert > -object ... syntax into nested QOM objects. > > - Define a mapping of the SASL protocol ontop NBD. SASL is a > generic pluggable authentication mechanism for network > protocols. It is already used in libvirt, VNC and SPICE, and > would easily fit in with NBD from a conceptual POV. When used in > combination with TLS, this offers a wide range of auth mechanisms > from simple username+password, to full integration with Kerberos. The first one sounds heavyweight but at least implementable from the virt-v2v point of view. The second one sounds like it would be impossible for mere humans to set it up. > If this need is urgent, I think we could partially unblock the TLS x509 > whitelisting support without much difficulty. We haven't been pushing hard > to unblock it simply because no one was urgently blocked by its absence > so far. This provides a strong solution, but the difficulty is that the > server may not know the x509 dname of the permitted client, which makes > it hard to use in practice. Can you clarify what you mean by the last sentence above? Can't we just create a client certificate in virt-v2v and pass that to qemu-img, and at the same time pass the server a list of permitted names? (likely only a single name in practice) > SASL with a simple username+password scheme > is thus still very compelling to implement, but will obviously take longer > due to the amount of code/spec work required. Rich.
On Mon, Apr 16, 2018 at 11:53:41AM +0100, Richard W.M. Jones wrote: > On Mon, Apr 16, 2018 at 11:31:18AM +0100, Daniel P. Berrangé wrote: > > Essentially this is abusing the export name as a crude authentication > > token. There are NBD servers that expect NBD_OPT_LIST to always succeeed > > I guess you mean "NBD clients" ... Sigh, yes, of course. > > when they detect that the new style protocol is available. I really hate > > the idea of making it possible to break the NBD_OPT_LIST functionality > > via a command line arg like this. > > The specific use case I have in mind is virt-v2v forked an instance of > ‘qemu-img convert’ which connects to the NBD server. > > Of course this does also reveal a flaw in the plan because ... > > > Furthermore, applications are *not* considering the export names to be > > security sensitive data, so will not be taking any precautions to ensure > > they remain secret, as they would do with authentication credentials. > > Again I really hate the idea of using NBD exports an an auth credential. > > ‘ps ax’ on the conversion server will reveal the export name/ticket > from the qemu-img command line. Yeah, exactly the kind of problem that hits when you mis-use a piece of traditionally public info as a security credential. > > > So I don't think we should be suggesting that security through obscurity of > > the export name is a supported approach to securing NBD. > > > > I understand the desire to be able to secure NBD exports though, so think > > we need to come up with some kind of supportable solution for this. There > > are two approaches we should take > > > > - Add support for TLS client certification whitelisting. eg every client > > has a unique identity based on the distinguished name (dname) in the > > x509 cert they were issued. The NBD server can be told which of these > > dnames should be a permitted to connect. This is supported in VNC for > > years, and I've had patches pending to support this in a QEMU for chardevs > > NBD and migration for a while. These were stalled on way to convert > > -object ... syntax into nested QOM objects. > > > > - Define a mapping of the SASL protocol ontop NBD. SASL is a > > generic pluggable authentication mechanism for network > > protocols. It is already used in libvirt, VNC and SPICE, and > > would easily fit in with NBD from a conceptual POV. When used in > > combination with TLS, this offers a wide range of auth mechanisms > > from simple username+password, to full integration with Kerberos. > > The first one sounds heavyweight but at least implementable from the > virt-v2v point of view. The second one sounds like it would be > impossible for mere humans to set it up. You'll want TLS no matter what really. All SASL mechanisms, with the exception of Kerberos, require that you have a secure data channel first - which means either UNIX sockets, or TCP with TLS. If you're using SASL for auth you can, however, avoid the need to require x509 client certs. > > If this need is urgent, I think we could partially unblock the TLS x509 > > whitelisting support without much difficulty. We haven't been pushing hard > > to unblock it simply because no one was urgently blocked by its absence > > so far. This provides a strong solution, but the difficulty is that the > > server may not know the x509 dname of the permitted client, which makes > > it hard to use in practice. > > Can you clarify what you mean by the last sentence above? Can't we > just create a client certificate in virt-v2v and pass that to > qemu-img, and at the same time pass the server a list of permitted > names? (likely only a single name in practice) I just mean that at the time the mgmt app sets up the NBD export, it might not know which client is going to use it, so it can't setup a x509 dname whitelist at that time. With SASL and username/password, you don't need to know who will use the export at setup time - you can simply give up username/password at time of use. Regards, Daniel
On 04/13/2018 02:26 PM, Nir Soffer wrote: > When a management application expose images using qemu-nbd, it needs a > secure way to allow temporary access to the disk. Using a random export > name can solve this problem: > > nbd://server:10809/22965f19-9ab5-4d18-94e1-cbeb321fa433 I share Dan's concerns that you are trying to protect information without requiring TLS. If you would just use TLS, then only clients that can authenticate can list the export names; the fact that the name leaks at all means you aren't using TLS, so you are just as vulnerable to a man-in-the-middle attack as you are to the information leak. > > Assuming that the url is passed to the user in a secure way, and the > user is using TLS to access the image. > > However, since qemu-nbd implements NBD_OPT_LIST, anyone can easily find > the secret export: > > $ nbd-client -l server 10809 > Negotiation: .. > 22965f19-9ab5-4d18-94e1-cbeb321fa433 If the server requires TLS, then 'nbd-client -l' already cannot list names without first negotiating TLS (all commands other than NBD_OPT_STARTTLS are rejected with NBD_REP_ERR_TLS_REQD if the server required TLS). Your example is thus invalidating your above assumption that the user is using TLS. > > Add a new --nolist option, disabling listing, similar the "allowlist" > nbd-server configuration option. This may still make sense to implement, but not necessarily for the reasons you are giving. > @@ -86,6 +88,7 @@ static void usage(const char *name) > " -v, --verbose display extra debugging information\n" > " -x, --export-name=NAME expose export by name\n" > " -D, --description=TEXT with -x, also export a human-readable description\n" > +" --nolist do not list export\n" s/export/exports/
On 04/16/2018 06:00 AM, Daniel P. Berrangé wrote: > On Mon, Apr 16, 2018 at 11:53:41AM +0100, Richard W.M. Jones wrote: >> On Mon, Apr 16, 2018 at 11:31:18AM +0100, Daniel P. Berrangé wrote: >>> Essentially this is abusing the export name as a crude authentication >>> token. There are NBD servers that expect NBD_OPT_LIST to always succeeed >> >> I guess you mean "NBD clients" ... > > Sigh, yes, of course. qemu 2.10 and older tries to use NBD_OPT_LIST, but gracefully still tries to connect even if the LIST fails (that is, it's use of NBD_OPT_LIST was for better error handling than what NBD_OPT_EXPORT_NAME gives, and not because it actually needed the list). The recent introduction in qemu 2.11 for support of NBD_OPT_GO means that modern qemu is no longer even attempting NBD_OPT_LIST when talking to a new server. But cross-implementation compatibility is still a concern, and there may indeed be non-qemu clients that choke if LIST fails, even though... > >>> when they detect that the new style protocol is available. I really hate >>> the idea of making it possible to break the NBD_OPT_LIST functionality >>> via a command line arg like this. ...the NBD spec suggests that a client that requires LIST to work is not fully compliant, since NBD_OPT_LIST is an optional feature.
diff --git a/blockdev-nbd.c b/blockdev-nbd.c index 65a84739ed..b9a885dc4b 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -37,7 +37,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, { qio_channel_set_name(QIO_CHANNEL(cioc), "nbd-server"); nbd_client_new(NULL, cioc, - nbd_server->tlscreds, NULL, + nbd_server->tlscreds, NULL, true, nbd_blockdev_client_closed); } diff --git a/include/block/nbd.h b/include/block/nbd.h index fcdcd54502..5c6b6272a0 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -308,6 +308,7 @@ void nbd_client_new(NBDExport *exp, QIOChannelSocket *sioc, QCryptoTLSCreds *tlscreds, const char *tlsaclname, + bool allow_list, void (*close_fn)(NBDClient *, bool)); void nbd_client_get(NBDClient *client); void nbd_client_put(NBDClient *client); diff --git a/nbd/server.c b/nbd/server.c index 9e1f227178..7b91922d1d 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -115,6 +115,7 @@ struct NBDClient { bool structured_reply; NBDExportMetaContexts export_meta; + bool allow_list; uint32_t opt; /* Current option being negotiated */ uint32_t optlen; /* remaining length of data in ioc for the option being @@ -1032,6 +1033,10 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags, case NBD_OPT_LIST: if (length) { ret = nbd_reject_length(client, false, errp); + } else if (!client->allow_list) { + ret = nbd_negotiate_send_rep_err(client, + NBD_REP_ERR_POLICY, errp, + "Listing exports is forbidden"); } else { ret = nbd_negotiate_handle_list(client, errp); } @@ -2141,6 +2146,7 @@ void nbd_client_new(NBDExport *exp, QIOChannelSocket *sioc, QCryptoTLSCreds *tlscreds, const char *tlsaclname, + bool allow_list, void (*close_fn)(NBDClient *, bool)) { NBDClient *client; @@ -2158,6 +2164,7 @@ void nbd_client_new(NBDExport *exp, object_ref(OBJECT(client->sioc)); client->ioc = QIO_CHANNEL(sioc); object_ref(OBJECT(client->ioc)); + client->allow_list = allow_list; client->close_fn = close_fn; co = qemu_coroutine_create(nbd_co_client_start, client); diff --git a/qemu-nbd.c b/qemu-nbd.c index 0af0560ad1..b63d4d9e8b 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -52,6 +52,7 @@ #define QEMU_NBD_OPT_TLSCREDS 261 #define QEMU_NBD_OPT_IMAGE_OPTS 262 #define QEMU_NBD_OPT_FORK 263 +#define QEMU_NBD_OPT_NOLIST 264 #define MBR_SIZE 512 @@ -66,6 +67,7 @@ static int shared = 1; static int nb_fds; static QIONetListener *server; static QCryptoTLSCreds *tlscreds; +static bool allow_list = true; static void usage(const char *name) { @@ -86,6 +88,7 @@ static void usage(const char *name) " -v, --verbose display extra debugging information\n" " -x, --export-name=NAME expose export by name\n" " -D, --description=TEXT with -x, also export a human-readable description\n" +" --nolist do not list export\n" "\n" "Exposing part of the image:\n" " -o, --offset=OFFSET offset into the image\n" @@ -355,7 +358,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, nb_fds++; nbd_update_server_watch(); nbd_client_new(newproto ? NULL : exp, cioc, - tlscreds, NULL, nbd_client_closed); + tlscreds, NULL, allow_list, nbd_client_closed); } static void nbd_update_server_watch(void) @@ -523,6 +526,7 @@ int main(int argc, char **argv) { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT }, { "export-name", required_argument, NULL, 'x' }, { "description", required_argument, NULL, 'D' }, + { "nolist", no_argument, NULL, QEMU_NBD_OPT_NOLIST }, { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS }, { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS }, { "trace", required_argument, NULL, 'T' }, @@ -717,6 +721,9 @@ int main(int argc, char **argv) case 'D': export_description = optarg; break; + case QEMU_NBD_OPT_NOLIST: + allow_list = false; + break; case 'v': verbose = 1; break; diff --git a/qemu-nbd.texi b/qemu-nbd.texi index 9a84e81eed..010b29588f 100644 --- a/qemu-nbd.texi +++ b/qemu-nbd.texi @@ -85,6 +85,8 @@ the new style NBD protocol negotiation @item -D, --description=@var{description} Set the NBD volume export description, as a human-readable string. Requires the use of @option{-x} +@item --nolist +Do not allow the client to fetch a list of exports from this server. @item --tls-creds=ID Enable mandatory TLS encryption for the server by setting the ID of the TLS credentials object previously created with the --object
When a management application expose images using qemu-nbd, it needs a secure way to allow temporary access to the disk. Using a random export name can solve this problem: nbd://server:10809/22965f19-9ab5-4d18-94e1-cbeb321fa433 Assuming that the url is passed to the user in a secure way, and the user is using TLS to access the image. However, since qemu-nbd implements NBD_OPT_LIST, anyone can easily find the secret export: $ nbd-client -l server 10809 Negotiation: .. 22965f19-9ab5-4d18-94e1-cbeb321fa433 Add a new --nolist option, disabling listing, similar the "allowlist" nbd-server configuration option. When used, listing exports will fail like this: $ nbd-client -l localhost 10809 Negotiation: .. E: listing not allowed by server. Server said: Listing exports is forbidden Signed-off-by: Nir Soffer <nirsof@gmail.com> --- blockdev-nbd.c | 2 +- include/block/nbd.h | 1 + nbd/server.c | 7 +++++++ qemu-nbd.c | 9 ++++++++- qemu-nbd.texi | 2 ++ 5 files changed, 19 insertions(+), 2 deletions(-)