Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLEVEL >= 3 #17763

romen · 2022-02-23T21:28:51Z

This is an alternative to #17759 to fix #17743.

It comprises 3 commits:

The first one improves maintainability of the codebase replacing leftover uses of SSL_kE(EC)?DH with SSL_k(EC)?DHE, as it the latter spelling is more common and familiar to internal and external developers. Each pair is already reported as aliases in our headers:

openssl/ssl/ssl_local.h

Lines 165 to 172 in a044af4

    
           /* tmp DH key no DH cert */ 
        
           # define SSL_kDHE                0x00000002U 
        
           /* synonym */ 
        
           # define SSL_kEDH                SSL_kDHE 
        
           /* ephemeral ECDH */ 
        
           # define SSL_kECDHE              0x00000004U 
        
           /* synonym */ 
        
           # define SSL_kEECDH              SSL_kECDHE

The second commit adds the SSL_kDHEPSK and SSL_kECDHEPSK flags to the PFS check for SECLEVEL >= 3
The last commit adds positive and negative tests to exercise the check with the flags added in the second commit

The test commit further revises the test binary to improve the -help documentation, syncing it with the actual behavior (e.g., the default is -dhe2048 not -dhe1024; -tls1_2 is a supported option, but was not documented as such), and also adds support (and documentation) for -tls1_1 and for well-known 4096-bit DH parameters.

The support for TLS 1.1 is added as the new tests require SECLEVEL >= 3, which implies TLS >= 1.1.
Similarly, the support for -dhe4096 is added because 2048-bit parameters fail to meet the requirement for >=128 security bits

Checklist

documentation is added or updated
tests are added or updated

`SSL_kECDHE` and `SSL_kEECDH`, and `SSL_kDHE` and `SSL_kEDH` are already marked as aliases of each other in the headers. This commit, for each pair, replaces the leftover uses of the latter synonym with the first one, which is considered more common.

…VEL >= 3 Fixes openssl#17743

…EL >= 3

romen · 2022-02-23T21:55:48Z

test/recipes/80-test_ssl_old.t

@@ -567,7 +567,7 @@ sub testssl {

            ok(run(test([@ssltest, "-bio_pair", "-tls1", "-cipher", "PSK", "-psk", "abc123"])),
               'test tls1 with PSK via BIO pair');
-	  }
+          }


The indentation of the SKIP: { blocks and related closing braces is a bit wonky throughout the file.
A pass could be done to improve the indentation, but I am likely not the right person because I don't fully grasp the convention used for our Perl test recipes.

I can do such a pass in this file if requested, but I would need guidance from @mattcaswell

…EL >= 3 Align option explanations

beldmit

Nice work!

slontis

Should this have a CHANGES entry? - at the minimum this then indicates that it was a bug and it is documented when it was fixed

…r SECLEVEL >= 3 Add CHANGES entry: credits to Dmitry Belyavskiy for reporting the issue and proposing the resolution, myself for the final implementation and work on tests.

romen · 2022-02-25T11:21:43Z

Should this have a CHANGES entry? - at the minimum this then indicates that it was a bug and it is documented when it was fixed

I pushed a fixup commit with a CHANGES entry.

@beldmit @paulidale can you rereview?
I am also adding the 1.1.1 label, after #17759 (comment).

beldmit · 2022-02-25T11:35:13Z

CHANGES.md

+   to the list of ciphersuites providing Perfect Forward Secrecy as
+   required by SECLEVEL >= 3.
+
+   *Dmitry Belyavskiy, Nicola Tuveri*


Not sure about the order because you did almost all work.

I stand by it: you reported the issue, proposed its resolution to the OTC, and the work we did independently to implement the actual change is virtually identical.

beldmit

LGTM

romen · 2022-02-25T12:19:47Z

The origin branch on my github fork is reporting failures in the compiler-zoo and windows work flows, but they seem to be due to GitHub timeouts rather than build errors under our control.

openssl-machine · 2022-02-25T14:00:12Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

t8m · 2022-02-28T09:35:33Z

@paulidale please reconfirm

`SSL_kECDHE` and `SSL_kEECDH`, and `SSL_kDHE` and `SSL_kEDH` are already marked as aliases of each other in the headers. This commit, for each pair, replaces the leftover uses of the latter synonym with the first one, which is considered more common. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17763)

…VEL >= 3 Fixes #17743 Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17763)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17763)

romen · 2022-03-01T22:24:26Z

Merged to master with:

66914fc [ssl] Prefer SSL_k(EC)?DHE to the SSL_kE(EC)?DH alias
b139a95 [ssl] Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLEVEL >= 3
d71151a [ssl] Add tests for Perfect Forward Secrecy criteria on SECLEVEL >= 3

