need robust way to handle key_update

[jyao1]

from intel/rust-spdm#27

if key_update message failed, we need rollback to original keys.

[OuyangHang33]

Table-1 Expected Behaviors when key update fail

Key Update Operation	Msg Header	Type	Expected Behaviors when get decodeable error msg	Expected Behaviors when get undecryptable garbage msg
UpdateAllkeys	KEY_UPDATE tag1	request	S2 and S3 in Rsp are not updated. if S2 old key can decode err msg, then Rsp send key_update err msg with S2 old key to Req for rolling back S2 and S3 in Req. Responder keys don't need backup.	no keys can decode msg, responder send CRYPTO_ERROR, waiting Req.
UpdateAllkeys	KEY_UPDATE_ACK tag1	response	S2 and S3 in Req are updated and old keys are backuped. if S3new key can decode msg and it means key_update fail, then rollback to old keys. if S3 old key can decode msg, then rollback to S3 old key directly	no keys can decode msg, need to end session.
UpdateSingleKey	KEY_UPDATE tag1	request	S2 in Rsp is not updated. if S2 old key can decode err msg, then Rsp send key_update err msg with S2 old key to Req for rolling back S2 in Req.	no keys can decode msg, responder send CRYPTO_ERROR, waiting Req.
UpdateSingleKey	KEY_UPDATE_ACK tag1	response	S2 in Req are updated and S2 old key is backuped. if S3 can decod msg and it means key_update fail, then rollback to S2 old key.	no keys can decode msg, need to end session.
VerifyNewKey	KEY_UPDATE tag2	request	S2 in Rsp is updated. if S2 new can decoded msg then handle the err normally. if S2 old can decoded msg, the S2 back up can't be dropped. Keep session working, if Rsp send the message created by err handle function, then need to wait Req next request	no keys can decode msg, responder send CRYPTO_ERROR, waiting Req.
VerifyNewKey	KEY_UPDATE_ACK tag2	response	S3 in Req is updated and old keys are dropped. if S3 new can decoded msg then handle the err normally. Keep session working, sender can retry the request	no keys can decode msg, need to end session

[OuyangHang33]

Table-2.1 Test UpdateAllKeys operation on SPDM-rs

