diff mbox

[FFmpeg-devel,5/5] lavf/tls: enable server verification by default when not on mbedtls

Message ID 20190118084604.82324-5-rodger.combs@gmail.com
State Withdrawn, archived
Headers show

Commit Message

Rodger Combs Jan. 18, 2019, 8:46 a.m. UTC
All other TLS wrappers now have a mechanism to load a system trust store
by default, without setting the cafile option. For Secure Transport and
Secure Channel, it's the OS. For OpenSSL and libtls, it's a path set at
compile-time. For GNUTLS, it's either a path set at compile-time, or the
OS trust store (if on macOS, iOS, or Windows). It's possible to configure
OpenSSL, GNUTLS, and libtls without a working trust store, but these are
broken configurations and I don't have a problem with requiring users with
that kind of install to either fix it, or explicitly opt in to insecure
behavior. mbedtls doesn't have a default trust store (it's assumed that the
application will provide one), so it continues to require the user to pass
in a path and enable verification manually.
---
 libavformat/tls.c | 3 +++
 libavformat/tls.h | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Carl Eugen Hoyos Jan. 18, 2019, 11:41 a.m. UTC | #1
2019-01-18 9:46 GMT+01:00, Rodger Combs <rodger.combs@gmail.com>:
> All other TLS wrappers now have a mechanism to load a system trust store
> by default, without setting the cafile option. For Secure Transport and
> Secure Channel, it's the OS. For OpenSSL and libtls, it's a path set at
> compile-time. For GNUTLS, it's either a path set at compile-time, or the
> OS trust store (if on macOS, iOS, or Windows). It's possible to configure
> OpenSSL, GNUTLS, and libtls without a working trust store, but these are
> broken configurations and I don't have a problem with requiring users with
> that kind of install to either fix it, or explicitly opt in to insecure
> behavior. mbedtls doesn't have a default trust store (it's assumed that the
> application will provide one), so it continues to require the user to pass
> in a path and enable verification manually.

I believe the current behaviour is more desirable as default for a multimedia
library.

Carl Eugen
Rodger Combs Jan. 18, 2019, 8:50 p.m. UTC | #2
> On Jan 18, 2019, at 05:41, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 
> 2019-01-18 9:46 GMT+01:00, Rodger Combs <rodger.combs@gmail.com>:
>> All other TLS wrappers now have a mechanism to load a system trust store
>> by default, without setting the cafile option. For Secure Transport and
>> Secure Channel, it's the OS. For OpenSSL and libtls, it's a path set at
>> compile-time. For GNUTLS, it's either a path set at compile-time, or the
>> OS trust store (if on macOS, iOS, or Windows). It's possible to configure
>> OpenSSL, GNUTLS, and libtls without a working trust store, but these are
>> broken configurations and I don't have a problem with requiring users with
>> that kind of install to either fix it, or explicitly opt in to insecure
>> behavior. mbedtls doesn't have a default trust store (it's assumed that the
>> application will provide one), so it continues to require the user to pass
>> in a path and enable verification manually.
> 
> I believe the current behaviour is more desirable as default for a multimedia
> library.

That's patent nonsense. Requests for media are as likely to contain authentication tokens as anything else today, and users have as much of a right to privacy in the specific media they consume as in anything else they use online. Without any verification of peer certificates, our current defaults are little better than using plaintext.

> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Michael Niedermayer Jan. 19, 2019, 11:25 p.m. UTC | #3
On Fri, Jan 18, 2019 at 02:50:29PM -0600, Rodger Combs wrote:
> 
> 
> > On Jan 18, 2019, at 05:41, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > 
> > 2019-01-18 9:46 GMT+01:00, Rodger Combs <rodger.combs@gmail.com>:
> >> All other TLS wrappers now have a mechanism to load a system trust store
> >> by default, without setting the cafile option. For Secure Transport and
> >> Secure Channel, it's the OS. For OpenSSL and libtls, it's a path set at
> >> compile-time. For GNUTLS, it's either a path set at compile-time, or the
> >> OS trust store (if on macOS, iOS, or Windows). It's possible to configure
> >> OpenSSL, GNUTLS, and libtls without a working trust store, but these are
> >> broken configurations and I don't have a problem with requiring users with
> >> that kind of install to either fix it, or explicitly opt in to insecure
> >> behavior. mbedtls doesn't have a default trust store (it's assumed that the
> >> application will provide one), so it continues to require the user to pass
> >> in a path and enable verification manually.
> > 
> > I believe the current behaviour is more desirable as default for a multimedia
> > library.
> 
> That's patent nonsense. Requests for media are as likely to contain authentication tokens as anything else today, and users have as much of a right to privacy in the specific media they consume as in anything else they use online. Without any verification of peer certificates, our current defaults are little better than using plaintext.

Iam in favor of security by default too.
But there may be issues iam unaware of of course ...

thx
diff mbox

Patch

diff --git a/libavformat/tls.c b/libavformat/tls.c
index a6dcd3cc96..c564b1252b 100644
--- a/libavformat/tls.c
+++ b/libavformat/tls.c
@@ -62,6 +62,9 @@  int ff_tls_open_underlying(TLSShared *c, URLContext *parent, const char *uri, AV
     const char *proxy_path;
     int use_proxy;
 
+    if (c->verify == -1)
+        c->verify = !c->listen && !CONFIG_MBEDTLS;
+
     set_options(c, uri);
 
     if (c->listen)
diff --git a/libavformat/tls.h b/libavformat/tls.h
index beb19d6d55..bc4ee1c216 100644
--- a/libavformat/tls.h
+++ b/libavformat/tls.h
@@ -45,7 +45,7 @@  typedef struct TLSShared {
 #define TLS_COMMON_OPTIONS(pstruct, options_field) \
     {"ca_file",    "Certificate Authority database file", offsetof(pstruct, options_field . ca_file),   AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
     {"cafile",     "Certificate Authority database file", offsetof(pstruct, options_field . ca_file),   AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
-    {"tls_verify", "Verify the peer certificate",         offsetof(pstruct, options_field . verify),    AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, .flags = TLS_OPTFL }, \
+    {"tls_verify", "Verify the peer certificate",         offsetof(pstruct, options_field . verify),    AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, .flags = TLS_OPTFL }, \
     {"cert_file",  "Certificate file",                    offsetof(pstruct, options_field . cert_file), AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
     {"key_file",   "Private key file",                    offsetof(pstruct, options_field . key_file),  AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
     {"listen",     "Listen for incoming connections",     offsetof(pstruct, options_field . listen),    AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, .flags = TLS_OPTFL }, \