Revert "conf: clean up memory containing secrets before freeing"

[PATCH] Revert "conf: clean up memory containing secrets before freeing"

Posted by Peter Krempa 1 year, 7 months ago

Adding supposedly secure cleanup for secrets in anything related to the
XML parser is pointless because there are multiple other un-sanitized
copies of the full XML and the XML parser state at the very least.

Similarly in case RPC was used to transport the XML the RPC buffers are
not sanitized.

Additionally this patch was incomplete as it didn't sanitize the the
password in the cleanup function for virDomainGraphicsAuthDef.

This reverts commit 51f8130d78fde3201b49c02b7095ff918b6e658a.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/conf/domain_conf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 3d1bf18c6c..406c348a00 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -60,7 +60,6 @@
 #include "virdomainsnapshotobjlist.h"
 #include "virdomaincheckpointobjlist.h"
 #include "virutil.h"
-#include "virsecureerase.h"
 #include "virdomainjob.h"

 #define VIR_FROM_THIS VIR_FROM_DOMAIN
@@ -10862,7 +10861,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
             virReportError(VIR_ERR_INTERNAL_ERROR,
                            _("cannot parse password validity time '%s', expect YYYY-MM-DDTHH:MM:SS"),
                            validTo);
-            virSecureEraseString(def->passwd);
             VIR_FREE(def->passwd);
             return -1;
         }
-- 
2.37.1

Re: [PATCH] Revert "conf: clean up memory containing secrets before freeing"

Posted by Ján Tomko 1 year, 7 months ago

On a Wednesday in 2022, Peter Krempa wrote:
>Adding supposedly secure cleanup for secrets in anything related to the
>XML parser is pointless because there are multiple other un-sanitized
>copies of the full XML and the XML parser state at the very least.
>
>Similarly in case RPC was used to transport the XML the RPC buffers are
>not sanitized.
>
>Additionally this patch was incomplete as it didn't sanitize the the

d/the /

>password in the cleanup function for virDomainGraphicsAuthDef.
>
>This reverts commit 51f8130d78fde3201b49c02b7095ff918b6e658a.

Please drop the trailing period.

>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/conf/domain_conf.c | 2 --
> 1 file changed, 2 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano

Re: [PATCH] Revert "conf: clean up memory containing secrets before freeing"

Posted by Martin Kletzander 1 year, 7 months ago

On Wed, Sep 07, 2022 at 01:13:23PM +0200, Peter Krempa wrote:
>Adding supposedly secure cleanup for secrets in anything related to the
>XML parser is pointless because there are multiple other un-sanitized
>copies of the full XML and the XML parser state at the very least.
>
>Similarly in case RPC was used to transport the XML the RPC buffers are
>not sanitized.
>
>Additionally this patch was incomplete as it didn't sanitize the the

s/the the/the/

>password in the cleanup function for virDomainGraphicsAuthDef.
>
>This reverts commit 51f8130d78fde3201b49c02b7095ff918b6e658a.
>
>Signed-off-by: Peter Krempa <pkrempa@redhat.com>
>---
> src/conf/domain_conf.c | 2 --
> 1 file changed, 2 deletions(-)
>
>diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>index 3d1bf18c6c..406c348a00 100644
>--- a/src/conf/domain_conf.c
>+++ b/src/conf/domain_conf.c
>@@ -60,7 +60,6 @@
> #include "virdomainsnapshotobjlist.h"
> #include "virdomaincheckpointobjlist.h"
> #include "virutil.h"
>-#include "virsecureerase.h"
> #include "virdomainjob.h"
>
> #define VIR_FROM_THIS VIR_FROM_DOMAIN
>@@ -10862,7 +10861,6 @@ virDomainGraphicsAuthDefParseXML(xmlNodePtr node,
>             virReportError(VIR_ERR_INTERNAL_ERROR,
>                            _("cannot parse password validity time '%s', expect YYYY-MM-DDTHH:MM:SS"),
>                            validTo);
>-            virSecureEraseString(def->passwd);
>             VIR_FREE(def->passwd);
>             return -1;
>         }
>-- 
>2.37.1
>

Re: [PATCH] Revert "conf: clean up memory containing secrets before freeing"

Posted by Pavel Hrdina 1 year, 7 months ago

On Wed, Sep 07, 2022 at 01:13:23PM +0200, Peter Krempa wrote:
> Adding supposedly secure cleanup for secrets in anything related to the
> XML parser is pointless because there are multiple other un-sanitized
> copies of the full XML and the XML parser state at the very least.
> 
> Similarly in case RPC was used to transport the XML the RPC buffers are
> not sanitized.
> 
> Additionally this patch was incomplete as it didn't sanitize the the
> password in the cleanup function for virDomainGraphicsAuthDef.
> 
> This reverts commit 51f8130d78fde3201b49c02b7095ff918b6e658a.
> 
> Signed-off-by: Peter Krempa <pkrempa@redhat.com>
> ---
>  src/conf/domain_conf.c | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Pavel Hrdina <phrdina@redhat.com>