After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 772989 - Totem allows invalid urls that might cause segfault that's irrecoverable
Totem allows invalid urls that might cause segfault that's irrecoverable
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: network
unspecified
Other Linux
: High critical
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2016-10-15 17:15 UTC by Mohammed Sadiq
Modified: 2018-02-01 15:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GSimpleProxyResolver: Add test case for broken hostname (1.02 KB, patch)
2016-10-27 16:00 UTC, Bastien Nocera
none Details | Review
httpproxy: Fix invalid request on invalid hostnames (2.31 KB, patch)
2016-12-07 16:45 UTC, Bastien Nocera
committed Details | Review
simpleproxyresolver: Don't crash on invalid hostname (1.25 KB, patch)
2016-12-07 16:45 UTC, Bastien Nocera
committed Details | Review
resolver: Return error looking up services on invalid hostnames (1.76 KB, patch)
2016-12-07 16:45 UTC, Bastien Nocera
committed Details | Review
resolver: Return error looking up invalid hostnames (1.81 KB, patch)
2016-12-07 16:45 UTC, Bastien Nocera
committed Details | Review
GSimpleProxyResolver: Add test case for broken hostname (1.71 KB, patch)
2017-01-12 14:50 UTC, Bastien Nocera
committed Details | Review
networkaddress: Return an error from _g_uri_parse_authority() (5.83 KB, patch)
2017-01-12 14:50 UTC, Bastien Nocera
committed Details | Review
networkaddress: Add early sanity check to _g_uri_parse_authority() (2.71 KB, patch)
2017-01-12 14:51 UTC, Bastien Nocera
committed Details | Review
resolver: Return early if URI is invalid (1.79 KB, patch)
2017-01-12 14:51 UTC, Bastien Nocera
committed Details | Review

Description Mohammed Sadiq 2016-10-15 17:15:28 UTC
Totem allows adding invalid urls. This may make Totem always segfault.

How to reproduce:

1. Press the Add (+) button -> Add web video.
2. Give some invalid url (eg: 'നല്ല കാര്യം' without quotes)
3. The added video has the label as '/'. Try to open the video. totem segfaults.
4. Now (If you are lucky), each time totem is run, it just segfaults. 
5. Else, remove the newly added link, go to 1.

How to recover: delete the file '~/.config/totem/session_state.xspf'

