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

Crypto API: deprecating commonly used functions severely decreases ease of use #14628

Closed
DDvO opened this issue Mar 20, 2021 · 60 comments
Closed
Labels
triaged: documentation The issue/pr deals with documentation (errors) triaged: feature The issue/pr requests/adds a feature triaged: OTC evaluated This issue/pr was triaged by OTC

Comments

@DDvO
Copy link
Contributor

DDvO commented Mar 20, 2021

Currently related PRs:

Over the last two days I've been struggling with making code that includes many rather superficial uses of OpenSSL 1.1.x ready for the upcoming version 3.0.
Taking deprecation warnings serious and cleanly getting rid of them was a PITA.
For many cases, user guidance, as well as library support for the new preferred style is still missing or rather poor.

Let me give a couple of examples.

  • Attempting to re-compile older code reveals that functions SHA256() and the like have been deprecated. Then one looks around for alternatives. The man page SHA256_Init.pod says:
All of the functions described on this page are deprecated.
Applications should instead use L<EVP_DigestInit_ex(3)>, L<EVP_DigestUpdate(3)>
and L<EVP_DigestFinal_ex(3)>.

Similarly, CHANGES.md says:

 * All of the low level MD2, MD4, MD5, MDC2, RIPEMD160, SHA1, SHA224, SHA256,
   SHA384, SHA512 and Whirlpool digest functions have been deprecated.
   These include:

   MD2, MD2_options, MD2_Init, MD2_Update, MD2_Final, MD4, MD4_Init,
   [...]

   Use of these low level functions has been informally discouraged
   for a long time.  Applications should use the EVP_DigestInit_ex(3),
   EVP_DigestUpdate(3) and EVP_DigestFinal_ex(3) functions instead.

Reading this, any - typically anyway heavy loaded - programmer will wonder:
For my innocent SHA hash value calculation, why do I suddenly have to bother using a combination of EVP_Digest* functions, EVP_MD* types, and related error conditions - OpenSSL guys, are you serious?

Digging around in OpenSSL lib, apps, and test code, I eventually found EVP_Digest(), which can serve as a reasonable replacement of
SHA256(data, count, md_buf) as follows (ignoring the result value here):
EVP_Digest(data, count, md_buf, NULL, EVP_sha256(), NULL).
Still it's cumbersome to change all occurrences of SHA*() by suitable invocations of EVP_Digest().
Why does OpenSSL not give the most typical use cases at least practically helpful hints, or even better provide automatically usable compatibility definitions?
In this case the old declaration

unsigned char *SHA256(const unsigned char *d, size_t n, unsigned char *md);

could be replaced by

#define SHA256(data, count, md_buf) \
(EVP_Digest(data, count, md_buf, NULL, EVP_sha256(), NULL), md_buf)
  • For HMAC() the deprecation situation is even worse.
    HMAC.pod just states that this and related functions are deprecated, but gives no hints.
    CHANGES.md gives some hints similar to those mentioned above:
Use of these low level functions has been informally discouraged for a long
   time.  Instead applications should use L<EVP_MAC_CTX_new(3)>,
   L<EVP_MAC_CTX_free(3)>, L<EVP_MAC_init(3)>, L<EVP_MAC_update(3)>
   and L<EVP_MAC_final(3)>.

I was not able to find a rather direct replacement, so I ended up defining one myself:

