Skip to content

Comments

crypto/ml_dsa: fix public_from_private() error path to return failure#28576

Closed
rodeka wants to merge 2 commits intoopenssl:masterfrom
rodeka:fix-ml-dsa-public-from-private
Closed

crypto/ml_dsa: fix public_from_private() error path to return failure#28576
rodeka wants to merge 2 commits intoopenssl:masterfrom
rodeka:fix-ml-dsa-public-from-private

Conversation

@rodeka
Copy link
Contributor

@rodeka rodeka commented Sep 16, 2025

The error label returned success (1) even on failure. Make it return failure (0) instead. Fixes #28562

I audited all references to public_from_private() in the tree. The function is internal. All call sites treat its return value as a boolean success (1) / failure (0) and propagate failures accordingly; none rely on the previous (buggy) success-on-error behavior. Therefore, no behavior changes or follow-up adjustments are required.

The error label returned success (1) even on failure. Make it return failure (0) instead.
Fixes openssl#28562

CLA: trivial
@t8m t8m added branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 labels Sep 16, 2025
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Sep 16, 2025
Set the ret variable to return as in other functions.

CLA: trivial
@rodeka
Copy link
Contributor Author

rodeka commented Sep 16, 2025

Set the ret variable to return as in other functions.

Copy link
Contributor

@tmshort tmshort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@paulidale paulidale added the approval: done This pull request has the required number of approvals label Sep 16, 2025
@rodeka rodeka requested a review from t8m September 17, 2025 05:33
@openssl-machine openssl-machine removed the approval: done This pull request has the required number of approvals label Sep 17, 2025
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@openssl-machine openssl-machine added the approval: ready to merge The 24 hour grace period has passed, ready to merge label Sep 17, 2025
@paulidale paulidale added the cla: trivial One of the commits is marked as 'CLA: trivial' label Sep 17, 2025
openssl-machine pushed a commit that referenced this pull request Sep 17, 2025
The error label returned success (1) even on failure. Make it return failure (0) instead.
Fixes #28562

CLA: trivial

Reviewed-by: Todd Short <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #28576)
@paulidale
Copy link
Contributor

Merged to 3.5, 3.6 and master. Thanks for the fix.

I assumed trivial was okay for @tmshort too.

@paulidale paulidale closed this Sep 17, 2025
openssl-machine pushed a commit that referenced this pull request Sep 17, 2025
The error label returned success (1) even on failure. Make it return failure (0) instead.
Fixes #28562

CLA: trivial

Reviewed-by: Todd Short <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #28576)

(cherry picked from commit 925e4fb)
openssl-machine pushed a commit that referenced this pull request Sep 17, 2025
The error label returned success (1) even on failure. Make it return failure (0) instead.
Fixes #28562

CLA: trivial

Reviewed-by: Todd Short <[email protected]>
Reviewed-by: Paul Dale <[email protected]>
(Merged from #28576)

(cherry picked from commit 925e4fb)
esyr added a commit to esyr/openssl that referenced this pull request Sep 30, 2025
3.5.4 CHANGES.md includes the following:
 * openssl#28098
 * openssl#28415
 * openssl#28504
 * openssl#28535
 * openssl#28569
 * openssl#28573
 * openssl#28576
 * openssl#28591
 * openssl#28603
 * openssl#28624
 * openssl#28642
 * openssl#28676

3.5.4 NEWS.md includes the following:
 * openssl#28603

Updated the changes and news in the previous branches.

Removed the attribution in NEWS.md incorrectly introduced in e551da6
"Update news and changes for the 3.5.3 release".

Release: Yes
Signed-off-by: Eugene Syromiatnikov <[email protected]>
esyr added a commit to esyr/openssl that referenced this pull request Sep 30, 2025
3.5.4 CHANGES.md includes the following:
 * openssl#28098
 * openssl#28415
 * openssl#28504
 * openssl#28535
 * openssl#28569
 * openssl#28573
 * openssl#28576
 * openssl#28591
 * openssl#28603
 * openssl#28624
 * openssl#28642
 * openssl#28676

3.5.4 NEWS.md includes the following:
 * openssl#28603

Updated the changes and news in the previous branches.

Removed the attribution in NEWS.md incorrectly introduced in e551da6
"Update news and changes for the 3.5.3 release".

Release: Yes
Signed-off-by: Eugene Syromiatnikov <[email protected]>
esyr added a commit to esyr/openssl that referenced this pull request Sep 30, 2025
3.5.4 CHANGES.md includes the following:
 * openssl#28098
 * openssl#28415
 * openssl#28504
 * openssl#28535
 * openssl#28569
 * openssl#28573
 * openssl#28576
 * openssl#28591
 * openssl#28603
 * openssl#28624
 * openssl#28642
 * openssl#28676

3.5.4 NEWS.md includes the following:
 * openssl#28603

Updated the changes and news in the previous branches.

Removed the attribution in NEWS.md incorrectly introduced in e551da6
"Update news and changes for the 3.5.3 release".

Release: Yes
Signed-off-by: Eugene Syromiatnikov <[email protected]>
esyr added a commit to esyr/openssl that referenced this pull request Sep 30, 2025
3.5.4 CHANGES.md includes the following:
 * openssl#28098
 * openssl#28415
 * openssl#28504
 * openssl#28535
 * openssl#28569
 * openssl#28573
 * openssl#28576
 * openssl#28591
 * openssl#28603
 * openssl#28624
 * openssl#28642
 * openssl#28676

3.5.4 NEWS.md includes the following:
 * openssl#28603

Updated the changes and news in the previous branches.

Removed the attribution in NEWS.md incorrectly introduced in e551da6
"Update news and changes for the 3.5.3 release".

Release: Yes
Signed-off-by: Eugene Syromiatnikov <[email protected]>
@jericson
Copy link
Member

I wrote about new contributions on the blog and this one got a mention, @rodeka .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Applies to master branch branch: 3.5 Applies to openssl-3.5 branch: 3.6 Applies to openssl-3.6 cla: trivial One of the commits is marked as 'CLA: trivial' severity: fips change The pull request changes FIPS provider sources triaged: bug The issue/pr is/fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The public_from_private function still returns 1 even when an error occurs in the previous steps

6 participants