It's insecure to log password, nomatter the password is encrypted or
not. And do not log it even in debug mode, in the consideration of
resilience, surposing that the log mode has been modified by the
attacker.

Signed-off-by: Zhang Bo <oscar.zhangbo@huawei.com>
---
 src/libvirt-domain.c    | 3 +--
 src/qemu/qemu_monitor.c | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
index a12809c2d5..e2a57c178b 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -11340,8 +11340,7 @@ virDomainSetUserPassword(virDomainPtr dom,
                          const char *password,
                          unsigned int flags)
 {
-    VIR_DOMAIN_DEBUG(dom, "user=%s, password=%s, flags=0x%x",
-                     NULLSTR(user), NULLSTR(password), flags);
+    VIR_DOMAIN_DEBUG(dom, "user=%s, flags=0x%x", NULLSTR(user), flags);
 
     virResetLastError();
 
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 9c853ccb93..9bfaf53b65 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -2241,8 +2241,7 @@ qemuMonitorSetPassword(qemuMonitorPtr mon,
     if (!protocol)
         return -1;
 
-    VIR_DEBUG("protocol=%s, password=%p, action_if_connected=%s",
-              protocol, password, action_if_connected);
+    VIR_DEBUG("protocol=%s, action_if_connected=%s", protocol, action_if_connected);
 
     QEMU_CHECK_MONITOR(mon);
 
-- 
2.23.0.windows.1

On Tue, May 12, 2020 at 21:06:14 +0800, Zhang Bo wrote:
> It's insecure to log password, nomatter the password is encrypted or
> not. And do not log it even in debug mode, in the consideration of
> resilience, surposing that the log mode has been modified by the
> attacker.

That is true ...

> 
> Signed-off-by: Zhang Bo <oscar.zhangbo@huawei.com>
> ---
>  src/libvirt-domain.c    | 3 +--
>  src/qemu/qemu_monitor.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index a12809c2d5..e2a57c178b 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -11340,8 +11340,7 @@ virDomainSetUserPassword(virDomainPtr dom,
>                           const char *password,
>                           unsigned int flags)
>  {
> -    VIR_DOMAIN_DEBUG(dom, "user=%s, password=%s, flags=0x%x",
> -                     NULLSTR(user), NULLSTR(password), flags);

Yes, this is wrong.

> +    VIR_DOMAIN_DEBUG(dom, "user=%s, flags=0x%x", NULLSTR(user), flags);
>  
>      virResetLastError();
>  
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 9c853ccb93..9bfaf53b65 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2241,8 +2241,7 @@ qemuMonitorSetPassword(qemuMonitorPtr mon,
>      if (!protocol)
>          return -1;
>  
> -    VIR_DEBUG("protocol=%s, password=%p, action_if_connected=%s",
> -              protocol, password, action_if_connected);

Here we just log the pointer, so this is okay. There's now way to get
the password back from this. One could argue it's pointless to log the
pointer though.

Unfortunatley in debug mode we still log the full monitor command sent
to qemu, where you'll get the password.

I don't think there's a reasonable way to avoid it unless we use the
qcryptosecret interface of qemu which we use for storage auth and
encryption keys.