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.

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

OCSP update time validation seems incorrect #54

Closed
timukasr opened this issue Feb 16, 2024 · 1 comment · Fixed by #55
Closed

OCSP update time validation seems incorrect #54

timukasr opened this issue Feb 16, 2024 · 1 comment · Fixed by #55

Comments

@timukasr
Copy link

From the description, it seems that thisUpdate and nextUpdate should be checked agaisnt system local time, but the code checks it againts producetAt value from OCSP response, so no matter the system time, response is always valid:

public static void validateCertificateStatusUpdateTime(SingleResp certStatusResponse, Date producedAt) throws UserCertificateOCSPCheckFailedException {
// From RFC 2560, https://www.ietf.org/rfc/rfc2560.txt:
// 4.2.2. Notes on OCSP Responses
// 4.2.2.1. Time
// Responses whose nextUpdate value is earlier than
// the local system time value SHOULD be considered unreliable.
// Responses whose thisUpdate time is later than the local system time
// SHOULD be considered unreliable.
// If nextUpdate is not set, the responder is indicating that newer
// revocation information is available all the time.
final Date notAllowedBefore = new Date(producedAt.getTime() - ALLOWED_TIME_SKEW);
final Date notAllowedAfter = new Date(producedAt.getTime() + ALLOWED_TIME_SKEW);
final Date thisUpdate = certStatusResponse.getThisUpdate();
final Date nextUpdate = certStatusResponse.getNextUpdate() != null ? certStatusResponse.getNextUpdate() : thisUpdate;
if (notAllowedAfter.before(thisUpdate) ||
notAllowedBefore.after(nextUpdate)) {
throw new UserCertificateOCSPCheckFailedException("Certificate status update time check failed: " +
"notAllowedBefore: " + toUtcString(notAllowedBefore) +
", notAllowedAfter: " + toUtcString(notAllowedAfter) +
", thisUpdate: " + toUtcString(thisUpdate) +
", nextUpdate: " + toUtcString(certStatusResponse.getNextUpdate()));
}
}

@mrts
Copy link
Member

mrts commented Feb 16, 2024

Thank you for highlighting the issue! Yes, this is a mistake - the comparison should be made against the local system time, not the producedAt time from the OCSP response according to RFC 2560. This will be fixed with #55.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants