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

Update API usage to OpenSSL 3 (RSA) #993

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 3 additions & 31 deletions .github/workflows/test-core.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ jobs:
with:
submodules: true
- name: Build Themis Core (OpenSSL)
if: ${{ matrix.os != 'ubuntu-22.04' }}
run: make prepare_tests_basic ENGINE=openssl BUILD_PATH=build-openssl
- name: Build Themis Core (OpenSSL 3.0)
if: ${{ matrix.os != 'ubuntu-20.04' }}
Expand All @@ -70,21 +69,15 @@ jobs:
export ENGINE_INCLUDE_PATH="$openssl3/include"
export ENGINE_LIB_PATH="$openssl3/lib"
fi
# TODO: stop using deprecated API so that warnings can be errors again
export WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT=yes
export WITH_FATAL_WARNINGS=no
make prepare_tests_basic BUILD_PATH=build-openssl-3.0
- name: Build Themis Core (BoringSSL)
if: always()
run: make prepare_tests_basic ENGINE=boringssl BUILD_PATH=build-boringssl
- name: Build Themis Core (WITH_SCELL_COMPAT)
if: ${{ matrix.os != 'ubuntu-22.04' }}
run: make prepare_tests_basic WITH_SCELL_COMPAT=yes BUILD_PATH=build-compat
- name: Run test suite (OpenSSL)
if: ${{ matrix.os != 'ubuntu-22.04' }}
run: make test ENGINE=openssl BUILD_PATH=build-openssl
- name: Run test suite (OpenSSL, uncompressed keys)
if: ${{ matrix.os != 'ubuntu-22.04' }}
env:
THEMIS_GEN_EC_KEY_PAIR_UNCOMPRESSED: "1"
run: make test ENGINE=openssl BUILD_PATH=build-openssl
Expand All @@ -98,8 +91,6 @@ jobs:
export ENGINE_INCLUDE_PATH="$openssl3/include"
export ENGINE_LIB_PATH="$openssl3/lib"
fi
export WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT=yes
export WITH_FATAL_WARNINGS=no
make test BUILD_PATH=build-openssl-3.0
- name: Run test suite (OpenSSL 3.0, uncompressed keys)
if: ${{ matrix.os != 'ubuntu-20.04' }}
Expand All @@ -113,8 +104,6 @@ jobs:
export ENGINE_INCLUDE_PATH="$openssl3/include"
export ENGINE_LIB_PATH="$openssl3/lib"
fi
export WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT=yes
export WITH_FATAL_WARNINGS=no
make test BUILD_PATH=build-openssl-3.0
- name: Run test suite (BoringSSL)
if: always()
Expand All @@ -125,26 +114,7 @@ jobs:
THEMIS_GEN_EC_KEY_PAIR_UNCOMPRESSED: "1"
run: make test ENGINE=boringssl BUILD_PATH=build-boringssl
- name: Run test suite (WITH_SCELL_COMPAT)
if: ${{ matrix.os != 'ubuntu-22.04' }}
run: make test WITH_SCELL_COMPAT=yes BUILD_PATH=build-compat
- name: Ensure OpenSSL 3.0 fails
if: ${{ matrix.os != 'ubuntu-20.04' }}
run: |
export ENGINE=openssl
# Themis uses OpenSSL 1.1 by default if installed.
# Explicitly request OpenSSL 3.0 by pointing the build into OpenSSL 3.0's paths.
if [[ "$MATRIX_OS" = "macos-12" ]]; then
openssl3=$(brew --prefix openssl@3)
export ENGINE_INCLUDE_PATH="$openssl3/include"
export ENGINE_LIB_PATH="$openssl3/lib"
fi
if ! make BUILD_PATH=build-openssl-3.0-without-magic-word
then
true
else
echo "Build with OpenSSL 3.0 did not fail when it should have"
exit 1
fi

examples:
name: Code examples
Expand Down Expand Up @@ -432,7 +402,9 @@ jobs:
runs-on: windows-2022
env:
SOTER_KDF_RUN_LONG_TESTS: yes
# TODO: stop using deprecated API so that warnings can be errors again
# TODO: remove these two options when MSYS2 build issues are solved,
# because now PKGBUILD.MSYS2 just downloads latest release archive
# from GitHub instead of using checked out files
WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT: yes
WITH_FATAL_WARNINGS: "no" # YAML :trollface:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

defaults:
Expand Down
4 changes: 0 additions & 4 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,6 @@ if (NO_NIST_STS)
add_definitions(-DNO_NIST_STS=1)
endif()

if (WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT)
add_definitions(-DTHEMIS_EXPERIMENTAL_OPENSSL_3_SUPPORT=1)
endif()

