[libvirt PATCH] remote: Avoid crash in remoteSplitURIScheme()
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 :|
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Peter Krempa