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_print fixes #19963

Closed
wants to merge 3 commits into from
Closed

X509_print fixes #19963

wants to merge 3 commits into from

Conversation

dhobsong
Copy link
Contributor

Some minor fixes to X509_print_ex() and X509_REQ_print_ex() around the handling of nmflags and use of X509_NAME_print_ex().

@dhobsong
x509_print_ex:Use correct constant for nmflag comparison
3755bac 
The X509_FLAG_COMPAT constant is defined as a value of the
X509_print_ex() cflags argument, and so it should not be used
to compare against values for use with X509_NAME_print flags.
Use XN_FLAG_COMPAT, which has the same value, instead.
@dhobsong
Fix X509_REQ_print_ex bug
ca9a32a 
Similar to the bug fixed in 02db735 (Fix bug in X509_print_ex).
The error return value from X509_NAME_print_ex() is different
depending on whether the flags are XN_FLAG_COMPAT or not.
Apply a similar fix to what was done for X509_print_ex here as well.
@dhobsong
x509_print_ex: Remove unused setting when XN_FLAG_COMPAT is set
baf6089 
Calling X509_NAME_print_ex with XN_FLAG_COMPAT falls back to calling
X509_NAME_print().  The obase parameter to X509_NAME_print() is not
used, so setting it to a different value has no effect.
@dhobsong dhobsong changed the title X509 print fixes X509_print fixes Dec 23, 2022
@t8m t8m added branch: master Merge to master branch approval: review pending This pull request needs review by a committer 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 branch: 3.1 Merge to openssl-3.1 hold: needs tests The PR needs tests to be added to it labels Dec 27, 2022
@t8m
Copy link
Member

t8m commented Dec 27, 2022

Would it be possible to add a testcase? Or is it too big effort in comparison to the fix itself?

@dhobsong
Copy link
Contributor Author

Would it be possible to add a testcase? Or is it too big effort in comparison to the fix itself?

Might be a bit much, but I guess that a test for the second patch (Fix X509_REQ_print_ex bug) could be added to test that trying to print an X509_REQ with an invalid name doesn't silently discard the failure. The other two are basically no-ops post compilation.

@dhobsong
Copy link
Contributor Author

dhobsong commented Jan 9, 2023

@t8m So, after looking more closely at the implementation of X509_NAME_print, it looks like the only ways to get it to fail is to either have:

  1. A name longer than X509_NAME_MAX (which should be impossible, since the name length is checked when it is copied into the X509_REQ object) or
  2. A memory allocation failure

I took a quick look at the test framework, but I didn't notice any way to easily simulate malloc failures. Is there some standardized wrapper provided for doing this? If not, I don't think that there is an easy way to write a test that verifies the problem and shows that the 2nd commit in this series fixes it.

I'm fine with dropping the "Fix X509_REQ_print_ex bug" commit from the series, if its unfeasible without a testcase. The function has been in use as is for a long time without complaint, and only affects the operation when the unrecommended XN_FLAG_COMPAT flag is used.

@t8m t8m added tests: exempted The PR is exempt from requirements for testing and removed hold: needs tests The PR needs tests to be added to it labels Jan 10, 2023
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jan 10, 2023
tmshort
tmshort requested changes Jan 13, 2023
View reviewed changes
crypto/x509/t_req.c Show resolved Hide resolved
crypto/x509/t_x509.c Show resolved Hide resolved
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

@dhobsong dhobsong requested a review from tmshort March 23, 2023 20:20
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 92 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 123 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 154 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 185 days ago

@tmshort tmshort removed the approval: review pending This pull request needs review by a committer label Oct 22, 2023
@tmshort tmshort added the approval: done This pull request has the required number of approvals label Oct 22, 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 Oct 23, 2023
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@hlandau
Copy link
Member

hlandau commented Oct 26, 2023

This may need a rebase.

@t8m t8m added the branch: 3.2 Merge to openssl-3.2 label Oct 26, 2023
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
x509_print_ex:Use correct constant for nmflag comparison
d7f20c7 
The X509_FLAG_COMPAT constant is defined as a value of the
X509_print_ex() cflags argument, and so it should not be used
to compare against values for use with X509_NAME_print flags.
Use XN_FLAG_COMPAT, which has the same value, instead.

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