file(GLOB SOTER_SOURCE_FILES src/soter/*.c src/soter/openssl/*.c src/soter/ed25519/*)
add_library(soter ${SOTER_SOURCE_FILES})
add_library(soter_shared SHARED ${SOTER_SOURCE_FILES})
Expand Down
4 changes: 0 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,6 @@ ifeq ($(RSA_KEY_LENGTH),8192)
CFLAGS += -DTHEMIS_RSA_KEY_LENGTH=RSA_KEY_LENGTH_8192
endif

ifeq ($(WITH_EXPERIMENTAL_OPENSSL_3_SUPPORT),yes)
CFLAGS += -DTHEMIS_EXPERIMENTAL_OPENSSL_3_SUPPORT=1
endif

########################################################################
#
# Compilation flags for C/C++ code
Expand Down
80 changes: 64 additions & 16 deletions src/soter/openssl/soter_asym_cipher.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@
#include "soter/soter_asym_cipher.h"

#include <openssl/evp.h>
#include <openssl/opensslv.h>
#include <openssl/rsa.h>
#if OPENSSL_VERSION_NUMBER >= 0x30000000
#include <openssl/core_names.h>
#endif

#include "soter/openssl/soter_engine.h"
#include "soter/soter_api.h"
Expand All @@ -30,6 +34,63 @@
#define SOTER_RSA_KEY_LENGTH 2048
#endif

#if OPENSSL_VERSION_NUMBER >= 0x30000000
static bool get_rsa_mod_size(EVP_PKEY* pkey, int* rsa_mod_size)
{
if (!EVP_PKEY_get_int_param(pkey, OSSL_PKEY_PARAM_BITS, rsa_mod_size)) {
return false;
}
*rsa_mod_size = (*rsa_mod_size + 7) / 8;
return true;
}
#else
static bool get_rsa_mod_size(EVP_PKEY* pkey, int* rsa_mod_size)
{
RSA* rsa = EVP_PKEY_get0(pkey);
if (!rsa) {
return false;
}
*rsa_mod_size = RSA_size(rsa);
return true;
}
#endif

#if OPENSSL_VERSION_NUMBER < 0x10100000L
static bool check_rsa_private_key(EVP_PKEY* pkey, EVP_PKEY_CTX* pkey_ctx)
{
UNUSED(pkey_ctx);
RSA* rsa = EVP_PKEY_get0(pkey);

if (rsa->d == NULL) {
return false;
}

return true;
}
#elif OPENSSL_VERSION_NUMBER < 0x30000000
static bool check_rsa_private_key(EVP_PKEY* pkey, EVP_PKEY_CTX* pkey_ctx)
{
UNUSED(pkey_ctx);
RSA* rsa = EVP_PKEY_get0(pkey);
const BIGNUM* d = NULL;

RSA_get0_key(rsa, NULL, NULL, &d);

if (NULL == d) {
return false;
}

return true;
}
#else /* OPENSSL_VERSION_NUMBER >= 0x30000000 */
static bool check_rsa_private_key(EVP_PKEY* pkey, EVP_PKEY_CTX* pkey_ctx)
{
UNUSED(pkey);
// EVP_PKEY_private_check was added in 3.0
return EVP_PKEY_private_check(pkey_ctx) == 1;
}
#endif

soter_status_t soter_asym_cipher_import_key(soter_asym_cipher_t* asym_cipher_ctx,
const void* key,
size_t key_length)
Expand Down Expand Up @@ -139,7 +200,6 @@ soter_status_t soter_asym_cipher_encrypt(soter_asym_cipher_t* asym_cipher,
size_t* cipher_data_length)
{
EVP_PKEY* pkey;
RSA* rsa;
int rsa_mod_size;
size_t output_length;

Expand All @@ -158,12 +218,10 @@ soter_status_t soter_asym_cipher_encrypt(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)) {
return SOTER_FAIL;
}

rsa_mod_size = RSA_size(rsa);
int oaep_max_payload_length = (rsa_mod_size - 2 - (2 * OAEP_HASH_SIZE));
if (oaep_max_payload_length < 0 || plain_data_length > (size_t)oaep_max_payload_length) {
/* The plaindata is too large for this key size */
Expand Down Expand Up @@ -223,8 +281,6 @@ soter_status_t soter_asym_cipher_decrypt(soter_asym_cipher_t* asym_cipher,
size_t* plain_data_length)
{
EVP_PKEY* pkey;
RSA* rsa;
const BIGNUM* d = NULL;
int rsa_mod_size;
size_t output_length;

Expand All @@ -243,13 +299,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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

now code looks cleaner

return SOTER_FAIL;
}

rsa_mod_size = RSA_size(rsa);

if (rsa_mod_size < 0 || cipher_data_length < (size_t)rsa_mod_size) {
/* The cipherdata is too small for this key size */
return SOTER_INVALID_PARAMETER;
Expand All @@ -258,12 +311,7 @@ soter_status_t soter_asym_cipher_decrypt(soter_asym_cipher_t* asym_cipher,
/* we can only decrypt, if we have the private key */
/* some versions of OpenSSL just crash, if you send RSA public key to EVP_PKEY_decrypt, so we do
* checks here */
#if OPENSSL_VERSION_NUMBER < 0x10100000L
d = rsa->d;
#else
RSA_get0_key(rsa, NULL, NULL, &d);
#endif
if (NULL == d) {
if (!check_rsa_private_key(pkey, asym_cipher->pkey_ctx)) {
return SOTER_INVALID_PARAMETER;
}

Expand Down
50 changes: 50 additions & 0 deletions src/soter/openssl/soter_bignum_utils.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
/*
* Copyright (c) 2023 Cossack Labs Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#ifndef THEMIS_SOTER_BIGNUM_UTILS_H
#define THEMIS_SOTER_BIGNUM_UTILS_H

#include <openssl/bn.h>
#include <openssl/opensslv.h>

#if OPENSSL_VERSION_NUMBER < 0x10100000L
/* Simple implementation for OpenSSL <1.1.0 where this function is missing */
static int BN_bn2binpad(const BIGNUM* a, unsigned char* to, int tolen)
{
int bn_size = BN_num_bytes(a);
int bytes_copied;

if (a == NULL || to == NULL) {
return -1;
}

if (tolen < bn_size) {
return -1;
}

bytes_copied = BN_bn2bin(a, to + (tolen - bn_size));

if (bytes_copied != bn_size) {
return -1;
}

memset(to, 0, (size_t)(tolen - bn_size));

return tolen;
}
#endif

#endif /* THEMIS_SOTER_BIGNUM_UTILS_H */
5 changes: 3 additions & 2 deletions src/soter/openssl/soter_ec_key.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <openssl/ec.h>
#include <openssl/evp.h>

#include "soter/openssl/soter_bignum_utils.h"
#include "soter/openssl/soter_ec_key_utils.h"
#include "soter/soter_portable_endian.h"

Expand Down Expand Up @@ -157,8 +158,8 @@ soter_status_t soter_engine_specific_to_ec_priv_key(const soter_engine_specific_
goto err;
}

if ((output_length - sizeof(soter_container_hdr_t))
!= bn_encode(d, (unsigned char*)(key + 1), output_length - sizeof(soter_container_hdr_t))) {
if (BN_bn2binpad(d, (unsigned char*)(key + 1), (int)(output_length - sizeof(soter_container_hdr_t)))
Lagovas marked this conversation as resolved.
Show resolved Hide resolved
== -1) {
res = SOTER_FAIL;
goto err;
}
Expand Down
4 changes: 2 additions & 2 deletions src/soter/openssl/soter_ec_key_3.c
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,8 @@ soter_status_t soter_engine_specific_to_ec_priv_key(const soter_engine_specific_
goto err;
}

if ((output_length - sizeof(soter_container_hdr_t))
!= bn_encode(d, (unsigned char*)(key + 1), output_length - sizeof(soter_container_hdr_t))) {
if (BN_bn2binpad(d, (unsigned char*)(key + 1), (int)(output_length - sizeof(soter_container_hdr_t)))
== -1) {
res = SOTER_FAIL;
goto err;
}
Expand Down
11 changes: 0 additions & 11 deletions src/soter/openssl/soter_ec_key_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

#include <string.h>

#include <openssl/bn.h>
#include <openssl/obj_mac.h>

#include "soter/soter_ec_key.h"
Expand Down Expand Up @@ -96,14 +95,4 @@ static char* ec_priv_key_tag(int curve)
}
}

static size_t bn_encode(const BIGNUM* bn, uint8_t* buffer, size_t length)
{
int bn_size = BN_num_bytes(bn);
if (length < (size_t)bn_size) {
return 0;
}
memset(buffer, 0, length - bn_size);
return (length - bn_size) + BN_bn2bin(bn, buffer + (length - bn_size));
}

#endif /* THEMIS_SOTER_EC_KEY_UTILS_H */
9 changes: 0 additions & 9 deletions src/soter/openssl/soter_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,6 @@

#include "soter/soter_asym_sign.h"

/*
* For the time being Themis and Soter do not support OpenSSL 3.0.
* The code seems to build fine but it fails the tests, so we're not sure
* that it is safe to use Soter with OpenSSL 3.0.
*/
#if OPENSSL_VERSION_NUMBER >= 0x30000000L && !THEMIS_EXPERIMENTAL_OPENSSL_3_SUPPORT
#error OpenSSL 3.0 is currently not supported
#endif

struct soter_hash_ctx_type {
EVP_MD_CTX* evp_md_ctx;
};
Expand Down
Loading