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

[Rodger Combs]

> On Jan 17, 2019, at 03:09, Nicolas George <george at 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
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Message signed with OpenPGP
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190117/b6bd6d5a/attachment.sig>