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

[Aaron Wyatt]

Hi Alexandre,
The problem I found with that is that the credential.psz_realm pointer 
can be changed by the credential_find_keystore function when a saved 
password is found. This means that the credential.psz_realm pointer ends 
up sharing an address with one of the credential's p_entry->ppsz_values. 
(Resulting in a double free error when that gets freed.) I thought about 
checking the credential.p_from_keystore value to check if that has 
happened and then free the value only if it hasn't, but that will result 
in a memory leak when it has happened unless either the 
credential_find_keystore function frees the member before changing it or 
we retain a reference to it in http.c (so we need an intermediate 
anyway). If we free it in credential_find_keystore then that could cause 
an issue with every other module that uses the keystore under the 
assumption that they retain ownership of the memory allocated to the 
realm variable.
Cheers,
Aaron

On 8/5/20 19:57, Alexandre Janniaux wrote:
> 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)