1313430 - ssl_CreateECDHEphemeralKeyPair needs database password

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

The test suite fails to execute on my local system (and I cannot explain why it doesn't fail inside our CI environments).

When a database is in FIPS mode, it's necessary to provide the password whenever performing operatins that involve the private key store.

Function ssl_CreateECDHEphemeralKeyPair passes NULL for the password parameter, and therefore the private key operation fail.

All callers of ssl_CreateECDHEphemeralKeyPair must pass on the password parameter that can be obtained with ss->pkcs11PinArg.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: 1313430-v1.patch

Comment 2

[Eric Rescorla (:ekr)]

Comment on attachment 8805209 [details] [diff] [review]
1313430-v1.patch

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

Kaie:
Given that ssl_CreateECDHEphemeralKeyPair has access to sslSocket, why not just have it internally use pinArg rather than passing it as a separate argument

Comment 3

[Kai Engert (:KaiE:)]

(In reply to Eric Rescorla (:ekr) from comment #2)
> Given that ssl_CreateECDHEphemeralKeyPair has access to sslSocket, why not
> just have it internally use pinArg rather than passing it as a separate
> argument

Because sometimes ssl_CreateECDHEphemeralKeyPair gets called with a NULL sslSocket parameter, for example from ssl_CreateStaticECDHEKeyPair (visible in the patch).

Comment 4

[Eric Rescorla (:ekr)]

(In reply to Kai Engert (:kaie) from comment #3)
> (In reply to Eric Rescorla (:ekr) from comment #2)
> > Given that ssl_CreateECDHEphemeralKeyPair has access to sslSocket, why not
> > just have it internally use pinArg rather than passing it as a separate
> > argument
> 
> Because sometimes ssl_CreateECDHEphemeralKeyPair gets called with a NULL
> sslSocket parameter, for example from ssl_CreateStaticECDHEKeyPair (visible
> in the patch).

In that case please refactor as follows:

- Have one function which takes the pin argument that is called from ssl_CreateStaticECDHEKeyPair
- Have another function which just takes ss and then calls the first function with ss->pin

It's not cool to have the SSL/TLS code need to know about the PIN.

Comment 5

[Martin Thomson [:mt:]]

Comment on attachment 8805209 [details] [diff] [review]
1313430-v1.patch

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

In the spirit of ekr's comments, I think that we can just pass the socket in sslgrp.c.  Much less code to touch.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: 1313430-v2.patch

(In reply to Martin Thomson [:mt:] from comment #5)
> In the spirit of ekr's comments, I think that we can just pass the socket in
> sslgrp.c.  Much less code to touch.

True.

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8805538 [details] [diff] [review]
1313430-v2.patch

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

This looks good.  Changes to taste only.

::: lib/ssl/sslgrp.c
@@ +15,5 @@
>      sslEphemeralKeyPair *keyPair;
>      PRCallOnceType once;
>  } gECDHEKeyPairs[SSL_NAMED_GROUP_COUNT];
>  
> +struct group_and_ss {

I think that I would prefer to keep with the naming convention here: sslSocketAndGroupArg might be better.  And I would typedef this so that you can use the name without `struct`.

@@ +76,5 @@
>  ssl_FilterSupportedGroups(sslSocket *ss)
>  {
>      unsigned int i;
>      PRStatus prv;
> +    struct group_and_ss arg;

I would init this with the ss and NULL.

@@ +109,5 @@
>          index = group - ssl_named_groups;
>          PORT_Assert(index < SSL_NAMED_GROUP_COUNT);
>  
> +        arg.group = group;
> +        arg.ss = ss;

Then you only need to tweak the group here.

@@ +135,5 @@
>      sslEphemeralKeyPair *keyPair;
>      /* We index gECDHEKeyPairs by the named group.  Pointer arithmetic! */
>      unsigned int index = ecGroup - ssl_named_groups;
>      PRStatus prv;
> +    struct group_and_ss arg;

Init with ss and group.

Comment 8

[Kai Engert (:KaiE:)]

Fixed including the requested changes from comment 7.

https://hg.mozilla.org/projects/nss/rev/72b92e116f60