[PATCH v2 0/3] ui: add support for 'secret' object to provide VNC/SPICE passwords

[PATCH v2 0/3] ui: add support for 'secret' object to provide VNC/SPICE passwords

[Daniel P. Berrangé]

This fixes a long standing limitation of the VNC/SPICE code which was
unable to securely accept passswords on the CLI, instead requiring use
of separate monitor commands after startup.

In v2:

 - Dropped patch removing ACL commands, as it will move to a bigger
   deprecation cleanup series
 - Rebased and resolved conflicts

Daniel P. Berrangé (3):
  ui: introduce "password-secret" option for VNC servers
  ui: introduce "password-secret" option for SPICE server
  ui: deprecate "password" option for SPICE server

 docs/system/deprecated.rst |  8 ++++++++
 qemu-options.hx            | 18 ++++++++++++++++--
 ui/spice-core.c            | 32 ++++++++++++++++++++++++++++++--
 ui/vnc.c                   | 23 ++++++++++++++++++++++-
 4 files changed, 76 insertions(+), 5 deletions(-)

-- 
2.29.2

[PATCH v2 1/3] ui: introduce "password-secret" option for VNC servers

[Daniel P. Berrangé]

Currently when using VNC the "password" flag turns on password based
authentication. The actual password has to be provided separately via
the monitor.

This introduces a "password-secret" option which lets the password be
provided up front.

  $QEMU --object secret,id=vncsec0,file=passwd.txt \
        --vnc localhost:0,password-secret=vncsec0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-options.hx |  5 +++++
 ui/vnc.c        | 23 ++++++++++++++++++++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 90801286c6..722d56eab3 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -2165,6 +2165,11 @@ SRST
         time to allow <protocol> password to expire immediately or never
         expire.
 
+    ``password-secret=<secret-id>``
+        Require that password based authentication is used for client
+        connections, using the password provided by the ``secret``
+        object identified by ``secret-id``.
+
     ``tls-creds=ID``
         Provides the ID of a set of TLS credentials to use to secure the
         VNC server. They will apply to both the normal VNC server socket
diff --git a/ui/vnc.c b/ui/vnc.c
index 310abc9378..e8e3426a65 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -48,6 +48,7 @@
 #include "crypto/tlscredsanon.h"
 #include "crypto/tlscredsx509.h"
 #include "crypto/random.h"
+#include "crypto/secret_common.h"
 #include "qom/object_interfaces.h"
 #include "qemu/cutils.h"
 #include "qemu/help_option.h"
