Deprecate the low level AES functions #10580

mattcaswell · 2019-12-05T18:12:51Z

Use of the low level AES functions has been informally discouraged for a
long time. We now formally deprecate them.

Applications should instead use the EVP APIs, e.g. EVP_EncryptInit_ex,
EVP_EncryptUpdate, EVP_EncryptFinal_ex, and the equivalently named decrypt
functions.

Need 'make update'

levitte · 2019-12-11T10:41:23Z

make update needed

mattcaswell · 2019-12-11T10:42:41Z

make update needed

Unfortunately much more than that is needed. This PR just doesn't work at all with "no-deprecated". I'm working on it.

mattcaswell · 2019-12-11T10:47:35Z

Temporarily I moved this into WIP because its currently broken. I've pushed a bunch of fixes for "no-deprecated" but there's still more to fix.

levitte · 2019-12-11T11:06:18Z

The last commit added OPENSSL_SUPRESS_DEPRECATION a little all over the place. Wouldn't the following be much simpler and easier to follow?

diff --git a/include/openssl/macros.h b/include/openssl/macros.h
index a38387f131..484d76c1e1 100644
--- a/include/openssl/macros.h
+++ b/include/openssl/macros.h
@@ -25,6 +25,9 @@
 
 /*
  * Generic deprecation macro
+ *
+ * If OPENSSL_SUPPRESS_DEPRECATED is defined, then DECLARE_DEPRECATED
+ * becomes a no-op
  */
 # ifndef DECLARE_DEPRECATED
 #  define DECLARE_DEPRECATED(f)   f;
@@ -43,6 +46,13 @@
 #  endif
 # endif
 
+/*
+ * If OPENSSL_SUPPRESS_DEPRECATED is defined, then turn off 'no-deprecated'
+ */
+#ifdef OPENSSL_SUPPRESS_DEPRECATED
+# undef OPENSSL_NO_DEPRECATED
+#endif
+
 /*
  * Applications should use -DOPENSSL_API_COMPAT=<version> to suppress the
  * declarations of functions deprecated in or before <version>.  If this is

mattcaswell · 2019-12-11T11:07:57Z

You mean instead of the other changes in macros.h? Yes that could work.

levitte · 2019-12-11T11:33:46Z

You mean instead of the other changes in macros.h? Yes that could work.

And elsewhere. Basically, you shouldn't need to check OPENSSL_SUPPRESS_DEPRECATE all over the place

mattcaswell · 2019-12-11T15:21:05Z

Updated so that it should now work in both normal builds and no-deprecated builds. I've left this as WIP for now because it now incorporates the commits from #10606 and #10608. Once those are merged I will rebase this and take it out of WIP.

levitte · 2019-12-11T15:41:49Z

I wouldn't mind if #10608 got reviewed. 😉

mattcaswell · 2019-12-16T15:07:27Z

Now that #10606 and #10608 have been merged I've rebased this and taken it out of WIP.

Please review.

slontis · 2019-12-17T01:24:12Z

travis isnt looking very healthly,
since deprecated warnings results in errors

slontis · 2019-12-17T01:34:42Z

crypto/cms/cms_env.c

        goto err;
    }

    wkey = OPENSSL_malloc(ec->keylen + 8);
-


The + 8 on the line above in the existing code looks a bit dangerous to me :)
Hopefully it equals wkeylen below.
A check might have been nice.

I added a comment about this, and later ossl_assert check

include/internal/deprecated.h

levitte · 2019-12-17T05:21:13Z

travis isnt looking very healthly,
since deprecated warnings results in errors

crypto/aes/aes_cbc.c needs to include "internsl/deprecated.h"

mattcaswell · 2019-12-17T12:54:43Z

crypto/aes/aes_cbc.c needs to include "internsl/deprecated.h"

Yes, and a couple of other spots. Now fixed.

Please take a look.

mattcaswell · 2019-12-18T10:09:23Z

Travis red cross is not relevant.

Ping?

slontis · 2019-12-18T22:49:04Z

include/crypto/ciphermode_platform.h

@@ -12,7 +12,8 @@

 # include "openssl/aes.h"

-# ifdef VPAES_ASM
+# ifndef OPENSSL_NO_DEPRECATED


Not quite sure I understand why the NO_DEPRECATED is required in this internal file?
(This file is used also inside provider implementations).

This file is included by e_camellia.c which does not include the "internal/deprecated.h" file, because it doesn't use any externally deprecated APIs. However this header file contains references to AES things that may not be defined in a no-deprecated build - and so the compiler complains loudly. Adding this guard means e_camellia.c compiles correctly. Another fix would be to instead include "internal/deprecated.h" in e_camellia.c - but I'd rather not do that.

cipher_platform.h should really be divided into algo specific headers.

Maybe. But I'd see that as outside of the scope of this PR.

Now that #10662 has gone in, this file no longer exists and these changes aren't necessary.

We should not be using the low level AES APIs in CMS. Instead we should be using EVP. There was a small amount of use of the low level key wrap APIs - so we convert that to EVP.

Use of the low level AES functions has been informally discouraged for a long time. We now formally deprecate them. Applications should instead use the EVP APIs, e.g. EVP_EncryptInit_ex, EVP_EncryptUpdate, EVP_EncryptFinal_ex, and the equivalently named decrypt functions.

mattcaswell · 2019-12-23T10:48:39Z

I've rebased this to pick up the changes from #10662. Please take another look.

mattcaswell · 2020-01-02T11:56:48Z

Ping?

t8m · 2020-01-02T15:22:23Z

include/internal/deprecated.h

+ * This header *must* be the first OpenSSL header included by a source file.
+ */
+
+#ifndef OSSL_INTERNAL_DEPRECTATED_H


