1282779 - Copy ALPN infos in ssl_DupSocket

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Franziskus Kiefer [:franziskus]]

ssl_DupSocket doesn't duplicate ALPN information and thus makes ALPN not usable in server settings such as mod_nss.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: copy_alpn_in_dupsocket.patch

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 8765915 [details] [diff] [review]
copy_alpn_in_dupsocket.patch

Review of attachment 8765915 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

::: external_tests/ssl_gtest/tls_connect.cc
@@ +378,5 @@
>    client_->EnableAlpn(val, len);
>    server_->EnableAlpn(val, len);
>  }
>  
> +void TlsConnectTestBase::SetupModelSockets() {

EnsureModelSockets

@@ +385,5 @@
> +    client_model_ = new TlsAgent(TlsAgent::kClient, TlsAgent::CLIENT, mode_);
> +  }
> +  if (!server_model_) {
> +    server_model_ = new TlsAgent(TlsAgent::kServerRsa, TlsAgent::SERVER, mode_);
> +  }

There's no non-error way that one of these can be nonzero, so I would just check that one is zero and ASSERT_EQ the other.

@@ +389,5 @@
> +  }
> +
> +  // Initialise agents.
> +  EXPECT_TRUE(client_model_->Init());
> +  EXPECT_TRUE(server_model_->Init());

I would ASSERT_TRUE() because bad shit is going to happen below.

@@ +393,5 @@
> +  EXPECT_TRUE(server_model_->Init());
> +
> +  // Set desired properties on the models.
> +  // For now only ALPN.
> +  static const uint8_t val[] = { 0x01, 0x62, 0x01, 0x61 };

Let's find some way to share the value here with the other EnableAlpn() thingy.

::: lib/ssl/sslsock.c
@@ +325,5 @@
>          ss->canFalseStartCallback = os->canFalseStartCallback;
>          ss->canFalseStartCallbackData = os->canFalseStartCallbackData;
>          ss->pkcs11PinArg = os->pkcs11PinArg;
> +        ss->nextProtoCallback = os->nextProtoCallback;
> +        ss->nextProtoArg = os->nextProtoArg;

This looks like a long-standing bug?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8765915 [details] [diff] [review]
copy_alpn_in_dupsocket.patch

Review of attachment 8765915 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm with ekr's comment addressed as well

Comment 4

[Franziskus Kiefer [:franziskus]]

> This looks like a long-standing bug?

yeah, looks like alpn never worked with a model socket.

landed as https://hg.mozilla.org/projects/nss/rev/ea61f0660c31