Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

X509_NAME_cmp fix for empty name #21155

Closed
wants to merge 1 commit into from
Closed

X509_NAME_cmp fix for empty name #21155

wants to merge 1 commit into from

Conversation

wimdec
Copy link
Contributor

@wimdec wimdec commented Jun 8, 2023 •
edited

The leaf certificate in our certificate chain currently has an empty subject.
This was verifying fine always until we updated alpine linux to version with OpenSSL 3.0.8 (from 1.1.1q).
It is also still validating fine on debian linux with OpenSSL 3.0.8.

Root cause analysis indicated that following commit caused the issue: 72ded6f

Reason is the empty subject is now always seen as an issue and returns -2. In both cases when empty subject is a or b. Which of course invalidates the compare invariant: "" < "anything" and "anything" < "".

We have a certificate chain that exactly triggers issue on alpine linux qsort that the correct certificate is not found anymore in verification.

Checklist
  • tests are added or updated

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jun 8, 2023
@wimdec wimdec mentioned this pull request Jun 8, 2023
Closed
t8m
t8m approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

This would be acceptable with CLA: trivial. Could you either amend the commit message (git commit --amend) to add CLA: trivial on a separate line in the commit message body? You can also add Fixes #21156 on another separate line.

Or, alternatively, you can submit a CLA according to https://www.openssl.org/policies/cla.html

@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer triaged: bug The issue/pr is/fixes a bug branch: 3.0 Merge to openssl-3.0 branch severity: regression The issue/pr is a regression from previous released version branch: 3.1 Merge to openssl-3.1 tests: deferred Tests will be added in a subsequent PR (label should be removed when the PR with tests is merged) labels Jun 8, 2023
@t8m
Copy link
Member

t8m commented Jun 8, 2023

It would be also nice to have a test case but that would make the PR outside of things acceptable with CLA: trivial.

@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jun 8, 2023
@wimdec
Copy link
Contributor Author

wimdec commented Jun 8, 2023

Added the CLA and Fixes to commit message. Also in process to submit CLA, but that can take time for corporate approvals.

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jun 9, 2023
tmshort
tmshort approved these changes Jun 11, 2023
View reviewed changes
crypto/x509/x509_cmp.c
return -2;

if (ret == 0)
if (ret == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could go a bit further by combining with the prior if, as both have ret == 0.

tmshort
tmshort reviewed Jun 11, 2023
View reviewed changes
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.

Just a nit. Does not block approval.

@tmshort tmshort 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 Jun 11, 2023
@openssl-machine 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 Jun 12, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@t8m t8m added the cla: trivial One of the commits is marked as 'CLA: trivial' label Jun 12, 2023
@t8m
Copy link
Member

t8m commented Jun 12, 2023

@tmshort Are you OK with CLA: trivial?

@tmshort
Copy link
Contributor

tmshort commented Jun 12, 2023

@t8m, if the nit I mentioned were fixed, then no; but as-is, I'm ok with (cla:trivial).

@t8m
Copy link
Member

t8m commented Jun 12, 2023

Merged to master, 3.1, and 3.0 branches. Thank you for your contribution.

@t8m t8m closed this Jun 12, 2023
openssl-machine pushed a commit that referenced this pull request Jun 12, 2023
@wimdec @t8m
X509_NAME_cmp fix for empty name
ef2a438 
CLA: trivial

Fixes #21156

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21155)

(cherry picked from commit ec59752)
openssl-machine pushed a commit that referenced this pull request Jun 12, 2023
@wimdec @t8m
X509_NAME_cmp fix for empty name
ec59752 
CLA: trivial

Fixes #21156

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21155)
openssl-machine pushed a commit that referenced this pull request Jun 12, 2023
@wimdec @t8m
X509_NAME_cmp fix for empty name
1cfc919 
CLA: trivial

Fixes #21156

Reviewed-by: Todd Short <todd.short@me.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #21155)

(cherry picked from commit ec59752)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmshort tmshort tmshort approved these changes

@t8m t8m t8m approved these changes

Assignees
No one assigned
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch branch: 3.1 Merge to openssl-3.1 cla: trivial One of the commits is marked as 'CLA: trivial' severity: fips change The pull request changes FIPS provider sources severity: regression The issue/pr is a regression from previous released version tests: deferred Tests will be added in a subsequent PR (label should be removed when the PR with tests is merged) triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@wimdec @t8m @openssl-machine @tmshort