On Tue, Jul 2, 2024 at 10:57 AM Ruediger Pluem <rpl...@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 <ylavic....@gmail.com> wrote:
> >>>>
> >>>> On Thu, Jun 27, 2024 at 12:07 PM Ruediger Pluem <rpl...@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.
