Hi team,
I believe I've found a potential soundness issue in the find_status function in openssl/src/ocsp.rs that could lead to Undefined Behavior.
Here is the relevant code snippet:
#[corresponds(OCSP_resp_find_status)]
pub fn find_status<'a>(&'a self, id: &OcspCertIdRef) -> Option<OcspStatus<'a>> {
unsafe {
// ...
let mut revocation_time = ptr::null_mut();
let mut this_update = ptr::null_mut();
let mut next_update = ptr::null_mut();
let r = ffi::OCSP_resp_find_status(
self.as_ptr(),
id.as_ptr(),
&mut status,
&mut reason,
&mut revocation_time,
&mut this_update,
&mut next_update,
);
if r == 1 {
let revocation_time = Asn1GeneralizedTimeRef::from_const_ptr_opt(revocation_time);
Some(OcspStatus {
status: OcspCertStatus(status),
reason: OcspRevokedStatus(status),
revocation_time,
this_update: Asn1GeneralizedTimeRef::from_ptr(this_update), // Potential UB
next_update: Asn1GeneralizedTimeRef::from_ptr(next_update), // Potential UB
})
} else {
None
}
}
}
The documentaiton says:
An OCSP response for a certificate contains thisUpdate and nextUpdate fields. Normally the current time should be between these two values. To account for clock skew the maxsec field can be set to nonzero in OCSP_check_validity(). Some responders do not set the nextUpdate field, this would otherwise mean an ancient response would be considered valid: the maxsec parameter to OCSP_check_validity() can be used to limit the permitted age of responses.
So, this is the entire analysis.
-
Initialization: The this_update and next_update pointers are initialized to ptr::null_mut().
-
FFI Call: They are passed as out-parameters to the C function ffi::OCSP_resp_find_status, which may write pointers into them.
-
Unconditional Call: If the FFI call succeeds (r == 1), the wrapper unconditionally calls Asn1GeneralizedTimeRef::from_ptr on both this_update and next_update.
-
Contract Violation: The safety contract for Asn1GeneralizedTimeRef::from_ptr requires its argument to be a non-null, valid pointer. However, the OCSP standard (and the documentation provided) explicitly states that the nextUpdate field is optional.
-
Conclusion: It is possible for OCSP_resp_find_status to return 1 (success) while leaving next_update (and possibly this_update, though less likely) as the null pointer it was initialized with. Passing this null pointer to Asn1GeneralizedTimeRef::from_ptr violates its safety contract and results in Undefined Behavior.
Thanks for maintaining this library!
Hi team,
I believe I've found a potential soundness issue in the find_status function in openssl/src/ocsp.rs that could lead to Undefined Behavior.
Here is the relevant code snippet:
The documentaiton says:
So, this is the entire analysis.
Initialization: The this_update and next_update pointers are initialized to ptr::null_mut().
FFI Call: They are passed as out-parameters to the C function ffi::OCSP_resp_find_status, which may write pointers into them.
Unconditional Call: If the FFI call succeeds (r == 1), the wrapper unconditionally calls Asn1GeneralizedTimeRef::from_ptr on both this_update and next_update.
Contract Violation: The safety contract for Asn1GeneralizedTimeRef::from_ptr requires its argument to be a non-null, valid pointer. However, the OCSP standard (and the documentation provided) explicitly states that the nextUpdate field is optional.
Conclusion: It is possible for OCSP_resp_find_status to return 1 (success) while leaving next_update (and possibly this_update, though less likely) as the null pointer it was initialized with. Passing this null pointer to Asn1GeneralizedTimeRef::from_ptr violates its safety contract and results in Undefined Behavior.
Thanks for maintaining this library!