[Spice-devel] [spice-common 3/3] ssl: Don't try hostname check if cert subject check fails

[Uri Lublin]

On 09/23/2013 11:23 AM, Christophe Fergeau wrote:
> On Sun, Sep 22, 2013 at 02:39:36PM +0300, Uri Lublin wrote:
>> On 09/20/2013 06:07 PM, Christophe Fergeau wrote:
>> What is v->verifyop value when this problem occurs ?
> When this occurs, v->verifyop would be SPICE_SSL_VERIFY_OP_HOSTNAME |
> SPICE_SSL_VERIFY_OP_SUBJECT. This will happen when a host subject is set
> from the command line, or through the controller (and probably through a
> .vv file).
>
>> It "feels" like the hostname check should not be skipped.
>>
>> It's probably better to not return after a successful check, but
>> to continue checking other required parts of the parameters (e.g. both
>> the hostname and the cert-subject).
> This wouldn't work, cert-subject is set when we know the hostname check
> will fail, and when something else should be used instead of the hostname
> to check the certificate. So we don't want to check both, and fail if both
> fail.
> host-subject and hostname are trying to verify the same part of the
> certificate (the 'subject' field, even though hostname will also be looked
> for in the altSubjectName field), so it does not feel that bad to not check
> hostname when cert-subject is set.

Hi,

It seems better to me that spice-common would check whatever it is 
asked, via v->verifyop,
and not return after the first successful test.

If hostname is known to be wrong, it should not be checked (its flag 
should be off).

I think the current openssl_verify() implementation is wrong (even if it 
works).
What do you think of the patch below. Would it not solve the original 
bug (I did not test it) ?
A follow-up (or squashed) patch would be to print the warnings when they 
occur,
instead of at the bottom of the function.

Thanks,
     Uri.

---

diff --git a/common/ssl_verify.c b/common/ssl_verify.c
index e10ed52..cb00b3c 100644
--- a/common/ssl_verify.c
+++ b/common/ssl_verify.c
@@ -460,23 +460,16 @@ static int openssl_verify(int preverify_ok, 
X509_STORE_CTX *ctx)
          return 0;

      if (v->verifyop & SPICE_SSL_VERIFY_OP_HOSTNAME) {
-        if (verify_hostname(cert, v->hostname))
-            return 1;
-        else
+       if (!verify_hostname(cert, v->hostname))
              failed_verifications |= SPICE_SSL_VERIFY_OP_HOSTNAME;
      }


      if (v->verifyop & SPICE_SSL_VERIFY_OP_SUBJECT) {
-        if (verify_subject(cert, v))
-            return 1;
-        else
+       if (!verify_subject(cert, v))
              failed_verifications |= SPICE_SSL_VERIFY_OP_SUBJECT;
      }

-    /* If we reach this code, this means all the tests failed, thus
-     * verification failed
-     */
      if (failed_verifications & SPICE_SSL_VERIFY_OP_PUBKEY)
          spice_warning("ssl: pubkey verification failed");

@@ -486,9 +479,11 @@ static int openssl_verify(int preverify_ok, 
X509_STORE_CTX *ctx)
      if (failed_verifications & SPICE_SSL_VERIFY_OP_SUBJECT)
          spice_warning("ssl: subject '%s' verification failed", 
v->subject);

-    spice_warning("ssl: verification failed");
+    if (failed_verifications) {
+        spice_warning("ssl: verification failed");
+    }

-    return 0;
+    return !failed_verifications;
  }

  SpiceOpenSSLVerify* spice_openssl_verify_new(SSL *ssl, 
SPICE_SSL_VERIFY_OP verifyop,