#if OPENSSL_VERSION_NUMBER >= 0x30000000L
unsigned char *v3_0_MAC(OSSL_LIB_CTX *libctx, const char *mac_name,
                        const char *propq, const char *digest_name,
                        const void *key, int key_len,
                        const unsigned char *data, size_t data_len,
                        unsigned char *md, unsigned int *md_len,
                        size_t outsize)
{
    EVP_MAC *mac;
    EVP_MAC_CTX *mctx;
    OSSL_PARAM macparams[2] =
        { OSSL_PARAM_construct_utf8_string("digest", (char *)digest_name, 0),
          OSSL_PARAM_END };
    size_t res_len;
    unsigned char *res = NULL;

    if ((mac = EVP_MAC_fetch(libctx, mac_name, propq)) != NULL
            && (mctx = EVP_MAC_CTX_new(mac)) != NULL
            && EVP_MAC_CTX_set_params(mctx, macparams)
            && EVP_MAC_init(mctx, key, key_len, macparams)
            && EVP_MAC_update(mctx, data, data_len)
            && EVP_MAC_final(mctx, md, &res_len, outsize))  {
        res = md;
        if (md_len != NULL)
            *md_len = (unsigned int)res_len;
    }
    EVP_MAC_CTX_free(mctx);
    EVP_MAC_free(mac);
    return res;
}
#endif

By inserting the following preprocessor trick:

#if OPENSSL_VERSION_NUMBER >= 0x30000000L
#define HMAC(evp_md, key, key_len, data, data_len, md, md_len) \
    v3_0_MAC(NULL, "HMAC", NULL, EVP_MD_name(evp_md), \
                     key, key_len, data, data_len, md, md_len, EVP_MD_size(evp_md))
#endif

I was able to keep the original code sections that call HMAC() untouched.

In order to avoid a clash with the current deprecation declaration of HMAC(),
at first I did this in a place not followed by an inclusion of hmac.h.
Then I realized this is better done using

#define OPENSSL_API_COMPAT 30000
#define OPENSSL_NO_DEPRECATED
  • For RSA and EC key generation, apparently most hitherto used functions are deprecated, but this is not even mentioned in doc files like RSA_new.pod and EC_KEY_new.pod.
    Their deprecation is just mentioned in CHANGES.md, where for RSA no replacement hints are given. For EC at least some hints are given:
   For replacement of the functions manipulating the EC_KEY objects
   see the EVP_PKEY-EC(7) manual page.

Since I did not find any simply readily usable functions, I defined them myself:

#if OPENSSL_VERSION_NUMBER >= 0x30000000L
static EVP_PKEY *rsa_keygen(int bits)
{
    EVP_PKEY *pkey = NULL;
    EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_from_name(NULL, "RSA", NULL);

    if (ctx != NULL
        && EVP_PKEY_keygen_init(ctx) > 0
        && EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, bits))
    {
        (void)EVP_PKEY_keygen(ctx, &pkey);
    }
    EVP_PKEY_CTX_free(ctx);
    return pkey;
}

static EVP_PKEY *ec_keygen(int nid)
{
    EVP_PKEY *pkey = NULL;
    EVP_PKEY_CTX *ctx = EVP_PKEY_CTX_new_from_name(NULL, "EC", NULL);

    if (ctx != NULL
        && EVP_PKEY_keygen_init(ctx) > 0
        && EVP_PKEY_CTX_set_ec_paramgen_curve_nid(ctx, nid))
    {
        EVP_PKEY_gen(ctx, &pkey);
    }
    EVP_PKEY_CTX_free(ctx);
    return pkey;
}
#endif

This code is entirely different to the even more alchemist way of doing the key generation using the pre-3.0. API.
I really wonder why nobody within OpenSSL bothered providing user-friendly key generation functions.
These would also be helpful in various places in the OpenSSL libs, apps, and tests, themselves.

Unless transition is significantly alleviated, user acceptance is put at risk.

@DDvO DDvO added issue: feature request The issue was opened to request a feature issue: documentation The issue reports errors in (or missing) documentation issue: bug report The issue was opened to report a bug labels Mar 20, 2021
@mattcaswell mattcaswell added triaged: documentation The issue/pr deals with documentation (errors) triaged: feature The issue/pr requests/adds a feature and removed issue: bug report The issue was opened to report a bug issue: documentation The issue reports errors in (or missing) documentation issue: feature request The issue was opened to request a feature labels Mar 22, 2021
@mattcaswell mattcaswell added this to the 3.0.0 beta1 milestone Mar 22, 2021
@paulidale
Copy link
Contributor

