search

DMTF / libspdm

BSD 3-Clause "New" or "Revised" License
90 stars 95 forks source link

library/spdm_responder: Fixup set cert checks #2708

Closed alistair23 closed 1 month ago

alistair23 commented 1 month ago

When we run checks against the certificate that the requester set we have the following function calls

At which point libspdm_verify_leaf_cert_spdm_extension() checks to make sure the id-DMTF-hardware-identity OID is not set if it's an AliasCert model.

This ends up being incorrect though. If using an AliasCert the SET_CERTIFICATE CertChain (table 93 - section 770) will "contain a partial certificate chain from the root CA to the Device Certificate CA". This means that the leaf certificate of that chain should set the the id-DMTF-hardware-identity OID as it isn't an alias certificate.

At this point the check in libspdm_verify_leaf_cert_spdm_extension() is incorrect.

The documentation of libspdm_x509_set_cert_certificate_check() states that: is_requester_cert Is the function verifying requester or responder cert.

Although we are a responder, we are verifying a certificate set by the requester, so change the is_requester_cert to true to avoid the incorrect id-DMTF-hardware-identity OID check and match the documentation.

alistair23 commented 1 month ago

The CI failure seems related to being unable to clone a repo and not due to this commit

steven-bellock commented 1 month ago

Going even further, since only a Responder can be SET_CERTIFICATEd then the is_requester_cert parameter is currently not needed at all in libspdm_x509_set_cert_certificate_check.

Looking at the two functions that use is_requester_cert

In this case (Responder, SET_CERTIFICATE, ALIAS_CERT_CAP=1, leaf certificate is the device certificate CA) it sounds like the verification functions need to know whether the leaf certificate is the device certificate, the device certificate CA, or the alias certificate. We currently have the cert_model field but that's not the same thing. @Wenxing-hou and @jyao1 can comment.

Wenxing-hou commented 1 month ago

I am checking this issue.

alistair23 commented 1 month ago

In this case (Responder, SET_CERTIFICATE, ALIAS_CERT_CAP=1, leaf certificate is the device certificate CA) it sounds like the verification functions need to know whether the leaf certificate is the device certificate, the device certificate CA, or the alias certificate. We currently have the cert_model field but that's not the same thing. @Wenxing-hou and @jyao1 can comment.

That seems reasonable to me. I'm happy to implement that if everyone agrees

Wenxing-hou commented 1 month ago

For this PR, I have different opinion. Set_cert command is requester set cert to responder. The cert will be wirted in responder. The cert is responder cert. And the is_requester_cert should be false.

jyao1 commented 1 month ago

Thank you @alistair23. Yes, this is a problem. Here is my thought:

1) is_requester_cert is to indicate if the cert if for requester or responder. In this case, it is for responder. As such, it should be FALSE. We should NOT change it to TRUE.

2) libspdm_x509_set_cert_certificate_check is to perform the check for SET_CERT function specifically. As such, it should understand if the responder device is in alias cert mode or device cert mode. Even if the responder is in alias cert mode, this function should treat the last certificate to be device CA cert, but NOT the real leaf cert. That is the key difference between GET_CERT and SET_CERT.

3) libspdm_x509_common_certificate_check is invoked by the libspdm_x509_set_cert_certificate_check and libspdm_x509_set_cert_certificate_check_ex. However, the GET_CERT or SET_CERT info is lost in the API call. I think that is the problem.

In conclusion, I recommend we change libspdm_x509_common_certificate_check to add GET_CERT or SET_CERT as parameter. If SET_CERT is used and the device is in alias cert mode, then it checks that the last cert is device CA cert, but not leaf cert.

alistair23 commented 1 month ago

Ok, I have created https://github.com/DMTF/libspdm/pull/2721