Bug 63810 (CVE-2007-6746) - idle basically doesn't validate SSL/TLS certificates
Summary: idle basically doesn't validate SSL/TLS certificates
Status: RESOLVED FIXED
Alias: CVE-2007-6746
Product: Telepathy
Classification: Unclassified
Component: idle (show other bugs)
Version: git master
Hardware: Other All
: medium major
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2013-04-22 17:46 UTC by Simon McVittie
Modified: 2013-05-09 16:29 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Include config.h in each source file (4.86 KB, patch)
2013-04-22 17:49 UTC, Simon McVittie
Details | Splinter Review
messages/invalid-utf8.py: amend test-case to work under GLib 2.36 (3.08 KB, patch)
2013-04-22 17:50 UTC, Simon McVittie
Details | Splinter Review
IdleServerConnection: check certificates properly, except in the tests (1.97 KB, patch)
2013-04-22 17:53 UTC, Simon McVittie
Details | Splinter Review

Description Simon McVittie 2013-04-22 17:46:04 UTC
telepathy-idle 0.0.4 (the code-drop from Sourceforge) didn't validate TLS certificates:

> /* TODO sometime in the future implement certificate verification */

telepathy-idle 0.1.11 moved it from OpenSSL to GIO TLS, but still didn't validate certificates, in order to make the regression tests work (Bug #37145):

>    """
>    The first one allows self-signed certificates, while the other two are
>    needed to satisfy the certificate used in the test suite. Once
>    Channel.Type.ServerTLSConnection is implemented we will see if we can
>    restore these checks.
>    """
>
> g_socket_client_set_tls_validation_flags(priv->socket_client,
>	G_TLS_CERTIFICATE_VALIDATE_ALL
>	& ~G_TLS_CERTIFICATE_UNKNOWN_CA
>	& ~G_TLS_CERTIFICATE_BAD_IDENTITY
>	& ~G_TLS_CERTIFICATE_EXPIRED);

If the regression tests need to turn off cert validation, this should not be at the expense of being insecure during normal usage.
Comment 1 Simon McVittie 2013-04-22 17:46:35 UTC
(Deliberately not under embargo because this has been fairly obvious in the source code since 2007.)
Comment 2 Simon McVittie 2013-04-22 17:49:42 UTC
Created attachment 78338 [details] [review]
Include config.h in each source file

Among other effects, this makes GLIB_VERSION_MIN_REQUIRED effective.

---

Not related to this bug, but without this patch, I can't compile Idle with its default warnings setup.
Comment 3 Simon McVittie 2013-04-22 17:50:21 UTC
Created attachment 78339 [details] [review]
messages/invalid-utf8.py: amend test-case to work under  GLib 2.36

---

Again, this is unrelated to this bug, but without this patch, Idle fails tests on my development machine.
Comment 4 Simon McVittie 2013-04-22 17:53:57 UTC
Created attachment 78341 [details] [review]
IdleServerConnection: check certificates properly,  except in the tests

---

I deliberately didn't add an ignore-ssl-errors boolean parameter, because there doesn't seem much point, other than regression testing: all public IRC services listen on a non-SSL port, and SSL vs. non-SSL cannot be auto-detected on IRC (there is no STARTTLS support), so we only use SSL if the user has explicitly set use-ssl to TRUE. So, users who would otherwise use ignore-ssl-errors can just turn off use-ssl.

Meanwhile, private SSL-only IRC servers (e.g. a company's internal IRC) can and should have a proper certificate, either from a company CA or from Startcom or something.
Comment 5 Guillaume Desmottes 2013-04-24 13:40:30 UTC
Comment on attachment 78338 [details] [review]
Include config.h in each source file

Review of attachment 78338 [details] [review]:
-----------------------------------------------------------------

++
Comment 6 Guillaume Desmottes 2013-04-24 13:41:29 UTC
Comment on attachment 78339 [details] [review]
messages/invalid-utf8.py: amend test-case to work under  GLib 2.36

Review of attachment 78339 [details] [review]:
-----------------------------------------------------------------

++
Comment 7 Guillaume Desmottes 2013-04-24 13:42:10 UTC
Comment on attachment 78341 [details] [review]
IdleServerConnection: check certificates properly,  except in the tests

Review of attachment 78341 [details] [review]:
-----------------------------------------------------------------

++
Comment 8 Simon McVittie 2013-04-24 17:37:31 UTC
Fixed in 0.1.15.

The minimal patch for distributors of Idle 0.1.11-0.1.14 is something more like this:

diff --git a/src/idle-server-connection.c b/src/idle-server-connection.c
index 5b8629c..8c8eeff 100644
--- a/src/idle-server-connection.c
+++ b/src/idle-server-connection.c
@@ -469,9 +469,4 @@ IdleServerConnectionState idle_server_connection_get_state(IdleServerConnection
 void idle_server_connection_set_tls(IdleServerConnection *conn, gboolean tls) {
 	IdleServerConnectionPrivate *priv = IDLE_SERVER_CONNECTION_GET_PRIVATE(conn);
 	g_socket_client_set_tls(priv->socket_client, tls);
-	g_socket_client_set_tls_validation_flags(priv->socket_client,
-		G_TLS_CERTIFICATE_VALIDATE_ALL
-		& ~G_TLS_CERTIFICATE_UNKNOWN_CA
-		& ~G_TLS_CERTIFICATE_BAD_IDENTITY
-		& ~G_TLS_CERTIFICATE_EXPIRED);
 }

(which breaks the regression tests, but is minimal). That's what I plan to do for Debian 7.

I don't plan to implement certificate verification for 0.1.10 or older. If a distributor wants to do so, they're welcome to share patches via this bug.
Comment 9 Sjoerd Simons 2013-04-28 20:06:42 UTC
(In reply to comment #4)
> Created attachment 78341 [details] [review] [review]
> IdleServerConnection: check certificates properly,  except in the tests
> 
> ---
> 
> I deliberately didn't add an ignore-ssl-errors boolean parameter, because
> there doesn't seem much point, other than regression testing: all public IRC
> services listen on a non-SSL port, and SSL vs. non-SSL cannot be
> auto-detected on IRC (there is no STARTTLS support), so we only use SSL if
> the user has explicitly set use-ssl to TRUE. So, users who would otherwise
> use ignore-ssl-errors can just turn off use-ssl.

This implies you seem to think encrypted, but unverified connections are not valuable. Which is not practically true.

> Meanwhile, private SSL-only IRC servers (e.g. a company's internal IRC) can
> and should have a proper certificate, either from a company CA or from
> Startcom or something.

They indeed should, but if they don't that doesn't mean their users can force that change to happen. Those users, without an option to turn of certificate checking (or until my interactive tls patch are merged) won't be able to use idle for IRC which is somewhat sad.
Comment 10 Manuel Monge 2013-05-09 16:24:32 UTC
I totally agree with Comment #9. Just removing the option leaves users with no option to use telepathy-idle. They will probably use other irc clients that have the unverified connection option available.
Comment 11 Will Thompson 2013-05-09 16:29:03 UTC
(In reply to comment #10)
> I totally agree with Comment #9. Just removing the option leaves users with
> no option to use telepathy-idle. They will probably use other irc clients
> that have the unverified connection option available.

We should probably have updated this bug: 0.1.16, released two days after 0.1.15, implements the Telepathy API for interactive certificate verification, so you can use servers with sketchy certificates again!
Comment 12 Will Thompson 2013-05-09 16:29:55 UTC
Release notes for 0.1.16: http://lists.freedesktop.org/archives/telepathy/2013-May/006434.html


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.