1242227 - Document SSL_ERROR_SERVER_KEY_EXCHANGE_FAILURE and SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE better

Status: NEW

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Improve error messages for SSL_ERROR_SERVER_KEY_EXCHANGE_FAILURE and SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE

SSL_ERROR_SERVER_KEY_EXCHANGE_FAILURE and SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE
are used in both the code that generates a ServerKeyExchange/ClientKeyExchange
and the code that processes a ServerKeyExchange/ClientKeyExchange. Their error
messages should cover both cases.

Also, SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE is incorrectly used in the
getWrappingKey function, which has nothing to do with ClientKeyExchange.

Comment 1

[Eric Rescorla (:ekr)]

r+ but I note that this does not change getWrappingKey.

Comment 2

[Wan-Teh Chang]

Attachment: Improve error reporting in ssl_UnwrapSymWrappingKey and getWrappingKey

1. Set an error code before returning on failure.

2. getWrappingKey should not use the high-level error code
   because it is never used in generating or processing the
   ClientKeyExchange message.

Comment 3

[Wan-Teh Chang]

I missed the actual high-level error code in the previous comment.
The second list item should read:

2. getWrappingKey should not use the high-level error code
   SSL_ERROR_CLIENT_KEY_EXCHANGE_FAILURE because it is never used
   in generating or processing the ClientKeyExchange message.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 8711475 [details] [diff] [review]
Improve error reporting in ssl_UnwrapSymWrappingKey and getWrappingKey

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

::: lib/ssl/ssl3con.c
@@ +5723,5 @@
>  #endif
>  
>      default:
>  	/* Assert? */
> +        PORT_SetError(SEC_ERROR_UNSUPPORTED_KEYALG);

Per this comment, I think an assert here would make sense.

@@ +5994,1 @@
>  	goto loser;

BUG? If the call to PK11_PubWrapSymKey() on line 5895 fails, doesn't we end up here with rv == SECFailure, but without having called PORT_SetError()?

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.