Update API usage to OpenSSL 3 (RSA) #993

iamnotacake · 2023-02-28T11:01:20Z

Second part regarding changes that replace deprecated OpenSSL API usage with new alternatives from v3. This time related to RSA keys serialization/deserialization, and few other related places.

Checklist

Change is covered by automated tests
Benchmark results are attached (if applicable)
The coding guidelines are followed
~~Public API has proper documentation~~
~~Example projects and code samples are up-to-date (in case of API changes)~~
Changelog is updated (in case of notable or breaking changes)

Just a copy for now, OpenSSL 3 specific changes will follow

* Get rid of `RSA*` struct usage and its depracated methods, extract bigints directly from `EVP_PKEY*` using EVP_PKEY_get_bn_param() * Extract following params for public key: - OSSL_PKEY_PARAM_RSA_N - OSSL_PKEY_PARAM_RSA_E * And a couple of additional ones for private key: - OSSL_PKEY_PARAM_RSA_FACTOR1 (also known as P) - OSSL_PKEY_PARAM_RSA_FACTOR2 (also known as Q) - OSSL_PKEY_PARAM_RSA_EXPONENT1 - OSSL_PKEY_PARAM_RSA_EXPONENT2 - OSSL_PKEY_PARAM_RSA_COEFFICIENT1

Implement public and private key deserialization using new EVP_PKEY_fromdata() function. Get rid of RSA*. Few other places were affected as well because things are quite bound to each other, EVP_PKEY_size() on newer key won't work correctly.

* Use EVP_PKEY_private_check() to check private key * Make rsa_mod_size round value to nearest whole byte * Remove few functions completely unused in OpenSSL 3 implementation of RSA routines

src/soter/openssl/soter_asym_cipher.c

src/soter/openssl/soter_rsa_key_3.c

Just serialize BIGINTs directly with BN_bn2binpad() instead of using functions that 1) check size 2) serialize number 3) add zeroes padding because BN_bn2binpad() could do all that and return -1 in case destination buffer is too small

Use the fact that EVP_PKEY_get_bn_param() could write into existing BIGINT instead of allocating a new one, reuse that single BIGINT for multiple values

src/soter/openssl/soter_asym_cipher.c

src/soter/openssl/soter_ec_key.c

src/soter/openssl/soter_rsa_key_3.c

ilammy · 2023-03-02T12:07:09Z

Tests pass ☀️

LGTM, provided a decision on OpenSSL 1.0.2 support is made (followed by some reverts, if we still support it).

iamnotacake · 2023-03-03T19:25:13Z

.github/workflows/test-core.yaml

@@ -432,8 +402,6 @@ jobs:
    runs-on: windows-2022
    env:
      SOTER_KDF_RUN_LONG_TESTS: yes
-      # TODO: stop using deprecated API so that warnings can be errors again
-      WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT: yes
      WITH_FATAL_WARNINGS: "no" # YAML :trollface:


Missed this line, will remove in next commit. BTW, why MSYS2 environment job fails like there is still "no support for OpenSSL 3" in header file?

why MSYS2 environment job fails like there is still "no support for OpenSSL 3" in header file?

Hm... Looks like the package uses the pristine 0.14.0 tarball, not the source from the current tree.

That is, when you build a package, it's built locally but still references the tarball from release artifacts, not packages the source from whatever the current branch contains. And it's been doing that for a while. Says a lot about how well-maintained MSYS2 builds are.

Uh, well... I think you should leave WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT: yes here for the time being. Then someone 🙄 should apply whatever hacks necessary for this job to package the tree being tested.

Kinda weird seeing Pacman being used on Windows. Anyway, added this line back and another comment instead, since "stop using deprecated API" is not relevant anymore

ilammy

Good job on getting all those warnings resolved! 💪

src/soter/openssl/soter_rsa_key_3.c

src/soter/openssl/soter_rsa_key_utils.h

iamnotacake · 2023-03-04T00:21:20Z

BTW, I managed to build (and even debug) libsoter with OpenSSL 1.0.2g. Using CLion (CMake project) and Docker (Ubuntu 16.04 with compiler and openssl being added). Even though current CMakeLists.txt requires CMake 3.8, it worked with CMake 3.5.1 that was available on old Ubuntu. Had to lower the version and add include_directories(include) to make it work though.

