We need to make sure the URI scheme is present before passing
it to strchr(), otherwise we're going to get
$ virt-ssh-helper foo
Segmentation fault (core dumped)
Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
src/remote/remote_sockets.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
index 2979576680..c315b24d30 100644
--- a/src/remote/remote_sockets.c
+++ b/src/remote/remote_sockets.c
@@ -69,7 +69,15 @@ remoteSplitURIScheme(virURI *uri,
char **driver,
remoteDriverTransport *transport)
{
- char *p = strchr(uri->scheme, '+');
+ char *p = NULL;
+
+ if (!uri->scheme) {
+ virReportError(VIR_ERR_INVALID_ARG, "%s",
+ _("missing scheme for URI"));
+ return -1;
+ }
+
+ p = strchr(uri->scheme, '+');
if (p)
*driver = g_strndup(uri->scheme, p - uri->scheme);
--
2.31.1
On Fri, Dec 10, 2021 at 10:59:27 +0100, Andrea Bolognani wrote:
> We need to make sure the URI scheme is present before passing
> it to strchr(), otherwise we're going to get
>
> $ virt-ssh-helper foo
> Segmentation fault (core dumped)
>
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
> src/remote/remote_sockets.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c
> index 2979576680..c315b24d30 100644
> --- a/src/remote/remote_sockets.c
> +++ b/src/remote/remote_sockets.c
> @@ -69,7 +69,15 @@ remoteSplitURIScheme(virURI *uri,
> char **driver,
> remoteDriverTransport *transport)
> {
> - char *p = strchr(uri->scheme, '+');
> + char *p = NULL;
> +
> + if (!uri->scheme) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("missing scheme for URI"));
The other place which leads to the call of this helper (virConnectOpenInternal)
uses the following error to reject the uri if scheme is missing:
virReportError(VIR_ERR_NO_CONNECT,
_("URI '%s' does not include a driver name"),
name);
It looks like it's unlikely that anybody would use virt-ssh-helper
manually though.
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On Fri, Dec 10, 2021 at 11:31:19AM +0100, Peter Krempa wrote:
> On Fri, Dec 10, 2021 at 10:59:27 +0100, Andrea Bolognani wrote:
> > @@ -69,7 +69,15 @@ remoteSplitURIScheme(virURI *uri,
> > char **driver,
> > remoteDriverTransport *transport)
> > {
> > - char *p = strchr(uri->scheme, '+');
> > + char *p = NULL;
> > +
> > + if (!uri->scheme) {
> > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > + _("missing scheme for URI"));
>
> The other place which leads to the call of this helper (virConnectOpenInternal)
> uses the following error to reject the uri if scheme is missing:
>
> virReportError(VIR_ERR_NO_CONNECT,
> _("URI '%s' does not include a driver name"),
> name);
Yeah, it seems safer to catch the issue inside the helper than
requiring the callers to perform the check ahead of time. It's okay
for virConnectOpen() to have a nicer error message, as it's the one
that people are more likely to see.
I entertained the thought of adding the check to virURIParse()
directly, because I can't think of a scenario where having a NULL
scheme would be considered valid. But that seemed like a change that
had the potential to break unrelated stuff, so I cowardly decided to
go with the safe version instead O:-)
> It looks like it's unlikely that anybody would use virt-ssh-helper
> manually though.
Absolutely! But it's still in the default $PATH, and not crashing
when passed invalid arguments is what I would consider the bare
minimum :)
Thanks for the review!
--
Andrea Bolognani / Red Hat / Virtualization
On Fri, Dec 10, 2021 at 05:47:41AM -0800, Andrea Bolognani wrote:
> On Fri, Dec 10, 2021 at 11:31:19AM +0100, Peter Krempa wrote:
> > On Fri, Dec 10, 2021 at 10:59:27 +0100, Andrea Bolognani wrote:
> > > @@ -69,7 +69,15 @@ remoteSplitURIScheme(virURI *uri,
> > > char **driver,
> > > remoteDriverTransport *transport)
> > > {
> > > - char *p = strchr(uri->scheme, '+');
> > > + char *p = NULL;
> > > +
> > > + if (!uri->scheme) {
> > > + virReportError(VIR_ERR_INVALID_ARG, "%s",
> > > + _("missing scheme for URI"));
> >
> > The other place which leads to the call of this helper (virConnectOpenInternal)
> > uses the following error to reject the uri if scheme is missing:
> >
> > virReportError(VIR_ERR_NO_CONNECT,
> > _("URI '%s' does not include a driver name"),
> > name);
>
> Yeah, it seems safer to catch the issue inside the helper than
> requiring the callers to perform the check ahead of time. It's okay
> for virConnectOpen() to have a nicer error message, as it's the one
> that people are more likely to see.
Yes, this is something I simply overlooked when refactoring
the code. The check should clearly be in this common helper.
> I entertained the thought of adding the check to virURIParse()
> directly, because I can't think of a scenario where having a NULL
> scheme would be considered valid. But that seemed like a change that
> had the potential to break unrelated stuff, so I cowardly decided to
> go with the safe version instead O:-)
We've supported URIs without a scheme in the past. IIRC, we allowed
a bath path to a UNIX socket for the original Xen driver. That
code is deleted now of course.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Dec 10, 2021 at 02:06:18PM +0000, Daniel P. Berrangé wrote: > On Fri, Dec 10, 2021 at 05:47:41AM -0800, Andrea Bolognani wrote: > > I entertained the thought of adding the check to virURIParse() > > directly, because I can't think of a scenario where having a NULL > > scheme would be considered valid. But that seemed like a change that > > had the potential to break unrelated stuff, so I cowardly decided to > > go with the safe version instead O:-) > > We've supported URIs without a scheme in the past. IIRC, we allowed > a bath path to a UNIX socket for the original Xen driver. That > code is deleted now of course. So do you think it would be possible to perform more strict validation in virURIParse() and reject this kind of wonky input outright at this point? -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Dec 10, 2021 at 06:22:04AM -0800, Andrea Bolognani wrote: > On Fri, Dec 10, 2021 at 02:06:18PM +0000, Daniel P. Berrangé wrote: > > On Fri, Dec 10, 2021 at 05:47:41AM -0800, Andrea Bolognani wrote: > > > I entertained the thought of adding the check to virURIParse() > > > directly, because I can't think of a scenario where having a NULL > > > scheme would be considered valid. But that seemed like a change that > > > had the potential to break unrelated stuff, so I cowardly decided to > > > go with the safe version instead O:-) > > > > We've supported URIs without a scheme in the past. IIRC, we allowed > > a bath path to a UNIX socket for the original Xen driver. That > > code is deleted now of course. > > So do you think it would be possible to perform more strict > validation in virURIParse() and reject this kind of wonky input > outright at this point? Probably, though calling it wonky input is a bit misleading. It is valid from a URI pov, it is just that libvirt doesn't need this ability to omit the scheme. I'd want to double check all the callers of virURIParse to be sure first though Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.