Skip to content

Fix error handling in x509v3_cache_extensions and related functions#10755

Closed
bernd-edlinger wants to merge 11 commits intoopenssl:masterfrom
bernd-edlinger:fix_error_handling_x509_flags
Closed

Fix error handling in x509v3_cache_extensions and related functions#10755
bernd-edlinger wants to merge 11 commits intoopenssl:masterfrom
bernd-edlinger:fix_error_handling_x509_flags

Conversation

@bernd-edlinger
Copy link
Member

Basically we use EXFLAG_INVALID for all kinds of out of memory and
all kinds of parse errors in x509v3_cache_extensions.

[extended tests]

Checklist
  • documentation is added or updated
  • tests are added or updated

@bernd-edlinger bernd-edlinger added the branch: master Applies to master branch label Jan 4, 2020
@bernd-edlinger bernd-edlinger force-pushed the fix_error_handling_x509_flags branch from 28c5aea to c9f275f Compare January 4, 2020 15:07
Basically we use EXFLAG_INVALID for all kinds of out of memory and
all kinds of parse errors in x509v3_cache_extensions.

[extended tests]
Copy link
Contributor

@FdaSilvaYY FdaSilvaYY left a comment

Choose a reason for hiding this comment

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

EXFLAG_INVALID is not very well documented .
I suggest to add something like this :
This bit may also be raised after some internal cert-processing failures
So it may not be related to the processed object itself.

BASIC_CONSTRAINTS_free(bs);
x->ex_flags |= EXFLAG_BCONS;
}
else if (i != -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: on previous line.

name = X509_get_subject_name(x->x509);
X509_digest(x->x509, evpmd, digest, NULL);
if (!X509_digest(x->x509, evpmd, digest, NULL)) {
BIO_printf(bio_err, "out of memory\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is out of memory the only possible failure?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think, only on X509*_get_ext_d2i a syntax or OOM error can both be possible

PROXY_CERT_INFO_EXTENSION_free(pci);
x->ex_flags |= EXFLAG_PROXY;
}
else if (i != -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Style nit: on previous line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

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.

LGTM

@t8m t8m added the approval: done This pull request has the required number of approvals label Mar 20, 2020
@bernd-edlinger
Copy link
Member Author

Mind, lookin at #10756 as well?

@openssl-machine
Copy link
Collaborator

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.

openssl-machine pushed a commit that referenced this pull request Mar 21, 2020
Basically we use EXFLAG_INVALID for all kinds of out of memory and
all kinds of parse errors in x509v3_cache_extensions.

[extended tests]

Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org>
(Merged from #10755)
@bernd-edlinger
Copy link
Member Author

Merged to master 7e06a67
Thanks!

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

Labels

approval: done This pull request has the required number of approvals branch: master Applies to master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants