Re: [libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
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(a)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