Master-Secret calculation inconsistent with SPDM specification

[annarossa]

In master_secret calculation salt and IKM seem to have swapped places.
In DSP0274_1.2.0 (paragraph 738 & 742) for Master-Secret salt is Salt_1 and IKM is 0_filled, and in the code it is otherwise.
master_secret calculation:

libspdm/library/spdm_secured_message_lib/libspdm_secmes_session.c

Lines 424 to 427 in 97d23cb

	status = libspdm_hmac_all(
	secured_message_context->base_hash_algo,
	m_zero_filled_buffer, hash_size, salt1, hash_size,
	secured_message_context->master_secret.master_secret);

libspdm/include/library/spdm_crypt_lib.h

Lines 722 to 724 in 97d23cb

	bool libspdm_hmac_all(uint32_t base_hash_algo, const void *data,
	size_t data_size, const uint8_t *key,
	size_t key_size, uint8_t *hmac_value);

[jyao1]

@annarossa , good catch!

[steven-bellock]

@jyao1 In the specification HMAC-Hash is ill-defined, but assuming that HMAC-Hash is defined as HMAC-Hash(key, data) libspdm is correct no?

[jyao1]

Checking

libspdm/library/spdm_secured_message_lib/libspdm_secmes_session.c

Lines 237 to 242 in 97d23cb

	status = libspdm_hmac_all(
	secured_message_context->base_hash_algo,
	m_zero_filled_buffer, hash_size,
	secured_message_context->master_secret.dhe_secret,
	secured_message_context->dhe_key_size,
	secured_message_context->master_secret.handshake_secret);

They must be consistent.

[jyao1]

Since they are not consistent, one of them is wrong.

[steven-bellock]

Sure we can make libspdm consistent, but we don't know if it's consistent with the specification, because the specification doesn't define which parameter in HMAC-Hash is the key and which is the data.

[annarossa]

I think the answer is here: https://datatracker.ietf.org/doc/html/rfc5869

2.1 Notation

HMAC always has two arguments: the first is a key
and the second an input (or message). (Note that in the extract
step, 'IKM' is used as the HMAC input, not as the HMAC key.)

HMAC-Hash(salt, IKM)

[jyao1]

Ok, then Master-Secret is correct, it is Handshake-Secret that is wrong.

[steven-bellock]

The SPDM specification says

A key schedule describes how the various keys such as encryption keys used by a session are derived, and when each key is used. The default SPDM key schedule makes heavy use of HMAC, which RFC2104 defines, and HKDF-Expand, which RFC5869 describes.

So 5869 is only used for HKDF-Expand. 2104 doesn't define HMAC or HMAC-Hash but it does define hmac_md5(text, text_len, key, key_len, digest) in which case the text / data is the first parameter and the key is the second parameter.

[jyao1]

I am filing SPDM spec issue to get it clarified - https://github.com/DMTF/Security-TF/issues/2076

Assuming HMAC-Hash(salt, IKM) == HMAC(key, input data), we need fix handshake secret calculation, right?

[steven-bellock]

Assuming HMAC-Hash(salt, IKM) == HMAC(key, input data), we need fix handshake secret calculation, right?

Correct, though we should delay the Q2 checkpoint until the specification is resolved and this issue needs to be broadcast widely since it effects interoperability and (possibly) security. It seems like folks that are implementing non-libspdm endpoints haven't made it to session establishment, and that's why we haven't run into this issue sooner.