Re: [Openvpn-devel] [PATCH v2] Fix memory leaks in HMAC initial packet id

[Gert D. <ge...@gr...>]

Hi,

On Wed, Mar 15, 2023 at 04:04:20PM +0100, Arne Schwabe wrote:
> The HMAC leaks are just forgotten frees/deinitialisations. tls_wrap_control
> will sometimes return the original buffer (non tls-crypt) and sometimes
> tls_wrap.work, handling this buffer lifetime is a bit more complicated. Instead
> of further complicating that code just give our work buffer the same lifetime
> as the other one inside tls_wrap.work as that is also more consistent.

This test passes all I've thrown at it, except stare-at-code...

Maybe I am misreading it, but want to make sure.

> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index 124ac76bd..fa2681dc7 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -3483,6 +3483,7 @@ do_init_frame_tls(struct context *c)
>          frame_print(&c->c2.tls_auth_standalone->frame, D_MTU_INFO,
>                      "TLS-Auth MTU parms");
>          c->c2.tls_auth_standalone->tls_wrap.work = alloc_buf_gc(BUF_SIZE(&c->c2.frame), &c->c2.gc);
> +        c->c2.tls_auth_standalone->workbuf = alloc_buf_gc(BUF_SIZE(&c->c2.frame), &c->c2.gc);
>      }

At this place, we have a tls_auth_standalone() (!= NULL) already, which
got initialized by tls_auth_standalone_init().

We allocate tas->workbuf here.

> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 78cec90a1..f2331f712 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1358,9 +1358,21 @@ tls_auth_standalone_init(struct tls_options *tls_options,
>      packet_id_init(&tas->tls_wrap.opt.packet_id, tls_options->replay_window,
>                     tls_options->replay_time, "TAS", 0);
>  
> +    tas->workbuf = alloc_buf_gc(tas->frame.buf.payload_size, gc);
>      return tas;
>  }

This is tls_auth_standalone_init(), which has already allocated
a tas->workbuf.

So my reading is "we allocate this twice" - which is not a super big
problem because it's both in the gc_arena and will be freed on session
exit (mmmh, which session, anyway?) - but it's still "too many bufs",
plus "readers of that code will get confused".

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             ge...@gr...