Merged to 3.0 (required trivial edits during cherry pick for CHANGES.md) with:

1925edb [ssl] Prefer SSL_k(EC)?DHE to the SSL_kE(EC)?DH alias
a108f66 [ssl] Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLEVEL >= 3
679a4f7 [ssl] Add tests for Perfect Forward Secrecy criteria on SECLEVEL >= 3

Backport to 1.1.1 is being tackled in #17791 as it requires non-trivial amendments.

romen · 2022-03-01T22:24:52Z

Thanks everyone for all the feedback!

`SSL_kECDHE` and `SSL_kEECDH`, and `SSL_kDHE` and `SSL_kEDH` are already marked as aliases of each other in the headers. This commit, for each pair, replaces the leftover uses of the latter synonym with the first one, which is considered more common. Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17763) (cherry picked from commit 66914fc)

…VEL >= 3 Fixes #17743 Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17763) (cherry picked from commit b139a95)

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #17763) (cherry picked from commit d71151a)

romen added 3 commits February 23, 2022 23:12

[ssl] Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLE…

ecd3acc

…VEL >= 3 Fixes openssl#17743

[ssl] Add tests for Perfect Forward Secrecy criteria on SECLEVEL >= 3

1aae611

romen added branch: master Merge to master branch approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch labels Feb 23, 2022

romen requested review from mattcaswell and beldmit February 23, 2022 21:28

romen self-assigned this Feb 23, 2022

romen mentioned this pull request Feb 23, 2022

Permit (EC)DHEPSK KEX on SECLEVEL3 #17759

Closed

2 tasks

fixup! [ssl] Add tests for Perfect Forward Secrecy criteria on SECLEV…

8ab78c7

…EL >= 3

romen commented Feb 23, 2022

View reviewed changes

fixup! [ssl] Add tests for Perfect Forward Secrecy criteria on SECLEV…

065a365

…EL >= 3 Align option explanations

paulidale approved these changes Feb 23, 2022

View reviewed changes

paulidale added approval: review pending This pull request needs review by a committer and removed approval: otc review pending This pull request needs review by an OTC member labels Feb 23, 2022

beldmit approved these changes Feb 24, 2022

View reviewed changes

beldmit added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Feb 24, 2022

slontis reviewed Feb 25, 2022

View reviewed changes

fixup! [ssl] Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites fo…

45eadce

…r SECLEVEL >= 3 Add CHANGES entry: credits to Dmitry Belyavskiy for reporting the issue and proposing the resolution, myself for the final implementation and work on tests.

romen added the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Feb 25, 2022

beldmit reviewed Feb 25, 2022

View reviewed changes

beldmit approved these changes Feb 25, 2022

View reviewed changes

paulidale approved these changes Mar 1, 2022

View reviewed changes

paulidale added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Mar 1, 2022

t8m approved these changes Mar 1, 2022

View reviewed changes

romen mentioned this pull request Mar 1, 2022

[1.1.1-bp] Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLEVEL >= 3 #17791

Closed

2 tasks

romen closed this Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLEVEL >= 3 #17763

Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLEVEL >= 3 #17763

romen commented Feb 23, 2022 •

edited

romen Feb 23, 2022

beldmit left a comment

slontis left a comment

romen commented Feb 25, 2022

beldmit Feb 25, 2022

romen Feb 25, 2022

beldmit left a comment

romen commented Feb 25, 2022

openssl-machine commented Feb 25, 2022

t8m commented Feb 28, 2022

romen commented Mar 1, 2022 •

edited

romen commented Mar 1, 2022

	/* tmp DH key no DH cert */
	# define SSL_kDHE 0x00000002U
	/* synonym */
	# define SSL_kEDH SSL_kDHE
	/* ephemeral ECDH */
	# define SSL_kECDHE 0x00000004U
	/* synonym */
	# define SSL_kEECDH SSL_kECDHE

Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLEVEL >= 3 #17763

Add SSL_kDHEPSK and SSL_kECDHEPSK as PFS ciphersuites for SECLEVEL >= 3 #17763

Conversation

romen commented Feb 23, 2022 • edited

Checklist

romen Feb 23, 2022

Choose a reason for hiding this comment

beldmit left a comment

Choose a reason for hiding this comment

slontis left a comment

Choose a reason for hiding this comment

romen commented Feb 25, 2022

beldmit Feb 25, 2022

Choose a reason for hiding this comment

romen Feb 25, 2022

Choose a reason for hiding this comment

beldmit left a comment

Choose a reason for hiding this comment

romen commented Feb 25, 2022

openssl-machine commented Feb 25, 2022

t8m commented Feb 28, 2022

romen commented Mar 1, 2022 • edited

romen commented Mar 1, 2022

romen commented Feb 23, 2022 •

edited

romen commented Mar 1, 2022 •

edited