I agree, helper functions would be really nice. I don't think we'll hold back beta for this. Likewise for the documentation. Now, if someone came along and presented a PR, we'd likely include it.

For MAC, I'd change digest_name to sub_algorithm_name (or something) since this covers GMAC and CMAC in addition to HMAC.

I'd also consider adding an OSSL_PARAM pointer to the function to allow a user to set additional things without diving into the curious details. I don't know if this is over complicating things.

@t-j-h
Copy link
Member

t-j-h commented Mar 23, 2021

We should be looking for a common prefix for the "helper" functions - and this is something that should consistent across all the APIs of this form. Perhaps EVP_Q_?

@DDvO
Copy link
Contributor Author

DDvO commented Mar 23, 2021

I've already started preparing a PR that includes at least

  • some quick fixes/extensions of deprecation hints in CHANGES.md and docs
  • an easy-to-use *MAC "helper" function -
    how about EVP_MAC_calc(), resembling EVP_MAC_init() and friends ?
  • high-level RSA+EC key gen functions

@DDvO
Copy link
Contributor Author

DDvO commented Mar 23, 2021

Using the new EVP_MAC_calc() helper function
will also remove needless clutter and errors from current OpenSSL lib, apps, and test code.
For instance, commit dbde472 forgot to call in apps/lib/s_cb.c:

    EVP_MAC_CTX_free(ctx);
    EVP_MAC_free(hmac);

As a side effect, the upcoming PR will thus also fix these mem leaks.

@levitte
Copy link
Member

levitte commented Mar 23, 2021

I'd also consider adding an OSSL_PARAM pointer to the function to allow a user to set additional things without diving into the curious details. I don't know if this is over complicating things.

Agreed. That pile of arguments to v3_0_MAC is scary, and highly MAC implementation specific...

@mattcaswell mattcaswell added the triaged: OTC evaluated This issue/pr was triaged by OTC label Mar 23, 2021
@DDvO
Copy link
Contributor Author

DDvO commented Mar 23, 2021

So I'll go for

unsigned char *EVP_MAC_calc(EVP_MAC *mac, const OSSL_PARAM *params,
                            const void *key, int keylen,
                            const unsigned char *data, size_t datalen,
                            unsigned char *out, size_t outsize,
                            unsigned int *outlen)

and some instances like OSSL_HMAC().

@DDvO
Copy link
Contributor Author

DDvO commented Mar 23, 2021

Why exactly are convenient functions like HMAC() deprecated?
Can't we keep them, just using a more modern implementation such as EVP_MAC_calc()?

@t8m
Copy link
Member

t8m commented Mar 23, 2021

The HMAC() call has an EVP_MD as a parameter which does not fit well with the EVP_MAC API. So no, this should not be kept non-deprecated. Also the name is asking for conflicts.

@levitte
Copy link
Member

levitte commented Mar 23, 2021

Why exactly are convenient functions like HMAC() deprecated?

I would argue that we want to stop locking ourselves to specific algorithms, and look to generic functionality that can serve any algo served by providers.

@DDvO
Copy link
Contributor Author

DDvO commented Mar 23, 2021

Why exactly are convenient functions like HMAC() deprecated?

I would argue that we want to stop locking ourselves to specific algorithms, and look to generic functionality that can serve any algo served by providers.

It's a valid aim to be fully general and future-proof, and I see the point of achieving this for 3.0.
Yet in practice this comes at a price - not only for the implementors, but also for the users!

There are surely some advanced users who are willing to complicate(!) their applications by switching to the new API because they appreciate the flexibility they gain this way and hopefully have the resources to deal with that.

BUT: I am pretty sure that the vast majority of users won't.
Most of them are more or less casual users of cryptography who have little interest and resources for diving into sophisticated ways of specifying algorithms and their parameters,
just for the (in their limited crypto experience) theoretical advantage of being more flexible.

