From ec940f898b42f16bb0727f807b72488fad6d6d12 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= <marten.nordheim@qt.io>
Date: Wed, 14 Aug 2019 14:20:03 +0200
Subject: [PATCH] SSL: ALPN: Don't include empty, too long or truncated names

As is said in RFC7301 in section 3.1 [1]:

Protocols are named by IANA-registered, opaque, non-empty byte strings
[...]. Empty strings MUST NOT be included and byte strings MUST NOT be
truncated.

[1]: https://tools.ietf.org/html/rfc7301#section-3.1

Change-Id: I2c41fa99984a53cc58803e5a264d06edac964cc6
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>
---
 src/network/ssl/qsslcontext_openssl.cpp | 44 ++++++++++++++-----------
 src/network/ssl/qsslsocket_mac.cpp      |  7 ++++
 2 files changed, 31 insertions(+), 20 deletions(-)

diff --git a/src/network/ssl/qsslcontext_openssl.cpp b/src/network/ssl/qsslcontext_openssl.cpp
index 35cca9f01a..675d5f8e62 100644
--- a/src/network/ssl/qsslcontext_openssl.cpp
+++ b/src/network/ssl/qsslcontext_openssl.cpp
@@ -157,32 +157,36 @@ SSL* QSslContext::createSsl()
         for (int a = 0; a < protocols.count(); ++a) {
             if (protocols.at(a).size() > 255) {
                 qCWarning(lcSsl) << "TLS NPN extension" << protocols.at(a)
-                                 << "is too long and will be truncated to 255 characters.";
-                protocols[a] = protocols.at(a).left(255);
+                                 << "is too long and will be ignored.";
+                continue;
+            } else if (protocols.at(a).isEmpty()) {
+                continue;
             }
             m_supportedNPNVersions.append(protocols.at(a).size()).append(protocols.at(a));
         }
-        m_npnContext.data = reinterpret_cast<unsigned char *>(m_supportedNPNVersions.data());
-        m_npnContext.len = m_supportedNPNVersions.count();
-        m_npnContext.status = QSslConfiguration::NextProtocolNegotiationNone;
+        if (m_supportedNPNVersions.size()) {
+            m_npnContext.data = reinterpret_cast<unsigned char *>(m_supportedNPNVersions.data());
+            m_npnContext.len = m_supportedNPNVersions.count();
+            m_npnContext.status = QSslConfiguration::NextProtocolNegotiationNone;
 #if OPENSSL_VERSION_NUMBER >= 0x10002000L
-        if (QSslSocket::sslLibraryVersionNumber() >= 0x10002000L) {
-            // Callback's type has a parameter 'const unsigned char ** out'
-            // since it was introduced in 1.0.2. Internally, OpenSSL's own code
-            // (tests/examples) cast it to unsigned char * (since it's 'out').
-            // We just re-use our NPN callback and cast here:
-            typedef int (*alpn_callback_t) (SSL *, const unsigned char **, unsigned char *,
-                                            const unsigned char *, unsigned int, void *);
-            // With ALPN callback is for a server side only, for a client m_npnContext.status
-            // will stay in NextProtocolNegotiationNone.
-            q_SSL_CTX_set_alpn_select_cb(ctx, alpn_callback_t(next_proto_cb), &m_npnContext);
-            // Client:
-            q_SSL_set_alpn_protos(ssl, m_npnContext.data, m_npnContext.len);
-        }
+            if (QSslSocket::sslLibraryVersionNumber() >= 0x10002000L) {
+                // Callback's type has a parameter 'const unsigned char ** out'
+                // since it was introduced in 1.0.2. Internally, OpenSSL's own code
+                // (tests/examples) cast it to unsigned char * (since it's 'out').
+                // We just re-use our NPN callback and cast here:
+                typedef int (*alpn_callback_t) (SSL *, const unsigned char **, unsigned char *,
+                                                const unsigned char *, unsigned int, void *);
+                // With ALPN callback is for a server side only, for a client m_npnContext.status
+                // will stay in NextProtocolNegotiationNone.
+                q_SSL_CTX_set_alpn_select_cb(ctx, alpn_callback_t(next_proto_cb), &m_npnContext);
+                // Client:
+                q_SSL_set_alpn_protos(ssl, m_npnContext.data, m_npnContext.len);
+            }
 #endif // OPENSSL_VERSION_NUMBER >= 0x10002000L ...
 
-        // And in case our peer does not support ALPN, but supports NPN:
-        q_SSL_CTX_set_next_proto_select_cb(ctx, next_proto_cb, &m_npnContext);
+            // And in case our peer does not support ALPN, but supports NPN:
+            q_SSL_CTX_set_next_proto_select_cb(ctx, next_proto_cb, &m_npnContext);
+        }
     }
 #endif // OPENSSL_VERSION_NUMBER >= 0x1000100fL ...
 
diff --git a/src/network/ssl/qsslsocket_mac.cpp b/src/network/ssl/qsslsocket_mac.cpp
index 7c5f4310f8..e6b71b293e 100644
--- a/src/network/ssl/qsslsocket_mac.cpp
+++ b/src/network/ssl/qsslsocket_mac.cpp
@@ -928,6 +928,13 @@ bool QSslSocketBackendPrivate::initSslContext()
         QCFType<CFMutableArrayRef> cfNames(CFArrayCreateMutable(nullptr, 0, &kCFTypeArrayCallBacks));
         if (cfNames) {
             for (const QByteArray &name : protocolNames) {
+                if (name.size() > 255) {
+                    qCWarning(lcSsl) << "TLS ALPN extension" << name
+                                     << "is too long and will be ignored.";
+                    continue;
+                } else if (name.isEmpty()) {
+                    continue;
+                }
                 QCFString cfName(QString::fromLatin1(name).toCFString());
                 CFArrayAppendValue(cfNames, cfName);
             }