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.
>
> >>>> 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.
> And patch candidate to apply next.
This one looks good to me.
Willy
