On 7/3/24 2:59 AM, Yann Ylavic wrote:
> 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
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
