Do not ignore SSL_TLSEXT_ERR_FATAL from the ALPN callback

As reported by Jeremy Harris, we inherited a strange behavior from
OpenSSL, in that we ignore the SSL_TLSEXT_ERR_FATAL return from the
ALPN callback. RFC 7301, 3.2 states: 'In the event that the server
supports no protocols that the client advertises, then the server
SHALL respond with a fatal "no_application_protocol" alert.'

Honor this requirement and succeed only on SSL_TLSEXT_ERR_{OK,NOACK}
which is the current behavior of OpenSSL. The documentation change
is taken from OpenSSL 1.1.1 as well.

As pointed out by jsing, there is more to be fixed here:
- ensure that the same protocol is selected on session resumption
- should the callback be called even if no ALPN extension was sent?
- ensure for TLSv1.2 and earlier that the SNI has already been processed

ok beck jsing

4 files changed, 31 insertions, 7 deletions

diff --git a/src/lib/libssl/man/SSL_CTX_set_alpn_select_cb.3 b/src/lib/libssl/man/SSL_CTX_set_alpn_select_cb.3
index 540fd011f5..683b6696e3 100644
--- a/src/lib/libssl/man/SSL_CTX_set_alpn_select_cb.3
+++ b/src/lib/libssl/man/SSL_CTX_set_alpn_select_cb.3
@@ -1,4 +1,4 @@
-.\"     $OpenBSD: SSL_CTX_set_alpn_select_cb.3,v 1.7 2018/03/23 14:28:16 schwarze Exp $
+.\"     $OpenBSD: SSL_CTX_set_alpn_select_cb.3,v 1.8 2021/09/10 09:25:29 tb Exp $
 .\"     OpenSSL 87b81496 Apr 19 12:38:27 2017 -0400
 .\"     OpenSSL b97fdb57 Nov 11 09:33:09 2016 +0100
 .\"
@@ -49,7 +49,7 @@
 .\" ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
 .\" OF THE POSSIBILITY OF SUCH DAMAGE.
 .\"
-.Dd $Mdocdate: March 23 2018 $
+.Dd $Mdocdate: September 10 2021 $
 .Dt SSL_CTX_SET_ALPN_SELECT_CB 3
 .Os
 .Sh NAME
@@ -252,8 +252,12 @@ must return one of the following:
 .Bl -tag -width Ds
 .It SSL_TLSEXT_ERR_OK
 ALPN protocol selected.
+.It SSL_TLSEXT_ERR_ALERT_FATAL
+There was no overlap between the client's supplied list and the
+server configuration.
 .It SSL_TLSEXT_ERR_NOACK
-ALPN protocol not selected.
+ALPN protocol not selected, e.g., because no ALPN protocols are
+configured for this connection.
 .El
 .Sh SEE ALSO
 .Xr ssl 3 ,
diff --git a/src/lib/libssl/ssl.h b/src/lib/libssl/ssl.h
index 7da3658d3f..fba9ea243f 100644
--- a/src/lib/libssl/ssl.h
+++ b/src/lib/libssl/ssl.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl.h,v 1.201 2021/09/10 08:59:56 tb Exp $ */
+/* $OpenBSD: ssl.h,v 1.202 2021/09/10 09:25:29 tb Exp $ */
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
@@ -1995,6 +1995,9 @@ void ERR_load_SSL_strings(void);
 #define SSL_R_MISSING_VERIFY_MESSAGE                     174
 #define SSL_R_MULTIPLE_SGC_RESTARTS                      346
 #define SSL_R_NON_SSLV2_INITIAL_PACKET                   175
+#if defined(LIBRESSL_INTERNAL)
+#define SSL_R_NO_APPLICATION_PROTOCOL                    235
+#endif
 #define SSL_R_NO_CERTIFICATES_RETURNED                   176
 #define SSL_R_NO_CERTIFICATE_ASSIGNED                    177
 #define SSL_R_NO_CERTIFICATE_RETURNED                    178
