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

2019-01-17 Thread 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,

-- 
  Nicolas George


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2019-01-17 Thread Rodger Combs 


> On Jan 17, 2019, at 03:09, Nicolas George  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 
>> #include 
>> #include 
>> +#include 
>> 
>> 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 >= 0x101fL
>> +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 >= 0x101fL
>> +// 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
> 
> 



signature.asc
Description: Message signed with OpenPGP
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2019-01-17 Thread Nicolas George 
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 
>  #include 
>  #include 
> +#include 
>  
>  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 >= 0x101fL
> +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 >= 0x101fL
> +// 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,

-- 
  Nicolas George


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

2019-01-17 Thread Rodger Combs 
---
 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 
 #include 
 #include 
+#include 
 
 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 >= 0x101fL
+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 >= 0x101fL
+// 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;
+}
+}
 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");
-- 
2.19.1

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel