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

[Uri Lublin]

On 09/20/2013 06:07 PM, Christophe Fergeau wrote:
> Currently, SSL verification of the peer certificate checks if
> the certificate's subject CN or one of its subjectAltName match
> the hostname. If this succeeds, then the verification succeeds.
> Otherwise openssl_verify() checks the cert subject if this was set,
> which means it checks the certificate's subject (not just its CN) matches
> exactly the cert subject string that is set in SpiceSession.
>
> Given that the cert subject is something the user specifies in addition
> to the hostname, the cert subject check should have priority over the
> hostname check, that is, when we have a cert subject set, the
> success/failure of the cert subject cert should determine the
> success/failure of openssl_verify(), and the hostname check
> should only be carried out when no cert subject was set.
>
> This fixes rhbz#871034

Hi Christophe

What is v->verifyop value when this problem occurs ?

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).

Thanks,
     Uri.


> ---
>   common/ssl_verify.c | 13 +++++--------
>   1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/common/ssl_verify.c b/common/ssl_verify.c
> index 7af78bc..8fdeaa0 100644
> --- a/common/ssl_verify.c
> +++ b/common/ssl_verify.c
> @@ -467,19 +467,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
> -            failed_verifications |= SPICE_SSL_VERIFY_OP_HOSTNAME;
> -    }
> -
> -
>       if (v->verifyop & SPICE_SSL_VERIFY_OP_SUBJECT) {
>           if (verify_subject(cert, v))
>               return 1;
>           else
>               failed_verifications |= SPICE_SSL_VERIFY_OP_SUBJECT;
> +    } else if (v->verifyop & SPICE_SSL_VERIFY_OP_HOSTNAME) {
> +       if (verify_hostname(cert, v->hostname))
> +           return 1;
> +        else
> +            failed_verifications |= SPICE_SSL_VERIFY_OP_HOSTNAME;
>       }
>   
>       /* If we reach this code, this means all the tests failed, thus