Copilot commented on code in PR #13350:
URL: https://github.com/apache/trafficserver/pull/13350#discussion_r3670231393
##########
src/iocore/net/unit_tests/test_SSLDHParams.cc:
##########
@@ -140,6 +198,55 @@ init_with_dhparams(char const *dhparams_file)
return ok;
}
+// A fixed-passphrase callback, matching how SSLPrivateKeyHandler consults the
+// SSL_CTX default password callback to decrypt an encrypted private key.
+char test_passphrase[]{"ats-secret-pass"};
+
+int
+fixed_passphrase_cb(char *buf, int size, int /* rwflag */, void * /* u */)
+{
+ int len{static_cast<int>(std::strlen(test_passphrase))};
+ if (len > size) {
+ len = size;
+ }
+ std::memcpy(buf, test_passphrase, len);
+ return len;
Review Comment:
fixed_passphrase_cb can return a length equal to `size` and does not
NUL-terminate `buf`. The OpenSSL password callback contract expects the
returned length to fit in the provided buffer; using `size - 1` and adding a
terminator avoids out-of-bounds/unterminated passphrases when OpenSSL treats
the buffer as a C string.
##########
src/iocore/net/unit_tests/test_SSLDHParams.cc:
##########
@@ -38,14 +43,25 @@
#include <openssl/core_names.h>
#include <openssl/evp.h>
#include <openssl/pem.h>
+#include <openssl/rsa.h>
#include <openssl/ssl.h>
+#include <openssl/x509.h>
#include <cstdio>
+#include <cstring>
#include <string>
namespace
{
+std::string
+bio_to_string(BIO *bio)
+{
+ BUF_MEM *bm = nullptr;
+ BIO_get_mem_ptr(bio, &bm);
+ return std::string{bm->data, bm->length};
+}
Review Comment:
bio_to_string assumes both `bio` and the retrieved `BUF_MEM*` are non-null.
If BIO allocation fails (or the BIO isn’t a memory BIO), this will dereference
null and crash the unit test; add REQUIRE checks so failures are reported
cleanly by Catch2.
This issue also appears in the following locations of the same file:
- line 80
- line 94
##########
src/iocore/net/SSLKeyUtils.cc:
##########
@@ -188,3 +189,34 @@ set_ctx_dh(SSL_CTX *ctx, dh_key_t *pkey)
}
#endif // OPENSSL_IS_OPENSSL3
+
+bool
+use_rsa_pkey_from_file(SSL_CTX *ctx, const char *keyPath)
+{
+ ink_assert(keyPath && keyPath[0] != '\0');
+ int const result{SSL_CTX_use_RSAPrivateKey_file(ctx, keyPath,
SSL_FILETYPE_PEM)};
+ if (1 != result) {
+ char err_buf[256]{};
+ ERR_error_string_n(ERR_get_error(), err_buf, sizeof(err_buf));
+ Error("failed to load RSA key %s: %s", keyPath, err_buf);
+ }
+ return 1 == result;
+}
+
+bool
+use_rsa_pkey_from_secret_data(SSL_CTX *ctx, const char *secret_data, int
secret_data_len)
+{
+ scoped_BIO bio(BIO_new_mem_buf(secret_data, secret_data_len));
+
+ pem_password_cb *password_cb = SSL_CTX_get_default_passwd_cb(ctx);
+ void *u = SSL_CTX_get_default_passwd_cb_userdata(ctx);
+ EVP_PKEY *pkey = PEM_read_bio_PrivateKey(bio.get(), nullptr,
password_cb, u);
+ if (nullptr == pkey) {
+ return false;
+ }
+ if (!SSL_CTX_use_PrivateKey(ctx, pkey)) {
+ EVP_PKEY_free(pkey);
+ return false;
+ }
+ return true;
Review Comment:
use_rsa_pkey_from_secret_data leaks the EVP_PKEY on success. Unlike
SSL_CTX_add_extra_chain_cert, SSL_CTX_use_PrivateKey does not take ownership;
callers elsewhere in the codebase free the key after a successful attach.
##########
src/iocore/net/SSLUtils.cc:
##########
@@ -856,43 +856,24 @@ SSLMultiCertConfigLoader::default_server_ssl_ctx()
static bool
SSLPrivateKeyHandler(SSL_CTX *ctx, const char *keyPath, const char
*secret_data, int secret_data_len)
{
- EVP_PKEY *pkey = nullptr;
-#if HAVE_ENGINE_GET_DEFAULT_RSA && HAVE_ENGINE_LOAD_PRIVATE_KEY
- ENGINE *e = ENGINE_get_default_RSA();
- if (e != nullptr) {
- pkey = ENGINE_load_private_key(e, keyPath, nullptr, nullptr);
- if (pkey) {
- if (!SSL_CTX_use_PrivateKey(ctx, pkey)) {
- Dbg(dbg_ctl_ssl_load, "failed to load server private key from engine");
- EVP_PKEY_free(pkey);
- return false;
- }
- }
+ bool result{false};
+ if (keyPath && keyPath[0] != '\0') {
+ result = use_rsa_pkey_from_file(ctx, keyPath);
}
-#else
- void *e = nullptr;
-#endif
- if (pkey == nullptr) {
- scoped_BIO bio(BIO_new_mem_buf(secret_data, secret_data_len));
-
- pem_password_cb *password_cb = SSL_CTX_get_default_passwd_cb(ctx);
- void *u = SSL_CTX_get_default_passwd_cb_userdata(ctx);
- pkey = PEM_read_bio_PrivateKey(bio.get(), nullptr,
password_cb, u);
- if (nullptr == pkey) {
- Dbg(dbg_ctl_ssl_load, "failed to load server private key (%.*s) from
%s", secret_data_len < 50 ? secret_data_len : 50,
- secret_data, (!keyPath || keyPath[0] == '\0') ? "[empty key path]" :
keyPath);
- return false;
- }
- if (!SSL_CTX_use_PrivateKey(ctx, pkey)) {
- Dbg(dbg_ctl_ssl_load, "failed to attach server private key loaded from
%s",
- (!keyPath || keyPath[0] == '\0') ? "[empty key path]" : keyPath);
- EVP_PKEY_free(pkey);
- return false;
- }
- if (e == nullptr && !SSL_CTX_check_private_key(ctx)) {
- Dbg(dbg_ctl_ssl_load, "server private key does not match the certificate
public key");
- return false;
- }
+
+ if (!result) {
+ result = use_rsa_pkey_from_secret_data(ctx, secret_data, secret_data_len);
+ }
+
+ if (!result) {
+ Dbg(dbg_ctl_ssl_load, "failed to load server private key (%.*s) from %s",
secret_data_len < 50 ? secret_data_len : 50,
+ secret_data, (!keyPath || keyPath[0] == '\0') ? "[empty key path]" :
keyPath);
+ return false;
Review Comment:
The failure log prints the first bytes of `secret_data`, which can contain
private key material (e.g. "-----BEGIN PRIVATE KEY-----"). Even though this is
behind a debug tag, it’s safer to avoid logging key contents; log the length
(or a hash) instead.
##########
src/iocore/net/SSLKeyUtils.cc:
##########
@@ -29,16 +29,17 @@
#include <tscore/ink_config.h>
#endif
Review Comment:
SSLKeyUtils.cc now uses ink_assert() in code that builds for both OpenSSL 3
and non-3 (e.g. use_rsa_pkey_from_file), but <tscore/ink_assert.h> is only
included when OPENSSL_IS_OPENSSL3 is set. This will fail to compile in OpenSSL
1.1.1 builds; include ink_assert.h unconditionally (and keep ink_config.h where
needed).
##########
src/iocore/net/SSLKeyUtils.cc:
##########
@@ -188,3 +189,34 @@ set_ctx_dh(SSL_CTX *ctx, dh_key_t *pkey)
}
#endif // OPENSSL_IS_OPENSSL3
+
+bool
+use_rsa_pkey_from_file(SSL_CTX *ctx, const char *keyPath)
+{
+ ink_assert(keyPath && keyPath[0] != '\0');
+ int const result{SSL_CTX_use_RSAPrivateKey_file(ctx, keyPath,
SSL_FILETYPE_PEM)};
+ if (1 != result) {
+ char err_buf[256]{};
+ ERR_error_string_n(ERR_get_error(), err_buf, sizeof(err_buf));
+ Error("failed to load RSA key %s: %s", keyPath, err_buf);
+ }
+ return 1 == result;
Review Comment:
use_rsa_pkey_from_file uses the RSA-only loader
(SSL_CTX_use_RSAPrivateKey_file). This will fail (and currently logs an
Error()) for non-RSA keys (EC/Ed25519/etc), even though ATS supports those key
types elsewhere (e.g. SSLCreateServerContext uses SSL_CTX_use_PrivateKey_file).
Using the generic loader avoids spurious errors and supports all key types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
