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

Adding initial pkcs#11 support for TLS private key storage #436

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Adding initial pkcs#11 support for TLS private key storage #436

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
56 changes: 53 additions & 3 deletions src/session_client_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <errno.h>
#include <string.h>
#include <unistd.h>
#include <openssl/engine.h>

#include <libyang/libyang.h>
#include <openssl/err.h>
Expand Down Expand Up @@ -508,6 +509,10 @@ nc_client_tls_update_opts(struct nc_client_tls_opts *opts, const char *peername)
char *key;
X509_LOOKUP *lookup;
X509_VERIFY_PARAM *vpm = NULL;
const int CMD_MANDATORY = 0;
EVP_PKEY *pkey = NULL;
ENGINE * pkcs11 = NULL;
const char* opensc_pkcs11_so = getenv("MODULE");

if (!opts->tls_ctx || opts->tls_ctx_change) {
SSL_CTX_free(opts->tls_ctx);
Expand Down Expand Up @@ -540,13 +545,57 @@ nc_client_tls_update_opts(struct nc_client_tls_opts *opts, const char *peername)
} else {
key = opts->key_path;
}
if (SSL_CTX_use_PrivateKey_file(opts->tls_ctx, key, SSL_FILETYPE_PEM) != 1) {
ERR(NULL, "Loading the client private key from \'%s\' failed (%s).", key,
ERR_reason_error_string(ERR_get_error()));

ENGINE_load_dynamic();
pkcs11 = ENGINE_by_id( "pkcs11" );
if ( pkcs11 == NULL )
{
ERR(NULL, "Error retrieving 'pkcs11' engine");
rc = -1;
goto cleanup;
Comment on lines +553 to +555
Copy link
Contributor

Choose a reason for hiding this comment

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

This means that the code would hard-fail at runtime when OpenSSL is built without this dynamic engine support. It's better to only request a dynamic engine when needed, i.e., hide this behind some switch to active this new functionality.

The library should continue to work as-is when the user does not opt-in to this new feature (which by definition requires some configuration).

}

if ( 0 != access( opensc_pkcs11_so, R_OK ) )
{
ERR(NULL, "Error finding '/usr/local/lib/libpkcs11-proxy.so'");
Copy link
Contributor

Choose a reason for hiding this comment

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

paths cannot be hardcoded like that, not even in error messages

rc = -1;
goto cleanup;
}

if ( 1 != ENGINE_ctrl_cmd_string( pkcs11, "MODULE_PATH", opensc_pkcs11_so, CMD_MANDATORY ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

This loads a .so into the process' address space based on a an environment variable. That's effectively an arbitrary code execution vulnerability based on a non-standard variable name.

How does OpenSSL typically solve this?

Also, the README upstream says that the command name is called SO_PATH, not MODULE_PATH.

{
ERR(NULL, "Error setting module_path <= '/usr/local/lib/libpkcs11-proxy.so'");
rc = -1;
goto cleanup;
}

if ( 1 != ENGINE_init( pkcs11 ) )
{
ERR(NULL, "Error pkcs11: unable to initialize engine");
rc = -1;
goto cleanup;
}

if ( 1 != ENGINE_ctrl_cmd_string( pkcs11, "PIN", "1234", CMD_MANDATORY ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

a hardcoded PIN?

{
ERR(NULL, "Error setting pin");
rc = -1;
goto cleanup;
}

pkey = ENGINE_load_private_key( pkcs11, key, NULL, NULL );
if (!key)
{
ERR(NULL, "Error reading private key");
rc = -1;
goto cleanup;
}
if ((SSL_CTX_use_PrivateKey(opts->tls_ctx, pkey) != 1))
{
ERR(NULL, "Loading the client private key failed (%s).", ERR_reason_error_string(ERR_get_error()));
rc = -1;
goto cleanup;
}
if (!SSL_CTX_load_verify_locations(opts->tls_ctx, opts->ca_file, opts->ca_dir)) {
ERR(NULL, "Failed to load the locations of trusted CA certificates (%s).",
ERR_reason_error_string(ERR_get_error()));
Expand Down Expand Up @@ -617,6 +666,7 @@ nc_client_tls_update_opts(struct nc_client_tls_opts *opts, const char *peername)

cleanup:
X509_VERIFY_PARAM_free(vpm);
EVP_PKEY_free(pkey);
return rc;
}

Expand Down
70 changes: 60 additions & 10 deletions src/session_server_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <poll.h>
#include <string.h>
#include <unistd.h>
#include <openssl/engine.h>

#include <openssl/err.h>
#include <openssl/evp.h>
Expand Down Expand Up @@ -1748,7 +1749,12 @@ nc_tls_ctx_set_server_cert_key(SSL_CTX *tls_ctx, const char *cert_name)
int ret = 0;
NC_SSH_KEY_TYPE privkey_type;
X509 *cert = NULL;
EVP_PKEY *pkey = NULL;
EVP_PKEY *key = NULL;
ENGINE * pkcs11 = NULL;
const int CMD_MANDATORY = 0;
const char* opensc_pkcs11_so = getenv("MODULE");
const char* uri = getenv("TOKEN_KEY_URI");
const char* pin = getenv("DEFAULT_USER_PIN");

if (!cert_name) {
ERR(NULL, "Server certificate not set.");
Expand Down Expand Up @@ -1781,26 +1787,70 @@ nc_tls_ctx_set_server_cert_key(SSL_CTX *tls_ctx, const char *cert_name)
}

/* load the private key */
if (privkey_path) {
if (SSL_CTX_use_PrivateKey_file(tls_ctx, privkey_path, SSL_FILETYPE_PEM) != 1) {
ERR(NULL, "Loading the server private key failed (%s).", ERR_reason_error_string(ERR_get_error()));
if (privkey_path) {
if (SSL_CTX_use_PrivateKey_file(tls_ctx, privkey_path, SSL_FILETYPE_PEM) != 1) {
ERR(NULL, "1 Loading the server private key failed (%s).", ERR_reason_error_string(ERR_get_error()));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra 1 ?

ret = -1;
goto cleanup;
}
} else {

ENGINE_load_dynamic();
pkcs11 = ENGINE_by_id( "pkcs11" );
if ( pkcs11 == NULL )
{
ERR(NULL, "Error retrieving 'pkcs11' engine");
ret = -1;
goto cleanup;
}
} else {
pkey = base64der_to_privatekey(privkey_data, nc_keytype2str(privkey_type));
if (!pkey || (SSL_CTX_use_PrivateKey(tls_ctx, pkey) != 1)) {
ERR(NULL, "Loading the server private key failed (%s).", ERR_reason_error_string(ERR_get_error()));

if ( 0 != access( opensc_pkcs11_so, R_OK ) )
{
ERR(NULL, "Error finding pkcs module");
ret = -1;
goto cleanup;
}

if ( 1 != ENGINE_ctrl_cmd_string( pkcs11, "MODULE_PATH", opensc_pkcs11_so, CMD_MANDATORY ) )
{
ERR(NULL, "Error setting module_path");
ret = -1;
goto cleanup;
}

if ( 1 != ENGINE_init( pkcs11 ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation says:

This returns zero if the ENGINE was not already operational and couldn't be successfully initialised (e.g. lack of system drivers, no special hardware attached, etc), otherwise it will return nonzero to indicate that the ENGINE is now operational and will have allocated a new functional reference to the ENGINE.

...yet the code checks for 1.

Also everywhere else.

{
ERR(NULL, "Error pkcs11: unable to initialize engine");
ret = -1;
goto cleanup;
}
}

if ( 1 != ENGINE_ctrl_cmd_string( pkcs11, "PIN", pin, CMD_MANDATORY ) )
{
ERR(NULL, "Error setting pin");
ret = -1;
goto cleanup;
}

key = ENGINE_load_private_key( pkcs11, uri, NULL, NULL );
if (!key)
{
ERR(NULL, "Error reading private key using uri");
ret = -1;
goto cleanup;
}

if ((SSL_CTX_use_PrivateKey(tls_ctx, key) != 1)) {
ERR(NULL, "Loading the server private key failed (%s).", ERR_reason_error_string(ERR_get_error()));
ret = -1;
goto cleanup;
}
}
ret = nc_tls_ctx_set_server_cert_chain(tls_ctx, cert_name);

cleanup:
X509_free(cert);
EVP_PKEY_free(pkey);
EVP_PKEY_free(key);
free(cert_path);
free(cert_data);
free(privkey_path);
Expand Down