I strongly fear that by trying to

  • force existing users to adapt their code base to the new paradigm and
  • trying to win many new users by the crypto-technical beauty of the new API

will not work and rather lead to an overall loss of acceptance.

Please note that I do not aim at arguing or working against the new API,
but at reaching out a helping hand to the user base by providing a convenience layer that

  • allows (re-)using existing application code basically as it is also with OpenSSL 3.0 and
  • lowers the bar for new applications when starting to use OpenSSL 3.0 and beyond.

@levitte
Copy link
Member

levitte commented Mar 23, 2021

@DDvO, it sounds like you're suggesting that SOMETHING_mac("HMAC", ...) it a very hard replacement compared to SOMETHING_HMAC(...).

I'm not trying to say that we shall not have these easy enough functions, I'm asking for a little bit of remaining generality.

@DDvO
Copy link
Contributor Author

DDvO commented Mar 24, 2021

Well, my response was aimed also at #14664 (comment) which asked for using the flexible OSSL_PARAM technique, and was also meant as a more general background motivation for my original post above, but likely this was not really needed since we (all) seem to basically agree here.

Sure SOMETHING_mac("HMAC", ...) is not really more complex than SOMETHING_HMAC(...). But still deprecating and eventually removing/forbidding the latter will force users to adapt their code, or at least to come up with and make use of awkward forward compatibility tricks such as

#if OPENSSL_VERSION_NUMBER >= 0x30000000L
# define HMAC(evp_md, key, keylen, data, datalen, out, outlen) \
    (EVP_MAC("HMAC", EVP_MD_name(evp_md), \
             key, keylen, data, datalen, out, EVP_MD_size(evp_md), outlen), \
     data)
#endif

Do we really want this?

@levitte
Copy link
Member

levitte commented Mar 24, 2021

But still deprecating and eventually removing/forbidding the latter will force users to adapt their code

Unless you plan on restoring the exact functions that are deprecated, they need to adapt their code anyway, so I really fail to see how adapting to a name string based call isn't at least as acceptable as a call where the intended algo is part of the function name.

@DDvO
Copy link
Contributor Author

DDvO commented Mar 24, 2021

But still deprecating and eventually removing/forbidding the latter will force users to adapt their code

Unless you plan on restoring the exact functions that are deprecated,

As mentioned above, there are also other ways, like using the modern implementation underneath and adding a wrapper declaration, either as a simple proper C function or as a macro.

they need to adapt their code anyway, so I really fail to see how adapting to a name string based call isn't at least as acceptable as a call where the intended algo is part of the function name.

What you fail to see is that even such innocent/trivial adaptations can be painful:
As mentioned, existing code will have to be touched, which can cause major further processing such as re-publishing code, re-reviewing it, and even re-certification -
and all of this maybe just because an application needs to switch to using a new OpenSSL version for maintenance and/or security reasons.

Another problem is that sometimes code needs to compile with OpenSSL versions both pre-3.0 and 3.0+.
Then as mentioned one would have to adapt that code forth and back depending on which OpenSSL version is used beneath, or one would have to add a compatibility layer with ugly hacks such as

#if OPENSSL_VERSION_NUMBER >= 0x30000000L
# define HMAC(evp_md, key, keylen, data, datalen, out, outlen) \
    (EVP_MAC("HMAC", EVP_MD_name(evp_md), \
             key, keylen, data, datalen, out, EVP_MD_size(evp_md), outlen), \
     data)
#endif

I developed a security utilities library on top of OpenSSL which is now facing exactly this issue,
so I know what I am talking about. The issue is not a theoretical fear, it's real.

@levitte
Copy link
Member

levitte commented Mar 24, 2021

As mentioned above, there are also other ways, like using the modern implementation underneath and adding a wrapper declaration, either as a simple proper C function or as a macro.

