Hi Dave,
On Wed, Jul 16, 2014 at 02:16:52PM -0400, Dave McCowan wrote:
> Hi Willy, Emeric--
> A commit on 6/24 changed the way ssl_sock_get_remote_common_name()
> works.
> I agree with this refactoring, unfortunately both make_proxy_line_v2()
> and the caller of make_proxy_line_v2() are using the global trash chunk as
> a workspace resulting in a memory overwrite.
Argh! yes you're right, make_proxy_line() is called with trash which is
not that fun. trash is supposed to be overwritable by any function call
which is exactly what happens here since the function became deeper.
> I've attached a patch to fix this.
>
> Commit comment: Use temporary trash chunk, instead of global trash chunk in
> make_proxy_line_v2() to avoid memory corruption.
Are you sure there is not a bug here :
> if (srv->pp_opts & SRV_PP_V2_SSL_CN) {
> + cn_trash = get_trash_chunk();
> if (ssl_sock_get_remote_common_name(remote,
> &trash) > 0) {
> - tlv_len =
> make_tlv(&buf[ret+ssl_tlv_len], (buf_len - ret - ssl_tlv_len),
> PP2_TYPE_SSL_CN, trash.len, trash.str);
> + tlv_len =
> make_tlv(&buf[ret+ssl_tlv_len], (buf_len - ret - ssl_tlv_len),
> PP2_TYPE_SSL_CN, cn_trash->len, cn_trash->str);
I'm seeing ssl_sock_get_remote_common_name() write into trash
and make_tlv() retrieve from cn_trash which is still empty, so
I don't know how this could have worked in your tests, so maybe
you checked then cleaned up a bit too much before sending the
patch ?
Thanks,
Willy