diff --git a/src/lib/libssl/ssl_err.c b/src/lib/libssl/ssl_err.c
index 8507690f79..9ea7cd469a 100644
--- a/src/lib/libssl/ssl_err.c
+++ b/src/lib/libssl/ssl_err.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_err.c,v 1.38 2021/05/16 08:24:21 jsing Exp $ */
+/* $OpenBSD: ssl_err.c,v 1.39 2021/09/10 09:25:29 tb Exp $ */
 /* ====================================================================
  * Copyright (c) 1999-2011 The OpenSSL Project.  All rights reserved.
  *
@@ -294,6 +294,7 @@ static ERR_STRING_DATA SSL_str_reasons[]= {
         {ERR_REASON(SSL_R_MISSING_VERIFY_MESSAGE), "missing verify message"},
         {ERR_REASON(SSL_R_MULTIPLE_SGC_RESTARTS) , "multiple sgc restarts"},
         {ERR_REASON(SSL_R_NON_SSLV2_INITIAL_PACKET), "non sslv2 initial packet"},
+        {ERR_REASON(SSL_R_NO_APPLICATION_PROTOCOL), "no application protocol"},
         {ERR_REASON(SSL_R_NO_CERTIFICATES_RETURNED), "no certificates returned"},
         {ERR_REASON(SSL_R_NO_CERTIFICATE_ASSIGNED), "no certificate assigned"},
         {ERR_REASON(SSL_R_NO_CERTIFICATE_RETURNED), "no certificate returned"},
diff --git a/src/lib/libssl/ssl_tlsext.c b/src/lib/libssl/ssl_tlsext.c
index 4d426f1487..3ad564964d 100644
--- a/src/lib/libssl/ssl_tlsext.c
+++ b/src/lib/libssl/ssl_tlsext.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssl_tlsext.c,v 1.98 2021/09/02 11:10:43 beck Exp $ */
+/* $OpenBSD: ssl_tlsext.c,v 1.99 2021/09/10 09:25:29 tb Exp $ */
 /*
  * Copyright (c) 2016, 2017, 2019 Joel Sing <jsing@openbsd.org>
  * Copyright (c) 2017 Doug Hogan <doug@openbsd.org>
@@ -85,9 +85,16 @@ tlsext_alpn_server_parse(SSL *s, uint16_t msg_types, CBS *cbs, int *alert)
         if (s->ctx->internal->alpn_select_cb == NULL)
                 return 1;
 
+        /*
+         * XXX - A few things should be considered here:
+         * 1. Ensure that the same protocol is selected on session resumption.
+         * 2. Should the callback be called even if no ALPN extension was sent?
+         * 3. TLSv1.2 and earlier: ensure that SNI has already been processed.
+         */
         r = s->ctx->internal->alpn_select_cb(s, &selected, &selected_len,
             CBS_data(&alpn), CBS_len(&alpn),
             s->ctx->internal->alpn_select_cb_arg);
+
         if (r == SSL_TLSEXT_ERR_OK) {
                 free(S3I(s)->alpn_selected);
                 if ((S3I(s)->alpn_selected = malloc(selected_len)) == NULL) {
@@ -97,9 +104,18 @@ tlsext_alpn_server_parse(SSL *s, uint16_t msg_types, CBS *cbs, int *alert)
                 }
                 memcpy(S3I(s)->alpn_selected, selected, selected_len);
                 S3I(s)->alpn_selected_len = selected_len;
+
+                return 1;
         }
 
-        return 1;
+        /* On SSL_TLSEXT_ERR_NOACK behave as if no callback was present. */
+        if (r == SSL_TLSEXT_ERR_NOACK)
+                return 1;
+
+        *alert = SSL_AD_NO_APPLICATION_PROTOCOL;
+        SSLerror(s, SSL_R_NO_APPLICATION_PROTOCOL);
+
+        return 0;
 
  err:
         *alert = SSL_AD_DECODE_ERROR;