Harden SPDM 1.4 `PSK_FINISH` opaque handling with core unit test coverage by czwolak · Pull Request #3606 · DMTF/libspdm

czwolak · 2026-04-22T11:41:31Z

Harden requester/responder SPDM 1.4 PSK_FINISH opaque validation paths.
Align requester SPDM 1.4 opaque format negotiation setup in UT.
Keep core opaque boundary/error coverage: req cases 18/19 and rsp cases 17/18.
Remove optional requester cases 20/21 and related harness scaffolding.

jyao1 · 2026-04-23T05:36:07Z

+                status = LIBSPDM_STATUS_INVALID_PARAMETER;
+                libspdm_release_sender_buffer (spdm_context);
+                goto error;
+            }


In current libspdm, we always use ASSERT for SPDM_MAX_OPAQUE_DATA_SIZE check.
You may search others, such as key_exchange, finish.
Why we have to add check here, in this psk_finish?

Agree. For outbound requester input-size validation, key_exchange / psk_exchange / finish use LIBSPDM_ASSERT and do not return runtime INVALID_PARAMETER.
I will align psk_finish with existing style by removing the runtime check and keeping ASSERT-only

jyao1 · 2026-04-23T05:37:43Z

+            if (requester_opaque_data_size != 0) {
+                status = LIBSPDM_STATUS_INVALID_PARAMETER;
+                libspdm_release_sender_buffer (spdm_context);
+                goto error;
+            }


Same question.
I think we need use a consistent way to decide when to check and when not to check.
Please refer to opaque data handling in other command.

Agree. The requester_opaque_data == NULL && requester_opaque_data_size != 0 runtime error is also not used in peer commands.
I will remove this runtime check for consistency with existing requester command patterns.

jyao1 · 2026-04-23T05:41:31Z

+        if (((spdm_context->connection_info.algorithm.other_params_support &
+              SPDM_ALGORITHMS_OPAQUE_DATA_FORMAT_MASK) == SPDM_ALGORITHMS_OPAQUE_DATA_FORMAT_NONE)
+            && (opaque_data_size != 0)) {
+            status = LIBSPDM_STATUS_INVALID_MSG_FIELD;
+            goto receive_done;
+        }


This is already checked in key_exchange. Why we need to check again?

Agree this is redundant with current negotiated state handling and inconsistent with finish/key_exchange response parsing style.
I will remove this extra check to keep behavior/policy consistent.

jyao1 · 2026-04-23T05:41:49Z


        if ((responder_opaque_data != NULL) && (responder_opaque_data_size != NULL)) {
-            if (opaque_data_size >= *responder_opaque_data_size) {
+            if (opaque_data_size > *responder_opaque_data_size) {


Good catch!

I notice finish has same issue.

Thanks. Do you think we better create separate follow-up issue or propose "finish" fix within this PR ?

I am OK to create another PR.
I just want to ensure 1) all issues are fixed, 2) the solution is consistent, especially the similar ones.
That is why I cross-check other commands.

Thank you :) can we confirm also another cases?

FINISH - what you found

libspdm/library/spdm_requester_lib/libspdm_req_finish.c

Lines 611 to 614 in 538d089

if (opaque_data_size >= *responder_opaque_data_size) {

status = LIBSPDM_STATUS_BUFFER_TOO_SMALL;

goto receive_done;

}

KEY_EXCHANGE

libspdm/library/spdm_requester_lib/libspdm_req_key_exchange.c

Lines 728 to 731 in 538d089

if (opaque_length >= *responder_opaque_data_size) {

status = LIBSPDM_STATUS_BUFFER_TOO_SMALL;

goto receive_done;

}

PSK_EXCHANGE

libspdm/library/spdm_requester_lib/libspdm_req_psk_exchange.c

Lines 459 to 462 in 538d089

if (spdm_response->opaque_length >= *responder_opaque_data_size) {

status = LIBSPDM_STATUS_BUFFER_TOO_SMALL;

goto receive_done;

}

jyao1 · 2026-04-23T05:41:56Z

        opaque_data_entry_size = 0;
    }
