[libvirt] [PATCH v3] Avoid a possible NULL pointer dereference in qemuDomainGetTLSObjects
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@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))) -- 2.5.5
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@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. Thanks, Erik
-- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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) John
Thanks, Erik
-- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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
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@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
On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <eskultet@redhat.com> 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@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?
When secinfo is passed, the line marked below should guarantee that secAlias is set to not-null. if (secinfo) { if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1; if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) <=== this should ensure secAlias != NULL or return -1 return -1; } Please let me know if any changes are required for this patch. Thanks! Ashish
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.
Thanks, Erik
-- 2.5.5
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Sep 20, 2017 at 12:19:16PM -0700, ashish mittal wrote:
On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <eskultet@redhat.com> 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@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?
When secinfo is passed, the line marked below should guarantee that secAlias is set to not-null.
if (secinfo) { if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1;
if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) <=== this should ensure secAlias != NULL or return -1 return -1; }
See John's reply to my response, long story short, in case of Veritas, this couldn't happen, but in general, we should cover the use case I'm describing as well, which is just a matter of very simple 'if' statement adjustment. Erik
On Thu, Sep 21, 2017 at 12:21 AM, Erik Skultety <eskultet@redhat.com> wrote:
On Wed, Sep 20, 2017 at 12:19:16PM -0700, ashish mittal wrote:
On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <eskultet@redhat.com> 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@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?
When secinfo is passed, the line marked below should guarantee that secAlias is set to not-null.
if (secinfo) { if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1;
if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) <=== this should ensure secAlias != NULL or return -1 return -1; }
See John's reply to my response, long story short, in case of Veritas, this couldn't happen, but in general, we should cover the use case I'm describing as well, which is just a matter of very simple 'if' statement adjustment.
Erik
if (secinfo) { if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1; if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) return -1; } Above code segment suggests that if secinfo != NULL, then secAlias should never be NULL . If that were not the case, we would have already seen a crash from such a case. I'm afraid changing the above "if" condition to "if (secinfo && secAlias)" is changing the behavior of the code to say "It is OK if secinfo != NULL and secAlias == NULL, I'll just skip the setting of *secAlias". I don't know the code well enough to say if this, or the above, is expected behavior. If the expected behavior is that secAlias should never be NULL when secinfo != NULL, then a safer change might be - if (secinfo) { if (!secAlias) return -1; .... Regards, Ashish
On Thu, Sep 21, 2017 at 01:24:29AM -0700, ashish mittal wrote:
On Thu, Sep 21, 2017 at 12:21 AM, Erik Skultety <eskultet@redhat.com> wrote:
On Wed, Sep 20, 2017 at 12:19:16PM -0700, ashish mittal wrote:
On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <eskultet@redhat.com> 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@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?
When secinfo is passed, the line marked below should guarantee that secAlias is set to not-null.
if (secinfo) { if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1;
if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) <=== this should ensure secAlias != NULL or return -1 return -1; }
See John's reply to my response, long story short, in case of Veritas, this couldn't happen, but in general, we should cover the use case I'm describing as well, which is just a matter of very simple 'if' statement adjustment.
Erik
if (secinfo) { if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1;
if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) return -1; }
Above code segment suggests that if secinfo != NULL, then secAlias should never be NULL . If that were not the case, we would have already seen a crash from such a case.
I'm afraid changing the above "if" condition to "if (secinfo && secAlias)" is changing the behavior of the code to say "It is OK if secinfo != NULL and secAlias == NULL, I'll just skip the setting of *secAlias". I don't know the code well enough to say if this, or the above, is expected
True, good point, in that case I'd make a very tiny adjustment and go with: if (!secAlias || !(*secAlias = qemuDomainGetSecretAESAlias())) return -1; Whatever the case, you're right in your reasoning and better be safe than sorry with this. Thanks, Erik
behavior. If the expected behavior is that secAlias should never be NULL when secinfo != NULL, then a safer change might be -
if (secinfo) { if (!secAlias) return -1; ....
Regards, Ashish
On 09/21/2017 04:33 AM, Erik Skultety wrote:
On Thu, Sep 21, 2017 at 01:24:29AM -0700, ashish mittal wrote:
On Thu, Sep 21, 2017 at 12:21 AM, Erik Skultety <eskultet@redhat.com> wrote:
On Wed, Sep 20, 2017 at 12:19:16PM -0700, ashish mittal wrote:
On Wed, Sep 20, 2017 at 6:11 AM, Erik Skultety <eskultet@redhat.com> 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@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?
When secinfo is passed, the line marked below should guarantee that secAlias is set to not-null.
if (secinfo) { if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1;
if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) <=== this should ensure secAlias != NULL or return -1 return -1; }
See John's reply to my response, long story short, in case of Veritas, this couldn't happen, but in general, we should cover the use case I'm describing as well, which is just a matter of very simple 'if' statement adjustment.
Erik
if (secinfo) { if (qemuBuildSecretInfoProps(secinfo, secProps) < 0) return -1;
if (!(*secAlias = qemuDomainGetSecretAESAlias(srcAlias, false))) return -1; }
Above code segment suggests that if secinfo != NULL, then secAlias should never be NULL . If that were not the case, we would have already seen a crash from such a case.
I'm afraid changing the above "if" condition to "if (secinfo && secAlias)" is changing the behavior of the code to say "It is OK if secinfo != NULL and secAlias == NULL, I'll just skip the setting of *secAlias". I don't know the code well enough to say if this, or the above, is expected
True, good point, in that case I'd make a very tiny adjustment and go with:
if (!secAlias || !(*secAlias = qemuDomainGetSecretAESAlias())) return -1;
Whatever the case, you're right in your reasoning and better be safe than sorry with this.
I squashed this into the v3 patch and pushed. Tks - John
participants (4)
-
Ashish Mittal -
ashish mittal
-
Erik Skultety -
John Ferlan