openssl: X509_PUBKEY_cmp() seems incorrect

There is static function X509_PUBKEY_cmp used by OSSL_CRMF_MSGS_verify_popo in crypto/crmf/crmf_lib.c". I wanted to copy-paste it for some gost-engine tests (since it’s not exported), but found it’s being strange:

/* returns 0 for equal, -1 for a < b or error on a, 1 for a > b or error on b */
static int X509_PUBKEY_cmp(X509_PUBKEY *a, X509_PUBKEY *b)
{
    X509_ALGOR *algA = NULL, *algB = NULL;
    int res = 0;

    if (a == b)
        return 0;
    if (a == NULL || !X509_PUBKEY_get0_param(NULL, NULL, NULL, &algA, a)
            || algA == NULL)
        return -1;
    if (b == NULL || !X509_PUBKEY_get0_param(NULL, NULL, NULL, &algB, b)
            || algB == NULL)
        return 1;
    if ((res = X509_ALGOR_cmp(algA, algB)) != 0)
        return res;
    return EVP_PKEY_cmp(X509_PUBKEY_get0(a), X509_PUBKEY_get0(b));
}

It should return 0 if keys match, but EVP_PKEY_cmp will return 1 if keys match.

Man: The function EVP_PKEY_cmp_parameters() and EVP_PKEY_cmp() return 1 if the keys match, 0 if they don't match, -1 if the key types are different and -2 if the operation is not supported..

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 28 (25 by maintainers)

Commits related to this issue

Fix X509_PUBKEY_cmp(), move to crypto/x509/x_pubkey.c, rename, export, and document it Fixes #11870 — committed to siemens/openssl by DDvO 4 years ago

Most upvoted comments

Good point @petrovr - it’s a pity that EVP_PKEY_cmp() and EVP_PKEY_cmp_parameters() are not called EVP_PKEY_eq() and EVP_PKEY_eq_parameters(), respectively.

This would avoid a lot confusion and would have .avoided the bug found by @vt-alt.

Isn’t it worth introducing these renaming here and now, altogether with deprecating the old ones as misleading?

beldmit on May 21, 2020

I do not think it makes sense to rename _cmp functions that return 0 for equality. And (hopefully) there are not many more than the EVP_PKEY_cmp* that return 1 for equality.

t8m on May 25, 2020

The difference between EVP_PKEY_cmp and EC_POINT_cmp is that the first one returns 1 for equal where the second returns 0 for equal. So no, EC_POINT_cmp does not need to be renamed.

But there is also EVP_PKEY_cmp_parameters, which has the same semantics as EVP_PKEY_cmp and so it should be probably renamed as well.

t8m on May 25, 2020

How to do this best? Can I rename for instance EVP_PKEY_cmp() to EVP_PKEY_eq() and add a `#define for backward compatibility?

If you do that the define should be within the #ifndef OPENSSL_NO_DEPRECATED_3_0. Or you can even do EVP_PKEY_cmp() as a thin wrapper function - that has an advantage of producing deprecated warnings if used in the code.

t8m on May 25, 2020