[vlc-devel] [PATCH] Stop realm variable in modules/access/http.c from being freed too early.

[Alexandre Janniaux]

Hi,

Thank you for the detailed explanation, it makes more sense.

As far a I understand, there is no need for intermediate
variable. You can directly strdup inside credential and
free before releasing credential.

Regards,
--
Alexandre Janniaux
Videolabs

On Fri, May 08, 2020 at 09:55:49AM +1000, Aaron Wyatt via vlc-devel wrote:
> Date: Fri, 8 May 2020 09:55:49 +1000
> From: Aaron Wyatt <github at psi-borg.org>
> To: vlc-devel at videolan.org
> Subject: Re: [vlc-devel] [PATCH] Stop realm variable in
>  modules/access/http.c from being freed too early.
> User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
>  Thunderbird/68.7.0
>
> Sorry, here's the more in depth explanation. So, p_sys is initialized at the
> start of Open. After Connect is called in Open (just after the "connect"
> label) if there's a 401 error, credential.psz_realm is set to point to
> psys->auth.psz_realm before credentials are retrieved from the keystore or
> user input. Immediately before this loops around to connect again with these
> new credentials, Disconnect is called, which calls vlc_http_auth_Deinit (in
> src/network/http_auth.c) on p_sys->auth.
>
> This frees p_sys->auth.psz_realm which is still pointed to by
> credential.psz_realm. So on looping back to the "connect" label to reattempt
> the connection now that we have credentials from the user,
> vlc_credential_store is called with credential.psz_realm pointing to freed
> memory. For keystore modules like keychain that don't use the realm as part
> of their storage mechanism, this isn't a problem. But for those that do,
> like kwallet, it means that the password is stored to a different location
> each time and never properly retrieved.
>
> Since I didn't want to mess with vlc_http_auth_Deinit (in case other code
> relied on it) storing a local copy of the string made the most sense.
> (Copying directly to credential.psz_realm would have required additional
> changes in src/misc/keystore.c to avoid the possibility of either a double
> free error or memory leak.) And thanks for the strdup tip. (I generally
> program in higher level languages like Objective C and various C++
> frameworks, so the tip is appreciated.)
> Cheers,
> Aaron
>
>
> So I guess with the strdup change the patch would be:
>
>
> ---
>  modules/access/http.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/modules/access/http.c b/modules/access/http.c
> index 4384e2b0a3..b4645c0f4e 100644
> --- a/modules/access/http.c
> +++ b/modules/access/http.c
> @@ -142,6 +142,7 @@ static int Open( vlc_object_t *p_this )
>      char *psz;
>      int ret = VLC_EGENERIC;
>      vlc_credential credential;
> +    char *psz_realm;
>
>      access_sys_t *p_sys = vlc_obj_malloc( p_this, sizeof(*p_sys) );
>      if( unlikely(p_sys == NULL) )
> @@ -166,6 +167,7 @@ static int Open( vlc_object_t *p_this )
>      p_sys->offset = 0;
>      p_sys->size = 0;
>      p_access->p_sys = p_sys;
> +    psz_realm = NULL;
>
>      if( vlc_UrlParse( &p_sys->url, psz_url ) || p_sys->url.psz_host == NULL
> )
>      {
> @@ -298,7 +300,9 @@ connect:
>          msg_Dbg( p_access, "authentication failed for realm %s",
>                   p_sys->auth.psz_realm );
>
> -        credential.psz_realm = p_sys->auth.psz_realm;
> +        free( psz_realm );
> +        psz_realm = strdup( p_sys->auth.psz_realm );
> +        credential.psz_realm = psz_realm;
>          credential.psz_authtype = p_sys->auth.psz_nonce  ? "Digest" :
> "Basic";
>
>           if( vlc_credential_get( &credential, p_access, NULL, NULL,
> @@ -339,6 +343,8 @@ connect:
>      p_access->pf_control = Control;
>      p_access->pf_seek = Seek;
>
> +    free( psz_realm );
> +    psz_realm = NULL;
>      vlc_credential_clean( &credential );
>
>      return VLC_SUCCESS;
> @@ -347,6 +353,8 @@ disconnect:
>      Disconnect( p_access );
>
>  error:
> +    free( psz_realm );
> +    psz_realm = NULL;
>      vlc_credential_clean( &credential );
>      vlc_UrlClean( &p_sys->url );
>      if( p_sys->b_proxy )
> --
> 2.24.2 (Apple Git-127)
>
>
> On 8/5/20 1:42 am, Alexandre Janniaux wrote:
> > Hi,
> >
> > Thanks for the patch, but I'm not sure of what issue it is
> > fixing. Do you have asan reports of use after free?
> >
> > It seems to be init in Connect and is not freed before the
> > end of this code as far as I can read, so if you have random
> > data it probably comes from a different part of the code.
> >
> > Also you can use strdup instead of malloc(strlen) + strcpy
> >
> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
>

> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel