Add SPDM extensions OID check #166

OuyangHang33 · 2023-12-11T04:27:46Z

Fix: #32
Add OID check for spdm extension:

Add SPDM extensions check after SubjectPublicKeyInfo in TBS cert.
Check if SPDM responder/requester auth OIDs appear in the leaf cert.

Find responder auth oid OID;
Find requester auth oid OID;
Check if these 2 OIDs appear in the leaf cert;

Check if Hardware Identity OID present in spdm extension:

Find the SPDM extensions OID;
Find Hardware/Mutable cert Identity OID;
Check if Hardware/Mutable Identity OID present in spdm extension;

Wenxing-hou · 2023-12-11T10:28:20Z

spdmlib/src/crypto/x509v3.rs

+    let mut requester_auth_oid_find_success: bool = false;
+    let mut spdm_extension_oid_find_success: bool = false;
+    let mut hardware_identity_oid_find_success: bool = false;
+    for index in 0..extensions_end_index_oid {


The check should be more detailed, because the scope of the extension is relatively large and there may be coincidences(the data is same as OID, but is not actual OID).

Thank you Wenxing, I will add OID TAG check after detect the target OID.

Wenxing-hou

The check logic is look good to me. The code detail need to be check by others.

longlongyang · 2023-12-20T23:50:20Z

spdmlib/src/crypto/x509v3.rs

+    //key_usage             EXTENSIONS,
+    let (find_key_usage, key_usage_value) = get_key_usage_value(&data[t_walker..])?;
+    let mut check_extensions_success =
+        !(find_key_usage && (LIBSPDM_CRYPTO_X509_KU_DIGITAL_SIGNATURE & key_usage_value == 0x00));


What does the key usage value 0x00 mean?
if key usage value has just several candidates, I would recommend use Enum for them.

Here only need to check key_usage value bit0-digitalSignature. If find the key usage value but get bit0 unset, then return false. It means subject public key in the cert SHOULD used for verifying digital signatures in an entity authentication service, a data origin authentication service, and/or an integrity service. Base on RFC5280 page 30-31:

The digitalSignature bit is asserted when the subject public key is used for verifying digital signatures, other than signatures on certificates (bit 5) and CRLs (bit 6), such as those used in an entity authentication service, a data origin authentication service, and/or an integrity service.

If the subject public key is only to be used for verifying signatures on certificates and/or CRLs, then the digitalSignature and nonRepudiation bits SHOULD NOT be set.

In libspdm, it doesn't check any other key_usage bits except bit0-digitalSignature. I follow the libspdm logic:
https://github.com/DMTF/libspdm/blob/3bbcb24dbae81a25cb04dd185b0fe948e6ac6b25/library/spdm_crypt_lib/libspdm_crypt_cert.c#L1082-L1093

Bit String Mapping:
Wenxing-hou/libspdm@f9e1dcf

Check only bit0-digitalSignature looks good to me now, one more suggestion:
define a macro for bit0-digitalSignature or add a comment besides it.

longlongyang · 2023-12-20T23:52:17Z

spdmlib/src/crypto/x509v3.rs

+fn check_extensions_spdm_oid(data: &[u8], is_leaf_cert: bool) -> bool {
+    let len = data.len();
+    let oid_len = 10;
+    let extensions_end_index_oid = len - oid_len;


if len < oid_len, this statement will introduce overflow panic

add the length check before let extensions_end_index_oid = len - oid_len;

looks good to me.

longlongyang · 2023-12-20T23:53:48Z

spdmlib/src/crypto/x509v3.rs

+    let mut responder_auth_oid_find_success: bool = false;
+    let mut requester_auth_oid_find_success: bool = false;
+    for index in 2..extensions_end_index_oid {
+        if object_identifiers_are_same(&data[index..index + oid_len], OID_DMTF_SPDM_DEVICE_INFO)


need to make sure index + oid_len is still a valid index.

add the lenght check before loop, and when execution this loop, data.len() is bigger than 12, and index + oid_len can be caculated as:

2+10 <= index + oid_len < extensions_end_index_oid +10 = data.len() ;

In another world, data[index..index + oid_len] belong to:

data[2..12]~ data[data.len()-10..data.len()]

OuyangHang33 · 2023-12-21T06:04:02Z

spdmlib/src/crypto/x509v3.rs

+    let mut hardware_identity_oid_find_success: bool = false;
+    let mut responder_auth_oid_find_success: bool = false;
+    let mut requester_auth_oid_find_success: bool = false;
+    if len < 2 + oid_len {


Add the length check before checking spdm oids via loop.
Because the head of spdm OID cost 2 bytes for descripte TYPE and LEN, so I use len < 2 + oid_len as a condition to determine whether it's possable to find the spdm OIDs in extensions.

looks good to me.

longlongyang · 2023-12-22T05:18:45Z

spdmlib/src/crypto/x509v3.rs

+    let mut hardware_identity_oid_find_success: bool = false;
+    let mut responder_auth_oid_find_success: bool = false;
+    let mut requester_auth_oid_find_success: bool = false;
+    if len < 2 + oid_len {


looks good to me.

longlongyang

Looks good to me

jyao1 · 2023-12-22T09:40:04Z

spdmlib/src/crypto/x509v3.rs

+        let extensions_end_index_oid = len - oid_len;
+        for index in 2..extensions_end_index_oid {
+            if object_identifiers_are_same(&data[index..index + oid_len], OID_DMTF_SPDM_DEVICE_INFO)
+                && (data[index - 2] == ASN1_TAG_OID)


It is quite unusual that use negative value as index - [index - 2].

It redundant to have data[index - 2] check in each if.

It is not efficient to move index by 1 in for, and also dangerous to mis-compare.

My expectation is:
Find the first tag, skip the tag-length and move to the next tag.
In case of anything wrong, stop the parsing and return error.

Thanks! I have changed the check_extensions_spdm_oid logic:
-Get extensions data from cert first
---Get the extnID of the first extension sequence, if not the target, then jump to next extension sequence
-----Search the target id-DMTF-spdm belong to this extension sequence
When didn't find the extension identify under the extension sequence or some length check fail, it will return error

RFC 5280 Page 17 extnID under extension sequence:

The id-DMTF-spdm should under the following extension sequence base on the SPDM spec, and I follow this to search the target id-DMTF-spdm OID to ensure they are in the right place:

jyao1 · 2023-12-22T09:40:36Z

spdmlib/src/crypto/x509v3.rs

+
+fn check_extensions_spdm_oid(data: &[u8], is_leaf_cert: bool) -> bool {
+    let len = data.len();
+    let oid_len = 10;


why oid_len is 10?

I have removed this line now. The new code based on the target SPDM-OID to check the oid_len automatically.

jyao1 · 2023-12-25T02:52:38Z

spdmlib/src/crypto/x509v3.rs

+        target_oid_find_success = false;
+    } else {
+        for index in 0..len - target_oid_len {
+            if data[index] == ASN1_TAG_OID {


please dont use 1 by 1 search.

jyao1 · 2023-12-25T02:54:20Z

spdmlib/src/crypto/x509v3.rs

+    } else {
+        let mut find_key_usage: bool = false;
+        for index in 0..len - key_usage_oid_len {
+            if object_identifiers_are_same(&data[index..index + key_usage_oid_len], OID_KEY_USAGE) {


please dont use 1 by 1 search.

Aleady clean all 1 by 1 search.

Signed-off-by: Ouyang, Hang <hang.ouyang@intel.com>

Wenxing-hou reviewed Dec 11, 2023

View reviewed changes

OuyangHang33 force-pushed the i32 branch from a5a1ae7 to 6ea2812 Compare December 12, 2023 06:25

Wenxing-hou approved these changes Dec 15, 2023

View reviewed changes

longlongyang suggested changes Dec 20, 2023

View reviewed changes

OuyangHang33 force-pushed the i32 branch from 6ea2812 to aebd4dc Compare December 21, 2023 05:52

OuyangHang33 commented Dec 21, 2023

View reviewed changes

OuyangHang33 requested a review from longlongyang December 21, 2023 06:23

longlongyang suggested changes Dec 22, 2023

View reviewed changes

OuyangHang33 force-pushed the i32 branch 5 times, most recently from db79341 to 9ee0be0 Compare December 22, 2023 07:15

longlongyang approved these changes Dec 22, 2023

View reviewed changes

jyao1 reviewed Dec 22, 2023

View reviewed changes

OuyangHang33 force-pushed the i32 branch 2 times, most recently from 749bd9f to 07ce026 Compare December 24, 2023 13:39

jyao1 reviewed Dec 25, 2023

View reviewed changes

OuyangHang33 force-pushed the i32 branch 2 times, most recently from f5e5a06 to 94f7f65 Compare December 25, 2023 05:49

Add SPDM extensions OID check

b56c54f

Signed-off-by: Ouyang, Hang <hang.ouyang@intel.com>

OuyangHang33 force-pushed the i32 branch from 94f7f65 to b56c54f Compare December 25, 2023 06:25

jyao1 merged commit a34daa3 into intel:main Jan 1, 2024
47 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SPDM extensions OID check #166

Add SPDM extensions OID check #166

OuyangHang33 commented Dec 11, 2023 •

edited

Loading

Wenxing-hou Dec 11, 2023

OuyangHang33 Dec 12, 2023

Wenxing-hou left a comment

longlongyang Dec 20, 2023

OuyangHang33 Dec 21, 2023 •

edited

Loading

longlongyang Dec 22, 2023

longlongyang Dec 20, 2023

OuyangHang33 Dec 21, 2023

longlongyang Dec 22, 2023

longlongyang Dec 20, 2023

OuyangHang33 Dec 21, 2023 •

edited

Loading

OuyangHang33 Dec 21, 2023

longlongyang Dec 22, 2023

longlongyang Dec 22, 2023

longlongyang left a comment

jyao1 Dec 22, 2023 •

edited

Loading

OuyangHang33 Dec 24, 2023

jyao1 Dec 22, 2023

OuyangHang33 Dec 24, 2023

jyao1 Dec 25, 2023

jyao1 Dec 25, 2023

OuyangHang33 Dec 25, 2023

Add SPDM extensions OID check #166

Add SPDM extensions OID check #166

Conversation

OuyangHang33 commented Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wenxing-hou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OuyangHang33 Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OuyangHang33 Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longlongyang left a comment

Choose a reason for hiding this comment

jyao1 Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OuyangHang33 commented Dec 11, 2023 •

edited

Loading

OuyangHang33 Dec 21, 2023 •

edited

Loading

OuyangHang33 Dec 21, 2023 •

edited

Loading

jyao1 Dec 22, 2023 •

edited

Loading