Adding Session ID to Vendor Defined Response

[m-milinkovic]

Hi libspdm contributors,

I'd like to raise a discussion about adding a pointer to Session ID to Vendor Defined Response. For example, it could be [potentially] added as a new argument to vendor_response_callback (i.e., to libspdm_vendor_response_callback_func type definition, introduced in libspdm v3.2), and then simply compared against NULL.

RATIONALE / MOTIVATION

IDE_KM and TDISP (i.e., from DMTF/spdm-emu repo: pci_ide_km_get_response, pci_tdisp_get_response, cxl_ide_km_get_response) can be processed through vendor_response_callback, instead of get_response_func callback (type: libspdm_get_response_func) as prior to libspdm v3.2. These protocols are coupled with SPDM secure session, e.g in PCI-SIG base specification 6.x it is explicitly stated that any IDE_KM exchange must occur through a Secure SPDM session.

Consequently, this is potentially a security threat because it is easy to forget checking if Session ID is valid in vendor_response_callback, thus allowing exchange of insecure Vendor Defined Messages.

I'm aware that Session ID can be obtained through last_spdm_request_session_id_valid and last_spdm_request_session_id. However, taking all into account (it's easy to forget and it's not quite trivial to obtain Session ID), it seems error prone. Also, in order to use related content of libspdm_context_t, internal/libspdm_common_lib.h must be included - this might be an overstatement, but maybe it could be treated as unnecessary breaking of libspdm encapsulation.

Please share your thoughts, I'd appreciate it very much.
If what's being said makes sense to you, I'd be happy to propose a solution.

BR!

[steven-bellock]

At a minimum we can expose last_spdm_request_session_id_valid through libspdm_get_data. If you feel like you need the session ID as well then that can be exposed too.

[m-milinkovic]

Hi @steven-bellock,
Many thanks for joining the discussion.

"If you feel like you need the session ID as well then that can be exposed too."
Yes, Session ID is needed. I'll explain it from IDE_KM point of view.

Based on PCI-SIG specification 6.x, every exchange between IDE_KM Requester and Responder must go through SPDM secure session (i.e., through secure Vendor Defined Request/Response).

Moreover, regarding IDE_KM, PCI-SIG specification 6.x is even more restrictive than simply checking if the Vendor Defined Request is secure or not:
"While the secure [SPDM] session that was used for initial key programming remains open, all QUERY and/or KEY_PROG requests that are received through a different secure [SPDM] session must be discarded by the Responder, and must not result in a response."
In other words, some kind of stream_id↔︎session_id mapping (or probably better port_index↔︎stream_id↔︎session_id) needs to be maintained on IDE_KM level, and Session ID is definitely needed for this.

Also, pci_ide_km_get_response takes a pointer to Session ID as an input argument and the user needs to provide one in their implementation of vendor_response_callback.

The remaining question is - are there enough arguments to support modifying libspdm_get_vendor_defined_response to provide Session ID pointer to vendor_response_callback (and modify its signature)?

From a user perspective, the simplest solution would be to receive a pointer to Session ID (const uint32_t *) from SPDM library as an input argument of vendor_response_callback, and then a user could simply compare it against NULL:

• if it's NULL, the message is insecure and it should be discarded
• if it's not NULL, Session ID is valid and can be stored in IDE_KM context

=> no need to expose spdm_context_t.

If you prefer avoiding changing vendor_response_callback signature and affecting other users, then your proposal (modifying libspdm_get_data only) is probably a good compromise.

Please let me know your decision.
I'd be glad to provide a patch.

[steven-bellock]

It's fine to add it to libspdm_vendor_response_callback_func. We'll just wrap it in an #ifdef that will be removed in libspdm 4.0.