Typo: deprectated

mattcaswell · 2020-01-02T16:45:17Z

Fixup commit pushed addressing latest feedback comment.

t8m

LGTM

mattcaswell · 2020-01-06T15:15:11Z

Pushed. Thanks.

We should not be using the low level AES APIs in CMS. Instead we should be using EVP. There was a small amount of use of the low level key wrap APIs - so we convert that to EVP. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #10580)

Use of the low level AES functions has been informally discouraged for a long time. We now formally deprecate them. Applications should instead use the EVP APIs, e.g. EVP_EncryptInit_ex, EVP_EncryptUpdate, EVP_EncryptFinal_ex, and the equivalently named decrypt functions. Reviewed-by: Tomas Mraz <tmraz@fedoraproject.org> (Merged from #10580)

paulidale · 2020-01-09T02:38:50Z

engines/e_padlock.c

+ * This file uses the low level AES functions (which are deprecated for
+ * non-internal use) in order to implement the padlock engine AES ciphers.
+ */
+#define OPENSSL_SUPPRESS_DEPRECATED


@mattcaswell does this work for an engine?
Won't the low level calls be hidden and non-resolvable?

There are two scenarios:

Compilation when "no-deprecated" has been defined

Compilation when "no-deprecated" has not been defined

In the first case this PR changed Configure so that padlock never gets compiled. In the second case the low level calls are resolvable and we are only concerned about the deprecation warnings that the compiler will emit. This line is to suppress those.

Thanks for the explanation, I missed the Configure change somehow.

Is this how we want to deal with engines that use deprecated APIs? e.g. the e_dasync engine uses SHA-1 and I modified it to not over in #10791.

In general no. I think where possible we should convert usage to use EVP instead. I took the decision not to do that with padlock because the usage was so extensive.

decrypt functions Some functions including low level AES functions would be deprecated in next OpenSSL version(3.0). OpenSSL team says that application should use the high level EVP APIs, so I added these functions. See also: openssl/openssl#10580 openssl/openssl#10740

mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels Dec 5, 2019

levitte previously approved these changes Dec 5, 2019

View reviewed changes

mattcaswell mentioned this pull request Dec 11, 2019

Cleanup legacy digest methods. #10602

Closed

2 tasks

mattcaswell changed the title ~~Deprecate the low level AES functions~~ WIP: Deprecate the low level AES functions Dec 11, 2019

This was referenced Dec 11, 2019

PROV: Move AES_CCM specialisation away from common cipher header #10606

Closed

Add better support for using deprecated symbols internally #10608

Closed

mattcaswell force-pushed the deprecate-aes branch from cca395f to ccba07c Compare December 11, 2019 15:15

mattcaswell force-pushed the deprecate-aes branch from ccba07c to 6c2ad4d Compare December 16, 2019 15:06

mattcaswell changed the title ~~WIP: Deprecate the low level AES functions~~ Deprecate the low level AES functions Dec 16, 2019

slontis reviewed Dec 17, 2019

View reviewed changes

levitte reviewed Dec 17, 2019

View reviewed changes

include/internal/deprecated.h Outdated Show resolved Hide resolved

slontis reviewed Dec 18, 2019

View reviewed changes

mattcaswell added 3 commits December 23, 2019 10:33

Don't use the low level AES key wrap APIs in CMS

dcee9d5

We should not be using the low level AES APIs in CMS. Instead we should be using EVP. There was a small amount of use of the low level key wrap APIs - so we convert that to EVP.

fixup! Deprecate the low level AES functions

c315e87

mattcaswell added 3 commits December 23, 2019 10:35

fixup! Don't use the low level AES key wrap APIs in CMS

88f3f13

fixup! Deprecate the low level AES functions

dd7ef45

fixup! Deprecate the low level AES functions

24e3db3

mattcaswell force-pushed the deprecate-aes branch from 40b0f90 to 24e3db3 Compare December 23, 2019 10:47

richsalz mentioned this pull request Dec 27, 2019

Add -DOPENSSL_BUILDING_OPENSSL to our builds #10666

Closed

mattcaswell mentioned this pull request Jan 2, 2020

Deprecate the Low Level Blowfish APIs #10740

Closed

t8m requested changes Jan 2, 2020

View reviewed changes

This was referenced Jan 2, 2020

Deprecate the Low Level Camellia APIs #10741

Closed

Deprecate the Low Level CAST APIs #10742

Closed

fixup! Deprecate the low level AES functions

697b733

t8m approved these changes Jan 3, 2020

View reviewed changes

t8m added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 3, 2020

levitte added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 4, 2020

mattcaswell closed this Jan 6, 2020

paulidale reviewed Jan 9, 2020

View reviewed changes

kubo39 mentioned this pull request Jan 26, 2020

Add EVP_EncryptInit_ex/EVP_EncryptFinish_ex, and the equivalently named sfackler/rust-openssl#1222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate the low level AES functions #10580

Deprecate the low level AES functions #10580

mattcaswell commented Dec 5, 2019

levitte commented Dec 11, 2019

mattcaswell commented Dec 11, 2019

mattcaswell commented Dec 11, 2019

levitte commented Dec 11, 2019 •

edited

mattcaswell commented Dec 11, 2019

levitte commented Dec 11, 2019

mattcaswell commented Dec 11, 2019 •

edited by levitte

levitte commented Dec 11, 2019

mattcaswell commented Dec 16, 2019

slontis commented Dec 17, 2019

slontis Dec 17, 2019

mattcaswell Dec 17, 2019

levitte commented Dec 17, 2019 •

edited

mattcaswell commented Dec 17, 2019

mattcaswell commented Dec 18, 2019

slontis Dec 18, 2019 •

edited

mattcaswell Dec 19, 2019

levitte Dec 19, 2019

mattcaswell Dec 19, 2019

levitte Dec 19, 2019

mattcaswell Dec 23, 2019

mattcaswell commented Dec 23, 2019

mattcaswell commented Jan 2, 2020

t8m Jan 2, 2020

mattcaswell Jan 2, 2020

mattcaswell commented Jan 2, 2020

t8m left a comment

mattcaswell commented Jan 6, 2020

paulidale Jan 9, 2020

mattcaswell Jan 9, 2020

paulidale Jan 9, 2020

mattcaswell Jan 9, 2020

Deprecate the low level AES functions #10580

Deprecate the low level AES functions #10580

Conversation

mattcaswell commented Dec 5, 2019

levitte commented Dec 11, 2019

mattcaswell commented Dec 11, 2019

mattcaswell commented Dec 11, 2019

levitte commented Dec 11, 2019 • edited

mattcaswell commented Dec 11, 2019

levitte commented Dec 11, 2019

mattcaswell commented Dec 11, 2019 • edited by levitte

levitte commented Dec 11, 2019

mattcaswell commented Dec 16, 2019

slontis commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Dec 17, 2019 • edited

mattcaswell commented Dec 17, 2019

mattcaswell commented Dec 18, 2019

slontis Dec 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell commented Dec 23, 2019

mattcaswell commented Jan 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattcaswell commented Jan 2, 2020

t8m left a comment

Choose a reason for hiding this comment

mattcaswell commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Dec 11, 2019 •

edited

mattcaswell commented Dec 11, 2019 •

edited by levitte

levitte commented Dec 17, 2019 •

edited

slontis Dec 18, 2019 •

edited