Most callers are way better off using memset directly additionally few
places didn't even use it to clear sensitive data in the first place
since the name probably sounded as the right thing to use.

Peter Krempa (14):
  hypervFreeInvokeParams: Don't use VIR_DISPOSE_N for freeing 'params'
  libxlMakeDomBuildInfo: Don't use VIR_DISPOSE_N for USB device list
  storage_backend_iscsi(_direct): Properly clear secrets
  libxlMakeNetworkDiskSrc: Avoid use of VIR_DISPOSE_N
  qemuDomainMasterKeyCreate: Don't use VIR_DISPOSE_N on failure
  qemu: domain: Use memset for clearing secrets instead of VIR_DISPOSE_N
  cmdSecretSetValue: Make it obvious that --file, --base64 and
    --interactive are exlcusive
  virsh: cmdSecretSetValue: Rework handling of the secret value
  virsh: cmdSecretGetValue: Use memset instead of VIR_DISPOSE_N
  virStorageBackendRBDOpenRADOSConn: Use memset instead of VIR_DISPOSE_N
  virCryptoEncryptDataAESgnutls: Use memset instead of VIR_DISPOSE_N
  storageBackendCreateQemuImgSecretPath: Use memset instead of
    VIR_DISPOSE_N
  tests: viralloc: Remove testDispose case
  util: viralloc: Remove VIR_DISPOSE(_N)

 src/hyperv/hyperv_wmi.c                    |  4 +-
 src/libvirt_private.syms                   |  1 -
 src/libxl/libxl_conf.c                     |  8 +--
 src/qemu/qemu_domain.c                     | 24 ++++++---
 src/storage/storage_backend_iscsi.c        | 16 +++---
 src/storage/storage_backend_iscsi_direct.c | 17 ++++---
 src/storage/storage_backend_rbd.c          |  5 +-
 src/storage/storage_util.c                 |  5 +-
 src/util/viralloc.c                        | 39 +-------------
 src/util/viralloc.h                        | 27 ----------
 src/util/vircrypto.c                       |  3 +-
 tests/viralloctest.c                       | 34 -------------
 tools/virsh-secret.c                       | 59 ++++++++++------------
 13 files changed, 76 insertions(+), 166 deletions(-)

-- 
2.29.2

On Mon, Feb 01, 2021 at 02:38:52PM +0100, Peter Krempa wrote:
> Most callers are way better off using memset directly additionally few
> places didn't even use it to clear sensitive data in the first place
> since the name probably sounded as the right thing to use.

Although virDispose did indeed use memset(), I don't think we should
be replacing it with use of memset(). This is well known to be subject
to compiler optimization eliminating the call entirely.

We shouldn't have used it in virDispose in the first place, instead
we need to call the platform specific "safe" method for erasing
data. Istead we ought to have been using  explicit_bzero or
memset_s(), or memset_explicitly, or $whatever.

At least with virDispose we would only have one place to fix this
problem, but this with series eliminating it, the callers that need
the secure erase are no longer distinct/visible from general memset
usage.

I think we ought to have a 'virSecureErase' function, that we can
back with the appropriate platform specific call.

If you don't want to get so deeply involved in that, I'd be
fine if this series too a minimialist approach and only introduced

  #define virSecureErase(ptr, len) memset(ptr, 0, len)

and then used virSecureErase intead of memset(). That would at least
make sure we're no worse than today and callers remain easily
identifiable.

Actually checking for the platform specific secure erase functions
and wiring them up could be a separate patch series at a later time. 


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

On Mon, Feb 01, 2021 at 13:52:11 +0000, Daniel Berrange wrote:
> On Mon, Feb 01, 2021 at 02:38:52PM +0100, Peter Krempa wrote:
> > Most callers are way better off using memset directly additionally few
> > places didn't even use it to clear sensitive data in the first place
> > since the name probably sounded as the right thing to use.
> 
> Although virDispose did indeed use memset(), I don't think we should
> be replacing it with use of memset(). This is well known to be subject
> to compiler optimization eliminating the call entirely.
> 
> We shouldn't have used it in virDispose in the first place, instead
> we need to call the platform specific "safe" method for erasing
> data. Istead we ought to have been using  explicit_bzero or
> memset_s(), or memset_explicitly, or $whatever.
> 
> At least with virDispose we would only have one place to fix this
> problem, but this with series eliminating it, the callers that need
> the secure erase are no longer distinct/visible from general memset
> usage.
> 
> I think we ought to have a 'virSecureErase' function, that we can
> back with the appropriate platform specific call.
> 
> If you don't want to get so deeply involved in that, I'd be
> fine if this series too a minimialist approach and only introduced
> 
>   #define virSecureErase(ptr, len) memset(ptr, 0, len)
> 
> and then used virSecureErase intead of memset(). That would at least
> make sure we're no worse than today and callers remain easily
> identifiable.

I will do that but it's worth mentioning that it might give us a false
sense of security since there's a lot of memset usage (prior to this
series, one place is visible in 11/14, where the encryption key used
with gnutls is cleared memset) thus any series wanting to do something
else than memset in virSecureErase will need to go through all of memset
calls anyways.