Skip to content

Check signing algo in COSE attached verify#7476

Merged
achamayou merged 13 commits intomainfrom
check_signing_algo_in_cose_attached_verify
Nov 24, 2025
Merged

Check signing algo in COSE attached verify#7476
achamayou merged 13 commits intomainfrom
check_signing_algo_in_cose_attached_verify

Conversation

@achamayou
Copy link
Copy Markdown
Member

@achamayou achamayou commented Nov 20, 2025

Prompted by #7468, and includes #7468 after further further checks.

This change uncovered what was effectively an abuse of the ECPublicKey_OpenSSL class to cover both EC and RSA verification. The following changes have been made:

  1. a generic PublicKey_OpenSSL has been added, to wrap either kind/all kinds of EVP_PKEY. This works well with t_cose, which also does.
  2. ECPublicKey_OpenSSL construction now fails if the input is an RSA key, matching RSAPublicKey_OpenSSL
  3. Both ECPublicKey_OpenSSL and RSAPublicKey_OpenSSL now inherit from PublicKey_OpenSSL to reduce code duplication. This may or may not be a good thing, as discussed with @maxtropets, but is an internal implementation choice that is not exposed in the public headers and can be adjusted at a later time if desired.

@achamayou achamayou requested a review from a team as a code owner November 20, 2025 14:01
Copilot AI review requested due to automatic review settings November 20, 2025 14:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds algorithm verification to the COSE Sign1 verification process by checking that the algorithm specified in the COSE message header matches the algorithm supported by the verification key.

Key Changes

  • Extracts the algorithm identifier from the COSE message's protected header
  • Derives the expected algorithm identifier from the public key's curve type
  • Validates that both algorithms match before proceeding with signature verification
  • Returns early with an error if algorithms are incompatible or cannot be determined

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@eddyashton
Copy link
Copy Markdown
Member

Looks like we do the same prefix of checks in verify_detached() - can we refactor this to avoid the duplication?

@achamayou
Copy link
Copy Markdown
Member Author

Ok, adding this apparently benign check has uncovered the following problems:

  1. UVM endorsements are sometimes signed with RSA keys
  2. That is ostensibly unsupported when constructing from a vector, whether it's DER or PEM (!!): https://github.com/microsoft/CCF/blob/main/src/crypto/openssl/cose_verifier.cpp#L129
  3. And at first glance, it looks building from a crypto::Pem is subject to the same restriction: https://github.com/microsoft/CCF/blob/main/src/crypto/openssl/cose_verifier.cpp#L137
  4. But it's not, really, there are no checks at all:
    ECPublicKey_OpenSSL::ECPublicKey_OpenSSL(const Pem& pem)
  5. This works for verification (OpenSSL handles that just fine), but obviously getting the curve from an object that thinks it's an EC Key but really holds an RSA EVP_PKEY does not work:
    CurveID ECPublicKey_OpenSSL::get_curve_id() const

@achamayou achamayou marked this pull request as draft November 20, 2025 18:37
@achamayou achamayou marked this pull request as ready for review November 21, 2025 12:57
@achamayou achamayou enabled auto-merge (squash) November 24, 2025 11:25
@achamayou achamayou merged commit 6a60b6f into main Nov 24, 2025
17 checks passed
@achamayou achamayou deleted the check_signing_algo_in_cose_attached_verify branch November 24, 2025 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants