Fix ALPN - more fixes

* Clear proposed, along with selected, before looking at ClientHello
* Add test case for above
* Clear NPN seen after selecting ALPN on server
* Minor documentation updates

Reviewed-by: Emilia Käsper <emilia@openssl.org>
Reviewed-by: Rich Salz <rsalz@openssl.org>

3 files changed, 21 insertions, 14 deletions

diff --git a/doc/ssl/SSL_CTX_set_alpn_select_cb.pod b/doc/ssl/SSL_CTX_set_alpn_select_cb.pod
index 974ca8618b..1a3d92c03a 100644
--- a/doc/ssl/SSL_CTX_set_alpn_select_cb.pod
+++ b/doc/ssl/SSL_CTX_set_alpn_select_cb.pod
@@ -2,8 +2,8 @@
 
 =head1 NAME
 
-SSL_CTX_set_alpn_select_cb, SSL_CTX_set_alpn_protos, SSL_set_alpn_protos,
-SSL_get0_alpn_selected, SSL_select_next_proto - handle application layer
+SSL_CTX_set_alpn_protos, SSL_set_alpn_protos, SSL_CTX_set_alpn_select_cb,
+SSL_select_next_proto, SSL_get0_alpn_selected - handle application layer
 protocol negotiation (ALPN)
 
 =head1 SYNOPSIS
@@ -38,19 +38,19 @@ B<protos_len>.
 
 SSL_CTX_set_alpn_select_cb() sets the application callback B<cb> used by a
 server to select which protocol to use for the incoming connection. When B<cb>
-is NULL, no ALPN is not used. The B<arg> value is pointer which is passed to
+is NULL, ALPN is not used. The B<arg> value is a pointer which is passed to
 the application callback.
 
 B<cb> is the application defined callback. The B<in>, B<inlen> parameters are a
 vector in protocol-list format. The value of the B<out>, B<outlen> vector
-should be set to the value of a single protocol contained with in the B<in>,
+should be set to the value of a single protocol selected from the B<in>,
 B<inlen> vector. The B<arg> parameter is the pointer set via
 SSL_CTX_set_alpn_select_cb().
 
 SSL_select_next_proto() is a helper function used to select protocols. It
 implements the standard protocol selection. It is expected that this function
 is called from the application callback B<cb>. The protocol data in B<server>,
-B<server_len> and B<client>, B<client_len> must be in protocol-list format
+B<server_len> and B<client>, B<client_len> must be in the protocol-list format
 described below. The first item in the B<server>, B<server_len> list that
 matches an item in the B<client>, B<client_len> list is selected, and returned
 in B<out>, B<outlen>. The B<out> value will point into either B<server> or
@@ -60,7 +60,7 @@ function can also be used in the NPN callback.
 
 SSL_get0_alpn_selected() returns a pointer to the selected protocol in B<data>
 with length B<len>. It is not NUL-terminated. B<data> is set to NULL and B<len>
-is set to 0 if no protocol has been selected. B<data> value must not be freed.
+is set to 0 if no protocol has been selected. B<data> must not be freed.
 
 =head1 NOTES
 
diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c
index 26c02a0e44..a20e85fb4b 100644
--- a/ssl/t1_lib.c
+++ b/ssl/t1_lib.c
@@ -1801,6 +1801,10 @@ static int tls1_alpn_handle_client_hello_late(SSL *s, int *ret, int *al)
                 return 0;
             }
             s->s3->alpn_selected_len = selected_len;
+#ifndef OPENSSL_NO_NEXTPROTONEG
+            /* ALPN takes precedence over NPN. */
+            s->s3->next_proto_neg_seen = 0;
+#endif
         } else {
             *al = SSL_AD_NO_APPLICATION_PROTOCOL;
             *ret = SSL_TLSEXT_ERR_ALERT_FATAL;
@@ -1902,6 +1906,10 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
 
     OPENSSL_free(s->s3->alpn_selected);
     s->s3->alpn_selected = NULL;
+    s->s3->alpn_selected_len = 0;
+    OPENSSL_free(s->s3->alpn_proposed);
+    s->s3->alpn_proposed = NULL;
+    s->s3->alpn_proposed_len = 0;
 #ifndef OPENSSL_NO_HEARTBEATS
     s->tlsext_heartbeat &= ~(SSL_DTLSEXT_HB_ENABLED |
                              SSL_DTLSEXT_HB_DONT_SEND_REQUESTS);
@@ -2216,8 +2224,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
 #endif
 #ifndef OPENSSL_NO_NEXTPROTONEG
         else if (type == TLSEXT_TYPE_next_proto_neg &&
-                 s->s3->tmp.finish_md_len == 0 &&
-                 s->s3->alpn_selected == NULL) {
+                 s->s3->tmp.finish_md_len == 0) {
             /*-
              * We shouldn't accept this extension on a
              * renegotiation.
@@ -2243,10 +2250,6 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
                  s->s3->tmp.finish_md_len == 0) {
             if (!tls1_alpn_handle_client_hello(s, &extension, al))
                 return 0;
-#ifndef OPENSSL_NO_NEXTPROTONEG
-            /* ALPN takes precedence over NPN. */
-            s->s3->next_proto_neg_seen = 0;
-#endif
         }
 
         /* session ticket processed earlier */
diff --git a/test/recipes/80-test_ssl.t b/test/recipes/80-test_ssl.t
index ba57695b22..da32fac6c7 100644
--- a/test/recipes/80-test_ssl.t
+++ b/test/recipes/80-test_ssl.t
@@ -627,10 +627,10 @@ sub testssl {
     subtest 'ALPN tests' => sub {
 	######################################################################
 
-	plan tests => 12;
+	plan tests => 13;
 
       SKIP: {
-	  skip "TLSv1.0 is not supported by this OpenSSL build", 12
+	  skip "TLSv1.0 is not supported by this OpenSSL build", 13
 	      if $no_tls1;
 
 	  ok(run(test([@ssltest, "-bio_pair", "-tls1", "-alpn_client", "foo"])));
@@ -658,6 +658,10 @@ sub testssl {
 		       "-alpn_server1", "foo,123", "-sn_server1", "alice",
 		       "-alpn_server2", "bar,456", "-sn_server2", "bob",
 		       "-alpn_expected", "bar"])));
+	  ok(run(test([@ssltest, "-bio_pair",
+		       "-alpn_client", "foo,bar", "-sn_client", "bob",
+		       "-alpn_server2", "bar,456", "-sn_server2", "bob",
+		       "-alpn_expected", "bar"])));
 	}
     };