1351314 - TLS exporter: reject contexts that are longer than 2^16

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Martin Thomson [:mt:]]

In TLS 1.2 and earlier, there is a size limit on the context that is passed to SSL_ExportKeyingMaterial.  We don't enforce this limit.

Comment 1

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Add a length check to conform to RFC 5705 and tests

Comment 2

[Martin Thomson [:mt:]]

Comment on attachment 8865675 [details] [diff] [review]
Add a length check to conform to RFC 5705 and tests

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

You should be putting these up on phabricator.  https://nss-review.dev.mozaws.net/

::: gtests/ssl_gtest/ssl_exporter_unittest.cc
@@ +94,5 @@
> +  EXPECT_EQ(SECFailure,
> +            SSL_ExportKeyingMaterial(
> +                server->ssl_fd(), kExporterLabel, strlen(kExporterLabel),
> +                PR_TRUE, kExporterContextTooLong,
> +                sizeof(kExporterContextTooLong), server_value, sizeof(server_value)));

You need to check the value returned by PORT_GetError() as well.

@@ +110,5 @@
> +TEST_P(TlsConnectPre12, ExporterContextLengthTooLongPreTls12) {
> +  EnsureTlsSetup();
> +  server_->EnableSingleCipher(TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA);
> +  Connect();
> +  CheckKeys();

You shouldn't need to split this test into two, you are only doing that to get a different cipher suite, which is an unnecessary step.

::: lib/ssl/sslinfo.c
@@ +1,5 @@
>  /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include <limits.h>

NSPR has its own limits, which you should use here.  I know, crappy.

Comment 3

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Add a length check to conform to RFC 5705 and tests

* Change limits.h/climits to stdint.h to compile on Windows
* Use TlsConnectGenericPre13
* clang-format

Comment 4

[Kate McKinley [:kmckinley, :Kate]]

Attachment: Add a length check to conform to RFC 5705 and tests

Waiting for Phabricator access

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8865691 [details] [diff] [review]
Add a length check to conform to RFC 5705 and tests

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

::: gtests/ssl_gtest/ssl_exporter_unittest.cc
@@ +106,5 @@
> +  server_->EnableSingleCipher(TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA);
> +  Connect();
> +  CheckKeys();
> +
> +  TooLongContextLengthShouldFail(client_, server_);

You can inline this when it's not used anywhere else.

Comment 6

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/44595eb0471ab460062b978bebf36144af8cb1ad
Bug 1351314 - Add length check for SSL context for TLS 1.2 or prior, r=mt