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

Submitted by Uri Lublin on Sept. 24, 2013, 5:47 p.m.

Details

Message ID 5241D039.2040309@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Uri Lublin Sept. 24, 2013, 5:47 p.m.
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.

---


@@ -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,

Patch hide | download patch | download mbox

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");

Comments

On Tue, Sep 24, 2013 at 08:47:37PM +0300, Uri Lublin wrote:
> 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).

The problem is that we are not doing this at the moment,
spice_set_session_option() will set v->verifyop to
SPICE_SSL_VERIFY_OP_HOSTNAME | SPICE_SSL_VERIFY_OP_SUBJECT if a
host subject was specified. VirtViewerSessionSpice::fill_session()
will do the same, and I suspect it's the same for the controller code.
The only reason to specify a host subject is when we know the hostname will
not be correct to verify the host TLS certificate.

If we want to use your patch, we need to change v->verifyop prior to the SSL
verification to remove SPICE_SSL_VERIFY_HOSTNAME when both
SPICE_SSL_VERIFY_OP_HOSTNAME and SPICE_SSL_VERIFY_OP_SUBJECT are set.

Christophe
On 09/25/2013 09:56 AM, Christophe Fergeau wrote:
> On Tue, Sep 24, 2013 at 08:47:37PM +0300, Uri Lublin wrote:
>> 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).
> The problem is that we are not doing this at the moment,
> spice_set_session_option() will set v->verifyop to
> SPICE_SSL_VERIFY_OP_HOSTNAME | SPICE_SSL_VERIFY_OP_SUBJECT if a
> host subject was specified. VirtViewerSessionSpice::fill_session()
> will do the same, and I suspect it's the same for the controller code.
> The only reason to specify a host subject is when we know the hostname will
> not be correct to verify the host TLS certificate.
>
> If we want to use your patch, we need to change v->verifyop prior to the SSL
> verification to remove SPICE_SSL_VERIFY_HOSTNAME when both
> SPICE_SSL_VERIFY_OP_HOSTNAME and SPICE_SSL_VERIFY_OP_SUBJECT are set.

Right, but that change follows the "host-subject overrides hostname" rule,
mentioned in a previous email.

Anyway, I don't feel strongly about that, and the patch itself is doing
what it claims to be doing and it solves the bug.