(cherry picked from commit da2dd3b)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
Fix X509_REQ_print_ex bug
e176569 
Similar to the bug fixed in 02db735 (Fix bug in X509_print_ex).
The error return value from X509_NAME_print_ex() is different
depending on whether the flags are XN_FLAG_COMPAT or not.
Apply a similar fix to what was done for X509_print_ex here as well.

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

(cherry picked from commit 2b5e028)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
x509_print_ex: Remove unused setting when XN_FLAG_COMPAT is set
6c6e9d4 
Calling X509_NAME_print_ex with XN_FLAG_COMPAT falls back to calling
X509_NAME_print().  The obase parameter to X509_NAME_print() is not
used, so setting it to a different value has no effect.

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

(cherry picked from commit 2126ca3)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
x509_print_ex:Use correct constant for nmflag comparison
da2dd3b 
The X509_FLAG_COMPAT constant is defined as a value of the
X509_print_ex() cflags argument, and so it should not be used
to compare against values for use with X509_NAME_print flags.
Use XN_FLAG_COMPAT, which has the same value, instead.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #19963)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
Fix X509_REQ_print_ex bug
2b5e028 
Similar to the bug fixed in 02db735 (Fix bug in X509_print_ex).
The error return value from X509_NAME_print_ex() is different
depending on whether the flags are XN_FLAG_COMPAT or not.
Apply a similar fix to what was done for X509_print_ex here as well.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Todd Short <todd.short@me.com>
(Merged from #19963)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
x509_print_ex: Remove unused setting when XN_FLAG_COMPAT is set
2126ca3 
Calling X509_NAME_print_ex with XN_FLAG_COMPAT falls back to calling
X509_NAME_print().  The obase parameter to X509_NAME_print() is not
used, so setting it to a different value has no effect.

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

Pushed to master/3.2/3.1/3.0. Thanks.

openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
x509_print_ex:Use correct constant for nmflag comparison
af3140e 
The X509_FLAG_COMPAT constant is defined as a value of the
X509_print_ex() cflags argument, and so it should not be used
to compare against values for use with X509_NAME_print flags.
Use XN_FLAG_COMPAT, which has the same value, instead.

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

(cherry picked from commit da2dd3b)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
Fix X509_REQ_print_ex bug
aa1a5ea 
Similar to the bug fixed in 02db735 (Fix bug in X509_print_ex).
The error return value from X509_NAME_print_ex() is different
depending on whether the flags are XN_FLAG_COMPAT or not.
Apply a similar fix to what was done for X509_print_ex here as well.

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

(cherry picked from commit 2b5e028)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
x509_print_ex: Remove unused setting when XN_FLAG_COMPAT is set
69bf1aa 
Calling X509_NAME_print_ex with XN_FLAG_COMPAT falls back to calling
X509_NAME_print().  The obase parameter to X509_NAME_print() is not
used, so setting it to a different value has no effect.

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

(cherry picked from commit 2126ca3)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
x509_print_ex:Use correct constant for nmflag comparison
0e3872a 
The X509_FLAG_COMPAT constant is defined as a value of the
X509_print_ex() cflags argument, and so it should not be used
to compare against values for use with X509_NAME_print flags.
Use XN_FLAG_COMPAT, which has the same value, instead.

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

(cherry picked from commit da2dd3b)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
Fix X509_REQ_print_ex bug
b01e6bc 
Similar to the bug fixed in 02db735 (Fix bug in X509_print_ex).
The error return value from X509_NAME_print_ex() is different
depending on whether the flags are XN_FLAG_COMPAT or not.
Apply a similar fix to what was done for X509_print_ex here as well.

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

(cherry picked from commit 2b5e028)
openssl-machine pushed a commit that referenced this pull request Oct 26, 2023
@dhobsong @mattcaswell
x509_print_ex: Remove unused setting when XN_FLAG_COMPAT is set
436c75b 
Calling X509_NAME_print_ex with XN_FLAG_COMPAT falls back to calling
X509_NAME_print().  The obase parameter to X509_NAME_print() is not
used, so setting it to a different value has no effect.

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

(cherry picked from commit 2126ca3)
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
@dhobsong
x509_print_ex:Use correct constant for nmflag comparison
d4c7c6d 
The X509_FLAG_COMPAT constant is defined as a value of the
X509_print_ex() cflags argument, and so it should not be used
to compare against values for use with X509_NAME_print flags.
Use XN_FLAG_COMPAT, which has the same value, instead.

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

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
@dhobsong
Fix X509_REQ_print_ex bug
9ea5fb9 
Similar to the bug fixed in 02db735 (Fix bug in X509_print_ex).
The error return value from X509_NAME_print_ex() is different
depending on whether the flags are XN_FLAG_COMPAT or not.
Apply a similar fix to what was done for X509_print_ex here as well.

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

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
@dhobsong
x509_print_ex: Remove unused setting when XN_FLAG_COMPAT is set
e1890fe 
Calling X509_NAME_print_ex with XN_FLAG_COMPAT falls back to calling
X509_NAME_print().  The obase parameter to X509_NAME_print() is not
used, so setting it to a different value has no effect.

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

Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
@dhobsong
x509_print_ex:Use correct constant for nmflag comparison
20de7fd 
The X509_FLAG_COMPAT constant is defined as a value of the
X509_print_ex() cflags argument, and so it should not be used
to compare against values for use with X509_NAME_print flags.
Use XN_FLAG_COMPAT, which has the same value, instead.

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

(cherry picked from commit da2dd3b51ddd69aae0fd840c0d23afa954c24ded)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
@dhobsong
Fix X509_REQ_print_ex bug
e18883f 
Similar to the bug fixed in 02db735 (Fix bug in X509_print_ex).
The error return value from X509_NAME_print_ex() is different
depending on whether the flags are XN_FLAG_COMPAT or not.
Apply a similar fix to what was done for X509_print_ex here as well.

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

(cherry picked from commit 2b5e028a2f70de216458a5140bcf4ec3d9236eeb)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 4, 2023
@dhobsong
x509_print_ex: Remove unused setting when XN_FLAG_COMPAT is set
dc7591b 
Calling X509_NAME_print_ex with XN_FLAG_COMPAT falls back to calling
X509_NAME_print().  The obase parameter to X509_NAME_print() is not
used, so setting it to a different value has no effect.

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

(cherry picked from commit 2126ca3dba3907f49b232442c06db1cae8bee0c3)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 7, 2023
@dhobsong
x509_print_ex:Use correct constant for nmflag comparison
fec9215 
The X509_FLAG_COMPAT constant is defined as a value of the
X509_print_ex() cflags argument, and so it should not be used
to compare against values for use with X509_NAME_print flags.
Use XN_FLAG_COMPAT, which has the same value, instead.

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

(cherry picked from commit da2dd3b51ddd69aae0fd840c0d23afa954c24ded)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 7, 2023
@dhobsong
Fix X509_REQ_print_ex bug
a7a2ea2 
Similar to the bug fixed in 02db735 (Fix bug in X509_print_ex).
The error return value from X509_NAME_print_ex() is different
depending on whether the flags are XN_FLAG_COMPAT or not.
Apply a similar fix to what was done for X509_print_ex here as well.

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

(cherry picked from commit 2b5e028a2f70de216458a5140bcf4ec3d9236eeb)
Signed-off-by: fly2x <fly2x@hitls.org>
wanghao75 pushed a commit to openeuler-mirror/openssl that referenced this pull request Nov 7, 2023
@dhobsong
x509_print_ex: Remove unused setting when XN_FLAG_COMPAT is set
3b51aac 
Calling X509_NAME_print_ex with XN_FLAG_COMPAT falls back to calling
X509_NAME_print().  The obase parameter to X509_NAME_print() is not
used, so setting it to a different value has no effect.

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

(cherry picked from commit 2126ca3dba3907f49b232442c06db1cae8bee0c3)
Signed-off-by: fly2x <fly2x@hitls.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@tmshort tmshort tmshort 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 branch: 3.2 Merge to openssl-3.2 tests: exempted The PR is exempt from requirements for testing 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
6 participants
@dhobsong @t8m @openssl-machine @hlandau @mattcaswell @tmshort