Fix unsound OCSP find_status handling of optional next_update field #2517
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
Author
|
I don't understand the warning in the MSRV builder. We have an |
alex
force-pushed
the
claude/fix-ocsp-find-status-011CUqcGFNKeKJitnywzYCna
branch
from
6aa5292 to
4eafa15
Compare
botovq
reviewed
Nov 6, 2025
Contributor
botovq
left a comment
botovq
left a comment
There was a problem hiding this comment.
The code itself reads fine modulo one thing that could be better if we didn't support ancient library versions.
(I really do not like Claude's overly repetitive and verbose commenting style.)
Collaborator
Author
|
You should have seen how bad the comments were before I trimmed them down 🙃. (I'm ultimately accountable for anything in this PR, so really it's better that I think of it as my comments are bad.) |
alex
force-pushed
the
claude/fix-ocsp-find-status-011CUqcGFNKeKJitnywzYCna
branch
from
4eafa15 to
bc3c83f
Compare
The OCSP find_status function was unsound because it treated next_update as always present, even though it's optional per RFC 6960. When absent, the null pointer from FFI was passed to from_ptr which doesn't check for null, causing undefined behavior. Fixed by using from_const_ptr_opt to safely handle the null case. Added next_update() method returning Option<&Asn1GeneralizedTimeRef> and deprecated the existing field which now contains a sentinel max time value (99991231235959Z) for backwards compatibility. Fixes rust-openssl#2516
alex
force-pushed
the
claude/fix-ocsp-find-status-011CUqcGFNKeKJitnywzYCna
branch
from
bc3c83f to
cc26867
Compare
botovq
approved these changes
Nov 6, 2025
Contributor
botovq
left a comment
botovq
left a comment
There was a problem hiding this comment.
Looks much better to me. One suggestion - feel free to ignore.
Collaborator
Author
|
I'll plan to do a release with this fix tomorrow. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The OCSP find_status function was unsound because it treated next_update as always present, even though it's optional per RFC 6960. When absent, the null pointer from FFI was passed to from_ptr which doesn't check for null, causing undefined behavior.
Fixed by using from_const_ptr_opt to safely handle the null case. Added next_update() method returning Option<&Asn1GeneralizedTimeRef> and deprecated the existing field which now contains a sentinel max time value (99991231235959Z) for backwards compatibility.
Fixes #2516