Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question about libspdm_check_request_flag_compatibility #2970

Open
rw8896 opened this issue Jan 29, 2025 · 5 comments
Open

Question about libspdm_check_request_flag_compatibility #2970

rw8896 opened this issue Jan 29, 2025 · 5 comments
Labels
question Further information is requested

Comments

@rw8896
Copy link
Contributor

rw8896 commented Jan 29, 2025

Hi,

I was wondering about the following check in libspdm_check_request_flag_compatibility().

if ((chal_cap == 0) && (key_ex_cap == 0)) {
return false;
}

I guess this check is added based on the following 1.3 spec change. If that's case, should we check this only when version >= 1.3?

Image

@steven-bellock steven-bellock added the bug Something isn't working label Jan 29, 2025
@steven-bellock
Copy link
Contributor

I guess this check is added based on the following 1.3 spec change.

No, that was added before SPDM 1.3 in #1512. The rationale is that if the Responder knows the Requester's public key then the Responder needs to be able to verify it, either through key exchange or challenge (which is deprecated). However there is an issue with SPDM 1.3 and the current check. In particular if a Requester sets EP_INFO==2 then that's another way to sign the message.

Note that this check might be overzealous. In particular there might be a Requester that only signs VDMs, in which case the Responder would reject its capabilities.

@rw8896
Copy link
Contributor Author

rw8896 commented Jan 29, 2025

No, that was added before SPDM 1.3 in #1512. The rationale is that if the Responder knows the Requester's public key then the Responder needs to be able to verify it, either through key exchange or challenge (which is deprecated). However there is an issue with SPDM 1.3 and the current check. In particular if a Requester sets EP_INFO==2 then that's another way to sign the message.

Thanks for background but why would the Responder always need to be able to verify the public key? The Responder might not support KEY_EXCHANGE at all.
Maybe we should move the check to where the Responder really needs the public key?

@steven-bellock
Copy link
Contributor

Maybe we should move the check to where the Responder really needs the public key?

There is no check because there would be nothing to do. From the Responder's point-of-view what does a Requester with

  • CERT_CAP==1
  • KEY_EX_CAP==0
  • CHAL_CAP==0
  • EP_INFO_CAP==0

mean? The Responder can somehow get the Requester's certificate chain and public key, but there are no messages for the Requester to sign with the private key, and no messages for the Responder to verify with the public key. Seems like an illegal combination.

@rw8896
Copy link
Contributor Author

rw8896 commented Jan 29, 2025

But it is still a valid combination in the spec and should not fail the capability check.
What looks invalid is when the Requester supports KEY_EX or CHAL but has no public key. And even in this case, if the Responder doesn't support or want to verify the Requester, it seems okay for the Responder to accept the capabilities.

@steven-bellock
Copy link
Contributor

But it is still a valid combination in the spec and should not fail the capability check.

As you pointed out, in SPDM 1.3 it would (probably) be more explicitly illegal. But the logic still stands for the specifications before 1.3.

And even in this case, if the Responder doesn't support or want to verify the Requester, it seems okay for the Responder to accept the capabilities.

The rationale is that an endpoint does not want to talk to a buggy or misconfigured endpoint. If GET_CAPABILITIES is messed up, who knows what other bad messages may come the Responder's way.

This had been previously discussed in #2329 (comment) and I floated the idea of LIBSPDM_DISABLE_STRICT_CHECKS to make things more permissive. We can resurrect that "enhancement" if it's deemed necessary. It would disable checks that aren't explicitly in the specification, but that are derived from the specification and/or make a lot of sense.

@steven-bellock steven-bellock added question Further information is requested and removed bug Something isn't working labels Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants