[FFmpeg-devel,3/4] lavf/tls_openssl: on 1.1 or later, verify the server's hostname

---
 libavformat/tls_openssl.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

Rodger Combs (12019-01-17):
> ---
>  libavformat/tls_openssl.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c
> index 493f43e610..bdc4985bad 100644
> --- a/libavformat/tls_openssl.c
> +++ b/libavformat/tls_openssl.c
> @@ -35,6 +35,7 @@
>  #include <openssl/bio.h>
>  #include <openssl/ssl.h>
>  #include <openssl/err.h>
> +#include <openssl/x509v3.h>
>  
>  static int openssl_init;
>  
> @@ -269,8 +270,6 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op
>          ret = AVERROR(EIO);
>          goto fail;
>      }
> -    // Note, this doesn't check that the peer certificate actually matches
> -    // the requested hostname.
>      if (c->verify)
>          SSL_CTX_set_verify(p->ctx, SSL_VERIFY_PEER|SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL);
>      p->ssl = SSL_new(p->ctx);
> @@ -294,8 +293,23 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op
>      bio->ptr = c->tcp;
>  #endif
>      SSL_set_bio(p->ssl, bio, bio);
> -    if (!c->listen && !c->numerichost)
> -        SSL_set_tlsext_host_name(p->ssl, c->host);

> +    if (!c->listen && !c->numerichost) {
> +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
> +        X509_VERIFY_PARAM *param = SSL_get0_param(p->ssl);
> +        X509_VERIFY_PARAM_set_hostflags(param, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
> +#endif
> +        if (
> +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
> +            // Note, if on OpenSSL prior to 1.1.0, we won't check that
> +            // the peer certificate actually matches the requested hostname.
> +            !X509_VERIFY_PARAM_set1_host(param, c->host, 0) ||
> +#endif
> +            !SSL_set_tlsext_host_name(p->ssl, c->host)) {
> +            av_log(h, AV_LOG_ERROR, "%s\n", ERR_error_string(ERR_get_error(), NULL));
> +            ret = AVERROR(EIO);
> +            goto fail;
> +        }
> +    }

I think AVERROR(EIO) is not the best choice. EPROTO would seem obvious,
but not supported on windows. Otherwise EPERM.

More importantly: with this change, users will no longer be able to
access misconfigured servers. An option to let them bypass the test
would be necessary.

>      ret = c->listen ? SSL_accept(p->ssl) : SSL_connect(p->ssl);
>      if (ret == 0) {
>          av_log(h, AV_LOG_ERROR, "Unable to negotiate TLS/SSL session\n");

Regards,

> On Jan 17, 2019, at 03:09, Nicolas George <george@nsup.org> wrote:
> 
> Signed PGP part
> Rodger Combs (12019-01-17):
>> ---
>> libavformat/tls_openssl.c | 22 ++++++++++++++++++----
>> 1 file changed, 18 insertions(+), 4 deletions(-)
>> 
>> diff --git a/libavformat/tls_openssl.c b/libavformat/tls_openssl.c
>> index 493f43e610..bdc4985bad 100644
>> --- a/libavformat/tls_openssl.c
>> +++ b/libavformat/tls_openssl.c
>> @@ -35,6 +35,7 @@
>> #include <openssl/bio.h>
>> #include <openssl/ssl.h>
>> #include <openssl/err.h>
>> +#include <openssl/x509v3.h>
>> 
>> static int openssl_init;
>> 
>> @@ -269,8 +270,6 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op
>>         ret = AVERROR(EIO);
>>         goto fail;
>>     }
>> -    // Note, this doesn't check that the peer certificate actually matches
>> -    // the requested hostname.
>>     if (c->verify)
>>         SSL_CTX_set_verify(p->ctx, SSL_VERIFY_PEER|SSL_VERIFY_FAIL_IF_NO_PEER_CERT, NULL);
>>     p->ssl = SSL_new(p->ctx);
>> @@ -294,8 +293,23 @@ static int tls_open(URLContext *h, const char *uri, int flags, AVDictionary **op
>>     bio->ptr = c->tcp;
>> #endif
>>     SSL_set_bio(p->ssl, bio, bio);
>> -    if (!c->listen && !c->numerichost)
>> -        SSL_set_tlsext_host_name(p->ssl, c->host);
> 
>> +    if (!c->listen && !c->numerichost) {
>> +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
>> +        X509_VERIFY_PARAM *param = SSL_get0_param(p->ssl);
>> +        X509_VERIFY_PARAM_set_hostflags(param, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS);
>> +#endif
>> +        if (
>> +#if OPENSSL_VERSION_NUMBER >= 0x1010000fL
>> +            // Note, if on OpenSSL prior to 1.1.0, we won't check that
>> +            // the peer certificate actually matches the requested hostname.
>> +            !X509_VERIFY_PARAM_set1_host(param, c->host, 0) ||
>> +#endif
>> +            !SSL_set_tlsext_host_name(p->ssl, c->host)) {
>> +            av_log(h, AV_LOG_ERROR, "%s\n", ERR_error_string(ERR_get_error(), NULL));
>> +            ret = AVERROR(EIO);
>> +            goto fail;
>> +        }
>> +    }
> 
> I think AVERROR(EIO) is not the best choice. EPROTO would seem obvious,
> but not supported on windows. Otherwise EPERM.
> 
> More importantly: with this change, users will no longer be able to
> access misconfigured servers. An option to let them bypass the test
> would be necessary.

What kind of misconfiguration are you referring to? The actual verification is still gated behind the tls_verify option; if that's set to 0, this won't actually do anything. (Well, it'll result in OpenSSL potentially generating a different error code internally, and then discarding it because we didn't enable verification).

> 
>>     ret = c->listen ? SSL_accept(p->ssl) : SSL_connect(p->ssl);
>>     if (ret == 0) {
>>         av_log(h, AV_LOG_ERROR, "Unable to negotiate TLS/SSL session\n");
> 
> Regards,
> 
> --
>  Nicolas George
> 
>

Rodger Combs (12019-01-17):
> What kind of misconfiguration are you referring to? The actual

A server with a certificate on a different name.

> verification is still gated behind the tls_verify option; if that's
> set to 0, this won't actually do anything.

Ok, that is good. Thanks for clarifying.

Regards,