-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add verify cert callback function for DiceTcbInfo #2672
Conversation
3cef1fd
to
488a782
Compare
void *spdm_root_cert_for_dicetcbinfo; | ||
size_t spdm_root_cert_size_for_dicetcbinfo; | ||
|
||
spdm_context = (void *)malloc(libspdm_get_context_size()); |
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.
The code should handle cases where malloc fails and returns NULL.
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.
Thanks. I have add the code to handle the failed case.
/*vendorInfo*/ | ||
result = libspdm_asn1_get_tag(&ptr, end, &obj_len, | ||
LIBSPDM_CRYPTO_ASN1_CONTEXT_SPECIFIC + 8); | ||
if (result) { |
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.
Why there is no check for vendorInfo and flags?
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.
Because there is no vendorInfo and flags reference. The check will skip.
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.
What if the verifier would like to check vendorInfo and/or flags against references?
Because there is no vendorInfo and flags reference. The check will skip.
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.
Please note, this is only reference implementation.
In a real production, the integrator can decide to add more fields to check, or skip some fields.
int32_t length; | ||
size_t obj_len; | ||
uint8_t *end; | ||
|
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.
General: we can't assume that the adopter code will provide reference values for all TCBinfo components hardcoded above (m_libspdm_dice_tcbinfo_xyz). If a reference is not provided, this function should skip mem_equal() for that component, even if the component is present in the TcbInfo extension.
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.
Yes. The code assume only follow reference will be provided. And the other components will be skip even if existing in cert.
/*vendor: INTC*/
/*model: S3M GNR*/
/*version: 000200000000008B*/
/*svn*/
/*layer*/
/*fwids*/
/*type*/
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 was saying that the code should not require hardcoded references of these components (vendor, model, version, etc.). For example, the verifier can choose to hardcode reference for only "version" because the verifier only cares about version.
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.
Please note, this test is only reference implementation, to demonstrate how to do the alias cert verification.
The integrator should decide what to do - input as parameter, or hardcode somewhere.
LIBSPDM_CRYPTO_ASN1_CONSTRUCTED); | ||
if (result) { | ||
/*verify Dice TCB info*/ | ||
result = libspdm_verify_cert_dicetcbinfo(tem_ptr, obj_len + (ptr - tem_ptr)); |
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.
The "m_libspdm_must_have_dice_tcb_info" is a global that applies to all certs in the chain, but TcbInfo is usually present and required for some certs (e.g., Alias cert).
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.
Please note, this is reference implementation. We cannot handle all difference use cases.
The integrator should choose what to do in the final production, based on their own use case.
488a782
to
7ab3f43
Compare
@xiaoyuruan, I would like to provide a general comment: This is only reference implementation to demonstrate how to do the alias cert check. If you have one specific use case to cover, please describe it clearly. We can cover that. That is not a problem. However, it is impossible to cover all use cases, which is NOT the goal of this PR. |
OK that's fine. Please document all assumptions of the implementation (maybe in the readme file?), so reviewers and verifier integrators have a proper expectation.
|
89ce60a
to
17ab69d
Compare
@Wenxing-hou @jyao1 please file a separate issue for OpenSSL's validation of certificates with critical OIDs. |
Yes. That is good suggestion. We should add readme to document that. I think current assumption is:
@Wenxing-hou , please double check if above assumption is satisfied. |
9c56e14
to
642defe
Compare
Hi Jiewen. I have checked the code, the code logic match the implementation assumption. libspdm/unit_test/test_spdm_callback/spdm_cert_verify_callback.c Lines 249 to 277 in 642defe
And I have added the Readme for the Callback function. |
3b21c2f
to
c1c1faa
Compare
LIBSPDM_CRYPTO_ASN1_CONSTRUCTED); | ||
if (result) { | ||
/*verify Dice TCB info*/ | ||
result = libspdm_verify_cert_dicetcbinfo(tem_ptr, obj_len + (ptr - tem_ptr), |
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.
If libspdm_verify_cert_dicetcbinfo returns FALSE and cert_dice_tcb_info_size!=0, tracing this function further, I think there is an error ("store buffer too small"- not sure what it meant). Is it necessary to handle the error here?
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.
Thanks.
Now libspdm_verify_cert_dicetcbinfo
hard code the DiceTcbInfo buffer size, and the code categorizes the buffer error to invalid error.
libspdm/unit_test/test_spdm_callback/spdm_cert_verify_callback.c
Lines 56 to 66 in c1c1faa
spdm_dice_tcb_info_size = 256; | |
*spdm_get_dice_tcb_info_size = 0; | |
result = libspdm_x509_get_extension_data(cert, cert_size, | |
m_libspdm_tcg_dice_tcbinfo_oid, | |
sizeof(m_libspdm_tcg_dice_tcbinfo_oid), | |
spdm_dice_tcb_info, &spdm_dice_tcb_info_size); | |
if (!result) { | |
return false; | |
} else if (spdm_dice_tcb_info_size == 0) { | |
return true; | |
} |
I don't think it is necessary to handle this issue.
2) **Reported TcbInfo** | ||
|
||
- Reported TcbInfo must be in at least one certificate. | ||
- If none of the certificates includes TcbInfo, the verification must fail. | ||
3) **TcbInfo Matching** | ||
|
||
- At least one reported TcbInfo must fully match the reference TcbInfo. | ||
- If none of the reported TcbInfos matches all fields in the reference TcbInfo, the verification must fail. | ||
- Once one of the reported TcbInfo fully matches, the verification must pass and the rests of reported TcbInfo must be ignored. |
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.
Please change to below:
2) Reported TcbInfo
* The number of reported TcbInfo must match the number of reference TcbInfo.
3) TcbInfo Matching
* Each of the reported TcbInfo must fully match each of the reference TcbInfo with same order.
c1c1faa
to
234ecf4
Compare
} else { | ||
if (cert_dice_tcb_info_size != 0) { | ||
cert_chain_have_matched_dice = true; | ||
number_dice_tcb_info++; |
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.
This is NOT correct.
The rule is to make sure the TcbInfo number matches, not the right TcbInfo number matches.
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.
Thanks.
I have updated the logic: change the number when the cert have DiceTcbInfo.
libspdm/unit_test/test_spdm_callback/spdm_cert_verify_callback.c
Lines 264 to 274 in 20640c8
if (!result) { | |
if (cert_dice_tcb_info_size == 0) { | |
return false; | |
} | |
number_dice_tcb_info++; | |
} else { | |
if (cert_dice_tcb_info_size != 0) { | |
cert_chain_have_matched_dice = true; | |
number_dice_tcb_info++; | |
} | |
} |
20640c8
to
c9312da
Compare
please resolve conflict. |
I'll provide my comments tomorrow. Was busy today. |
c9312da
to
7d13678
Compare
Thanks. I have fixed the conflict. |
Thanks for your support. |
@Wenxing-hou we have |
Also if we pass sample cert chain from libspdm unit test. Will this function verify that ? |
So I think there are three separate issues / pull requests here.
|
Yes, the Thanks. |
Please refer to the Readme.md: Thanks. |
Thanks for your suggestion. I will do the corresponding work based on your feedback. |
Let's make sure @jyao1 approves first. |
Agree. Please file a new issue.
Agree. Please file a new issue.
Agree. Please move this to os_stub/spdm_cert_verify_callback_sample. Please only do this in this PR. |
7d13678
to
be35cca
Compare
For the first feedback, I have submitted the issue: #2695 |
* License: BSD 3-Clause License. For full text see link: https://github.com/DMTF/libspdm/blob/main/LICENSE.md | ||
**/ | ||
|
||
#include <stdarg.h> |
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.
Trim the headers to those that are actually used.
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.
Thanks. I have deleted the unused header.
* License: BSD 3-Clause License. For full text see link: https://github.com/DMTF/libspdm/blob/main/LICENSE.md | ||
**/ | ||
|
||
#ifndef __SPDM_CERT_VERIFY_CALLBACK_INTERNAL_H__ |
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.
__SPDM_CERT_VERIFY_CALLBACK_INTERNAL_H__
-> SPDM_CERT_VERIFY_CALLBACK_INTERNAL_H
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.
Thanks. I have fixed the MACRO.
|
||
#include "library/spdm_crypt_lib.h" | ||
#include "spdm_crypt_ext_lib/spdm_crypt_ext_lib.h" | ||
#include "spdm_crypt_ext_lib/cryptlib_ext.h" |
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.
Trim the headers to those that are actually used.
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.
Thanks. I have deleted the unused header.
@@ -0,0 +1,37 @@ | |||
## This is reference implementation to verify CertChain DiceTcbInfo extension. |
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.
This file should be in os_stub/spdm_cert_verify_callback_sample
.
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.
Thanks. I have moved the README to os_stub.
@@ -0,0 +1,140 @@ | |||
/** |
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.
This directory should be called unit_test/test_spdm_sample
.
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.
Thanks. I have changed the folder name.
be35cca
to
f40a9d7
Compare
free(file_data); | ||
} | ||
|
||
int libspdm_crypt_lib_setup(void **state) |
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.
All these instances of *_crypt_lib_*
need to be renamed to *_spdm_sample_*
.
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.
Thanks. I have changed the name.
Refer the spec https://trustedcomputinggroup.org/resource/dice-attestation-architecture/ add verify cert callback function for Cert extentison DiceTcbInfo check. Signed-off-by: Wenxing Hou <[email protected]>
f40a9d7
to
e08a2d2
Compare
Refer the spec: https://trustedcomputinggroup.org/wp-content/uploads/DICE-Attestation-Architecture-Version-1.1-Revision-18_pub.pdf ,
add verify cert callback function for cert DiceTcbInfo extension check.