Closed Bug 1521578 Opened 5 years ago Closed 5 years ago

x25519 support in pk11pars.c

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alexander.m.scheel, Assigned: alexander.m.scheel)

Details

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36

Steps to reproduce:

NSS does not understand curve25519/x25519 as a cipher under the config="<...>" parameter in pkcs11.txt.

From here:

https://github.com/nss-dev/nss/blob/master/lib/pk11wrap/pk11pars.c#L197

It appears to be missing as an entry in curveOptList.

A minimal patch to fix this would be:

diff --git a/lib/pk11wrap/pk11pars.c b/lib/pk11wrap/pk11pars.c
index db60f7c9d..b50316db4 100644
--- a/lib/pk11wrap/pk11pars.c
+++ b/lib/pk11wrap/pk11pars.c
@@ -238,6 +238,8 @@ static const oidValDef curveOptList[] = {
NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },
{ CIPHER_NAME("SECP521R1"), SEC_OID_SECG_EC_SECP521R1,
NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },

  • { CIPHER_NAME("CURVE25519"), SEC_OID_CURVE25519,
  •  NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },
    
    /* ANSI X9.62 named elliptic curves (characteristic two field) */
    { CIPHER_NAME("C2PNB163V1"), SEC_OID_ANSIX962_EC_C2PNB163V1,
    NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },

More detail:

This page mentions configuration options: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_Config_Options

Curve25519 is not mentioned on that page, but NSS has support for Curve25519. Disabling Curve25519 via configuration option appears not to work. This can be tested by, e.g., using a NSS-enabled build of curl, capturing the Client Hello and noticing that X25519 is specified. Adding config="disallow=Curve25519" to the default NSS DB's pkcs11.txt will not remove X25519 from the Client Hello (supported groups extension), but adding config="disallow=secp256r1" will remove the specified curve as that parameter is understood.

While disabling x25519 is generally a bad idea, it should be added for completeness (and FIPS support).

Actual results:

Curve25519 or x25519 parameters were ignored.

Expected results:

Curve25519 parameters should be respected, both under the disallow= or allow= lists.

Looks like the patch didn't get formatted well:

diff --git a/lib/pk11wrap/pk11pars.c b/lib/pk11wrap/pk11pars.c
index db60f7c9d..b50316db4 100644
--- a/lib/pk11wrap/pk11pars.c
+++ b/lib/pk11wrap/pk11pars.c
@@ -238,6 +238,8 @@ static const oidValDef curveOptList[] = {
       NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },
     { CIPHER_NAME("SECP521R1"), SEC_OID_SECG_EC_SECP521R1,
       NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },
+    { CIPHER_NAME("CURVE25519"), SEC_OID_CURVE25519,
+      NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },
     /* ANSI X9.62 named elliptic curves (characteristic two field) */
     { CIPHER_NAME("C2PNB163V1"), SEC_OID_ANSIX962_EC_C2PNB163V1,
       NSS_USE_ALG_IN_SSL_KX | NSS_USE_ALG_IN_CERT_SIGNATURE },

Thank you, Alexander. Would you like to submit the patch following:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch

It would be nice if the patch comes with a test, something like:
https://hg.mozilla.org/projects/nss/rev/4bc22e14a592

Flags: needinfo?(alexander.m.scheel)
Status: UNCONFIRMED → NEW
Ever confirmed: true

Sure, happy to. I'll try to get to this later today.

-- Alex

Flags: needinfo?(alexander.m.scheel)

How's that look? I'm not 100% sure on the test case -- my testing with:

time HOST=localhost DOMSUF=localdomain GTESTS="ssl_gtest" ./tests/gtests/gtests.sh

didn't seem to trigger the test case. Perhaps I'm running it wrong. Log here:

https://paste.fedoraproject.org/paste/GcdOTKfBaOH1R5whYgPv1w

Thanks!

It appears that there is something wrong in the individual test setup, indeed. I guess you could run it through the entire test harness like this:

