[PATCH 1/3] conf: clean up memory containing secrets before freeing

[Peter Krempa]

On Wed, Sep 07, 2022 at 11:54:59 +0200, Michal Prívozník wrote:
> On 9/6/22 15:48, Jiacheng Jiang wrote:
> > From: jiangjiacheng <jiangjiacheng at huawei.com>
> > 
> > The password may not be valid in the error branch, but for
> > higher security, it's better to clean up the memory before
> > freeing it.
> > 
> > Signed-off-by: jiangjiacheng <jiangjiacheng at huawei.com>
> > ---
> >  src/conf/domain_conf.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index 970cc85ded..d456fd0067 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -60,6 +60,7 @@
> >  #include "virdomainsnapshotobjlist.h"
> >  #include "virdomaincheckpointobjlist.h"
> >  #include "virutil.h"
> > +#include "virsecureerase.h"
> >  
> >  #define VIR_FROM_THIS VIR_FROM_DOMAIN
> >  
> > @@ -10888,6 +10889,7 @@ 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;
> >          }
> 
> There are other 'return -1' statements which leave
> virDomainGraphicsAuthDef partially filled. Eventually, the error leads
> to virDomainGraphicsDefFree() being called which in turn calls
> virDomainGraphicsAuthDefClear() which does not call virSecureEraseString().
> 
> I wonder what we can do about it.

As noted in my reply you've got another copy in the XML parser and yet
another copy in the string that holds the full XML before it was parsed.

Trying to sanitize secrets for anything XML related is pointless.

Even the one piece of code which supposedly seems to make sense
(fetching and encrypting disk secrets) where we sanitize the
un-encrypted secret we got from the secret driver doesn't actually
sanitize the RPC buffers which were used to communicate with the secret
daemon.