dmtf / libspdm Goto Github PK
View Code? Open in Web Editor NEWLicense: BSD 3-Clause "New" or "Revised" License
License: BSD 3-Clause "New" or "Revised" License
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.
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.
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.
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 ?
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]
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.
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.
migrate from jyao1/openspdm#178 and jyao1/openspdm#176
See detail discussion there.
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.
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.
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?
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.
Current spdm_verify_peer_cert_chain_buffer() assumes the first cert is root cert.
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.
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.
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.
libspdm/library/spdm_common_lib/crypto_service_session.c
Lines 226 to 243 in 631028a
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.
libspdm/library/spdm_common_lib/crypto_service_session.c
Lines 82 to 84 in 631028a
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#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
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.
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.
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);
@steven-bellock will propose a set of secure code review guidelines tailored towards SPDM.
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.
Develop fuzzing state machine as a document to help with fuzz testing.
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;
}
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.
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;
}
......
bash_hash_algo
should be renamed to base_hash_algo
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.
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;
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.
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
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.
Examples include:
From jyao1/openspdm#170
For example: all session states, negotiated algo, etc,
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.
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.
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.
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?
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?
libspdm/library/spdm_requester_lib/get_digests.c
Lines 150 to 157 in ad088c1
libspdm/library/spdm_common_lib/crypto_service.c
Lines 361 to 378 in ad088c1
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.
And if we can't think of a good reason, then let's use types from <stdint.h>.
It's in spdm_set_data and spdm_get_data but no other code uses it.
The spec says that hashes of certificate chains, for example DIGESTS.digest, need to be big-endian. Is libspdm consistent with this?
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()
{
SpdmGetResponseFoo()
{
The current responder code for MEASUREMENT signature generation in the other hand implemented the similar requirement.
SpdmGetResponseMeasurement()
{
A declarative, efficient, and flexible JavaScript library for building user interfaces.
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
An Open Source Machine Learning Framework for Everyone
The Web framework for perfectionists with deadlines.
A PHP framework for web artisans
Bring data to life with SVG, Canvas and HTML. πππ
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
Some thing interesting about web. New door for the world.
A server is a program made to process requests and deliver data to clients.
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
Some thing interesting about visualization, use data art
Some thing interesting about game, make everyone happy.
We are working to build community through open source technology. NB: members must have two-factor auth.
Open source projects and samples from Microsoft.
Google β€οΈ Open Source for everyone.
Alibaba Open Source for everyone
Data-Driven Documents codes.
China tencent open source team.