1296303 - Make ssl_GetECGroupForServerSocket better

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Franziskus Kiefer [:franziskus]]

ssl_GetECGroupForServerSocket current does the following

> Find the "weakest link".  Get the strength of the signature and symmetric
> keys and choose a curve based on the weakest of those two.

We shouldn't do this, i.e. we shouldn't reduce strength of the key exchange if the certificate key is weak. Instead we should make the curve strength dependent on the strength of the record protection.

Comment 1

[Eric Rescorla (:ekr)]

But this would imply that I should use P521 with ChaCha, even though I use P256 with AES-128. That seems unfortunate.

Comment 2

[Franziskus Kiefer [:franziskus]]

(In reply to Eric Rescorla (:ekr) from comment #1)
> But this would imply that I should use P521 with ChaCha, even though I use
> P256 with AES-128. That seems unfortunate.

Yeah, I agree that's not ideal. But we have to find a better way to choose the right curve. From a crypto point something like P521 should go together with ChaCha. But if ChaCha is used only for better performance and not the additional security, something smaller than P521 should probably be allowed.
Considering that we only support 128-bit secure curves right now we don't pick a small curve only because the server cert is 2048 bit RSA. So maybe there's nothing to do here right now. But the algorithm we use here doesn't make me particularly happy.

Comment 3

[Martin Thomson [:mt:]]

Maybe we can try to be a little less mathematical.

if (256-bit cipher) {
  curve_min_bits = 384;
} else {
  curve_min_bits = 255;
}

The same logic might apply to FFDHE, substitute 3072 and 2048.  Yeah, those don't even come close in terms of strength, but we have to balance performance here.

Comment 4

[Franziskus Kiefer [:franziskus]]

(In reply to Martin Thomson [:mt:] from comment #3)
> Maybe we can try to be a little less mathematical.
> 
> if (256-bit cipher) {
>   curve_min_bits = 384;
> } else {
>   curve_min_bits = 255;
> }

This sounds reasonable to me. Unfortunately it breaks quite some tests, which makes me think that it might break more. In particular do we use P256 for AES256 but would force 384 with this approach. If it's really only fixing tests, that's fine with me. But I'm not sure if we wouldn't break compatibility with many other implementations.