[libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects

[John Ferlan]

On 09/20/2017 11:26 AM, Erik Skultety wrote:
> On Wed, Sep 20, 2017 at 10:41:49AM -0400, John Ferlan wrote:
>>
>>
>> On 09/20/2017 09:11 AM, Erik Skultety wrote:
>>> On Wed, Sep 20, 2017 at 05:32:29AM -0700, Ashish Mittal wrote:
>>>> Passing a NULL value for the argument secAlias to the function
>>>> qemuDomainGetTLSObjects would cause a segmentation fault in
>>>> libvirtd.
>>>>
>>>> Changed code to not dereference a NULL secAlias.
>>>>
>>>> Signed-off-by: Ashish Mittal <ashmit602 at gmail.com>
>>>> ---
>>>>  src/qemu/qemu_hotplug.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>>>> index 7dd6e5f..9ecdf0a 100644
>>>> --- a/src/qemu/qemu_hotplug.c
>>>> +++ b/src/qemu/qemu_hotplug.c
>>>> @@ -1643,7 +1643,8 @@ qemuDomainGetTLSObjects(virQEMUCapsPtr qemuCaps,
>>>>      }
>>>>
>>>>      if (qemuBuildTLSx509BackendProps(tlsCertdir, tlsListen, tlsVerify,
>>>> -                                     *secAlias, qemuCaps, tlsProps) < 0)
>>>> +                                     secAlias ? *secAlias : NULL, qemuCaps,
>>>> +                                     tlsProps) < 0)
>>>>          return -1;
>>>>
>>>>      if (!(*tlsAlias = qemuAliasTLSObjFromSrcAlias(srcAlias)))
>>>
>>> A few lines above this context, we check whether we have a valid reference to
>>> @secinfo and if so, we try to fill the @secAlias. Can we guarantee that when
>>> @secinfo is passed, @secAlias has been set as well? I'm asking because I
>>> haven't touched the TLS code yet and I just ran a few argument combinations
>>> mentally and this one got me wondering. If the combination I'm describing is a
>>> pure non-sense, the patch can go straight in.
>>>
>>
>> True, right, ugh.  A result of multiple rewrites along the way in patch
>> review resulting in me missing something.
>>
>> In the case of the Veritas series, it won't matter because secinfo will
>> be NULL, but that condition should be
>>
>>     if (secinfo && tlsAlias)
> 
> tlsAlias?? I thought it should be @secAlias, since that's the one we're
> setting, can you elaborate more? I'm not familiar with our TLS code so I'd like
> to learn something :).
> 
> Erik
> 

Well clearly I cannot juggle too many things in my head right now...
Yes, I meant secAlias not tlsAlias.

As for your other somewhat open ended question... For TLS there's a few
definitions in play - they're in cfg->{subsys}TLSx509{param}, where
{subsys} would be default, vnc, spice, chardev, migrate, and now vxhs.
The {param} would be verify and seretUUID).  Those are read from/filled
in from qemu.conf, but can also use default provided values.

For TLS, can either use a {subsys} specific directory to store the
certificates required (see qemu.conf for the list) or they can use a
default directory which may or may not exist, but then comes with a
default environment.

What the user can "change" is the "verify" option which allows them to
require that the client also present a certificate that the server can
validate. That requires a specifically named certificate file in the
certificate directory. QEMU has the code to handle reading (thankfully).

Additionally, the server certificate can be protected by a password.
That's where the secretUUID comes in. This ends up being a UUID for the
secret to decrypt the certificate. Since we have a need to not pass an
encoded password on the command line to qemu, we use the domain master
secret to encrypt the password. This all results in a lot more objects
that have a cascading and tight relationship.

When something uses a certificate it needs to let QEMU know whether it's
a client or server - that's where the listen value comes in. Migration
ends up being both and various chardevs also have both. It gets very
confusing, very fast.

For Veritas - there's already a server "somewhere" that could have a TLS
server certificate that would need a client to provide it's certificate.
The VxHS code also requires the client be authenticated as well - thus
the verify is always true. For libvirt purposes, it's always going to be
the client (e.g. listen = false) and there's no server certificate to be
decrypted, thus no need for a vxhs*secretUUID.

I'm sure to have missed a few details, but hopefully this helps a little
bit at least.  I'm not the expert in this matters ;-)! Just providing
some details on what little bit I figured out!

John