-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix possible infinite loop when loading cert chains from Java P11KeyStore #216
base: main
Are you sure you want to change the base?
Conversation
Hi, Thank you for the pull request. Would you be willing to add a test that exercises the use of this feature? |
pkcs11/src/backend/session.rs
Outdated
if let Some(kind) = requirements.kind { | ||
kind == obj.kind | ||
if (matches!(requirements.kind, Some(ObjectKind::Certificate)) | ||
&& kind == obj.kind | ||
&& requirements.subject.is_some()) { | ||
requirements.subject == obj.subject | ||
} else { | ||
kind == obj.kind | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this you don't need to coy the subject
in Object
if let Some(kind) = requirements.kind { | |
kind == obj.kind | |
if (matches!(requirements.kind, Some(ObjectKind::Certificate)) | |
&& kind == obj.kind | |
&& requirements.subject.is_some()) { | |
requirements.subject == obj.subject | |
} else { | |
kind == obj.kind | |
} | |
if let Some(kind) = requirements.kind { | |
kind == obj.kind | |
&& matches!(kind, ObjectKind::Certificate) | |
&& obj.attr(CKA_SUBJECT).map(|attr| attr.as_bytes()) | |
== requirements.subject.as_deref() | |
} else { | |
true | |
} | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do the following, so you can extend easily on further requirements support:
if let Some(kind) = requirements.kind {
// kind must match
if kind != obj.kind {
false
// extra checks if kind is Cerificate
} else if kind == ObjectKind::Certificate {
// When Subject is provided as requirement, it must match
requirements.cka_subject.is_none() ||
obj.attr(CKA_SUBJECT)
.map(|attr| attr.as_bytes())
== requirements.cka_subject.as_deref()
// On other kinds, no need for extra checks
} else {
true
}
} else {
true
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, please make it as simple as possible. Avoid extra complexity for future checks that may never exist.
…tore When HSM contains certificate chains, the JDK P11KeyStore tries to load the full chain within loadChain() method. This action is performed in a while(true) loop as: while (true) { CK_ATTRIBUTE[] attrs = new CK_ATTRIBUTE[] { ATTR_TOKEN_TRUE, ATTR_CLASS_CERT, new CK_ATTRIBUTE(CKA_SUBJECT, next.getIssuerX500Principal().getEncoded()) }; long[] ch = findObjects(session, attrs); if (ch == null || ch.length == 0) { // done break; } else { // Just take the first next = loadCert(session, ch[0]); lChain.add(next); if (next.getSubjectX500Principal().equals (next.getIssuerX500Principal())) { // self signed break; } } } Here, supporting filtering certificates by CKA_SUBJECT is crucial otherwise the while true loop would continue forever (until findObjects returns some certificates and first one is not self signed) Signed-off-by: Alberto Panizzo <[email protected]>
8b1ecd8
to
3269137
Compare
When HSM contains certificate chains, the JDK P11KeyStore tries to load the full chain within loadChain() method.
This action is performed in a while(true) loop as:
Here, supporting filtering certificates by CKA_SUBJECT is crucial otherwise the while true loop would continue forever (until findObjects returns some certificates and first one is not self signed)