You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by yl...@apache.org on 2023/03/19 21:30:47 UTC
svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Author: ylavic
Date: Sun Mar 19 21:30:47 2023
New Revision: 1908537
URL: http://svn.apache.org/viewvc?rev=1908537&view=rev
Log:
mod_ssl: Fix deprecation warnings with openssl-3.
* mod_ssl_openssl.h:
Make it the first openssl to be included openssl header, selecting the
OpenSSL api based on OPENSSL_API_COMPAT eventually.
* ssl_private.h;
Define OPENSSL_API_COMPAT to version 1.1.1 (last one supporting EGINE_ API)
before including mod_ssl_openssl.h to enable the ENGINE_ api (TODO: switch to
new "providers" api before the ENGINE_ api is abandonned..).
mod_ssl.h is now implicitely included from there.
Fix preprocessor "#define FOO (COND)" to "#if COND #define FOO 1 #else #define FOO 0".
Define MODSSL_HAVE_ENGINE_API iff OPENSSL_API_COMPAT < 3.0 (otherwise all the
engine features are disabled, only "builtin" is accepted).
Define HAVE_SRP iff OPENSSL_API_COMPAT < 3.0 (no replacement for this api
above, so it might not be implemenentedain httpd anymore at some point..).
Define X509_get_not{Before,After} if missing to the non deprecated version.
New modssl_set_io_callbacks() to factorize compat code for io callbacks.
ssl_dh_GetParamFromFile() becomes modssl_dh_from_file() for openssl < 3.0 and
modssl_dh_pkey_from_file() for openssl >= 3.0.
* mod_ssl.c, mod_ssl_ct.c, ssl_util_stapling:
Including "ssl_private.h" only is suited/enough now.
* mod_ssl_ct.c, ssl_ct_log_config:
Use EVP api with openssl >= 3 instead of the deprecated SHA256 one.
* ssl_engine_config.c(ssl_cmd_SSLCryptoDevice):
Disabled engines (besides NULL/"builtin"/NULL) unless MODSSL_HAVE_ENGINE_API.
* ssl_engine_init:
New compat modssl_runtime_lib_version() to address deprecated SSLeay().
ssl_init_Engine() does nothing unless MODSSL_HAVE_ENGINE_API.
Simplify ssl_init_server_certs() (less #ifdef-ery) with scoped local vars.
Compat loading DH parameters and EC curve from cert.
* ssl_engine_io.c, ssl_engine_kernel.c:
Implement common modssl_set_io_callbacks() and use it.
* ssl_engine_pphrase(modssl_load_engine_keypair):
Depend on MODSSL_HAVE_ENGINE_API, or return ENOTIMPL.
* ssl_util.c(modssl_is_engine_id):
No engine supported unless MODSSL_HAVE_ENGINE_API.
* ssl_util_ssl.c(modssl_dh_pkey_from_file, modssl_ec_group_from_file):
Compat with openssl >= 3.0.
Modified:
httpd/httpd/trunk/modules/ssl/mod_ssl.c
httpd/httpd/trunk/modules/ssl/mod_ssl_ct.c
httpd/httpd/trunk/modules/ssl/mod_ssl_openssl.h
httpd/httpd/trunk/modules/ssl/ssl_ct_log_config.c
httpd/httpd/trunk/modules/ssl/ssl_engine_config.c
httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c
httpd/httpd/trunk/modules/ssl/ssl_private.h
httpd/httpd/trunk/modules/ssl/ssl_util.c
httpd/httpd/trunk/modules/ssl/ssl_util_ssl.c
httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c
Modified: httpd/httpd/trunk/modules/ssl/mod_ssl.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/mod_ssl.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/mod_ssl.c (original)
+++ httpd/httpd/trunk/modules/ssl/mod_ssl.c Sun Mar 19 21:30:47 2023
@@ -25,8 +25,7 @@
*/
#include "ssl_private.h"
-#include "mod_ssl.h"
-#include "mod_ssl_openssl.h"
+
#include "util_md5.h"
#include "util_mutex.h"
#include "ap_provider.h"
Modified: httpd/httpd/trunk/modules/ssl/mod_ssl_ct.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/mod_ssl_ct.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/mod_ssl_ct.c (original)
+++ httpd/httpd/trunk/modules/ssl/mod_ssl_ct.c Sun Mar 19 21:30:47 2023
@@ -70,14 +70,13 @@
#endif
#include "mod_proxy.h"
-#include "mod_ssl.h"
-#include "mod_ssl_openssl.h"
+#include "mod_ssl_openssl.h"
#include "ssl_ct_util.h"
#include "ssl_ct_sct.h"
-#include "openssl/x509v3.h"
-#include "openssl/ocsp.h"
+#include <openssl/x509v3.h>
+#include <openssl/ocsp.h>
#if OPENSSL_VERSION_NUMBER < 0x10002003L
#error "mod_ssl_ct requires OpenSSL 1.0.2-beta3 or later"
@@ -1592,26 +1591,55 @@ static const char *gen_key(conn_rec *c,
ct_conn_config *conncfg)
{
const char *fp;
- SHA256_CTX sha256ctx;
unsigned char digest[SHA256_DIGEST_LENGTH];
fp = get_cert_fingerprint(c->pool, cc->leaf);
- SHA256_Init(&sha256ctx); /* UNDOC */
- SHA256_Update(&sha256ctx, (unsigned char *)fp, strlen(fp)); /* UNDOC */
- if (conncfg->cert_sct_list) {
- SHA256_Update(&sha256ctx, conncfg->cert_sct_list,
- conncfg->cert_sct_list_size);
- }
- if (conncfg->serverhello_sct_list) {
- SHA256_Update(&sha256ctx, conncfg->serverhello_sct_list,
- conncfg->serverhello_sct_list_size);
- }
- if (conncfg->ocsp_sct_list) {
- SHA256_Update(&sha256ctx, conncfg->ocsp_sct_list,
- conncfg->ocsp_sct_list_size);
+#if OPENSSL_VERSION_NUMBER < 0x30000000L
+ {
+ SHA256_CTX sha256ctx;
+ SHA256_Init(&sha256ctx); /* UNDOC */
+ SHA256_Update(&sha256ctx, (unsigned char *)fp, strlen(fp)); /* UNDOC */
+ if (conncfg->cert_sct_list) {
+ SHA256_Update(&sha256ctx, conncfg->cert_sct_list,
+ conncfg->cert_sct_list_size);
+ }
+ if (conncfg->serverhello_sct_list) {
+ SHA256_Update(&sha256ctx, conncfg->serverhello_sct_list,
+ conncfg->serverhello_sct_list_size);
+ }
+ if (conncfg->ocsp_sct_list) {
+ SHA256_Update(&sha256ctx, conncfg->ocsp_sct_list,
+ conncfg->ocsp_sct_list_size);
+ }
+ SHA256_Final(digest, &sha256ctx); /* UNDOC */
}
- SHA256_Final(digest, &sha256ctx); /* UNDOC */
+#else
+ {
+ EVP_MD_CTX *md_ctx;
+ unsigned int dlen = 0;
+ md_ctx = EVP_MD_CTX_create();
+ ap_assert(md_ctx != NULL);
+ ap_assert(EVP_DigestInit_ex(md_ctx, EVP_sha256(), NULL));
+ ap_assert(EVP_DigestUpdate(md_ctx, (unsigned char *)fp, strlen(fp)));
+ if (conncfg->cert_sct_list) {
+ ap_assert(EVP_DigestUpdate(md_ctx, conncfg->cert_sct_list,
+ conncfg->cert_sct_list_size));
+ }
+ if (conncfg->serverhello_sct_list) {
+ ap_assert(EVP_DigestUpdate(md_ctx, conncfg->serverhello_sct_list,
+ conncfg->serverhello_sct_list_size));
+ }
+ if (conncfg->ocsp_sct_list) {
+ ap_assert(EVP_DigestUpdate(md_ctx, conncfg->ocsp_sct_list,
+ conncfg->ocsp_sct_list_size));
+ }
+ ap_assert(EVP_DigestFinal_ex(md_ctx, digest, &dlen));
+ ap_assert(dlen == SHA256_DIGEST_LENGTH);
+ EVP_MD_CTX_destroy(md_ctx);
+ }
+#endif
+
return apr_pescape_hex(c->pool, digest, sizeof digest, 0);
}
Modified: httpd/httpd/trunk/modules/ssl/mod_ssl_openssl.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/mod_ssl_openssl.h?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/mod_ssl_openssl.h (original)
+++ httpd/httpd/trunk/modules/ssl/mod_ssl_openssl.h Sun Mar 19 21:30:47 2023
@@ -30,14 +30,17 @@
/* OpenSSL headers */
-#ifndef SSL_PRIVATE_H
#include <openssl/opensslv.h>
-#if (OPENSSL_VERSION_NUMBER >= 0x10001000)
+#if OPENSSL_VERSION_NUMBER >= 0x30000000
+#include <openssl/macros.h> /* for OPENSSL_API_LEVEL */
+#endif
+#if OPENSSL_VERSION_NUMBER >= 0x10001000
/* must be defined before including ssl.h */
#define OPENSSL_NO_SSL_INTERN
#endif
#include <openssl/ssl.h>
-#endif
+#include <openssl/evp.h>
+#include <openssl/x509.h>
/**
* init_server hook -- allow SSL_CTX-specific initialization to be performed by
Modified: httpd/httpd/trunk/modules/ssl/ssl_ct_log_config.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_ct_log_config.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_ct_log_config.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_ct_log_config.c Sun Mar 19 21:30:47 2023
@@ -115,14 +115,30 @@ static void digest_public_key(EVP_PKEY *
int len = i2d_PUBKEY(pubkey, NULL);
unsigned char *val = ap_malloc(len);
unsigned char *tmp = val;
- SHA256_CTX sha256ctx;
ap_assert(LOG_ID_SIZE == SHA256_DIGEST_LENGTH);
i2d_PUBKEY(pubkey, &tmp);
- SHA256_Init(&sha256ctx);
- SHA256_Update(&sha256ctx, (unsigned char *)val, len);
- SHA256_Final(digest, &sha256ctx);
+#if OPENSSL_VERSION_NUMBER < 0x30000000
+ {
+ SHA256_CTX sha256ctx;
+ SHA256_Init(&sha256ctx);
+ SHA256_Update(&sha256ctx, (unsigned char *)val, len);
+ SHA256_Final(digest, &sha256ctx);
+ }
+#else
+ {
+ EVP_MD_CTX *md_ctx;
+ unsigned int dlen = 0;
+ md_ctx = EVP_MD_CTX_create();
+ ap_assert(md_ctx != NULL);
+ ap_assert(EVP_DigestInit_ex(md_ctx, EVP_sha256(), NULL));
+ ap_assert(EVP_DigestUpdate(md_ctx, val, len));
+ ap_assert(EVP_DigestFinal_ex(md_ctx, digest, &dlen));
+ ap_assert(dlen == SHA256_DIGEST_LENGTH);
+ EVP_MD_CTX_destroy(md_ctx);
+ }
+#endif
free(val);
}
Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_config.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_config.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_config.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_config.c Sun Mar 19 21:30:47 2023
@@ -31,6 +31,7 @@
#include "ssl_private.h"
#include "ssl_policies.h"
+
#include "util_mutex.h"
#include "ap_provider.h"
@@ -675,7 +676,9 @@ const char *ssl_cmd_SSLCryptoDevice(cmd_
{
SSLModConfigRec *mc = myModConfig(cmd->server);
const char *err;
+#if MODSSL_HAVE_ENGINE_API
ENGINE *e;
+#endif
if ((err = ap_check_cmd_context(cmd, GLOBAL_ONLY))) {
return err;
@@ -687,13 +690,16 @@ const char *ssl_cmd_SSLCryptoDevice(cmd_
if (strcEQ(arg, "builtin")) {
mc->szCryptoDevice = NULL;
}
+#if MODSSL_HAVE_ENGINE_API
else if ((e = ENGINE_by_id(arg))) {
mc->szCryptoDevice = arg;
ENGINE_free(e);
}
+#endif
else {
err = "SSLCryptoDevice: Invalid argument; must be one of: "
"'builtin' (none)";
+#if MODSSL_HAVE_ENGINE_API
e = ENGINE_get_first();
while (e) {
err = apr_pstrcat(cmd->pool, err, ", '", ENGINE_get_id(e),
@@ -702,6 +708,7 @@ const char *ssl_cmd_SSLCryptoDevice(cmd_
* on the 'old' e, per the docs in engine.h. */
e = ENGINE_get_next(e);
}
+#endif
return err;
}
Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_init.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Sun Mar 19 21:30:47 2023
@@ -27,8 +27,7 @@
see Recursive.''
-- Unknown */
#include "ssl_private.h"
-#include "mod_ssl.h"
-#include "mod_ssl_openssl.h"
+
#include "mpm_common.h"
#include "mod_md.h"
#include "util_md5.h"
@@ -221,6 +220,16 @@ static apr_status_t modssl_fips_cleanup(
}
#endif
+static APR_INLINE unsigned long modssl_runtime_lib_version(void)
+{
+#if MODSSL_USE_OPENSSL_PRE_1_1_API
+ return SSLeay();
+#else
+ return OpenSSL_version_num();
+#endif
+}
+
+
/*
* Per-module initialization
*/
@@ -228,6 +237,7 @@ apr_status_t ssl_init_Module(apr_pool_t
apr_pool_t *ptemp,
server_rec *base_server)
{
+ unsigned long runtime_lib_version = modssl_runtime_lib_version();
SSLModConfigRec *mc = myModConfig(base_server);
SSLSrvConfigRec *sc;
server_rec *s;
@@ -236,12 +246,13 @@ apr_status_t ssl_init_Module(apr_pool_t
AP_DEBUG_ASSERT(mc);
- if (SSLeay() < MODSSL_LIBRARY_VERSION) {
+ if (runtime_lib_version < MODSSL_LIBRARY_VERSION) {
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, base_server, APLOGNO(01882)
"Init: this version of mod_ssl was compiled against "
- "a newer library (%s, version currently loaded is %s)"
+ "a newer library (%s (%s), version currently loaded is 0x%lX)"
" - may result in undefined or erroneous behavior",
- MODSSL_LIBRARY_TEXT, MODSSL_LIBRARY_DYNTEXT);
+ MODSSL_LIBRARY_TEXT, MODSSL_LIBRARY_DYNTEXT,
+ runtime_lib_version);
}
#ifdef MODSSL_USE_SSLRAND
@@ -323,11 +334,9 @@ apr_status_t ssl_init_Module(apr_pool_t
/*
* SSL external crypto device ("engine") support
*/
-#if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
if ((rv = ssl_init_Engine(base_server, p)) != APR_SUCCESS) {
return rv;
}
-#endif
ap_log_error(APLOG_MARK, APLOG_INFO, 0, base_server, APLOGNO(01883)
"Init: Initialized %s library", MODSSL_LIBRARY_NAME);
@@ -491,9 +500,9 @@ apr_status_t ssl_init_Module(apr_pool_t
* Support for external a Crypto Device ("engine"), usually
* a hardware accelerator card for crypto operations.
*/
-#if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
apr_status_t ssl_init_Engine(server_rec *s, apr_pool_t *p)
{
+#if MODSSL_HAVE_ENGINE_API
SSLModConfigRec *mc = myModConfig(s);
ENGINE *e;
@@ -525,10 +534,9 @@ apr_status_t ssl_init_Engine(server_rec
ENGINE_free(e);
}
-
+#endif
return APR_SUCCESS;
}
-#endif
#ifdef HAVE_TLSEXT
static apr_status_t ssl_init_ctx_tls_extensions(server_rec *s,
@@ -1369,15 +1377,6 @@ static int ssl_no_passwd_prompt_cb(char
return 0;
}
-static APR_INLINE int modssl_DH_bits(DH *dh)
-{
-#if OPENSSL_VERSION_NUMBER < 0x30000000L
- return DH_bits(dh);
-#else
- return BN_num_bits(DH_get0_p(dh));
-#endif
-}
-
/* SSL_CTX_use_PrivateKey_file() can fail either because the private
* key was encrypted, or due to a mismatch between an already-loaded
* cert and the key - a common misconfiguration - from calling
@@ -1403,15 +1402,10 @@ static apr_status_t ssl_init_server_cert
SSLModConfigRec *mc = myModConfig(s);
const char *vhost_id = mctx->sc->vhost_id, *key_id, *certfile, *keyfile;
int i;
- X509 *cert;
- DH *dh;
+ EVP_PKEY *pkey;
#ifdef HAVE_ECC
- EC_GROUP *ecparams = NULL;
- int nid;
- EC_KEY *eckey = NULL;
-#endif
-#ifndef HAVE_SSL_CONF_CMD
- SSL *ssl;
+ EC_GROUP *ecgroup = NULL;
+ int curve_nid = 0;
#endif
/* no OpenSSL default prompts for any of the SSL_CTX_use_* calls, please */
@@ -1422,7 +1416,7 @@ static apr_status_t ssl_init_server_cert
(certfile = APR_ARRAY_IDX(mctx->pks->cert_files, i,
const char *));
i++) {
- EVP_PKEY *pkey;
+ X509 *cert = NULL;
const char *engine_certfile = NULL;
key_id = apr_psprintf(ptemp, "%s:%d", vhost_id, i);
@@ -1465,8 +1459,6 @@ static apr_status_t ssl_init_server_cert
if (modssl_is_engine_id(keyfile)) {
apr_status_t rv;
- cert = NULL;
-
if ((rv = modssl_load_engine_keypair(s, ptemp, vhost_id,
engine_certfile, keyfile,
&cert, &pkey))) {
@@ -1537,22 +1529,21 @@ static apr_status_t ssl_init_server_cert
* assume that if SSL_CONF is available, it's OpenSSL 1.0.2 or later,
* and SSL_CTX_get0_certificate is implemented.)
*/
- if (!(cert = SSL_CTX_get0_certificate(mctx->ssl_ctx))) {
+ cert = SSL_CTX_get0_certificate(mctx->ssl_ctx);
#else
- ssl = SSL_new(mctx->ssl_ctx);
- if (ssl) {
- /* Workaround bug in SSL_get_certificate in OpenSSL 0.9.8y */
- SSL_set_connect_state(ssl);
- cert = SSL_get_certificate(ssl);
+ {
+ SSL *ssl = SSL_new(mctx->ssl_ctx);
+ if (ssl) {
+ /* Workaround bug in SSL_get_certificate in OpenSSL 0.9.8y */
+ SSL_set_connect_state(ssl);
+ cert = SSL_get_certificate(ssl);
+ SSL_free(ssl);
+ }
}
- if (!ssl || !cert) {
#endif
+ if (!cert) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02566)
"Unable to retrieve certificate %s", key_id);
-#ifndef HAVE_SSL_CONF_CMD
- if (ssl)
- SSL_free(ssl);
-#endif
return APR_EGENERAL;
}
@@ -1574,10 +1565,6 @@ static apr_status_t ssl_init_server_cert
}
#endif
-#ifndef HAVE_SSL_CONF_CMD
- SSL_free(ssl);
-#endif
-
ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(02568)
"Certificate and private key %s configured from %s and %s",
key_id, certfile, keyfile);
@@ -1587,15 +1574,33 @@ static apr_status_t ssl_init_server_cert
* Try to read DH parameters from the (first) SSLCertificateFile
*/
certfile = APR_ARRAY_IDX(mctx->pks->cert_files, 0, const char *);
- if (certfile && !modssl_is_engine_id(certfile)
- && (dh = ssl_dh_GetParamFromFile(certfile))) {
- /* ### This should be replaced with SSL_CTX_set0_tmp_dh_pkey()
- * for OpenSSL 3.0+. */
- SSL_CTX_set_tmp_dh(mctx->ssl_ctx, dh);
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02540)
- "Custom DH parameters (%d bits) for %s loaded from %s",
- modssl_DH_bits(dh), vhost_id, certfile);
- DH_free(dh);
+ if (certfile && !modssl_is_engine_id(certfile)) {
+ int done = 0, num_bits = 0;
+#if OPENSSL_VERSION_NUMBER < 0x30000000L
+ DH *dh = modssl_dh_from_file(certfile);
+ if (dh) {
+ num_bits = DH_bits(dh);
+ SSL_CTX_set_tmp_dh(mctx->ssl_ctx, dh);
+ DH_free(dh);
+ done = 1;
+ }
+#else
+ pkey = modssl_dh_pkey_from_file(certfile);
+ if (pkey) {
+ num_bits = EVP_PKEY_get_bits(pkey);
+ if (!SSL_CTX_set0_tmp_dh_pkey(mctx->ssl_ctx, pkey)) {
+ EVP_PKEY_free(pkey);
+ }
+ else {
+ done = 1;
+ }
+ }
+#endif
+ if (done) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02540)
+ "Custom DH parameters (%d bits) for %s loaded from %s",
+ num_bits, vhost_id, certfile);
+ }
}
#if !MODSSL_USE_OPENSSL_PRE_1_1_API
else {
@@ -1610,13 +1615,27 @@ static apr_status_t ssl_init_server_cert
* Similarly, try to read the ECDH curve name from SSLCertificateFile...
*/
if (certfile && !modssl_is_engine_id(certfile)
- && (ecparams = ssl_ec_GetParamFromFile(certfile))
- && (nid = EC_GROUP_get_curve_name(ecparams))
- && (eckey = EC_KEY_new_by_curve_name(nid))) {
- SSL_CTX_set_tmp_ecdh(mctx->ssl_ctx, eckey);
- ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02541)
- "ECDH curve %s for %s specified in %s",
- OBJ_nid2sn(nid), vhost_id, certfile);
+ && (ecgroup = modssl_ec_group_from_file(certfile))
+ && (curve_nid = EC_GROUP_get_curve_name(ecgroup))) {
+#if OPENSSL_VERSION_NUMBER < 0x30000000L
+ EC_KEY *eckey = EC_KEY_new_by_curve_name(curve_nid);
+ if (eckey) {
+ SSL_CTX_set_tmp_ecdh(mctx->ssl_ctx, eckey);
+ EC_KEY_free(eckey);
+ }
+ else {
+ curve_nid = 0;
+ }
+#else
+ if (!SSL_CTX_set1_curves(mctx->ssl_ctx, &curve_nid, 1)) {
+ curve_nid = 0;
+ }
+#endif
+ if (curve_nid) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02541)
+ "ECDH curve %s for %s specified in %s",
+ OBJ_nid2sn(curve_nid), vhost_id, certfile);
+ }
}
/*
* ...otherwise, enable auto curve selection (OpenSSL 1.0.2)
@@ -1624,18 +1643,20 @@ static apr_status_t ssl_init_server_cert
* ECDH is always enabled in 1.1.0 unless excluded from SSLCipherList
*/
#if MODSSL_USE_OPENSSL_PRE_1_1_API
- else {
+ if (!curve_nid) {
#if defined(SSL_CTX_set_ecdh_auto)
SSL_CTX_set_ecdh_auto(mctx->ssl_ctx, 1);
#else
- eckey = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
- SSL_CTX_set_tmp_ecdh(mctx->ssl_ctx, eckey);
+ EC_KEY *eckey = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
+ if (eckey) {
+ SSL_CTX_set_tmp_ecdh(mctx->ssl_ctx, eckey);
+ EC_KEY_free(eckey);
+ }
#endif
}
#endif
/* OpenSSL assures us that _free() is NULL-safe */
- EC_KEY_free(eckey);
- EC_GROUP_free(ecparams);
+ EC_GROUP_free(ecgroup);
#endif
return APR_SUCCESS;
Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Mar 19 21:30:47 2023
@@ -28,8 +28,7 @@
core keeps dumping.''
-- Unknown */
#include "ssl_private.h"
-#include "mod_ssl.h"
-#include "mod_ssl_openssl.h"
+
#include "apr_date.h"
APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(ssl, SSL, int, proxy_post_handshake,
@@ -2283,14 +2282,7 @@ apr_status_t ssl_io_filter_init(conn_rec
ssl_io_filter_cleanup, apr_pool_cleanup_null);
if (APLOG_CS_IS_LEVEL(c, mySrvFromConn(c), APLOG_TRACE4)) {
- BIO *rbio = SSL_get_rbio(ssl),
- *wbio = SSL_get_wbio(ssl);
- BIO_set_callback(rbio, ssl_io_data_cb);
- BIO_set_callback_arg(rbio, (void *)ssl);
- if (wbio && wbio != rbio) {
- BIO_set_callback(wbio, ssl_io_data_cb);
- BIO_set_callback_arg(wbio, (void *)ssl);
- }
+ modssl_set_io_callbacks(ssl);
}
return APR_SUCCESS;
@@ -2374,13 +2366,22 @@ static void ssl_io_data_dump(conn_rec *c
"+-------------------------------------------------------------------------+");
}
-long ssl_io_data_cb(BIO *bio, int cmd,
- const char *argp,
- int argi, long argl, long rc)
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
+ size_t len, int argi, long argl, int rc,
+ size_t *processed)
+#else
+static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
+ int argi, long argl, long rc)
+#endif
{
SSL *ssl;
conn_rec *c;
server_rec *s;
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+ (void)len;
+ (void)processed;
+#endif
if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
return rc;
@@ -2402,7 +2403,7 @@ long ssl_io_data_cb(BIO *bio, int cmd,
"%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
MODSSL_LIBRARY_NAME,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
- rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
+ (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
bio, argp, dump);
if (*dump != '\0' && argp != NULL)
ssl_io_data_dump(c, s, argp, rc);
@@ -2417,3 +2418,25 @@ long ssl_io_data_cb(BIO *bio, int cmd,
}
return rc;
}
+
+static APR_INLINE void set_bio_callback(BIO *bio, void *arg)
+{
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+ BIO_set_callback_ex(bio, modssl_io_cb);
+#else
+ BIO_set_callback(bio, modssl_io_cb);
+#endif
+ BIO_set_callback_arg(bio, arg);
+}
+
+void modssl_set_io_callbacks(SSL *ssl)
+{
+ BIO *rbio = SSL_get_rbio(ssl),
+ *wbio = SSL_get_wbio(ssl);
+ if (rbio) {
+ set_bio_callback(rbio, ssl);
+ }
+ if (wbio && wbio != rbio) {
+ set_bio_callback(wbio, ssl);
+ }
+}
Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Sun Mar 19 21:30:47 2023
@@ -2588,6 +2588,7 @@ static int ssl_find_vhost(void *serverna
sc->server->pks->service_unavailable : 0;
ap_update_child_status_from_server(c->sbh, SERVER_BUSY_READ, c, s);
+
/*
* There is one special filter callback, which is set
* very early depending on the base_server's log level.
@@ -2596,14 +2597,7 @@ static int ssl_find_vhost(void *serverna
* we need to set that callback here.
*/
if (APLOGtrace4(s)) {
- BIO *rbio = SSL_get_rbio(ssl),
- *wbio = SSL_get_wbio(ssl);
- BIO_set_callback(rbio, ssl_io_data_cb);
- BIO_set_callback_arg(rbio, (void *)ssl);
- if (wbio && wbio != rbio) {
- BIO_set_callback(wbio, ssl_io_data_cb);
- BIO_set_callback_arg(wbio, (void *)ssl);
- }
+ modssl_set_io_callbacks(ssl);
}
return 1;
Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c Sun Mar 19 21:30:47 2023
@@ -30,6 +30,8 @@
-- Clifford Stoll */
#include "ssl_private.h"
+#include <openssl/ui.h>
+
typedef struct {
server_rec *s;
apr_pool_t *p;
@@ -574,7 +576,7 @@ int ssl_pphrase_Handle_CB(char *buf, int
return (len);
}
-#if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
+#if MODSSL_HAVE_ENGINE_API
/* OpenSSL UI implementation for passphrase entry; largely duplicated
* from ssl_pphrase_Handle_CB but adjusted for UI API. TODO: Might be
@@ -797,7 +799,7 @@ apr_status_t modssl_load_engine_keypair(
const char *certid, const char *keyid,
X509 **pubkey, EVP_PKEY **privkey)
{
-#if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
+#if MODSSL_HAVE_ENGINE_API
const char *c, *scheme;
ENGINE *e;
UI_METHOD *ui_method = get_passphrase_ui(p);
Modified: httpd/httpd/trunk/modules/ssl/ssl_private.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_private.h?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_private.h (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_private.h Sun Mar 19 21:30:47 2023
@@ -83,16 +83,13 @@
#include "ap_expr.h"
-/* OpenSSL headers */
-#include <openssl/opensslv.h>
-#if (OPENSSL_VERSION_NUMBER >= 0x10001000)
-/* must be defined before including ssl.h */
-#define OPENSSL_NO_SSL_INTERN
-#endif
-#if OPENSSL_VERSION_NUMBER >= 0x30000000
-#include <openssl/core_names.h>
+/* keep first for compat API */
+#ifndef OPENSSL_API_COMPAT
+#define OPENSSL_API_COMPAT 0x10101000 /* for ENGINE_ API */
#endif
-#include <openssl/ssl.h>
+#include "mod_ssl_openssl.h"
+
+/* OpenSSL headers */
#include <openssl/err.h>
#include <openssl/x509.h>
#include <openssl/pem.h>
@@ -102,12 +99,24 @@
#include <openssl/x509v3.h>
#include <openssl/x509_vfy.h>
#include <openssl/ocsp.h>
+#include <openssl/dh.h>
+#if OPENSSL_VERSION_NUMBER >= 0x30000000
+#include <openssl/core_names.h>
+#endif
/* Avoid tripping over an engine build installed globally and detected
* when the user points at an explicit non-engine flavor of OpenSSL
*/
#if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
+#if OPENSSL_VERSION_NUMBER < 0x30000000 \
+ || (defined(OPENSSL_API_LEVEL) && OPENSSL_API_LEVEL < 30000)
#include <openssl/engine.h>
+#define MODSSL_HAVE_ENGINE_API 1
+#endif
+#ifndef MODSSL_HAVE_ENGINE_API
+#define MODSSL_HAVE_ENGINE_API 0
+#endif
+
#endif
#if (OPENSSL_VERSION_NUMBER < 0x0090801f)
@@ -142,10 +151,18 @@
* include most changes from OpenSSL >= 1.1 (new functions, macros,
* deprecations, ...), so we have to work around this...
*/
-#define MODSSL_USE_OPENSSL_PRE_1_1_API (LIBRESSL_VERSION_NUMBER < 0x2070000f)
+#if LIBRESSL_VERSION_NUMBER < 0x2070000f
+#define MODSSL_USE_OPENSSL_PRE_1_1_API 1
+#else
+#define MODSSL_USE_OPENSSL_PRE_1_1_API 0
+#endif
#else /* defined(LIBRESSL_VERSION_NUMBER) */
-#define MODSSL_USE_OPENSSL_PRE_1_1_API (OPENSSL_VERSION_NUMBER < 0x10100000L)
+#if OPENSSL_VERSION_NUMBER < 0x10100000L
+#define MODSSL_USE_OPENSSL_PRE_1_1_API 1
+#else
+#define MODSSL_USE_OPENSSL_PRE_1_1_API 0
#endif
+#endif /* defined(LIBRESSL_VERSION_NUMBER) */
#if OPENSSL_VERSION_NUMBER < 0x10101000
#define MODSSL_USE_SSLRAND
@@ -215,7 +232,10 @@
#endif
/* Secure Remote Password */
-#if !defined(OPENSSL_NO_SRP) && defined(SSL_CTRL_SET_TLS_EXT_SRP_USERNAME_CB)
+#if !defined(OPENSSL_NO_SRP) \
+ && (OPENSSL_VERSION_NUMBER < 0x30000000L \
+ || (defined(OPENSSL_API_LEVEL) && OPENSSL_API_LEVEL < 30000)) \
+ && defined(SSL_CTRL_SET_TLS_EXT_SRP_USERNAME_CB)
#define HAVE_SRP
#include <openssl/srp.h>
#endif
@@ -262,6 +282,14 @@ void free_bio_methods(void);
#endif
#endif
+/* those may be deprecated */
+#ifndef X509_get_notBefore
+#define X509_get_notBefore X509_getm_notBefore
+#endif
+#ifndef X509_get_notAfter
+#define X509_get_notAfter X509_getm_notAfter
+#endif
+
#if OPENSSL_VERSION_NUMBER >= 0x10101000L && !defined(LIBRESSL_VERSION_NUMBER)
#define HAVE_OPENSSL_KEYLOG
#endif
@@ -1022,7 +1050,7 @@ void modssl_callback_keylog(cons
/** I/O */
apr_status_t ssl_io_filter_init(conn_rec *, request_rec *r, SSL *);
void ssl_io_filter_register(apr_pool_t *);
-long ssl_io_data_cb(BIO *, int, const char *, int, long, long);
+void modssl_set_io_callbacks(SSL *ssl);
/* ssl_io_buffer_fill fills the setaside buffering of the HTTP request
* to allow an SSL renegotiation to take place. */
@@ -1064,9 +1092,13 @@ apr_status_t modssl_load_engine_keypair(
X509 **pubkey, EVP_PKEY **privkey);
/** Diffie-Hellman Parameter Support */
-DH *ssl_dh_GetParamFromFile(const char *);
+#if OPENSSL_VERSION_NUMBER < 0x30000000L
+DH *modssl_dh_from_file(const char *);
+#else
+EVP_PKEY *modssl_dh_pkey_from_file(const char *);
+#endif
#ifdef HAVE_ECC
-EC_GROUP *ssl_ec_GetParamFromFile(const char *);
+EC_GROUP *modssl_ec_group_from_file(const char *);
#endif
/* Store the EVP_PKEY key (serialized into DER) in the hash table with
Modified: httpd/httpd/trunk/modules/ssl/ssl_util.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_util.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_util.c Sun Mar 19 21:30:47 2023
@@ -500,7 +500,7 @@ void ssl_util_thread_setup(apr_pool_t *p
int modssl_is_engine_id(const char *name)
{
-#if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
+#if MODSSL_USE_ENGINE_API
/* ### Can handle any other special ENGINE key names here? */
return strncmp(name, "pkcs11:", 7) == 0;
#else
Modified: httpd/httpd/trunk/modules/ssl/ssl_util_ssl.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_ssl.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_util_ssl.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_util_ssl.c Sun Mar 19 21:30:47 2023
@@ -464,29 +464,52 @@ BOOL modssl_X509_match_name(apr_pool_t *
** _________________________________________________________________
*/
-DH *ssl_dh_GetParamFromFile(const char *file)
+#if OPENSSL_VERSION_NUMBER < 0x30000000L
+DH *modssl_dh_from_file(const char *file)
{
- DH *dh = NULL;
+ DH *dh;
BIO *bio;
if ((bio = BIO_new_file(file, "r")) == NULL)
return NULL;
dh = PEM_read_bio_DHparams(bio, NULL, NULL, NULL);
BIO_free(bio);
- return (dh);
+
+ return dh;
+}
+#else
+EVP_PKEY *modssl_dh_pkey_from_file(const char *file)
+{
+ EVP_PKEY *pkey;
+ BIO *bio;
+
+ if ((bio = BIO_new_file(file, "r")) == NULL)
+ return NULL;
+ pkey = PEM_read_bio_Parameters(bio, NULL);
+ BIO_free(bio);
+
+ return pkey;
}
+#endif
#ifdef HAVE_ECC
-EC_GROUP *ssl_ec_GetParamFromFile(const char *file)
+EC_GROUP *modssl_ec_group_from_file(const char *file)
{
- EC_GROUP *group = NULL;
+ EC_GROUP *group;
BIO *bio;
if ((bio = BIO_new_file(file, "r")) == NULL)
return NULL;
+#if OPENSSL_VERSION_NUMBER < 0x30000000L
group = PEM_read_bio_ECPKParameters(bio, NULL, NULL, NULL);
+#else
+ group = PEM_ASN1_read_bio((void *)d2i_ECPKParameters,
+ PEM_STRING_ECPARAMETERS, bio,
+ NULL, NULL, NULL);
+#endif
BIO_free(bio);
- return (group);
+
+ return group;
}
#endif
Modified: httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c?rev=1908537&r1=1908536&r2=1908537&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c Sun Mar 19 21:30:47 2023
@@ -29,9 +29,9 @@
-- Alexei Sayle */
#include "ssl_private.h"
+
#include "ap_mpm.h"
#include "apr_thread_mutex.h"
-#include "mod_ssl_openssl.h"
APR_IMPLEMENT_OPTIONAL_HOOK_RUN_ALL(ssl, SSL, int, init_stapling_status,
(server_rec *s, apr_pool_t *p,
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Joe Orton <jo...@redhat.com>.
On Sun, Mar 19, 2023 at 09:30:47PM -0000, ylavic@apache.org wrote:
> Author: ylavic
> Date: Sun Mar 19 21:30:47 2023
> New Revision: 1908537
>
> URL: http://svn.apache.org/viewvc?rev=1908537&view=rev
> Log:
> mod_ssl: Fix deprecation warnings with openssl-3.
Great stuff, thank you Yann!
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 7/3/24 11:09 AM, Yann Ylavic wrote:
> On Wed, Jul 3, 2024 at 11:01 AM Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Wed, Jul 3, 2024 at 10:57 AM Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem <rp...@apache.org> wrote:
>>>>
>>>> On 7/3/24 2:59 AM, Yann Ylavic wrote:
>>>>> On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rp...@apache.org> wrote:
>>>>>>
>>>>>> Updated patch.
>>> []
>>>>>> const char *dump = "";
>>>>>> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
>>>>>> if (argp != NULL)
>>>>>> @@ -2400,23 +2413,28 @@
>>>>>> dump = "(Oops, no memory buffer?)";
>>>>>> }
>>>>>> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
>>>>>> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
>>>>>> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
>>>>>> + " bytes %s BIO#%pp [mem: %pp] %s",
>>>>>> MODSSL_LIBRARY_NAME,
>>>>>> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
>>>>>> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>>>>>> + actual_len, requested_len,
>>>>>> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>>>>>> bio, argp, dump);
>>>>>> if (*dump != '\0' && argp != NULL)
>>
>> Here we could check for APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7) too, to
>> save the call when ssl_io_data_dump() is a noop anyway.
>
> In this area, maybe we want to move the check for APLOG_TRACE4 from
> the caller of modssl_set_io_callbacks() to the helper itself (which
> knows better). Something like the attached eventually, feel free to
> use since you are touching this code ;)
>
This is a good idea, but I would handle this separately. Feel free to commit.
Regards
Rüdiger
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jul 3, 2024 at 11:01 AM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Wed, Jul 3, 2024 at 10:57 AM Yann Ylavic <yl...@gmail.com> wrote:
> >
> > On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem <rp...@apache.org> wrote:
> > >
> > > On 7/3/24 2:59 AM, Yann Ylavic wrote:
> > > > On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rp...@apache.org> wrote:
> > > >>
> > > >> Updated patch.
> > []
> > > >> const char *dump = "";
> > > >> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> > > >> if (argp != NULL)
> > > >> @@ -2400,23 +2413,28 @@
> > > >> dump = "(Oops, no memory buffer?)";
> > > >> }
> > > >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> > > >> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> > > >> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> > > >> + " bytes %s BIO#%pp [mem: %pp] %s",
> > > >> MODSSL_LIBRARY_NAME,
> > > >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> > > >> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> > > >> + actual_len, requested_len,
> > > >> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> > > >> bio, argp, dump);
> > > >> if (*dump != '\0' && argp != NULL)
>
> Here we could check for APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7) too, to
> save the call when ssl_io_data_dump() is a noop anyway.
In this area, maybe we want to move the check for APLOG_TRACE4 from
the caller of modssl_set_io_callbacks() to the helper itself (which
knows better). Something like the attached eventually, feel free to
use since you are touching this code ;)
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jul 3, 2024 at 4:40 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 7/3/24 3:23 PM, Yann Ylavic wrote:
> > On Wed, Jul 3, 2024 at 2:34 PM Ruediger Pluem <rp...@apache.org> wrote:
> >>
> >> Thanks for the intense review. I hope I captured all stuff in the below.
> >
> > Thanks for working on this.
>
> Committed in r1918880 considering your comments. Feel free to add the patch you provided
> with regards to the move of the trace level checking.
Done in r1918883.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jul 3, 2024 at 4:40 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 7/3/24 3:23 PM, Yann Ylavic wrote:
> > On Wed, Jul 3, 2024 at 2:34 PM Ruediger Pluem <rp...@apache.org> wrote:
> >>
> >> Thanks for the intense review. I hope I captured all stuff in the below.
> >
> > Thanks for working on this.
>
> Committed in r1918880 considering your comments. Feel free to add the patch you provided
> with regards to the move of the trace level checking.
Great, thanks Rüdiger!
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 7/3/24 3:23 PM, Yann Ylavic wrote:
> On Wed, Jul 3, 2024 at 2:34 PM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> Thanks for the intense review. I hope I captured all stuff in the below.
>
> Thanks for working on this.
Committed in r1918880 considering your comments. Feel free to add the patch you provided
with regards to the move of the trace level checking.
Regards
Rüdiger
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jul 3, 2024 at 2:34 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> Thanks for the intense review. I hope I captured all stuff in the below.
Thanks for working on this.
> I intentionally left the 'len' parameter of ssl_io_data_dump an apr_size_t in case we ever get to the conclusion that we want to
> make MODSSL_IO_DUMP_MAX larger. It will fit then automatically.
>
> Index: modules/ssl/ssl_engine_io.c
> ===================================================================
> --- modules/ssl/ssl_engine_io.c (revision 1918869)
> +++ modules/ssl/ssl_engine_io.c (working copy)
> @@ -2308,7 +2308,7 @@
> #define DUMP_WIDTH 16
>
> static void ssl_io_data_dump(conn_rec *c, server_rec *s,
> - const char *b, long len)
> + const char *b, apr_size_t len)
> {
> char buf[256];
> int i, j, rows, trunc, pos;
The problem is that some of the i,j,.. should apr_size_t too then.
I'd rather we use "int len" here because APR_INT32_MAX is really a
hard limit IMHO, we shouldn't take seconds/minutes to log things to
disk anyway, and it'd avoid thinking about overflows here and keep
things simple.
> @@ -2361,11 +2361,13 @@
> }
> if (trunc > 0)
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
> - "| %04ld - <SPACES/NULS>", len + trunc);
> + "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len + (size_t)trunc);
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
> "+-------------------------------------------------------------------------+");
> }
>
> +#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX
> +
> #if OPENSSL_VERSION_NUMBER >= 0x30000000L
> static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
> size_t len, int argi, long argl, int rc,
> @@ -2379,9 +2381,9 @@
> conn_rec *c;
> server_rec *s;
> #if OPENSSL_VERSION_NUMBER >= 0x30000000L
> - (void)len;
> - (void)processed;
> + (void)argi;
> #endif
> + (void)argl;
>
> if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
> return rc;
> @@ -2391,27 +2393,58 @@
>
> if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
> || cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
> - if (rc >= 0) {
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
> + apr_size_t requested_len = len;
> + /*
> + * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
> + * BIO_write_ex functions return value and not the one of
> + * BIO_read and BIO_write. Hence 0 indicates an error.
> + */
> + int ok = (rc > 0);
> +#else
> + apr_size_t requested_len = (apr_size_t)argi;
> + int ok = (rc >= 0);
> +#endif
> + if (ok) {
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
> + apr_size_t actual_len = *processed;
> +#else
> + apr_size_t actual_len = (apr_size_t)rc;
> +#endif
> const char *dump = "";
> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> - if (argp != NULL)
> + if (argp == NULL)
> + dump = "(Oops, no memory buffer?)";
> + else if (actual_len > MODSSL_IO_DUMP_MAX)
> + dump = "(BIO dump follows, truncated to "
> + APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
> + else
> dump = "(BIO dump follows)";
> - else
> - dump = "(Oops, no memory buffer?)";
> }
> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> + " bytes %s BIO#%pp [mem: %pp] %s",
> MODSSL_LIBRARY_NAME,
> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> + actual_len, requested_len,
> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> bio, argp, dump);
> - if (*dump != '\0' && argp != NULL)
> - ssl_io_data_dump(c, s, argp, rc);
> + /*
> + * *dump will only be != '\0' if
> + * APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)
> + */
> + if (*dump != '\0' && argp != NULL) {
> + apr_size_t dump_len = (actual_len >= MODSSL_IO_DUMP_MAX
> + ? MODSSL_IO_DUMP_MAX
> + : actual_len);
So "int dump_len" here should be (more than) enough?
> + ssl_io_data_dump(c, s, argp, dump_len);
> + }
> }
> else {
> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> - "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
> - MODSSL_LIBRARY_NAME, argi,
> + "%s: I/O error, %" APR_SIZE_T_FMT
> + " bytes expected to %s on BIO#%pp [mem: %pp]",
> + MODSSL_LIBRARY_NAME, requested_len,
> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> bio, argp);
> }
Regards;
Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 7/3/24 11:01 AM, Yann Ylavic wrote:
> On Wed, Jul 3, 2024 at 10:57 AM Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem <rp...@apache.org> wrote:
>>>
>>> On 7/3/24 2:59 AM, Yann Ylavic wrote:
>>>> On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rp...@apache.org> wrote:
>>>>>
>>>>> Updated patch.
>> []
>>>>> const char *dump = "";
>>>>> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
>>>>> if (argp != NULL)
>>>>> @@ -2400,23 +2413,28 @@
>>>>> dump = "(Oops, no memory buffer?)";
>>>>> }
>>>>> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
>>>>> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
>>>>> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
>>>>> + " bytes %s BIO#%pp [mem: %pp] %s",
>>>>> MODSSL_LIBRARY_NAME,
>>>>> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
>>>>> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>>>>> + actual_len, requested_len,
>>>>> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>>>>> bio, argp, dump);
>>>>> if (*dump != '\0' && argp != NULL)
>
> Here we could check for APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7) too, to
> save the call when ssl_io_data_dump() is a noop anyway.
>
>>>>> - ssl_io_data_dump(c, s, argp, rc);
>>>>> + ssl_io_data_dump(c, s, argp, actual_len);
>>>> And here:
>>>> long dump_len = (actual_len >= APR_UINT16_MAX
>>>> ? APR_UINT16_MAX
>>>> : actual_len);
>>>
>>> Hm. As you point out: SSL records should be of limited length anyway, but if we really truncate the output shouldn't we log this?
>>> A log message like: Dump truncated to first XXX bytes?
>>
>> Something like this to initialize "dump" above?
>>
>> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
>> if (argp == NULL)
>> dump = "(Oops, no memory buffer?)";
>> else if (actual_len > MODSSL_IO_DUMP_MAX)
>> dump = "(BIO dump follows, truncated to "
>> APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
>> else
>> dump = "(BIO dump follows)";
>> }
>>
>> where we'd "#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX" and use it
>> everywhere eventually.
>>
>>>
>>>> ssl_io_data_dump(c, s, argp, dump_len);
>>>>> }
>>>>> else {
>>>>> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
>>>>> - "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
>>>>> - MODSSL_LIBRARY_NAME, argi,
>>>>> + "%s: I/O error, %" APR_SIZE_T_FMT
>>>>> + " bytes expected to %s on BIO#%pp [mem: %pp]",
>>>>> + MODSSL_LIBRARY_NAME, requested_len,
>>>>> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
>>>>> bio, argp);
>>>>> }
>>>>> }
>>>>> return rc;
>>>>> +#undef requested_len
>>>>> +#undef actual_len
>>>>> }
>>>> ?
>
Thanks for the intense review. I hope I captured all stuff in the below.
I intentionally left the 'len' parameter of ssl_io_data_dump an apr_size_t in case we ever get to the conclusion that we want to
make MODSSL_IO_DUMP_MAX larger. It will fit then automatically.
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1918869)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2308,7 +2308,7 @@
#define DUMP_WIDTH 16
static void ssl_io_data_dump(conn_rec *c, server_rec *s,
- const char *b, long len)
+ const char *b, apr_size_t len)
{
char buf[256];
int i, j, rows, trunc, pos;
@@ -2361,11 +2361,13 @@
}
if (trunc > 0)
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
- "| %04ld - <SPACES/NULS>", len + trunc);
+ "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len + (size_t)trunc);
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
"+-------------------------------------------------------------------------+");
}
+#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX
+
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
size_t len, int argi, long argl, int rc,
@@ -2379,9 +2381,9 @@
conn_rec *c;
server_rec *s;
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
- (void)len;
- (void)processed;
+ (void)argi;
#endif
+ (void)argl;
if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
return rc;
@@ -2391,27 +2393,58 @@
if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
|| cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
- if (rc >= 0) {
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+ apr_size_t requested_len = len;
+ /*
+ * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
+ * BIO_write_ex functions return value and not the one of
+ * BIO_read and BIO_write. Hence 0 indicates an error.
+ */
+ int ok = (rc > 0);
+#else
+ apr_size_t requested_len = (apr_size_t)argi;
+ int ok = (rc >= 0);
+#endif
+ if (ok) {
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+ apr_size_t actual_len = *processed;
+#else
+ apr_size_t actual_len = (apr_size_t)rc;
+#endif
const char *dump = "";
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
- if (argp != NULL)
+ if (argp == NULL)
+ dump = "(Oops, no memory buffer?)";
+ else if (actual_len > MODSSL_IO_DUMP_MAX)
+ dump = "(BIO dump follows, truncated to "
+ APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
+ else
dump = "(BIO dump follows)";
- else
- dump = "(Oops, no memory buffer?)";
}
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
- "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
+ "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
+ " bytes %s BIO#%pp [mem: %pp] %s",
MODSSL_LIBRARY_NAME,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
- (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
+ actual_len, requested_len,
+ (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
bio, argp, dump);
- if (*dump != '\0' && argp != NULL)
- ssl_io_data_dump(c, s, argp, rc);
+ /*
+ * *dump will only be != '\0' if
+ * APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)
+ */
+ if (*dump != '\0' && argp != NULL) {
+ apr_size_t dump_len = (actual_len >= MODSSL_IO_DUMP_MAX
+ ? MODSSL_IO_DUMP_MAX
+ : actual_len);
+ ssl_io_data_dump(c, s, argp, dump_len);
+ }
}
else {
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
- "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
- MODSSL_LIBRARY_NAME, argi,
+ "%s: I/O error, %" APR_SIZE_T_FMT
+ " bytes expected to %s on BIO#%pp [mem: %pp]",
+ MODSSL_LIBRARY_NAME, requested_len,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
bio, argp);
}
Regards
Rüdiger
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jul 3, 2024 at 10:57 AM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem <rp...@apache.org> wrote:
> >
> > On 7/3/24 2:59 AM, Yann Ylavic wrote:
> > > On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rp...@apache.org> wrote:
> > >>
> > >> Updated patch.
> []
> > >> const char *dump = "";
> > >> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> > >> if (argp != NULL)
> > >> @@ -2400,23 +2413,28 @@
> > >> dump = "(Oops, no memory buffer?)";
> > >> }
> > >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> > >> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> > >> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> > >> + " bytes %s BIO#%pp [mem: %pp] %s",
> > >> MODSSL_LIBRARY_NAME,
> > >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> > >> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> > >> + actual_len, requested_len,
> > >> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> > >> bio, argp, dump);
> > >> if (*dump != '\0' && argp != NULL)
Here we could check for APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7) too, to
save the call when ssl_io_data_dump() is a noop anyway.
> > >> - ssl_io_data_dump(c, s, argp, rc);
> > >> + ssl_io_data_dump(c, s, argp, actual_len);
> > > And here:
> > > long dump_len = (actual_len >= APR_UINT16_MAX
> > > ? APR_UINT16_MAX
> > > : actual_len);
> >
> > Hm. As you point out: SSL records should be of limited length anyway, but if we really truncate the output shouldn't we log this?
> > A log message like: Dump truncated to first XXX bytes?
>
> Something like this to initialize "dump" above?
>
> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> if (argp == NULL)
> dump = "(Oops, no memory buffer?)";
> else if (actual_len > MODSSL_IO_DUMP_MAX)
> dump = "(BIO dump follows, truncated to "
> APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
> else
> dump = "(BIO dump follows)";
> }
>
> where we'd "#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX" and use it
> everywhere eventually.
>
> >
> > > ssl_io_data_dump(c, s, argp, dump_len);
> > >> }
> > >> else {
> > >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> > >> - "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
> > >> - MODSSL_LIBRARY_NAME, argi,
> > >> + "%s: I/O error, %" APR_SIZE_T_FMT
> > >> + " bytes expected to %s on BIO#%pp [mem: %pp]",
> > >> + MODSSL_LIBRARY_NAME, requested_len,
> > >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> > >> bio, argp);
> > >> }
> > >> }
> > >> return rc;
> > >> +#undef requested_len
> > >> +#undef actual_len
> > >> }
> > > ?
>
>
> Cheers;
> Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jul 3, 2024 at 8:58 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 7/3/24 2:59 AM, Yann Ylavic wrote:
> > On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rp...@apache.org> wrote:
> >>
> >> Updated patch.
[]
> >> const char *dump = "";
> >> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> >> if (argp != NULL)
> >> @@ -2400,23 +2413,28 @@
> >> dump = "(Oops, no memory buffer?)";
> >> }
> >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> >> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> >> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> >> + " bytes %s BIO#%pp [mem: %pp] %s",
> >> MODSSL_LIBRARY_NAME,
> >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> >> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> >> + actual_len, requested_len,
> >> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> >> bio, argp, dump);
> >> if (*dump != '\0' && argp != NULL)
> >> - ssl_io_data_dump(c, s, argp, rc);
> >> + ssl_io_data_dump(c, s, argp, actual_len);
> > And here:
> > long dump_len = (actual_len >= APR_UINT16_MAX
> > ? APR_UINT16_MAX
> > : actual_len);
>
> Hm. As you point out: SSL records should be of limited length anyway, but if we really truncate the output shouldn't we log this?
> A log message like: Dump truncated to first XXX bytes?
Something like this to initialize "dump" above?
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
if (argp == NULL)
dump = "(Oops, no memory buffer?)";
else if (actual_len > MODSSL_IO_DUMP_MAX)
dump = "(BIO dump follows, truncated to "
APR_STRINGIFY(MODSSL_IO_DUMP_MAX) ")";
else
dump = "(BIO dump follows)";
}
where we'd "#define MODSSL_IO_DUMP_MAX APR_UINT16_MAX" and use it
everywhere eventually.
>
> > ssl_io_data_dump(c, s, argp, dump_len);
> >> }
> >> else {
> >> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> >> - "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
> >> - MODSSL_LIBRARY_NAME, argi,
> >> + "%s: I/O error, %" APR_SIZE_T_FMT
> >> + " bytes expected to %s on BIO#%pp [mem: %pp]",
> >> + MODSSL_LIBRARY_NAME, requested_len,
> >> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> >> bio, argp);
> >> }
> >> }
> >> return rc;
> >> +#undef requested_len
> >> +#undef actual_len
> >> }
> > ?
Cheers;
Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 7/3/24 2:59 AM, Yann Ylavic wrote:
> On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 7/1/24 3:01 PM, Ruediger Pluem wrote:
>>>
>>>
>>> On 6/27/24 3:48 PM, Ruediger Pluem wrote:
>>>>
>>>>
>>>> On 6/27/24 12:47 PM, Yann Ylavic wrote:
>>>>> On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic <yl...@gmail.com> wrote:
>>>>>>
>>>>>> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rp...@apache.org> wrote:
>>>>>>>
>>>>>>> How about the below? I am yet undecided if I should follow the below approach to have
>>>>>>> two completely separate callbacks depending on the OpenSSL version or one with a lot of
>>>>>>> #If statements in it, but as much shared code as possible. Thoughts?
>>>>>>
>>>>>> Hm, I wouldn't duplicate the whole thing depending on openssl version.
>>>>>>
>>>>>> Something like the attached maybe? modssl_io_cb() will just consider
>>>>>> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
>>>>>> than enough for debug logs..
>>>>>
>>>>> We could even log the real length still if it matters, see attached v2.
>>>>
>>>> Thanks. Unfortunately the meaning of rc varies depending on if we have the ex or the non ex callback.
>>>> This is not in the documentation but only in the OpenSSL code :-(.
>>>> Furthermore the processed amount of data is in *processed in the ex case whereas in the non ex case it is in
>>>> rc. The intended amount of data to be processed is in len in the ex case and in argi in the non ex case.
>>>> Hence I propose the patch below. Of course we can have a longer debate if the len parameter to ssl_io_data_dump
>>>> should be int or size_t :-). And yes dumping more than APR_INT32_MAX bytes to the log might be bad.
>>>
>>> Any further comments on whether we should limit the dump to APR_INT32_MAX or not? I would guess that it will not matter
>>> in practice, but I am still open to it.
>>> BTW: I guess
>>>
>>> int len = data_len & APR_INT32_MAX;
>>>
>>> would be wrong. It would need to be
>>>
>>> int len = data_len > APR_INT32_MAX ? APR_INT32_MAX : (int)data_len;
>>>
>>> instead.
>
> Right! Maybe APR_UINT16_MAX is enough, I don't think we should log
> more undecipherable text than that :)
> IIRC an SSL/TLS payload is 16K (still?) anyway so we shouldn't be
> called for more than that (though multiple times)?
>
>>
>> Updated patch.
>>
>> Index: modules/ssl/ssl_engine_io.c
>> ===================================================================
>> --- modules/ssl/ssl_engine_io.c (revision 1918813)
>> +++ modules/ssl/ssl_engine_io.c (working copy)
>> @@ -2308,7 +2308,7 @@
>> #define DUMP_WIDTH 16
>>
>> static void ssl_io_data_dump(conn_rec *c, server_rec *s,
>> - const char *b, long len)
>> + const char *b, size_t len)
>> {
>> char buf[256];
>> int i, j, rows, trunc, pos;
>> @@ -2361,7 +2361,7 @@
>> }
>> if (trunc > 0)
>> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
>> - "| %04ld - <SPACES/NULS>", len + trunc);
>> + "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len + (size_t)trunc);
>> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
>> "+-------------------------------------------------------------------------+");
>> }
>> @@ -2379,9 +2379,9 @@
>> conn_rec *c;
>> server_rec *s;
>> #if OPENSSL_VERSION_NUMBER >= 0x30000000L
>> - (void)len;
>> - (void)processed;
>> + (void)argi;
>> #endif
>> + (void)argl;
>>
>> if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
>> return rc;
>> @@ -2391,7 +2391,20 @@
>>
>> if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
>> || cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
>> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
>> +#define requested_len (len)
>> +#define actual_len (*processed)
> requested_len = len
>> + /*
>> + * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
>> + * BIO_write_ex functions return value and not the one of
>> + * BIO_read and BIO_write. Hence 0 indicates an error.
>> + */
>> + if (rc > 0) {
>> +#else
>> +#define requested_len ((size_t)argi)
>> +#define actual_len ((size_t)rc)
>> if (rc >= 0) {
>> +#endif
> Maybe use variables like:
> #if OPENSSL_VERSION_NUMBER >= 0x30000000L
> apr_size_t requested_len = len;
> int ok = (rc > 0);
> #else
> apr_size_t requested_len = argi;
> int ok = (rc >= 0);
> #endif
> if (ok) {
> #if OPENSSL_VERSION_NUMBER >= 0x30000000L
> apr_size_t actual_len = *processed;
> #else
> apr_size_t actual_len = rc;
> #endif
I am fine with this. Maybe my desire to save stack memory by not using further
variables was a bit over the top here.
>> const char *dump = "";
>> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
>> if (argp != NULL)
>> @@ -2400,23 +2413,28 @@
>> dump = "(Oops, no memory buffer?)";
>> }
>> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
>> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
>> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
>> + " bytes %s BIO#%pp [mem: %pp] %s",
>> MODSSL_LIBRARY_NAME,
>> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
>> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>> + actual_len, requested_len,
>> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>> bio, argp, dump);
>> if (*dump != '\0' && argp != NULL)
>> - ssl_io_data_dump(c, s, argp, rc);
>> + ssl_io_data_dump(c, s, argp, actual_len);
> And here:
> long dump_len = (actual_len >= APR_UINT16_MAX
> ? APR_UINT16_MAX
> : actual_len);
Hm. As you point out: SSL records should be of limited length anyway, but if we really truncate the output shouldn't we log this?
A log message like: Dump truncated to first XXX bytes?
> ssl_io_data_dump(c, s, argp, dump_len);
>> }
>> else {
>> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
>> - "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
>> - MODSSL_LIBRARY_NAME, argi,
>> + "%s: I/O error, %" APR_SIZE_T_FMT
>> + " bytes expected to %s on BIO#%pp [mem: %pp]",
>> + MODSSL_LIBRARY_NAME, requested_len,
>> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
>> bio, argp);
>> }
>> }
>> return rc;
>> +#undef requested_len
>> +#undef actual_len
>> }
> ?
Regards
Rüdiger
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 7/1/24 3:01 PM, Ruediger Pluem wrote:
> >
> >
> > On 6/27/24 3:48 PM, Ruediger Pluem wrote:
> >>
> >>
> >> On 6/27/24 12:47 PM, Yann Ylavic wrote:
> >>> On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic <yl...@gmail.com> wrote:
> >>>>
> >>>> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rp...@apache.org> wrote:
> >>>>>
> >>>>> How about the below? I am yet undecided if I should follow the below approach to have
> >>>>> two completely separate callbacks depending on the OpenSSL version or one with a lot of
> >>>>> #If statements in it, but as much shared code as possible. Thoughts?
> >>>>
> >>>> Hm, I wouldn't duplicate the whole thing depending on openssl version.
> >>>>
> >>>> Something like the attached maybe? modssl_io_cb() will just consider
> >>>> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
> >>>> than enough for debug logs..
> >>>
> >>> We could even log the real length still if it matters, see attached v2.
> >>
> >> Thanks. Unfortunately the meaning of rc varies depending on if we have the ex or the non ex callback.
> >> This is not in the documentation but only in the OpenSSL code :-(.
> >> Furthermore the processed amount of data is in *processed in the ex case whereas in the non ex case it is in
> >> rc. The intended amount of data to be processed is in len in the ex case and in argi in the non ex case.
> >> Hence I propose the patch below. Of course we can have a longer debate if the len parameter to ssl_io_data_dump
> >> should be int or size_t :-). And yes dumping more than APR_INT32_MAX bytes to the log might be bad.
> >
> > Any further comments on whether we should limit the dump to APR_INT32_MAX or not? I would guess that it will not matter
> > in practice, but I am still open to it.
> > BTW: I guess
> >
> > int len = data_len & APR_INT32_MAX;
> >
> > would be wrong. It would need to be
> >
> > int len = data_len > APR_INT32_MAX ? APR_INT32_MAX : (int)data_len;
> >
> > instead.
Right! Maybe APR_UINT16_MAX is enough, I don't think we should log
more undecipherable text than that :)
IIRC an SSL/TLS payload is 16K (still?) anyway so we shouldn't be
called for more than that (though multiple times)?
>
> Updated patch.
>
> Index: modules/ssl/ssl_engine_io.c
> ===================================================================
> --- modules/ssl/ssl_engine_io.c (revision 1918813)
> +++ modules/ssl/ssl_engine_io.c (working copy)
> @@ -2308,7 +2308,7 @@
> #define DUMP_WIDTH 16
>
> static void ssl_io_data_dump(conn_rec *c, server_rec *s,
> - const char *b, long len)
> + const char *b, size_t len)
> {
> char buf[256];
> int i, j, rows, trunc, pos;
> @@ -2361,7 +2361,7 @@
> }
> if (trunc > 0)
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
> - "| %04ld - <SPACES/NULS>", len + trunc);
> + "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len + (size_t)trunc);
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
> "+-------------------------------------------------------------------------+");
> }
> @@ -2379,9 +2379,9 @@
> conn_rec *c;
> server_rec *s;
> #if OPENSSL_VERSION_NUMBER >= 0x30000000L
> - (void)len;
> - (void)processed;
> + (void)argi;
> #endif
> + (void)argl;
>
> if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
> return rc;
> @@ -2391,7 +2391,20 @@
>
> if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
> || cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
> +#define requested_len (len)
> +#define actual_len (*processed)
requested_len = len
> + /*
> + * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
> + * BIO_write_ex functions return value and not the one of
> + * BIO_read and BIO_write. Hence 0 indicates an error.
> + */
> + if (rc > 0) {
> +#else
> +#define requested_len ((size_t)argi)
> +#define actual_len ((size_t)rc)
> if (rc >= 0) {
> +#endif
Maybe use variables like:
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
apr_size_t requested_len = len;
int ok = (rc > 0);
#else
apr_size_t requested_len = argi;
int ok = (rc >= 0);
#endif
if (ok) {
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
apr_size_t actual_len = *processed;
#else
apr_size_t actual_len = rc;
#endif
> const char *dump = "";
> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> if (argp != NULL)
> @@ -2400,23 +2413,28 @@
> dump = "(Oops, no memory buffer?)";
> }
> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> + " bytes %s BIO#%pp [mem: %pp] %s",
> MODSSL_LIBRARY_NAME,
> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> + actual_len, requested_len,
> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> bio, argp, dump);
> if (*dump != '\0' && argp != NULL)
> - ssl_io_data_dump(c, s, argp, rc);
> + ssl_io_data_dump(c, s, argp, actual_len);
And here:
long dump_len = (actual_len >= APR_UINT16_MAX
? APR_UINT16_MAX
: actual_len);
ssl_io_data_dump(c, s, argp, dump_len);
> }
> else {
> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> - "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
> - MODSSL_LIBRARY_NAME, argi,
> + "%s: I/O error, %" APR_SIZE_T_FMT
> + " bytes expected to %s on BIO#%pp [mem: %pp]",
> + MODSSL_LIBRARY_NAME, requested_len,
> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> bio, argp);
> }
> }
> return rc;
> +#undef requested_len
> +#undef actual_len
> }
?
Regards;
Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 7/1/24 3:01 PM, Ruediger Pluem wrote:
>
>
> On 6/27/24 3:48 PM, Ruediger Pluem wrote:
>>
>>
>> On 6/27/24 12:47 PM, Yann Ylavic wrote:
>>> On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic <yl...@gmail.com> wrote:
>>>>
>>>> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rp...@apache.org> wrote:
>>>>>
>>>>> How about the below? I am yet undecided if I should follow the below approach to have
>>>>> two completely separate callbacks depending on the OpenSSL version or one with a lot of
>>>>> #If statements in it, but as much shared code as possible. Thoughts?
>>>>
>>>> Hm, I wouldn't duplicate the whole thing depending on openssl version.
>>>>
>>>> Something like the attached maybe? modssl_io_cb() will just consider
>>>> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
>>>> than enough for debug logs..
>>>
>>> We could even log the real length still if it matters, see attached v2.
>>
>> Thanks. Unfortunately the meaning of rc varies depending on if we have the ex or the non ex callback.
>> This is not in the documentation but only in the OpenSSL code :-(.
>> Furthermore the processed amount of data is in *processed in the ex case whereas in the non ex case it is in
>> rc. The intended amount of data to be processed is in len in the ex case and in argi in the non ex case.
>> Hence I propose the patch below. Of course we can have a longer debate if the len parameter to ssl_io_data_dump
>> should be int or size_t :-). And yes dumping more than APR_INT32_MAX bytes to the log might be bad.
>
> Any further comments on whether we should limit the dump to APR_INT32_MAX or not? I would guess that it will not matter
> in practice, but I am still open to it.
> BTW: I guess
>
> int len = data_len & APR_INT32_MAX;
>
> would be wrong. It would need to be
>
> int len = data_len > APR_INT32_MAX ? APR_INT32_MAX : (int)data_len;
>
> instead.
Updated patch.
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1918813)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2308,7 +2308,7 @@
#define DUMP_WIDTH 16
static void ssl_io_data_dump(conn_rec *c, server_rec *s,
- const char *b, long len)
+ const char *b, size_t len)
{
char buf[256];
int i, j, rows, trunc, pos;
@@ -2361,7 +2361,7 @@
}
if (trunc > 0)
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
- "| %04ld - <SPACES/NULS>", len + trunc);
+ "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len + (size_t)trunc);
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
"+-------------------------------------------------------------------------+");
}
@@ -2379,9 +2379,9 @@
conn_rec *c;
server_rec *s;
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
- (void)len;
- (void)processed;
+ (void)argi;
#endif
+ (void)argl;
if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
return rc;
@@ -2391,7 +2391,20 @@
if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
|| cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+#define requested_len (len)
+#define actual_len (*processed)
+ /*
+ * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
+ * BIO_write_ex functions return value and not the one of
+ * BIO_read and BIO_write. Hence 0 indicates an error.
+ */
+ if (rc > 0) {
+#else
+#define requested_len ((size_t)argi)
+#define actual_len ((size_t)rc)
if (rc >= 0) {
+#endif
const char *dump = "";
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
if (argp != NULL)
@@ -2400,23 +2413,28 @@
dump = "(Oops, no memory buffer?)";
}
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
- "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
+ "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
+ " bytes %s BIO#%pp [mem: %pp] %s",
MODSSL_LIBRARY_NAME,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
- (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
+ actual_len, requested_len,
+ (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
bio, argp, dump);
if (*dump != '\0' && argp != NULL)
- ssl_io_data_dump(c, s, argp, rc);
+ ssl_io_data_dump(c, s, argp, actual_len);
}
else {
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
- "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
- MODSSL_LIBRARY_NAME, argi,
+ "%s: I/O error, %" APR_SIZE_T_FMT
+ " bytes expected to %s on BIO#%pp [mem: %pp]",
+ MODSSL_LIBRARY_NAME, requested_len,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
bio, argp);
}
}
return rc;
+#undef requested_len
+#undef actual_len
}
static APR_INLINE void set_bio_callback(BIO *bio, void *arg)
Regards
Rüdiger
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/27/24 3:48 PM, Ruediger Pluem wrote:
>
>
> On 6/27/24 12:47 PM, Yann Ylavic wrote:
>> On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rp...@apache.org> wrote:
>>>>
>>>> How about the below? I am yet undecided if I should follow the below approach to have
>>>> two completely separate callbacks depending on the OpenSSL version or one with a lot of
>>>> #If statements in it, but as much shared code as possible. Thoughts?
>>>
>>> Hm, I wouldn't duplicate the whole thing depending on openssl version.
>>>
>>> Something like the attached maybe? modssl_io_cb() will just consider
>>> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
>>> than enough for debug logs..
>>
>> We could even log the real length still if it matters, see attached v2.
>
> Thanks. Unfortunately the meaning of rc varies depending on if we have the ex or the non ex callback.
> This is not in the documentation but only in the OpenSSL code :-(.
> Furthermore the processed amount of data is in *processed in the ex case whereas in the non ex case it is in
> rc. The intended amount of data to be processed is in len in the ex case and in argi in the non ex case.
> Hence I propose the patch below. Of course we can have a longer debate if the len parameter to ssl_io_data_dump
> should be int or size_t :-). And yes dumping more than APR_INT32_MAX bytes to the log might be bad.
Any further comments on whether we should limit the dump to APR_INT32_MAX or not? I would guess that it will not matter
in practice, but I am still open to it.
BTW: I guess
int len = data_len & APR_INT32_MAX;
would be wrong. It would need to be
int len = data_len > APR_INT32_MAX ? APR_INT32_MAX : (int)data_len;
instead.
Regards
Rüdiger
>
>
> Index: modules/ssl/ssl_engine_io.c
> ===================================================================
> --- modules/ssl/ssl_engine_io.c (revision 1918662)
> +++ modules/ssl/ssl_engine_io.c (working copy)
> @@ -2308,7 +2308,7 @@
> #define DUMP_WIDTH 16
>
> static void ssl_io_data_dump(conn_rec *c, server_rec *s,
> - const char *b, long len)
> + const char *b, size_t len)
> {
> char buf[256];
> int i, j, rows, trunc, pos;
> @@ -2361,7 +2361,7 @@
> }
> if (trunc > 0)
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
> - "| %04ld - <SPACES/NULS>", len + trunc);
> + "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len + (size_t)trunc);
> ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
> "+-------------------------------------------------------------------------+");
> }
> @@ -2379,9 +2379,9 @@
> conn_rec *c;
> server_rec *s;
> #if OPENSSL_VERSION_NUMBER >= 0x30000000L
> - (void)len;
> - (void)processed;
> + (void)argi;
> #endif
> + (void)argl;
>
> if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
> return rc;
> @@ -2391,7 +2391,20 @@
>
> if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
> || cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
> +#if OPENSSL_VERSION_NUMBER >= 0x30000000L
> +#define requested_len (len)
> +#define actual_len (*processed)
> + /*
> + * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
> + * BIO_write_ex functions return value and not the one of
> + * BIO_read and BIO_write. Hence 0 indicates an error.
> + */
> + if (rc > 0) {
> +#else
> +#define requested_len ((size_t)argi)
> +#define actual_len ((size_t)rc)
> if (rc >= 0) {
> +#endif
> const char *dump = "";
> if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
> if (argp != NULL)
> @@ -2400,18 +2413,21 @@
> dump = "(Oops, no memory buffer?)";
> }
> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> - "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> + "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
> + " bytes %s BIO#%pp [mem: %pp] %s",
> MODSSL_LIBRARY_NAME,
> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> - (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> + actual_len, requested_len,
> + (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> bio, argp, dump);
> if (*dump != '\0' && argp != NULL)
> - ssl_io_data_dump(c, s, argp, rc);
> + ssl_io_data_dump(c, s, argp, actual_len);
> }
> else {
> ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
> - "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
> - MODSSL_LIBRARY_NAME, argi,
> + "%s: I/O error, %" APR_SIZE_T_FMT
> + " bytes expected to %s on BIO#%pp [mem: %pp]",
> + MODSSL_LIBRARY_NAME, requested_len,
> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> bio, argp);
> }
>
> Regards
>
> Rüdiger
>
>>
>>>
>>> Regards;
>>> Yann.
>
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/27/24 12:47 PM, Yann Ylavic wrote:
> On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rp...@apache.org> wrote:
>>>
>>> How about the below? I am yet undecided if I should follow the below approach to have
>>> two completely separate callbacks depending on the OpenSSL version or one with a lot of
>>> #If statements in it, but as much shared code as possible. Thoughts?
>>
>> Hm, I wouldn't duplicate the whole thing depending on openssl version.
>>
>> Something like the attached maybe? modssl_io_cb() will just consider
>> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
>> than enough for debug logs..
>
> We could even log the real length still if it matters, see attached v2.
Thanks. Unfortunately the meaning of rc varies depending on if we have the ex or the non ex callback.
This is not in the documentation but only in the OpenSSL code :-(.
Furthermore the processed amount of data is in *processed in the ex case whereas in the non ex case it is in
rc. The intended amount of data to be processed is in len in the ex case and in argi in the non ex case.
Hence I propose the patch below. Of course we can have a longer debate if the len parameter to ssl_io_data_dump
should be int or size_t :-). And yes dumping more than APR_INT32_MAX bytes to the log might be bad.
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1918662)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2308,7 +2308,7 @@
#define DUMP_WIDTH 16
static void ssl_io_data_dump(conn_rec *c, server_rec *s,
- const char *b, long len)
+ const char *b, size_t len)
{
char buf[256];
int i, j, rows, trunc, pos;
@@ -2361,7 +2361,7 @@
}
if (trunc > 0)
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
- "| %04ld - <SPACES/NULS>", len + trunc);
+ "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len + (size_t)trunc);
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
"+-------------------------------------------------------------------------+");
}
@@ -2379,9 +2379,9 @@
conn_rec *c;
server_rec *s;
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
- (void)len;
- (void)processed;
+ (void)argi;
#endif
+ (void)argl;
if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
return rc;
@@ -2391,7 +2391,20 @@
if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
|| cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+#define requested_len (len)
+#define actual_len (*processed)
+ /*
+ * On OpenSSL >= 3 rc uses the meaning of the BIO_read_ex and
+ * BIO_write_ex functions return value and not the one of
+ * BIO_read and BIO_write. Hence 0 indicates an error.
+ */
+ if (rc > 0) {
+#else
+#define requested_len ((size_t)argi)
+#define actual_len ((size_t)rc)
if (rc >= 0) {
+#endif
const char *dump = "";
if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
if (argp != NULL)
@@ -2400,18 +2413,21 @@
dump = "(Oops, no memory buffer?)";
}
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
- "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
+ "%s: %s %" APR_SIZE_T_FMT "/%" APR_SIZE_T_FMT
+ " bytes %s BIO#%pp [mem: %pp] %s",
MODSSL_LIBRARY_NAME,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
- (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
+ actual_len, requested_len,
+ (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
bio, argp, dump);
if (*dump != '\0' && argp != NULL)
- ssl_io_data_dump(c, s, argp, rc);
+ ssl_io_data_dump(c, s, argp, actual_len);
}
else {
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
- "%s: I/O error, %d bytes expected to %s on BIO#%pp [mem: %pp]",
- MODSSL_LIBRARY_NAME, argi,
+ "%s: I/O error, %" APR_SIZE_T_FMT
+ " bytes expected to %s on BIO#%pp [mem: %pp]",
+ MODSSL_LIBRARY_NAME, requested_len,
(cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
bio, argp);
}
Regards
Rüdiger
>
>>
>> Regards;
>> Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Jun 27, 2024 at 12:38 PM Yann Ylavic <yl...@gmail.com> wrote:
>
> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rp...@apache.org> wrote:
> >
> > How about the below? I am yet undecided if I should follow the below approach to have
> > two completely separate callbacks depending on the OpenSSL version or one with a lot of
> > #If statements in it, but as much shared code as possible. Thoughts?
>
> Hm, I wouldn't duplicate the whole thing depending on openssl version.
>
> Something like the attached maybe? modssl_io_cb() will just consider
> up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
> than enough for debug logs..
We could even log the real length still if it matters, see attached v2.
>
> Regards;
> Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rp...@apache.org> wrote:
>
> How about the below? I am yet undecided if I should follow the below approach to have
> two completely separate callbacks depending on the OpenSSL version or one with a lot of
> #If statements in it, but as much shared code as possible. Thoughts?
Hm, I wouldn't duplicate the whole thing depending on openssl version.
Something like the attached maybe? modssl_io_cb() will just consider
up to APR_INT32_MAX bytes, more shouldn't happen anyway and it's more
than enough for debug logs..
Regards;
Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 6/26/24 11:33 AM, Yann Ylavic wrote:
> On Wed, Jun 26, 2024 at 10:28 AM Ruediger Pluem <rp...@apache.org> wrote:
>>
>> On 3/19/23 10:30 PM, ylavic@apache.org wrote:
>>>
>>> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
>>> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Mar 19 21:30:47 2023
>>
>>> @@ -2402,7 +2403,7 @@ long ssl_io_data_cb(BIO *bio, int cmd,
>>> "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
>>> MODSSL_LIBRARY_NAME,
>>> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
>>> - rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>>> + (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>>
>> I think rc has a different meaning with OpenSSL 3. I think we need to use len in case of OpenSSL 3.
>> I noticed that with OpenSSL 3 only single bytes get dumped and no longer the whole buffer.
>>
>>> bio, argp, dump);
>>> if (*dump != '\0' && argp != NULL)
>>> ssl_io_data_dump(c, s, argp, rc);
>>
>> Similar to above. I think we need to use len here in case of OpenSSL 3.
>> If my findings are seen as correct I could work on a patch :-).
>
> I think you are right from the man page of BIO_set_callback{,_ex}(),
> please go ahead ;)
How about the below? I am yet undecided if I should follow the below approach to have
two completely separate callbacks depending on the OpenSSL version or one with a lot of
#If statements in it, but as much shared code as possible. Thoughts?
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1918662)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -2308,7 +2308,7 @@
#define DUMP_WIDTH 16
static void ssl_io_data_dump(conn_rec *c, server_rec *s,
- const char *b, long len)
+ const char *b, size_t len)
{
char buf[256];
int i, j, rows, trunc, pos;
@@ -2361,7 +2361,7 @@
}
if (trunc > 0)
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
- "| %04ld - <SPACES/NULS>", len + trunc);
+ "| %04" APR_SIZE_T_FMT " - <SPACES/NULS>", len + trunc);
ap_log_cserror(APLOG_MARK, APLOG_TRACE7, 0, c, s,
"+-------------------------------------------------------------------------+");
}
@@ -2370,18 +2370,54 @@
static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
size_t len, int argi, long argl, int rc,
size_t *processed)
+{
+ SSL *ssl;
+ conn_rec *c;
+ server_rec *s;
+
+ if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
+ return rc;
+ if ((c = (conn_rec *)SSL_get_app_data(ssl)) == NULL)
+ return rc;
+ s = mySrvFromConn(c);
+
+ if ( cmd == (BIO_CB_WRITE|BIO_CB_RETURN)
+ || cmd == (BIO_CB_READ |BIO_CB_RETURN) ) {
+ if (rc > 0) {
+ const char *dump = "";
+ if (APLOG_CS_IS_LEVEL(c, s, APLOG_TRACE7)) {
+ if (argp != NULL)
+ dump = "(BIO dump follows)";
+ else
+ dump = "(Oops, no memory buffer?)";
+ }
+ ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
+ "%s: %s " APR_SIZE_T_FMT "/" APR_SIZE_T_FMT " bytes %s BIO#%pp [mem: %pp] %s",
+ MODSSL_LIBRARY_NAME,
+ (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
+ *processed, len, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
+ bio, argp, dump);
+ if (*dump != '\0' && argp != NULL)
+ ssl_io_data_dump(c, s, argp, *processed);
+ }
+ else {
+ ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
+ "%s: I/O error, " APR_SIZE_T_FMT " bytes expected to %s on BIO#%pp [mem: %pp]",
+ MODSSL_LIBRARY_NAME, len,
+ (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
+ bio, argp);
+ }
+ }
+ return rc;
+}
+
#else
static long modssl_io_cb(BIO *bio, int cmd, const char *argp,
int argi, long argl, long rc)
-#endif
{
SSL *ssl;
conn_rec *c;
server_rec *s;
-#if OPENSSL_VERSION_NUMBER >= 0x30000000L
- (void)len;
- (void)processed;
-#endif
if ((ssl = (SSL *)BIO_get_callback_arg(bio)) == NULL)
return rc;
@@ -2406,7 +2442,7 @@
(long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
bio, argp, dump);
if (*dump != '\0' && argp != NULL)
- ssl_io_data_dump(c, s, argp, rc);
+ ssl_io_data_dump(c, s, argp, (size_t)rc);
}
else {
ap_log_cserror(APLOG_MARK, APLOG_TRACE4, 0, c, s,
@@ -2418,6 +2454,7 @@
}
return rc;
}
+#endif
static APR_INLINE void set_bio_callback(BIO *bio, void *arg)
{
Regards
Rüdiger
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Jun 26, 2024 at 10:28 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 3/19/23 10:30 PM, ylavic@apache.org wrote:
> >
> > --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Mar 19 21:30:47 2023
>
> > @@ -2402,7 +2403,7 @@ long ssl_io_data_cb(BIO *bio, int cmd,
> > "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> > MODSSL_LIBRARY_NAME,
> > (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> > - rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> > + (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
>
> I think rc has a different meaning with OpenSSL 3. I think we need to use len in case of OpenSSL 3.
> I noticed that with OpenSSL 3 only single bytes get dumped and no longer the whole buffer.
>
> > bio, argp, dump);
> > if (*dump != '\0' && argp != NULL)
> > ssl_io_data_dump(c, s, argp, rc);
>
> Similar to above. I think we need to use len here in case of OpenSSL 3.
> If my findings are seen as correct I could work on a patch :-).
I think you are right from the man page of BIO_set_callback{,_ex}(),
please go ahead ;)
Regards;
Yann.
Re: svn commit: r1908537 - /httpd/httpd/trunk/modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 3/19/23 10:30 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Sun Mar 19 21:30:47 2023
> New Revision: 1908537
>
> URL: http://svn.apache.org/viewvc?rev=1908537&view=rev
> Log:
> mod_ssl: Fix deprecation warnings with openssl-3.
>
> * mod_ssl_openssl.h:
> Make it the first openssl to be included openssl header, selecting the
> OpenSSL api based on OPENSSL_API_COMPAT eventually.
>
> * ssl_private.h;
> Define OPENSSL_API_COMPAT to version 1.1.1 (last one supporting EGINE_ API)
> before including mod_ssl_openssl.h to enable the ENGINE_ api (TODO: switch to
> new "providers" api before the ENGINE_ api is abandonned..).
> mod_ssl.h is now implicitely included from there.
> Fix preprocessor "#define FOO (COND)" to "#if COND #define FOO 1 #else #define FOO 0".
> Define MODSSL_HAVE_ENGINE_API iff OPENSSL_API_COMPAT < 3.0 (otherwise all the
> engine features are disabled, only "builtin" is accepted).
> Define HAVE_SRP iff OPENSSL_API_COMPAT < 3.0 (no replacement for this api
> above, so it might not be implemenentedain httpd anymore at some point..).
> Define X509_get_not{Before,After} if missing to the non deprecated version.
> New modssl_set_io_callbacks() to factorize compat code for io callbacks.
> ssl_dh_GetParamFromFile() becomes modssl_dh_from_file() for openssl < 3.0 and
> modssl_dh_pkey_from_file() for openssl >= 3.0.
>
> * mod_ssl.c, mod_ssl_ct.c, ssl_util_stapling:
> Including "ssl_private.h" only is suited/enough now.
>
> * mod_ssl_ct.c, ssl_ct_log_config:
> Use EVP api with openssl >= 3 instead of the deprecated SHA256 one.
>
> * ssl_engine_config.c(ssl_cmd_SSLCryptoDevice):
> Disabled engines (besides NULL/"builtin"/NULL) unless MODSSL_HAVE_ENGINE_API.
>
> * ssl_engine_init:
> New compat modssl_runtime_lib_version() to address deprecated SSLeay().
> ssl_init_Engine() does nothing unless MODSSL_HAVE_ENGINE_API.
> Simplify ssl_init_server_certs() (less #ifdef-ery) with scoped local vars.
> Compat loading DH parameters and EC curve from cert.
>
> * ssl_engine_io.c, ssl_engine_kernel.c:
> Implement common modssl_set_io_callbacks() and use it.
>
> * ssl_engine_pphrase(modssl_load_engine_keypair):
> Depend on MODSSL_HAVE_ENGINE_API, or return ENOTIMPL.
>
> * ssl_util.c(modssl_is_engine_id):
> No engine supported unless MODSSL_HAVE_ENGINE_API.
>
> * ssl_util_ssl.c(modssl_dh_pkey_from_file, modssl_ec_group_from_file):
> Compat with openssl >= 3.0.
>
>
> Modified:
> httpd/httpd/trunk/modules/ssl/mod_ssl.c
> httpd/httpd/trunk/modules/ssl/mod_ssl_ct.c
> httpd/httpd/trunk/modules/ssl/mod_ssl_openssl.h
> httpd/httpd/trunk/modules/ssl/ssl_ct_log_config.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_config.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_init.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c
> httpd/httpd/trunk/modules/ssl/ssl_private.h
> httpd/httpd/trunk/modules/ssl/ssl_util.c
> httpd/httpd/trunk/modules/ssl/ssl_util_ssl.c
> httpd/httpd/trunk/modules/ssl/ssl_util_stapling.c
>
return APR_SUCCESS;
>
> Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_io.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_io.c?rev=1908537&r1=1908536&r2=1908537&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_io.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_io.c Sun Mar 19 21:30:47 2023
> @@ -2402,7 +2403,7 @@ long ssl_io_data_cb(BIO *bio, int cmd,
> "%s: %s %ld/%d bytes %s BIO#%pp [mem: %pp] %s",
> MODSSL_LIBRARY_NAME,
> (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "write" : "read"),
> - rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
> + (long)rc, argi, (cmd == (BIO_CB_WRITE|BIO_CB_RETURN) ? "to" : "from"),
I think rc has a different meaning with OpenSSL 3. I think we need to use len in case of OpenSSL 3.
I noticed that with OpenSSL 3 only single bytes get dumped and no longer the whole buffer.
> bio, argp, dump);
> if (*dump != '\0' && argp != NULL)
> ssl_io_data_dump(c, s, argp, rc);
Similar to above. I think we need to use len here in case of OpenSSL 3.
If my findings are seen as correct I could work on a patch :-).
Regards
Rüdiger