Totem should not allow invalid links for web. Or at least, it should recover from segfault next time the totem is run. This can be irritating, when a user accidentally pastes some invalid url. (I haven't checked with valid urls that contains unicode).

Thanks
Comment 1 Bastien Nocera 2016-10-15 17:51:53 UTC
Thanks for taking the time to report this.
Without a stack trace from the crash it's very hard to determine what caused it.
Can you get us a stack trace? Please see https://wiki.gnome.org/Community/GettingInTouch/Bugzilla/GettingTraces for more information on how to do so. When pasting a stack trace in this bug report, please reset the status of this bug report from NEEDINFO to its previous status. Thanks in advance!
Comment 2 Mohammed Sadiq 2016-10-16 00:21:24 UTC
If the strace below isn't helping, please let me know. I don't have enough space to install the debug symbols. I shall try to create a better one if you need. Thanks

Thread 9 (Thread 0x7fffb9d52700 (LWP 6891))

  • #0 syscall
  • #1 g_cond_wait
  • #2 g_async_queue_pop_intern_unlocked
  • #3 g_async_queue_pop
  • #4 gom_adapter_worker
  • #5 g_thread_proxy
  • #6 start_thread
  • #7 clone

Thread 8 (Thread 0x7fffba553700 (LWP 6890))

  • #0 syscall
  • #1 g_cond_wait_until
  • #2 g_async_queue_pop_intern_unlocked
  • #3 g_thread_pool_thread_proxy
  • #4 g_thread_proxy
  • #5 start_thread
  • #6 clone

Thread 7 (Thread 0x7fffbbdb2700 (LWP 6889))

  • #0 syscall
  • #1 g_cond_wait
  • #2 g_async_queue_pop_intern_unlocked
  • #3 g_async_queue_pop
  • #4 gom_adapter_worker
  • #5 g_thread_proxy
  • #6 start_thread
  • #7 clone

Thread 5 (Thread 0x7fffcf100700 (LWP 6882))

  • #0 syscall
  • #1 g_cond_wait
  • #2 g_async_queue_pop_intern_unlocked
  • #3 g_thread_pool_thread_proxy
  • #4 g_thread_proxy
  • #5 start_thread
  • #6 clone

Comment 3 Mohammed Sadiq 2016-10-16 00:37:40 UTC
I can't reset the 'NEEDINFO' status. You will have to do it yourself.
Comment 4 Bastien Nocera 2016-10-16 00:38:51 UTC
Totem passes it to GStreamer, which passes it to libsoup, which passes it to gio and crashes.

Reassigning to libsoup because I don't think it should be passing it down at that level.
Comment 5 Dan Winship 2016-10-27 12:26:47 UTC
(In reply to Bastien Nocera from comment #4)
> Totem passes it to GStreamer, which passes it to libsoup, which passes it to
> gio and crashes.
> 
> Reassigning to libsoup because I don't think it should be passing it down at
> that level.

What is the chain of function calls there? I assume the libsoup part involves SoupRequestFile? But SoupRequestFile would only be involved if GStreamer or Totem prepended "file://" to the string before passing it to libsoup, and in that case, passing it to gio and expecting gio to either accept it or return a GError seems correct...
Comment 6 Bastien Nocera 2016-10-27 14:17:39 UTC
Backtrace with debug.
  • #0 strlen
    at ../sysdeps/x86_64/strlen.S line 106
  • #1 ignore_host
    at gsimpleproxyresolver.c line 292
  • #2 g_simple_proxy_resolver_lookup
    at gsimpleproxyresolver.c line 331
  • #3 g_proxy_resolver_gnome_lookup_internal
  • #4 g_proxy_resolver_gnome_lookup
  • #5 g_proxy_address_enumerator_next
    at gproxyaddressenumerator.c line 185
  • #6 g_socket_client_connect
    at gsocketclient.c line 1011
  • #7 soup_socket_connect_sync_internal
    at soup-socket.c line 1059
  • #8 soup_connection_connect_sync
    at soup-connection.c line 455
  • #9 get_connection
    at soup-session.c line 1957
  • #10 soup_session_process_queue_item
    at soup-session.c line 1978
  • #11 soup_session_send
    at soup-session.c line 4420
  • #12 gst_soup_http_src_do_request
  • #13 gst_soup_http_src_create
  • #14 gst_base_src_get_range
  • #15 gst_base_src_loop
  • #16 gst_task_func
  • #17 g_thread_pool_thread_proxy
    at gthreadpool.c line 307
  • #18 g_thread_proxy
    at gthread.c line 784
  • #19 start_thread
    at pthread_create.c line 333
  • #20 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 105

Looks like a bug in gsimpleproxyresolver.c rather, sorry about that. (ignore host is being passed a NULL host, which it should probably guard against)
Comment 7 Bastien Nocera 2016-10-27 16:00:44 UTC
Created attachment 338624 [details] [review]
GSimpleProxyResolver: Add test case for broken hostname
Comment 8 Dan Winship 2016-11-03 16:22:43 UTC
(The problem is code not checking for a NULL return from g_hostname_to_ascii(), and it needs to be fixed in a few different places.)
Comment 9 Bastien Nocera 2016-12-07 16:45:21 UTC
Created attachment 341566 [details] [review]
httpproxy: Fix invalid request on invalid hostnames

When an invalid hostname is passed for connection, the
g_hostname_to_ascii() might fail when creating the request in
create_request(). Make sure that error is caught and reported rather
than passing "(null)" as the hostname of the site we want to connect to.
Comment 10 Bastien Nocera 2016-12-07 16:45:28 UTC
Created attachment 341567 [details] [review]
simpleproxyresolver: Don't crash on invalid hostname

Check for g_hostname_to_ascii() failure, rather than crashing when
checking whether an invalid hostname should go through the proxy.

The HTTP library should report the error about the invalid hostname
once we actually try to connect to it.
Comment 11 Bastien Nocera 2016-12-07 16:45:34 UTC
Created attachment 341568 [details] [review]
resolver: Return error looking up services on invalid hostnames

Make g_resolver_lookup_service() and
g_resolver_lookup_service_async() error out when invalid hostnames are
passed.
Comment 12 Bastien Nocera 2016-12-07 16:45:39 UTC
Created attachment 341569 [details] [review]
resolver: Return error looking up invalid hostnames

Make g_resolver_lookup_by_name() and g_resolver_lookup_by_name_async()
error out when invalid hostnames are passed.
Comment 13 Dan Winship 2016-12-07 18:18:39 UTC
Comment on attachment 341567 [details] [review]
simpleproxyresolver: Don't crash on invalid hostname

ok... perhaps it would be better to have _g_uri_parse_authority() return a GError in this case, and perhaps even to check from g_proxy_resolver_lookup()/lookup_async(), not at the GSimpleProxyResolver level.
Comment 14 Dan Winship 2016-12-07 18:19:21 UTC
Comment on attachment 341568 [details] [review]
resolver: Return error looking up services on invalid hostnames

>@@ -744,6 +753,7 @@ g_resolver_lookup_service_async (GResolver           *resolver,
>   g_return_if_fail (domain != NULL);
> 
>   rrname = g_resolver_get_service_rrname (service, protocol, domain);
>+  g_return_if_fail (rrname != NULL);

Should return a proper error here, like in the other cases
Comment 15 Dan Winship 2016-12-07 18:22:21 UTC
Comment on attachment 341569 [details] [review]
resolver: Return error looking up invalid hostnames

ok, but remove the comment in g_resolver_lookup_by_name_finish() in the tagged case since it's no longer accurate
Comment 16 Bastien Nocera 2017-01-03 13:36:55 UTC
Attachment 341566 [details] pushed as 5c566e4 - httpproxy: Fix invalid request on invalid hostnames
Attachment 341569 [details] pushed as 442b7ce - resolver: Return error looking up invalid hostnames
Comment 17 Bastien Nocera 2017-01-03 17:10:13 UTC
(In reply to Dan Winship from comment #13)
> Comment on attachment 341567 [details] [review] [review]
> simpleproxyresolver: Don't crash on invalid hostname
> 
> ok... perhaps it would be better to have _g_uri_parse_authority() return a
> GError in this case, and perhaps even to check from
> g_proxy_resolver_lookup()/lookup_async(), not at the GSimpleProxyResolver
> level.

I don't understand how. _g_uri_parse_authority() doesn't return on error, it just returns a non-ascii hostname:
/static-proxy/uri: GLib-GIO-Message: _g_uri_parse_authority: നല
Comment 18 Dan Winship 2017-01-03 18:17:08 UTC
hm? _g_uri_parse_authority() looks like it should return FALSE if @uri doesn't contain "//"...
Comment 19 Bastien Nocera 2017-01-03 18:57:51 UTC
(In reply to Dan Winship from comment #18)
> hm? _g_uri_parse_authority() looks like it should return FALSE if @uri
> doesn't contain "//"...

Yes, and it doesn't run ignore_host(). So how would it run into this code?
Comment 20 Dan Winship 2017-01-05 14:25:21 UTC
The code currently in git crashes if you pass it a garbage URI. Your patch changes it so that if you pass in a garbage URI, it will return priv->default_proxy. I'm saying you should change it so that if you pass in a garbage URI, it will return an error saying that you passed in a garbage URI.

The best way to do that, I think, would be to make g_proxy_resolver_lookup()/g_proxy_resolver_lookup_async() call _g_uri_parse_authority() on the passed-in URI to sanity-check it, before ever calling into gsimpleproxyresolver.c, more-or-less like what you did with g_resolver_lookup_by_name(). (And either _g_uri_parse_authority() or g_proxy_resolver_lookup/lookup_async() would have to call g_hostname_to_ascii() as well.)
Comment 21 Bastien Nocera 2017-01-06 12:48:23 UTC
(In reply to Dan Winship from comment #20)
> The code currently in git crashes if you pass it a garbage URI. Your patch
> changes it so that if you pass in a garbage URI, it will return
> priv->default_proxy. I'm saying you should change it so that if you pass in
> a garbage URI, it will return an error saying that you passed in a garbage
> URI.

Is the test URI above garbage? I didn't know that.

> The best way to do that, I think, would be to make
> g_proxy_resolver_lookup()/g_proxy_resolver_lookup_async() call
> _g_uri_parse_authority() on the passed-in URI to sanity-check it, before
> ever calling into gsimpleproxyresolver.c, more-or-less like what you did
> with g_resolver_lookup_by_name(). (And either _g_uri_parse_authority() or
> g_proxy_resolver_lookup/lookup_async() would have to call
> g_hostname_to_ascii() as well.)

So I guess that a hostname needs to be ASCII to be valid? So I need to run  g_hostname_to_ascii() in the same place I run _g_uri_parse_authority().
Comment 22 Dan Winship 2017-01-08 16:43:12 UTC
(In reply to Bastien Nocera from comment #21)
> Is the test URI above garbage? I didn't know that.

Well, "നല" isn't a URI at all, obviously, but based on where the crash was happening, it looks like something along the chain must be turning it into "http://നല" or something like that. But anyway, for proxy-resolving purposes, if the hostname part isn't a valid hostname, then the URI is invalid.

> So I guess that a hostname needs to be ASCII to be valid? So I need to run 
> g_hostname_to_ascii() in the same place I run _g_uri_parse_authority().

Yeah. Well, not "needs to be ASCII" exactly, but "needs to be acceptable to g_hostname_to_ascii()". If we had "g_hostname_is_legal()" you could call that, but we don't, so the only way to check if it's legal is to see if g_hostname_to_ascii() accepts it.
Comment 23 Bastien Nocera 2017-01-12 14:20:33 UTC
(In reply to Dan Winship from comment #22)
> (In reply to Bastien Nocera from comment #21)
> > Is the test URI above garbage? I didn't know that.
> 
> Well, "നല" isn't a URI at all, obviously, but based on where the crash was
> happening, it looks like something along the chain must be turning it into
> "http://നല" or something like that. But anyway, for proxy-resolving
> purposes, if the hostname part isn't a valid hostname, then the URI is
> invalid.

The test is "http://നല", that's how it's transformed by totem's entry, as you can see in the backtrace:
g_simple_proxy_resolver_lookup (proxy_resolver=<optimized out>, uri=0x5555571171c0 "http://%E0%B4%A8%E0%B4%B2%E0%B5%8D%E0%B4%B2%20%E0%B4%95%E0%B4%BE%E0%B4%B0%E0%B5%8D%E0%B4%AF%E0%B4%82:80/", cancellable=<optimized out>, error=<optimized out>) at gsimpleproxyresolver.c:331

> > So I guess that a hostname needs to be ASCII to be valid? So I need to run 
> > g_hostname_to_ascii() in the same place I run _g_uri_parse_authority().
> 
> Yeah. Well, not "needs to be ASCII" exactly, but "needs to be acceptable to
> g_hostname_to_ascii()". If we had "g_hostname_is_legal()" you could call
> that, but we don't, so the only way to check if it's legal is to see if
> g_hostname_to_ascii() accepts it.

Fair enough, I'll make that change.
Comment 24 Bastien Nocera 2017-01-12 14:50:53 UTC
Created attachment 343369 [details] [review]
GSimpleProxyResolver: Add test case for broken hostname
Comment 25 Bastien Nocera 2017-01-12 14:50:59 UTC
Created attachment 343370 [details] [review]
networkaddress: Return an error from _g_uri_parse_authority()

So that errors can be propagated if necessary.
Comment 26 Bastien Nocera 2017-01-12 14:51:06 UTC
Created attachment 343371 [details] [review]
networkaddress: Add early sanity check to _g_uri_parse_authority()

Check whether the URI is valid ASCII before trying to parse it. This
should catch broken URIs early.
Comment 27 Bastien Nocera 2017-01-12 14:51:13 UTC
Created attachment 343372 [details] [review]
resolver: Return early if URI is invalid
Comment 28 Philip Withnall 2017-09-11 19:05:00 UTC
Review of attachment 343369 [details] [review]:

::: gio/tests/simple-proxy.c
@@ +47,3 @@
+
+  resolver = g_simple_proxy_resolver_new (NULL, ignore_hosts);
+  uri = "http://%E0%B4%A8%E0%B4%B2:80/";

This looks identical to the first test case. Why the duplicate?

@@ +48,3 @@
+  resolver = g_simple_proxy_resolver_new (NULL, ignore_hosts);
+  uri = "http://%E0%B4%A8%E0%B4%B2:80/";
+  proxies = g_proxy_resolver_lookup (resolver, uri, NULL, &error);

`proxies` is leaked.
Comment 29 Philip Withnall 2017-09-11 19:08:28 UTC
Review of attachment 343370 [details] [review]:

::: gio/gnetworkaddress.c
@@ +521,3 @@
+    {
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
+		   "Invalid URI '%s'",

s/'%s'/“%s”/ (GLib uses Unicode quotes now).

@@ +537,3 @@
+    {
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
+		   "Invalid URI '%s'",

Same here.

@@ +573,3 @@
+	        {
+		  g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
+		               "Invalid URI '%s'",

And here.

@@ +590,3 @@
+	    {
+	      g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
+		           "Invalid URI '%s'",

And here. In fact, it might be best to factor all of these out into a single common error path (`goto invalid_uri` or somesuch).
Comment 30 Philip Withnall 2017-09-11 19:09:55 UTC
Review of attachment 343371 [details] [review]:

::: gio/gnetworkaddress.c
@@ +512,3 @@
+    {
+      g_set_error (error, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
+		   "Invalid URI '%s'",

Same comment as above about the Unicode quotes and the common error path.
Comment 31 Philip Withnall 2017-09-11 19:10:34 UTC
Review of attachment 343371 [details] [review]:

::: gio/gnetworkaddress.c
@@ +508,3 @@
     *userinfo = NULL;
 
+  ascii_uri = g_hostname_to_ascii (uri);

It would also be useful to add a comment above this block explaining why we’re converting to ASCII.
Comment 32 Philip Withnall 2017-09-11 19:12:06 UTC
Review of attachment 343372 [details] [review]:

::: gio/gproxyresolver.c
@@ +183,3 @@
+
+      task = g_task_new (resolver, cancellable, callback, user_data);
+      g_task_set_source_tag (task, g_proxy_resolver_lookup_async);

This can be simplified with g_task_report_error().
Comment 33 Philip Withnall 2017-11-13 11:35:26 UTC
Any progress on these patches?
Comment 34 Bastien Nocera 2017-11-14 09:23:33 UTC
(In reply to Philip Withnall from comment #33)
> Any progress on these patches?

No, and none planned for a while either. If somebody beats me to copy/pasting unicode quoting marks...
Comment 35 Philip Withnall 2018-02-01 15:00:13 UTC
Updated the patches to fix the review issues (mostly, Unicode quotation marks were not what needed changing) and pushed to master, thanks.

Attachment 341567 [details] pushed as cb8c919 - simpleproxyresolver: Don't crash on invalid hostname
Attachment 341568 [details] pushed as 132cf9a - resolver: Return error looking up services on invalid hostnames
Attachment 343369 [details] pushed as 8836662 - GSimpleProxyResolver: Add test case for broken hostname
Attachment 343370 [details] pushed as 5f2c20e - networkaddress: Return an error from _g_uri_parse_authority()
Attachment 343371 [details] pushed as 99b792f - networkaddress: Add early sanity check to _g_uri_parse_authority()
Attachment 343372 [details] pushed as e889fb2 - resolver: Return early if URI is invalid