GNOME Bugzilla – Bug 708306
Don't silently accept self-signed certificate
Last modified: 2018-09-21 17:28:48 UTC
This patch series updates the http backend to use the SoupSession class that was introduced in recent libsoup releases. My motivation for that is that currently gvfs will silently accept self-signed certificate in https connections due to the defaults used in SoupSessionSync/SoupSessionAsync. The defaults are more strict in SoupSession, see https://developer.gnome.org/libsoup/stable/libsoup-session-porting.html The end result is that before this patch, gvfs-copy https://linuxfr.org/images/sections/83.png . works, and after it fails with 'SSL handshake failed' (the certificate is a CACert one, which is not in the default CA trust store). I've only lightly tested the end result (only a few gvfs-copy calls)...
Created attachment 255223 [details] [review] http: Remove use of SoupSessionSync/SoupSessionAsync Since libsoup 2.42.0, SoupSessionSync/SoupSessionAsync are deprecated, and replaced by direct use of SoupSession: "As of libsoup 2.42, SoupSession is no longer an abstract class, and the base SoupSession class is now preferred over its traditional subclasses, SoupSessionAsync and SoupSessionSync. There are several changes in behavior between the old and new sessions to be aware of. The new SoupSession has different (and hopefully better) defaults than SoupSessionAsync and SoupSessionSync: The system TLS/SSL certificate database is used by default to validate https certificates, and sites with invalid certificates will refuse to load with a SOUP_STATUS_SSL_FAILED error. [...] SoupSessionAsync always uses asynchronous I/O, and SoupSessionSync always uses blocking I/O, regardless of the operation. In the new SoupSession, soup_session_queue_message() uses asynchronous I/O (like SoupSessionAsync), and soup_session_send_message() uses blocking I/O (like SoupSessionSync)." This means that using GIO to interact with https files on sites using self-signed certificates will now be rejected with 'SSL handshake failed'
Created attachment 255224 [details] [review] http: Remove GVfsBackendHttp::session_async With newer libsoup, we don't need to separate SoupSession instances, sync/async calls are made depending on the method used, not depending on the type of object used.
Created attachment 255225 [details] [review] http: Don't add resolver to our SoupSession libsoup automatically adds one for us by default when SoupSession is used: "Every session gets a SoupProxyResolverDefault [...] attached to it by default"
Created attachment 255226 [details] [review] http: Don't add SoupContentDecoder to our SoupSession libsoup does this automatically for us: "If you are using a plain SoupSession (ie, not SoupSessionAsync or SoupSessionSync), then a SoupContentDecoder will automatically be added to the session by default." (from SoupContentDecoder API doc)
Created attachment 255227 [details] [review] http: Don't set SoupSession::use-thread-context This property is unused in SoupSession: "In particular, the "async-context" and "use-thread-context" properties are now effectively unused, and the session always queues asynchronous requests in the GMainContext that was is the thread default when the asynchronous operation is started."
Created attachment 255228 [details] [review] http: Remove trailing whitespace
Review of attachment 255223 [details] [review]: Simple patch, but this should get review from danw. ::: daemon/gvfsbackendhttp.c @@ +94,3 @@ + backend->session_async = soup_session_new_with_options ("user-agent", + "gvfs/" VERSION, + NULL); Why create two session objects now? @@ +97,2 @@ /* SoupRequester seems to depend on use-thread-context */ g_object_set (G_OBJECT (backend->session_async), "use-thread-context", TRUE, NULL); Looks like we still need this, but it'd be good to double check.
Review of attachment 255224 [details] [review]: Should be squashed with the earlier patch, I think.
Review of attachment 255225 [details] [review]: OK.
Review of attachment 255226 [details] [review]: Makes sense (could also be squashed with previous).
Review of attachment 255227 [details] [review]: Right.
Review of attachment 255228 [details] [review]: Could just commit stuff like this.
Created attachment 255279 [details] [review] http: Remove use of SoupSessionSync/SoupSessionAsync Since libsoup 2.42.0, SoupSessionSync/SoupSessionAsync are deprecated, and replaced by direct use of SoupSession as described on https://developer.gnome.org/libsoup/stable/libsoup-session-porting.html This commit removes use of SoupSessionSync/SoupSessionAsync and adjusts the code according to the advice in the doc above: - we only need one SoupSession instance as sync/async calls are made depending on the SoupSession method we use, not depending on the instance type - SoupSession already comes with a SoupProxyResolverDefault, we don't need to add it ourselves - SoupSession already comes with a SoupContentDecoder, we don't need to add it ourselves - SoupSession:use-thread-context is now unused and always set to TRUE, so we don't need to change it Because of different defaults between SoupSession and the old sync/async classes, after this change, using GIO to interact with https files on sites using self-signed certificates will now be rejected with 'SSL handshake failed'. This is a better behaviour than silently accepting to interact with such setups.
I've squashed everything together, though rereading the log in this bug, it seems you were not suggesting that ;) I can resplit in 2 or 3 different patches.
Comment on attachment 255279 [details] [review] http: Remove use of SoupSessionSync/SoupSessionAsync yup, looks right to me
Created attachment 255311 [details] [review] http: remove some more historical cruft
Review of attachment 255311 [details] [review]: Looks good too
How exactly are gvfs using apps supposed to opt-out of this if the *want* to trust the self-signed certificate?
Comment on attachment 255311 [details] [review] http: remove some more historical cruft A form of this was applied as: commit 20418890e206f0e06e2e98c010733627a144e2fd Author: Ross Lagerwall <rosslagerwall@gmail.com> Date: Fri Oct 11 10:06:30 2013 +0200 http: Clean up usage of libsoup Use libsoup rather than libsoup-gnome since libsoup-gnome is not required anymore. Bump the libsoup requirement to 2.42 to prevent a build failure with old libsoups (undefined references to soup_session_request_uri). Don't include individual libsoup headers as recommended by the libsoup documentation. https://bugzilla.gnome.org/show_bug.cgi?id=587890
Comment on attachment 255228 [details] [review] http: Remove trailing whitespace Pushed to master with a minor addition as 444a63d09fdaf4db8124801ec6f4ff26ca3c7c0e.
Review of attachment 255279 [details] [review]: An update of this patch was committed as part of bug 732090 but it was changed to ignore SSL failures, so I'll leave this bug open.
*** Bug 741410 has been marked as a duplicate of this bug. ***
(In reply to comment #18) > How exactly are gvfs using apps supposed to opt-out of this if the *want* to > trust the self-signed certificate? Mount operation asking the user like browsers do?
That would work for webdav... I haven't investigated if it would work for http since it's supposed to automount.
Created attachment 294995 [details] [review] don't accept invalid certificates We can ask user if he provides mount operation. So gvfs-cat failed with this patch if certificate can't be verified: $ gvfs-cat https://honk.sigxcpu.org/piki/projects/krb5-auth-dialog/ gvfs-cat: https://honk.sigxcpu.org/piki/projects/krb5-auth-dialog/: error opening file: Automount failed: The signing certificate authority is not known. But user can provide mount operation: $ gvfs-mount https://honk.sigxcpu.org/piki/projects/krb5-auth-dialog/ The site's identity can't be verified: The signing certificate authority is not known. Are you really sure you would like to continue? [1] Yes [2] No Choice: 1
Is there something what this patch could break? I would like to make same fix for webdav (or share it for both somehow). Ross mentioned in some of the duplicate bugs, that this could break personal owncloud setups, but libsoup see system CA certificates, so you can import own certificate to avoid the error by: trust anchor my.crt update-ca-trust
It may break some stuff which blindly tries to open http URLs using gvfs (although as you said it is possible for the user to work around it by importing the certificate). But I think it is still worth doing, even more so for webdav where it needs to be mounted first anyway.
Created attachment 295266 [details] [review] http: don't accept invalid certificates So, there is improved patch which provides api for dav.
Created attachment 295267 [details] [review] dav: don't accept invalid certificates
Ross, could you test the patch for dav please? You have already configured some dav servers, right?
I can take a look this weekend.
Thanks! (I'm just realized that I forget to free the errors, however I will fix it after your review...)
Created attachment 295276 [details] [review] http: don't accept invalid certificates I forgot to amend the patch, so reattaching...
Review of attachment 295276 [details] [review]: While this does work, it unfortunately requires doing an extra HEAD for each new URL that is used. Given the latency of the Internet, this adds a substantial overhead. I think a better approach would be to: * do a HEAD check in do_mount only if is_automount is false and it is an https URL. * Fail try_open_for_read (or try_query_info) if an https error occurs, but only if the user didn't override it in do_mount. I'd imagine the common use case for the http backend is to do something like gvfs-cat http(s)://... and we don't want to make this use case any slower.
Review of attachment 295267 [details] [review]: This works OK, but I think it can be done better :-) Since do_mount already sends an OPTIONS message to the server, instead of adding another call, it would be better to just check for https errors from the OPTIONS message. The user can then be offered the choice whether to proceed after that.
Thanks for reviews! (In reply to comment #34) > Review of attachment 295276 [details] [review]: > > While this does work, it unfortunately requires doing an extra HEAD for each > new URL that is used. Given the latency of the Internet, this adds a > substantial overhead. I don't think it is so substantial, but... (After I sow, how libreoffice use gvfs to read from http (if native support is disabled), one more request is reeeeeaaaaally nothing.) > I think a better approach would be to: > * do a HEAD check in do_mount only if is_automount is false and it is an https > URL. > * Fail try_open_for_read (or try_query_info) if an https error occurs, but only > if the user didn't override it in do_mount. I wanted to implement this exactly you mentioned, but there is one serious problem, because automount is successful, e.g.: $ gvfs-cat https://wrongcert.com Error wrong cert $ gvfs-mount https://wrongcert.com Error already mounted So, gvfs-cat fails, but automount is successful and you can't mount it by hand again to accept certificate, because it is already mounted... (In reply to comment #35) > Review of attachment 295267 [details] [review]: > > This works OK, but I think it can be done better :-) > > Since do_mount already sends an OPTIONS message to the server, instead of > adding another call, it would be better to just check for https errors from the > OPTIONS message. The user can then be offered the choice whether to proceed > after that. I did the patch as is to make the code more readable. As I wrote before, I don't think one more request is so substantial overhead, but you are right in this case it isn't necessary.
(In reply to comment #36) > Thanks for reviews! > > (In reply to comment #34) > > Review of attachment 295276 [details] [review] [details]: > > > > While this does work, it unfortunately requires doing an extra HEAD for each > > new URL that is used. Given the latency of the Internet, this adds a > > substantial overhead. > > I don't think it is so substantial, but... > > (After I sow, how libreoffice use gvfs to read from http (if native support is > disabled), one more request is reeeeeaaaaally nothing.) Well it can make some difference depending on the latency: Before... $ time for i in `seq 10`; do gvfs-cat http://shinjiru.com/?q=$i > /dev/null; done real 0m11.969s user 0m0.013s sys 0m0.020s With this patch... $ time for i in `seq 10`; do gvfs-cat http://shinjiru.com/?q=$i > /dev/null; done real 0m20.405s user 0m0.040s sys 0m0.023s > > > I think a better approach would be to: > > * do a HEAD check in do_mount only if is_automount is false and it is an https > > URL. > > * Fail try_open_for_read (or try_query_info) if an https error occurs, but only > > if the user didn't override it in do_mount. > > I wanted to implement this exactly you mentioned, but there is one serious > problem, because automount is successful, e.g.: > $ gvfs-cat https://wrongcert.com > Error wrong cert > $ gvfs-mount https://wrongcert.com > Error already mounted > > So, gvfs-cat fails, but automount is successful and you can't mount it by hand > again to accept certificate, because it is already mounted... Hmm, if you could unmount it, then it would fix that problem. I've attached a patch which allows find_enclosing_mount to find mounts that are not visible, e.g. automounted mounts. Unmounting then works automatically. We should probably get Alex's input on this, since a GMount is supposed to be a user-visible object.
Created attachment 295599 [details] [review] client: Return user-invisible mounts from find_enclosing_mount This could be useful to allow unmounting automounted mounts.
Created attachment 298184 [details] [review] dav: Verify TLS certificates When mounting a secure webdav share, verify the certificate or ask whether user whether it is OK. ssl-strict is enabled for the SoupSession. If the first connection attempt fails, the certificate is presented to the user. If they agree to the certificate, then it is stored and compared with the certificate presented on each subsequent connection. It is an error if the certificate changes.
After having implemented ftps support, I realized the approach taken in attachment 295267 [details] [review] is not quite sufficient. It only does a single security check at the beginning. For each connection after this, the attacker can do whatever they want and the user will not know. To fix this, either ssl-strict needs to be enabled or the certificate needs to be checked for each connection. I've done this in the above patch. Still not sure what to do about the http backend, but fixing the problem for davs is a good start :-)
Note that this patch should be applied on top of attachment 298040 [details] [review].
Review of attachment 298184 [details] [review]: Thanks, it looks good, just details... ::: daemon/gvfsbackenddav.c @@ +132,3 @@ + + if (dav_backend->certificate) + g_object_unref (dav_backend->certificate); g_clear_object (&dav_backend->certificate) @@ +560,3 @@ + gpointer user_data) +{ + if (G_VFS_BACKEND_DAV (backend)->certificate) Wouldn't be better to check for G_VFS_BACKEND_DAV (backend)->certificate_errors as in other cases... @@ -1855,2 @@ status = g_vfs_backend_dav_send_message (backend, msg_opts); - Unnecessary whitespace change...
Created attachment 299140 [details] [review] dav: Verify TLS certificates When mounting a secure webdav share, verify the certificate or ask whether user whether it is OK. ssl-strict is enabled for the SoupSession. If the first connection attempt fails, the certificate is presented to the user. If they agree to the certificate, then it is stored and compared with the certificate presented on each subsequent connection. It is an error if the certificate changes.
(In reply to Ondrej Holy from comment #42) > Review of attachment 298184 [details] [review] [review]: > > Thanks, it looks good, just details... > > @@ -1855,2 @@ > status = g_vfs_backend_dav_send_message (backend, msg_opts); > - > > Unnecessary whitespace change... This was intentional to group the statements more clearly.
Ok then, I just don't like whitespace changes which are not necessary, because it causes problems when applying patches in future, tracking changes etc.
Review of attachment 299140 [details] [review]: Looks good! I'm not really right person to do corrections of english texts, but "...or ask whether user whether it is OK..." should be probably "...or ask user whether it is OK...".
(In reply to Ondrej Holy from comment #45) > Ok then, I just don't like whitespace changes which are not necessary, > because it causes problems when applying patches in future, tracking changes > etc. Of course, but sometimes it is valid to make a whitespace change if it improves readability.
Comment on attachment 299140 [details] [review] dav: Verify TLS certificates Pushed to master as 5aec716..f5ee590. Thanks for the reviews!
Created attachment 302547 [details] [review] tests: confirm unknown certificates for webdav Two webdav test cases are now broken, because they don't expect the certificate prompt. This patch fixes the broken tests...
Review of attachment 302547 [details] [review]: Looks good!
Comment on attachment 302547 [details] [review] tests: confirm unknown certificates for webdav Thanks for review, pushed to master as: commit 254151072b40ecb83eb8034dac12b5fe5b8896ab
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gvfs/issues/208.