request_size paramater in libspdm_get_response_finish

[PavelPu123]

Hello

As I understand "request_size" in above functions is updated after the transport header is removed, thus the equality sign should stay here, as we shouldn't allow a larger message size (and it should be considered an invalid request). Please correct me if I'm missing something. Thanks.

libspdm/library/spdm_responder_lib/libspdm_rsp_finish.c

Lines 453 to 454 in 4d2b685

	if (request_size <
	sizeof(spdm_finish_request_t) + signature_size + hmac_size) {

Commit:
4d2b685#diff-e20c71e6f0c9740582f8b26119596ac39833a47ed51f0f0da9285e46ad7fab54

[jyao1]

We allow the transport layer to add padding. (for example, PCI_DOE requires 4 bytes aligned). A larger message maybe a valid one.
The parser should re-calculate the size. All message parser follows the same pattern.

[steven-bellock]

Do you assume

1. The receive buffer is first zeroized.
2. The message is placed into the receive buffer.
3. The message is parsed to determine its size.

and if the space outside of the message is non-zero, that this would be considered an invalid request? It's possible that the data outside of the parsed message could be used by an attacker in a ROP-style attack, especially if the endpoint doesn't support W^X. In such a case the endpoint could perform that check, preferably as soon as possible.

[PavelPu123]

1. Yes
2. Taking for example KEY_EXCHANGE:

libspdm/library/spdm_responder_lib/libspdm_rsp_key_exchange.c

Lines 167 to 171 in 9ad78c3

	libspdm_return_t libspdm_get_response_key_exchange(libspdm_context_t *spdm_context,
	size_t request_size,
	const void *request,
	size_t *response_size,
	void *response)

The receive buffer in this case is "const void *request".

3. The size determination is done here:

libspdm/library/spdm_responder_lib/libspdm_rsp_key_exchange.c

Lines 289 to 296 in 9ad78c3

	if (request_size < sizeof(spdm_key_exchange_request_t) + dhe_key_size +
	sizeof(uint16_t) + opaque_data_length) {
	return libspdm_generate_error_response(spdm_context,
	SPDM_ERROR_CODE_INVALID_REQUEST, 0,
	response_size, response);
	}
	request_size = sizeof(spdm_key_exchange_request_t) + dhe_key_size +
	sizeof(uint16_t) + opaque_data_length;

So if "In such a case the endpoint could perform that check, preferably as soon as possible." - where do you propose to perform this check (assuming that message fields are only known when message arrived to this function).

Thanks.

[PavelPu123]

@steven-bellock Can you please answer my last question regarding size check? It would be much appreciated. Thanks.

[steven-bellock]

The message size that is calculated by parsing would be determined after the SPDMVersion and RequestResponseCode have been validated.

[PavelPu123]

Which is where exactly? Here?

libspdm/library/spdm_responder_lib/libspdm_rsp_key_exchange.c

Line 200 in 9ad78c3

}

And how do you propose to do the check (assuming we can't use the "request_size" parameter as mentioned above)?
Thanks.