Operation	type	Expected Behaviors when fail	Responder (Rsp) Actual Behaviors	Request (Req) Actual Behaviors
UpdateAllkeys	request (send unknow request)	use old key, rsp should return Err, req must do rollback	receive unkonw update key request msg, key update fail and rsp return 12 7f 01 00 (no bug)	After Rsp handle the unknow request from Req, Req can't decode err msg from Rsp (bug, #79 fix). After Req decode err msg and rolling back keys, the next operation get measure fail (bug, #81 fix )
UpdateAllkeys	response (modify msg)	use old key, rsp no Err, req must do rollback	send rsp no err (no bug)	rollback old key without checking backup is empty or not(bug, #76 fix). get measure fail (no bug see #7 (comment))
VerifyNewKey	request (send unknow request)	rsp return Err. req get Err and keep session working	rsp return 12 7f 01 00. get measure success (no bug)	send req no err, can receive err msg, get measure success (no bug)
VerifyNewKey	response (modify msg)	rsp no Err, req keep session working, keys backup should not be cleanup. Sender can retry the request	Rsp dropped both S2 and S3 keys (bug, #82 fix). rsp send no err msg (no bug)	return verify fail. Get measure success. (no bug)

Table-2.2 Test UpdateAllKeys operation on SPDM-EMU(libspmd)

Operation	type	Expected Behaviors when fail	Responder (Rsp) Actual Behaviors	Request (Req) Actual Behaviors
UpdateAllkeys	request (send unknow request)	use old key, rsp should return Err, req must do rollback	send unkonw and update key, then fail and rsp return 12 7f 01 00(no bug)	can receive err msg. rollback old keys success. Get measure success (no bug)
UpdateAllkeys(no bug)	response (modify msg)	use old key, rsp no Err, req must do rollback	rsp no err. (no bug)	return update fail, and rollback old keys, but get measure fail. ( no bug see #7 (comment))
VerifyNewKey	request (send unknow request)	rsp return Err. req get Err and keep session working	rsp return 12 7f 01 00. get measure success (no bug)	send req no err, can receive err msg, get measure success (no bug)
VerifyNewKey	response (modify msg)	rsp no Err, req keep session working, keys backup should not be cleanup. Sender can retry the request	rsp send no err msg (no bug)	return verify fail. Get measure success. (no bug)

[OuyangHang33]

Regards to this scenario: When SPDM-rs Requester create new keys and send wrong KeyUpdate operation, it can't receive Err msg from responder.

Root cause: responder keys between Req and Rsp are dismatched. SPDM-rs only try decode_spdm_secured_message with current keys, and rolling back to backup keys only happends after receiving Err msg. It shall try rollback keys when decoding msg fail.

spdm-rs/spdmlib/src/common/session.rs

Lines 897 to 916 in 003a498

	SpdmSessionState::SpdmSessionEstablished => {
	if is_requester {
	let r = self.decode_msg(
	secured_buffer,
	app_buffer,
	&self.application_secret.request_direction,
	);
	if r != Err(SPDM_STATUS_SEQUENCE_NUMBER_OVERFLOW) {
	self.application_secret.request_direction.sequence_number += 1;
	}
	r
	} else {
	let r = self.decode_msg(
	secured_buffer,
	app_buffer,
	&self.application_secret.response_direction,
	);
	if r != Err(SPDM_STATUS_SEQUENCE_NUMBER_OVERFLOW) {
	self.application_secret.response_direction.sequence_number += 1;
	}

Libspdm give a good example libspdm_receive_response to solve this problem:
https://github.com/DMTF/libspdm/blob/3bbcb24dbae81a25cb04dd185b0fe948e6ac6b25/library/spdm_requester_lib/libspdm_req_send_receive.c#L194-L197

 if (status == LIBSPDM_STATUS_SESSION_TRY_DISCARD_KEY_UPDATE) {
        /* Failed to decode, but have backup keys. Try rolling back before aborting.
         * message_session_id must be valid for us to have attempted decryption. */
        if (message_session_id == NULL) {
            return LIBSPDM_STATUS_INVALID_STATE_LOCAL;
        }
        temp_session_context = libspdm_get_secured_message_context_via_session_id(
            context, *message_session_id);
        if (temp_session_context == NULL) {
            return LIBSPDM_STATUS_INVALID_STATE_LOCAL;
        }

        result = libspdm_activate_update_session_data_key(
            temp_session_context, LIBSPDM_KEY_UPDATE_ACTION_RESPONDER, false);
        if (!result) {
            return LIBSPDM_STATUS_INVALID_STATE_LOCAL;
        }

        /* Retry decoding message with backup Requester key.
         * Must reset some of the parameters in case they were modified */
        message_session_id = NULL;
        is_message_app_message = false;
        *response = backup_response;
        *response_size = backup_response_size;
        status = context->transport_decode_message(
            context, &message_session_id, &is_message_app_message,
            false, message_size, message, response_size, response);

        reset_key_update = true;
    }

[OuyangHang33]

For this scenario:

Key_Update_Operation	Msg Header	type	Expected Behaviors when fail
UpdateAllkeys	KEY_UPDATE_ACK tag1	response	use old key, if S2 and S3 updated, must do rollback. sender keys backup should be keeped

Test scenario:

After responder receive the correct UpdateAllkeys request and handle the key update operation (Update S2new and S3new, and write response with S3new), the requester get the incomplete message (like origin is 12, 69, 01, 01, but get 12, 69, 01), the requester will do rollback keys.
For requester, S2new and S3new is not activated, S2new and S3new will be rollback to old one.
For responder, S3new is activated, S3 old key has been droped, so only S2new will be rollback to old one.

Result:

Requester get old S3 key, and responder get S3 new key, dismatched, other operation will be fail like get_measurement.

Question:

From spec v1.2.1 p.p.131:

1. The sender shall not use the new transmit key until after reception of the KEY_UPDATE_ACK response.
2. If the sender has not received KEY_UPDATE_ACK , the sender can retry or end the session. The sender shall not proceed to the next step until successfully receiving the corresponding KEY_UPDATE_ACK .

Does it mean that if retry fail, session should be end when meet above scenario?

[jyao1]

I think we only need to consider below failure scenario:

1. An ERROR message that can be decrypted successfully.
2. A garbage message that cannot be decrypted by old key or new key.
3. No message is received before timeout. - Similar to garbage message.

We don't need to consider the situation that attacker can create an invalid message that can be decrypted successfully. If the attacker has such capability, then there will be no security any more.