Adding initial pkcs#11 support for TLS private key storage #436
Conversation
|
|
||
| if ( 0 != access( opensc_pkcs11_so, R_OK ) ) | ||
| { | ||
| ERR(NULL, "Error finding '/usr/local/lib/libpkcs11-proxy.so'"); |
There was a problem hiding this comment.
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 ) ) |
There was a problem hiding this comment.
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 retrieving 'pkcs11' engine"); | ||
| rc = -1; | ||
| goto cleanup; |
There was a problem hiding this comment.
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).
| goto cleanup; | ||
| } | ||
|
|
||
| if ( 1 != ENGINE_ctrl_cmd_string( pkcs11, "PIN", "1234", CMD_MANDATORY ) ) |
| 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())); |
| goto cleanup; | ||
| } | ||
|
|
||
| if ( 1 != ENGINE_init( pkcs11 ) ) |
There was a problem hiding this comment.
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.
|
another related merge request is the !454 Since ENGINE is deprecated since OpenSSL 3, PROVIDER shall be used instead. |
This is an initial commit adding pkcs#11 support for TLS private key storage. This was tested using Intel 'Key Management Reference Architecture (KMRA)' project, which uses SGx enclave to store manage keys, but could be used with other pkcs#11 targets. Moves private keys from local file to remote key management storage.