Lagovas · 2023-03-03T15:56:25Z

src/soter/openssl/soter_asym_cipher.c

@@ -243,13 +296,10 @@ soter_status_t soter_asym_cipher_decrypt(soter_asym_cipher_t* asym_cipher,
        return SOTER_INVALID_PARAMETER;
    }

-    rsa = EVP_PKEY_get0(pkey);
-    if (NULL == rsa) {
+    if (!get_rsa_mod_size(pkey, &rsa_mod_size)) {


now code looks cleaner

src/soter/openssl/soter_ec_key_utils.h

src/soter/openssl/soter_rsa_key_3.c

Lagovas · 2023-03-06T11:50:42Z

src/soter/openssl/soter_rsa_key_3.c

+    uint32_t* pub_exp;
+    // This bigint is reused multiple times: read into it, serialize it, read into it...
+    BIGNUM* tmp = NULL;
+    unsigned char* curr_bn = (unsigned char*)(key + 1);


same here. can you review all functions and add checks for input arguments?

This one is kinda weird. Indeed, we assign curr_bn = key before checking key. But we access curr_bn only after checking key, so there is no bug here. To make it cleaner, I'll move assignment to a place between check and first usage

src/soter/openssl/soter_rsa_key_utils.h

Lagovas

lgtm but lets make tests green

Lagovas · 2023-03-06T18:55:54Z

due to tests fails because of one dependency increased required version of cargo and this PR merges into the temporary branch before merging to master, we can merge it to it, unblock final reviewing and fixing tests in separate PR #995

* Move common RSA util functinos to separate file * Create copy of soter_rsa_key.c for OpenSSL 3 Just a copy for now, OpenSSL 3 specific changes will follow * Rewrite RSA keys serialization * Get rid of `RSA*` struct usage and its depracated methods, extract bigints directly from `EVP_PKEY*` using EVP_PKEY_get_bn_param() * Extract following params for public key: - OSSL_PKEY_PARAM_RSA_N - OSSL_PKEY_PARAM_RSA_E * And a couple of additional ones for private key: - OSSL_PKEY_PARAM_RSA_FACTOR1 (also known as P) - OSSL_PKEY_PARAM_RSA_FACTOR2 (also known as Q) - OSSL_PKEY_PARAM_RSA_EXPONENT1 - OSSL_PKEY_PARAM_RSA_EXPONENT2 - OSSL_PKEY_PARAM_RSA_COEFFICIENT1 * Implement RSA key deserialization, few related fixes Implement public and private key deserialization using new EVP_PKEY_fromdata() function. Get rid of RSA*. Few other places were affected as well because things are quite bound to each other, EVP_PKEY_size() on newer key won't work correctly. * Fix build, minor updates * Fix build * Fix build * Fix build * Fix build * Updates after review * Use EVP_PKEY_private_check() to check private key * Make rsa_mod_size round value to nearest whole byte * Remove few functions completely unused in OpenSSL 3 implementation of RSA routines * Simplify bigint serialization, remove redundant functions Just serialize BIGINTs directly with BN_bn2binpad() instead of using functions that 1) check size 2) serialize number 3) add zeroes padding because BN_bn2binpad() could do all that and return -1 in case destination buffer is too small * Make RSA key serialization functions reuse bigint Use the fact that EVP_PKEY_get_bn_param() could write into existing BIGINT instead of allocating a new one, reuse that single BIGINT for multiple values * Zeroize partially serialized RSA private key on fail * Updates after review * Updates after review * Enable building on Ubuntu 22.04, remove experimental flag * Remove unneeded CI job * Enable build with warnings as errors * Minor updates after review * Fix build * Add few more checks, move common function to separate file

* Update API usage to OpenSSL 3 (#989) These changes are supposed to make Themis more compatible with OpenSSL 3 by dropping usage of deprecated functions and using new slternatives instead. There are also places where non-deprecated functions were used, but they turned out to be incompatible with EVP_PKEY* created using newer API. Such places are affected as well, using `#if` macro to conditionally compile code based on target OpenSSL version. Update CMakeLists.txt, add flags * to control building for OpenSSL 3 using WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT * to disable NIST STS tests Create copy of `soter_ec_key.c` that contains newer implementation and uses OpenSSL 3 APIs for key serialization/deserialization routines. Difference from OpenSSL 1.1 includes: * Get rid of `EC_KEY*` * Get rid of `EC_GROUP*`, use string curve identifier instead, extracted from `EVP_PKEY*` using `EVP_PKEY_get_utf8_string_param()` * Get rid of `EC_POINT*`, use `EVP_PKEY_get_octet_string_param()` to extract curve public key from `EVP_PKEY*` directly. Deserialize public EC key directly from provided buffer using `EVP_PKEY_fromdata()`, this same function yields recreated `EVP_PKEY*` in case of success Also, in a different file: * Replace `EVP_MD_CTX_md()` with `EVP_MD_CTX_get0_md()` Update CHANGELOG.md * Update API usage to OpenSSL 3 (RSA) (#993) * Move common RSA util functinos to separate file * Create copy of soter_rsa_key.c for OpenSSL 3 Just a copy for now, OpenSSL 3 specific changes will follow * Rewrite RSA keys serialization * Get rid of `RSA*` struct usage and its depracated methods, extract bigints directly from `EVP_PKEY*` using EVP_PKEY_get_bn_param() * Extract following params for public key: - OSSL_PKEY_PARAM_RSA_N - OSSL_PKEY_PARAM_RSA_E * And a couple of additional ones for private key: - OSSL_PKEY_PARAM_RSA_FACTOR1 (also known as P) - OSSL_PKEY_PARAM_RSA_FACTOR2 (also known as Q) - OSSL_PKEY_PARAM_RSA_EXPONENT1 - OSSL_PKEY_PARAM_RSA_EXPONENT2 - OSSL_PKEY_PARAM_RSA_COEFFICIENT1 * Implement RSA key deserialization, few related fixes Implement public and private key deserialization using new EVP_PKEY_fromdata() function. Get rid of RSA*. Few other places were affected as well because things are quite bound to each other, EVP_PKEY_size() on newer key won't work correctly. * Fix build, minor updates * Fix build * Fix build * Fix build * Fix build * Updates after review * Use EVP_PKEY_private_check() to check private key * Make rsa_mod_size round value to nearest whole byte * Remove few functions completely unused in OpenSSL 3 implementation of RSA routines * Simplify bigint serialization, remove redundant functions Just serialize BIGINTs directly with BN_bn2binpad() instead of using functions that 1) check size 2) serialize number 3) add zeroes padding because BN_bn2binpad() could do all that and return -1 in case destination buffer is too small * Make RSA key serialization functions reuse bigint Use the fact that EVP_PKEY_get_bn_param() could write into existing BIGINT instead of allocating a new one, reuse that single BIGINT for multiple values * Zeroize partially serialized RSA private key on fail * Updates after review * Updates after review * Enable building on Ubuntu 22.04, remove experimental flag * Remove unneeded CI job * Enable build with warnings as errors * Minor updates after review * Fix build * Add few more checks, move common function to separate file * Update CHANGELOG.md * Don't use EVP_PKEY_get_bn_param() for private keys This function uses temporary buffer inside, asks EVP_PKEY_get_params() to put bigint into it, makes BIGINT, doesn't clean the buffer afterwards. Decided to instead call EVP_PKEY_get_params() manually, and free the buffer after usage. Also have to reverse byte order in this case because of the fact that EVP_PKEY_get_params() puts bigints in native-endian. * Attempt to fix implementation on macOS Explicitly zeroize buffers for bigints in case EVP_PKEY_get_params() doesn't add padding * Attempt to fix implementation on macOS Revert back to using BIGNUM* during key serialization, but create it with custom get_bn_param() function. * Fix build Replace EVP_PKEY_get_bn_param -> get_bn_param in one more place, remove memcpy_big_endian() * Updates after review * up version of macos runner and sdk --------- Co-authored-by: Lagovas <[email protected]>

iamnotacake added 10 commits February 28, 2023 01:08

Move common RSA util functinos to separate file

5aee50e

Create copy of soter_rsa_key.c for OpenSSL 3

bcd0dd5

Just a copy for now, OpenSSL 3 specific changes will follow

Fix build, minor updates

cc8bac5

Fix build

7d33ed0

Fix build

c244dc8

Fix build

cc289b2

Fix build

9678f51

Updates after review

c3c7c1f

* Use EVP_PKEY_private_check() to check private key * Make rsa_mod_size round value to nearest whole byte * Remove few functions completely unused in OpenSSL 3 implementation of RSA routines

iamnotacake marked this pull request as ready for review February 28, 2023 14:02

iamnotacake requested review from vixentael, Lagovas, ilammy and G1gg1L3s as code owners February 28, 2023 14:02

Lagovas reviewed Feb 28, 2023

View reviewed changes

src/soter/openssl/soter_asym_cipher.c Outdated Show resolved Hide resolved

Lagovas reviewed Feb 28, 2023

View reviewed changes

iamnotacake added 3 commits March 1, 2023 19:15

Make RSA key serialization functions reuse bigint

83d24ac

Use the fact that EVP_PKEY_get_bn_param() could write into existing BIGINT instead of allocating a new one, reuse that single BIGINT for multiple values

Zeroize partially serialized RSA private key on fail

565f753

ilammy reviewed Mar 2, 2023

View reviewed changes

iamnotacake added 3 commits March 3, 2023 17:16

Updates after review

364b646

Updates after review

83f07f9

Enable building on Ubuntu 22.04, remove experimental flag

c65552f

iamnotacake requested a review from shadinua as a code owner March 3, 2023 18:52

iamnotacake added 2 commits March 3, 2023 21:16

Remove unneeded CI job

8e65111

Enable build with warnings as errors

10452fc

iamnotacake commented Mar 3, 2023

View reviewed changes

ilammy approved these changes Mar 3, 2023

View reviewed changes

src/soter/openssl/soter_rsa_key_3.c Outdated Show resolved Hide resolved

src/soter/openssl/soter_rsa_key_3.c Outdated Show resolved Hide resolved

src/soter/openssl/soter_rsa_key_utils.h Outdated Show resolved Hide resolved

iamnotacake added 2 commits March 4, 2023 02:10

Minor updates after review

f6b69fb

Fix build

8ecea16

iamnotacake requested a review from Lagovas March 4, 2023 00:22

Lagovas reviewed Mar 6, 2023

View reviewed changes

Add few more checks, move common function to separate file

3792ead

Lagovas approved these changes Mar 6, 2023

View reviewed changes

Lagovas merged commit b201fc9 into cossacklabs:openssl3-support Mar 6, 2023

iamnotacake deleted the anatolii-T2710-openssl3-porting-rsa branch March 6, 2023 18:56

iamnotacake mentioned this pull request Mar 6, 2023

OpenSSL 3 support #996

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update API usage to OpenSSL 3 (RSA) #993

Update API usage to OpenSSL 3 (RSA) #993

iamnotacake commented Feb 28, 2023 •

edited

Loading

ilammy commented Mar 2, 2023

iamnotacake Mar 3, 2023

ilammy Mar 3, 2023

iamnotacake Mar 4, 2023

ilammy left a comment

iamnotacake commented Mar 4, 2023

Lagovas Mar 3, 2023

Lagovas Mar 6, 2023

iamnotacake Mar 6, 2023

Lagovas left a comment

Lagovas commented Mar 6, 2023

Update API usage to OpenSSL 3 (RSA) #993

Update API usage to OpenSSL 3 (RSA) #993

Conversation

iamnotacake commented Feb 28, 2023 • edited Loading

Checklist

ilammy commented Mar 2, 2023

iamnotacake Mar 3, 2023

Choose a reason for hiding this comment

ilammy Mar 3, 2023

Choose a reason for hiding this comment

iamnotacake Mar 4, 2023

Choose a reason for hiding this comment

ilammy left a comment

Choose a reason for hiding this comment

iamnotacake commented Mar 4, 2023

Lagovas Mar 3, 2023

Choose a reason for hiding this comment

Lagovas Mar 6, 2023

Choose a reason for hiding this comment

iamnotacake Mar 6, 2023

Choose a reason for hiding this comment

Lagovas left a comment

Choose a reason for hiding this comment

Lagovas commented Mar 6, 2023

iamnotacake commented Feb 28, 2023 •

edited

Loading