@@ -3459,6 +3460,9 @@ static QemuOptsList qemu_vnc_opts = {
         },{
             .name = "password",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "reverse",
             .type = QEMU_OPT_BOOL,
@@ -3931,6 +3935,7 @@ void vnc_display_open(const char *id, Error **errp)
     int lock_key_sync = 1;
     int key_delay_ms;
     const char *audiodev;
+    const char *passwordSecret;
 
     if (!vd) {
         error_setg(errp, "VNC display not active");
@@ -3948,7 +3953,23 @@ void vnc_display_open(const char *id, Error **errp)
         goto fail;
     }
 
-    password = qemu_opt_get_bool(opts, "password", false);
+
+    passwordSecret = qemu_opt_get(opts, "password-secret");
+    if (passwordSecret) {
+        if (qemu_opt_get(opts, "password")) {
+            error_setg(errp,
+                       "'password' flag is redundant with 'password-secret'");
+            goto fail;
+        }
+        vd->password = qcrypto_secret_lookup_as_utf8(passwordSecret,
+                                                     errp);
+        if (!vd->password) {
+            goto fail;
+        }
+        password = true;
+    } else {
+        password = qemu_opt_get_bool(opts, "password", false);
+    }
     if (password) {
         if (fips_get_state()) {
             error_setg(errp,
-- 
2.29.2

[PATCH v2 2/3] ui: introduce "password-secret" option for SPICE server

[Daniel P. Berrangé]

Currently when using SPICE the "password" option provides the password
in plain text on the command line. This is insecure as it is visible
to all processes on the host. As an alternative, the password can be
provided separately via the monitor.

This introduces a "password-secret" option which lets the password be
provided up front.

  $QEMU --object secret,id=vncsec0,file=passwd.txt \
        --spice port=5901,password-secret=vncsec0

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-options.hx |  9 +++++++--
 ui/spice-core.c | 30 ++++++++++++++++++++++++++++--
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 722d56eab3..77bb834e37 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1899,7 +1899,8 @@ DEF("spice", HAS_ARG, QEMU_OPTION_spice,
     "       [,tls-ciphers=<list>]\n"
     "       [,tls-channel=[main|display|cursor|inputs|record|playback]]\n"
     "       [,plaintext-channel=[main|display|cursor|inputs|record|playback]]\n"
-    "       [,sasl=on|off][,password=<secret>][,disable-ticketing=on|off]\n"
+    "       [,sasl=on|off][,disable-ticketing=on|off]\n"
+    "       [,password=<string>][,password-secret=<secret-id>]\n"
     "       [,image-compression=[auto_glz|auto_lz|quic|glz|lz|off]]\n"
     "       [,jpeg-wan-compression=[auto|never|always]]\n"
     "       [,zlib-glz-wan-compression=[auto|never|always]]\n"
@@ -1924,9 +1925,13 @@ SRST
     ``ipv4=on|off``; \ ``ipv6=on|off``; \ ``unix=on|off``
         Force using the specified IP version.
 
-    ``password=<secret>``
+    ``password=<string>``
         Set the password you need to authenticate.
 
+    ``password-secret=<secret-id>``
+        Set the ID of the ``secret`` object containing the password
+        you need to authenticate.
+
     ``sasl=on|off``
         Require that the client use SASL to authenticate with the spice.
         The exact choice of authentication method used is controlled
diff --git a/ui/spice-core.c b/ui/spice-core.c
index beee932f55..7f0e005ca9 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -34,6 +34,7 @@
 #include "qapi/qapi-events-ui.h"
 #include "qemu/notify.h"
 #include "qemu/option.h"
+#include "crypto/secret_common.h"
 #include "migration/misc.h"
 #include "hw/pci/pci_bus.h"
 #include "ui/spice-display.h"
@@ -415,6 +416,9 @@ static QemuOptsList qemu_spice_opts = {
         },{
             .name = "password",
             .type = QEMU_OPT_STRING,
+        },{
+            .name = "password-secret",
+            .type = QEMU_OPT_STRING,
         },{
             .name = "disable-ticketing",
             .type = QEMU_OPT_BOOL,
@@ -636,7 +640,9 @@ void qemu_spice_display_init_done(void)
 static void qemu_spice_init(void)
 {
     QemuOpts *opts = QTAILQ_FIRST(&qemu_spice_opts.head);
-    const char *password, *str, *x509_dir, *addr,
+    char *password = NULL;
+    const char *passwordSecret;
+    const char *str, *x509_dir, *addr,
         *x509_key_password = NULL,
         *x509_dh_file = NULL,
         *tls_ciphers = NULL;
@@ -663,7 +669,26 @@ static void qemu_spice_init(void)
         error_report("spice tls-port is out of range");
         exit(1);
     }
-    password = qemu_opt_get(opts, "password");
+    passwordSecret = qemu_opt_get(opts, "password-secret");
+    if (passwordSecret) {
+        Error *local_err = NULL;
+        if (qemu_opt_get(opts, "password")) {
+            error_report("'password' option is mutually exclusive with "
+                         "'password-secret'");
+            exit(1);
+        }
+        password = qcrypto_secret_lookup_as_utf8(passwordSecret,
+                                                 &local_err);
+        if (!password) {
+            error_report_err(local_err);
+            exit(1);
+        }
+    } else {
+        str = qemu_opt_get(opts, "password");
+        if (str) {
+            password = g_strdup(str);
+        }
+    }
 
     if (tls_port) {
         x509_dir = qemu_opt_get(opts, "x509-dir");
@@ -809,6 +834,7 @@ static void qemu_spice_init(void)
     g_free(x509_key_file);
     g_free(x509_cert_file);
     g_free(x509_cacert_file);
+    g_free(password);
 
 #ifdef HAVE_SPICE_GL
     if (qemu_opt_get_bool(opts, "gl", 0)) {
-- 
2.29.2

[PATCH v2 3/3] ui: deprecate "password" option for SPICE server

[Daniel P. Berrangé]

With the new "password-secret" option, there is no reason to use the old
inecure "password" option with -spice, so it can be deprecated.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/system/deprecated.rst | 8 ++++++++
 qemu-options.hx            | 4 ++++
 ui/spice-core.c            | 2 ++
 3 files changed, 14 insertions(+)

diff --git a/docs/system/deprecated.rst b/docs/system/deprecated.rst
index 241b28a521..e742c8d311 100644
--- a/docs/system/deprecated.rst
+++ b/docs/system/deprecated.rst
@@ -166,6 +166,14 @@ Using ``-M kernel-irqchip=off`` with x86 machine types that include a local
 APIC is deprecated.  The ``split`` setting is supported, as is using
 ``-M kernel-irqchip=off`` with the ISA PC machine type.
 
+``-spice password=string`` (since 6.0)
+''''''''''''''''''''''''''''''''''''''
+
+This option is insecure because the SPICE password remains visible in
+the process listing. This is replaced by the new ``password-secret``
+option which lets the password be securely provided on the command
+line using a ``secret`` object instance.
+
 QEMU Machine Protocol (QMP) commands
 ------------------------------------
 
diff --git a/qemu-options.hx b/qemu-options.hx
index 77bb834e37..48382a8a2a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1928,6 +1928,10 @@ SRST
     ``password=<string>``
         Set the password you need to authenticate.
 
+        This option is deprecated and insecure because it leaves the
+        password visible in the process listing. Use ``password-secret``
+        instead.
+
     ``password-secret=<secret-id>``
         Set the ID of the ``secret`` object containing the password
         you need to authenticate.
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 7f0e005ca9..235d61f0c1 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -686,6 +686,8 @@ static void qemu_spice_init(void)
     } else {
         str = qemu_opt_get(opts, "password");
         if (str) {
+            warn_report("'password' option is deprecated and insecure, "
+                        "use 'password-secret' instead");
             password = g_strdup(str);
         }
     }
-- 
2.29.2

Re: [PATCH v2 0/3] ui: add support for 'secret' object to provide VNC/SPICE passwords

[Gerd Hoffmann]

On Thu, Mar 11, 2021 at 11:43:40AM +0000, Daniel P. Berrangé wrote:
> This fixes a long standing limitation of the VNC/SPICE code which was
> unable to securely accept passswords on the CLI, instead requiring use
> of separate monitor commands after startup.
> 
> In v2:
> 
>  - Dropped patch removing ACL commands, as it will move to a bigger
>    deprecation cleanup series
>  - Rebased and resolved conflicts

Added to UI queue.

thanks,
  Gerd