Coverity was not wrong about the usage of 'a'/'A' modifiers for
virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
leak/double-free, and add test to make sure it works as expected.

This series also cleans up direct access to attributes of virJSONValue
and in the end privatizes the implementation so that all users are
forced to use accessors.

Peter Krempa (9):
  util: json: Fix freeing of objects appended to virJSONValue
  tests: json: Validate that attribute values are properly stolen
  qemu: monitor: Use virJSONValueObjectKeysNumber in
    qemuMonitorJSONGetCPUModelExpansion
  qemu: agent: Avoid unnecessary JSON object type check
  json: Replace access to virJSONValue->type by virJSONValueGetType
  util: json: Add accessor for geting a VIR_JSON_TYPE_NUMBER as string
  util: qemu: Don't access virJSONValue members directly in
    virQEMUBuildCommandLineJSONRecurse
  qemu: monitor: Don't resist stealing 'actions' in
    qemuMonitorJSONTransaction
  util: json: Privatize struct _virJSONValue and sub-structs

 src/libvirt_private.syms     |  1 +
 src/qemu/qemu_agent.c        | 21 +++++-----------
 src/qemu/qemu_block.c        | 22 +++++------------
 src/qemu/qemu_command.c      |  2 +-
 src/qemu/qemu_driver.c       |  4 +--
 src/qemu/qemu_monitor.c      |  4 +--
 src/qemu/qemu_monitor.h      |  2 +-
 src/qemu/qemu_monitor_json.c | 59 ++++++++++++++------------------------------
 src/qemu/qemu_monitor_json.h |  2 +-
 src/util/virjson.c           | 59 +++++++++++++++++++++++++++++++++++++++++---
 src/util/virjson.h           | 39 +----------------------------
 src/util/virqemu.c           | 13 ++++++----
 tests/qemublocktest.c        |  4 +--
 tests/virjsontest.c          | 47 +++++++++++++++++++++++++++++++++++
 14 files changed, 152 insertions(+), 127 deletions(-)

-- 
2.16.2

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Mar 30, 2018 at 12:59:07 +0200, Peter Krempa wrote:
> Coverity was not wrong about the usage of 'a'/'A' modifiers for
> virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
> leak/double-free, and add test to make sure it works as expected.

[1] https://www.redhat.com/archives/libvir-list/2017-November/msg00306.html
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Mar 30, 2018 at 12:59:07PM +0200, Peter Krempa wrote:
> Coverity was not wrong about the usage of 'a'/'A' modifiers for
> virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
> leak/double-free, and add test to make sure it works as expected.

Do you have any idea how to trigger the double-free scenario ? In particular
is it something that a broken / malicious QEMU monitor / guest agent can
cause us todo. If so we'll need to request a CVE assignment for this flaw.


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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 03, 2018 at 11:56:33 +0100, Daniel Berrange wrote:
> On Fri, Mar 30, 2018 at 12:59:07PM +0200, Peter Krempa wrote:
> > Coverity was not wrong about the usage of 'a'/'A' modifiers for
> > virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
> > leak/double-free, and add test to make sure it works as expected.
> 
> Do you have any idea how to trigger the double-free scenario ? In particular
> is it something that a broken / malicious QEMU monitor / guest agent can
> cause us todo. If so we'll need to request a CVE assignment for this flaw.

It can be triggered when we are formatting the virJSONValue using
callers of virJSONValueObjectAddVArgs, so it would require malformed
parameters to a libvirt API and I did not inspec the possibility of
that.

You can see the paths which potentially could hit the double-free in
this patch, since it's modifying all the cases.

All paths where we format anything after an 'a' or 'A' valuea which can
fail after the 'a' or 'A' was successful and then the original value is
freed.

I doubt that it's CVE-material
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 03, 2018 at 13:01:48 +0200, Peter Krempa wrote:
> On Tue, Apr 03, 2018 at 11:56:33 +0100, Daniel Berrange wrote:
> > On Fri, Mar 30, 2018 at 12:59:07PM +0200, Peter Krempa wrote:
> > > Coverity was not wrong about the usage of 'a'/'A' modifiers for
> > > virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
> > > leak/double-free, and add test to make sure it works as expected.
> > 
> > Do you have any idea how to trigger the double-free scenario ? In particular
> > is it something that a broken / malicious QEMU monitor / guest agent can
> > cause us todo. If so we'll need to request a CVE assignment for this flaw.
> 
> It can be triggered when we are formatting the virJSONValue using
> callers of virJSONValueObjectAddVArgs, so it would require malformed
> parameters to a libvirt API and I did not inspec the possibility of
> that.
> 
> You can see the paths which potentially could hit the double-free in
> this patch, since it's modifying all the cases.
> 
> All paths where we format anything after an 'a' or 'A' valuea which can
> fail after the 'a' or 'A' was successful and then the original value is
> freed.

So the calls which match the above condition are from:

qemuBlockStorageSourceGetNBDProps,
qemuBlockStorageSourceGetRBDProps,
qemuBlockStorageSourceGetSshProps,
qemuMonitorJSONSendKey

All of the above format only optional strings ('S') or integers after
the 'a'/'A' argument, so the double free scenario would happen only on a
out-of-memory condition.

> I doubt that it's CVE-material

It's not.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Tue, Apr 03, 2018 at 01:07:29PM +0200, Peter Krempa wrote:
> On Tue, Apr 03, 2018 at 13:01:48 +0200, Peter Krempa wrote:
> > On Tue, Apr 03, 2018 at 11:56:33 +0100, Daniel Berrange wrote:
> > > On Fri, Mar 30, 2018 at 12:59:07PM +0200, Peter Krempa wrote:
> > > > Coverity was not wrong about the usage of 'a'/'A' modifiers for
> > > > virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
> > > > leak/double-free, and add test to make sure it works as expected.
> > > 
> > > Do you have any idea how to trigger the double-free scenario ? In particular
> > > is it something that a broken / malicious QEMU monitor / guest agent can
> > > cause us todo. If so we'll need to request a CVE assignment for this flaw.
> > 
> > It can be triggered when we are formatting the virJSONValue using
> > callers of virJSONValueObjectAddVArgs, so it would require malformed
> > parameters to a libvirt API and I did not inspec the possibility of
> > that.
> > 
> > You can see the paths which potentially could hit the double-free in
> > this patch, since it's modifying all the cases.
> > 
> > All paths where we format anything after an 'a' or 'A' valuea which can
> > fail after the 'a' or 'A' was successful and then the original value is
> > freed.
> 
> So the calls which match the above condition are from:
> 
> qemuBlockStorageSourceGetNBDProps,
> qemuBlockStorageSourceGetRBDProps,
> qemuBlockStorageSourceGetSshProps,
> qemuMonitorJSONSendKey
> 
> All of the above format only optional strings ('S') or integers after
> the 'a'/'A' argument, so the double free scenario would happen only on a
> out-of-memory condition.
> 
> > I doubt that it's CVE-material
> 
> It's not.


Great, thanks for checking

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list