tools: ssh-proxy: Check for domain status before parsing its CID

[PATCH] tools: ssh-proxy: Check for domain status before parsing its CID

Posted by Michal Privoznik 7 months, 2 weeks ago

Inactive domain XML can be wildly different to the live XML. For
instance, it can have VSOCK CID of that from another (running)
domain. Since domain status is not checked for, attempting to ssh
into an inactive domain may in fact result in opening a
connection to a different live domain that listens on said CID
currently.

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/737
Resolves: https://issues.redhat.com/browse/RHEL-75577

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/ssh-proxy/ssh-proxy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/ssh-proxy/ssh-proxy.c b/tools/ssh-proxy/ssh-proxy.c
index e60c58d57f..22daffeb63 100644
--- a/tools/ssh-proxy/ssh-proxy.c
+++ b/tools/ssh-proxy/ssh-proxy.c
@@ -194,7 +194,10 @@ lookupDomainAndFetchCID(const char *uri,
         if (virStrToLong_i(domname, NULL, 10, &id) >= 0)
             dom = virDomainLookupByID(conn, id);
     }
-    if (!dom)
+
+    /* If no domain is found, return an error. Similarly, inactive domain may
+     * contain CID of another (running) domain, yielding misleading results. */
+    if (!dom || virDomainIsActive(dom) <= 0)
         return -1;
 
     return extractCID(dom, cid);
-- 
2.45.2

Re: [PATCH] tools: ssh-proxy: Check for domain status before parsing its CID

Posted by Jiri Denemark 7 months, 2 weeks ago

On Tue, Jan 21, 2025 at 12:46:53 +0100, Michal Privoznik wrote:
> Inactive domain XML can be wildly different to the live XML. For
> instance, it can have VSOCK CID of that from another (running)
> domain. Since domain status is not checked for, attempting to ssh
> into an inactive domain may in fact result in opening a
> connection to a different live domain that listens on said CID
> currently.
> 
> Resolves: https://gitlab.com/libvirt/libvirt/-/issues/737
> Resolves: https://issues.redhat.com/browse/RHEL-75577
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  tools/ssh-proxy/ssh-proxy.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/ssh-proxy/ssh-proxy.c b/tools/ssh-proxy/ssh-proxy.c
> index e60c58d57f..22daffeb63 100644
> --- a/tools/ssh-proxy/ssh-proxy.c
> +++ b/tools/ssh-proxy/ssh-proxy.c
> @@ -194,7 +194,10 @@ lookupDomainAndFetchCID(const char *uri,
>          if (virStrToLong_i(domname, NULL, 10, &id) >= 0)
>              dom = virDomainLookupByID(conn, id);
>      }
> -    if (!dom)
> +
> +    /* If no domain is found, return an error. Similarly, inactive domain may
> +     * contain CID of another (running) domain, yielding misleading results. */
> +    if (!dom || virDomainIsActive(dom) <= 0)
>          return -1;
>  
>      return extractCID(dom, cid);

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>