GNOME Bugzilla – Bug 772989
Totem allows invalid urls that might cause segfault that's irrecoverable
Last modified: 2018-02-01 15:00:53 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
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!
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
+ Trace 236743
Thread 9 (Thread 0x7fffb9d52700 (LWP 6891))
Thread 8 (Thread 0x7fffba553700 (LWP 6890))
Thread 7 (Thread 0x7fffbbdb2700 (LWP 6889))
Thread 5 (Thread 0x7fffcf100700 (LWP 6882))
I can't reset the 'NEEDINFO' status. You will have to do it yourself.
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.
(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...
Backtrace with debug.
+ Trace 236778
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)
Created attachment 338624 [details] [review] GSimpleProxyResolver: Add test case for broken hostname
(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.)
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.
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.
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.
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 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 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 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
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
(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: നല
hm? _g_uri_parse_authority() looks like it should return FALSE if @uri doesn't contain "//"...
(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?
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.)
(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().
(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.
(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.
Created attachment 343369 [details] [review] GSimpleProxyResolver: Add test case for broken hostname
Created attachment 343370 [details] [review] networkaddress: Return an error from _g_uri_parse_authority() So that errors can be propagated if necessary.
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.
Created attachment 343372 [details] [review] resolver: Return early if URI is invalid
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.
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).
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.
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.
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().
Any progress on these patches?
(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...
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