Re: [PATCH] In handle_channel_request_open(), variable type is freed too early and cause memory corruptions.

[Vic Lee <llyzs@xxxxxxx>]

Hi Aris,

It's hard to describe exactly how to reproduce the double-free but it
did happen to me from time to time. From the source code before the
patch, any "goto error" after "string_free(type);" will always
double-free, because there's another string_free(type) in the error
handler. But it's hard to say when "goto error" will happen. My suspect
is this:

    msg->channel_request_open.originator = string_to_char(type);

If type is freed, it could point to arbitrary data. The string_to_char
function could try to allocate an unexpected large amount of memory,
fail, then return NULL, and finally lead to double-free in the error
handler.

Vic

On Tue, 2010-01-19 at 18:12 +0100, Aris Adamantiadis wrote:
> Vic Lee a écrit :
> > Hi,
> > 
> > I recently encounter some occasional crashes when trying to use channel
> > request stuff. It turned out that in function
> > handle_channel_request_open(), a variable 'type' is freed but then used
> > in later codes, which causes unexpected result, sometimes double-freed
> > and crash, but if you are lucky sometimes things won't happen.
> > 
> > I will consider this bug a very critical one because this will make the
> > function handle_channel_request_open() very unstable, affecting both
> > server side (all channel requests) and client side (x11 and
> > forward-listen requests)
> > 
> > I found this in v0-4 branch, please help me to check master as well...
> > 
> > Thanks,
> > 
> > Vic
> > 
> Thanks for your patch.
> 
> I will check on the master branch. I remember having reworked that code
> (it's funny I did not see the problem), maybe I have corrected it.
> Moreover, a double free is a security problem. If it is easy to
> reproduce, we may have to do a security release.
> 
> Aris
>