[PATCH] block/curl: free s->password in cleanup paths

zhaoguohan_salmon@163.com posted 1 patch 2 days, 6 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20260320063016.262954-1-zhaoguohan._5Fsalmon@163.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
block/curl.c | 2 ++
1 file changed, 2 insertions(+)
[PATCH] block/curl: free s->password in cleanup paths
Posted by zhaoguohan_salmon@163.com 2 days, 6 hours ago
From: GuoHan Zhao <zhaoguohan@kylinos.cn>

When password-secret is used, curl_open() resolves it with
qcrypto_secret_lookup_as_utf8() and stores the returned buffer in
s->password.

Unlike s->proxypassword, s->password is not freed either in the open
failure path or in curl_close(), so the resolved secret leaks once it
has been allocated.

Free s->password in both cleanup paths.

Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
---
 block/curl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/curl.c b/block/curl.c
index 66aecfb20ec6..419df78258bc 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -903,6 +903,7 @@ out_noclean:
     g_free(s->cookie);
     g_free(s->url);
     g_free(s->username);
+    g_free(s->password);
     g_free(s->proxyusername);
     g_free(s->proxypassword);
     if (s->sockets) {
@@ -1014,6 +1015,7 @@ static void curl_close(BlockDriverState *bs)
     g_free(s->cookie);
     g_free(s->url);
     g_free(s->username);
+    g_free(s->password);
     g_free(s->proxyusername);
     g_free(s->proxypassword);
 }
-- 
2.43.0
Re: [PATCH] block/curl: free s->password in cleanup paths
Posted by Philippe Mathieu-Daudé 2 days, 3 hours ago
Hi,

On 20/3/26 07:30, zhaoguohan_salmon@163.com wrote:
> From: GuoHan Zhao <zhaoguohan@kylinos.cn>
> 
> When password-secret is used, curl_open() resolves it with
> qcrypto_secret_lookup_as_utf8() and stores the returned buffer in
> s->password.
> 
> Unlike s->proxypassword, s->password is not freed either in the open
> failure path or in curl_close(), so the resolved secret leaks once it
> has been allocated.
> 
> Free s->password in both cleanup paths.
> 
> Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
> ---
>   block/curl.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/block/curl.c b/block/curl.c
> index 66aecfb20ec6..419df78258bc 100644
> --- a/block/curl.c
> +++ b/block/curl.c
> @@ -903,6 +903,7 @@ out_noclean:
>       g_free(s->cookie);
>       g_free(s->url);
>       g_free(s->username);
> +    g_free(s->password);
>       g_free(s->proxyusername);
>       g_free(s->proxypassword);
>       if (s->sockets) {

Should we directly call curl_close() here instead? Otherwise
factor a common curl_cleanup() out and reuse?

> @@ -1014,6 +1015,7 @@ static void curl_close(BlockDriverState *bs)
>       g_free(s->cookie);
>       g_free(s->url);
>       g_free(s->username);
> +    g_free(s->password);
>       g_free(s->proxyusername);
>       g_free(s->proxypassword);
>   }