> Le 8 mars 2017 à 12:15, Willy Tarreau <[email protected]> a écrit :
>
> On Wed, Mar 08, 2017 at 12:01:39PM +0100, Emmanuel Hocdet wrote:
>> No because the block can be truncated, sample must be stored in a variable
>> size buffer
>> to fix that. Do fingerprint earlier avoid such manipulation.
>> With this example it's now easy to add another sample. I would do it as
>> needed :)
>
> The problem is that in order to fit your very specific need, you would
> force everyone to store an untruncated sha256, which is a cryptographically
> safe hash, for something which doesn't warrant such a use at all. Since you
> are the one who *wants* this cryptographically safe hash, you are the one
> who has to take the cost of this specific use case. So let's just have a
> 1kB or so buffer to record the complete cipher string and apply your hash
> to it. That way it fits all purposes : those who don't need to apply crypto
> on this stuff will just store the minimum amount of info and those who have
> specific needs have to pay the cost.
>
Yep, it’s why i will eventualy add a sample TLSfingerprint to only compute
cryptographically
hash with all TLS informations. It will not impact others users and will not
eat unneeded buffers.
>
>
>>
>>>>>> For the code:
>>>>>> ssl->msg_callback_arg is considering as ssl internal. It should be a
>>>>>> very good
>>>>>> think to avoid internal structures/undocumented call usage to try to
>>>>>> control the
>>>>>> beast or limit painful compatibilities.
>>>>>> In this case SSL_set_ex_data and SSL_get_ex_data will do the job.
>>>>>
>>>>> Ah yes definitely, thanks for reporting this. I guess that openssl 1.1
>>>>> will
>>>>> complain with a warning because of this.
>>>>>
>>>>>> I will try to fix the patch, it breaks my compile environment.
>>>>>
>>>>
>>>> and the patch:
>>>
>>> Thanks. Hmmm comments below :
>>>
>>> @@ -4541,13 +4522,8 @@ static int ssl_sock_from_buf(struct connection
>>> *conn, struct buffer *buf, int fl
>>> }
>>>
>>> static void ssl_sock_close(struct connection *conn) {
>>> - struct ssl_capture *capture;
>>>
>>> if (conn->xprt_ctx) {
>>> -#if OPENSSL_VERSION_NUMBER >= 0x00907000L
>>> - capture = SSL_get_msg_callback_arg(conn->xprt_ctx);
>>> - pool_free2(pool2_ssl_capture, capture);
>>> -#endif
>>>
>>> This strongly looks like a memory leak to me, it looks like instead
>>> you want this :
>>>
>>> capture = SSL_get_ex_data(conn->xprt_ctx,
>>> ssl_capture_ptr_index);
>>> pool_free2(pool2_ssl_capture, capture);
>>
>> It's done via callback when SSL is freeing:
>> ssl_capture_ptr_index = SSL_CTX_get_ex_new_index(0, NULL, NULL,
>> NULL, ssl_sock_capture_free_func);
>> (SSL_CTX_get_ex_new_index/SSL_get_ex_new_index do exactly the same job)
>
> Ah OK I didn't notice this one.
>
> However as I said to Thierry, please don't add "if (ptr)" before
> a pool_free2(), we have the same semantics as free() which is a
> NOP on NULL on all supported operating systems. If you want I can
> change it myself and merge.
>
Ok, you can do the change.
>> And patch candidate to apply next.
>
> This one looks good to me.
>
Manu
