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

[Aaron Wyatt]

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
>