DMTF / libspdm

BSD 3-Clause "New" or "Revised" License
104 stars 100 forks source link

libspdm mandating `OpaqueData` to be set to `SPDM_ALGORITHMS_OPAQUE_DATA_FORMAT_1` #2732

Closed PrithviAPai closed 3 months ago

PrithviAPai commented 3 months ago

Some SPDM Responders(v 1.2) don't support OpaqueData and do not set OtherParamsSupported. But libspdm mandates OpaqueData to be set to SPDM_ALGORITHMS_OPAQUE_DATA_FORMAT_1 (https://github.com/DMTF/libspdm/blob/main/library/spdm_requester_lib/libspdm_req_negotiate_algorithms.c#L576) I could not find the reason for this in SPDM Spec. Need some info on this.

Thanks, Prithvi A Pai

Wenxing-hou commented 3 months ago

Although the requester want to set connection_info.algorithm.other_params_support, the connection_info.algorithm.other_params_support will still be 0 when spdm_response->other_params_selection is 0.

https://github.com/DMTF/libspdm/blob/d69d7e9a2eb4ea4ce1e8dd755cc1da7919ca2702/library/spdm_requester_lib/libspdm_req_negotiate_algorithms.c#L382-L384

PrithviAPai commented 3 months ago

Although the requester want to set connection_info.algorithm.other_params_support, the connection_info.algorithm.other_params_support will still be 0 when spdm_response->other_params_selection is 0.

https://github.com/DMTF/libspdm/blob/d69d7e9a2eb4ea4ce1e8dd755cc1da7919ca2702/library/spdm_requester_lib/libspdm_req_negotiate_algorithms.c#L382-L384

Yes, I agree here, the connection_info.algorithm.other_params_support will be 0. But further in response validation phase of ALGORITHMS we have a check where we try equating connection_info.algorithm.other_params_support (the value of which is 0) with SPDM_ALGORITHMS_OPAQUE_DATA_FORMAT_1(the value of which is 2). Is this correct ? (https://github.com/DMTF/libspdm/blob/main/library/spdm_requester_lib/libspdm_req_negotiate_algorithms.c#L576) if (spdm_response->header.spdm_version >= SPDM_MESSAGE_VERSION_12) { if ((spdm_context->connection_info.algorithm.other_params_support & SPDM_ALGORITHMS_OPAQUE_DATA_FORMAT_MASK) != SPDM_ALGORITHMS_OPAQUE_DATA_FORMAT_1) { status = LIBSPDM_STATUS_NEGOTIATION_FAIL; goto receive_done; } }

PrithviAPai commented 3 months ago

@steven-bellock can you please share your thoughts ?

steven-bellock commented 3 months ago

This is a case of #85. However in this particular case it is a bug, as it can handle and check other messages if there is no common opaque data format. For example : https://github.com/DMTF/libspdm/blob/d69d7e9a2eb4ea4ce1e8dd755cc1da7919ca2702/library/spdm_requester_lib/libspdm_req_get_measurements.c#L404-L412

jyao1 commented 3 months ago

The code logic is: If KEY_EX is supported, then OPAQUE_FORMAT_1 must be set. The reason is that we need to rely on OPAQUE data to exchange DSP0277 format.

But if the device does not support SPDM session, then OPAQUE_FORMAT can be NULL.

@PrithviAPai, please share with us the device capability. E.g. does this device support SPDM session for DSP0277?

PrithviAPai commented 3 months ago

@jyao1 Here is the CAPS Request and Response libspdm_send_spdm_request[0] msg SPDM_GET_CAPABILITIES(0xe1), size (0x14): 0000: 12 e1 00 00 00 00 00 00 c2 22 00 00 00 12 00 00 00 12 00 00

libspdm_receive_spdm_response[0] msg SPDM_CAPABILITIES(0x61), size (0x14): 0000: 12 61 00 00 00 17 00 00 f6 03 04 00 f8 0f 00 00 f8 0f 00 00

Yes, KEY_EX is supported

jyao1 commented 3 months ago

If KEY_EX is supported, then endpoint shall use opaque data to negotiate secure SPDM version in DSP0277.

Steven's feedback https://github.com/DMTF/libspdm/issues/2732#issuecomment-2179073165 just causes failure happening later, but it does not resolve any problem.

steven-bellock commented 3 months ago

@jyao1 the issue is that libspdm is returning an error during VCA that prevents the Requester from issuing other messages such as GET_MEASUREMENTS, etc. libspdm should allow any legal value for OpaqueDataFmt* during VCA.

jyao1 commented 3 months ago

See our check in Capability, such as https://github.com/DMTF/libspdm/blob/main/library/spdm_requester_lib/libspdm_req_get_capabilities.c#L100-L104. Our rule is to report error in place, if the sees the configuration error violating the spec. (Otherwise, you may argue why not defer the check in KEY_EX or PSK_EX later ???)

KEY_EX without Opaque support is wrong configuration. We are following the consistent rule here.

steven-bellock commented 3 months ago

@PrithviAPai what does NEGOTIATE_ALGORITHMS.OtherParamsSupport and ALGORITHMS.OtherParamsSelection look like?

The specification is ambiguous on whether it's legal for a Responder to unconditionally set OtherParamsSelection.OpaqueDataFmt to 0. I'll file an issue against the specification for that.

@jyao1 the key exchange check is performed when calling the key exchange function. For example : https://github.com/DMTF/libspdm/blob/d69d7e9a2eb4ea4ce1e8dd755cc1da7919ca2702/library/spdm_requester_lib/libspdm_req_key_exchange.c#L323-L328

PrithviAPai commented 3 months ago

@steven-bellock Here is the Request/Response payload

libspdm_send_spdm_request[0] msg SPDM_NEGOTIATE_ALGORITHMS(0xe3), size (0x30): 0000: 12 e3 04 00 30 00 01 02 80 00 00 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0020: 02 20 12 00 03 20 02 00 04 20 80 00 05 20 01 00

libspdm_receive_spdm_response[0] msg SPDM_ALGORITHMS(0x63), size (0x34): 0000: 12 63 04 00 34 00 01 00 04 00 00 00 80 00 00 00 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0020: 00 00 00 00 02 20 10 00 03 20 02 00 04 20 80 00 05 20 01 00

NEGOTIATE_ALGORITHMS.OtherParamsSupport = 0x02 ALGORITHMS.OtherParamsSelection = 0x00

steven-bellock commented 3 months ago

So either the Responder only supports OpaqueDataFmt0 or it doesn't support any opaque data format. If it's the former then the Requester's VCA should succeed, or maybe produce a warning.

jyao1 commented 3 months ago

Without KEY_EX cap, we can provide a warning, since opaque is optional. With KEY_EX cap, it is an error, because libspdm cannot support other opaque format, or non-opaque.

steven-bellock commented 3 months ago

1993 is also a related issue on how libspdm currently handles opaque data negotiation during key exchange.

steven-bellock commented 3 months ago

I did not see that the current opaque data format check is conditional on key exchange being supported by both the Requester and Responder. It is somewhat reasonable to return an error in that case. Currently if the Requester does not want to do key exchange then it should not set its KEY_EX_CAP. If, in the future, this is too restrictive then libspdm could return a warning.

steven-bellock commented 3 months ago

Closing this issue for now. If the SPDM Working Group deems that it is illegal for a Responder to support no opaque data then I'll file a separate issue.

steven-bellock commented 3 months ago

If the SPDM Working Group deems that it is illegal for a Responder to support no opaque data

The SPDM Working Group has deemed that it is legal for a Responder to support no opaque data format.