[Spice-devel] [spice-common 8/8] ssl: Don't use uninitialized variable in verify_subject()

[Christophe Fergeau]

If verify_subject() is called with a SpiceOpenSSLVerify struct containing a
non-NULL 'in_subject' member, it would try to use the local 'in_entries'
variable without having initialized it first. This could happen if
verify_subject() was called multiple time with the same SpiceOpenSSLVerify
context, which probably isn't occurring the way we are using it.

However, since verify_subject() is the only method which needs in_subject,
we don't need to have it stored in SpiceOpenSSLVerify, and we can
recreate it as needed locally in that method, which avoids that issue.
---
 common/ssl_verify.c | 20 +++++++++-----------
 common/ssl_verify.h |  1 -
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/common/ssl_verify.c b/common/ssl_verify.c
index 8fdeaa0..a830800 100644
--- a/common/ssl_verify.c
+++ b/common/ssl_verify.c
@@ -357,6 +357,7 @@ fail:
 static int verify_subject(X509* cert, SpiceOpenSSLVerify* verify)
 {
     X509_NAME *cert_subject = NULL;
+    X509_NAME* in_subject;
     int ret;
     int in_entries;
 
@@ -371,22 +372,21 @@ static int verify_subject(X509* cert, SpiceOpenSSLVerify* verify)
         return 0;
     }
 
-    if (!verify->in_subject) {
-        verify->in_subject = subject_to_x509_name(verify->subject, &in_entries);
-        if (!verify->in_subject) {
-            spice_debug("warning: no in_subject!");
-            return 0;
-        }
+    in_subject = subject_to_x509_name(verify->subject, &in_entries);
+    if (!in_subject) {
+        spice_debug("warning: no in_subject!");
+        return 0;
     }
 
     /* Note: this check is redundant with the pre-condition in X509_NAME_cmp */
     if (X509_NAME_entry_count(cert_subject) != in_entries) {
         spice_debug("subject mismatch: #entries cert=%d, input=%d",
             X509_NAME_entry_count(cert_subject), in_entries);
+        X509_NAME_free(in_subject);
         return 0;
     }
 
-    ret = X509_NAME_cmp(cert_subject, verify->in_subject);
+    ret = X509_NAME_cmp(cert_subject, in_subject);
 
     if (ret == 0) {
         spice_debug("subjects match");
@@ -398,10 +398,11 @@ static int verify_subject(X509* cert, SpiceOpenSSLVerify* verify)
         spice_debug("cert_subject: %s", p);
         free(p);
 
-        p = X509_NAME_oneline(verify->in_subject, NULL, 0);
+        p = X509_NAME_oneline(in_subject, NULL, 0);
         spice_debug("in_subject:   %s", p);
         free(p);
     }
+    X509_NAME_free(in_subject);
 
     return !ret;
 }
@@ -533,9 +534,6 @@ void spice_openssl_verify_free(SpiceOpenSSLVerify* verify)
     free(verify->subject);
     free(verify->hostname);
 
-    if (verify->in_subject)
-        X509_NAME_free(verify->in_subject);
-
     if (verify->ssl)
         SSL_set_app_data(verify->ssl, NULL);
     free(verify);
diff --git a/common/ssl_verify.h b/common/ssl_verify.h
index 37c123e..bfbd8a4 100644
--- a/common/ssl_verify.h
+++ b/common/ssl_verify.h
@@ -54,7 +54,6 @@ typedef struct {
     char                *pubkey;
     size_t              pubkey_size;
     char                *subject;
-    X509_NAME           *in_subject;
 } SpiceOpenSSLVerify;
 
 SpiceOpenSSLVerify* spice_openssl_verify_new(SSL *ssl, SPICE_SSL_VERIFY_OP verifyop,
-- 
1.8.4.2