cd tests
DOMSUF=localdomain USE_64=1 NSS_TESTS=ssl_gtests NSS_CYCLES=standard GTESTFILTER='ExtensionTls12/TlsExtensionTest12.SupportedCurvesDisableX25519/*' ./all.sh
Comment on attachment 9048970 [details] [diff] [review]
nss-x25519-pkcs11.patch

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

::: gtests/ssl_gtest/ssl_extension_unittest.cc
@@ +486,5 @@
> +  // Per rfc8422, x25519 in the supported curves extension is given value 29.
> +  uint32_t known_x25519 = 29;
> +
> +  // Ensure that we can enable its use in the key exchange.
> +  SECStatus rv = NSS_SetAlgorithmPolicy(SEC_OID_CURVE25519,

SEC_OID_CURVE25519 needs to be listed in TlsConnectTestBase.algorithms_ so that the change only affects this test.

@@ +511,5 @@
> +      break;
> +    }
> +  }
> +  ASSERT_EQ(true, seen1_x25519);
> +

Reset() is needed here.

@@ +539,5 @@
> +    }
> +  }
> +  ASSERT_EQ(false, seen2_x25519);
> +
> +  ASSERT_EQ(false, true);

Remove this.
Attached patch nss-x25519-pkcs11-tested.patch (obsolete) — Splinter Review

Sorry, I apparently can't use HG, take two.

Attachment #9049590 - Attachment is obsolete: true

Cool, many thanks for your help! The new test command worked better. :)

+  // Clean up after the last run.
+  ClearServerCache();
+  Reset();

One thing I found out was that Reset() alone wasn't sufficient. I'd get output like this:

