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.

Sign up for GitHub

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

Jump to bottom

Fix #272: check c_obj_create error code for OBJ_R_OID_EXISTS. #294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix #272: check c_obj_create error code for OBJ_R_OID_EXISTS. #294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ jobs:
name: Run tests
command: |
if << parameters.OQS_PROVIDER_BUILD_STATIC >> ; then
ctest --test-dir _build/
ctest --output-on-failure --test-dir _build/
else
./scripts/runtests.sh -V
fi
Expand Down
23 changes: 16 additions & 7 deletions oqsprov/oqsprov.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <openssl/core_names.h>
#include <openssl/err.h>
#include <openssl/objects.h>
#include <openssl/objectserr.h>
#include <openssl/params.h>
#include <openssl/provider.h>
#include <stdio.h>
Expand Down Expand Up @@ -917,6 +918,7 @@ int OQS_PROVIDER_ENTRYPOINT_NAME(const OSSL_CORE_HANDLE *handle,
OSSL_PARAM version_request[] = {{"openssl-version", OSSL_PARAM_UTF8_PTR,
&opensslv, sizeof(&opensslv), 0},
{NULL, 0, NULL, 0, 0}};
unsigned long last_error = 0;

OQS_init();

Expand Down Expand Up @@ -968,10 +970,14 @@ int OQS_PROVIDER_ENTRYPOINT_NAME(const OSSL_CORE_HANDLE *handle,
for (i = 0; i < OQS_OID_CNT; i += 2) {
if (!c_obj_create(handle, oqs_oid_alg_list[i], oqs_oid_alg_list[i + 1],
oqs_oid_alg_list[i + 1])) {
ERR_raise(ERR_LIB_USER, OQSPROV_R_OBJ_CREATE_ERR);
fprintf(stderr, "error registering NID for %s\n",
oqs_oid_alg_list[i + 1]);
goto end_init;
last_error = ERR_peek_last_error();
if ((last_error != ERR_PACK(ERR_LIB_OBJ, 0, OBJ_R_OID_EXISTS))
|| (OBJ_txt2nid(oqs_oid_alg_list[i]) == NID_undef)) {
Copy link
Member

Choose a reason for hiding this comment

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

rather than check for equality with NID_undef, what about checking for inequality with oqs_oid_alg_list[i]? Oh, and the parameter for OBJ_txt2nid seems to be indexed incorrectly (i-> i+1), no?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're passing oqs_oid_alg_list[i] as second argument of c_obj_create, and according to the implementation, OBJ_txt2nid is called with the second argument:

https://github.com/openssl/openssl/blob/26ecab1fd7fa7f5103ac57ef41ee0dd38fbe2ddc/crypto/provider_core.c#L2219-L2225

static int core_obj_create(const OSSL_CORE_HANDLE *prov, const char *oid,
                           const char *sn, const char *ln)
{
    /* Check if it already exists and create it if not */
    return OBJ_txt2nid(oid) != NID_undef
           || OBJ_create(oid, sn, ln) != NID_undef;
}

Although OBJ_create returns the NID as an integer, c_obj_create only returns 1 or 0. So after adding a new object using c_obj_create, we don't get back its NID as an integer, so we cannot compare it to the returned value of OBJ_txt2nid…

ERR_raise(ERR_LIB_USER, OQSPROV_R_OBJ_CREATE_ERR);
fprintf(stderr, "error registering NID for %s\n",
oqs_oid_alg_list[i + 1]);
goto end_init;
}
}

/* create object (NID) again to avoid setup corner case problems
Expand Down Expand Up @@ -1016,9 +1022,12 @@ int OQS_PROVIDER_ENTRYPOINT_NAME(const OSSL_CORE_HANDLE *handle,
|| ((libctx = OSSL_LIB_CTX_new_child(handle, orig_in)) == NULL)
|| ((*provctx = oqsx_newprovctx(libctx, handle, corebiometh))
== NULL)) {
OQS_PROV_PRINTF("OQS PROV: error creating new provider context\n");
ERR_raise(ERR_LIB_USER, OQSPROV_R_LIB_CREATE_ERR);
goto end_init;
last_error = ERR_peek_last_error();
if (last_error != ERR_PACK(ERR_LIB_OBJ, 0, OBJ_R_OID_EXISTS)) {
OQS_PROV_PRINTF("OQS PROV: error creating new provider context\n");
ERR_raise(ERR_LIB_USER, OQSPROV_R_LIB_CREATE_ERR);
goto end_init;
}
}

*out = oqsprovider_dispatch_table;
Expand Down
12 changes: 12 additions & 0 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,18 @@ set_tests_properties(oqs_endecode
)
endif()

add_executable(oqs_test_multi_thread oqs_test_multi_thread.c)
target_link_libraries(oqs_test_multi_thread PRIVATE ${OPENSSL_CRYPTO_LIBRARY})
add_test(
NAME oqs_multi_thread
COMMAND oqs_test_multi_thread
"oqsprovider"
"${CMAKE_CURRENT_SOURCE_DIR}/openssl-ca.cnf"
)
set_tests_properties(oqs_multi_thread
PROPERTIES ENVIRONMENT "OPENSSL_MODULES=${OQS_PROV_BINARY_DIR}"
)

if (OQS_PROVIDER_BUILD_STATIC)
targets_set_static_provider(oqs_test_signatures
oqs_test_kems
Expand Down
99 changes: 99 additions & 0 deletions test/oqs_test_multi_thread.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// SPDX-License-Identifier: Apache-2.0 AND MIT

#include <pthread.h>

#include <openssl/crypto.h>
#include <openssl/provider.h>

#include "test_common.h"

static const size_t N_THREADS = 32;

static const char *kModuleName = NULL;
static const char *kConfigFile = NULL;

/** \brief Loads the oqs-provider in an `OSSL_LIB_CTX` object. */
static int load_oqs_provider_thread(OSSL_LIB_CTX *lib_ctx)
{
OSSL_PROVIDER *default_provider = NULL;
OSSL_PROVIDER *oqs_provider = NULL;
int ret = -1;

#ifdef OQS_PROVIDER_STATIC

if ((default_provider = OSSL_PROVIDER_load(lib_ctx, "default")) == NULL) {
goto end;
}
if (OSSL_PROVIDER_add_builtin(lib_ctx, "oqsprovider",
OQS_PROVIDER_ENTRYPOINT_NAME)
!= 1) {
putchar('-');
goto unload_default_provider;
}
if ((oqs_provider = OSSL_PROVIDER_load(lib_ctx, "oqsprovider")) == NULL) {
putchar('/');
goto unload_default_provider;
}
ret = 0;
OSS_PROVIDER_unload(oqs_provider);

#else

if (OSSL_LIB_CTX_load_config(lib_ctx, kConfigFile) == 1
&& OSSL_PROVIDER_available(lib_ctx, kModuleName)) {
putchar('>');
ret = 0;
}
goto end;

#endif // ifdef OQS_PROVIDER_STATIC

unload_default_provider:
OSSL_PROVIDER_unload(default_provider);

end:
return ret;
}

/** \brief Creates an OSSL_LIB_CTX object and loads oqs-provider. */
static void *thread_create_ossl_lib_ctx(void *arg)
{
OSSL_LIB_CTX *lib_ctx = NULL;
int ret = -1;

(void)arg;

if ((lib_ctx = OSSL_LIB_CTX_new()) == NULL) {
goto end;
}
ret = load_oqs_provider_thread(lib_ctx);
OSSL_LIB_CTX_free(lib_ctx);

end:
return (void *)(size_t)ret;
}

int main(int argc, char **argv)
{
size_t i;
pthread_t threads[N_THREADS];
void *result;
int ret = 0;

T(argc == 3);

kModuleName = argv[1];
kConfigFile = argv[2];

for (i = 0; i < N_THREADS; ++i) {
pthread_create(threads + i, NULL, thread_create_ossl_lib_ctx, NULL);
}

for (i = 0; i < N_THREADS; ++i) {
result = NULL;
pthread_join(threads[i], &result);
ret |= (int)(size_t)result;
}

return ret;
}
Loading