-    spdm_response_size = sizeof(spdm_finish_response_t) + opaque_data_entry_size;
+    spdm_response_size = sizeof(spdm_psk_finish_response_t) + opaque_data_entry_size;


Good catch!

jyao1 · 2026-04-23T05:46:16Z

            }
+
+            if ((opaque_data_size > SPDM_MAX_OPAQUE_DATA_SIZE) ||
+                (opaque_data_size > max_opaque_data_size)) {


I am not sure how it is possible that (opaque_data_size > max_opaque_data_size).

I feel (opaque_data_size > SPDM_MAX_OPAQUE_DATA_SIZE) should be enough.

I recommend keeping both checks:

SPDM_MAX_OPAQUE_DATA_SIZE: protocol/library upper bound
max_opaque_data_size: output-buffer capacity bound
They protect different constraints, so both are valid.

It can happen when the protocol limit and the actual buffer capacity are different.

SPDM_MAX_OPAQUE_DATA_SIZE is a global maximum allowed by libspdm/protocol.
max_opaque_data_size is how many bytes are currently free in this specific response buffer after headers.
So opaque_data_size may still be valid globally, but too large for the current output buffer.
Example: global max is 1024, but current buffer has only 80 bytes left; function generates 120 bytes. Then:

120 <= 1024 (passes global check)
120 > 80 (fails buffer-capacity check)

I am looking at libspdm_psk_finish_rsp_opaque_data() API. opaque_data_size is an IN OUT.
opaque_data_size is already max_opaque_data_size on input.

The output opaque_data_size must be equal or smaller than the input size.
That is why I think it is impossible. Otherwise, the buffer overflow already happened.

OK, I understand we want to assume IN OUT is working here, if it didn't then it is already too late to detect. I will remove this.

…erage - Harden requester/responder SPDM 1.4 PSK_FINISH opaque validation paths. - Align requester SPDM 1.4 opaque format negotiation setup in UT. - Keep core opaque boundary/error coverage: req cases 18/19 and rsp cases 17/18. - Remove optional requester cases 20/21 and related harness scaffolding. Signed-off-by: Cezary Zwolak <cezary.zwolak@intel.com>

czwolak requested review from jyao1 and steven-bellock as code owners April 22, 2026 11:41

czwolak mentioned this pull request Apr 22, 2026

PSK_FINISH/PSK_FINISH_RSP (SPDM 1.4): Opaque-Length Validation and Response Completeness Gaps #3592

Open

czwolak force-pushed the czwolak/issue-3592-psk-finish-opaque branch from 93d5603 to ce5ae7d Compare April 22, 2026 11:48

steven-bellock changed the title ~~#3592 Harden SPDM 1.4 PSK_FINISH opaque handling with core UT coverage~~ Harden SPDM 1.4 PSK_FINISH opaque handling with core unit test coverage Apr 22, 2026

jyao1 reviewed Apr 23, 2026

View reviewed changes

czwolak force-pushed the czwolak/issue-3592-psk-finish-opaque branch 2 times, most recently from 38d26ed to 08382fc Compare April 24, 2026 07:57

czwolak force-pushed the czwolak/issue-3592-psk-finish-opaque branch from 08382fc to be32879 Compare April 24, 2026 08:29

	if (opaque_data_size >= *responder_opaque_data_size) {
	status = LIBSPDM_STATUS_BUFFER_TOO_SMALL;
	goto receive_done;
	}

	if (opaque_length >= *responder_opaque_data_size) {
	status = LIBSPDM_STATUS_BUFFER_TOO_SMALL;
	goto receive_done;
	}

	if (spdm_response->opaque_length >= *responder_opaque_data_size) {
	status = LIBSPDM_STATUS_BUFFER_TOO_SMALL;
	goto receive_done;
	}

Conversation

czwolak commented Apr 22, 2026 • edited by steven-bellock Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jyao1 Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czwolak Apr 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

czwolak commented Apr 22, 2026 •

edited by steven-bellock

Loading

jyao1 Apr 23, 2026 •

edited

Loading

czwolak Apr 24, 2026 •

edited

Loading