[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from ExtensionTls12/TlsExtensionTest12
[ RUN      ] ExtensionTls12/TlsExtensionTest12.SupportedCurvesDisableX25519/0
Version: TLS 1.2
server: Changing state from INIT to CONNECTING
client: Changing state from INIT to CONNECTING
server: Handshake success
server: Changing state from CONNECTING to CONNECTED
client: Handshake success
client: Changing state from CONNECTING to CONNECTED
Connected with version 771 cipher suite TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
client: Warning alert sent: 0
server: Changing state from INIT to CONNECTING
client: Changing state from INIT to CONNECTING
client: Handshake success
../../gtests/ssl_gtest/tls_agent.cc:765: Failure
Expected equality of these values:
  !expect_resumption_
    Which is: true
  auth_certificate_hook_called_
    Which is: false
../../gtests/ssl_gtest/tls_agent.cc:836: Failure
Expected equality of these values:
  expect_resumption_
    Which is: false
  info_.resumed == 1
    Which is: true
client: Changing state from CONNECTING to CONNECTED
server: Handshake success
../../gtests/ssl_gtest/tls_agent.cc:763: Failure
Expected equality of these values:
  ((!expect_resumption_ && have_sni) || expected_version_ >= 0x0304)
    Which is: true
  sni_hook_called_
    Which is: false
../../gtests/ssl_gtest/tls_agent.cc:836: Failure
Expected equality of these values:
  expect_resumption_
    Which is: false
  info_.resumed == 1
    Which is: true
server: Changing state from CONNECTING to CONNECTED
Connected with version 771 cipher suite TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
../../gtests/ssl_gtest/tls_connect.cc:567: Failure
Expected equality of these values:
  resume_count
    Which is: 0
  stats->hch_sid_cache_hits
    Which is: 1
../../gtests/ssl_gtest/tls_connect.cc:568: Failure
Expected equality of these values:
  resume_count
    Which is: 0
  stats->hsh_sid_cache_hits
    Which is: 1
client: Warning alert sent: 0
[  FAILED  ] ExtensionTls12/TlsExtensionTest12.SupportedCurvesDisableX25519/0, where GetParam() = (0, 771) (15 ms)
[ RUN      ] ExtensionTls12/TlsExtensionTest12.SupportedCurvesDisableX25519/1
Version: DTLS 1.2
server: Changing state from INIT to CONNECTING
client: Changing state from INIT to CONNECTING
server: Handshake success
server: Changing state from CONNECTING to CONNECTED
client: Handshake success
client: Changing state from CONNECTING to CONNECTED
Connected with version 771 cipher suite TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
client: Warning alert sent: 0
server: Changing state from INIT to CONNECTING
client: Changing state from INIT to CONNECTING
client: Handshake success
../../gtests/ssl_gtest/tls_agent.cc:765: Failure
Expected equality of these values:
  !expect_resumption_
    Which is: true
  auth_certificate_hook_called_
    Which is: false
../../gtests/ssl_gtest/tls_agent.cc:836: Failure
Expected equality of these values:
  expect_resumption_
    Which is: false
  info_.resumed == 1
    Which is: true
../../gtests/ssl_gtest/tls_agent.cc:817: Failure
Expected equality of these values:
  expected
    Which is: 2
  cipherSpecs
    Which is: 3
Cipher specs for client
  write spec epoch=0 (cleartext) refct=1
  read spec epoch=1 ((null)) refct=1
  write spec epoch=1 ((null)) refct=2
client: Changing state from CONNECTING to CONNECTED
server: Handshake success
../../gtests/ssl_gtest/tls_agent.cc:763: Failure
Expected equality of these values:
  ((!expect_resumption_ && have_sni) || expected_version_ >= 0x0304)
    Which is: true
  sni_hook_called_
    Which is: false
../../gtests/ssl_gtest/tls_agent.cc:836: Failure
Expected equality of these values:
  expect_resumption_
    Which is: false
  info_.resumed == 1
    Which is: true
../../gtests/ssl_gtest/tls_agent.cc:817: Failure
Expected equality of these values:
  expected
    Which is: 3
  cipherSpecs
    Which is: 2
Cipher specs for server
  read spec epoch=1 ((null)) refct=1
  write spec epoch=1 ((null)) refct=1
server: Changing state from CONNECTING to CONNECTED
Connected with version 771 cipher suite TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
../../gtests/ssl_gtest/tls_connect.cc:567: Failure
Expected equality of these values:
  resume_count
    Which is: 0
  stats->hch_sid_cache_hits
    Which is: 1
../../gtests/ssl_gtest/tls_connect.cc:568: Failure
Expected equality of these values:
  resume_count
    Which is: 0
  stats->hsh_sid_cache_hits
    Which is: 1
client: Warning alert sent: 0
[  FAILED  ] ExtensionTls12/TlsExtensionTest12.SupportedCurvesDisableX25519/1, where GetParam() = (1, 771) (14 ms)
[----------] 2 tests from ExtensionTls12/TlsExtensionTest12 (29 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (30 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] ExtensionTls12/TlsExtensionTest12.SupportedCurvesDisableX25519/0, where GetParam() = (0, 771)
[  FAILED  ] ExtensionTls12/TlsExtensionTest12.SupportedCurvesDisableX25519/1, where GetParam() = (1, 771)

 2 FAILED TESTS
ssl_gtest.sh: #23: ssl_gtests ran successfully  - FAILED

Which to me says that the server is caching the previous session... So I saw the ClearServerCache() call and that seemed to help. Perhaps I'm misunderstanding the testing API though.

Other than that, your previous feedback should be addressed in this patch.

(In reply to Alexander Scheel from comment #10)

Thank you for the update, and sorry for the delay; it's taking time to recover from the holidays.

One thing I found out was that Reset() alone wasn't sufficient. I'd get output like this:
[...]
client: Handshake success
../../gtests/ssl_gtest/tls_agent.cc:765: Failure
Expected equality of these values:
!expect_resumption_
Which is: true
auth_certificate_hook_called_
Which is: false
../../gtests/ssl_gtest/tls_agent.cc:836: Failure

Which to me says that the server is caching the previous session... So I saw the ClearServerCache() call and that seemed to help. Perhaps I'm misunderstanding the testing API though.

I would rather disable session resumption as we are not interested in testing that. Anyway, it looks good to me.

Attachment #9049594 - Flags: review+
Assignee: nobody → alexander.m.scheel
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.44

Cool, changes friendly.

ssl_grp_ec_curve25519 and ConfigureSessionCache were things I missed. :)

Many thanks!

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: