ecx_keys: Handle weak x keys as insufficient_security alert by jogme · Pull Request #27597 · openssl/openssl

jogme · 2025-05-11T08:25:53Z

OpenSSL handles this issue with internal_error which is not giving a proper explanation of the issue.

Thank you for the report @GeorgePantelakis!

Resolves: #27531

Checklist

documentation is added or updated
tests are added or updated

Sashan

just fix the cstyle and you'll be good to go. thanks.

Sashan · 2025-05-12T08:52:00Z

-            ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_DURING_DERIVATION);
-            return 0;
-        }
+                ERR_raise(ERR_LIB_PROV, ERR_LIB_EC);


I think the indentation here is of. it needs to be moved by 4 spaces left. So it is aligned with ossl_
cstyle here is hard.

Sashan · 2025-05-12T08:52:23Z

                & S390X_CAPBIT(S390X_SCALAR_MULTIPLY_X448)) {
            if (s390x_x448_mul(secret, peer->pubkey, priv->privkey) == 0) {
-                ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_DURING_DERIVATION);
+                ERR_raise(ERR_LIB_PROV, ERR_LIB_EC);


c-style here is OK

Sashan · 2025-05-12T08:52:36Z

-            ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_DURING_DERIVATION);
-            return 0;
-        }
+                ERR_raise(ERR_LIB_PROV, ERR_LIB_EC);


and here is off again.

For some reason the style check did detect issues here and this was the solution. I might check the style-checker for some errors

The style checker has to be taken with grain of salt. It is not and cannot be perfect. We have the style: waived label that can be added after manual review of the style checker report.

For some reason the style check did detect issues here and this was the solution. I might check the style-checker for some errors

I think c-style checker is confused by errors which exists in the file already

Okay, I changed it back to normal and then deleted as seen below

t8m · 2025-05-12T08:58:10Z

                & S390X_CAPBIT(S390X_SCALAR_MULTIPLY_X25519)) {
            if (s390x_x25519_mul(secret, peer->pubkey, priv->privkey) == 0) {
-                ERR_raise(ERR_LIB_PROV, PROV_R_FAILED_DURING_DERIVATION);
+                ERR_raise(ERR_LIB_PROV, ERR_LIB_EC);


Why are you changing the reason code? That does not make sense to me. You can test for ERR_LIB_PROV && PROV_R_FAILED_DURING_DERIVATION in libssl with no issue.

For the first time it looked like there are more paths this error code can happen, but now I searched it up and it's used only in one place of hpke code and here. So it doesn't make sense to use a different error code here. Changed back.

t8m · 2025-05-12T09:03:21Z

+        /*
+         * the public key probably was a weak key
+         */
+        if (ERR_GET_REASON(ERR_peek_last_error()) == ERR_LIB_EC) {
+            SSLfatal(s, SSL_AD_INSUFFICIENT_SECURITY, SSL_R_BAD_ECPOINT);
+        } else {
+            SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
+        }


I would not check the reason code at all and used SSL_AD_ILLEGAL_PARAMETER instead as suggested in #27531. The returned reason code should be SSL_R_BAD_KEY_SHARE.

The EVP_PKEY_derive can fail on many different reasons; would it be good to report all of them as ILLEGAL_PARAMETER?

It is IMO OK. The alert is just an alert. This is the most common case of the failure.

t8m · 2025-05-12T15:26:19Z

There is also #25781 which is related to this PR.

Sashan

looks good to me

openssl-machine · 2025-05-14T08:00:24Z

This pull request is ready to merge

jogme · 2025-05-15T08:00:13Z

There is also #25781 which is related to this PR.

~~I updated the PR with the new commit partly (the key share part) resolving issue #25781. PTAL~~

moved to the PR #27627

Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #27597)

Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #27597) (cherry picked from commit 5da4ea1)

t8m · 2025-05-15T08:50:31Z

Merged to all the active branches. Thank you.

Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#27597)

Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#27597) (cherry picked from commit 5da4ea1)

openssl-machine added the hold: cla required The contributor needs to submit a license agreement label May 11, 2025

github-actions Bot added the severity: fips change The pull request changes FIPS provider sources label May 11, 2025

jogme force-pushed the handle_bad_x_keyshare branch from 83d69a5 to d3a5580 Compare May 11, 2025 15:39

paulidale reviewed May 11, 2025

View reviewed changes

Comment thread ssl/s3_lib.c Outdated

jogme force-pushed the handle_bad_x_keyshare branch from d3a5580 to 2dbdc05 Compare May 12, 2025 07:06

openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label May 12, 2025

Sashan requested changes May 12, 2025

View reviewed changes

t8m added branch: master Applies to master branch approval: review pending This pull request needs review by a committer labels May 12, 2025

t8m requested changes May 12, 2025

View reviewed changes

t8m added triaged: bug The issue/pr is/fixes a bug tests: exempted The PR is exempt from requirements for testing labels May 12, 2025

jogme force-pushed the handle_bad_x_keyshare branch from 2dbdc05 to 56be945 Compare May 12, 2025 09:47

github-actions Bot removed the severity: fips change The pull request changes FIPS provider sources label May 12, 2025

jogme force-pushed the handle_bad_x_keyshare branch from 56be945 to 3228720 Compare May 12, 2025 11:13

t8m requested changes May 12, 2025

View reviewed changes

Comment thread ssl/s3_lib.c Outdated

Comment thread ssl/s3_lib.c Outdated

Comment thread ssl/s3_lib.c Outdated

jogme force-pushed the handle_bad_x_keyshare branch from 3228720 to 33fa9ac Compare May 12, 2025 14:32

t8m previously approved these changes May 12, 2025

View reviewed changes

t8m added branch: 3.0 Applies to openssl-3.0 branch branch: 3.2 Applies to openssl-3.2 (EOL) branch: 3.3 Applies to openssl-3.3 (EOL) branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 labels May 12, 2025

Sashan previously approved these changes May 13, 2025

View reviewed changes

Sashan 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 May 13, 2025

openssl-machine 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 May 14, 2025

s3_lib.c: Handle weak x keys as illegal_parameter alert

cc2c9e6

jogme dismissed stale reviews from Sashan and t8m via a8192c4 May 15, 2025 07:58

jogme force-pushed the handle_bad_x_keyshare branch from 33fa9ac to a8192c4 Compare May 15, 2025 07:58

jogme changed the title ~~ecx_keys: Handle weak x keys as insufficient_security alert~~ Handle weak x keys and mlkem failed encapsulation as illegal_parameter alert May 15, 2025

jogme force-pushed the handle_bad_x_keyshare branch from a8192c4 to cc2c9e6 Compare May 15, 2025 08:13

jogme changed the title ~~Handle weak x keys and mlkem failed encapsulation as illegal_parameter alert~~ ecx_keys: Handle weak x keys as insufficient_security alert May 15, 2025

openssl-machine pushed a commit that referenced this pull request May 15, 2025

s3_lib.c: Handle weak x keys as illegal_parameter alert

5da4ea1

Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #27597)

t8m closed this May 15, 2025

The-Mule pushed a commit to The-Mule/openssl that referenced this pull request May 16, 2025

s3_lib.c: Handle weak x keys as illegal_parameter alert

be76fb6

Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#27597)

Sashan pushed a commit to Sashan/openssl that referenced this pull request May 16, 2025

s3_lib.c: Handle weak x keys as illegal_parameter alert

f796f99

Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#27597)

nhorman mentioned this pull request May 19, 2025

internal_error instead of illegal_parameter for improper key share value #27531

Closed

DDvO pushed a commit to siemens/openssl that referenced this pull request Jun 16, 2025

s3_lib.c: Handle weak x keys as illegal_parameter alert

6f49d60

Reviewed-by: Saša Nedvědický <sashan@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#27597)

Uh oh!

Conversation

jogme commented May 11, 2025

Checklist

Uh oh!

Uh oh!

Sashan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jogme May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t8m commented May 12, 2025

Uh oh!

Sashan left a comment

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented May 14, 2025

Uh oh!

jogme commented May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t8m commented May 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jogme May 12, 2025 •

edited

Loading

jogme commented May 15, 2025 •

edited

Loading