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,
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel