Skip to content

Check parameters before comparing pqdsa public keys #3229

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.

Sign up for GitHub

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

Merged
geedo0 merged 1 commit into from
May 12, 2026
+41 −5
Merged

Check parameters before comparing pqdsa public keys #3229

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions crypto/evp_extra/p_kem_asn1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,14 @@ static int kem_get_pub_raw(const EVP_PKEY *pkey, uint8_t *out,
return 1;
}

// kem_cmp_parameters returns 1 if |a| and |b| hold populated KEM keys with
// the same KEM NID, 0 if their NIDs differ, or -2 if either operand is
// missing its key or parameters. The tri-state return aligns with the
// |EVP_PKEY_cmp| convention (1 = equal, 0 = not equal, negative = error).
static int kem_cmp_parameters(const EVP_PKEY *a, const EVP_PKEY *b) {
if (a == NULL || b == NULL) {
return -2;
}
const KEM_KEY *a_key = a->pkey.kem_key;
const KEM_KEY *b_key = b->pkey.kem_key;
if (a_key == NULL || b_key == NULL) {
Expand Down
39 changes: 34 additions & 5 deletions crypto/evp_extra/p_pqdsa_asn1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,13 +137,42 @@ static int pqdsa_pub_encode(CBB *out, const EVP_PKEY *pkey) {
return 1;
}

// pqdsa_cmp_parameters returns 1 if |a| and |b| hold populated PQDSA keys
// with the same ML-DSA NID, 0 if their NIDs differ, or -2 if either operand
// is missing its key or parameters. The tri-state return aligns with the
// |EVP_PKEY_cmp| convention (1 = equal, 0 = not equal, negative = error).
static int pqdsa_cmp_parameters(const EVP_PKEY *a, const EVP_PKEY *b) {
if (a == NULL || b == NULL) {
return -2;
}
const PQDSA_KEY *a_key = a->pkey.pqdsa_key;
Copy link
Copy Markdown
Contributor

@WillChilds-Klein WillChilds-Klein May 11, 2026

Choose a reason for hiding this comment

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

do we need NULL checks on a and b here before the dereference?

also, can we document this functions return values in a comment above? why return -2 instead of 0? callers might inadvertently do !pqdsa_cmp_parameters(...) and have unexpected behavior

Copy link
Copy Markdown
Contributor Author

@samuel40791765 samuel40791765 May 11, 2026

Choose a reason for hiding this comment

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

Added NULL checks and a doc comment.

I was confused about this at first too, but the tri-state return is intentional to align with EVP_PKEY_cmp's convention (1 = equal, 0 = not equal, negative = error). Also mirrored the same NULL check and docstring onto kem_cmp_parameters since it's applicable there as well.

aws-lc/include/openssl/evp.h

Lines 56 to 61 in 9651480

// EVP_PKEY_cmp compares |a| and |b| and returns one if they are equal, zero if
// not and a negative number on error.
//
// WARNING: this differs from the traditional return value of a "cmp"
// function.
OPENSSL_EXPORT int EVP_PKEY_cmp(const EVP_PKEY *a, const EVP_PKEY *b);

Copy link
Copy Markdown
Contributor

@WillChilds-Klein WillChilds-Klein May 11, 2026

Choose a reason for hiding this comment

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

thanks. if we need to return negative on error, -2 still "feels" like an odd return value vs. -1 but if "negative" is the documented contract it doesn't really matter.

const PQDSA_KEY *b_key = b->pkey.pqdsa_key;
if (a_key == NULL || b_key == NULL) {
return -2;
}

const PQDSA *a_pqdsa = a_key->pqdsa;
const PQDSA *b_pqdsa = b_key->pqdsa;
if (a_pqdsa == NULL || b_pqdsa == NULL) {
return -2;
}

return a_pqdsa->nid == b_pqdsa->nid;
}

static int pqdsa_pub_cmp(const EVP_PKEY *a, const EVP_PKEY *b) {
PQDSA_KEY *a_key = a->pkey.pqdsa_key;
PQDSA_KEY *b_key = b->pkey.pqdsa_key;
int ret = pqdsa_cmp_parameters(a, b);
if (ret <= 0) {
return ret;
}

return OPENSSL_memcmp(a_key->public_key,
b_key->public_key,
a->pkey.pqdsa_key->pqdsa->public_key_len) == 0;
const PQDSA_KEY *a_key = a->pkey.pqdsa_key;
const PQDSA_KEY *b_key = b->pkey.pqdsa_key;
if (a_key->public_key == NULL || b_key->public_key == NULL) {
return -2;
}
return OPENSSL_memcmp(a_key->public_key, b_key->public_key,
a_key->pqdsa->public_key_len) == 0;
}

static int pqdsa_priv_decode(EVP_PKEY *out, CBS *oid, CBS *params, CBS *key, CBS *pubkey) {
Expand Down
Loading