GNOME Bugzilla – Bug 796349
Do not listen all if invalid interface is provided
Last modified: 2018-09-12 07:05:42 UTC
It is not a good idea from security point of view to listen all interfaces in case of invalid interface is provided. We should rather listen to nothing and print error in journal.
Created attachment 372354 [details] [review] Do not listen all if invalid interface is provided
Review of attachment 372354 [details] [review]: Seems fine.
Originally, this has been reported downstream: https://bugzilla.redhat.com/show_bug.cgi?id=1580577
Thanks! Attachment 372354 [details] pushed as bfa1432 - Do not listen all if invalid interface is provided
There is obviously an issue with that patch as per the downstream report. I am going to propose additional fixes here...
Created attachment 373453 [details] [review] Prevent monitoring all interfaces after change of other props Commit bfa1432 prevents monitoring all interfaces if invalid interface is provided, but it works only in some cases, because the invalid interface is not remebered and for example consequent change of port will cause that all interfaces are monitored again. Remember the invalid interface to prevent monitoring all interfaces even after change of other properties...
Created attachment 373454 [details] [review] Properly remove watches when changing server props vino_server_init_io_channels calls vino_server_deinit_io_channels at the beginning, however the watches and channels don't have to be removed respective closed, because it relies on rfbListenSock array, which can be already modified as a consequence of changing server properties. Let's call vino_server_deinit_io_channels before changing server properties in order to prevent the following errors: rfbCheckFds: accept: Invalid argument
Created attachment 373455 [details] [review] Return empty string instead of NULL to prevent criticals The code expects that avahi_client_get_host_name_fqdn never return NULL, but it can happen in some cases. Return empty string instead of NULL to prevent the following criticals: GLib-CRITICAL **: 14:29:52.305: g_variant_new_string: assertion 'string != NULL' failed
David, can you please take a look at again?
I have created the following MR for those patches for easier reviewing: https://gitlab.gnome.org/GNOME/vino/merge_requests/5
Review of attachment 373453 [details] [review]: Small typo "remebered" in commit message, but otherwise fine.
Review of attachment 373454 [details] [review]: Looks fine.
Review of attachment 373455 [details] [review]: Looks good.
Created attachment 373614 [details] [review] Prevent monitoring all interfaces after change of other props Fixed typo.
Thanks for the review! Attachment 373454 [details] pushed as 56ac246 - Properly remove watches when changing server props Attachment 373455 [details] pushed as 15eb3c5 - Return empty string instead of NULL to prevent criticals Attachment 373614 [details] pushed as e49456a - Prevent monitoring all interfaces after change of other props