dmtf / libspdm Goto Github PK

Current implementation hardcodes the measurement index to 1, if only one measurement block is requested.
This is not right interpretation. The measurement index shall always match the measurement block from the device.

Same issue when all measurement blocks are returned, the index should NOT be from 1 to N. They shall used the measurement block index from the device.

It's in spdm_set_data and spdm_get_data but no other code uses it.

In spec, responder will return digests of its local certchains one by one when received a valid get_digests request,
and requester should compare cached digests to digests in response(spec 214 and 234).

In libspdm, this implemented by compare digest of local peer_cert_chain_provision to digests in response.
But now it looks like that treated all digests as a whole and finally only compared first digest in response.
Is it a bug?

See https://tls.mbed.org/security
https://github.com/ARMmbed/mbedtls/releases

Currently, the function description for spdm_bin_concat in library/spdm_secured_message_lib/session.c mentions that the parameter label_size should include the NULL string terminator, when describing the size of the string pointed to by "label".

However, the implementation of the function does not expect a NULL terminator, and usage of this function shows that the NULL terminator is not included in label_size arguments.

This description should be updated, or the function itself updated.

The LibSpdmConfig.h file is currently located within the repository. A user, who doesn`t want to use the default configuration, need to modify the file within the repo.
It would be more flexible to pass the file at compilation time, externally from the repo.

From jyao1/openspdm#138

TestSpdmRequesterGetMeasurement fail due to one of the following reasons:
Attempting to send a large message to the requester triggers a runtime assert on SpdmTransportTestLib.c:87 (test cases 18, 30, 31)

Current version SpdmTransportTestLib only support stack allocation. To support large portion, I think we need enhance the SpdmTransportTestLib to allocate from heap, in case the MessageSize is greater than MAX_SPDM_MESSAGE_BUFFER_SIZE.

Responder authentication multiple key exchange example
The DHE secrets are computed as specified in clause 7.4 of RFC 8446.

in RFC 8446, 7.4:
The negotiated key (Z) is converted to a
byte string by encoding in big-endian form and left-padded with zeros
up to the size of the prime.

migrate from jyao1/openspdm#178 and jyao1/openspdm#176

See detail discussion there.

And if we can't think of a good reason, then let's use types from <stdint.h>.

For example in context_data.c:

switch (data_type) {
case SPDM_DATA_SESSION_USE_PSK:
case SPDM_DATA_SESSION_MUT_AUTH_REQUESTED:
case SPDM_DATA_SESSION_END_SESSION_ATTRIBUTES:
	return TRUE;
}
return FALSE;

produces error: enumeration value 'SPDM_DATA_SPDM_VERSION' not handled in switch [-Werror=switch]

from jyao1/openspdm#6

The code requires that MANAGED_BUFFER be the same as the first two components of LARGE/SMALL_MANAGED_BUFFER.
To facilitate such, suggest below:

typedef struct {
UINTN MaxBufferSize;
UINTN BufferSize;
//UINT8 Buffer[MaxBufferSize];
} MANAGED_BUFFER;

typedef struct {
MANAGED_BUFFER ManagedBufferSizes;
UINT8 Buffer[MAX_SPDM_MESSAGE_BUFFER_SIZE];
} LARGE_MANAGED_BUFFER;

typedef struct {
MANAGED_BUFFER ManagedBufferSizes;
UINT8 Buffer[MAX_SPDM_MESSAGE_SMALL_BUFFER_SIZE];
} SMALL_MANAGED_BUFFER;

The type uintn is defined in processor_bind.h, and is described as

Unsigned value of native width. (4 bytes on supported 32-bit processor instructions,
8 bytes on supported 64-bit processor instructions)

There is a latent issue that may arise due to the use of uintn. libspdm may be ported to 8-bit or 16-bit microcontrollers, in which case uintn is not large enough to hold certain values. For example the function

/**
  Return the size in bytes of the SPDM context.

  @return the size in bytes of the SPDM context.
**/
uintn spdm_get_context_size(void);

returns the size of the internal context and is most assuredly greater than 255 bytes and, sometimes, even greater than 65535 bytes.

From jyao1/openspdm#182

The spec requires the message concatenation performed only after a successful completion response.

258 Concatenate() is the standard concatenation function that is performed only after a successful
completion response on the entire request and response contents.

265 ◦ If a response contains an ErrorCode other than ResponseNotReady :
266 No concatenation function is performed on the contents of both the original request and
response.

But the behavior of the current Responder is typically:

SpdmGetResponseFoo()
{

1. validate request
2. append request into managed buffer
3. generate response
4. append response into managed buffer
   }
   This model didn't handle the scenario when some errors occur after 2), the request shouldn't be appended into the buffer.
   It might be better to change the behavior as:

SpdmGetResponseFoo()
{

1. validate request
2. generate response
3. if everything is okay, concatenate request and response and append it into the managed buffer together.
   }
   (Note that lower layer failures, e.g. errors in MCTP or SMBus causing the response not sent to the requester, are ignored here.)

The current responder code for MEASUREMENT signature generation in the other hand implemented the similar requirement.

SpdmGetResponseMeasurement()
{

1. validate request
2. append request into managed buffer
3. generate response
   3-1. reset the buffer if error X occurs
   3-2. reset the buffer if error Y occurs
   ... and so on.
   }
   But it should be simpler to also apply the proposed model to simplify the flow.

PKCS11 is an industry-standard and generic cryptography interface. Instead of libSPDM supporting multiple bindings to cryptography libraries it could have a single PKCS11 layer that then binds to the underlying cryptography libraries.

e.g. opensource implementation: https://github.com/tpm2-software/tpm2-pkcs11

One possible way is to plug-in pkcs11 instead of replacing crytolib directly.
We can let libspdm->cryptlib->pkcs11. e.g. create cryptlib_pkcs11
If anyone implements a pkcs11 lib, then this pkcs11 lib can be plug in.
People still prefers to keep cryptlib interface here, because it is simpler than pkcs11.

Currently a spdm_context only have one connection info field,
It seems that another context is needed If want to connect with another Responder,
And this may cause the same session-ID to be assigned.
Or one connection will bind one transport layer channel?

Issue from jyao1/openspdm#193

In SpdmServerConnectionStateCallback(), if connection state is SpdmConnectionStateNegotiated, a new SPDM_CERT_CHAIN will be allocated by ReadResponderPublicCertificateChain(), and then the allocated SPDM_CERT_CHAIN will be held in SpdmContext->LocalContext.LocalCertChainProvision.

That's a smart design but I didn't seem to see the code to free the newly allocated SPDM_CERT_CHAIN. Would you point me to where the buffer gets freed?

BTW, I think we don't need to compose the SPDM_CERT_CHAIN every time when the connection state reaches to Negotiated. We only need to recompose the SPDM_CERT_CHAIN only when the negotiated base hash algorithm changes.

In the function spdm_check_response_flag_compability(), the if statement of Case3 in below is a dead code because it will be blocked by the Case1 (key_ex_cap != 0) and Case2 (key_ex_cap == 0), we cannot enter into the return statement of Case3.

case SPDM_MESSAGE_VERSION_11: {
            ......
	//Case1: Key_ex_cap set and encrypt_cap+mac_cap cleared
	if (key_ex_cap != 0 && (encrypt_cap == 0 && mac_cap == 0)) {
		return FALSE;
	}
            ......
	//Case2: Handshake_in_the_clear_cap set and key_ex_cap cleared
	if (handshake_in_the_clear_cap != 0 && key_ex_cap == 0) {
		return FALSE;
	}

	//Case3: Handshake_in_the_clear_cap set and encrypt_cap+mac_cap cleared
	if ((encrypt_cap == 0 && mac_cap == 0) &&
	    handshake_in_the_clear_cap != 0) {
		return FALSE;
	}
	......

Majority of files are licensed under DMTF BSD 3-Clause License while there are some files (eg. include/HAL) have BSD-2-Clause-Patent. Any plans to unify licensing across whole library ?

From jyao1/openspdm#184

Current code keeps the whole transcript and calculate hash finally. That means we need big memory to hold the transcript.
The better idea is to just keep calculating the hash. Then we just need memory to hold the hash context.

After GET_VERSION, the requester need decide which version is final selected and always use it.

After GET_CAPABILITY, the responder will know this is the version selected by the requester and always stick it it.

Current implementation just check all supported versions, which is too relax.

In functions where a transcript hash is required, we frequently allocate more buffers than necessary, and we can save on stack space by being more prudent with how buffers are utilized. Since these functions can be the culprit of largest stack usage, lowering this stack space is valuable for running on any resource-limited platforms.

For example, in spdm_generate_key_exchange_rsp_signature, we allocate "th_curr_data", a buffer of MAX_SPDM_MESSAGE_BUFFER_SIZE. Later, we pass "th_curr_data" as the output buffer when calling the function spdm_calculate_th_for_exchange.

The function spdm_calculate_th_for_exchange allocates its own large_managed_buffer_t, "th_curr", to hold the transcript.

Once the transcript is copied into the temporary "th_curr", it is copied out into "th_curr_data" at the end of the function.

On the stack, this requires two MAX_SPDM_MESSAGE_BUFFER_SIZE byte buffers to be allocated on the stack. In this scenario, "th_curr" is redundant, and we could simply utilize "th_curr_data" directly as the output buffer in spdm_calculate_th_for_exchange, removing one of the large buffers from the stack.

This usage pattern is typical for transcript calculation functions, so we could make this improvement in multiple areas.

From jyao1/openspdm#3

SPDM_DEVICE_CONTEXT is overloaded with both Requester and Responder context.

Suggest define SPDM_REQUESTER_CONTEXT and SPDM_RESPONDER_CONTEXT instead.

From jyao1/openspdm#170

For example: all session states, negotiated algo, etc,

Develop fuzzing state machine as a document to help with fuzz testing.

There are multiple instances where, after spdm_append_message* is called, the requester or responder returns SPDM_ERROR_CODE_INVALID_REQUEST. The error should be SPDM_ERROR_CODE_UNSPECIFIED, since the specification lacks a good error for when the endpoint has run out of resources.

Wondering the possibility for expanding the #define flags in SpdmLibConfig.h to include different optional messages and/or capabilities, such that the code for handling the response to these messages is #define'd out and not compiled.

The scenario I had in mind is for a memory-constrained Responder. For example, if CERT_CAP isn't going to be utilized, a user would be able to set some define, say "#OPENSPDM_RESPONDER_CERT_CAP_SUPPORT 0". This define would then remove the related certificate handling code, leaving only the necessary error handling code - that way, in case the Requester still sent GET_DIGESTS/GET_CERTIFICATE, the Responder would respond with expected error.

It would require some thinking through - likely requiring warning/error if user tries to set a capability that was compiled out. However, it would be a great way for a consumer of OpenSPDM, with code-size limitations and a static set of capabilities, to easily trim down the code size without having to manually remove messages.

It is for microcontroller case - memory constrain environment. Compiler-time optimization to save space.

From jyao1/openspdm#146

As such, this callback can be dispatched from SPDM directly.
This is not feasible, because SPDM does not have PciIdeKm knowledge.

The alternative is to allow register multiple GetResponseFunc() functions.
The SPDM responder can dispatch them one by one. Each Response Func should only process its own protocol, and return RETURN_UNSUPPORTED for the unknown ones.

The dispatch will stop once there is one GetResponseFunc() returns SUCCESS.
It fails only after all dispatch functions return UNSUPPORTED.

encode_decode.c contains the code

*(uint64 *)salt = *(uint64 *)salt ^ sequence_number;

where salt is an array of bytes. This is interpreted differently on big-endian and little endian architectures. Need to use shifts and bitmasks to perform this operation in an endianness-agnostic manner.

bash_hash_algo should be renamed to base_hash_algo

From jyao1/openspdm#183

Error code InvalidRequest is described as "One or more request fields are invalid".
However, it's often seen in the current implementation to use SPDM_ERROR_CODE_INVALID_REQUEST for responder internal errors.
For example, the request message is valid but AppendMessage fails.

Status = SpdmAppendMessageB (SpdmContext, SpdmRequest, SpdmRequestSize);
if (RETURN_ERROR(Status)) {
SpdmGenerateErrorResponse (SpdmContext, SPDM_ERROR_CODE_INVALID_REQUEST, 0, ResponseSize, Response);
return RETURN_SUCCESS;
}
Would it be better to use other error codes, e.g. Unspecified, or Vendor/Other Standards Defined?
InvalidRequest is kind of misleading.

Currently libspdm unconditionally resets the connection state even if the GET_VERSION request is invalid. It should first check that the request is valid before resetting state.

There will be a change to the specification to clarify this issue. See https://github.com/DMTF/Security-TF/issues/1384

Examples include:

• Validate that one-hot fields are actually one-hot.
• In negotiate_algorithms the requester loops over ALGORITHMS.param1 without validating it first.

From jyao1/openspdm#173

SPDM 1.1 says
"The format of the provisioned public key is out of scope of this specification. Vendors can use proprietary formats or public key formats that other standards define, such as RFC7250 and RFC4716."

Suggest:
OpenSPDM implements a "raw_public_key_extract" API prototype and leaves to the integrator to implement.
input: raw public key of format defined by vendor
output: raw public key of the following format
256-bit ECC: 32-byte x || 32-byte y.
384-bit ECC: 48-byte x || 48-byte y.
2048-bit RSA: 256-byte n || 8-byte e.
3072-bit RSA: 384-byte n || 8-byte e.
4096-bit RSA: 512-byte n || 8-byte e.

The input is saved in "context" for transcript hash construction.
The output is saved in "context" for session establishment, mutual authentication, and other requests requiring signature.

This issue is depending on approval of SPDM issue
DMTF/Security-TF#1110

NOTE:
Current OpenSPDM uses X509 certificate based public key. The whole public key certificate is included in transcript hash calculation.
I think we can use ASN.1_subjectPublicKeyInfo defined in https://tools.ietf.org/html/rfc7250.
Maybe we take a look at the openssl/mbedtls implementation on how to handle the raw keys.

The spec says that hashes of certificate chains, for example DIGESTS.digest, need to be big-endian. Is libspdm consistent with this?

There are multiple instances of

if (spdm_context->local_context.local_cert_chain_provision == NULL)

that produce the warning from Coverity

CID 344411 (#1 of 1): Array compared against 0 (NO_EFFECT)
array_null: Comparing an array to null is not useful: spdm_context->local_context.local_cert_chain_provision == NULL, since the test will always evaluate as true.

See https://www.openssl.org/news/vulnerabilities-1.1.1.html
https://www.openssl.org/source/

random_bytes returns a boolean value to indicate whether random number generation was successful. However spdm_get_random_number does not convey this information to the caller. On the responder side this would more than likely result in an Unspecified error.

void spdm_get_random_number(IN uintn size, OUT uint8 *rand)
{
	random_bytes(rand, size);

	return;
}

Receiving a SPDM message with a VERSION 0x0 request_response_code instead of a GET_VERSION 0x84.
Expected behavior: the responder refuses the VERSION message and produces an ERROR message indicating the InvalidRequest.
But current behavior: it is not refuse this message and works without error.

Test code example:
spdm_get_version_request_t m_spdm_get_version_request4 = {
{
SPDM_MESSAGE_VERSION_10,
0x0, // Correct value should be SPDM_GET_VERSION 0x84
},
};
uintn m_spdm_get_version_request4_size = sizeof(m_spdm_get_version_request4);

status = spdm_get_response_version(spdm_context,
m_spdm_get_version_request4_size,
&m_spdm_get_version_request4,
&response_size, response);
assert_int_equal(status, RETURN_SUCCESS);
assert_int_equal(spdm_response->header.request_response_code, SPDM_ERROR);

In the MEASUREMENT response in SPDM the structure always includes a Nonce for SPDM 1.0, 1.0.1, 1.1, and 1.1.1. The requirement comes from the table "Successful MEASUREMENTS response message":
"8 + L Nonce 32 The Responder should choose a random value."

Yet the SPDM Requestor code that parses the MEASUREMENT response does not account for the Nonce in the non-GENERATE_SIGNATURE case. Specifically in try_spdm_get_measurement() the if-block for generating a signature does contain code to account for the Nonce.

    	if (request_attribute ==
    	    SPDM_GET_MEASUREMENTS_REQUEST_ATTRIBUTES_GENERATE_SIGNATURE) {

But then the complementary else-statement both fails to account for the None and has a comment saying the Nonce is not there.

} else {
    		//
    		// nonce is absent if there is not signature
    		//
    		if (spdm_response_size <
    		    sizeof(spdm_measurements_response_t) +
    			    measurement_record_data_length + sizeof(uint16)) {
    			return RETURN_DEVICE_ERROR;
    		}
    		ptr = measurement_record_data + measurement_record_data_length;
    		opaque_length = *(uint16 *)ptr;
    		if (opaque_length > MAX_SPDM_OPAQUE_DATA_SIZE) {
    			return RETURN_SECURITY_VIOLATION;
    		}
    		ptr += <span class="pl-k">sizeof</span>(uint16);<!--EndFragment-->

The statement about losing a Nonce if a signature is not being generated in SPDM 1.0, 1.0.1, 1.1, and 1.1.1 is only true in the requester structure's table "GET_MEASUREMENTS request message":
"4 Nonce 32 The Requester should choose a random value. This field is only present if a signature is required
on the response. See Table 23."

I haven't fully analyzed the SPDM Responder code for the MEASUREMENT response to see if it produces the Nonce field in both cases.

Unfortunately, I'm in a position where I won't be able to provide a code contribution for an unknown amount of time. This is a pretty fundamental piece, so it may need to be fixed sooner than I can contribute.

The current draft of SPDM (maybe 1.2) does contain text to remove the Nonce field from the MEASUREMENT response if no signature is being generated.

@steven-bellock will propose a set of secure code review guidelines tailored towards SPDM.

Some of the SPDM responder operations for crypto signing take time and cannot be returned to requester immediately.

Per DSP0274 version 1.1.1/Section 10.13 , error ResponseNotReady could be returned to the spdm requester and it’s up to the requester to get asynchronous result with RESPONSE_IF_READY.

openspdm currently does not support ResponseNotReady/RESPONSE_IF_READY. Could you help adding this important functionality to openspdm?

From jyao1/openspdm#172

SPDM spec: Any communication between Requester and Responder other than a GET_MEASUREMENTS request or response resets L1/L2 computation to null.

Also reset MessageB/C for any interrupt:

If the Requester issued GET_MEASUREMENTS or KEY_EXCHANGE or FINISH or PSK_EXCHANGE or PSK_FINISH or KEY_UPDATE or HEARTBEAT or GET_ENCAPSULATED_REQUEST or DELIVER_ENCAPSULATED_RESPONSE or END_SESSSION request(s) and skipped CHALLENGE completion, M1 and M2 are reset to null .

Same criteria for MessageMutB/MutC

The functions registered trough spdm_register_device_io_func may need extra information that is not available in spdm_context (therefore not easily obtainable).

Would it be the case to pass an opaque pointer to these function calls?
The opaque pointer could be registered in spdm_register_device_io_func along with the functions.

Current spdm_verify_peer_cert_chain_buffer() assumes the first cert is root cert.

Looking forward in time as libspdm matures, is the intent to be more than just a sample implementation?

The README.md says:

Support to be included in UEFI host environment EDKII, such as edkii_spdm_requester
Support to be included in OpenBMC. It is in planning, see SPDM Integration.

... but it also says:

This openspdm is a sample implementation for the DMTF SPDM specification

and

Known limitation: This package is only the sample code to show the concept. It does not have a full validation such as robustness functional test and fuzzing test. It does not meet the production quality yet. Any codes including the API definition, the libary and the drivers are subject to change.

One interpretation of these statements is that libspdm isn't production quality yet, and that possibly the author's intent is to make libspdm a reference implementation instead of just a sample implementation. As someone relatively new to the libspdm project, it wasn't clear whether the goal of libspdm is to remain a sample (non-production) or serve as a reference implementation for vendors / implementers.