So a backward compatibility library of sorts. This is simple in some cases (like MACs), but quite challenging in others (any RSA function... you'll end up having an RSA type aliased to an EVP_PKEY that itself points at key data where the provider also uses an RSA type that looks completely different. It's doable, but better not mix the header files...)

@levitte
Copy link
Member

levitte commented Mar 24, 2021

You know, the other way is, of course, to continue to use the deprecated functions all you want, and to tell the compiler to be silent about that particular warning. You will have at least 5 years to adapt...

@t-j-h
Copy link
Member

t-j-h commented Mar 24, 2021

@levitte depreciation is a strong recommendation to move away from using an API and many organisations have policies to not use deprecated APIs. If we have a problem with APIs then pushing this off does not help - it just creates an ever increasing mess.

Helper functions can ease the burden of supporting multiple versions and in reality a lot of our users have to support multiple versions in the one code base.

@DDvO
Copy link
Contributor Author

DDvO commented Mar 24, 2021

You know, the other way is, of course, to continue to use the deprecated functions all you want, and to tell the compiler to be silent about that particular warning.

Yes, but this would just defer the problem until 5 years later when one is forced to migrate to the new API (or stick with an unmaintained OpenSSL version or move away from OpenSSL).
By that time, chances are high that the original app code developers are no longer available,
and those who inherited responsibility for it can get in severe trouble with the by then 5+ years old code.

You will have at least 5 years to adapt...

If one adapts in between, the other issues mentioned above still apply at that point.

@paulidale
Copy link
Contributor

paulidale commented Apr 13, 2021

OTC is in favour of having new APIs for one shots (e.g. EVP_Digest).

Older one shots (HMAC, SHA256) that can be implemented as a macro wrapping a single function call should be supported. However, such one shots for algorithms in the legacy provider should not be done -- they are legacy and should go away.

There will need to be a macro guard to avoid the new macros and the deprecated functions from clashing.

@mattcaswell mattcaswell removed the hold: need otc decision The OTC needs to make a decision label Apr 13, 2021
@mattcaswell
Copy link
Member

mattcaswell commented Apr 13, 2021

OTC are also ok with a oneshot keygen function which takes both a "size" parameter and a string parameter to handle both RSA and EC use cases.

@DDvO
Copy link
Contributor Author

DDvO commented Apr 13, 2021

There will need to be a macro guard to avoid the new macros and the deprecated functions from clashing.

Currently the suggested name for that is OPENSSL_COMPENSATE_DEPRECATION.
Any objections/improvements on this name?

@paulidale
Copy link
Contributor

Would #ifdef OPENSSL_NO_DEPRECATED_3_0 work for the guard?

@DDvO
Copy link
Contributor Author

DDvO commented Apr 13, 2021

AFAICS, yes!
Then the pattern, as used e.g., in hmac.h as being adapted in #14664, will simplify from

# ifndef OPENSSL_NO_DEPRECATED_3_0
...
OSSL_DEPRECATEDIN_3_0 unsigned char *HMAC(const EVP_MD *evp_md, const void *key,
                                          int key_len, const unsigned char *d,
                                          size_t n, unsigned char *md,
                                          unsigned int *md_len);
...
# else
#  ifdef OPENSSL_COMPENSATE_DEPRECATION
#   define HMAC(evp_md, key, keylen, data, datalen, out, outlen) \
   EVP_Q_mac(NULL, "HMAC", NULL, EVP_MD_name(evp_md), NULL, \
             key, keylen, data, datalen, out, EVP_MD_size(evp_md), outlen)
#  endif
# endif

to

# ifndef OPENSSL_NO_DEPRECATED_3_0
...
OSSL_DEPRECATEDIN_3_0 unsigned char *HMAC(const EVP_MD *evp_md, const void *key,
                                          int key_len, const unsigned char *d,
                                          size_t n, unsigned char *md,
                                          unsigned int *md_len);
...
# else
#  define HMAC(evp_md, key, keylen, data, datalen, out, outlen) \
  EVP_Q_mac(NULL, "HMAC", NULL, EVP_MD_name(evp_md), NULL, \
            key, keylen, data, datalen, out, EVP_MD_size(evp_md), outlen)
# endif

@t8m
Copy link
Member

t8m commented Apr 13, 2021

Would #ifdef OPENSSL_NO_DEPRECATED_3_0 work for the guard?

IMO it is not a good idea. The user should explicitly indicate that he wants these macros. Especially if we are talking about macros such as SHA256() or similar.

@paulidale
Copy link
Contributor

I disagree, we are adding these to make life easier for users. Forcing an additional macro to be defined is an extra step.

We've decided that we want these helpers as macros, lets just have them regardless of deprecation.

@levitte
Copy link
Member

levitte commented Apr 14, 2021

The discussion about guards is really not relevant. What you're talking about is to undeprecate certain symbols (such as HMAC), so the action is really very simple: drop the old function declaration from public headers, drop the deprecation stuff around it, write the macro without any deprecation stuff around it, done.... where applicable, of course.

@DDvO
Copy link
Contributor Author

DDvO commented Apr 14, 2021

Sweet!

So

# ifndef OPENSSL_NO_DEPRECATED_3_0
...
OSSL_DEPRECATEDIN_3_0 unsigned char *HMAC(const EVP_MD *evp_md, const void *key,
                                          int key_len, const unsigned char *d,
                                          size_t n, unsigned char *md,
                                          unsigned int *md_len);
...
# else
#  ifdef OPENSSL_COMPENSATE_DEPRECATION
#   define HMAC(evp_md, key, keylen, data, datalen, out, outlen) \
   EVP_Q_mac(NULL, "HMAC", NULL, EVP_MD_name(evp_md), NULL, \
             key, keylen, data, datalen, out, EVP_MD_size(evp_md), outlen)
#  endif
# endif

reduces to simply

# define HMAC(evp_md, key, keylen, data, datalen, out, outlen) \
  EVP_Q_mac(NULL, "HMAC", NULL, EVP_MD_name(evp_md), NULL, \
            key, keylen, data, datalen, out, EVP_MD_size(evp_md), outlen)

@mattcaswell mattcaswell removed this from the 3.0.0 beta1 milestone Apr 20, 2021
@h-vetinari
Copy link
Contributor

0a8a6af used the phrase Partially fixes #14628, where GH just responds to the magic incantation "fixes #xxx" and disregards the "partially" - so this issue probably shouldn't be closed yet

@DDvO
Copy link
Contributor Author

DDvO commented May 11, 2021

Very pleased that finally the PRs improving the situation for HMAC(), SHA*(), and key generation are merged.

Are there any further crypto API elements that are planned to be deprecated but would deserve to be handled more user-friendly?

@paulidale
Copy link
Contributor

The AES functions in aes.h all take an AES_KEY pointer, so they aren't suitable for this treatment. They're mostly old modes, so not a big loss IMO.

@richsalz
Copy link
Contributor

Ready to close?

@t8m
Copy link
Member

t8m commented May 24, 2021

All related PRs are now merged. Closing.

@t8m t8m closed this as completed May 24, 2021
devnexen pushed a commit to devnexen/openssl that referenced this issue Jul 7, 2021
… MAC functions

This helps compensating for deprecated functions such as HMAC()
and reduces clutter in the crypto lib, apps, and tests.
Also fixes memory leaks in generate_cookie_callback() of apps/lib/s_cb.c.
and replaces 'B<...>' by 'I<...>' where appropriate in HMAC.pod

Partially fixes openssl#14628.

Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from openssl#14664)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triaged: documentation The issue/pr deals with documentation (errors) triaged: feature The issue/pr requests/adds a feature triaged: OTC evaluated This issue/pr was triaged by OTC
Projects
None yet
Development

No branches or pull requests

9 participants